LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] staging: r8188eu: Remove _enter/_exit_critical_mutex()
@ 2021-08-19 12:49 Fabio M. De Francesco
  2021-08-19 14:54 ` Greg Kroah-Hartman
  2021-08-19 15:08 ` kernel test robot
  0 siblings, 2 replies; 4+ messages in thread
From: Fabio M. De Francesco @ 2021-08-19 12:49 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Larry Finger, Phillip Potter, Martin Kaiser,
	Michael Straube, linux-staging, linux-kernel
  Cc: 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.
Replace them with the in-kernel API. Ignore return values as it was
in the old code.

Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---

v2: Ignore return values from Mutexes API.

 drivers/staging/r8188eu/core/rtw_mlme_ext.c     |  5 +++--
 drivers/staging/r8188eu/hal/usb_ops_linux.c     |  5 +++--
 drivers/staging/r8188eu/include/osdep_service.h | 13 -------------
 drivers/staging/r8188eu/os_dep/os_intfs.c       |  5 +++--
 4 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
index 5325fe41fbee..9f53cab33333 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
@@ -4359,7 +4359,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))
+		;	/*ignore return value */
 	pxmitpriv->ack_tx = true;
 
 	pmgntframe->ack_report = 1;
@@ -4368,7 +4369,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..120d6e2c6065 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -32,7 +32,8 @@ 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))
+		;	/* ignore return value */
 
 	/*  Acquire IO memory for vendorreq */
 	pIo_buf = dvobjpriv->usb_vendor_req_buf;
@@ -96,7 +97,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..3823042256a6 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))
+		;	/* ignore return value */
 	ret = _netdev_open(pnetdev);
-	_exit_critical_mutex(padapter->hw_init_mutex, NULL);
+	mutex_unlock(padapter->hw_init_mutex);
 	return ret;
 }
 
-- 
2.32.0


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

* Re: [PATCH v2] staging: r8188eu: Remove _enter/_exit_critical_mutex()
  2021-08-19 12:49 [PATCH v2] staging: r8188eu: Remove _enter/_exit_critical_mutex() Fabio M. De Francesco
@ 2021-08-19 14:54 ` Greg Kroah-Hartman
  2021-08-19 19:54   ` Fabio M. De Francesco
  2021-08-19 15:08 ` kernel test robot
  1 sibling, 1 reply; 4+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-19 14:54 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Martin Kaiser, Michael Straube,
	linux-staging, linux-kernel

On Thu, Aug 19, 2021 at 02:49:55PM +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 argument named pirqL of type unsigned long.
> Replace them with the in-kernel API. Ignore return values as it was
> in the old code.
> 
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
> 
> v2: Ignore return values from Mutexes API.
> 
>  drivers/staging/r8188eu/core/rtw_mlme_ext.c     |  5 +++--
>  drivers/staging/r8188eu/hal/usb_ops_linux.c     |  5 +++--
>  drivers/staging/r8188eu/include/osdep_service.h | 13 -------------
>  drivers/staging/r8188eu/os_dep/os_intfs.c       |  5 +++--
>  4 files changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> index 5325fe41fbee..9f53cab33333 100644
> --- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> +++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> @@ -4359,7 +4359,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))
> +		;	/*ignore return value */

Ick, no.  (not to mention the wrong comment style...)

If this really is "criticial", why can it be interrupted?

The existing code is such that the code can be interrupted, but if it
fails, the lock is not gotten, and the CODE CONTINUES AS IF IT IS OK!

So either this is never interruptable (my guess, one almost never needs
interruptable locks in a driver) and should just do a normal mutex lock,
or the code is totally broken and the locking should be revisited
entirely.

But a "blind" change like this is not good, let's get it right...

thanks,

greg k-h

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

* Re: [PATCH v2] staging: r8188eu: Remove _enter/_exit_critical_mutex()
  2021-08-19 12:49 [PATCH v2] staging: r8188eu: Remove _enter/_exit_critical_mutex() Fabio M. De Francesco
  2021-08-19 14:54 ` Greg Kroah-Hartman
@ 2021-08-19 15:08 ` kernel test robot
  1 sibling, 0 replies; 4+ messages in thread
From: kernel test robot @ 2021-08-19 15:08 UTC (permalink / raw)
  To: Fabio M. De Francesco, Greg Kroah-Hartman, Larry Finger,
	Phillip Potter, Martin Kaiser, Michael Straube, linux-staging,
	linux-kernel
  Cc: kbuild-all, Fabio M. De Francesco

[-- Attachment #1: Type: text/plain, Size: 3160 bytes --]

Hi "Fabio,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]

url:    https://github.com/0day-ci/linux/commits/Fabio-M-De-Francesco/staging-r8188eu-Remove-_enter-_exit_critical_mutex/20210819-205259
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 093991aaadf0fbb34184fa37a46e7a157da3f386
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/be2317351161f572a68f71544046d8491856818f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Fabio-M-De-Francesco/staging-r8188eu-Remove-_enter-_exit_critical_mutex/20210819-205259
        git checkout be2317351161f572a68f71544046d8491856818f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=m68k 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/staging/r8188eu/core/rtw_mlme_ext.c: In function 'dump_mgntframe_and_wait_ack':
>> drivers/staging/r8188eu/core/rtw_mlme_ext.c:4363:17: warning: suggest braces around empty body in an 'if' statement [-Wempty-body]
    4363 |                 ;       /*ignore return value */
         |                 ^
--
   drivers/staging/r8188eu/hal/usb_ops_linux.c: In function 'usbctrl_vendorreq':
>> drivers/staging/r8188eu/hal/usb_ops_linux.c:36:17: warning: suggest braces around empty body in an 'if' statement [-Wempty-body]
      36 |                 ;       /* ignore return value */
         |                 ^
--
   drivers/staging/r8188eu/os_dep/os_intfs.c: In function 'netdev_open':
>> drivers/staging/r8188eu/os_dep/os_intfs.c:1069:17: warning: suggest braces around empty body in an 'if' statement [-Wempty-body]
    1069 |                 ;       /* ignore return value */
         |                 ^


vim +/if +4363 drivers/staging/r8188eu/core/rtw_mlme_ext.c

  4352	
  4353	s32 dump_mgntframe_and_wait_ack(struct adapter *padapter, struct xmit_frame *pmgntframe)
  4354	{
  4355		s32 ret = _FAIL;
  4356		u32 timeout_ms = 500;/*   500ms */
  4357		struct xmit_priv	*pxmitpriv = &padapter->xmitpriv;
  4358	
  4359		if (padapter->bSurpriseRemoved || padapter->bDriverStopped)
  4360			return -1;
  4361	
  4362		if (mutex_lock_interruptible(&pxmitpriv->ack_tx_mutex))
> 4363			;	/*ignore return value */
  4364		pxmitpriv->ack_tx = true;
  4365	
  4366		pmgntframe->ack_report = 1;
  4367		if (rtw_hal_mgnt_xmit(padapter, pmgntframe) == _SUCCESS) {
  4368			ret = rtw_ack_tx_wait(pxmitpriv, timeout_ms);
  4369		}
  4370	
  4371		pxmitpriv->ack_tx = false;
  4372		mutex_unlock(&pxmitpriv->ack_tx_mutex);
  4373	
  4374		return ret;
  4375	}
  4376	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 60728 bytes --]

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

* Re: [PATCH v2] staging: r8188eu: Remove _enter/_exit_critical_mutex()
  2021-08-19 14:54 ` Greg Kroah-Hartman
@ 2021-08-19 19:54   ` Fabio M. De Francesco
  0 siblings, 0 replies; 4+ messages in thread
From: Fabio M. De Francesco @ 2021-08-19 19:54 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Larry Finger, Phillip Potter, Martin Kaiser, Michael Straube,
	linux-staging, linux-kernel

On Thursday, August 19, 2021 4:54:00 PM CEST Greg Kroah-Hartman wrote:
> On Thu, Aug 19, 2021 at 02:49:55PM +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 argument named pirqL of type unsigned long.
> > Replace them with the in-kernel API. Ignore return values as it was
> > in the old code.
> > 
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> > 
> > v2: Ignore return values from Mutexes API.
> > 
> >  drivers/staging/r8188eu/core/rtw_mlme_ext.c     |  5 +++--
> >  drivers/staging/r8188eu/hal/usb_ops_linux.c     |  5 +++--
> >  drivers/staging/r8188eu/include/osdep_service.h | 13 -------------
> >  drivers/staging/r8188eu/os_dep/os_intfs.c       |  5 +++--
> >  4 files changed, 9 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> > index 5325fe41fbee..9f53cab33333 100644
> > --- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> > +++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> > @@ -4359,7 +4359,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))
> > +		;	/*ignore return value */
> 
> Ick, no.  (not to mention the wrong comment style...)
> 
> If this really is "criticial", why can it be interrupted?
> 
> The existing code is such that the code can be interrupted, but if it
> fails, the lock is not gotten, and the CODE CONTINUES AS IF IT IS OK!

This was perfectly clear. The old code ignored -EINTR and, in case when
blocking to wait for the lock to become available is interrupted (although
I really don't know how - a signal to a driver? I guess it cannot happen) it
continues to execute in the critical region so leading to potential race 
conditions.

This is what I get and I guess I'm not too far from the truth... Did I get it
right?
 
> So either this is never interruptable (my guess, one almost never needs
> interruptable locks in a driver) 

This is what I think, why interruptible locks in a driver? I suppose that they
should only be used when the kernel executes on behalf of a process. 

> and should just do a normal mutex lock,
> or the code is totally broken and the locking should be revisited
> entirely.

My guess is that the code that is waiting to acquire a lock could *really*
be interrupted should manage that interruption without falling blindly
in the critical region and we have two ways to do that:

1) return -EINTR to the caller which should manage that error by retrying
the acquisition, or

2) retry in a loop inside the callee, avoiding to proceed to the critical section
without an acquired mutex.

A third option is using mutex_lock() (uninterruptible), which I would prefer
if I were absolutely sure that nothing could interrupt waiting for acquiring
the lock. 

I have to look at how other drivers manage similar cases, although I'm 
pretty convinced that a simple mutex_lock() should be fine here.

Thanks,

Fabio

> But a "blind" change like this is not good, let's get it right...

I agree, let me take some time to investigate what are the best practices 
that the kernel implements in cases like this. What I'm sure at the moment is 
that the old code, as-is, is broken.

Thanks,

Fabio
 
> thanks,
> 
> greg k-h
> 





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

end of thread, other threads:[~2021-08-19 19:54 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-19 12:49 [PATCH v2] staging: r8188eu: Remove _enter/_exit_critical_mutex() Fabio M. De Francesco
2021-08-19 14:54 ` Greg Kroah-Hartman
2021-08-19 19:54   ` Fabio M. De Francesco
2021-08-19 15:08 ` kernel test robot

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).