Re: [PATCH v3] staging: rtl8712: Use completions for signaling

["Fabio M. De Francesco" <fmdefrancesco@gmail.com>]

On mercoledì 23 marzo 2022 05:55:15 CET Sathish Kumar wrote:
> r8712_sitesurvey_cmd() uses a variable to notify r8712_SetFilter() that it
> has completed operation. There is no sort of assurance that the variable will
> actually change and it could cache the value the first time it is read and
> then never update it for the whole loop logic.
> 
> Use completion variables because they are better suited for the purpose.
> 
> This patch fixes the checkpatch.pl warnings like:
> CHECK: Avoid CamelCase: <blnEnableRxFF0Filter>
> +   u8 blnEnableRxFF0Filter;
> 
> Signed-off-by: Sathish Kumar <skumark1902@gmail.com>
> ---
> Changes in v2:
>   - Remove the "bln" prefix
> 
> Changes in v3:
>   - Replace the variable used for signaling with completion
> ---
>  drivers/staging/rtl8712/drv_types.h   | 3 +--
>  drivers/staging/rtl8712/rtl871x_cmd.c | 2 +-
>  drivers/staging/rtl8712/usb_intf.c    | 2 +-
>  drivers/staging/rtl8712/xmit_linux.c  | 8 +-------
>  4 files changed, 4 insertions(+), 11 deletions(-)
> 
Hi Sathish,

First of all, I must admit that you have impressed me :)

Aside from the specific code of this driver that I don't know and aside
from the specific problem that you've been suggested to fix, I see that
you have been able to research and understand the subjects that Greg and 
I talked to you about.

By reading what Greg wrote soon after me, I believe that he was expecting
a slight different solution. I suggest you to read carefully what he writes
rather than what I write, just because he has at least 20 or more years 
of experience than me and because I'm just a spare time type of kernel 
developer. 

You chose to use wait_for_completion() / complete() (notice that these API 
use barriers indirectly). What I can say is that they look to be suited for 
solving the issues that you have here, even if there are other approaches.

Since, as said, it looks like you have understood how to select and use the 
one of the API that were suggested, you have my...

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

Thanks,

Fabio