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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> Thanks for you ack. I really appreciated it. Best regards, Fabio > > Regards, > Phil >
