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

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

[Sathish Kumar]

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(-)

diff --git a/drivers/staging/rtl8712/drv_types.h b/drivers/staging/rtl8712/drv_types.h
index a44d04effc8b..76ac798642bd 100644
--- a/drivers/staging/rtl8712/drv_types.h
+++ b/drivers/staging/rtl8712/drv_types.h
@@ -157,12 +157,11 @@ struct _adapter {
 	struct iw_statistics iwstats;
 	int pid; /*process id from UI*/
 	struct work_struct wk_filter_rx_ff0;
-	u8 blnEnableRxFF0Filter;
-	spinlock_t lock_rx_ff0_filter;
 	const struct firmware *fw;
 	struct usb_interface *pusb_intf;
 	struct mutex mutex_start;
 	struct completion rtl8712_fw_ready;
+	struct completion rx_filter_ready;
 };
 
 static inline u8 *myid(struct eeprom_priv *peepriv)
diff --git a/drivers/staging/rtl8712/rtl871x_cmd.c b/drivers/staging/rtl8712/rtl871x_cmd.c
index acda930722b2..f5746020a1b7 100644
--- a/drivers/staging/rtl8712/rtl871x_cmd.c
+++ b/drivers/staging/rtl8712/rtl871x_cmd.c
@@ -202,7 +202,7 @@ u8 r8712_sitesurvey_cmd(struct _adapter *padapter,
 	mod_timer(&pmlmepriv->scan_to_timer,
 		  jiffies + msecs_to_jiffies(SCANNING_TIMEOUT));
 	padapter->ledpriv.LedControlHandler(padapter, LED_CTL_SITE_SURVEY);
-	padapter->blnEnableRxFF0Filter = 0;
+	complete(&padapter->rx_filter_ready);
 	return _SUCCESS;
 }
 
diff --git a/drivers/staging/rtl8712/usb_intf.c b/drivers/staging/rtl8712/usb_intf.c
index ee4c61f85a07..8df50e24f2b8 100644
--- a/drivers/staging/rtl8712/usb_intf.c
+++ b/drivers/staging/rtl8712/usb_intf.c
@@ -568,7 +568,7 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf,
 	/* step 6. Load the firmware asynchronously */
 	if (rtl871x_load_fw(padapter))
 		goto deinit_drv_sw;
-	spin_lock_init(&padapter->lock_rx_ff0_filter);
+	init_completion(&padapter->rx_filter_ready);
 	mutex_init(&padapter->mutex_start);
 	return 0;
 
diff --git a/drivers/staging/rtl8712/xmit_linux.c b/drivers/staging/rtl8712/xmit_linux.c
index 90d34cf9d2ff..4a93839bf947 100644
--- a/drivers/staging/rtl8712/xmit_linux.c
+++ b/drivers/staging/rtl8712/xmit_linux.c
@@ -95,18 +95,12 @@ void r8712_SetFilter(struct work_struct *work)
 	struct _adapter *adapter = container_of(work, struct _adapter,
 						wk_filter_rx_ff0);
 	u8  oldvalue = 0x00, newvalue = 0x00;
-	unsigned long irqL;
 
 	oldvalue = r8712_read8(adapter, 0x117);
 	newvalue = oldvalue & 0xfe;
 	r8712_write8(adapter, 0x117, newvalue);
 
-	spin_lock_irqsave(&adapter->lock_rx_ff0_filter, irqL);
-	adapter->blnEnableRxFF0Filter = 1;
-	spin_unlock_irqrestore(&adapter->lock_rx_ff0_filter, irqL);
-	do {
-		msleep(100);
-	} while (adapter->blnEnableRxFF0Filter == 1);
+	wait_for_completion(&adapter->rx_filter_ready);
 	r8712_write8(adapter, 0x117, oldvalue);
 }
 
-- 
2.17.1

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

[Fabio M. De Francesco]

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