* [PATCH] staging: r8188eu: Remove _enter/_exit_critical_mutex()
@ 2021-08-19 6:08 Fabio M. De Francesco
2021-08-19 6:30 ` Greg Kroah-Hartman
0 siblings, 1 reply; 5+ messages in thread
From: Fabio M. De Francesco @ 2021-08-19 6:08 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
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] staging: r8188eu: Remove _enter/_exit_critical_mutex()
2021-08-19 6:08 [PATCH] staging: r8188eu: Remove _enter/_exit_critical_mutex() Fabio M. De Francesco
@ 2021-08-19 6:30 ` Greg Kroah-Hartman
2021-08-19 7:07 ` Fabio M. De Francesco
0 siblings, 1 reply; 5+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-19 6:30 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 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] staging: r8188eu: Remove _enter/_exit_critical_mutex()
2021-08-19 6:30 ` Greg Kroah-Hartman
@ 2021-08-19 7:07 ` Fabio M. De Francesco
2021-08-19 10:16 ` Fabio M. De Francesco
0 siblings, 1 reply; 5+ messages in thread
From: Fabio M. De Francesco @ 2021-08-19 7:07 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 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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] staging: r8188eu: Remove _enter/_exit_critical_mutex()
2021-08-19 7:07 ` Fabio M. De Francesco
@ 2021-08-19 10:16 ` Fabio M. De Francesco
2021-08-19 14:51 ` Greg Kroah-Hartman
0 siblings, 1 reply; 5+ messages in thread
From: Fabio M. De Francesco @ 2021-08-19 10:16 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 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] staging: r8188eu: Remove _enter/_exit_critical_mutex()
2021-08-19 10:16 ` Fabio M. De Francesco
@ 2021-08-19 14:51 ` Greg Kroah-Hartman
0 siblings, 0 replies; 5+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-19 14:51 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 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-08-19 14:51 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-19 6:08 [PATCH] staging: r8188eu: Remove _enter/_exit_critical_mutex() Fabio M. De Francesco
2021-08-19 6:30 ` Greg Kroah-Hartman
2021-08-19 7:07 ` Fabio M. De Francesco
2021-08-19 10:16 ` Fabio M. De Francesco
2021-08-19 14:51 ` Greg Kroah-Hartman
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).