[RFC PATCH] staging: r8188eu: Use completions instead of semaphores

[RFC PATCH] staging: r8188eu: Use completions instead of semaphores

[Fabio M. De Francesco]

rtw_cmd_thread() "up(s)" a semaphore twice, first to notify callers when
its execution is started and then to notify when it is about to end.

It makes the same semaphore go "up" twice in the same thread. This
construct makes Smatch to warn of duplicate "up(s)".

This thread uses interruptible semaphores where instead completions are
more suitable. For this purpose it calls an helper (_rtw_down_sema())
that returns values that are never checked. It may lead to bugs.

To address the above-mentioned issues, use two completions variables
instead of semaphores. Use the uninterruptible versions of
wake_for_completion*() because the interruptible / killable versions are
not necessary.

Tested with "ASUSTek Computer, Inc. Realtek 8188EUS [USB-N10 Nano]".

This is an RFC patch because I'm not sure that changing this code
from using semaphores to using completions variables is actually required.
After all, the code was working properly with semaphores and, at the same
time, I'm not sure if the Smatch warning about duplicate "up(s)" should
actually be addressed.

I'm waiting for Maintainers and other Reviewers to say if this patch is
actually needed and, if so, also for suggestions about how to improve
it. In particular I'm interested to know what they think of using the
uninterruptible version of wait_for_completion*().

Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_cmd.c    | 7 ++++---
 drivers/staging/r8188eu/include/rtw_cmd.h | 3 ++-
 drivers/staging/r8188eu/os_dep/os_intfs.c | 6 ++++--
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_cmd.c b/drivers/staging/r8188eu/core/rtw_cmd.c
index e17332677daa..195390449502 100644
--- a/drivers/staging/r8188eu/core/rtw_cmd.c
+++ b/drivers/staging/r8188eu/core/rtw_cmd.c
@@ -23,7 +23,8 @@ static int _rtw_init_cmd_priv(struct cmd_priv *pcmdpriv)
 
 	sema_init(&pcmdpriv->cmd_queue_sema, 0);
 	/* sema_init(&(pcmdpriv->cmd_done_sema), 0); */
-	sema_init(&pcmdpriv->terminate_cmdthread_sema, 0);
+	init_completion(&pcmdpriv->start_cmd_thread);
+	init_completion(&pcmdpriv->stop_cmd_thread);
 
 	rtw_init_queue(&pcmdpriv->cmd_queue);
 
@@ -248,7 +249,7 @@ int rtw_cmd_thread(void *context)
 	pcmdbuf = pcmdpriv->cmd_buf;
 
 	pcmdpriv->cmdthd_running = true;
-	up(&pcmdpriv->terminate_cmdthread_sema);
+	complete(&pcmdpriv->start_cmd_thread);
 
 	while (1) {
 		if (_rtw_down_sema(&pcmdpriv->cmd_queue_sema) == _FAIL)
@@ -329,7 +330,7 @@ int rtw_cmd_thread(void *context)
 		rtw_free_cmd_obj(pcmd);
 	} while (1);
 
-	up(&pcmdpriv->terminate_cmdthread_sema);
+	complete(&pcmdpriv->stop_cmd_thread);
 
 	thread_exit();
 }
diff --git a/drivers/staging/r8188eu/include/rtw_cmd.h b/drivers/staging/r8188eu/include/rtw_cmd.h
index 83fbb922db2c..b6266e3e2c40 100644
--- a/drivers/staging/r8188eu/include/rtw_cmd.h
+++ b/drivers/staging/r8188eu/include/rtw_cmd.h
@@ -34,7 +34,8 @@ struct cmd_obj {
 
 struct cmd_priv {
 	struct semaphore cmd_queue_sema;
-	struct semaphore terminate_cmdthread_sema;
+	struct completion start_cmd_thread;
+	struct completion stop_cmd_thread;
 	struct __queue cmd_queue;
 	u8	cmd_seq;
 	u8	*cmd_buf;	/* shall be non-paged, and 4 bytes aligned */
diff --git a/drivers/staging/r8188eu/os_dep/os_intfs.c b/drivers/staging/r8188eu/os_dep/os_intfs.c
index e7964a048c99..0bcea66f550b 100644
--- a/drivers/staging/r8188eu/os_dep/os_intfs.c
+++ b/drivers/staging/r8188eu/os_dep/os_intfs.c
@@ -385,7 +385,8 @@ u32 rtw_start_drv_threads(struct adapter *padapter)
 	if (IS_ERR(padapter->cmdThread))
 		_status = _FAIL;
 	else
-		_rtw_down_sema(&padapter->cmdpriv.terminate_cmdthread_sema); /* wait for cmd_thread to run */
+		/* wait for rtw_cmd_thread() to start running */
+		wait_for_completion(&padapter->cmdpriv.start_cmd_thread);
 
 	return _status;
 }
@@ -395,7 +396,8 @@ void rtw_stop_drv_threads(struct adapter *padapter)
 	/* Below is to termindate rtw_cmd_thread & event_thread... */
 	up(&padapter->cmdpriv.cmd_queue_sema);
 	if (padapter->cmdThread)
-		_rtw_down_sema(&padapter->cmdpriv.terminate_cmdthread_sema);
+		/* wait for rtw_cmd_thread() to stop running */
+		wait_for_completion(&padapter->cmdpriv.stop_cmd_thread);
 }
 
 static u8 rtw_init_default_value(struct adapter *padapter)
-- 
2.33.0

Re: [RFC PATCH] staging: r8188eu: Use completions instead of semaphores

[Dan Carpenter]

On Fri, Oct 15, 2021 at 01:02:38PM +0200, Fabio M. De Francesco wrote:
> rtw_cmd_thread() "up(s)" a semaphore twice, first to notify callers when
> its execution is started and then to notify when it is about to end.
> 
> It makes the same semaphore go "up" twice in the same thread. This
> construct makes Smatch to warn of duplicate "up(s)".
> 
> This thread uses interruptible semaphores where instead completions are
> more suitable. For this purpose it calls an helper (_rtw_down_sema())
> that returns values that are never checked. It may lead to bugs.
> 
> To address the above-mentioned issues, use two completions variables
> instead of semaphores. Use the uninterruptible versions of
> wake_for_completion*() because the interruptible / killable versions are
> not necessary.
> 
> Tested with "ASUSTek Computer, Inc. Realtek 8188EUS [USB-N10 Nano]".
> 
> This is an RFC patch because I'm not sure that changing this code
> from using semaphores to using completions variables is actually required.
> After all, the code was working properly with semaphores and, at the same
> time, I'm not sure if the Smatch warning about duplicate "up(s)" should
> actually be addressed.
> 
> I'm waiting for Maintainers and other Reviewers to say if this patch is
> actually needed and, if so, also for suggestions about how to improve
> it. In particular I'm interested to know what they think of using the
> uninterruptible version of wait_for_completion*().
> 
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

This is basically what Arnd did to rtl8723bs in commit:

commit 09a8ea34cf431bfb77159197e46753d101c528c5
Author: Arnd Bergmann <arnd@arndb.de>
Date:   Mon Dec 10 22:40:30 2018 +0100

    staging: rtl8723bs: change semaphores to completions

But there are some differences.  His patch is a little bit cleaner
because it gets rid of "pcmdpriv->cmd_queue_sema".  Could you basically
just ports Arnd's patch for this driver?

His patch goes quite a bit further as well, and change some other
semaphors but we could do it piece meal and just change the
rtw_cmd_thread() related ones.

regards,
dan carpenter

Re: [RFC PATCH] staging: r8188eu: Use completions instead of semaphores

[Fabio M. De Francesco]

On Friday, October 15, 2021 1:37:15 PM CEST Dan Carpenter wrote:
> On Fri, Oct 15, 2021 at 01:02:38PM +0200, Fabio M. De Francesco wrote:
> > rtw_cmd_thread() "up(s)" a semaphore twice, first to notify callers when
> > its execution is started and then to notify when it is about to end.
> > 
> > It makes the same semaphore go "up" twice in the same thread. This
> > construct makes Smatch to warn of duplicate "up(s)".
> >
> > [...]
> >
> > I'm waiting for Maintainers and other Reviewers to say if this patch is
> > actually needed and, if so, also for suggestions about how to improve
> > it. In particular I'm interested to know what they think of using the
> > uninterruptible version of wait_for_completion*().
> > 
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> 
> This is basically what Arnd did to rtl8723bs in commit:
> 
> commit 09a8ea34cf431bfb77159197e46753d101c528c5
> Author: Arnd Bergmann <arnd@arndb.de>
> Date:   Mon Dec 10 22:40:30 2018 +0100
> 
>     staging: rtl8723bs: change semaphores to completions
> 
> But there are some differences.  His patch is a little bit cleaner
> because it gets rid of "pcmdpriv->cmd_queue_sema".  Could you basically
> just ports Arnd's patch for this driver?
> 
> His patch goes quite a bit further as well, and change some other
> semaphors but we could do it piece meal and just change the
> rtw_cmd_thread() related ones.
> 
> regards,
> dan carpenter
> 
Hi Dan,

Thanks for your review. 

I wasn't aware of Arnd's patch. If I were I would have sent a "normal" patch.

Beyond this, I noticed that other semaphore (pcmdpriv->cmd_queue_sema) but, 
since I was not 100% sure that my changes would be accepted, I decided to 
leave it as-is for now and wait for reviews like yours.

Now that I know that this changes are welcome I'll also make the other 
changes. 

I guess that I have to change one semaphore per patch and make a series. 
However, now I see that Arnd's patch makes all the necessary changes in a 
single patch. What is the correct approach? Is one patch per semaphore 
preferred or one big patch for all of those that need to be changed?

Again, thank you very much.

Regards,

Fabio M. De Francesco

Re: [RFC PATCH] staging: r8188eu: Use completions instead of semaphores

[Dan Carpenter]

On Fri, Oct 15, 2021 at 02:11:41PM +0200, Fabio M. De Francesco wrote:
> On Friday, October 15, 2021 1:37:15 PM CEST Dan Carpenter wrote:
> > On Fri, Oct 15, 2021 at 01:02:38PM +0200, Fabio M. De Francesco wrote:
> > > rtw_cmd_thread() "up(s)" a semaphore twice, first to notify callers when
> > > its execution is started and then to notify when it is about to end.
> > > 
> > > It makes the same semaphore go "up" twice in the same thread. This
> > > construct makes Smatch to warn of duplicate "up(s)".
> > >
> > > [...]
> > >
> > > I'm waiting for Maintainers and other Reviewers to say if this patch is
> > > actually needed and, if so, also for suggestions about how to improve
> > > it. In particular I'm interested to know what they think of using the
> > > uninterruptible version of wait_for_completion*().
> > > 
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > 
> > This is basically what Arnd did to rtl8723bs in commit:
> > 
> > commit 09a8ea34cf431bfb77159197e46753d101c528c5
> > Author: Arnd Bergmann <arnd@arndb.de>
> > Date:   Mon Dec 10 22:40:30 2018 +0100
> > 
> >     staging: rtl8723bs: change semaphores to completions
> > 
> > But there are some differences.  His patch is a little bit cleaner
> > because it gets rid of "pcmdpriv->cmd_queue_sema".  Could you basically
> > just ports Arnd's patch for this driver?
> > 
> > His patch goes quite a bit further as well, and change some other
> > semaphors but we could do it piece meal and just change the
> > rtw_cmd_thread() related ones.
> > 
> > regards,
> > dan carpenter
> > 
> Hi Dan,
> 
> Thanks for your review. 
> 
> I wasn't aware of Arnd's patch. If I were I would have sent a "normal" patch.
> 
> Beyond this, I noticed that other semaphore (pcmdpriv->cmd_queue_sema) but, 
> since I was not 100% sure that my changes would be accepted, I decided to 
> leave it as-is for now and wait for reviews like yours.
> 
> Now that I know that this changes are welcome I'll also make the other 
> changes. 
> 
> I guess that I have to change one semaphore per patch and make a series. 
> However, now I see that Arnd's patch makes all the necessary changes in a 
> single patch. What is the correct approach? Is one patch per semaphore 
> preferred or one big patch for all of those that need to be changed?
> 

The two semaphores used in that function are very connected so I don't
think it makes sense to split those up.  The others are less connected.

regards,
dan carpenter

Re: [RFC PATCH] staging: r8188eu: Use completions instead of semaphores

[Phillip Potter]

On Fri, Oct 15, 2021 at 01:02:38PM +0200, Fabio M. De Francesco wrote:
> rtw_cmd_thread() "up(s)" a semaphore twice, first to notify callers when
> its execution is started and then to notify when it is about to end.
> 
> It makes the same semaphore go "up" twice in the same thread. This
> construct makes Smatch to warn of duplicate "up(s)".
> 
> This thread uses interruptible semaphores where instead completions are
> more suitable. For this purpose it calls an helper (_rtw_down_sema())
> that returns values that are never checked. It may lead to bugs.
> 
> To address the above-mentioned issues, use two completions variables
> instead of semaphores. Use the uninterruptible versions of
> wake_for_completion*() because the interruptible / killable versions are
> not necessary.
> 
> Tested with "ASUSTek Computer, Inc. Realtek 8188EUS [USB-N10 Nano]".
> 
> This is an RFC patch because I'm not sure that changing this code
> from using semaphores to using completions variables is actually required.
> After all, the code was working properly with semaphores and, at the same
> time, I'm not sure if the Smatch warning about duplicate "up(s)" should
> actually be addressed.
> 
> I'm waiting for Maintainers and other Reviewers to say if this patch is
> actually needed and, if so, also for suggestions about how to improve
> it. In particular I'm interested to know what they think of using the
> uninterruptible version of wait_for_completion*().
> 
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
> 

Dear Fabio,

Sounds like a good approach to me, nice work. I agree with Dan's
feedback also - will wait for the final patchset then give it a test for
you :-) Apologies I've been a little on the quiet side as of late.

Regards,
Phil

Re: [RFC PATCH] staging: r8188eu: Use completions instead of semaphores

[Fabio M. De Francesco]

On Friday, October 15, 2021 2:50:20 PM CEST Dan Carpenter wrote:
> On Fri, Oct 15, 2021 at 02:11:41PM +0200, Fabio M. De Francesco wrote:

[...]

> > Hi Dan,
> > 
> > Thanks for your review. 
> > 
> > I wasn't aware of Arnd's patch. If I were I would have sent a "normal" 
patch.
> > 
> > Beyond this, I noticed that other semaphore (pcmdpriv->cmd_queue_sema) 
but, 
> > since I was not 100% sure that my changes would be accepted, I decided to 
> > leave it as-is for now and wait for reviews like yours.
> > 
> > Now that I know that this changes are welcome I'll also make the other 
> > changes. 
> > 
> > I guess that I have to change one semaphore per patch and make a series. 
> > However, now I see that Arnd's patch makes all the necessary changes in a 
> > single patch. What is the correct approach? Is one patch per semaphore 
> > preferred or one big patch for all of those that need to be changed?
> > 
> 
> The two semaphores used in that function are very connected so I don't
> think it makes sense to split those up.

I agree with you: the two semaphores in rtw_cmd_thread() are (somewhat) 
connected. However they serve different purposes. 

The first is used in to signal start and end of command thread 
(rtw_cmd_thread()). The second is used to notify that same thread that some 
commands have been enqueued. They serve two different purposes.

I prefer to make a series of three patches that I'll call "staging: r8188eu: 
use conditions variables and clean rtw_cmd_thread()". This choice is based on 
the above-mentioned fact that the two semaphores are there for different 
purposes. Let me explain what I'll put in each of the three patches... 

1) The first semaphore came to my attention because of a Smatch warning about 
duplicate releases ("up(s)) of the same semaphore in the same function. While 
addressing that problem I noticed that completions variables are more suited 
than semaphores for doing the work. 

So I didn't merely change the names of the semaphores in order to silence 
Smatch, instead I chose a more radical approach that is to replace semaphores 
with condition variables.

The first patch is there, I have the commit message ready to be re-used, and  
it is self-contained even if we still have a second semaphore that is there 
for servicing another purpose.

2) The second semaphore came to my attention while working on the first and 
trying to understand what rtw_cmd_thread() is meant for. It is there for very 
different reasons. There is only one relation between them, that is that they 
are used into the same function. Nothing else.

So I prefer to write a second commit message (in patch 2/3) that explains 
what the semaphore does and why it is better to replace it with a condition 
variable. Obviously, this "why are you changing it?" has a different answer 
with respect of what I say in patch 1/3.

3) With 3/3 there is the "clean" part of "staging: r8188eu: use conditions 
variables and clean rtw_cmd_thread()". While reading rtw_cmd_thread(), I 
noticed that there is an unnecessary duplicate of an 'if' statement. I want 
to remove the first (the one before the "_next:" label.

Other semaphores is r8188eu (if there are more) will be eventually addressed 
in future patches.

I hope that you and Phillip agree with me on this step by step approach.

Regards,

Fabio M. De Francesco

> The others are less connected.
> 
> regards,
> dan carpenter
> 
> 

Re: [RFC PATCH] staging: r8188eu: Use completions instead of semaphores

[Fabio M. De Francesco]

On Friday, October 15, 2021 7:52:07 PM CEST Phillip Potter wrote:
> 
> Dear Fabio,
> 
> Sounds like a good approach to me, nice work.

Dear Phil,

Thanks for the "good job". These words always sound very rewarding :)

Please read my reply to Dan's message. I have decided to go through a 
different approach and make two patches for the semaphores => condition 
variables and a third for removing a duplicate 'if' statement in that  
rtw_cmd_thread() where now we have the semaphores.

As I wrote in that reply, I hope that you and Dan agree with me on this 
different approach to fix rtw_cmd_thread().

Thanks,

Fabio 

> I agree with Dan's
> feedback also - will wait for the final patchset then give it a test for
> you :-) Apologies I've been a little on the quiet side as of late.

P.S.: No need to apologize at all :)

I had noticed that you haven't been around for a while. I would have 
appreciated a review from you (and Larry) on the series of 32 patches that 
Pavel Skripkin and I co-developed and submitted some weeks ago.

However, I am 100% sure that you really had some important reasons for not 
being here.

> 
> Regards,
> Phil
> 

Re: [RFC PATCH] staging: r8188eu: Use completions instead of semaphores

[Dan Carpenter]

On Sat, Oct 16, 2021 at 08:43:32AM +0200, Fabio M. De Francesco wrote:
> 2) The second semaphore came to my attention while working on the first and 
> trying to understand what rtw_cmd_thread() is meant for. It is there for very 
> different reasons. There is only one relation between them, that is that they 
> are used into the same function. Nothing else.
> 
> So I prefer to write a second commit message (in patch 2/3) that explains 
> what the semaphore does and why it is better to replace it with a condition 
> variable. Obviously, this "why are you changing it?" has a different answer 
> with respect of what I say in patch 1/3.
> 

I don't think you're correct, but I will wait for you patch and look
again.

regards,
dan carpenter

Re: [RFC PATCH] staging: r8188eu: Use completions instead of semaphores

[Phillip Potter]

On Sat, Oct 16, 2021 at 08:59:27AM +0200, Fabio M. De Francesco wrote:
> P.S.: No need to apologize at all :)
> 
> I had noticed that you haven't been around for a while. I would have 
> appreciated a review from you (and Larry) on the series of 32 patches that 
> Pavel Skripkin and I co-developed and submitted some weeks ago.
> 
> However, I am 100% sure that you really had some important reasons for not 
> being here.
> 
> > 
> > Regards,
> > Phil
> > 
> 

I have no excuse, other than simply not having enough hours in the day
:-)

Regards,
Phil