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

[Phillip Potter <phil@philpotter.co.uk>]

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