[PATCH 0/3] staging: r8188eu: clean up osdep_service.h

[PATCH 0/3] staging: r8188eu: clean up osdep_service.h

[Martin Kaiser]

Remove unused code. Don't allow user space to kill our cmd thread.

This should be applied on top of "staging: r8188eu: clean up the
Makefile".

Martin Kaiser (3):
  staging: r8188eu: res_to_status is unused
  staging: r8188eu: daemonize is not defined
  staging: r8188eu: don't accept SIGTERM for cmd thread

 drivers/staging/r8188eu/core/rtw_cmd.c          |  2 --
 drivers/staging/r8188eu/include/osdep_service.h | 13 -------------
 2 files changed, 15 deletions(-)

-- 
2.20.1

[PATCH 1/3] staging: r8188eu: res_to_status is unused

[Martin Kaiser]

The function res_to_status is not used. Remove it.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/include/osdep_service.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/staging/r8188eu/include/osdep_service.h b/drivers/staging/r8188eu/include/osdep_service.h
index 4622ce400e41..d148137adf39 100644
--- a/drivers/staging/r8188eu/include/osdep_service.h
+++ b/drivers/staging/r8188eu/include/osdep_service.h
@@ -174,11 +174,6 @@ static inline void flush_signals_thread(void)
 		flush_signals(current);
 }
 
-static inline int res_to_status(int res)
-{
-	return res;
-}
-
 #define _RND(sz, r) ((((sz)+((r)-1))/(r))*(r))
 #define RND4(x)	(((x >> 2) + (((x & 3) == 0) ?  0: 1)) << 2)
 
-- 
2.20.1

Re: [PATCH 1/3] staging: r8188eu: res_to_status is unused

[Fabio M. De Francesco]

On Saturday, October 16, 2021 8:13:41 PM CEST Martin Kaiser wrote:
> The function res_to_status is not used. Remove it.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/staging/r8188eu/include/osdep_service.h | 5 -----
>  1 file changed, 5 deletions(-)

Acked-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Regards,

Fabio

Re: [PATCH 1/3] staging: r8188eu: res_to_status is unused

[Michael Straube]

On 10/16/21 20:13, Martin Kaiser wrote:
> The function res_to_status is not used. Remove it.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>   drivers/staging/r8188eu/include/osdep_service.h | 5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/include/osdep_service.h b/drivers/staging/r8188eu/include/osdep_service.h
> index 4622ce400e41..d148137adf39 100644
> --- a/drivers/staging/r8188eu/include/osdep_service.h
> +++ b/drivers/staging/r8188eu/include/osdep_service.h
> @@ -174,11 +174,6 @@ static inline void flush_signals_thread(void)
>   		flush_signals(current);
>   }
>   
> -static inline int res_to_status(int res)
> -{
> -	return res;
> -}
> -
>   #define _RND(sz, r) ((((sz)+((r)-1))/(r))*(r))
>   #define RND4(x)	(((x >> 2) + (((x & 3) == 0) ?  0: 1)) << 2)
>   
> 

Acked-by: Michael Straube <straube.linux@gmail.com>

[PATCH 2/3] staging: r8188eu: daemonize is not defined

[Martin Kaiser]

Remove dead code that depends on daemonize.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/include/osdep_service.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/r8188eu/include/osdep_service.h b/drivers/staging/r8188eu/include/osdep_service.h
index d148137adf39..ee8a64bb3126 100644
--- a/drivers/staging/r8188eu/include/osdep_service.h
+++ b/drivers/staging/r8188eu/include/osdep_service.h
@@ -162,9 +162,6 @@ static inline unsigned char _cancel_timer_ex(struct timer_list *ptimer)
 
 static __inline void thread_enter(char *name)
 {
-#ifdef daemonize
-	daemonize("%s", name);
-#endif
 	allow_signal(SIGTERM);
 }
 
-- 
2.20.1

Re: [PATCH 2/3] staging: r8188eu: daemonize is not defined

[Fabio M. De Francesco]

On Saturday, October 16, 2021 8:13:42 PM CEST Martin Kaiser wrote:
> Remove dead code that depends on daemonize.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/staging/r8188eu/include/osdep_service.h | 3 ---
>  1 file changed, 3 deletions(-)

Acked-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Regards,

Fabio

Re: [PATCH 2/3] staging: r8188eu: daemonize is not defined

[Michael Straube]

On 10/16/21 20:13, Martin Kaiser wrote:
> Remove dead code that depends on daemonize.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>   drivers/staging/r8188eu/include/osdep_service.h | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/include/osdep_service.h b/drivers/staging/r8188eu/include/osdep_service.h
> index d148137adf39..ee8a64bb3126 100644
> --- a/drivers/staging/r8188eu/include/osdep_service.h
> +++ b/drivers/staging/r8188eu/include/osdep_service.h
> @@ -162,9 +162,6 @@ static inline unsigned char _cancel_timer_ex(struct timer_list *ptimer)
>   
>   static __inline void thread_enter(char *name)
>   {
> -#ifdef daemonize
> -	daemonize("%s", name);
> -#endif
>   	allow_signal(SIGTERM);
>   }
>   
> 

Acked-by: Michael Straube <straube.linux@gmail.com>

[PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread

[Martin Kaiser]

At the moment, our command thread can be killed by user space.

[root@host ]# kill `pidof RTW_CMD_THREAD`

The driver will then stop working until the module is unloaded
and reloaded.

Don't process SIGTERM in the command thread. Other drivers that have a
command thread don't process SIGTERM either.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/core/rtw_cmd.c          | 2 --
 drivers/staging/r8188eu/include/osdep_service.h | 5 -----
 2 files changed, 7 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_cmd.c b/drivers/staging/r8188eu/core/rtw_cmd.c
index e17332677daa..b834fac41627 100644
--- a/drivers/staging/r8188eu/core/rtw_cmd.c
+++ b/drivers/staging/r8188eu/core/rtw_cmd.c
@@ -243,8 +243,6 @@ int rtw_cmd_thread(void *context)
 	struct adapter *padapter = (struct adapter *)context;
 	struct cmd_priv *pcmdpriv = &padapter->cmdpriv;
 
-	thread_enter("RTW_CMD_THREAD");
-
 	pcmdbuf = pcmdpriv->cmd_buf;
 
 	pcmdpriv->cmdthd_running = true;
diff --git a/drivers/staging/r8188eu/include/osdep_service.h b/drivers/staging/r8188eu/include/osdep_service.h
index ee8a64bb3126..886a1b6f30b4 100644
--- a/drivers/staging/r8188eu/include/osdep_service.h
+++ b/drivers/staging/r8188eu/include/osdep_service.h
@@ -160,11 +160,6 @@ static inline unsigned char _cancel_timer_ex(struct timer_list *ptimer)
 	return del_timer_sync(ptimer);
 }
 
-static __inline void thread_enter(char *name)
-{
-	allow_signal(SIGTERM);
-}
-
 static inline void flush_signals_thread(void)
 {
 	if (signal_pending (current))
-- 
2.20.1

Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread

[Fabio M. De Francesco]

On Saturday, October 16, 2021 8:13:43 PM CEST Martin Kaiser wrote:
> At the moment, our command thread can be killed by user space.
> 
> [root@host ]# kill `pidof RTW_CMD_THREAD`
> 
> The driver will then stop working until the module is unloaded
> and reloaded.
> 
> Don't process SIGTERM in the command thread. Other drivers that have a
> command thread don't process SIGTERM either.

Hi Martin,

This is _really_ interesting :)

May be that you have had time to read my last email in reply to a message of 
Phillip P. Soon after writing of the arguments in favor of using 
wait_for_completion_killable() (in patch 2/3 of the series I sent today), I 
read your patch.

If you are right (and I think you are) I'll have to send a v2 that replaces 
the killable wait with an uninterruptible one.

Unfortunately I have not the needed experience to decide whether or not to 
ack your patch, even if I'm strongly tempted to do it.

Let's wait for more experienced people.

Thanks,

Fabio 

> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/staging/r8188eu/core/rtw_cmd.c          | 2 --
>  drivers/staging/r8188eu/include/osdep_service.h | 5 -----
>  2 files changed, 7 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_cmd.c b/drivers/staging/
r8188eu/core/rtw_cmd.c
> index e17332677daa..b834fac41627 100644
> --- a/drivers/staging/r8188eu/core/rtw_cmd.c
> +++ b/drivers/staging/r8188eu/core/rtw_cmd.c
> @@ -243,8 +243,6 @@ int rtw_cmd_thread(void *context)
>  	struct adapter *padapter = (struct adapter *)context;
>  	struct cmd_priv *pcmdpriv = &padapter->cmdpriv;
>  
> -	thread_enter("RTW_CMD_THREAD");
> -
>  	pcmdbuf = pcmdpriv->cmd_buf;
>  
>  	pcmdpriv->cmdthd_running = true;
> diff --git a/drivers/staging/r8188eu/include/osdep_service.h b/drivers/
staging/r8188eu/include/osdep_service.h
> index ee8a64bb3126..886a1b6f30b4 100644
> --- a/drivers/staging/r8188eu/include/osdep_service.h
> +++ b/drivers/staging/r8188eu/include/osdep_service.h
> @@ -160,11 +160,6 @@ static inline unsigned char _cancel_timer_ex(struct 
timer_list *ptimer)
>  	return del_timer_sync(ptimer);
>  }
>  
> -static __inline void thread_enter(char *name)
> -{
> -	allow_signal(SIGTERM);
> -}
> -
>  static inline void flush_signals_thread(void)
>  {
>  	if (signal_pending (current))
> -- 
> 2.20.1
> 
> 
> 

Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread

[Phillip Potter]

On Sat, Oct 16, 2021 at 08:53:15PM +0200, Fabio M. De Francesco wrote:
> On Saturday, October 16, 2021 8:13:43 PM CEST Martin Kaiser wrote:
> > At the moment, our command thread can be killed by user space.
> > 
> > [root@host ]# kill `pidof RTW_CMD_THREAD`
> > 
> > The driver will then stop working until the module is unloaded
> > and reloaded.
> > 
> > Don't process SIGTERM in the command thread. Other drivers that have a
> > command thread don't process SIGTERM either.
> 
> Hi Martin,
> 
> This is _really_ interesting :)
> 
> May be that you have had time to read my last email in reply to a message of 
> Phillip P. Soon after writing of the arguments in favor of using 
> wait_for_completion_killable() (in patch 2/3 of the series I sent today), I 
> read your patch.
> 
> If you are right (and I think you are) I'll have to send a v2 that replaces 
> the killable wait with an uninterruptible one.
> 
> Unfortunately I have not the needed experience to decide whether or not to 
> ack your patch, even if I'm strongly tempted to do it.
> 
> Let's wait for more experienced people.
> 
> Thanks,
> 
> Fabio 
> 

So I myself am a little confused on this one :-)

Based on my understanding, so correct me if I'm wrong, a process (kthread or
otherwise) can still be killed if marked TASK_KILLABLE, even if ignoring
SIGTERM. Indeed, from a userspace perspective, SIGKILL is unblockable
anyway - although of course kernel code can choose how to respond to it.

So in other words, the kthread could still be killed while waiting
in the wait_for_completion_killable() call, even if we are ignoring
SIGTERM. From that perspective I guess, it is therefore not 'incorrect' as
such - if indeed we wanted that behaviour.

That said, killing it would still cause the behaviour Martin mentions -
I guess we don't want it to be either killable or interruptible based on
that logic?

Regards,
Phil

Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread

[Fabio M. De Francesco]

On Sunday, October 17, 2021 12:29:02 PM CEST Phillip Potter wrote:
> On Sat, Oct 16, 2021 at 08:53:15PM +0200, Fabio M. De Francesco wrote:
> > On Saturday, October 16, 2021 8:13:43 PM CEST Martin Kaiser wrote:
> > > At the moment, our command thread can be killed by user space.
> > > 
> > > [root@host ]# kill `pidof RTW_CMD_THREAD`
> > > 
> > > The driver will then stop working until the module is unloaded
> > > and reloaded.
> > > 
> > > Don't process SIGTERM in the command thread. Other drivers that have a
> > > command thread don't process SIGTERM either.
> > 
> > Hi Martin,
> > 
> > This is _really_ interesting :)
> > 
> > May be that you have had time to read my last email in reply to a message 
of 
> > Phillip P. Soon after writing of the arguments in favor of using 
> > wait_for_completion_killable() (in patch 2/3 of the series I sent today), 
I 
> > read your patch.
> > 
> > If you are right (and I think you are) I'll have to send a v2 that 
replaces 
> > the killable wait with an uninterruptible one.
> > 
> > Unfortunately I have not the needed experience to decide whether or not 
to 
> > ack your patch, even if I'm strongly tempted to do it.
> > 
> > Let's wait for more experienced people.
> > 
> > Thanks,
> > 
> > Fabio 
> > 
> 
> So I myself am a little confused on this one :-)
> 
> Based on my understanding, so correct me if I'm wrong, a process (kthread 
or
> otherwise) can still be killed if marked TASK_KILLABLE, even if ignoring
> SIGTERM. Indeed, from a userspace perspective, SIGKILL is unblockable
> anyway - although of course kernel code can choose how to respond to it.

Correct.

> 
> So in other words, the kthread could still be killed while waiting
> in the wait_for_completion_killable() call, even if we are ignoring
> SIGTERM. From that perspective I guess, it is therefore not 'incorrect' as
> such - if indeed we wanted that behaviour.

No. This misunderstandings is my fault. :(

In Martin's patch I read "SIGTERM" but for some reason I thought he was 
talking of "SIGKILL".

At the moment, without Martin's patch, the kthread can be terminated by the 
command "kill -TERM <PID>". If we try "kill -KILL <PID>", nothing happens.
This is because only "allow_signal(SIGTERM);" is present in the code.

I think that kthreads must also allow  SIGKILL with "allow_signal(SIGKILL);" 
for allowing root to make them terminate.

For what relates to my patch, it doesn't matter if I either leave 
wait_for_completion_killable() as-is or change it to wait_for_completion().
This is because at the moment SIGKILL cannot kill rtw_cmd_thread(), while 
SIGTERM can.

However, for consistency, I should better change it to the uninterruptible 
version.

@Martin: Please correct me if I'm wrong. Thanks.

Regards,

Fabio

> 
> That said, killing it would still cause the behaviour Martin mentions -
> I guess we don't want it to be either killable or interruptible based on
> that logic?
> 
> Regards,
> Phil
> 

Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread

[Fabio M. De Francesco]

On Sunday, October 17, 2021 2:51:40 PM CEST Fabio M. De Francesco wrote:
> On Sunday, October 17, 2021 12:29:02 PM CEST Phillip Potter wrote:
>
> 
> Correct.

Please disregard this second mail. It is a duplicate due to some weird KMail 
crashes.

Fabio

Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread

[Martin Kaiser]

Hi Fabio and all,

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

> On Sunday, October 17, 2021 12:29:02 PM CEST Phillip Potter wrote:

> > So I myself am a little confused on this one :-)

> > Based on my understanding, so correct me if I'm wrong, a process
> > (kthread or otherwise) can still be killed if marked TASK_KILLABLE,
> > even if ignoring SIGTERM. Indeed, from a userspace perspective,
> > SIGKILL is unblockable anyway - although of course kernel code can
> > choose how to respond to it.

> Correct.

And it seems that by default, a kthread can't be killed with SIGKILL.

> > So in other words, the kthread could still be killed while waiting
> > in the wait_for_completion_killable() call, even if we are ignoring
> > SIGTERM. From that perspective I guess, it is therefore not 'incorrect' as
> > such - if indeed we wanted that behaviour.

> No. This misunderstandings is my fault. :(

> In Martin's patch I read "SIGTERM" but for some reason I thought he was 
> talking of "SIGKILL".

> At the moment, without Martin's patch, the kthread can be terminated by the 
> command "kill -TERM <PID>". If we try "kill -KILL <PID>", nothing happens.
> This is because only "allow_signal(SIGTERM);" is present in the code.

Exactly. And this is probably not by intention. It would be consistent
to either allow both or none - the latter makes more sense, and it's
what most other drivers do.

> I think that kthreads must also allow  SIGKILL with "allow_signal(SIGKILL);" 
> for allowing root to make them terminate.

Probably. nfsd seems to do this.

> For what relates to my patch, it doesn't matter if I either leave 
> wait_for_completion_killable() as-is or change it to wait_for_completion().
> This is because at the moment SIGKILL cannot kill rtw_cmd_thread(), while 
> SIGTERM can.

> However, for consistency, I should better change it to the uninterruptible 
> version.

That makes sense to me.

Let's see what Greg and others say...

Best regards,
Martin

Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread

[Phillip Potter]

On Sun, Oct 17, 2021 at 08:02:37PM +0200, Martin Kaiser wrote:
> Hi Fabio and all,
> 
> Thus wrote Fabio M. De Francesco (fmdefrancesco@gmail.com):
> 
> > On Sunday, October 17, 2021 12:29:02 PM CEST Phillip Potter wrote:
> 
> > > So I myself am a little confused on this one :-)
> 
> > > Based on my understanding, so correct me if I'm wrong, a process
> > > (kthread or otherwise) can still be killed if marked TASK_KILLABLE,
> > > even if ignoring SIGTERM. Indeed, from a userspace perspective,
> > > SIGKILL is unblockable anyway - although of course kernel code can
> > > choose how to respond to it.
> 
> > Correct.
> 
> And it seems that by default, a kthread can't be killed with SIGKILL.
> 

Ah, makes sense. This was a misconception on my part, apologies :-)

> > > So in other words, the kthread could still be killed while waiting
> > > in the wait_for_completion_killable() call, even if we are ignoring
> > > SIGTERM. From that perspective I guess, it is therefore not 'incorrect' as
> > > such - if indeed we wanted that behaviour.
> 
> > No. This misunderstandings is my fault. :(
> 
> > In Martin's patch I read "SIGTERM" but for some reason I thought he was 
> > talking of "SIGKILL".
> 
> > At the moment, without Martin's patch, the kthread can be terminated by the 
> > command "kill -TERM <PID>". If we try "kill -KILL <PID>", nothing happens.
> > This is because only "allow_signal(SIGTERM);" is present in the code.
> 
> Exactly. And this is probably not by intention. It would be consistent
> to either allow both or none - the latter makes more sense, and it's
> what most other drivers do.
> 
> > I think that kthreads must also allow  SIGKILL with "allow_signal(SIGKILL);" 
> > for allowing root to make them terminate.
> 
> Probably. nfsd seems to do this.
> 
> > For what relates to my patch, it doesn't matter if I either leave 
> > wait_for_completion_killable() as-is or change it to wait_for_completion().
> > This is because at the moment SIGKILL cannot kill rtw_cmd_thread(), while 
> > SIGTERM can.
> 
> > However, for consistency, I should better change it to the uninterruptible 
> > version.
> 
> That makes sense to me.
> 
> Let's see what Greg and others say...

I've found this discussion to be most enlightening :-) I certainly agree
that consistency in approach is a good thing, so perhaps
wait_for_completion() is a better choice therefore, despite it having
the same semantics in this instance due to us not allowing SIGKILL by
default anyway. Others will know far more than me in this regard though.

Regards,
Phil

Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread

[Fabio M. De Francesco]

On Sunday, October 17, 2021 12:29:02 PM CEST Phillip Potter wrote:
> On Sat, Oct 16, 2021 at 08:53:15PM +0200, Fabio M. De Francesco wrote:
> > On Saturday, October 16, 2021 8:13:43 PM CEST Martin Kaiser wrote:
> > > At the moment, our command thread can be killed by user space.
> > > 
> > > [root@host ]# kill `pidof RTW_CMD_THREAD`
> > > 
> > > The driver will then stop working until the module is unloaded
> > > and reloaded.
> > > 
> > > Don't process SIGTERM in the command thread. Other drivers that have a
> > > command thread don't process SIGTERM either.
> > 
> > Hi Martin,
> > 
> > This is _really_ interesting :)
> > 
> > May be that you have had time to read my last email in reply to a message 
of 
> > Phillip P. Soon after writing of the arguments in favor of using 
> > wait_for_completion_killable() (in patch 2/3 of the series I sent today), 
I 
> > read your patch.
> > 
> > If you are right (and I think you are) I'll have to send a v2 that 
replaces 
> > the killable wait with an uninterruptible one.
> > 
> > Unfortunately I have not the needed experience to decide whether or not 
to 
> > ack your patch, even if I'm strongly tempted to do it.
> > 
> > Let's wait for more experienced people.
> > 
> > Thanks,
> > 
> > Fabio 
> > 
> 
> So I myself am a little confused on this one :-)
> 
> Based on my understanding, so correct me if I'm wrong, a process (kthread 
or
> otherwise) can still be killed if marked TASK_KILLABLE, even if ignoring
> SIGTERM. Indeed, from a userspace perspective, SIGKILL is unblockable
> anyway - although of course kernel code can choose how to respond to it.

@Phil, Correct.
@Martin, Please correct me if I'm missing something in what follows...

> So in other words, the kthread could still be killed while waiting
> in the wait_for_completion_killable() call, even if we are ignoring
> SIGTERM. 

No, this confusion is my fault.

I read Martin's patch, but in my mind I exchanged "SIGTERM" with "SIGKILL".

At this moment, without Martin's patch, only SIGTERM is delivered to the 
kthread. This is due to the line "allow_signal(SIGTERM);".

If we try to kill the kthread with "kill -KILL <PID>", nothing happens. 
Instead if we use "kill -TERM <PID>", the kthread terminates.

For what is related to my code, there is no functional changes between using 
the killable or the uninterruptible version (I guess). But for sake of 
consistency, since SIGKILL is not allowed, I should use either 
wait_for_completion_interruptible() (without Martin's patch) or 
wait_for_completion() (with Martin's patch).

However, I re-iterate that, since SIGKILL is not allowed in the current code, 
"kill -KILL <PID>" has no effect at all and the wait is not interruptible 
with my killable version of the wait.

> From that perspective I guess, it is therefore not 'incorrect' as
> such - if indeed we wanted that behaviour.
> 
> That said, killing it would still cause the behaviour Martin mentions -
> I guess we don't want it to be either killable or interruptible based on
> that logic?

Yes, I agree. I should replace the killable version with the uninterruptible 
one.

Thanks,

Fabio

> 
> Regards,
> Phil
> 

Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread

[Fabio M. De Francesco]

On Sunday, October 17, 2021 3:14:49 PM CEST Fabio M. De Francesco wrote:
> On Sunday, October 17, 2021 12:29:02 PM CEST Phillip Potter wrote:

Please disregard this email. It's a duplicate due to some weird crashes of 
KMail.

Fabio

> @Phil, Correct.
> @Martin, Please correct me if I'm missing something in what follows...

Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread

[Fabio M. De Francesco]

On Sunday, October 17, 2021 12:29:02 PM CEST Phillip Potter wrote:
> On Sat, Oct 16, 2021 at 08:53:15PM +0200, Fabio M. De Francesco wrote:
> > On Saturday, October 16, 2021 8:13:43 PM CEST Martin Kaiser wrote:
> > > At the moment, our command thread can be killed by user space.
> > > 
> > > [root@host ]# kill `pidof RTW_CMD_THREAD`
> > > 
> > > The driver will then stop working until the module is unloaded
> > > and reloaded.
> > > 
> > > Don't process SIGTERM in the command thread. Other drivers that have a
> > > command thread don't process SIGTERM either.
> > 
> > Hi Martin,
> > 
> > This is _really_ interesting :)
> > 
> > May be that you have had time to read my last email in reply to a message 
of 
> > Phillip P. Soon after writing of the arguments in favor of using 
> > wait_for_completion_killable() (in patch 2/3 of the series I sent today), 
I 
> > read your patch.
> > 
> > If you are right (and I think you are) I'll have to send a v2 that 
replaces 
> > the killable wait with an uninterruptible one.
> > 
> > Unfortunately I have not the needed experience to decide whether or not 
to 
> > ack your patch, even if I'm strongly tempted to do it.
> > 
> > Let's wait for more experienced people.
> > 
> > Thanks,
> > 
> > Fabio 
> > 
> 
> So I myself am a little confused on this one :-)
> 
> Based on my understanding, so correct me if I'm wrong, a process (kthread 
or
> otherwise) can still be killed if marked TASK_KILLABLE, even if ignoring
> SIGTERM. Indeed, from a userspace perspective, SIGKILL is unblockable
> anyway - although of course kernel code can choose how to respond to it.
> 

@Phil, Correct: the kernel can choose how to respond to signals.
@Martin, Please correct me if I'm missing something in what follows...

> So in other words, the kthread could still be killed while waiting
> in the wait_for_completion_killable() call, even if we are ignoring
> SIGTERM. 

No, this confusion is my fault.

I read Martin's patch, but in my mind I exchanged "SIGTERM" with "SIGKILL".

At this moment, without Martin's patch, only SIGTERM is delivered to the 
kthread. This is due to the line "allow_signal(SIGTERM);".

If we try to kill the kthread with "kill -KILL <PID>", nothing happens. 
Instead if we use "kill -TERM <PID>", the kthread terminates.

For what is related to my code, there is no functional changes between using 
the killable or the uninterruptible version (I guess). But for sake of 
consistency, since SIGKILL is not allowed, I should use either 
wait_for_completion_interruptible() (without Martin's patch) or 
wait_for_completion() (with Martin's patch).

However, I re-iterate that, since SIGKILL is not allowed in the current code, 
"kill -KILL <PID>" has no effect at all and the wait is not interruptible 
with my killable version of the wait.

> From that perspective I guess, it is therefore not 'incorrect' as
> such - if indeed we wanted that behaviour.
> 
> That said, killing it would still cause the behaviour Martin mentions -
> I guess we don't want it to be either killable or interruptible based on
> that logic?

Yes, I agree. I should replace the killable version with the uninterruptible 
one.

Thanks,

Fabio

> 
> Regards,
> Phil
> 

Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread

[Fabio M. De Francesco]

On Saturday, October 16, 2021 8:13:43 PM CEST Martin Kaiser wrote:
> At the moment, our command thread can be killed by user space.
> 
> [root@host ]# kill `pidof RTW_CMD_THREAD`
> 
> The driver will then stop working until the module is unloaded
> and reloaded.
> 
> Don't process SIGTERM in the command thread. Other drivers that have a
> command thread don't process SIGTERM either.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/staging/r8188eu/core/rtw_cmd.c          | 2 --
>  drivers/staging/r8188eu/include/osdep_service.h | 5 -----
>  2 files changed, 7 deletions(-)

Dear Martin,

After thinking a little more about this topic, now I'm ready for...

Acked-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Regards,

Fabio