[PATCH 1/4] scsi: ufs: atomic update for clkgating_enable

[PATCH 1/4] scsi: ufs: atomic update for clkgating_enable

[Jaegeuk Kim]

From: Jaegeuk Kim <jaegeuk@google.com>

When giving a stress test which enables/disables clkgating, we hit device
timeout sometimes. This patch avoids subtle racy condition to address it.

Cc: Alim Akhtar <alim.akhtar@samsung.com>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Can Guo <cang@codeaurora.org>
Signed-off-by: Jaegeuk Kim <jaegeuk@google.com>
---
 drivers/scsi/ufs/ufshcd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 1d157ff58d817..d929c3d1e58cc 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1791,19 +1791,19 @@ static ssize_t ufshcd_clkgate_enable_store(struct device *dev,
 		return -EINVAL;
 
 	value = !!value;
+
+	spin_lock_irqsave(hba->host->host_lock, flags);
 	if (value == hba->clk_gating.is_enabled)
 		goto out;
 
-	if (value) {
-		ufshcd_release(hba);
-	} else {
-		spin_lock_irqsave(hba->host->host_lock, flags);
+	if (value)
+		hba->clk_gating.active_reqs--;
+	else
 		hba->clk_gating.active_reqs++;
-		spin_unlock_irqrestore(hba->host->host_lock, flags);
-	}
 
 	hba->clk_gating.is_enabled = value;
 out:
+	spin_unlock_irqrestore(hba->host->host_lock, flags);
 	return count;
 }
 
-- 
2.28.0.806.g8561365e88-goog

[PATCH 2/4] scsi: ufs: clear UAC for FFU and RPMB LUNs

[Jaegeuk Kim]

From: Jaegeuk Kim <jaegeuk@google.com>

In order to conduct FFU or RPMB operations, UFS needs to clear UAC. This patch
clears it explicitly, so that we could get no failure given early execution.

Cc: Alim Akhtar <alim.akhtar@samsung.com>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Can Guo <cang@codeaurora.org>
Signed-off-by: Jaegeuk Kim <jaegeuk@google.com>
---
 drivers/scsi/ufs/ufshcd.c | 70 +++++++++++++++++++++++++++++++++++----
 drivers/scsi/ufs/ufshcd.h |  1 +
 2 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index d929c3d1e58cc..0bb07b50bd23e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6841,7 +6841,6 @@ static inline void ufshcd_blk_pm_runtime_init(struct scsi_device *sdev)
 static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
 {
 	int ret = 0;
-	struct scsi_device *sdev_rpmb;
 	struct scsi_device *sdev_boot;
 
 	hba->sdev_ufs_device = __scsi_add_device(hba->host, 0, 0,
@@ -6854,14 +6853,14 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
 	ufshcd_blk_pm_runtime_init(hba->sdev_ufs_device);
 	scsi_device_put(hba->sdev_ufs_device);
 
-	sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
+	hba->sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
 		ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN), NULL);
-	if (IS_ERR(sdev_rpmb)) {
-		ret = PTR_ERR(sdev_rpmb);
+	if (IS_ERR(hba->sdev_rpmb)) {
+		ret = PTR_ERR(hba->sdev_rpmb);
 		goto remove_sdev_ufs_device;
 	}
-	ufshcd_blk_pm_runtime_init(sdev_rpmb);
-	scsi_device_put(sdev_rpmb);
+	ufshcd_blk_pm_runtime_init(hba->sdev_rpmb);
+	scsi_device_put(hba->sdev_rpmb);
 
 	sdev_boot = __scsi_add_device(hba->host, 0, 0,
 		ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_BOOT_WLUN), NULL);
@@ -7385,6 +7384,63 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
 	return ret;
 }
 
+static int
+ufshcd_send_request_sense(struct ufs_hba *hba, struct scsi_device *sdp);
+
+static int ufshcd_clear_ua_wlun(struct ufs_hba *hba, u8 wlun)
+{
+	struct scsi_device *sdp;
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(hba->host->host_lock, flags);
+	if (wlun  == UFS_UPIU_UFS_DEVICE_WLUN)
+		sdp = hba->sdev_ufs_device;
+	else if (wlun  == UFS_UPIU_RPMB_WLUN)
+		sdp = hba->sdev_rpmb;
+	else
+		BUG_ON(1);
+	if (sdp) {
+		ret = scsi_device_get(sdp);
+		if (!ret && !scsi_device_online(sdp)) {
+			ret = -ENODEV;
+			scsi_device_put(sdp);
+		}
+	} else {
+		ret = -ENODEV;
+	}
+	spin_unlock_irqrestore(hba->host->host_lock, flags);
+	if (ret)
+		goto out_err;
+
+	ret = ufshcd_send_request_sense(hba, sdp);
+	scsi_device_put(sdp);
+out_err:
+	if (ret)
+		dev_err(hba->dev, "%s: UAC clear LU=%x ret = %d\n",
+				__func__, wlun, ret);
+	return ret;
+}
+
+static int ufshcd_clear_ua_wluns(struct ufs_hba *hba)
+{
+	int ret = 0;
+
+	if (!hba->wlun_dev_clr_ua)
+		goto out;
+
+	ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_UFS_DEVICE_WLUN);
+	if (!ret)
+		ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN);
+	if (!ret)
+		hba->wlun_dev_clr_ua = false;
+out:
+	if (ret)
+		dev_err(hba->dev, "%s: Failed to clear UAC WLUNS ret = %d\n",
+				__func__, ret);
+	return ret;
+}
+
 /**
  * ufshcd_probe_hba - probe hba to detect device and initialize
  * @hba: per-adapter instance
@@ -7500,6 +7556,8 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 		pm_runtime_put_sync(hba->dev);
 		ufshcd_exit_clk_scaling(hba);
 		ufshcd_hba_exit(hba);
+	} else {
+		ufshcd_clear_ua_wluns(hba);
 	}
 }
 
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 363589c0bd370..8344d8cb36786 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -662,6 +662,7 @@ struct ufs_hba {
 	 * "UFS device" W-LU.
 	 */
 	struct scsi_device *sdev_ufs_device;
+	struct scsi_device *sdev_rpmb;
 
 	enum ufs_dev_pwr_mode curr_dev_pwr_mode;
 	enum uic_link_state uic_link_state;
-- 
2.28.0.806.g8561365e88-goog

Re: [PATCH 2/4] scsi: ufs: clear UAC for FFU and RPMB LUNs

[Can Guo]

On 2020-10-06 06:36, Jaegeuk Kim wrote:
> From: Jaegeuk Kim <jaegeuk@google.com>
> 
> In order to conduct FFU or RPMB operations, UFS needs to clear UAC. 
> This patch
> clears it explicitly, so that we could get no failure given early 
> execution.
> 

Usually it is the user's/utility's/tool's responsiblity to clear UA by 
sending a
request sense cmd and retry previous cmd, now we are doing it for the 
users in driver?
As per my understanding, driver only reports UA to SCSI layer and let 
users decide
what to do with it - maybe users need to do something specifically regs 
it, but
the change clears it even before the users get to know it.

Besides, this change clears UA for W-LUs, but the UFS driver still 
reports UA to SCSI
layer for each SCSI device by calling scsi_report_bus_reset() in
ufshcd_reset_and_restore(). This will make SCSI layer treat 
sdev->expecting_cc_ua
wrongly, because for W-LUs, their expecting_cc_ua should not be set as 
you have
cleared their UAs.

Thanks,

Can Guo.

> Cc: Alim Akhtar <alim.akhtar@samsung.com>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Can Guo <cang@codeaurora.org>
> Signed-off-by: Jaegeuk Kim <jaegeuk@google.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 70 +++++++++++++++++++++++++++++++++++----
>  drivers/scsi/ufs/ufshcd.h |  1 +
>  2 files changed, 65 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index d929c3d1e58cc..0bb07b50bd23e 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6841,7 +6841,6 @@ static inline void
> ufshcd_blk_pm_runtime_init(struct scsi_device *sdev)
>  static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
>  {
>  	int ret = 0;
> -	struct scsi_device *sdev_rpmb;
>  	struct scsi_device *sdev_boot;
> 
>  	hba->sdev_ufs_device = __scsi_add_device(hba->host, 0, 0,
> @@ -6854,14 +6853,14 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba 
> *hba)
>  	ufshcd_blk_pm_runtime_init(hba->sdev_ufs_device);
>  	scsi_device_put(hba->sdev_ufs_device);
> 
> -	sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
> +	hba->sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
>  		ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN), NULL);
> -	if (IS_ERR(sdev_rpmb)) {
> -		ret = PTR_ERR(sdev_rpmb);
> +	if (IS_ERR(hba->sdev_rpmb)) {
> +		ret = PTR_ERR(hba->sdev_rpmb);
>  		goto remove_sdev_ufs_device;
>  	}
> -	ufshcd_blk_pm_runtime_init(sdev_rpmb);
> -	scsi_device_put(sdev_rpmb);
> +	ufshcd_blk_pm_runtime_init(hba->sdev_rpmb);
> +	scsi_device_put(hba->sdev_rpmb);
> 
>  	sdev_boot = __scsi_add_device(hba->host, 0, 0,
>  		ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_BOOT_WLUN), NULL);
> @@ -7385,6 +7384,63 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
>  	return ret;
>  }
> 
> +static int
> +ufshcd_send_request_sense(struct ufs_hba *hba, struct scsi_device 
> *sdp);
> +
> +static int ufshcd_clear_ua_wlun(struct ufs_hba *hba, u8 wlun)
> +{
> +	struct scsi_device *sdp;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	spin_lock_irqsave(hba->host->host_lock, flags);
> +	if (wlun  == UFS_UPIU_UFS_DEVICE_WLUN)
> +		sdp = hba->sdev_ufs_device;
> +	else if (wlun  == UFS_UPIU_RPMB_WLUN)
> +		sdp = hba->sdev_rpmb;
> +	else
> +		BUG_ON(1);
> +	if (sdp) {
> +		ret = scsi_device_get(sdp);
> +		if (!ret && !scsi_device_online(sdp)) {
> +			ret = -ENODEV;
> +			scsi_device_put(sdp);
> +		}
> +	} else {
> +		ret = -ENODEV;
> +	}
> +	spin_unlock_irqrestore(hba->host->host_lock, flags);
> +	if (ret)
> +		goto out_err;
> +
> +	ret = ufshcd_send_request_sense(hba, sdp);
> +	scsi_device_put(sdp);
> +out_err:
> +	if (ret)
> +		dev_err(hba->dev, "%s: UAC clear LU=%x ret = %d\n",
> +				__func__, wlun, ret);
> +	return ret;
> +}
> +
> +static int ufshcd_clear_ua_wluns(struct ufs_hba *hba)
> +{
> +	int ret = 0;
> +
> +	if (!hba->wlun_dev_clr_ua)
> +		goto out;
> +
> +	ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_UFS_DEVICE_WLUN);
> +	if (!ret)
> +		ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN);
> +	if (!ret)
> +		hba->wlun_dev_clr_ua = false;
> +out:
> +	if (ret)
> +		dev_err(hba->dev, "%s: Failed to clear UAC WLUNS ret = %d\n",
> +				__func__, ret);
> +	return ret;
> +}
> +
>  /**
>   * ufshcd_probe_hba - probe hba to detect device and initialize
>   * @hba: per-adapter instance
> @@ -7500,6 +7556,8 @@ static void ufshcd_async_scan(void *data,
> async_cookie_t cookie)
>  		pm_runtime_put_sync(hba->dev);
>  		ufshcd_exit_clk_scaling(hba);
>  		ufshcd_hba_exit(hba);
> +	} else {
> +		ufshcd_clear_ua_wluns(hba);
>  	}
>  }
> 
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 363589c0bd370..8344d8cb36786 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -662,6 +662,7 @@ struct ufs_hba {
>  	 * "UFS device" W-LU.
>  	 */
>  	struct scsi_device *sdev_ufs_device;
> +	struct scsi_device *sdev_rpmb;
> 
>  	enum ufs_dev_pwr_mode curr_dev_pwr_mode;
>  	enum uic_link_state uic_link_state;

Re: [PATCH 2/4] scsi: ufs: clear UAC for FFU and RPMB LUNs

[jaegeuk]

On 10/20, Can Guo wrote:
> On 2020-10-06 06:36, Jaegeuk Kim wrote:
> > From: Jaegeuk Kim <jaegeuk@google.com>
> > 
> > In order to conduct FFU or RPMB operations, UFS needs to clear UAC. This
> > patch
> > clears it explicitly, so that we could get no failure given early
> > execution.
> > 
> 
> Usually it is the user's/utility's/tool's responsiblity to clear UA by
> sending a
> request sense cmd and retry previous cmd, now we are doing it for the users
> in driver?
> As per my understanding, driver only reports UA to SCSI layer and let users
> decide
> what to do with it - maybe users need to do something specifically regs it,
> but
> the change clears it even before the users get to know it.

Well, instead of your expectation, many users have actually complained about why
they need to do this. Even, taking a look at some custom implementations given
by SoC branches, they simply clears it internally.

> 
> Besides, this change clears UA for W-LUs, but the UFS driver still reports
> UA to SCSI
> layer for each SCSI device by calling scsi_report_bus_reset() in
> ufshcd_reset_and_restore(). This will make SCSI layer treat
> sdev->expecting_cc_ua
> wrongly, because for W-LUs, their expecting_cc_ua should not be set as you
> have
> cleared their UAs.

Is that fully restricted? What is the buggy scenario if sub-scsi clears it under
its control?

> 
> Thanks,
> 
> Can Guo.
> 
> > Cc: Alim Akhtar <alim.akhtar@samsung.com>
> > Cc: Avri Altman <avri.altman@wdc.com>
> > Cc: Can Guo <cang@codeaurora.org>
> > Signed-off-by: Jaegeuk Kim <jaegeuk@google.com>
> > ---
> >  drivers/scsi/ufs/ufshcd.c | 70 +++++++++++++++++++++++++++++++++++----
> >  drivers/scsi/ufs/ufshcd.h |  1 +
> >  2 files changed, 65 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index d929c3d1e58cc..0bb07b50bd23e 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -6841,7 +6841,6 @@ static inline void
> > ufshcd_blk_pm_runtime_init(struct scsi_device *sdev)
> >  static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
> >  {
> >  	int ret = 0;
> > -	struct scsi_device *sdev_rpmb;
> >  	struct scsi_device *sdev_boot;
> > 
> >  	hba->sdev_ufs_device = __scsi_add_device(hba->host, 0, 0,
> > @@ -6854,14 +6853,14 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba
> > *hba)
> >  	ufshcd_blk_pm_runtime_init(hba->sdev_ufs_device);
> >  	scsi_device_put(hba->sdev_ufs_device);
> > 
> > -	sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
> > +	hba->sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
> >  		ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN), NULL);
> > -	if (IS_ERR(sdev_rpmb)) {
> > -		ret = PTR_ERR(sdev_rpmb);
> > +	if (IS_ERR(hba->sdev_rpmb)) {
> > +		ret = PTR_ERR(hba->sdev_rpmb);
> >  		goto remove_sdev_ufs_device;
> >  	}
> > -	ufshcd_blk_pm_runtime_init(sdev_rpmb);
> > -	scsi_device_put(sdev_rpmb);
> > +	ufshcd_blk_pm_runtime_init(hba->sdev_rpmb);
> > +	scsi_device_put(hba->sdev_rpmb);
> > 
> >  	sdev_boot = __scsi_add_device(hba->host, 0, 0,
> >  		ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_BOOT_WLUN), NULL);
> > @@ -7385,6 +7384,63 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
> >  	return ret;
> >  }
> > 
> > +static int
> > +ufshcd_send_request_sense(struct ufs_hba *hba, struct scsi_device
> > *sdp);
> > +
> > +static int ufshcd_clear_ua_wlun(struct ufs_hba *hba, u8 wlun)
> > +{
> > +	struct scsi_device *sdp;
> > +	unsigned long flags;
> > +	int ret = 0;
> > +
> > +	spin_lock_irqsave(hba->host->host_lock, flags);
> > +	if (wlun  == UFS_UPIU_UFS_DEVICE_WLUN)
> > +		sdp = hba->sdev_ufs_device;
> > +	else if (wlun  == UFS_UPIU_RPMB_WLUN)
> > +		sdp = hba->sdev_rpmb;
> > +	else
> > +		BUG_ON(1);
> > +	if (sdp) {
> > +		ret = scsi_device_get(sdp);
> > +		if (!ret && !scsi_device_online(sdp)) {
> > +			ret = -ENODEV;
> > +			scsi_device_put(sdp);
> > +		}
> > +	} else {
> > +		ret = -ENODEV;
> > +	}
> > +	spin_unlock_irqrestore(hba->host->host_lock, flags);
> > +	if (ret)
> > +		goto out_err;
> > +
> > +	ret = ufshcd_send_request_sense(hba, sdp);
> > +	scsi_device_put(sdp);
> > +out_err:
> > +	if (ret)
> > +		dev_err(hba->dev, "%s: UAC clear LU=%x ret = %d\n",
> > +				__func__, wlun, ret);
> > +	return ret;
> > +}
> > +
> > +static int ufshcd_clear_ua_wluns(struct ufs_hba *hba)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!hba->wlun_dev_clr_ua)
> > +		goto out;
> > +
> > +	ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_UFS_DEVICE_WLUN);
> > +	if (!ret)
> > +		ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN);
> > +	if (!ret)
> > +		hba->wlun_dev_clr_ua = false;
> > +out:
> > +	if (ret)
> > +		dev_err(hba->dev, "%s: Failed to clear UAC WLUNS ret = %d\n",
> > +				__func__, ret);
> > +	return ret;
> > +}
> > +
> >  /**
> >   * ufshcd_probe_hba - probe hba to detect device and initialize
> >   * @hba: per-adapter instance
> > @@ -7500,6 +7556,8 @@ static void ufshcd_async_scan(void *data,
> > async_cookie_t cookie)
> >  		pm_runtime_put_sync(hba->dev);
> >  		ufshcd_exit_clk_scaling(hba);
> >  		ufshcd_hba_exit(hba);
> > +	} else {
> > +		ufshcd_clear_ua_wluns(hba);
> >  	}
> >  }
> > 
> > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> > index 363589c0bd370..8344d8cb36786 100644
> > --- a/drivers/scsi/ufs/ufshcd.h
> > +++ b/drivers/scsi/ufs/ufshcd.h
> > @@ -662,6 +662,7 @@ struct ufs_hba {
> >  	 * "UFS device" W-LU.
> >  	 */
> >  	struct scsi_device *sdev_ufs_device;
> > +	struct scsi_device *sdev_rpmb;
> > 
> >  	enum ufs_dev_pwr_mode curr_dev_pwr_mode;
> >  	enum uic_link_state uic_link_state;

[PATCH 3/4] scsi: ufs: use WQ_HIGHPRI for gating work

[Jaegeuk Kim]

From: Jaegeuk Kim <jaegeuk@google.com>

Must have WQ_MEM_RECLAIM
``WQ_MEM_RECLAIM``
  All wq which might be used in the memory reclaim paths **MUST**
  have this flag set.  The wq is guaranteed to have at least one
  execution context regardless of memory pressure.

Cc: Alim Akhtar <alim.akhtar@samsung.com>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Can Guo <cang@codeaurora.org>
Signed-off-by: Jaegeuk Kim <jaegeuk@google.com>
---
 drivers/scsi/ufs/ufshcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 0bb07b50bd23e..76e95963887be 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1849,7 +1849,7 @@ static void ufshcd_init_clk_gating(struct ufs_hba *hba)
 	snprintf(wq_name, ARRAY_SIZE(wq_name), "ufs_clk_gating_%d",
 		 hba->host->host_no);
 	hba->clk_gating.clk_gating_workq = alloc_ordered_workqueue(wq_name,
-							   WQ_MEM_RECLAIM);
+					WQ_MEM_RECLAIM | WQ_HIGHPRI);
 
 	hba->clk_gating.is_enabled = true;
 
-- 
2.28.0.806.g8561365e88-goog

Re: [PATCH 3/4] scsi: ufs: use WQ_HIGHPRI for gating work

[Can Guo]

On 2020-10-06 06:36, Jaegeuk Kim wrote:
> From: Jaegeuk Kim <jaegeuk@google.com>
> 
> Must have WQ_MEM_RECLAIM
> ``WQ_MEM_RECLAIM``
>   All wq which might be used in the memory reclaim paths **MUST**
>   have this flag set.  The wq is guaranteed to have at least one
>   execution context regardless of memory pressure.
> 

The commit msg is not telling the same story as the change/title does.

Regards,

Can Guo.

> Cc: Alim Akhtar <alim.akhtar@samsung.com>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Can Guo <cang@codeaurora.org>
> Signed-off-by: Jaegeuk Kim <jaegeuk@google.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 0bb07b50bd23e..76e95963887be 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1849,7 +1849,7 @@ static void ufshcd_init_clk_gating(struct ufs_hba 
> *hba)
>  	snprintf(wq_name, ARRAY_SIZE(wq_name), "ufs_clk_gating_%d",
>  		 hba->host->host_no);
>  	hba->clk_gating.clk_gating_workq = alloc_ordered_workqueue(wq_name,
> -							   WQ_MEM_RECLAIM);
> +					WQ_MEM_RECLAIM | WQ_HIGHPRI);
> 
>  	hba->clk_gating.is_enabled = true;

Re: [PATCH 3/4] scsi: ufs: use WQ_HIGHPRI for gating work

[jaegeuk]

On 10/20, Can Guo wrote:
> On 2020-10-06 06:36, Jaegeuk Kim wrote:
> > From: Jaegeuk Kim <jaegeuk@google.com>
> > 
> > Must have WQ_MEM_RECLAIM
> > ``WQ_MEM_RECLAIM``
> >   All wq which might be used in the memory reclaim paths **MUST**
> >   have this flag set.  The wq is guaranteed to have at least one
> >   execution context regardless of memory pressure.
> > 
> 
> The commit msg is not telling the same story as the change/title does.

This message explains why we need to keep WQ_MEM_RECLAIM when adding WQ_HIGHPRI.

Thanks,

> 
> Regards,
> 
> Can Guo.
> 
> > Cc: Alim Akhtar <alim.akhtar@samsung.com>
> > Cc: Avri Altman <avri.altman@wdc.com>
> > Cc: Can Guo <cang@codeaurora.org>
> > Signed-off-by: Jaegeuk Kim <jaegeuk@google.com>
> > ---
> >  drivers/scsi/ufs/ufshcd.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 0bb07b50bd23e..76e95963887be 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -1849,7 +1849,7 @@ static void ufshcd_init_clk_gating(struct ufs_hba
> > *hba)
> >  	snprintf(wq_name, ARRAY_SIZE(wq_name), "ufs_clk_gating_%d",
> >  		 hba->host->host_no);
> >  	hba->clk_gating.clk_gating_workq = alloc_ordered_workqueue(wq_name,
> > -							   WQ_MEM_RECLAIM);
> > +					WQ_MEM_RECLAIM | WQ_HIGHPRI);
> > 
> >  	hba->clk_gating.is_enabled = true;

[PATCH 4/4] scsi: add more contexts in the ufs tracepoints

[Jaegeuk Kim]

From: Jaegeuk Kim <jaegeuk@google.com>

This adds user-friendly tracepoints with group id.

Cc: Alim Akhtar <alim.akhtar@samsung.com>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Can Guo <cang@codeaurora.org>
Signed-off-by: Jaegeuk Kim <jaegeuk@google.com>
---
 drivers/scsi/ufs/ufshcd.c  |  6 ++++--
 include/trace/events/ufs.h | 21 +++++++++++++++++----
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 76e95963887be..a2db8182663da 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -336,7 +336,7 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba,
 		unsigned int tag, const char *str)
 {
 	sector_t lba = -1;
-	u8 opcode = 0;
+	u8 opcode = 0, group_id = 0;
 	u32 intr, doorbell;
 	struct ufshcd_lrb *lrbp = &hba->lrb[tag];
 	struct scsi_cmnd *cmd = lrbp->cmd;
@@ -362,13 +362,15 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba,
 				lba = cmd->request->bio->bi_iter.bi_sector;
 			transfer_len = be32_to_cpu(
 				lrbp->ucd_req_ptr->sc.exp_data_transfer_len);
+			if (opcode == WRITE_10)
+				group_id = lrbp->cmd->cmnd[6];
 		}
 	}
 
 	intr = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
 	doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
 	trace_ufshcd_command(dev_name(hba->dev), str, tag,
-				doorbell, transfer_len, intr, lba, opcode);
+			doorbell, transfer_len, intr, lba, opcode, group_id);
 }
 
 static void ufshcd_print_clk_freqs(struct ufs_hba *hba)
diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
index 84841b3a7ffd5..50654f3526392 100644
--- a/include/trace/events/ufs.h
+++ b/include/trace/events/ufs.h
@@ -11,6 +11,15 @@
 
 #include <linux/tracepoint.h>
 
+#define str_opcode(opcode)						\
+	__print_symbolic(opcode,					\
+		{ WRITE_16,		"WRITE_16" },			\
+		{ WRITE_10,		"WRITE_10" },			\
+		{ READ_16,		"READ_16" },			\
+		{ READ_10,		"READ_10" },			\
+		{ SYNCHRONIZE_CACHE,	"SYNC" },			\
+		{ UNMAP,		"UNMAP" })
+
 #define UFS_LINK_STATES			\
 	EM(UIC_LINK_OFF_STATE)		\
 	EM(UIC_LINK_ACTIVE_STATE)	\
@@ -215,9 +224,10 @@ DEFINE_EVENT(ufshcd_template, ufshcd_init,
 TRACE_EVENT(ufshcd_command,
 	TP_PROTO(const char *dev_name, const char *str, unsigned int tag,
 			u32 doorbell, int transfer_len, u32 intr, u64 lba,
-			u8 opcode),
+			u8 opcode, u8 group_id),
 
-	TP_ARGS(dev_name, str, tag, doorbell, transfer_len, intr, lba, opcode),
+	TP_ARGS(dev_name, str, tag, doorbell, transfer_len,
+				intr, lba, opcode, group_id),
 
 	TP_STRUCT__entry(
 		__string(dev_name, dev_name)
@@ -228,6 +238,7 @@ TRACE_EVENT(ufshcd_command,
 		__field(u32, intr)
 		__field(u64, lba)
 		__field(u8, opcode)
+		__field(u8, group_id)
 	),
 
 	TP_fast_assign(
@@ -239,13 +250,15 @@ TRACE_EVENT(ufshcd_command,
 		__entry->intr = intr;
 		__entry->lba = lba;
 		__entry->opcode = opcode;
+		__entry->group_id = group_id;
 	),
 
 	TP_printk(
-		"%s: %s: tag: %u, DB: 0x%x, size: %d, IS: %u, LBA: %llu, opcode: 0x%x",
+		"%s: %s: tag: %u, DB: 0x%x, size: %d, IS: %u, LBA: %llu, opcode: 0x%x (%s), group_id: 0x%x",
 		__get_str(str), __get_str(dev_name), __entry->tag,
 		__entry->doorbell, __entry->transfer_len,
-		__entry->intr, __entry->lba, (u32)__entry->opcode
+		__entry->intr, __entry->lba, (u32)__entry->opcode,
+		str_opcode(__entry->opcode), (u32)__entry->group_id
 	)
 );
 
-- 
2.28.0.806.g8561365e88-goog

Re: [PATCH 4/4] scsi: add more contexts in the ufs tracepoints

[Can Guo]

On 2020-10-06 06:36, Jaegeuk Kim wrote:
> From: Jaegeuk Kim <jaegeuk@google.com>
> 
> This adds user-friendly tracepoints with group id.
> 
> Cc: Alim Akhtar <alim.akhtar@samsung.com>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Can Guo <cang@codeaurora.org>
> Signed-off-by: Jaegeuk Kim <jaegeuk@google.com>

Reviewed-by: Can Guo <cang@codeaurora.org>

> ---
>  drivers/scsi/ufs/ufshcd.c  |  6 ++++--
>  include/trace/events/ufs.h | 21 +++++++++++++++++----
>  2 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 76e95963887be..a2db8182663da 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -336,7 +336,7 @@ static void ufshcd_add_command_trace(struct ufs_hba 
> *hba,
>  		unsigned int tag, const char *str)
>  {
>  	sector_t lba = -1;
> -	u8 opcode = 0;
> +	u8 opcode = 0, group_id = 0;
>  	u32 intr, doorbell;
>  	struct ufshcd_lrb *lrbp = &hba->lrb[tag];
>  	struct scsi_cmnd *cmd = lrbp->cmd;
> @@ -362,13 +362,15 @@ static void ufshcd_add_command_trace(struct 
> ufs_hba *hba,
>  				lba = cmd->request->bio->bi_iter.bi_sector;
>  			transfer_len = be32_to_cpu(
>  				lrbp->ucd_req_ptr->sc.exp_data_transfer_len);
> +			if (opcode == WRITE_10)
> +				group_id = lrbp->cmd->cmnd[6];
>  		}
>  	}
> 
>  	intr = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
>  	doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>  	trace_ufshcd_command(dev_name(hba->dev), str, tag,
> -				doorbell, transfer_len, intr, lba, opcode);
> +			doorbell, transfer_len, intr, lba, opcode, group_id);
>  }
> 
>  static void ufshcd_print_clk_freqs(struct ufs_hba *hba)
> diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
> index 84841b3a7ffd5..50654f3526392 100644
> --- a/include/trace/events/ufs.h
> +++ b/include/trace/events/ufs.h
> @@ -11,6 +11,15 @@
> 
>  #include <linux/tracepoint.h>
> 
> +#define str_opcode(opcode)						\
> +	__print_symbolic(opcode,					\
> +		{ WRITE_16,		"WRITE_16" },			\
> +		{ WRITE_10,		"WRITE_10" },			\
> +		{ READ_16,		"READ_16" },			\
> +		{ READ_10,		"READ_10" },			\
> +		{ SYNCHRONIZE_CACHE,	"SYNC" },			\
> +		{ UNMAP,		"UNMAP" })
> +
>  #define UFS_LINK_STATES			\
>  	EM(UIC_LINK_OFF_STATE)		\
>  	EM(UIC_LINK_ACTIVE_STATE)	\
> @@ -215,9 +224,10 @@ DEFINE_EVENT(ufshcd_template, ufshcd_init,
>  TRACE_EVENT(ufshcd_command,
>  	TP_PROTO(const char *dev_name, const char *str, unsigned int tag,
>  			u32 doorbell, int transfer_len, u32 intr, u64 lba,
> -			u8 opcode),
> +			u8 opcode, u8 group_id),
> 
> -	TP_ARGS(dev_name, str, tag, doorbell, transfer_len, intr, lba, 
> opcode),
> +	TP_ARGS(dev_name, str, tag, doorbell, transfer_len,
> +				intr, lba, opcode, group_id),
> 
>  	TP_STRUCT__entry(
>  		__string(dev_name, dev_name)
> @@ -228,6 +238,7 @@ TRACE_EVENT(ufshcd_command,
>  		__field(u32, intr)
>  		__field(u64, lba)
>  		__field(u8, opcode)
> +		__field(u8, group_id)
>  	),
> 
>  	TP_fast_assign(
> @@ -239,13 +250,15 @@ TRACE_EVENT(ufshcd_command,
>  		__entry->intr = intr;
>  		__entry->lba = lba;
>  		__entry->opcode = opcode;
> +		__entry->group_id = group_id;
>  	),
> 
>  	TP_printk(
> -		"%s: %s: tag: %u, DB: 0x%x, size: %d, IS: %u, LBA: %llu, opcode: 
> 0x%x",
> +		"%s: %s: tag: %u, DB: 0x%x, size: %d, IS: %u, LBA: %llu, opcode:
> 0x%x (%s), group_id: 0x%x",
>  		__get_str(str), __get_str(dev_name), __entry->tag,
>  		__entry->doorbell, __entry->transfer_len,
> -		__entry->intr, __entry->lba, (u32)__entry->opcode
> +		__entry->intr, __entry->lba, (u32)__entry->opcode,
> +		str_opcode(__entry->opcode), (u32)__entry->group_id
>  	)
>  );

RE: [PATCH 4/4] scsi: add more contexts in the ufs tracepoints

[Avri Altman]

> 
> On 2020-10-06 06:36, Jaegeuk Kim wrote:
> > From: Jaegeuk Kim <jaegeuk@google.com>
> >
> > This adds user-friendly tracepoints with group id.
You have the entire cdb as part of the upiu trace,
Can't you parse what you need from there?

Thanks,
Avri

Re: [PATCH 4/4] scsi: add more contexts in the ufs tracepoints

[Can Guo]

On 2020-10-20 18:51, Avri Altman wrote:
>> 
>> On 2020-10-06 06:36, Jaegeuk Kim wrote:
>> > From: Jaegeuk Kim <jaegeuk@google.com>
>> >
>> > This adds user-friendly tracepoints with group id.
> You have the entire cdb as part of the upiu trace,
> Can't you parse what you need from there?
> 
> Thanks,
> Avri

Yes, but assume we have a large trace log file, having a
groud id allows us to filter the data by it easily, right?

Thanks,

Can Guo.

RE: [PATCH 4/4] scsi: add more contexts in the ufs tracepoints

[Avri Altman]

> 
> On 2020-10-20 18:51, Avri Altman wrote:
> >>
> >> On 2020-10-06 06:36, Jaegeuk Kim wrote:
> >> > From: Jaegeuk Kim <jaegeuk@google.com>
> >> >
> >> > This adds user-friendly tracepoints with group id.
> > You have the entire cdb as part of the upiu trace,
> > Can't you parse what you need from there?
> >
> > Thanks,
> > Avri
> 
> Yes, but assume we have a large trace log file, having a
> groud id allows us to filter the data by it easily, right?
> 
Ahha, ok.

Thanks,
Avri

Re: [PATCH 4/4] scsi: add more contexts in the ufs tracepoints

[Can Guo]

On 2020-10-20 19:02, Can Guo wrote:
> On 2020-10-20 18:51, Avri Altman wrote:
>>> 
>>> On 2020-10-06 06:36, Jaegeuk Kim wrote:
>>> > From: Jaegeuk Kim <jaegeuk@google.com>
>>> >
>>> > This adds user-friendly tracepoints with group id.
>> You have the entire cdb as part of the upiu trace,
>> Can't you parse what you need from there?
>> 
>> Thanks,
>> Avri
> 
> Yes, but assume we have a large trace log file, having a
> groud id allows us to filter the data by it easily, right?
> 
> Thanks,
> 
> Can Guo.

I just dobule checked WRITE(10)'s CDB, byte 6 has group
ID ONLY. So Avri is right, we don't even need to parse it,
we can easily filter a ftrace log file by byte 6 to get the
WRITE(10) cmds with specific group ID - we don't need this
change.

Thanks,

Can Guo.

Re: [PATCH 4/4] scsi: add more contexts in the ufs tracepoints

[Can Guo]

On 2020-10-20 19:57, Can Guo wrote:
> On 2020-10-20 19:02, Can Guo wrote:
>> On 2020-10-20 18:51, Avri Altman wrote:
>>>> 
>>>> On 2020-10-06 06:36, Jaegeuk Kim wrote:
>>>> > From: Jaegeuk Kim <jaegeuk@google.com>
>>>> >
>>>> > This adds user-friendly tracepoints with group id.
>>> You have the entire cdb as part of the upiu trace,
>>> Can't you parse what you need from there?
>>> 
>>> Thanks,
>>> Avri
>> 
>> Yes, but assume we have a large trace log file, having a
>> groud id allows us to filter the data by it easily, right?
>> 
>> Thanks,
>> 
>> Can Guo.
> 
> I just dobule checked WRITE(10)'s CDB, byte 6 has group
> ID ONLY. So Avri is right, we don't even need to parse it,
> we can easily filter a ftrace log file by byte 6 to get the
> WRITE(10) cmds with specific group ID - we don't need this
> change.
> 
> Thanks,
> 
> Can Guo.

Please ignore my previous mail, I misunderstood the change. :(
You have my reivewed-by tag for this change.

Regards,

Can Guo.

Re: [PATCH 1/4] scsi: ufs: atomic update for clkgating_enable

[Can Guo]

On 2020-10-06 06:36, Jaegeuk Kim wrote:
> From: Jaegeuk Kim <jaegeuk@google.com>
> 
> When giving a stress test which enables/disables clkgating, we hit 
> device
> timeout sometimes. This patch avoids subtle racy condition to address 
> it.
> 
> Cc: Alim Akhtar <alim.akhtar@samsung.com>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Can Guo <cang@codeaurora.org>
> Signed-off-by: Jaegeuk Kim <jaegeuk@google.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 1d157ff58d817..d929c3d1e58cc 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1791,19 +1791,19 @@ static ssize_t
> ufshcd_clkgate_enable_store(struct device *dev,
>  		return -EINVAL;
> 
>  	value = !!value;
> +
> +	spin_lock_irqsave(hba->host->host_lock, flags);
>  	if (value == hba->clk_gating.is_enabled)
>  		goto out;
> 
> -	if (value) {
> -		ufshcd_release(hba);
> -	} else {
> -		spin_lock_irqsave(hba->host->host_lock, flags);
> +	if (value)
> +		hba->clk_gating.active_reqs--;
> +	else
>  		hba->clk_gating.active_reqs++;
> -		spin_unlock_irqrestore(hba->host->host_lock, flags);
> -	}
> 
>  	hba->clk_gating.is_enabled = value;
>  out:
> +	spin_unlock_irqrestore(hba->host->host_lock, flags);
>  	return count;
>  }

I agree that we should protect the flag "is_enabled" with spin lock,
but I prefer the old logic of calling ufshcd_release() instead of
just doing hba->clk_gating.active_reqs--, you can use 
__ufshcd_release(),
which is free of locking.

Thanks,

Can Guo.