[PATCH 0/3] slimbus: use 'time_left' instead of 'timeout' with wait_for_*() functions
There is a confusing pattern in the kernel to use a variable named 'timeout' to store the result of wait_for_*() 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 (some parts may still be WIP):
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 (3): slimbus: messaging: use 'time_left' variable with wait_for_completion_timeout() slimbus: qcom-ctrl: use 'time_left' variable with wait_for_completion_timeout() slimbus: qcom-ngd-ctrl: use 'time_left' variable with wait_for_completion_timeout()
drivers/slimbus/messaging.c | 9 +++++---- drivers/slimbus/qcom-ctrl.c | 7 ++++--- drivers/slimbus/qcom-ngd-ctrl.c | 29 ++++++++++++++++------------- 3 files changed, 25 insertions(+), 20 deletions(-)
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.
Fix to the proper variable type 'unsigned long' while here.
Signed-off-by: Wolfram Sang wsa+renesas@sang-engineering.com --- drivers/slimbus/messaging.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/slimbus/messaging.c b/drivers/slimbus/messaging.c index 4ce0cb61e481..242570a5e565 100644 --- a/drivers/slimbus/messaging.c +++ b/drivers/slimbus/messaging.c @@ -111,7 +111,8 @@ int slim_do_transfer(struct slim_controller *ctrl, struct slim_msg_txn *txn) { DECLARE_COMPLETION_ONSTACK(done); bool need_tid = false, clk_pause_msg = false; - int ret, timeout; + int ret; + unsigned long time_left;
/* * do not vote for runtime-PM if the transactions are part of clock @@ -151,9 +152,9 @@ int slim_do_transfer(struct slim_controller *ctrl, struct slim_msg_txn *txn) if (!ret && need_tid && !txn->msg->comp) { unsigned long ms = txn->rl + HZ;
- timeout = wait_for_completion_timeout(txn->comp, - msecs_to_jiffies(ms)); - if (!timeout) { + time_left = wait_for_completion_timeout(txn->comp, + msecs_to_jiffies(ms)); + if (!time_left) { ret = -ETIMEDOUT; slim_free_txn_tid(ctrl, txn); }
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.
Fix to the proper variable type 'unsigned long' while here.
Signed-off-by: Wolfram Sang wsa+renesas@sang-engineering.com --- drivers/slimbus/qcom-ctrl.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/slimbus/qcom-ctrl.c b/drivers/slimbus/qcom-ctrl.c index 400b7b385a44..c613c7517f99 100644 --- a/drivers/slimbus/qcom-ctrl.c +++ b/drivers/slimbus/qcom-ctrl.c @@ -330,7 +330,8 @@ static int qcom_xfer_msg(struct slim_controller *sctrl, void *pbuf = slim_alloc_txbuf(ctrl, txn, &done); unsigned long ms = txn->rl + HZ; u8 *puc; - int ret = 0, timeout, retries = QCOM_BUF_ALLOC_RETRIES; + int ret = 0, retries = QCOM_BUF_ALLOC_RETRIES; + unsigned long time_left; u8 la = txn->la; u32 *head; /* HW expects length field to be excluded */ @@ -374,9 +375,9 @@ static int qcom_xfer_msg(struct slim_controller *sctrl, memcpy(puc, txn->msg->wbuf, txn->msg->num_bytes);
qcom_slim_queue_tx(ctrl, head, txn->rl, MGR_TX_MSG); - timeout = wait_for_completion_timeout(&done, msecs_to_jiffies(ms)); + time_left = wait_for_completion_timeout(&done, msecs_to_jiffies(ms));
- if (!timeout) { + if (!time_left) { dev_err(ctrl->dev, "TX timed out:MC:0x%x,mt:0x%x", txn->mc, txn->mt); ret = -ETIMEDOUT;
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.
Fix to the proper variable type 'unsigned long' while here.
Signed-off-by: Wolfram Sang wsa+renesas@sang-engineering.com --- drivers/slimbus/qcom-ngd-ctrl.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c index efeba8275a66..21b4008de4f3 100644 --- a/drivers/slimbus/qcom-ngd-ctrl.c +++ b/drivers/slimbus/qcom-ngd-ctrl.c @@ -789,7 +789,8 @@ static int qcom_slim_ngd_xfer_msg(struct slim_controller *sctrl, struct qcom_slim_ngd_ctrl *ctrl = dev_get_drvdata(sctrl->dev); DECLARE_COMPLETION_ONSTACK(tx_sent); DECLARE_COMPLETION_ONSTACK(done); - int ret, timeout, i; + int ret, i; + unsigned long time_left; u8 wbuf[SLIM_MSGQ_BUF_LEN]; u8 rbuf[SLIM_MSGQ_BUF_LEN]; u32 *pbuf; @@ -891,8 +892,8 @@ static int qcom_slim_ngd_xfer_msg(struct slim_controller *sctrl, return ret; }
- timeout = wait_for_completion_timeout(&tx_sent, HZ); - if (!timeout) { + time_left = wait_for_completion_timeout(&tx_sent, HZ); + if (!time_left) { dev_err(sctrl->dev, "TX timed out:MC:0x%x,mt:0x%x", txn->mc, txn->mt); mutex_unlock(&ctrl->tx_lock); @@ -900,8 +901,8 @@ static int qcom_slim_ngd_xfer_msg(struct slim_controller *sctrl, }
if (usr_msg) { - timeout = wait_for_completion_timeout(&done, HZ); - if (!timeout) { + time_left = wait_for_completion_timeout(&done, HZ); + if (!time_left) { dev_err(sctrl->dev, "TX timed out:MC:0x%x,mt:0x%x", txn->mc, txn->mt); mutex_unlock(&ctrl->tx_lock); @@ -917,7 +918,8 @@ static int qcom_slim_ngd_xfer_msg_sync(struct slim_controller *ctrl, struct slim_msg_txn *txn) { DECLARE_COMPLETION_ONSTACK(done); - int ret, timeout; + int ret; + unsigned long time_left;
ret = pm_runtime_get_sync(ctrl->dev); if (ret < 0) @@ -929,8 +931,8 @@ static int qcom_slim_ngd_xfer_msg_sync(struct slim_controller *ctrl, if (ret) goto pm_put;
- timeout = wait_for_completion_timeout(&done, HZ); - if (!timeout) { + time_left = wait_for_completion_timeout(&done, HZ); + if (!time_left) { dev_err(ctrl->dev, "TX timed out:MC:0x%x,mt:0x%x", txn->mc, txn->mt); ret = -ETIMEDOUT; @@ -1169,11 +1171,12 @@ static int qcom_slim_ngd_power_up(struct qcom_slim_ngd_ctrl *ctrl) enum qcom_slim_ngd_state cur_state = ctrl->state; struct qcom_slim_ngd *ngd = ctrl->ngd; u32 laddr, rx_msgq; - int timeout, ret = 0; + int ret = 0; + unsigned long time_left;
if (ctrl->state == QCOM_SLIM_NGD_CTRL_DOWN) { - timeout = wait_for_completion_timeout(&ctrl->qmi.qmi_comp, HZ); - if (!timeout) + time_left = wait_for_completion_timeout(&ctrl->qmi.qmi_comp, HZ); + if (!time_left) return -EREMOTEIO; }
@@ -1218,8 +1221,8 @@ static int qcom_slim_ngd_power_up(struct qcom_slim_ngd_ctrl *ctrl) ngd->base + NGD_RX_MSGQ_CFG); qcom_slim_ngd_setup(ctrl);
- timeout = wait_for_completion_timeout(&ctrl->reconf, HZ); - if (!timeout) { + time_left = wait_for_completion_timeout(&ctrl->reconf, HZ); + if (!time_left) { dev_err(ctrl->dev, "capability exchange timed-out\n"); return -ETIMEDOUT; }
On Tue, Apr 30, 2024 at 02:00:58PM +0200, Wolfram Sang wrote:
There is a confusing pattern in the kernel to use a variable named 'timeout' to store the result of wait_for_*() 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 (some parts may still be WIP):
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.
Reviewed-by: Bjorn Andersson quic_bjorande@quicinc.com
Regards, Bjorn
Wolfram Sang (3): slimbus: messaging: use 'time_left' variable with wait_for_completion_timeout() slimbus: qcom-ctrl: use 'time_left' variable with wait_for_completion_timeout() slimbus: qcom-ngd-ctrl: use 'time_left' variable with wait_for_completion_timeout()
drivers/slimbus/messaging.c | 9 +++++---- drivers/slimbus/qcom-ctrl.c | 7 ++++--- drivers/slimbus/qcom-ngd-ctrl.c | 29 ++++++++++++++++------------- 3 files changed, 25 insertions(+), 20 deletions(-)
-- 2.43.0
On Tue, Apr 30, 2024 at 06:30:00AM -0700, Bjorn Andersson wrote:
On Tue, Apr 30, 2024 at 02:00:58PM +0200, Wolfram Sang wrote:
There is a confusing pattern in the kernel to use a variable named 'timeout' to store the result of wait_for_*() 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 (some parts may still be WIP):
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.
Reviewed-by: Bjorn Andersson quic_bjorande@quicinc.com
Thanks, Bjorn. Is Srinivas still pick slimbus patches?
On Tue, 30 Apr 2024 14:00:58 +0200, Wolfram Sang wrote:
There is a confusing pattern in the kernel to use a variable named 'timeout' to store the result of wait_for_*() 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.
[...]
Applied, thanks!
[1/3] slimbus: messaging: use 'time_left' variable with wait_for_completion_timeout() commit: 0eb9dda9d1db40acbabe923fe22c002e13890d39 [2/3] slimbus: qcom-ctrl: use 'time_left' variable with wait_for_completion_timeout() commit: 7d317b95d0334371481ec00ca31f5bf76bae8a82 [3/3] slimbus: qcom-ngd-ctrl: use 'time_left' variable with wait_for_completion_timeout() commit: 9f5fd5e2aebf06c37355cacc7f4b4410bc0ea233
Best regards,
participants (3)
-
Bjorn Andersson -
Srinivas Kandagatla -
Wolfram Sang