[PATCH RESEND] Staging: rtl8188eu: fix double unlock

[PATCH RESEND] Staging: rtl8188eu: fix double unlock

[Matteo Semenzato]

From: Matteo Semenzato <mattew8898@gmail.com>

The rtw_cmd_thread semaphore was being unlocked twice.

Signed-off-by: Matteo Semenzato <mattew8898@gmail.com>
---
 drivers/staging/rtl8188eu/core/rtw_cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_cmd.c b/drivers/staging/rtl8188eu/core/rtw_cmd.c
index 4b43462..43a8515 100644
--- a/drivers/staging/rtl8188eu/core/rtw_cmd.c
+++ b/drivers/staging/rtl8188eu/core/rtw_cmd.c
@@ -173,7 +173,7 @@ int rtw_cmd_thread(void *context)
 	allow_signal(SIGTERM);
 
 	pcmdpriv->cmdthd_running = true;
-	up(&pcmdpriv->terminate_cmdthread_sema);
+	down(&pcmdpriv->terminate_cmdthread_sema);
 
 	RT_TRACE(_module_rtl871x_cmd_c_, _drv_info_, ("start r871x rtw_cmd_thread !!!!\n"));
 
-- 
2.3.1

Re: [PATCH RESEND] Staging: rtl8188eu: fix double unlock

[Larry Finger]

On 03/07/2015 09:33 AM, Matteo Semenzato wrote:
> From: Matteo Semenzato <mattew8898@gmail.com>
>
> The rtw_cmd_thread semaphore was being unlocked twice.
>
> Signed-off-by: Matteo Semenzato <mattew8898@gmail.com>
> ---
>   drivers/staging/rtl8188eu/core/rtw_cmd.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8188eu/core/rtw_cmd.c b/drivers/staging/rtl8188eu/core/rtw_cmd.c
> index 4b43462..43a8515 100644
> --- a/drivers/staging/rtl8188eu/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8188eu/core/rtw_cmd.c
> @@ -173,7 +173,7 @@ int rtw_cmd_thread(void *context)
>   	allow_signal(SIGTERM);
>
>   	pcmdpriv->cmdthd_running = true;
> -	up(&pcmdpriv->terminate_cmdthread_sema);
> +	down(&pcmdpriv->terminate_cmdthread_sema);
>
>   	RT_TRACE(_module_rtl871x_cmd_c_, _drv_info_, ("start r871x rtw_cmd_thread !!!!\n"));

I get nervous when I see patch 2/2 immediately resent as a single patch. What is 
going on?

Have you actually tested this patch, or did you find this with some static checker?

Larry

Re: [PATCH RESEND] Staging: rtl8188eu: fix double unlock

[Matteo Semenzato]

Il giorno sab, 07/03/2015 alle 11.28 -0600, Larry Finger ha scritto:
> Have you actually tested this patch, or did you find this with some static checker?
> 
> Larry
The double unlock was found using smatch.

Re: [PATCH RESEND] Staging: rtl8188eu: fix double unlock

[Larry Finger]

On 03/07/2015 11:35 AM, Matteo Semenzato wrote:
> Il giorno sab, 07/03/2015 alle 11.28 -0600, Larry Finger ha scritto:
>> Have you actually tested this patch, or did you find this with some static checker?
>>
>> Larry
> The double unlock was found using smatch.

Are you really willing to test changes in locking based on a static checker? I 
would not be so quick.

By the way, my reading of the documentation is that up releases the lock whereas 
down sets it.  Thus, I think your patch introduces a bug rather than fix one. I 
agree that there is a mismatch, but I do not think your patch fixes anything.

Until this is actually tested on running code, NACK.

Larry

Re: [PATCH RESEND] Staging: rtl8188eu: fix double unlock

[Dan Carpenter]

Why is it a RESEND?

RESEND is a bit rude because it implies that we messed up by ignoring
your first email so you're sending us the exact same thing again.
Sometimes rudeness is valid if people are ignoring good patches but you
send the first email 4 minutes before sending the second email.

On Sat, Mar 07, 2015 at 04:33:27PM +0100, Matteo Semenzato wrote:
> From: Matteo Semenzato <mattew8898@gmail.com>

No need for this, we can get it from your email.

> 
> The rtw_cmd_thread semaphore was being unlocked twice.

This patch is probably correct, but it's a bit risky without testing or
further analysis.  Please explain how you verified that it won't cause
a deadlock.

regards,
dan carpenter

Re: [PATCH RESEND] Staging: rtl8188eu: fix double unlock

[Matteo Semenzato]

Il giorno lun, 09/03/2015 alle 12.16 +0300, Dan Carpenter ha scritto:
> Why is it a RESEND?
> 
> RESEND is a bit rude because it implies that we messed up by ignoring
> your first email so you're sending us the exact same thing again.
> Sometimes rudeness is valid if people are ignoring good patches but you
> send the first email 4 minutes before sending the second email.
This patch is a RESEND because i forgot to remove PATCH 2/2 from the subject.
> On Sat, Mar 07, 2015 at 04:33:27PM +0100, Matteo Semenzato wrote:
> > From: Matteo Semenzato <mattew8898@gmail.com>
> 
> No need for this, we can get it from your email.
> 
> > 
> > The rtw_cmd_thread semaphore was being unlocked twice.
> 
> This patch is probably correct, but it's a bit risky without testing or
> further analysis.  Please explain how you verified that it won't cause
> a deadlock.
This patch is wrong because it tries to call down on a semaphore that is
initialized to 0 and the semaphore is not unlocked anywhere else.

Re: [PATCH RESEND] Staging: rtl8188eu: fix double unlock

[Dan Carpenter]

On Mon, Mar 09, 2015 at 03:21:13PM +0100, Matteo Semenzato wrote:
> Il giorno lun, 09/03/2015 alle 12.16 +0300, Dan Carpenter ha scritto:
> > Why is it a RESEND?
> > 
> > RESEND is a bit rude because it implies that we messed up by ignoring
> > your first email so you're sending us the exact same thing again.
> > Sometimes rudeness is valid if people are ignoring good patches but you
> > send the first email 4 minutes before sending the second email.
> This patch is a RESEND because i forgot to remove PATCH 2/2 from the subject.
> > On Sat, Mar 07, 2015 at 04:33:27PM +0100, Matteo Semenzato wrote:
> > > From: Matteo Semenzato <mattew8898@gmail.com>
> > 
> > No need for this, we can get it from your email.
> > 
> > > 
> > > The rtw_cmd_thread semaphore was being unlocked twice.
> > 
> > This patch is probably correct, but it's a bit risky without testing or
> > further analysis.  Please explain how you verified that it won't cause
> > a deadlock.
> This patch is wrong because it tries to call down on a semaphore that is
> initialized to 0 and the semaphore is not unlocked anywhere else.

Yep.  Just remove the locking and use kthread_stop() in
rtw_stop_drv_threads().

regards,
dan carpenter