[PATCH 0/3] staging: r8188eu: use completions and clean rtw_cmd_thread()

[PATCH 0/3] staging: r8188eu: use completions and clean rtw_cmd_thread()

[Fabio M. De Francesco]

This series replaces two semaphores with three completion variables
in rtw_cmd_thread(). Completions variables are better suited for the
purposes that are explained in detail in the commit messages of patches
1/3 and 2/3. Furthermore, patch 3/3 removes a redundant 'if' statement
from that same rtw_cmd_thread().

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

Many thanks to Dan Carpenter <dan.carpenter@oracle.com> who helped with
his review of the RFC Patch.

Fabio M. De Francesco (3):
  staging: r8188eu: Use completions for signaling start and end of
    kthread
  staging: r8188eu: Use completions for signaling enqueueing of commands
  staging: r8188eu: Remove redundant 'if' statement

 drivers/staging/r8188eu/core/rtw_cmd.c    | 19 +++++++------------
 drivers/staging/r8188eu/include/rtw_cmd.h |  5 +++--
 drivers/staging/r8188eu/os_dep/os_intfs.c |  8 +++++---
 3 files changed, 15 insertions(+), 17 deletions(-)

-- 
2.33.0

[PATCH 1/3] staging: r8188eu: Use completions for signaling start and end kthread

[Fabio M. De Francesco]

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

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

This kthread 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
wait_for_completion*() because the interruptible / killable versions are
not necessary.

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

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: [PATCH 1/3] staging: r8188eu: Use completions for signaling start and end kthread

[Dan Carpenter]

Yeah, you're right.  This is fine.  It's a bit confusing how Arnd
re-used complete to start the function.

regards,
dan carpenter

Re: [PATCH 1/3] staging: r8188eu: Use completions for signaling start and end kthread

[Fabio M. De Francesco]

On Saturday, October 16, 2021 9:26:04 PM CEST Dan Carpenter wrote:
> Yeah, you're right.  This is fine.  It's a bit confusing how Arnd
> re-used complete to start the function.
> 
> regards,
> dan carpenter
> 

Thanks, Dan.

What still confuses me is why rtw_start_drv_threads() wants to be notified 
that the kthread has started. Can you please answer this question?

Regards,

Fabio

[PATCH 2/3] staging: r8188eu: Use completions for signaling enqueueing

[Fabio M. De Francesco]

rtw_enqueue_cmd() uses a semaphore to notify rtw_cmd_thread() that it
has enqueued commands. rtw_cmd_thread() "down(s)" in interruptible mode
to wait to be notified.

Use completion variables because they are better suited for the purpose.

In rtw_cmd_thread(), wait in killable mode because there are no other
reasons to interrupt waiting if not for killing.

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

diff --git a/drivers/staging/r8188eu/core/rtw_cmd.c b/drivers/staging/r8188eu/core/rtw_cmd.c
index 195390449502..2900c31d8ec9 100644
--- a/drivers/staging/r8188eu/core/rtw_cmd.c
+++ b/drivers/staging/r8188eu/core/rtw_cmd.c
@@ -21,7 +21,7 @@ static int _rtw_init_cmd_priv(struct cmd_priv *pcmdpriv)
 {
 	int res = _SUCCESS;
 
-	sema_init(&pcmdpriv->cmd_queue_sema, 0);
+	init_completion(&pcmdpriv->enqueue_cmd);
 	/* sema_init(&(pcmdpriv->cmd_done_sema), 0); */
 	init_completion(&pcmdpriv->start_cmd_thread);
 	init_completion(&pcmdpriv->stop_cmd_thread);
@@ -198,7 +198,7 @@ u32 rtw_enqueue_cmd(struct cmd_priv *pcmdpriv, struct cmd_obj *cmd_obj)
 	res = _rtw_enqueue_cmd(&pcmdpriv->cmd_queue, cmd_obj);
 
 	if (res == _SUCCESS)
-		up(&pcmdpriv->cmd_queue_sema);
+		complete(&pcmdpriv->enqueue_cmd);
 
 exit:
 
@@ -252,7 +252,7 @@ int rtw_cmd_thread(void *context)
 	complete(&pcmdpriv->start_cmd_thread);
 
 	while (1) {
-		if (_rtw_down_sema(&pcmdpriv->cmd_queue_sema) == _FAIL)
+		if (wait_for_completion_killable(&pcmdpriv->enqueue_cmd))
 			break;
 
 		if (padapter->bDriverStopped ||
diff --git a/drivers/staging/r8188eu/include/rtw_cmd.h b/drivers/staging/r8188eu/include/rtw_cmd.h
index b6266e3e2c40..47c3c80cc24a 100644
--- a/drivers/staging/r8188eu/include/rtw_cmd.h
+++ b/drivers/staging/r8188eu/include/rtw_cmd.h
@@ -33,7 +33,7 @@ struct cmd_obj {
 };
 
 struct cmd_priv {
-	struct semaphore cmd_queue_sema;
+	struct completion enqueue_cmd;
 	struct completion start_cmd_thread;
 	struct completion stop_cmd_thread;
 	struct __queue cmd_queue;
diff --git a/drivers/staging/r8188eu/os_dep/os_intfs.c b/drivers/staging/r8188eu/os_dep/os_intfs.c
index 0bcea66f550b..96e9897085fe 100644
--- a/drivers/staging/r8188eu/os_dep/os_intfs.c
+++ b/drivers/staging/r8188eu/os_dep/os_intfs.c
@@ -394,7 +394,7 @@ u32 rtw_start_drv_threads(struct adapter *padapter)
 void rtw_stop_drv_threads(struct adapter *padapter)
 {
 	/* Below is to termindate rtw_cmd_thread & event_thread... */
-	up(&padapter->cmdpriv.cmd_queue_sema);
+	complete(&padapter->cmdpriv.enqueue_cmd);
 	if (padapter->cmdThread)
 		/* wait for rtw_cmd_thread() to stop running */
 		wait_for_completion(&padapter->cmdpriv.stop_cmd_thread);
-- 
2.33.0

[PATCH 3/3] staging: r8188eu: Remove redundant 'if' statement

[Fabio M. De Francesco]

Remove a redundant 'if' statement.

Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_cmd.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_cmd.c b/drivers/staging/r8188eu/core/rtw_cmd.c
index 2900c31d8ec9..2263e35b45cb 100644
--- a/drivers/staging/r8188eu/core/rtw_cmd.c
+++ b/drivers/staging/r8188eu/core/rtw_cmd.c
@@ -255,12 +255,6 @@ int rtw_cmd_thread(void *context)
 		if (wait_for_completion_killable(&pcmdpriv->enqueue_cmd))
 			break;
 
-		if (padapter->bDriverStopped ||
-		    padapter->bSurpriseRemoved) {
-			DBG_88E("%s: DriverStopped(%d) SurpriseRemoved(%d) break at line %d\n",
-				__func__, padapter->bDriverStopped, padapter->bSurpriseRemoved, __LINE__);
-			break;
-		}
 _next:
 		if (padapter->bDriverStopped ||
 		    padapter->bSurpriseRemoved) {
-- 
2.33.0

Re: [PATCH 3/3] staging: r8188eu: Remove redundant 'if' statement

[Martin Kaiser]

Thus wrote Fabio M. De Francesco (fmdefrancesco@gmail.com):

> Remove a redundant 'if' statement.

> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  drivers/staging/r8188eu/core/rtw_cmd.c | 6 ------
>  1 file changed, 6 deletions(-)

> diff --git a/drivers/staging/r8188eu/core/rtw_cmd.c b/drivers/staging/r8188eu/core/rtw_cmd.c
> index 2900c31d8ec9..2263e35b45cb 100644
> --- a/drivers/staging/r8188eu/core/rtw_cmd.c
> +++ b/drivers/staging/r8188eu/core/rtw_cmd.c
> @@ -255,12 +255,6 @@ int rtw_cmd_thread(void *context)
>  		if (wait_for_completion_killable(&pcmdpriv->enqueue_cmd))
>  			break;

> -		if (padapter->bDriverStopped ||
> -		    padapter->bSurpriseRemoved) {
> -			DBG_88E("%s: DriverStopped(%d) SurpriseRemoved(%d) break at line %d\n",
> -				__func__, padapter->bDriverStopped, padapter->bSurpriseRemoved, __LINE__);
> -			break;
> -		}
>  _next:
>  		if (padapter->bDriverStopped ||
>  		    padapter->bSurpriseRemoved) {
> -- 
> 2.33.0

Acked-by: Martin Kaiser <martin@kaiser.cx>

Re: [PATCH 0/3] staging: r8188eu: use completions and clean rtw_cmd_thread()

[Phillip Potter]

On Sat, Oct 16, 2021 at 11:10:39AM +0200, Fabio M. De Francesco wrote:
> This series replaces two semaphores with three completion variables
> in rtw_cmd_thread(). Completions variables are better suited for the
> purposes that are explained in detail in the commit messages of patches
> 1/3 and 2/3. Furthermore, patch 3/3 removes a redundant 'if' statement
> from that same rtw_cmd_thread().
> 
> Tested with ASUSTek Computer, Inc. Realtek 8188EUS [USB-N10 Nano]
> 
> Many thanks to Dan Carpenter <dan.carpenter@oracle.com> who helped with
> his review of the RFC Patch.
> 
> Fabio M. De Francesco (3):
>   staging: r8188eu: Use completions for signaling start and end of
>     kthread
>   staging: r8188eu: Use completions for signaling enqueueing of commands
>   staging: r8188eu: Remove redundant 'if' statement
> 
>  drivers/staging/r8188eu/core/rtw_cmd.c    | 19 +++++++------------
>  drivers/staging/r8188eu/include/rtw_cmd.h |  5 +++--
>  drivers/staging/r8188eu/os_dep/os_intfs.c |  8 +++++---
>  3 files changed, 15 insertions(+), 17 deletions(-)
> 
> -- 
> 2.33.0
> 

Dear Fabio,

Built and tested on my USB-N10 Nano, working well. In terms of how
you've split the patches out, I have no problem with it personally,
given that one semaphore was there for kthread start/stop and the other
for queuing. Looks good to me anyway based on what I know of completion
variables :-) I assume you've not made the waits killable or
interruptible in patch 1 due to the fact they are specifically related
to kthread start/stop? Anyhow:

For whole series:
Acked-by: Phillip Potter <phil@philpotter.co.uk>

Regards,
Phil

Re: [PATCH 0/3] staging: r8188eu: use completions and clean rtw_cmd_thread()

[Fabio M. De Francesco]

On Saturday, October 16, 2021 5:27:40 PM CEST Phillip Potter wrote:
> On Sat, Oct 16, 2021 at 11:10:39AM +0200, Fabio M. De Francesco wrote:
> > This series replaces two semaphores with three completion variables
> > in rtw_cmd_thread(). Completions variables are better suited for the
> > purposes that are explained in detail in the commit messages of patches
> > 1/3 and 2/3. Furthermore, patch 3/3 removes a redundant 'if' statement
> > from that same rtw_cmd_thread().
> > 
> > Tested with ASUSTek Computer, Inc. Realtek 8188EUS [USB-N10 Nano]
> > 
> > Many thanks to Dan Carpenter <dan.carpenter@oracle.com> who helped with
> > his review of the RFC Patch.
> >
> > [...]
> >

Dear Phil,

> 
> Dear Fabio,
> 
> Built and tested on my USB-N10 Nano, working well. In terms of how
> you've split the patches out, I have no problem with it personally,

I guess that Dan will disagree with us :) Did you read his last message?

I hope that he has time to review these patches. He expressed some doubts 
about splitting the changes in two separate patches. As far as I know, since 
Dan is a very experienced engineer (I am not even graduated and everything I 
know of Computer Science is self-taught), I could have been wrong in doing 
this work the way I did.

> given that one semaphore was there for kthread start/stop and the other
> for queuing. Looks good to me anyway based on what I know of completion
> variables :-) I assume you've not made the waits killable or
> interruptible in patch 1 due to the fact they are specifically related
> to kthread start/stop?

Good question! :)

Let me explain how I chose to make one wait killable and the other 
uninterruptible.

As far as I know, waiters may spin or sleep while waiting to acquire a lock  
(see spinlocks or mutexes for instance) or to be awakened (completions and 
condition variables for instance).

These were the cases of sleeping waiters. Sleeping can be done in 
uninterruptible, interruptible / killable, and timed-out states.

Where I'm 100% sure that the code doesn't require / want to be interrupted 
for whatever reason I prefer to use uninterruptible variants (and so I did in 
1/3).

When I'm not sure of the requirement above, I prefer to avoid that the 
process or the entire system hangs while waiting to acquire a mutex or to be 
awakened by a complete() (and so on).

Conversely, using interruptible versions without proper checking of return 
codes and without proper managing of errors may lead to serious bugs.

Kernel threads (kthreads) are like user processes / threads and are scheduled 
the same way the former are. One noteworthy difference is that their mm 
pointer is NULL (they have not an userspace address spaces). However they are 
still threads that have a PID in userspace and they can be killed by root.

This is the output of the "ps -ef" command after "modprobe r8188eu":

localhost:~ # ps -ef | grep RTW
root      1726     2  0 19:06 ?        00:00:00 [RTW_CMD_THREAD]

Since the developers who wrote the original code thought that that thread 
must be interrupted I thought that restricting interruptions to kills was the 
wiser choice in 2/3. Conversely, I cannot see reasons to interrupt the core 
part of a driver, so I chose to use an uninterruptible version of 
wait_for_completion*() in the other parts of the code.

I warned you that I'm not an engineer, so please double check my argument :)

> Anyhow:
> 
> For whole series:
> Acked-by: Phillip Potter <phil@philpotter.co.uk>

Thanks for you ack. I really appreciated it.

Best regards,

Fabio

> 
> Regards,
> Phil
> 

Re: [PATCH 0/3] staging: r8188eu: use completions and clean rtw_cmd_thread()

[Phillip Potter]

On Sat, Oct 16, 2021 at 07:54:51PM +0200, Fabio M. De Francesco wrote:
> I guess that Dan will disagree with us :) Did you read his last message?
> 
> I hope that he has time to review these patches. He expressed some doubts 
> about splitting the changes in two separate patches. As far as I know, since 
> Dan is a very experienced engineer (I am not even graduated and everything I 
> know of Computer Science is self-taught), I could have been wrong in doing 
> this work the way I did.
> 

I did read it yes, he makes good points, and my motivation is simply
that the patches look fine as they are to me personally :-)

> > given that one semaphore was there for kthread start/stop and the other
> > for queuing. Looks good to me anyway based on what I know of completion
> > variables :-) I assume you've not made the waits killable or
> > interruptible in patch 1 due to the fact they are specifically related
> > to kthread start/stop?
> 
> Good question! :)
> 
> Let me explain how I chose to make one wait killable and the other 
> uninterruptible.
> 
> As far as I know, waiters may spin or sleep while waiting to acquire a lock  
> (see spinlocks or mutexes for instance) or to be awakened (completions and 
> condition variables for instance).
> 
> These were the cases of sleeping waiters. Sleeping can be done in 
> uninterruptible, interruptible / killable, and timed-out states.
> 
> Where I'm 100% sure that the code doesn't require / want to be interrupted 
> for whatever reason I prefer to use uninterruptible variants (and so I did in 
> 1/3).
> 
> When I'm not sure of the requirement above, I prefer to avoid that the 
> process or the entire system hangs while waiting to acquire a mutex or to be 
> awakened by a complete() (and so on).
> 
> Conversely, using interruptible versions without proper checking of return 
> codes and without proper managing of errors may lead to serious bugs.
> 
> Kernel threads (kthreads) are like user processes / threads and are scheduled 
> the same way the former are. One noteworthy difference is that their mm 
> pointer is NULL (they have not an userspace address spaces). However they are 
> still threads that have a PID in userspace and they can be killed by root.
> 
> This is the output of the "ps -ef" command after "modprobe r8188eu":
> 
> localhost:~ # ps -ef | grep RTW
> root      1726     2  0 19:06 ?        00:00:00 [RTW_CMD_THREAD]
> 
> Since the developers who wrote the original code thought that that thread 
> must be interrupted I thought that restricting interruptions to kills was the 
> wiser choice in 2/3. Conversely, I cannot see reasons to interrupt the core 
> part of a driver, so I chose to use an uninterruptible version of 
> wait_for_completion*() in the other parts of the code.
> 
> I warned you that I'm not an engineer, so please double check my argument :)
> 

Sounds good to me, just wanted to know your reasoning.

> > Anyhow:
> > 
> > For whole series:
> > Acked-by: Phillip Potter <phil@philpotter.co.uk>
> 
> Thanks for you ack. I really appreciated it.
> 

You're welcome :-)

Regards,
Phil