Skip to contentSkip to conversation

staging: r8188eu: Remove _enter/_exit_critical_mutex()

PATCH Superseded Fabio M. De Francesco 3 participants 6 messages Aug 19, 2021 – Aug 28, 2021 Staging

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Fabio M. De Francesco

author
#0 lore raw

PATCH v3 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.

Signed-off-by Fabio M. De Francesco <[email protected]>

---

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 5325fe41fbee..873b54c1715f 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 953fa05dc30c..a93d5cfe4635 100644
@@ -32,7 +32,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;

@@ -96,7 +96,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 1aa65925e1da..e675e4e9550e 100644
@@ -1065,9 +1065,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
maintainer
#1 lore raw
Quoted 11 lines · Fabio M. De Francesco

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.

Signed-off-by Fabio M. De Francesco <[email protected]>

---

You have changed the behavior of the code here, how have you tested that this still works properly?

thanks,

greg k-h

author
#2 lore raw
Quoted 13 lines · CEST Greg Kroah-Hartman
Quoted 11 lines · Fabio M. De Francesco

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.

Signed-off-by Fabio M. De Francesco <[email protected]>

---

Quoted 6 lines

You have changed the behavior of the code here, how have you tested that this still works properly?

thanks,

greg k-h

I forgot to say in this commit message that I've not tested it. I said that in other patches I've submitted during this days (it's because I'm on vacation and I haven't my device with me), I'm sorry that I forgot to say the same also when I submitted this.

I understand that I've changed the behaviour of the code. I did that because the old code had mutex_lock_interruptible() that, if interrupted while waiting for acquiring the mutex, it led to a potential race condition.

In the first version of my patch I wrote:

- _enter_critical_mutex(&pxmitpriv->ack_tx_mutex, NULL);
+ if (mutex_lock_interruptible(&pxmitpriv->ack_tx_mutex))
+ return -EINTR;

I didn't explain why I decided to check and handle a possible -EINTR. Since the original code had no checks of the return value from mutex_lock_interruptible() I thought it could potentially enter the critical section, so I decided to check the return value and exit if I have -EINTR. I guess it shouldn't be allowed to enter the critical section without proper locking.

Then I read another message of yours: "[] (my guess, one almost never needs interruptible locks in a driver)". I interpreted this as: you should change the code to use mutex_lock() (that is uninterruptible) because you don't need to use interruptible locks here. That's why at v3 I changed again the code above and used an uninterruptible mutex_lock(). That's it.

Now let's go back to the question you asked with your last message. I must admit that, although I have been working here in staging for more or less four months, I still have some problems to decode your quite terse style :)

Can you please say what you mean with your question? is it (1) or (2)?

1) The code is _plain_wrong_: go back to your v1, use the interruptible lock as you did there and return -EINTR if interrupted; in the while, also explain why you decided to check for errors and what could happen if you don't test for -EINTR (i.e., explicitly say that you could incur in race conditions if you entered the critical section with no locks acquired).

2) The code _looks_good_, however I want t know if and how you tested it.

Unfortunately I won't be able to test before the first week of September, when I'll be back home. Please, can someone test it for me?

Which of the two above? (If you have no time to answer, can someone else give some hint?).

Thanks very much,

Fabio

P.S.: Am I pushing myself beyond my current knowledge of how the kernel actually works?

maintainer
#3 lore raw
Quoted 51 lines · Fabio M. De Francesco
Quoted 13 lines · CEST Greg Kroah-Hartman
Quoted 11 lines · Fabio M. De Francesco

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.

Signed-off-by Fabio M. De Francesco <[email protected]>

---

Quoted 6 lines

You have changed the behavior of the code here, how have you tested that this still works properly?

thanks,

greg k-h

I forgot to say in this commit message that I've not tested it. I said that in other patches I've submitted during this days (it's because I'm on vacation and I haven't my device with me), I'm sorry that I forgot to say the same also when I submitted this.

I understand that I've changed the behaviour of the code. I did that because the old code had mutex_lock_interruptible() that, if interrupted while waiting for acquiring the mutex, it led to a potential race condition.

In the first version of my patch I wrote:

(quoted diff) +2 -1
-       _enter_critical_mutex(&pxmitpriv->ack_tx_mutex, NULL);
+       if (mutex_lock_interruptible(&pxmitpriv->ack_tx_mutex))
+               return -EINTR;

I didn't explain why I decided to check and handle a possible -EINTR. Since the original code had no checks of the return value from mutex_lock_interruptible() I thought it could potentially enter the critical section, so I decided to check the return value and exit if I have -EINTR. I guess it shouldn't be allowed to enter the critical section without proper locking.

Then I read another message of yours: "[] (my guess, one almost never needs interruptible locks in a driver)". I interpreted this as: you should change the code to use mutex_lock() (that is uninterruptible) because you don't need to use interruptible locks here. That's why at v3 I changed again the code above and used an uninterruptible mutex_lock(). That's it.

Now let's go back to the question you asked with your last message. I must admit that, although I have been working here in staging for more or less four months, I still have some problems to decode your quite terse style :)

Quoted 8 lines

Can you please say what you mean with your question? is it (1) or (2)?

1) The code is _plain_wrong_: go back to your v1, use the interruptible lock as you did there and return -EINTR if interrupted; in the while, also explain why you decided to check for errors and what could happen if you don't test for -EINTR (i.e., explicitly say that you could incur in race conditions if you entered the critical section with no locks acquired).

2) The code _looks_good_, however I want t know if and how you tested it.

This one, it looks right, but I want to make sure how you tested it and verified that it still works properly.

No rush, resend when you are back and have a chance to do so.

thanks,

greg k-h

Tested-by Reviewed-by on v3 1/1
#4 lore raw
In reply to Fabio M. De Francesco
Quoted 10 lines · Fabio M. De Francesco

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.

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

Thanks!

With regards,
Pavel Skripkin

author
#5 lore raw
Quoted 12 lines · CEST Pavel Skripkin
Quoted 10 lines · Fabio M. De Francesco

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.

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

Thanks!

With regards,
Pavel Skripkin

Hi Pavel,

Thanks very much for testing and reviewing my patch. As you know, Greg wanted it tested but I couldn't do that. I'll add your tags and send a v4 and I guess that now Greg will easily apply.

Great help from you, thanks again :)

Regards,

Fabio