[PATCH] staging: r8188eu: Remove _enter/_exit_critical_mutex()

[PATCH] staging: r8188eu: Remove _enter/_exit_critical_mutex()

[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
second parameter "unsigned long *pirqL".

Use directly the in-kernel API; check and manage the return value of
mutex_lock_interruptible().

Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_mlme_ext.c     |  5 +++--
 drivers/staging/r8188eu/hal/usb_ops_linux.c     |  7 +++++--
 drivers/staging/r8188eu/include/osdep_service.h | 13 -------------
 drivers/staging/r8188eu/os_dep/os_intfs.c       |  5 +++--
 4 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
index f6ee72d5af09..484083468ebb 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
@@ -4358,7 +4358,8 @@ 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);
+	if (mutex_lock_interruptible(&pxmitpriv->ack_tx_mutex))
+		return -EINTR;
 	pxmitpriv->ack_tx = true;
 
 	pmgntframe->ack_report = 1;
@@ -4367,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;
 }
diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 953fa05dc30c..52cb32f898e0 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -32,7 +32,10 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
 		goto exit;
 	}
 
-	_enter_critical_mutex(&dvobjpriv->usb_vendor_req_mutex, NULL);
+	if (mutex_lock_interruptible(&dvobjpriv->usb_vendor_req_mutex)) {
+		status = -EINTR;
+		goto exit;
+	}
 
 	/*  Acquire IO memory for vendorreq */
 	pIo_buf = dvobjpriv->usb_vendor_req_buf;
@@ -96,7 +99,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;
 }
diff --git a/drivers/staging/r8188eu/include/osdep_service.h b/drivers/staging/r8188eu/include/osdep_service.h
index 029aa4e92c9b..bb92b9d74bd7 100644
--- a/drivers/staging/r8188eu/include/osdep_service.h
+++ b/drivers/staging/r8188eu/include/osdep_service.h
@@ -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);
diff --git a/drivers/staging/r8188eu/os_dep/os_intfs.c b/drivers/staging/r8188eu/os_dep/os_intfs.c
index 1aa65925e1da..4768e3a507f6 100644
--- a/drivers/staging/r8188eu/os_dep/os_intfs.c
+++ b/drivers/staging/r8188eu/os_dep/os_intfs.c
@@ -1065,9 +1065,10 @@ 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);
+	if (mutex_lock_interruptible(padapter->hw_init_mutex))
+		return -EINTR;
 	ret = _netdev_open(pnetdev);
-	_exit_critical_mutex(padapter->hw_init_mutex, NULL);
+	mutex_unlock(padapter->hw_init_mutex);
 	return ret;
 }
 
-- 
2.32.0

Re: [PATCH] staging: r8188eu: Remove _enter/_exit_critical_mutex()

[Greg Kroah-Hartman]

On Thu, Aug 19, 2021 at 08:08:37AM +0200, Fabio M. De Francesco wrote:
> 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
> second parameter "unsigned long *pirqL".
> 
> Use directly the in-kernel API; check and manage the return value of
> mutex_lock_interruptible().
> 
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  drivers/staging/r8188eu/core/rtw_mlme_ext.c     |  5 +++--
>  drivers/staging/r8188eu/hal/usb_ops_linux.c     |  7 +++++--
>  drivers/staging/r8188eu/include/osdep_service.h | 13 -------------
>  drivers/staging/r8188eu/os_dep/os_intfs.c       |  5 +++--
>  4 files changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> index f6ee72d5af09..484083468ebb 100644
> --- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> +++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> @@ -4358,7 +4358,8 @@ 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);
> +	if (mutex_lock_interruptible(&pxmitpriv->ack_tx_mutex))
> +		return -EINTR;

But the code never would return this value if the lock function returned
an error.  Why do that here now?

thanks,

greg k-h

Re: [PATCH] staging: r8188eu: Remove _enter/_exit_critical_mutex()

[Fabio M. De Francesco]

On Thursday, August 19, 2021 8:30:21 AM CEST Greg Kroah-Hartman wrote:
> On Thu, Aug 19, 2021 at 08:08:37AM +0200, Fabio M. De Francesco wrote:
> > 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
> > second parameter "unsigned long *pirqL".
> > 
> > Use directly the in-kernel API; check and manage the return value of
> > mutex_lock_interruptible().
> > 
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> >  drivers/staging/r8188eu/core/rtw_mlme_ext.c     |  5 +++--
> >  drivers/staging/r8188eu/hal/usb_ops_linux.c     |  7 +++++--
> >  drivers/staging/r8188eu/include/osdep_service.h | 13 -------------
> >  drivers/staging/r8188eu/os_dep/os_intfs.c       |  5 +++--
> >  4 files changed, 11 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> > index f6ee72d5af09..484083468ebb 100644
> > --- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> > +++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> > @@ -4358,7 +4358,8 @@ 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);
> > +	if (mutex_lock_interruptible(&pxmitpriv->ack_tx_mutex))
> > +		return -EINTR;
> 
> But the code never would return this value if the lock function returned
> an error.  Why do that here now?

I read from the documentation that "[mutex_lock_interruptible()] Return: 0 if 
the lock was successfully acquired or -EINTR if a signal arrived.". 

After reading that, I thought that if I got -EINTR I should return it. Shouldn't I?

Now I've just checked its usage pattern in another file where we have exactly 12
times the same management of the error (the example I'm talking about is in
drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c):

"if (mutex_lock_interruptible(&instance->vchiq_mutex))
                return -EINTR;".

Unless you mean that I should return the "ret" variable, which is already set to 
"_FAIL", I am really confused. Please, can you further elaborate what I'm doing 
wrong?

Thanks,

Fabio



 
  
> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH] staging: r8188eu: Remove _enter/_exit_critical_mutex()

[Fabio M. De Francesco]

On Thursday, August 19, 2021 9:07:20 AM CEST Fabio M. De Francesco wrote:
> On Thursday, August 19, 2021 8:30:21 AM CEST Greg Kroah-Hartman wrote:
> > On Thu, Aug 19, 2021 at 08:08:37AM +0200, Fabio M. De Francesco wrote:
> > > 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
> > > second parameter "unsigned long *pirqL".
> > > 
> > > Use directly the in-kernel API; check and manage the return value of
> > > mutex_lock_interruptible().
> > > 
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > ---
> > >  drivers/staging/r8188eu/core/rtw_mlme_ext.c     |  5 +++--
> > >  drivers/staging/r8188eu/hal/usb_ops_linux.c     |  7 +++++--
> > >  drivers/staging/r8188eu/include/osdep_service.h | 13 -------------
> > >  drivers/staging/r8188eu/os_dep/os_intfs.c       |  5 +++--
> > >  4 files changed, 11 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> > > index f6ee72d5af09..484083468ebb 100644
> > > --- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> > > +++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> > > @@ -4358,7 +4358,8 @@ 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);
> > > +	if (mutex_lock_interruptible(&pxmitpriv->ack_tx_mutex))
> > > +		return -EINTR;
> > 
> > But the code never would return this value if the lock function returned
> > an error.  Why do that here now?

Ah, now I think I understand what you asked me ... sorry for not having 
immediately grasped the meaning of your objection. :(

I guess you wanted to know why I decided to check and handle the 
return values of mutex_lock_interruptible (), as the original code didn't. 
Did I understand the correct meaning of your question?

If so, now I can explain why I did it ...

A few months ago I did the conversion of the visorhba (Unisys) driver from 
IDR to XArray. Since the old code did not check IDR API return values, I had 
decided not to check for XArray API return values as well.

Then Dan C. asked me to implement the checks that were missing in the 
original code. So, today I decided to implement them before I was asked 
to do it. Now it's clear that in this case they are not needed.

That's all. :-)

I'm about to send a v2 without those unnecessary checks.

Thanks,

Fabio

> I read from the documentation that "[mutex_lock_interruptible()] Return: 0 if 
> the lock was successfully acquired or -EINTR if a signal arrived.". 
> 
> After reading that, I thought that if I got -EINTR I should return it. Shouldn't I?
> 
> Now I've just checked its usage pattern in another file where we have exactly 12
> times the same management of the error (the example I'm talking about is in
> drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c):
> 
> "if (mutex_lock_interruptible(&instance->vchiq_mutex))
>                 return -EINTR;".
> 
> Unless you mean that I should return the "ret" variable, which is already set to 
> "_FAIL", I am really confused. Please, can you further elaborate what I'm doing 
> wrong?
> 
> Thanks,
> 
> Fabio

Re: [PATCH] staging: r8188eu: Remove _enter/_exit_critical_mutex()

[Greg Kroah-Hartman]

On Thu, Aug 19, 2021 at 12:16:03PM +0200, Fabio M. De Francesco wrote:
> On Thursday, August 19, 2021 9:07:20 AM CEST Fabio M. De Francesco wrote:
> > On Thursday, August 19, 2021 8:30:21 AM CEST Greg Kroah-Hartman wrote:
> > > On Thu, Aug 19, 2021 at 08:08:37AM +0200, Fabio M. De Francesco wrote:
> > > > 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
> > > > second parameter "unsigned long *pirqL".
> > > > 
> > > > Use directly the in-kernel API; check and manage the return value of
> > > > mutex_lock_interruptible().
> > > > 
> > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > ---
> > > >  drivers/staging/r8188eu/core/rtw_mlme_ext.c     |  5 +++--
> > > >  drivers/staging/r8188eu/hal/usb_ops_linux.c     |  7 +++++--
> > > >  drivers/staging/r8188eu/include/osdep_service.h | 13 -------------
> > > >  drivers/staging/r8188eu/os_dep/os_intfs.c       |  5 +++--
> > > >  4 files changed, 11 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> > > > index f6ee72d5af09..484083468ebb 100644
> > > > --- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> > > > +++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> > > > @@ -4358,7 +4358,8 @@ 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);
> > > > +	if (mutex_lock_interruptible(&pxmitpriv->ack_tx_mutex))
> > > > +		return -EINTR;
> > > 
> > > But the code never would return this value if the lock function returned
> > > an error.  Why do that here now?
> 
> Ah, now I think I understand what you asked me ... sorry for not having 
> immediately grasped the meaning of your objection. :(
> 
> I guess you wanted to know why I decided to check and handle the 
> return values of mutex_lock_interruptible (), as the original code didn't. 
> Did I understand the correct meaning of your question?

Yes, that is correct.