[PATCH 0/6] net: use 'time_left' instead of 'timeout' with wait_*() functions

[PATCH 0/6] net: use 'time_left' instead of 'timeout' with wait_*() functions

[Wolfram Sang]

There is a confusing pattern in the kernel to use a variable named 'timeout' to
store the result of wait_*() functions causing patterns like:

        timeout = wait_for_completion_timeout(...)
        if (!timeout) return -ETIMEDOUT;

with all kinds of permutations. Use 'time_left' as a variable to make the code
obvious and self explaining.

This is part of a tree-wide series. The rest of the patches can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/time_left

Because these patches are generated, I audit them before sending. This is why I
will send series step by step. Build bot is happy with these patches, though.
No functional changes intended.


Wolfram Sang (6):
  wifi: ath11k: use 'time_left' variable with wait_event_timeout()
  wifi: brcmfmac: use 'time_left' variable with wait_event_timeout()
  wifi: mac80211: use 'time_left' variable with
    wait_for_completion_timeout()
  wifi: p54: use 'time_left' variable with
    wait_for_completion_interruptible_timeout()
  wifi: rtw89: use 'time_left' variable with
    wait_for_completion_timeout()
  wifi: zd1211rw: use 'time_left' variable with
    wait_for_completion_timeout()

 drivers/net/wireless/ath/ath11k/qmi.c         | 20 +++++++++----------
 .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 10 +++++-----
 drivers/net/wireless/intersil/p54/fwio.c      |  6 +++---
 drivers/net/wireless/intersil/p54/p54pci.c    |  8 ++++----
 drivers/net/wireless/intersil/p54/p54spi.c    | 10 +++++-----
 drivers/net/wireless/marvell/mwl8k.c          | 10 +++++-----
 drivers/net/wireless/realtek/rtw89/core.c     |  6 +++---
 drivers/net/wireless/zydas/zd1211rw/zd_usb.c  |  8 ++++----
 8 files changed, 39 insertions(+), 39 deletions(-)

-- 
2.43.0

[PATCH 1/6] wifi: ath11k: use 'time_left' variable with wait_event_timeout()

[Wolfram Sang]

There is a confusing pattern in the kernel to use a variable named 'timeout' to
store the result of wait_event_timeout() causing patterns like:

	timeout = wait_event_timeout(...)
	if (!timeout) return -ETIMEDOUT;

with all kinds of permutations. Use 'time_left' as a variable to make the code
self explaining.

Fix to the proper variable type 'long' while here.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/net/wireless/ath/ath11k/qmi.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/qmi.c b/drivers/net/wireless/ath/ath11k/qmi.c
index d4a243b64f6c..2fe0ef660456 100644
--- a/drivers/net/wireless/ath/ath11k/qmi.c
+++ b/drivers/net/wireless/ath/ath11k/qmi.c
@@ -2859,7 +2859,7 @@ int ath11k_qmi_firmware_start(struct ath11k_base *ab,
 
 int ath11k_qmi_fwreset_from_cold_boot(struct ath11k_base *ab)
 {
-	int timeout;
+	long time_left;
 
 	if (!ath11k_core_coldboot_cal_support(ab) ||
 	    ab->hw_params.cbcal_restart_fw == 0)
@@ -2867,11 +2867,11 @@ int ath11k_qmi_fwreset_from_cold_boot(struct ath11k_base *ab)
 
 	ath11k_dbg(ab, ATH11K_DBG_QMI, "wait for cold boot done\n");
 
-	timeout = wait_event_timeout(ab->qmi.cold_boot_waitq,
-				     (ab->qmi.cal_done == 1),
-				     ATH11K_COLD_BOOT_FW_RESET_DELAY);
+	time_left = wait_event_timeout(ab->qmi.cold_boot_waitq,
+				       (ab->qmi.cal_done == 1),
+				       ATH11K_COLD_BOOT_FW_RESET_DELAY);
 
-	if (timeout <= 0) {
+	if (time_left <= 0) {
 		ath11k_warn(ab, "Coldboot Calibration timed out\n");
 		return -ETIMEDOUT;
 	}
@@ -2886,7 +2886,7 @@ EXPORT_SYMBOL(ath11k_qmi_fwreset_from_cold_boot);
 
 static int ath11k_qmi_process_coldboot_calibration(struct ath11k_base *ab)
 {
-	int timeout;
+	long time_left;
 	int ret;
 
 	ret = ath11k_qmi_wlanfw_mode_send(ab, ATH11K_FIRMWARE_MODE_COLD_BOOT);
@@ -2897,10 +2897,10 @@ static int ath11k_qmi_process_coldboot_calibration(struct ath11k_base *ab)
 
 	ath11k_dbg(ab, ATH11K_DBG_QMI, "Coldboot calibration wait started\n");
 
-	timeout = wait_event_timeout(ab->qmi.cold_boot_waitq,
-				     (ab->qmi.cal_done  == 1),
-				     ATH11K_COLD_BOOT_FW_RESET_DELAY);
-	if (timeout <= 0) {
+	time_left = wait_event_timeout(ab->qmi.cold_boot_waitq,
+				       (ab->qmi.cal_done  == 1),
+				       ATH11K_COLD_BOOT_FW_RESET_DELAY);
+	if (time_left <= 0) {
 		ath11k_warn(ab, "coldboot calibration timed out\n");
 		return 0;
 	}
-- 
2.43.0

Re: [PATCH 1/6] wifi: ath11k: use 'time_left' variable with wait_event_timeout()

[Jeff Johnson]

On 6/3/2024 2:15 AM, Wolfram Sang wrote:
> There is a confusing pattern in the kernel to use a variable named 'timeout' to
> store the result of wait_event_timeout() causing patterns like:
> 
> 	timeout = wait_event_timeout(...)
> 	if (!timeout) return -ETIMEDOUT;
> 
> with all kinds of permutations. Use 'time_left' as a variable to make the code
> self explaining.
> 
> Fix to the proper variable type 'long' while here.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/net/wireless/ath/ath11k/qmi.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/qmi.c b/drivers/net/wireless/ath/ath11k/qmi.c
> index d4a243b64f6c..2fe0ef660456 100644
> --- a/drivers/net/wireless/ath/ath11k/qmi.c
> +++ b/drivers/net/wireless/ath/ath11k/qmi.c
> @@ -2859,7 +2859,7 @@ int ath11k_qmi_firmware_start(struct ath11k_base *ab,
>  
>  int ath11k_qmi_fwreset_from_cold_boot(struct ath11k_base *ab)
>  {
> -	int timeout;
> +	long time_left;
>  
>  	if (!ath11k_core_coldboot_cal_support(ab) ||
>  	    ab->hw_params.cbcal_restart_fw == 0)
> @@ -2867,11 +2867,11 @@ int ath11k_qmi_fwreset_from_cold_boot(struct ath11k_base *ab)
>  
>  	ath11k_dbg(ab, ATH11K_DBG_QMI, "wait for cold boot done\n");
>  
> -	timeout = wait_event_timeout(ab->qmi.cold_boot_waitq,
> -				     (ab->qmi.cal_done == 1),
> -				     ATH11K_COLD_BOOT_FW_RESET_DELAY);
> +	time_left = wait_event_timeout(ab->qmi.cold_boot_waitq,
> +				       (ab->qmi.cal_done == 1),
> +				       ATH11K_COLD_BOOT_FW_RESET_DELAY);
>  
> -	if (timeout <= 0) {
> +	if (time_left <= 0) {
>  		ath11k_warn(ab, "Coldboot Calibration timed out\n");
>  		return -ETIMEDOUT;
>  	}
> @@ -2886,7 +2886,7 @@ EXPORT_SYMBOL(ath11k_qmi_fwreset_from_cold_boot);
>  
>  static int ath11k_qmi_process_coldboot_calibration(struct ath11k_base *ab)
>  {
> -	int timeout;
> +	long time_left;
>  	int ret;
>  
>  	ret = ath11k_qmi_wlanfw_mode_send(ab, ATH11K_FIRMWARE_MODE_COLD_BOOT);
> @@ -2897,10 +2897,10 @@ static int ath11k_qmi_process_coldboot_calibration(struct ath11k_base *ab)
>  
>  	ath11k_dbg(ab, ATH11K_DBG_QMI, "Coldboot calibration wait started\n");
>  
> -	timeout = wait_event_timeout(ab->qmi.cold_boot_waitq,
> -				     (ab->qmi.cal_done  == 1),
> -				     ATH11K_COLD_BOOT_FW_RESET_DELAY);
> -	if (timeout <= 0) {
> +	time_left = wait_event_timeout(ab->qmi.cold_boot_waitq,
> +				       (ab->qmi.cal_done  == 1),
> +				       ATH11K_COLD_BOOT_FW_RESET_DELAY);
> +	if (time_left <= 0) {
>  		ath11k_warn(ab, "coldboot calibration timed out\n");
>  		return 0;
>  	}

This looks ok to me, but note that changes to ath11k go through the ath tree,
so, unless Kalle has a different opinion, this should be submitted separately
from changes that go through the wireless tree.

Re: [PATCH 1/6] wifi: ath11k: use 'time_left' variable with wait_event_timeout()

[Kalle Valo]

Jeff Johnson <quic_jjohnson@quicinc.com> writes:

> On 6/3/2024 2:15 AM, Wolfram Sang wrote:
>
>> There is a confusing pattern in the kernel to use a variable named 'timeout' to
>> store the result of wait_event_timeout() causing patterns like:
>> 
>> 	timeout = wait_event_timeout(...)
>> 	if (!timeout) return -ETIMEDOUT;
>> 
>> with all kinds of permutations. Use 'time_left' as a variable to make the code
>> self explaining.
>> 
>> Fix to the proper variable type 'long' while here.
>> 
>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> ---
>>  drivers/net/wireless/ath/ath11k/qmi.c | 20 ++++++++++----------
>>  1 file changed, 10 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/net/wireless/ath/ath11k/qmi.c b/drivers/net/wireless/ath/ath11k/qmi.c
>> index d4a243b64f6c..2fe0ef660456 100644
>> --- a/drivers/net/wireless/ath/ath11k/qmi.c
>> +++ b/drivers/net/wireless/ath/ath11k/qmi.c
>> @@ -2859,7 +2859,7 @@ int ath11k_qmi_firmware_start(struct ath11k_base *ab,
>>  
>>  int ath11k_qmi_fwreset_from_cold_boot(struct ath11k_base *ab)
>>  {
>> -	int timeout;
>> +	long time_left;
>>  
>>  	if (!ath11k_core_coldboot_cal_support(ab) ||
>>  	    ab->hw_params.cbcal_restart_fw == 0)
>> @@ -2867,11 +2867,11 @@ int ath11k_qmi_fwreset_from_cold_boot(struct ath11k_base *ab)
>>  
>>  	ath11k_dbg(ab, ATH11K_DBG_QMI, "wait for cold boot done\n");
>>  
>> -	timeout = wait_event_timeout(ab->qmi.cold_boot_waitq,
>> -				     (ab->qmi.cal_done == 1),
>> -				     ATH11K_COLD_BOOT_FW_RESET_DELAY);
>> +	time_left = wait_event_timeout(ab->qmi.cold_boot_waitq,
>> +				       (ab->qmi.cal_done == 1),
>> +				       ATH11K_COLD_BOOT_FW_RESET_DELAY);
>>  
>> -	if (timeout <= 0) {
>> +	if (time_left <= 0) {
>>  		ath11k_warn(ab, "Coldboot Calibration timed out\n");
>>  		return -ETIMEDOUT;
>>  	}
>> @@ -2886,7 +2886,7 @@ EXPORT_SYMBOL(ath11k_qmi_fwreset_from_cold_boot);
>>  
>>  static int ath11k_qmi_process_coldboot_calibration(struct ath11k_base *ab)
>>  {
>> -	int timeout;
>> +	long time_left;
>>  	int ret;
>>  
>>  	ret = ath11k_qmi_wlanfw_mode_send(ab, ATH11K_FIRMWARE_MODE_COLD_BOOT);
>> @@ -2897,10 +2897,10 @@ static int ath11k_qmi_process_coldboot_calibration(struct ath11k_base *ab)
>>  
>>  	ath11k_dbg(ab, ATH11K_DBG_QMI, "Coldboot calibration wait started\n");
>>  
>> -	timeout = wait_event_timeout(ab->qmi.cold_boot_waitq,
>> -				     (ab->qmi.cal_done  == 1),
>> -				     ATH11K_COLD_BOOT_FW_RESET_DELAY);
>> -	if (timeout <= 0) {
>> +	time_left = wait_event_timeout(ab->qmi.cold_boot_waitq,
>> +				       (ab->qmi.cal_done  == 1),
>> +				       ATH11K_COLD_BOOT_FW_RESET_DELAY);
>> +	if (time_left <= 0) {
>>  		ath11k_warn(ab, "coldboot calibration timed out\n");
>>  		return 0;
>>  	}
>
> This looks ok to me, but note that changes to ath11k go through the ath tree,
> so, unless Kalle has a different opinion, this should be submitted separately
> from changes that go through the wireless tree.

As there are no dependencies to other patches I can take this directly
to ath.git, so no need to resend because of this.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 1/6] wifi: ath11k: use 'time_left' variable with wait_event_timeout()

[Kalle Valo]

Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> There is a confusing pattern in the kernel to use a variable named 'timeout' to
> store the result of wait_event_timeout() causing patterns like:
> 
>         timeout = wait_event_timeout(...)
>         if (!timeout) return -ETIMEDOUT;
> 
> with all kinds of permutations. Use 'time_left' as a variable to make the code
> self explaining.
> 
> Fix to the proper variable type 'long' while here.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

Patch applied to ath-next branch of ath.git, thanks.

65a8368bf34e wifi: ath11k: use 'time_left' variable with wait_event_timeout()

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20240603091541.8367-2-wsa+renesas@sang-engineering.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH 2/6] wifi: brcmfmac: use 'time_left' variable with wait_event_timeout()

[Wolfram Sang]

There is a confusing pattern in the kernel to use a variable named 'timeout' to
store the result of wait_event_timeout() causing patterns like:

	timeout = wait_event_timeout(...)
	if (!timeout) return -ETIMEDOUT;

with all kinds of permutations. Use 'time_left' as a variable to make the code
self explaining.

Fix to the proper variable type 'long' while here.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 .../wireless/broadcom/brcm80211/brcmfmac/cfg80211.c    | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 5fe0e671ecb3..1585a5653ee4 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -4071,7 +4071,7 @@ static void brcmf_report_wowl_wakeind(struct wiphy *wiphy, struct brcmf_if *ifp)
 	struct cfg80211_wowlan_wakeup *wakeup;
 	u32 wakeind;
 	s32 err;
-	int timeout;
+	long time_left;
 
 	err = brcmf_fil_iovar_data_get(ifp, "wowl_wakeind", &wake_ind_le,
 				       sizeof(wake_ind_le));
@@ -4113,10 +4113,10 @@ static void brcmf_report_wowl_wakeind(struct wiphy *wiphy, struct brcmf_if *ifp)
 		}
 		if (wakeind & BRCMF_WOWL_PFN_FOUND) {
 			brcmf_dbg(INFO, "WOWL Wake indicator: BRCMF_WOWL_PFN_FOUND\n");
-			timeout = wait_event_timeout(cfg->wowl.nd_data_wait,
-				cfg->wowl.nd_data_completed,
-				BRCMF_ND_INFO_TIMEOUT);
-			if (!timeout)
+			time_left = wait_event_timeout(cfg->wowl.nd_data_wait,
+						       cfg->wowl.nd_data_completed,
+						       BRCMF_ND_INFO_TIMEOUT);
+			if (!time_left)
 				bphy_err(drvr, "No result for wowl net detect\n");
 			else
 				wakeup_data.net_detect = cfg->wowl.nd_info;
-- 
2.43.0

Re: [PATCH 2/6] wifi: brcmfmac: use 'time_left' variable with wait_event_timeout()

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 934 bytes --]

On 6/3/2024 11:15 AM, Wolfram Sang wrote:
> There is a confusing pattern in the kernel to use a variable named 'timeout' to
> store the result of wait_event_timeout() causing patterns like:
> 
> 	timeout = wait_event_timeout(...)
> 	if (!timeout) return -ETIMEDOUT;
> 
> with all kinds of permutations. Use 'time_left' as a variable to make the code
> self explaining.

I feel this type of changes fall into the category of bike-shedding. 
People should know how wait_event_timeout() works and then a variable 
name does not really matter.

> Fix to the proper variable type 'long' while here.

But it may have useful side-effects to go over the code with fresh look. 
Thanks.

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>   .../wireless/broadcom/brcm80211/brcmfmac/cfg80211.c    | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH 2/6] wifi: brcmfmac: use 'time_left' variable with wait_event_timeout()

[Wolfram Sang]

> I feel this type of changes fall into the category of bike-shedding. People

I have two motivations for this change: One may be bike-shedding, yet
"if (!timeout) return -ETIMEDOUT" looks stupid to me.

> should know how wait_event_timeout() works and then a variable name does not
> really matter.

And for a new developer, I am quite sure the change will help to
understand how wait_event-family() works. Especially given that
wait_event_interruptible() returns 0 if condition is true and
wait_event_interruptible_timeout() returns 0 if condition is false.

> > Fix to the proper variable type 'long' while here.
> 
> But it may have useful side-effects to go over the code with fresh look.

:)

> Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>

Thank you!

Re: [PATCH 2/6] wifi: brcmfmac: use 'time_left' variable with wait_event_timeout()

[Kalle Valo]

Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> There is a confusing pattern in the kernel to use a variable named 'timeout' to
> store the result of wait_event_timeout() causing patterns like:
> 
> 	timeout = wait_event_timeout(...)
> 	if (!timeout) return -ETIMEDOUT;
> 
> with all kinds of permutations. Use 'time_left' as a variable to make the code
> self explaining.
> 
> Fix to the proper variable type 'long' while here.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>

4 patches applied to wireless-next.git, thanks.

158d5a1b3caa wifi: brcmfmac: use 'time_left' variable with wait_event_timeout()
0c0668813cc0 wifi: mwl8k: use 'time_left' variable with wait_for_completion_timeout()
a37f6947ff07 wifi: p54: use 'time_left' variable with wait_for_completion_interruptible_timeout()
a2ead3445a63 wifi: zd1211rw: use 'time_left' variable with wait_for_completion_timeout()

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20240603091541.8367-3-wsa+renesas@sang-engineering.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH 3/6] wifi: mac80211: use 'time_left' variable with wait_for_completion_timeout()

[Wolfram Sang]

There is a confusing pattern in the kernel to use a variable named 'timeout' to
store the result of wait_for_completion_timeout() causing patterns like:

	timeout = wait_for_completion_timeout(...)
	if (!timeout) return -ETIMEDOUT;

with all kinds of permutations. Use 'time_left' as a variable to make the code
self explaining.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/net/wireless/marvell/mwl8k.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwl8k.c b/drivers/net/wireless/marvell/mwl8k.c
index d3d07bb26335..241a02a0accd 100644
--- a/drivers/net/wireless/marvell/mwl8k.c
+++ b/drivers/net/wireless/marvell/mwl8k.c
@@ -2211,7 +2211,7 @@ static int mwl8k_post_cmd(struct ieee80211_hw *hw, struct mwl8k_cmd_pkt_hdr *cmd
 	dma_addr_t dma_addr;
 	unsigned int dma_size;
 	int rc;
-	unsigned long timeout = 0;
+	unsigned long time_left = 0;
 	u8 buf[32];
 	u32 bitmap = 0;
 
@@ -2258,8 +2258,8 @@ static int mwl8k_post_cmd(struct ieee80211_hw *hw, struct mwl8k_cmd_pkt_hdr *cmd
 	iowrite32(MWL8K_H2A_INT_DUMMY,
 		regs + MWL8K_HIU_H2A_INTERRUPT_EVENTS);
 
-	timeout = wait_for_completion_timeout(&cmd_wait,
-				msecs_to_jiffies(MWL8K_CMD_TIMEOUT_MS));
+	time_left = wait_for_completion_timeout(&cmd_wait,
+						msecs_to_jiffies(MWL8K_CMD_TIMEOUT_MS));
 
 	priv->hostcmd_wait = NULL;
 
@@ -2267,7 +2267,7 @@ static int mwl8k_post_cmd(struct ieee80211_hw *hw, struct mwl8k_cmd_pkt_hdr *cmd
 	dma_unmap_single(&priv->pdev->dev, dma_addr, dma_size,
 			 DMA_BIDIRECTIONAL);
 
-	if (!timeout) {
+	if (!time_left) {
 		wiphy_err(hw->wiphy, "Command %s timeout after %u ms\n",
 			  mwl8k_cmd_name(cmd->code, buf, sizeof(buf)),
 			  MWL8K_CMD_TIMEOUT_MS);
@@ -2275,7 +2275,7 @@ static int mwl8k_post_cmd(struct ieee80211_hw *hw, struct mwl8k_cmd_pkt_hdr *cmd
 	} else {
 		int ms;
 
-		ms = MWL8K_CMD_TIMEOUT_MS - jiffies_to_msecs(timeout);
+		ms = MWL8K_CMD_TIMEOUT_MS - jiffies_to_msecs(time_left);
 
 		rc = cmd->result ? -EINVAL : 0;
 		if (rc)
-- 
2.43.0

Re: [PATCH 3/6] wifi: mac80211: use 'time_left' variable with wait_for_completion_timeout()

[Kalle Valo]

Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> There is a confusing pattern in the kernel to use a variable named 'timeout' to
> store the result of wait_for_completion_timeout() causing patterns like:
> 
> 	timeout = wait_for_completion_timeout(...)
> 	if (!timeout) return -ETIMEDOUT;
> 
> with all kinds of permutations. Use 'time_left' as a variable to make the code
> self explaining.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

The title should be:

wifi: mwl8k: use 'time_left' variable with wait_for_completion_timeout()

I can fix that, no need to resend because of this.

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20240603091541.8367-4-wsa+renesas@sang-engineering.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 3/6] wifi: mac80211: use 'time_left' variable with wait_for_completion_timeout()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 174 bytes --]


> The title should be:
> 
> wifi: mwl8k: use 'time_left' variable with wait_for_completion_timeout()

Oh, that escaped my audit. Sorry, and thank you for fixing it!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 4/6] wifi: p54: use 'time_left' variable with wait_for_completion_interruptible_timeout()

[Wolfram Sang]

There is a confusing pattern in the kernel to use a variable named 'timeout' to
store the result of wait_for_completion_interruptible_timeout() causing patterns like:

	timeout = wait_for_completion_interruptible_timeout(...)
	if (!timeout) return -ETIMEDOUT;

with all kinds of permutations. Use 'time_left' as a variable to make the code
self explaining.

Fix to the proper variable type 'long' while here.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/net/wireless/intersil/p54/fwio.c   |  6 +++---
 drivers/net/wireless/intersil/p54/p54pci.c |  8 ++++----
 drivers/net/wireless/intersil/p54/p54spi.c | 10 +++++-----
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/intersil/p54/fwio.c b/drivers/net/wireless/intersil/p54/fwio.c
index c4fe70e05b9b..772084a9bd8d 100644
--- a/drivers/net/wireless/intersil/p54/fwio.c
+++ b/drivers/net/wireless/intersil/p54/fwio.c
@@ -216,7 +216,7 @@ int p54_download_eeprom(struct p54_common *priv, void *buf,
 	struct sk_buff *skb;
 	size_t eeprom_hdr_size;
 	int ret = 0;
-	long timeout;
+	long time_left;
 
 	if (priv->fw_var >= 0x509)
 		eeprom_hdr_size = sizeof(*eeprom_hdr);
@@ -245,9 +245,9 @@ int p54_download_eeprom(struct p54_common *priv, void *buf,
 
 	p54_tx(priv, skb);
 
-	timeout = wait_for_completion_interruptible_timeout(
+	time_left = wait_for_completion_interruptible_timeout(
 			&priv->eeprom_comp, HZ);
-	if (timeout <= 0) {
+	if (time_left <= 0) {
 		wiphy_err(priv->hw->wiphy,
 			"device does not respond or signal received!\n");
 		ret = -EBUSY;
diff --git a/drivers/net/wireless/intersil/p54/p54pci.c b/drivers/net/wireless/intersil/p54/p54pci.c
index e97ee547b9f3..6588f5477762 100644
--- a/drivers/net/wireless/intersil/p54/p54pci.c
+++ b/drivers/net/wireless/intersil/p54/p54pci.c
@@ -434,7 +434,7 @@ static int p54p_open(struct ieee80211_hw *dev)
 {
 	struct p54p_priv *priv = dev->priv;
 	int err;
-	long timeout;
+	long time_left;
 
 	init_completion(&priv->boot_comp);
 	err = request_irq(priv->pdev->irq, p54p_interrupt,
@@ -472,12 +472,12 @@ static int p54p_open(struct ieee80211_hw *dev)
 	P54P_WRITE(dev_int, cpu_to_le32(ISL38XX_DEV_INT_RESET));
 	P54P_READ(dev_int);
 
-	timeout = wait_for_completion_interruptible_timeout(
+	time_left = wait_for_completion_interruptible_timeout(
 			&priv->boot_comp, HZ);
-	if (timeout <= 0) {
+	if (time_left <= 0) {
 		wiphy_err(dev->wiphy, "Cannot boot firmware!\n");
 		p54p_stop(dev);
-		return timeout ? -ERESTARTSYS : -ETIMEDOUT;
+		return time_left ? -ERESTARTSYS : -ETIMEDOUT;
 	}
 
 	P54P_WRITE(int_enable, cpu_to_le32(ISL38XX_INT_IDENT_UPDATE));
diff --git a/drivers/net/wireless/intersil/p54/p54spi.c b/drivers/net/wireless/intersil/p54/p54spi.c
index 0073b5e0f9c9..d33a994906a7 100644
--- a/drivers/net/wireless/intersil/p54/p54spi.c
+++ b/drivers/net/wireless/intersil/p54/p54spi.c
@@ -518,7 +518,7 @@ static void p54spi_work(struct work_struct *work)
 static int p54spi_op_start(struct ieee80211_hw *dev)
 {
 	struct p54s_priv *priv = dev->priv;
-	unsigned long timeout;
+	long time_left;
 	int ret = 0;
 
 	if (mutex_lock_interruptible(&priv->mutex)) {
@@ -538,10 +538,10 @@ static int p54spi_op_start(struct ieee80211_hw *dev)
 
 	mutex_unlock(&priv->mutex);
 
-	timeout = msecs_to_jiffies(2000);
-	timeout = wait_for_completion_interruptible_timeout(&priv->fw_comp,
-							    timeout);
-	if (!timeout) {
+	time_left = msecs_to_jiffies(2000);
+	time_left = wait_for_completion_interruptible_timeout(&priv->fw_comp,
+							      time_left);
+	if (!time_left) {
 		dev_err(&priv->spi->dev, "firmware boot failed");
 		p54spi_power_off(priv);
 		ret = -1;
-- 
2.43.0

[PATCH 5/6] wifi: rtw89: use 'time_left' variable with wait_for_completion_timeout()

[Wolfram Sang]

There is a confusing pattern in the kernel to use a variable named 'timeout' to
store the result of wait_for_completion_timeout() causing patterns like:

	timeout = wait_for_completion_timeout(...)
	if (!timeout) return -ETIMEDOUT;

with all kinds of permutations. Use 'time_left' as a variable to make the code
self explaining.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/net/wireless/realtek/rtw89/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index ddc390d24ec1..16ab834185b4 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -4022,15 +4022,15 @@ void rtw89_core_update_beacon_work(struct work_struct *work)
 int rtw89_wait_for_cond(struct rtw89_wait_info *wait, unsigned int cond)
 {
 	struct completion *cmpl = &wait->completion;
-	unsigned long timeout;
+	unsigned long time_left;
 	unsigned int cur;
 
 	cur = atomic_cmpxchg(&wait->cond, RTW89_WAIT_COND_IDLE, cond);
 	if (cur != RTW89_WAIT_COND_IDLE)
 		return -EBUSY;
 
-	timeout = wait_for_completion_timeout(cmpl, RTW89_WAIT_FOR_COND_TIMEOUT);
-	if (timeout == 0) {
+	time_left = wait_for_completion_timeout(cmpl, RTW89_WAIT_FOR_COND_TIMEOUT);
+	if (time_left == 0) {
 		atomic_set(&wait->cond, RTW89_WAIT_COND_IDLE);
 		return -ETIMEDOUT;
 	}
-- 
2.43.0

RE: [PATCH 5/6] wifi: rtw89: use 'time_left' variable with wait_for_completion_timeout()

[Ping-Ke Shih]

Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
> There is a confusing pattern in the kernel to use a variable named 'timeout' to
> store the result of wait_for_completion_timeout() causing patterns like:
> 
>         timeout = wait_for_completion_timeout(...)
>         if (!timeout) return -ETIMEDOUT;
> 
> with all kinds of permutations. Use 'time_left' as a variable to make the code
> self explaining.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Acked-by: Ping-Ke Shih <pkshih@realtek.com>

Re: [PATCH 5/6] wifi: rtw89: use 'time_left' variable with wait_for_completion_timeout()

[Kalle Valo]

Ping-Ke Shih <pkshih@realtek.com> writes:

> Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
>> There is a confusing pattern in the kernel to use a variable named 'timeout' to
>> store the result of wait_for_completion_timeout() causing patterns like:
>> 
>>         timeout = wait_for_completion_timeout(...)
>>         if (!timeout) return -ETIMEDOUT;
>> 
>> with all kinds of permutations. Use 'time_left' as a variable to make the code
>> self explaining.
>> 
>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> Acked-by: Ping-Ke Shih <pkshih@realtek.com>

BTW Ping, you can also take it directly to your tree if you want. But if you
want me to take the patch, then please assign it to me on patchwork (ie.
change 'Delegate to' to 'kvalo'). My preference is to take it to your
tree, smaller risk of concflicts that way, but up to you.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

RE: [PATCH 5/6] wifi: rtw89: use 'time_left' variable with wait_for_completion_timeout()

[Ping-Ke Shih]

Kalle Valo <kvalo@kernel.org> wrote:
> Ping-Ke Shih <pkshih@realtek.com> writes:
> 
> > Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
> >> There is a confusing pattern in the kernel to use a variable named 'timeout' to
> >> store the result of wait_for_completion_timeout() causing patterns like:
> >>
> >>         timeout = wait_for_completion_timeout(...)
> >>         if (!timeout) return -ETIMEDOUT;
> >>
> >> with all kinds of permutations. Use 'time_left' as a variable to make the code
> >> self explaining.
> >>
> >> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >
> > Acked-by: Ping-Ke Shih <pkshih@realtek.com>
> 
> BTW Ping, you can also take it directly to your tree if you want. But if you
> want me to take the patch, then please assign it to me on patchwork (ie.
> change 'Delegate to' to 'kvalo'). My preference is to take it to your
> tree, smaller risk of concflicts that way, but up to you.

I will take it to my tree. 

Ping-Ke

Re: [PATCH 5/6] wifi: rtw89: use 'time_left' variable with wait_for_completion_timeout()

[Ping-Ke Shih]

Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> There is a confusing pattern in the kernel to use a variable named 'timeout' to
> store the result of wait_for_completion_timeout() causing patterns like:
> 
> 	timeout = wait_for_completion_timeout(...)
> 	if (!timeout) return -ETIMEDOUT;
> 
> with all kinds of permutations. Use 'time_left' as a variable to make the code
> self explaining.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Acked-by: Ping-Ke Shih <pkshih@realtek.com>

1 patch(es) applied to rtw-next branch of rtw.git, thanks.

3d530eeaf8e7 wifi: rtw89: use 'time_left' variable with wait_for_completion_timeout()

---
https://github.com/pkshih/rtw.git

[PATCH 6/6] wifi: zd1211rw: use 'time_left' variable with wait_for_completion_timeout()

[Wolfram Sang]

There is a confusing pattern in the kernel to use a variable named 'timeout' to
store the result of wait_for_completion_timeout() causing patterns like:

	timeout = wait_for_completion_timeout(...)
	if (!timeout) return -ETIMEDOUT;

with all kinds of permutations. Use 'time_left' as a variable to make the code
self explaining.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/net/wireless/zydas/zd1211rw/zd_usb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/zydas/zd1211rw/zd_usb.c b/drivers/net/wireless/zydas/zd1211rw/zd_usb.c
index f3b567a13ded..2ee4218da1c5 100644
--- a/drivers/net/wireless/zydas/zd1211rw/zd_usb.c
+++ b/drivers/net/wireless/zydas/zd1211rw/zd_usb.c
@@ -1698,7 +1698,7 @@ int zd_usb_ioread16v(struct zd_usb *usb, u16 *values,
 	int r, i, req_len, actual_req_len, try_count = 0;
 	struct usb_device *udev;
 	struct usb_req_read_regs *req = NULL;
-	unsigned long timeout;
+	unsigned long time_left;
 	bool retry = false;
 
 	if (count < 1) {
@@ -1748,9 +1748,9 @@ int zd_usb_ioread16v(struct zd_usb *usb, u16 *values,
 		goto error;
 	}
 
-	timeout = wait_for_completion_timeout(&usb->intr.read_regs.completion,
-					      msecs_to_jiffies(50));
-	if (!timeout) {
+	time_left = wait_for_completion_timeout(&usb->intr.read_regs.completion,
+						msecs_to_jiffies(50));
+	if (!time_left) {
 		disable_read_regs_int(usb);
 		dev_dbg_f(zd_usb_dev(usb), "read timed out\n");
 		r = -ETIMEDOUT;
-- 
2.43.0