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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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