Skip to contentSkip to conversation

staging: r8188eu: Remove _enter/_exit_critical_mutex()

PATCH Superseded Fabio M. De Francesco 2 participants 3 messages Aug 28, 2021 Staging

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Fabio M. De Francesco

author
#0 lore raw

PATCH v4 1/1 staging: r8188eu: Remove _enter/_exit_critical_mutex()

Remove _enter_critical_mutex() and _exit_critical_mutex(). They are unnecessary wrappers, respectively to mutex_lock_interruptible() and to mutex_unlock(). They also have an odd interface that takes an unused argument named pirqL of type unsigned long.
The original code enters the critical section if the mutex API is interrupted while waiting to acquire the lock; therefore it could lead to a race condition. Use mutex_lock() because it is uninterruptible and so avoid that above-mentioned potential race condition.

Tested-by Pavel Skripkin <[email protected]>
Reviewed-by Pavel Skripkin <[email protected]>
Signed-off-by Fabio M. De Francesco <[email protected]>

---

v4: Tested and reviewed by Pavel Skripkin. No changes to the code.

v3: Assume that the original authors don't expect that mutex_lock_interruptible() can be really interrupted and then lead to a potential race condition. Furthermore, Greg Kroah-Hartman makes me notice that "[] one almost never needs interruptable locks in a driver". Therefore, replace the calls to mutex_lock_interruptible() with calls to mutex_lock() since the latter is uninterruptible and avoid race conditions without the necessity to handle -EINTR errors.

v2: Ignore return values from Mutexes API because the original authors don't check and manage return errors from mutex_lock_interruptible().

 drivers/staging/r8188eu/core/rtw_mlme_ext.c     |  4 ++--
 drivers/staging/r8188eu/hal/usb_ops_linux.c     |  4 ++--
 drivers/staging/r8188eu/include/osdep_service.h | 13 -------------
 drivers/staging/r8188eu/os_dep/os_intfs.c       |  4 ++--
 4 files changed, 6 insertions(+), 19 deletions(-)
drivers/staging/r8188eu/core/rtw_mlme_ext.c +2 -2
index 5a472a4954b0..3df02bd0db89 100644
@@ -4359,7 +4359,7 @@ s32 dump_mgntframe_and_wait_ack(struct adapter *padapter, struct xmit_frame *pmg
 	if (padapter->bSurpriseRemoved || padapter->bDriverStopped)
 		return -1;
 
-	_enter_critical_mutex(&pxmitpriv->ack_tx_mutex, NULL);
+	mutex_lock(&pxmitpriv->ack_tx_mutex);
 	pxmitpriv->ack_tx = true;
 
 	pmgntframe->ack_report = 1;

@@ -4368,7 +4368,7 @@ s32 dump_mgntframe_and_wait_ack(struct adapter *padapter, struct xmit_frame *pmg
 	}
 
 	pxmitpriv->ack_tx = false;
-	_exit_critical_mutex(&pxmitpriv->ack_tx_mutex, NULL);
+	mutex_unlock(&pxmitpriv->ack_tx_mutex);
 
 	return ret;
 }
drivers/staging/r8188eu/hal/usb_ops_linux.c +2 -2
index 0cf69033c529..065b0d8e030a 100644
@@ -29,7 +29,7 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
 		goto exit;
 	}
 
-	_enter_critical_mutex(&dvobjpriv->usb_vendor_req_mutex, NULL);
+	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
 
 	/*  Acquire IO memory for vendorreq */
 	pIo_buf = dvobjpriv->usb_vendor_req_buf;

@@ -92,7 +92,7 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
 			break;
 	}
 release_mutex:
-	_exit_critical_mutex(&dvobjpriv->usb_vendor_req_mutex, NULL);
+	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
 exit:
 	return status;
 }
drivers/staging/r8188eu/include/osdep_service.h +0 -13
index 029aa4e92c9b..bb92b9d74bd7 100644
@@ -56,19 +56,6 @@ static inline struct list_head *get_list_head(struct __queue *queue)
 	return (&(queue->queue));
 }
 
-static inline int _enter_critical_mutex(struct mutex *pmutex, unsigned long *pirqL)
-{
-	int ret;
-
-	ret = mutex_lock_interruptible(pmutex);
-	return ret;
-}
-
-static inline void _exit_critical_mutex(struct mutex *pmutex, unsigned long *pirqL)
-{
-		mutex_unlock(pmutex);
-}
-
 static inline void rtw_list_delete(struct list_head *plist)
 {
 	list_del_init(plist);
drivers/staging/r8188eu/os_dep/os_intfs.c +2 -2
index 8d0158f4a45d..6821dcdf1523 100644
@@ -1062,9 +1062,9 @@ int netdev_open(struct net_device *pnetdev)
 	int ret;
 	struct adapter *padapter = (struct adapter *)rtw_netdev_priv(pnetdev);
 
-	_enter_critical_mutex(padapter->hw_init_mutex, NULL);
+	mutex_lock(padapter->hw_init_mutex);
 	ret = _netdev_open(pnetdev);
-	_exit_critical_mutex(padapter->hw_init_mutex, NULL);
+	mutex_unlock(padapter->hw_init_mutex);
 	return ret;
 }
Signature · 1 line
2.32.0
#1 lore raw

Unrelated to the patch, but if this was a typo then you can easily modify the subject prefix for new versions with -v4 using git format-patch

-Aakash

author
#2 lore raw
Quoted 5 lines · CEST Aakash Hemadri

Unrelated to the patch, but if this was a typo then you can easily modify the subject prefix for new versions with -v4 using git format-patch

-Aakash

Thanks for catching that typo. I use Vim and I should have pressed the 'i' twice to go in insert mode. I prefer to not use git send-mail to add version numbers. I'll resend it soon.

Regards,

Fabio