linux-staging.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] staging: r8188eu: use completions and clean rtw_cmd_thread()
@ 2021-10-17 14:28 Fabio M. De Francesco
  2021-10-17 14:28 ` [PATCH v2 1/3] staging: r8188eu: Use completions for signaling start / end kthread Fabio M. De Francesco
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Fabio M. De Francesco @ 2021-10-17 14:28 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Dan Carpenter,
	Martin Kaiser, linux-staging, linux-kernel
  Cc: 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.

v1 => v2:
	Patch 1/3: No changes;
	Patch 2/3: Replace wait_for_completion_killable() with
		   wait_for_completion() because killing the kthread is
		   not allowed and so there is no need for killable
		   wait;
	Patch 3/3: No changes.

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

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

-- 
2.33.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 1/3] staging: r8188eu: Use completions for signaling start / end kthread
  2021-10-17 14:28 [PATCH v2 0/3] staging: r8188eu: use completions and clean rtw_cmd_thread() Fabio M. De Francesco
@ 2021-10-17 14:28 ` Fabio M. De Francesco
  2021-10-17 14:28 ` [PATCH v2 2/3] staging: r8188eu: Use completions for signaling enqueueing Fabio M. De Francesco
  2021-10-17 14:28 ` [PATCH 3/3] staging: r8188eu: Remove redundant 'if' statement Fabio M. De Francesco
  2 siblings, 0 replies; 7+ messages in thread
From: Fabio M. De Francesco @ 2021-10-17 14:28 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Dan Carpenter,
	Martin Kaiser, linux-staging, linux-kernel
  Cc: 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]".

Acked-by: Phillip Potter <phil@philpotter.co.uk>
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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 2/3] staging: r8188eu: Use completions for signaling enqueueing
  2021-10-17 14:28 [PATCH v2 0/3] staging: r8188eu: use completions and clean rtw_cmd_thread() Fabio M. De Francesco
  2021-10-17 14:28 ` [PATCH v2 1/3] staging: r8188eu: Use completions for signaling start / end kthread Fabio M. De Francesco
@ 2021-10-17 14:28 ` Fabio M. De Francesco
  2021-10-17 14:28 ` [PATCH 3/3] staging: r8188eu: Remove redundant 'if' statement Fabio M. De Francesco
  2 siblings, 0 replies; 7+ messages in thread
From: Fabio M. De Francesco @ 2021-10-17 14:28 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Dan Carpenter,
	Martin Kaiser, linux-staging, linux-kernel
  Cc: 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 uninterruptible mode, even if the original
code uses down_interruptible(), because the interruption of
rtw_cmd_thread() is not allowed and unwanted.

Acked-by: Phillip Potter <phil@philpotter.co.uk>
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 | 2 +-
 drivers/staging/r8188eu/os_dep/os_intfs.c | 2 +-
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_cmd.c b/drivers/staging/r8188eu/core/rtw_cmd.c
index 195390449502..6fb79d711692 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,8 +252,7 @@ int rtw_cmd_thread(void *context)
 	complete(&pcmdpriv->start_cmd_thread);
 
 	while (1) {
-		if (_rtw_down_sema(&pcmdpriv->cmd_queue_sema) == _FAIL)
-			break;
+		wait_for_completion(&pcmdpriv->enqueue_cmd);
 
 		if (padapter->bDriverStopped ||
 		    padapter->bSurpriseRemoved) {
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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 3/3] staging: r8188eu: Remove redundant 'if' statement
  2021-10-17 14:28 [PATCH v2 0/3] staging: r8188eu: use completions and clean rtw_cmd_thread() Fabio M. De Francesco
  2021-10-17 14:28 ` [PATCH v2 1/3] staging: r8188eu: Use completions for signaling start / end kthread Fabio M. De Francesco
  2021-10-17 14:28 ` [PATCH v2 2/3] staging: r8188eu: Use completions for signaling enqueueing Fabio M. De Francesco
@ 2021-10-17 14:28 ` Fabio M. De Francesco
  2021-10-18 14:42   ` Greg Kroah-Hartman
  2 siblings, 1 reply; 7+ messages in thread
From: Fabio M. De Francesco @ 2021-10-17 14:28 UTC (permalink / raw)
  To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, Dan Carpenter,
	Martin Kaiser, linux-staging, linux-kernel
  Cc: Fabio M. De Francesco

Remove a redundant 'if' statement.

Acked-by: Martin Kaiser <martin@kaiser.cx>
Acked-by: Phillip Potter <phil@philpotter.co.uk>
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 6fb79d711692..42084b029473 100644
--- a/drivers/staging/r8188eu/core/rtw_cmd.c
+++ b/drivers/staging/r8188eu/core/rtw_cmd.c
@@ -254,12 +254,6 @@ int rtw_cmd_thread(void *context)
 	while (1) {
 		wait_for_completion(&pcmdpriv->enqueue_cmd);
 
-		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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/3] staging: r8188eu: Remove redundant 'if' statement
  2021-10-17 14:28 ` [PATCH 3/3] staging: r8188eu: Remove redundant 'if' statement Fabio M. De Francesco
@ 2021-10-18 14:42   ` Greg Kroah-Hartman
  2021-10-18 16:28     ` Fabio M. De Francesco
  0 siblings, 1 reply; 7+ messages in thread
From: Greg Kroah-Hartman @ 2021-10-18 14:42 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Dan Carpenter, Martin Kaiser,
	linux-staging, linux-kernel

On Sun, Oct 17, 2021 at 04:28:12PM +0200, Fabio M. De Francesco wrote:
> Remove a redundant 'if' statement.
> 
> Acked-by: Martin Kaiser <martin@kaiser.cx>
> Acked-by: Phillip Potter <phil@philpotter.co.uk>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  drivers/staging/r8188eu/core/rtw_cmd.c | 6 ------
>  1 file changed, 6 deletions(-)

Why is there not a "v2" in the subject line like the other patches in
this series?

> 
> diff --git a/drivers/staging/r8188eu/core/rtw_cmd.c b/drivers/staging/r8188eu/core/rtw_cmd.c
> index 6fb79d711692..42084b029473 100644
> --- a/drivers/staging/r8188eu/core/rtw_cmd.c
> +++ b/drivers/staging/r8188eu/core/rtw_cmd.c
> @@ -254,12 +254,6 @@ int rtw_cmd_thread(void *context)
>  	while (1) {
>  		wait_for_completion(&pcmdpriv->enqueue_cmd);
>  
> -		if (padapter->bDriverStopped ||
> -		    padapter->bSurpriseRemoved) {
> -			DBG_88E("%s: DriverStopped(%d) SurpriseRemoved(%d) break at line %d\n",
> -				__func__, padapter->bDriverStopped, padapter->bSurpriseRemoved, __LINE__);
> -			break;
> -		}


Why is this redundant?

It is not obvious from the diff what is going on so you should say a bit
more in the changelog text please.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/3] staging: r8188eu: Remove redundant 'if' statement
  2021-10-18 14:42   ` Greg Kroah-Hartman
@ 2021-10-18 16:28     ` Fabio M. De Francesco
  2021-10-19  5:51       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 7+ messages in thread
From: Fabio M. De Francesco @ 2021-10-18 16:28 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Larry Finger, Phillip Potter, Dan Carpenter, Martin Kaiser,
	linux-staging, linux-kernel

On Monday, October 18, 2021 4:42:51 PM CEST Greg Kroah-Hartman wrote:
> On Sun, Oct 17, 2021 at 04:28:12PM +0200, Fabio M. De Francesco wrote:
> > Remove a redundant 'if' statement.
> > 
> > Acked-by: Martin Kaiser <martin@kaiser.cx>
> > Acked-by: Phillip Potter <phil@philpotter.co.uk>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> >  drivers/staging/r8188eu/core/rtw_cmd.c | 6 ------
> >  1 file changed, 6 deletions(-)
> 
> Why is there not a "v2" in the subject line like the other patches in
> this series?

Simply because I edited manually the "Subject" and overlooked to write "v2".

> 
> > 
> > diff --git a/drivers/staging/r8188eu/core/rtw_cmd.c b/drivers/staging/
r8188eu/core/rtw_cmd.c
> > index 6fb79d711692..42084b029473 100644
> > --- a/drivers/staging/r8188eu/core/rtw_cmd.c
> > +++ b/drivers/staging/r8188eu/core/rtw_cmd.c
> > @@ -254,12 +254,6 @@ int rtw_cmd_thread(void *context)
> >  	while (1) {
> >  		wait_for_completion(&pcmdpriv->enqueue_cmd);
> >  
> > -		if (padapter->bDriverStopped ||
> > -		    padapter->bSurpriseRemoved) {
> > -			DBG_88E("%s: DriverStopped(%d) 
SurpriseRemoved(%d) break at line %d\n",
> > -				__func__, padapter-
>bDriverStopped, padapter->bSurpriseRemoved, __LINE__);
> > -			break;
> > -		}
> 
> 
> Why is this redundant?
> 
> It is not obvious from the diff what is going on so you should say a bit
> more in the changelog text please.

Yes you are right. I wrongly thought that is was "obvious", but re-reading my 
own text I noticed that it is not. 

In 1/3 and 2/3 I was particularly careful in writing changelogs. Instead in 
3/3 I forgot that commit messages _must_ explain "what" and "why" :(

I've just sent version 3 of the series.

Thanks for reviewing my work,

Fabio

> 
> thanks,
> 
> greg k-h
> 





^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/3] staging: r8188eu: Remove redundant 'if' statement
  2021-10-18 16:28     ` Fabio M. De Francesco
@ 2021-10-19  5:51       ` Greg Kroah-Hartman
  0 siblings, 0 replies; 7+ messages in thread
From: Greg Kroah-Hartman @ 2021-10-19  5:51 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: Larry Finger, Phillip Potter, Dan Carpenter, Martin Kaiser,
	linux-staging, linux-kernel

On Mon, Oct 18, 2021 at 06:28:46PM +0200, Fabio M. De Francesco wrote:
> On Monday, October 18, 2021 4:42:51 PM CEST Greg Kroah-Hartman wrote:
> > On Sun, Oct 17, 2021 at 04:28:12PM +0200, Fabio M. De Francesco wrote:
> > > Remove a redundant 'if' statement.
> > > 
> > > Acked-by: Martin Kaiser <martin@kaiser.cx>
> > > Acked-by: Phillip Potter <phil@philpotter.co.uk>
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > ---
> > >  drivers/staging/r8188eu/core/rtw_cmd.c | 6 ------
> > >  1 file changed, 6 deletions(-)
> > 
> > Why is there not a "v2" in the subject line like the other patches in
> > this series?
> 
> Simply because I edited manually the "Subject" and overlooked to write "v2".

'git format-patchj -v 2' is your friend.


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-10-19  5:51 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-17 14:28 [PATCH v2 0/3] staging: r8188eu: use completions and clean rtw_cmd_thread() Fabio M. De Francesco
2021-10-17 14:28 ` [PATCH v2 1/3] staging: r8188eu: Use completions for signaling start / end kthread Fabio M. De Francesco
2021-10-17 14:28 ` [PATCH v2 2/3] staging: r8188eu: Use completions for signaling enqueueing Fabio M. De Francesco
2021-10-17 14:28 ` [PATCH 3/3] staging: r8188eu: Remove redundant 'if' statement Fabio M. De Francesco
2021-10-18 14:42   ` Greg Kroah-Hartman
2021-10-18 16:28     ` Fabio M. De Francesco
2021-10-19  5: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).