linux-staging.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH] staging: r8188eu: Use a Mutex instead of a binary Semaphore
@ 2021-10-22 17:19 Fabio M. De Francesco
  2021-10-22 17:52 ` Larry Finger
  0 siblings, 1 reply; 3+ messages in thread
From: Fabio M. De Francesco @ 2021-10-22 17:19 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, linux-staging,
	linux-kernel
  Cc: Fabio M. De Francesco

Use a Mutex instead of a binary Semaphore for the purpose of enforcing
mutual exclusive access to the "pwrctrl_priv" structure.

Mutexes are sleeping locks similar to Semaphores with a 'count' of one
(like binary Semaphores), however they have a simpler interface, more
efficient performance, and additional constraints.

There is no change in the logic of the new code; however it is more
simple because it gets rid of four unnecessary wrappers:
_init_pwrlock(), _enter_pwrlock(),_exit_pwrlock(), _rtw_down_sema().

Actually, there is a change in the state in which the code waits for
acquiring locks, because it makes it in an uninterruptible state
(instead the old code used down_interruptibe()). Interruptible
waits are neither required nor wanted in this driver.

Tested with ASUSTek Computer, Inc. Realtek 8188EUS [USB-N10 Nano].

Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_pwrctrl.c      | 10 +++++-----
 drivers/staging/r8188eu/include/osdep_service.h |  2 --
 drivers/staging/r8188eu/include/rtw_pwrctrl.h   | 17 +----------------
 drivers/staging/r8188eu/os_dep/osdep_service.c  |  8 --------
 drivers/staging/r8188eu/os_dep/usb_intf.c       |  8 ++++----
 5 files changed, 10 insertions(+), 35 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_pwrctrl.c b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
index 19cac5814ea4..5d595cf2a47e 100644
--- a/drivers/staging/r8188eu/core/rtw_pwrctrl.c
+++ b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
@@ -21,7 +21,7 @@ void ips_enter(struct adapter *padapter)
 		return;
 	}
 
-	_enter_pwrlock(&pwrpriv->lock);
+	mutex_lock(&pwrpriv->lock);
 
 	pwrpriv->bips_processing = true;
 
@@ -42,7 +42,7 @@ void ips_enter(struct adapter *padapter)
 	}
 	pwrpriv->bips_processing = false;
 
-	_exit_pwrlock(&pwrpriv->lock);
+	mutex_unlock(&pwrpriv->lock);
 }
 
 int ips_leave(struct adapter *padapter)
@@ -53,7 +53,7 @@ int ips_leave(struct adapter *padapter)
 	int result = _SUCCESS;
 	int keyid;
 
-	_enter_pwrlock(&pwrpriv->lock);
+	mutex_lock(&pwrpriv->lock);
 
 	if ((pwrpriv->rf_pwrstate == rf_off) && (!pwrpriv->bips_processing)) {
 		pwrpriv->bips_processing = true;
@@ -87,7 +87,7 @@ int ips_leave(struct adapter *padapter)
 		pwrpriv->bpower_saving = false;
 	}
 
-	_exit_pwrlock(&pwrpriv->lock);
+	mutex_unlock(&pwrpriv->lock);
 
 	return result;
 }
@@ -337,7 +337,7 @@ void rtw_init_pwrctrl_priv(struct adapter *padapter)
 {
 	struct pwrctrl_priv *pwrctrlpriv = &padapter->pwrctrlpriv;
 
-	_init_pwrlock(&pwrctrlpriv->lock);
+	mutex_init(&pwrctrlpriv->lock);
 	pwrctrlpriv->rf_pwrstate = rf_on;
 	pwrctrlpriv->ips_enter_cnts = 0;
 	pwrctrlpriv->ips_leave_cnts = 0;
diff --git a/drivers/staging/r8188eu/include/osdep_service.h b/drivers/staging/r8188eu/include/osdep_service.h
index 886a1b6f30b4..efab3a97eb46 100644
--- a/drivers/staging/r8188eu/include/osdep_service.h
+++ b/drivers/staging/r8188eu/include/osdep_service.h
@@ -141,8 +141,6 @@ extern unsigned char RSN_TKIP_CIPHER[4];
 
 void *rtw_malloc2d(int h, int w, int size);
 
-u32  _rtw_down_sema(struct semaphore *sema);
-
 #define rtw_init_queue(q)					\
 	do {							\
 		INIT_LIST_HEAD(&((q)->queue));			\
diff --git a/drivers/staging/r8188eu/include/rtw_pwrctrl.h b/drivers/staging/r8188eu/include/rtw_pwrctrl.h
index 04236e42fbf9..b19ef796ab54 100644
--- a/drivers/staging/r8188eu/include/rtw_pwrctrl.h
+++ b/drivers/staging/r8188eu/include/rtw_pwrctrl.h
@@ -27,21 +27,6 @@ enum power_mgnt {
 	PS_MODE_NUM
 };
 
-static inline void _init_pwrlock(struct semaphore  *plock)
-{
-	sema_init(plock, 1);
-}
-
-static inline void _enter_pwrlock(struct semaphore  *plock)
-{
-	_rtw_down_sema(plock);
-}
-
-static inline void _exit_pwrlock(struct semaphore  *plock)
-{
-	up(plock);
-}
-
 #define LPS_DELAY_TIME	1*HZ /*  1 sec */
 
 /*  RF state. */
@@ -60,7 +45,7 @@ enum { /*  for ips_mode */
 };
 
 struct pwrctrl_priv {
-	struct semaphore lock;
+	struct mutex lock; /* Mutex used to protect struct pwrctrl_priv */
 
 	u8	pwr_mode;
 	u8	smart_ps;
diff --git a/drivers/staging/r8188eu/os_dep/osdep_service.c b/drivers/staging/r8188eu/os_dep/osdep_service.c
index 6bee194fc35d..59bdd0abea7e 100644
--- a/drivers/staging/r8188eu/os_dep/osdep_service.c
+++ b/drivers/staging/r8188eu/os_dep/osdep_service.c
@@ -42,14 +42,6 @@ Otherwise, there will be racing condition.
 Caller must check if the list is empty before calling rtw_list_delete
 */
 
-u32 _rtw_down_sema(struct semaphore *sema)
-{
-	if (down_interruptible(sema))
-		return _FAIL;
-	else
-		return _SUCCESS;
-}
-
 inline u32 rtw_systime_to_ms(u32 systime)
 {
 	return systime * 1000 / HZ;
diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
index 7ed9f5f54472..fc74c93272a8 100644
--- a/drivers/staging/r8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
@@ -233,7 +233,7 @@ static int rtw_suspend(struct usb_interface *pusb_intf, pm_message_t message)
 	rtw_cancel_all_timer(padapter);
 	LeaveAllPowerSaveMode(padapter);
 
-	_enter_pwrlock(&pwrpriv->lock);
+	mutex_lock(&pwrpriv->lock);
 	/* s1. */
 	if (pnetdev) {
 		netif_carrier_off(pnetdev);
@@ -262,7 +262,7 @@ static int rtw_suspend(struct usb_interface *pusb_intf, pm_message_t message)
 	rtw_free_network_queue(padapter, true);
 
 	rtw_dev_unload(padapter);
-	_exit_pwrlock(&pwrpriv->lock);
+	mutex_unlock(&pwrpriv->lock);
 
 	if (check_fwstate(pmlmepriv, _FW_UNDER_SURVEY))
 		rtw_indicate_scan_done(padapter, 1);
@@ -291,7 +291,7 @@ static int rtw_resume(struct usb_interface *pusb_intf)
 	pnetdev = padapter->pnetdev;
 	pwrpriv = &padapter->pwrctrlpriv;
 
-	_enter_pwrlock(&pwrpriv->lock);
+	mutex_lock(&pwrpriv->lock);
 	rtw_reset_drv_sw(padapter);
 	if (pwrpriv)
 		pwrpriv->bkeepfwalive = false;
@@ -303,7 +303,7 @@ static int rtw_resume(struct usb_interface *pusb_intf)
 	netif_device_attach(pnetdev);
 	netif_carrier_on(pnetdev);
 
-	_exit_pwrlock(&pwrpriv->lock);
+	mutex_unlock(&pwrpriv->lock);
 
 	if (padapter->pid[1] != 0) {
 		DBG_88E("pid[1]:%d\n", padapter->pid[1]);
-- 
2.33.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] staging: r8188eu: Use a Mutex instead of a binary Semaphore
  2021-10-22 17:19 [PATCH] staging: r8188eu: Use a Mutex instead of a binary Semaphore Fabio M. De Francesco
@ 2021-10-22 17:52 ` Larry Finger
  2021-10-23 14:02   ` Fabio M. De Francesco
  0 siblings, 1 reply; 3+ messages in thread
From: Larry Finger @ 2021-10-22 17:52 UTC (permalink / raw)
  To: Fabio M. De Francesco, Phillip Potter, Greg Kroah-Hartman,
	linux-staging, linux-kernel

On 10/22/21 12:19, Fabio M. De Francesco wrote:
> Use a Mutex instead of a binary Semaphore for the purpose of enforcing
> mutual exclusive access to the "pwrctrl_priv" structure.
> 
> Mutexes are sleeping locks similar to Semaphores with a 'count' of one
> (like binary Semaphores), however they have a simpler interface, more
> efficient performance, and additional constraints.
> 
> There is no change in the logic of the new code; however it is more
> simple because it gets rid of four unnecessary wrappers:
> _init_pwrlock(), _enter_pwrlock(),_exit_pwrlock(), _rtw_down_sema().
> 
> Actually, there is a change in the state in which the code waits for
> acquiring locks, because it makes it in an uninterruptible state
> (instead the old code used down_interruptibe()). Interruptible
> waits are neither required nor wanted in this driver.
> 
> Tested with ASUSTek Computer, Inc. Realtek 8188EUS [USB-N10 Nano].
> 
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Well done.

Acked-by: Larry Finger <Larry.Finger@lwfinger.net>

> ---
>   drivers/staging/r8188eu/core/rtw_pwrctrl.c      | 10 +++++-----
>   drivers/staging/r8188eu/include/osdep_service.h |  2 --
>   drivers/staging/r8188eu/include/rtw_pwrctrl.h   | 17 +----------------
>   drivers/staging/r8188eu/os_dep/osdep_service.c  |  8 --------
>   drivers/staging/r8188eu/os_dep/usb_intf.c       |  8 ++++----
>   5 files changed, 10 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_pwrctrl.c b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
> index 19cac5814ea4..5d595cf2a47e 100644
> --- a/drivers/staging/r8188eu/core/rtw_pwrctrl.c
> +++ b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
> @@ -21,7 +21,7 @@ void ips_enter(struct adapter *padapter)
>   		return;
>   	}
>   
> -	_enter_pwrlock(&pwrpriv->lock);
> +	mutex_lock(&pwrpriv->lock);
>   
>   	pwrpriv->bips_processing = true;
>   
> @@ -42,7 +42,7 @@ void ips_enter(struct adapter *padapter)
>   	}
>   	pwrpriv->bips_processing = false;
>   
> -	_exit_pwrlock(&pwrpriv->lock);
> +	mutex_unlock(&pwrpriv->lock);
>   }
>   
>   int ips_leave(struct adapter *padapter)
> @@ -53,7 +53,7 @@ int ips_leave(struct adapter *padapter)
>   	int result = _SUCCESS;
>   	int keyid;
>   
> -	_enter_pwrlock(&pwrpriv->lock);
> +	mutex_lock(&pwrpriv->lock);
>   
>   	if ((pwrpriv->rf_pwrstate == rf_off) && (!pwrpriv->bips_processing)) {
>   		pwrpriv->bips_processing = true;
> @@ -87,7 +87,7 @@ int ips_leave(struct adapter *padapter)
>   		pwrpriv->bpower_saving = false;
>   	}
>   
> -	_exit_pwrlock(&pwrpriv->lock);
> +	mutex_unlock(&pwrpriv->lock);
>   
>   	return result;
>   }
> @@ -337,7 +337,7 @@ void rtw_init_pwrctrl_priv(struct adapter *padapter)
>   {
>   	struct pwrctrl_priv *pwrctrlpriv = &padapter->pwrctrlpriv;
>   
> -	_init_pwrlock(&pwrctrlpriv->lock);
> +	mutex_init(&pwrctrlpriv->lock);
>   	pwrctrlpriv->rf_pwrstate = rf_on;
>   	pwrctrlpriv->ips_enter_cnts = 0;
>   	pwrctrlpriv->ips_leave_cnts = 0;
> diff --git a/drivers/staging/r8188eu/include/osdep_service.h b/drivers/staging/r8188eu/include/osdep_service.h
> index 886a1b6f30b4..efab3a97eb46 100644
> --- a/drivers/staging/r8188eu/include/osdep_service.h
> +++ b/drivers/staging/r8188eu/include/osdep_service.h
> @@ -141,8 +141,6 @@ extern unsigned char RSN_TKIP_CIPHER[4];
>   
>   void *rtw_malloc2d(int h, int w, int size);
>   
> -u32  _rtw_down_sema(struct semaphore *sema);
> -
>   #define rtw_init_queue(q)					\
>   	do {							\
>   		INIT_LIST_HEAD(&((q)->queue));			\
> diff --git a/drivers/staging/r8188eu/include/rtw_pwrctrl.h b/drivers/staging/r8188eu/include/rtw_pwrctrl.h
> index 04236e42fbf9..b19ef796ab54 100644
> --- a/drivers/staging/r8188eu/include/rtw_pwrctrl.h
> +++ b/drivers/staging/r8188eu/include/rtw_pwrctrl.h
> @@ -27,21 +27,6 @@ enum power_mgnt {
>   	PS_MODE_NUM
>   };
>   
> -static inline void _init_pwrlock(struct semaphore  *plock)
> -{
> -	sema_init(plock, 1);
> -}
> -
> -static inline void _enter_pwrlock(struct semaphore  *plock)
> -{
> -	_rtw_down_sema(plock);
> -}
> -
> -static inline void _exit_pwrlock(struct semaphore  *plock)
> -{
> -	up(plock);
> -}
> -
>   #define LPS_DELAY_TIME	1*HZ /*  1 sec */
>   
>   /*  RF state. */
> @@ -60,7 +45,7 @@ enum { /*  for ips_mode */
>   };
>   
>   struct pwrctrl_priv {
> -	struct semaphore lock;
> +	struct mutex lock; /* Mutex used to protect struct pwrctrl_priv */
>   
>   	u8	pwr_mode;
>   	u8	smart_ps;
> diff --git a/drivers/staging/r8188eu/os_dep/osdep_service.c b/drivers/staging/r8188eu/os_dep/osdep_service.c
> index 6bee194fc35d..59bdd0abea7e 100644
> --- a/drivers/staging/r8188eu/os_dep/osdep_service.c
> +++ b/drivers/staging/r8188eu/os_dep/osdep_service.c
> @@ -42,14 +42,6 @@ Otherwise, there will be racing condition.
>   Caller must check if the list is empty before calling rtw_list_delete
>   */
>   
> -u32 _rtw_down_sema(struct semaphore *sema)
> -{
> -	if (down_interruptible(sema))
> -		return _FAIL;
> -	else
> -		return _SUCCESS;
> -}
> -
>   inline u32 rtw_systime_to_ms(u32 systime)
>   {
>   	return systime * 1000 / HZ;
> diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
> index 7ed9f5f54472..fc74c93272a8 100644
> --- a/drivers/staging/r8188eu/os_dep/usb_intf.c
> +++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
> @@ -233,7 +233,7 @@ static int rtw_suspend(struct usb_interface *pusb_intf, pm_message_t message)
>   	rtw_cancel_all_timer(padapter);
>   	LeaveAllPowerSaveMode(padapter);
>   
> -	_enter_pwrlock(&pwrpriv->lock);
> +	mutex_lock(&pwrpriv->lock);
>   	/* s1. */
>   	if (pnetdev) {
>   		netif_carrier_off(pnetdev);
> @@ -262,7 +262,7 @@ static int rtw_suspend(struct usb_interface *pusb_intf, pm_message_t message)
>   	rtw_free_network_queue(padapter, true);
>   
>   	rtw_dev_unload(padapter);
> -	_exit_pwrlock(&pwrpriv->lock);
> +	mutex_unlock(&pwrpriv->lock);
>   
>   	if (check_fwstate(pmlmepriv, _FW_UNDER_SURVEY))
>   		rtw_indicate_scan_done(padapter, 1);
> @@ -291,7 +291,7 @@ static int rtw_resume(struct usb_interface *pusb_intf)
>   	pnetdev = padapter->pnetdev;
>   	pwrpriv = &padapter->pwrctrlpriv;
>   
> -	_enter_pwrlock(&pwrpriv->lock);
> +	mutex_lock(&pwrpriv->lock);
>   	rtw_reset_drv_sw(padapter);
>   	if (pwrpriv)
>   		pwrpriv->bkeepfwalive = false;
> @@ -303,7 +303,7 @@ static int rtw_resume(struct usb_interface *pusb_intf)
>   	netif_device_attach(pnetdev);
>   	netif_carrier_on(pnetdev);
>   
> -	_exit_pwrlock(&pwrpriv->lock);
> +	mutex_unlock(&pwrpriv->lock);
>   
>   	if (padapter->pid[1] != 0) {
>   		DBG_88E("pid[1]:%d\n", padapter->pid[1]);
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] staging: r8188eu: Use a Mutex instead of a binary Semaphore
  2021-10-22 17:52 ` Larry Finger
@ 2021-10-23 14:02   ` Fabio M. De Francesco
  0 siblings, 0 replies; 3+ messages in thread
From: Fabio M. De Francesco @ 2021-10-23 14:02 UTC (permalink / raw)
  To: Phillip Potter, Greg Kroah-Hartman, linux-staging, linux-kernel,
	Larry Finger

On Friday, October 22, 2021 7:52:33 PM CEST Larry Finger wrote:
> On 10/22/21 12:19, Fabio M. De Francesco wrote:
> > Use a Mutex instead of a binary Semaphore for the purpose of enforcing
> > mutual exclusive access to the "pwrctrl_priv" structure.
> > 
> > Mutexes are sleeping locks similar to Semaphores with a 'count' of one
> > (like binary Semaphores), however they have a simpler interface, more
> > efficient performance, and additional constraints.
> > 
> > There is no change in the logic of the new code; however it is more
> > simple because it gets rid of four unnecessary wrappers:
> > _init_pwrlock(), _enter_pwrlock(),_exit_pwrlock(), _rtw_down_sema().
> > 
> > Actually, there is a change in the state in which the code waits for
> > acquiring locks, because it makes it in an uninterruptible state
> > (instead the old code used down_interruptible()). Interruptible
> > waits are neither required nor wanted in this driver.
> > 
> > Tested with ASUSTek Computer, Inc. Realtek 8188EUS [USB-N10 Nano].
> > 
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> 
> Well done.
> 
> Acked-by: Larry Finger <Larry.Finger@lwfinger.net>
> 
Hi Larry, 

Thank you very much for giving your "Acked-by" tag, and, above all, for the 
"Well done".

Best regards,

Fabio




^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-10-23 14:02 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-22 17:19 [PATCH] staging: r8188eu: Use a Mutex instead of a binary Semaphore Fabio M. De Francesco
2021-10-22 17:52 ` Larry Finger
2021-10-23 14:02   ` Fabio M. De Francesco

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).