| Message ID | 20240603091541.8367-2-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 65a8368bf34e51b4499205658d7015d40e5a3f80 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | net: use 'time_left' instead of 'timeout' with wait_*() functions | expand |
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.
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.
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()
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; }
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(-)