[PATCH RFC] ufs: Make ufshcd_wait_for_register() sleep instead of busy-waiting

[PATCH RFC] ufs: Make ufshcd_wait_for_register() sleep instead of busy-waiting

[Bart Van Assche]

The ufshcd_wait_for_register() function either sleeps or spins until the
specified register has reached the desired value. Busy-waiting is not
only considered a bad practice but also has a bad impact on energy
consumption. Always sleep instead of spinning by making sure that all
ufshcd_wait_for_register() calls happen from a context where it is
allowed to sleep. The only function call that has to be moved is the
ufshcd_hba_stop() call in ufshcd_host_reset_and_restore().

Cc: Can Guo <cang@codeaurora.org>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Bean Huo <beanhuo@micron.com>
Cc: Alim Akhtar <alim.akhtar@samsung.com>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 51 +++++++++++++++++++++------------------
 drivers/scsi/ufs/ufshcd.h |  2 +-
 2 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index bf7caea2253b..72069d4faa1e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -563,21 +563,21 @@ void ufshcd_delay_us(unsigned long us, unsigned long tolerance)
 }
 EXPORT_SYMBOL_GPL(ufshcd_delay_us);
 
-/*
+/**
  * ufshcd_wait_for_register - wait for register value to change
- * @hba - per-adapter interface
- * @reg - mmio register offset
- * @mask - mask to apply to read register value
- * @val - wait condition
- * @interval_us - polling interval in microsecs
- * @timeout_ms - timeout in millisecs
- * @can_sleep - perform sleep or just spin
+ * @hba: per-adapter interface
+ * @reg: mmio register offset
+ * @mask: mask to apply to the read register value
+ * @val: value to wait for
+ * @interval_us: polling interval in microseconds
+ * @timeout_ms: timeout in milliseconds
  *
- * Returns -ETIMEDOUT on error, zero on success
+ * Return:
+ * -ETIMEDOUT on error, zero on success.
  */
 int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 mask,
 				u32 val, unsigned long interval_us,
-				unsigned long timeout_ms, bool can_sleep)
+				unsigned long timeout_ms)
 {
 	int err = 0;
 	unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms);
@@ -586,10 +586,7 @@ int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 mask,
 	val = val & mask;
 
 	while ((ufshcd_readl(hba, reg) & mask) != val) {
-		if (can_sleep)
-			usleep_range(interval_us, interval_us + 50);
-		else
-			udelay(interval_us);
+		usleep_range(interval_us, interval_us + 50);
 		if (time_after(jiffies, timeout)) {
 			if ((ufshcd_readl(hba, reg) & mask) != val)
 				err = -ETIMEDOUT;
@@ -2589,7 +2586,7 @@ ufshcd_clear_cmd(struct ufs_hba *hba, int tag)
 	 */
 	err = ufshcd_wait_for_register(hba,
 			REG_UTP_TRANSFER_REQ_DOOR_BELL,
-			mask, ~mask, 1000, 1000, true);
+			mask, ~mask, 1000, 1000);
 
 	return err;
 }
@@ -4258,16 +4255,23 @@ EXPORT_SYMBOL_GPL(ufshcd_make_hba_operational);
 /**
  * ufshcd_hba_stop - Send controller to reset state
  * @hba: per adapter instance
- * @can_sleep: perform sleep or just spin
  */
-static inline void ufshcd_hba_stop(struct ufs_hba *hba, bool can_sleep)
+static inline void ufshcd_hba_stop(struct ufs_hba *hba)
 {
+	unsigned long flags;
 	int err;
 
+	/*
+	 * Obtain the host lock to prevent that the controller is disabled
+	 * while the UFS interrupt handler is active on another CPU.
+	 */
+	spin_lock_irqsave(hba->host->host_lock, flags);
 	ufshcd_writel(hba, CONTROLLER_DISABLE,  REG_CONTROLLER_ENABLE);
+	spin_unlock_irqrestore(hba->host->host_lock, flags);
+
 	err = ufshcd_wait_for_register(hba, REG_CONTROLLER_ENABLE,
 					CONTROLLER_ENABLE, CONTROLLER_DISABLE,
-					10, 1, can_sleep);
+					10, 1);
 	if (err)
 		dev_err(hba->dev, "%s: Controller disable failed\n", __func__);
 }
@@ -4288,7 +4292,7 @@ int ufshcd_hba_enable(struct ufs_hba *hba)
 
 	if (!ufshcd_is_hba_active(hba))
 		/* change controller state to "reset state" */
-		ufshcd_hba_stop(hba, true);
+		ufshcd_hba_stop(hba);
 
 	/* UniPro link is disabled at this point */
 	ufshcd_set_link_off(hba);
@@ -5911,7 +5915,7 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag)
 	/* poll for max. 1 sec to clear door bell register by h/w */
 	err = ufshcd_wait_for_register(hba,
 			REG_UTP_TASK_REQ_DOOR_BELL,
-			mask, 0, 1000, 1000, true);
+			mask, 0, 1000, 1000);
 out:
 	return err;
 }
@@ -6478,8 +6482,9 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
 	 * Stop the host controller and complete the requests
 	 * cleared by h/w
 	 */
+	ufshcd_hba_stop(hba);
+
 	spin_lock_irqsave(hba->host->host_lock, flags);
-	ufshcd_hba_stop(hba, false);
 	hba->silence_err_logs = true;
 	ufshcd_complete_requests(hba);
 	hba->silence_err_logs = false;
@@ -7986,7 +7991,7 @@ static int ufshcd_link_state_transition(struct ufs_hba *hba,
 		 * Change controller state to "reset state" which
 		 * should also put the link in off/reset state
 		 */
-		ufshcd_hba_stop(hba, true);
+		ufshcd_hba_stop(hba);
 		/*
 		 * TODO: Check if we need any delay to make sure that
 		 * controller is reset
@@ -8545,7 +8550,7 @@ void ufshcd_remove(struct ufs_hba *hba)
 	scsi_remove_host(hba->host);
 	/* disable interrupts */
 	ufshcd_disable_intr(hba, hba->intr_mask);
-	ufshcd_hba_stop(hba, true);
+	ufshcd_hba_stop(hba);
 
 	ufshcd_exit_clk_scaling(hba);
 	ufshcd_exit_clk_gating(hba);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 733722840794..5c944530cde5 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -822,7 +822,7 @@ int ufshcd_uic_hibern8_exit(struct ufs_hba *hba);
 void ufshcd_delay_us(unsigned long us, unsigned long tolerance);
 int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 mask,
 				u32 val, unsigned long interval_us,
-				unsigned long timeout_ms, bool can_sleep);
+				unsigned long timeout_ms);
 void ufshcd_parse_dev_ref_clk_freq(struct ufs_hba *hba, struct clk *refclk);
 void ufshcd_update_reg_hist(struct ufs_err_reg_hist *reg_hist,
 			    u32 reg);

Re: [PATCH RFC] ufs: Make ufshcd_wait_for_register() sleep instead of busy-waiting

[Asutosh Das (asd)]

On 5/7/2020 3:27 PM, Bart Van Assche wrote:
> The ufshcd_wait_for_register() function either sleeps or spins until the
> specified register has reached the desired value. Busy-waiting is not
> only considered a bad practice but also has a bad impact on energy
> consumption. Always sleep instead of spinning by making sure that all
> ufshcd_wait_for_register() calls happen from a context where it is
> allowed to sleep. The only function call that has to be moved is the
> ufshcd_hba_stop() call in ufshcd_host_reset_and_restore().
> 
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Alim Akhtar <alim.akhtar@samsung.com>
> Cc: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>


>   drivers/scsi/ufs/ufshcd.c | 51 +++++++++++++++++++++------------------
>   drivers/scsi/ufs/ufshcd.h |  2 +-
>   2 files changed, 29 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index bf7caea2253b..72069d4faa1e 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -563,21 +563,21 @@ void ufshcd_delay_us(unsigned long us, unsigned long tolerance)
>   }
>   EXPORT_SYMBOL_GPL(ufshcd_delay_us);
>   
> -/*
> +/**
>    * ufshcd_wait_for_register - wait for register value to change
> - * @hba - per-adapter interface
> - * @reg - mmio register offset
> - * @mask - mask to apply to read register value
> - * @val - wait condition
> - * @interval_us - polling interval in microsecs
> - * @timeout_ms - timeout in millisecs
> - * @can_sleep - perform sleep or just spin
> + * @hba: per-adapter interface
> + * @reg: mmio register offset
> + * @mask: mask to apply to the read register value
> + * @val: value to wait for
> + * @interval_us: polling interval in microseconds
> + * @timeout_ms: timeout in milliseconds
>    *
> - * Returns -ETIMEDOUT on error, zero on success
> + * Return:
> + * -ETIMEDOUT on error, zero on success.
>    */
>   int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 mask,
>   				u32 val, unsigned long interval_us,
> -				unsigned long timeout_ms, bool can_sleep)
> +				unsigned long timeout_ms)
>   {
>   	int err = 0;
>   	unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms);
> @@ -586,10 +586,7 @@ int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 mask,
>   	val = val & mask;
>   
>   	while ((ufshcd_readl(hba, reg) & mask) != val) {
> -		if (can_sleep)
> -			usleep_range(interval_us, interval_us + 50);
> -		else
> -			udelay(interval_us);
> +		usleep_range(interval_us, interval_us + 50);
>   		if (time_after(jiffies, timeout)) {
>   			if ((ufshcd_readl(hba, reg) & mask) != val)
>   				err = -ETIMEDOUT;
> @@ -2589,7 +2586,7 @@ ufshcd_clear_cmd(struct ufs_hba *hba, int tag)
>   	 */
>   	err = ufshcd_wait_for_register(hba,
>   			REG_UTP_TRANSFER_REQ_DOOR_BELL,
> -			mask, ~mask, 1000, 1000, true);
> +			mask, ~mask, 1000, 1000);
>   
>   	return err;
>   }
> @@ -4258,16 +4255,23 @@ EXPORT_SYMBOL_GPL(ufshcd_make_hba_operational);
>   /**
>    * ufshcd_hba_stop - Send controller to reset state
>    * @hba: per adapter instance
> - * @can_sleep: perform sleep or just spin
>    */
> -static inline void ufshcd_hba_stop(struct ufs_hba *hba, bool can_sleep)
> +static inline void ufshcd_hba_stop(struct ufs_hba *hba)
>   {
> +	unsigned long flags;
>   	int err;
>   
> +	/*
> +	 * Obtain the host lock to prevent that the controller is disabled
> +	 * while the UFS interrupt handler is active on another CPU.
> +	 */
> +	spin_lock_irqsave(hba->host->host_lock, flags);
>   	ufshcd_writel(hba, CONTROLLER_DISABLE,  REG_CONTROLLER_ENABLE);
> +	spin_unlock_irqrestore(hba->host->host_lock, flags);
> +
>   	err = ufshcd_wait_for_register(hba, REG_CONTROLLER_ENABLE,
>   					CONTROLLER_ENABLE, CONTROLLER_DISABLE,
> -					10, 1, can_sleep);
> +					10, 1);
>   	if (err)
>   		dev_err(hba->dev, "%s: Controller disable failed\n", __func__);
>   }
> @@ -4288,7 +4292,7 @@ int ufshcd_hba_enable(struct ufs_hba *hba)
>   
>   	if (!ufshcd_is_hba_active(hba))
>   		/* change controller state to "reset state" */
> -		ufshcd_hba_stop(hba, true);
> +		ufshcd_hba_stop(hba);
>   
>   	/* UniPro link is disabled at this point */
>   	ufshcd_set_link_off(hba);
> @@ -5911,7 +5915,7 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag)
>   	/* poll for max. 1 sec to clear door bell register by h/w */
>   	err = ufshcd_wait_for_register(hba,
>   			REG_UTP_TASK_REQ_DOOR_BELL,
> -			mask, 0, 1000, 1000, true);
> +			mask, 0, 1000, 1000);
>   out:
>   	return err;
>   }
> @@ -6478,8 +6482,9 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
>   	 * Stop the host controller and complete the requests
>   	 * cleared by h/w
>   	 */
> +	ufshcd_hba_stop(hba);
> +
>   	spin_lock_irqsave(hba->host->host_lock, flags);
> -	ufshcd_hba_stop(hba, false);
>   	hba->silence_err_logs = true;
>   	ufshcd_complete_requests(hba);
>   	hba->silence_err_logs = false;
> @@ -7986,7 +7991,7 @@ static int ufshcd_link_state_transition(struct ufs_hba *hba,
>   		 * Change controller state to "reset state" which
>   		 * should also put the link in off/reset state
>   		 */
> -		ufshcd_hba_stop(hba, true);
> +		ufshcd_hba_stop(hba);
>   		/*
>   		 * TODO: Check if we need any delay to make sure that
>   		 * controller is reset
> @@ -8545,7 +8550,7 @@ void ufshcd_remove(struct ufs_hba *hba)
>   	scsi_remove_host(hba->host);
>   	/* disable interrupts */
>   	ufshcd_disable_intr(hba, hba->intr_mask);
> -	ufshcd_hba_stop(hba, true);
> +	ufshcd_hba_stop(hba);
>   
>   	ufshcd_exit_clk_scaling(hba);
>   	ufshcd_exit_clk_gating(hba);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 733722840794..5c944530cde5 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -822,7 +822,7 @@ int ufshcd_uic_hibern8_exit(struct ufs_hba *hba);
>   void ufshcd_delay_us(unsigned long us, unsigned long tolerance);
>   int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 mask,
>   				u32 val, unsigned long interval_us,
> -				unsigned long timeout_ms, bool can_sleep);
> +				unsigned long timeout_ms);
>   void ufshcd_parse_dev_ref_clk_freq(struct ufs_hba *hba, struct clk *refclk);
>   void ufshcd_update_reg_hist(struct ufs_err_reg_hist *reg_hist,
>   			    u32 reg);
> 


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project

Re: [PATCH RFC] ufs: Make ufshcd_wait_for_register() sleep instead of busy-waiting

[Bart Van Assche]

On 2020-05-08 09:27, Asutosh Das (asd) wrote:
> On 5/7/2020 3:27 PM, Bart Van Assche wrote:
>> The ufshcd_wait_for_register() function either sleeps or spins until the
>> specified register has reached the desired value. Busy-waiting is not
>> only considered a bad practice but also has a bad impact on energy
>> consumption. Always sleep instead of spinning by making sure that all
>> ufshcd_wait_for_register() calls happen from a context where it is
>> allowed to sleep. The only function call that has to be moved is the
>> ufshcd_hba_stop() call in ufshcd_host_reset_and_restore().
>>
>> Cc: Can Guo <cang@codeaurora.org>
>> Cc: Avri Altman <avri.altman@wdc.com>
>> Cc: Bean Huo <beanhuo@micron.com>
>> Cc: Alim Akhtar <alim.akhtar@samsung.com>
>> Cc: Asutosh Das <asutoshd@codeaurora.org>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>
> Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>

Thanks for the review Asutosh. Does anyone else want to review and/or test this patch?

Thanks,

Bart.

RE: [EXT] Re: [PATCH RFC] ufs: Make ufshcd_wait_for_register() sleep instead of busy-waiting

[Bean Huo (beanhuo)]

> >>
> >> Cc: Can Guo <cang@codeaurora.org>
> >> Cc: Avri Altman <avri.altman@wdc.com>
> >> Cc: Bean Huo <beanhuo@micron.com>
> >> Cc: Alim Akhtar <alim.akhtar@samsung.com>
> >> Cc: Asutosh Das <asutoshd@codeaurora.org>
> >> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> >
> > Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>
> 
> Thanks for the review Asutosh. Does anyone else want to review and/or test this
> patch?
> 
> Thanks,
> 
> Bart.

Bart,
I will help you to test your patch on our platform this weekend, make sure problem free.

Bean

Re: [PATCH RFC] ufs: Make ufshcd_wait_for_register() sleep instead of busy-waiting

[Stanley Chu]

Hi Bart,

On Fri, 2020-05-15 at 12:28 -0700, Bart Van Assche wrote:
> On 2020-05-08 09:27, Asutosh Das (asd) wrote:
> > On 5/7/2020 3:27 PM, Bart Van Assche wrote:
> >> The ufshcd_wait_for_register() function either sleeps or spins until the
> >> specified register has reached the desired value. Busy-waiting is not
> >> only considered a bad practice but also has a bad impact on energy
> >> consumption. Always sleep instead of spinning by making sure that all
> >> ufshcd_wait_for_register() calls happen from a context where it is
> >> allowed to sleep. The only function call that has to be moved is the
> >> ufshcd_hba_stop() call in ufshcd_host_reset_and_restore().
> >>
> >> Cc: Can Guo <cang@codeaurora.org>
> >> Cc: Avri Altman <avri.altman@wdc.com>
> >> Cc: Bean Huo <beanhuo@micron.com>
> >> Cc: Alim Akhtar <alim.akhtar@samsung.com>
> >> Cc: Asutosh Das <asutoshd@codeaurora.org>
> >> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

RE: [EXT] Re: [PATCH RFC] ufs: Make ufshcd_wait_for_register() sleep instead of busy-waiting

[Bean Huo (beanhuo)]

Reviewed-by: Bean Huo <beanhuo@micron.com>
Tested-by: Bean Huo <beanhuo@micron.com>

Re: [PATCH RFC] ufs: Make ufshcd_wait_for_register() sleep instead of busy-waiting

[Martin K. Petersen]

On Thu, 7 May 2020 15:27:50 -0700, Bart Van Assche wrote:

> The ufshcd_wait_for_register() function either sleeps or spins until the
> specified register has reached the desired value. Busy-waiting is not
> only considered a bad practice but also has a bad impact on energy
> consumption. Always sleep instead of spinning by making sure that all
> ufshcd_wait_for_register() calls happen from a context where it is
> allowed to sleep. The only function call that has to be moved is the
> ufshcd_hba_stop() call in ufshcd_host_reset_and_restore().

Applied to 5.8/scsi-queue, thanks!

[1/1] scsi: ufs: Make ufshcd_wait_for_register() sleep instead of busy-waiting
      https://git.kernel.org/mkp/scsi/c/5cac1095cf28

-- 
Martin K. Petersen	Oracle Linux Engineering