propose some UFS fixes

propose some UFS fixes

[Jaegeuk Kim]

Change log from v1:
 - remove clkgating_enable check in __ufshcd_release
 - use __uhfshcd_release instead of active_req.

[PATCH v2 1/5] 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 b8f573a02713..7344353a9167 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1807,19 +1807,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)
+		__ufshcd_release(hba);
+	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.29.0.rc1.297.gfa9743e501-goog

[PATCH v2 2/5] 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 7344353a9167..feb10ebf7a35 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7057,7 +7057,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,
@@ -7070,14 +7069,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);
@@ -7601,6 +7600,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
@@ -7720,6 +7776,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 47eb1430274c..718881d038f5 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -681,6 +681,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.29.0.rc1.297.gfa9743e501-goog

[PATCH v2 3/5] 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 feb10ebf7a35..0858c0b55eac 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1867,7 +1867,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.29.0.rc1.297.gfa9743e501-goog

[PATCH v2 4/5] 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>
Reviewed-by: 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 0858c0b55eac..b5ca0effe636 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -348,7 +348,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;
@@ -374,13 +374,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 84841b3a7ffd..50654f352639 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.29.0.rc1.297.gfa9743e501-goog

[PATCH v2 5/5] scsi: ufs: fix clkgating on/off correctly

[Jaegeuk Kim]

The below call stack prevents clk_gating at every IO completion.
We can remove the condition, ufshcd_any_tag_in_use(), since clkgating_work
will check it again.

ufshcd_complete_requests(struct ufs_hba *hba)
  ufshcd_transfer_req_compl()
    __ufshcd_transfer_req_compl()
      __ufshcd_release(hba)
        if (ufshcd_any_tag_in_use() == 1)
           return;
  ufshcd_tmc_handler(hba);
    blk_mq_tagset_busy_iter();

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@kernel.org>
---
 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 b5ca0effe636..cecbd4ace8b4 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1746,7 +1746,7 @@ static void __ufshcd_release(struct ufs_hba *hba)
 
 	if (hba->clk_gating.active_reqs || hba->clk_gating.is_suspended ||
 	    hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL ||
-	    ufshcd_any_tag_in_use(hba) || hba->outstanding_tasks ||
+	    hba->outstanding_tasks ||
 	    hba->active_uic_cmd || hba->uic_async_done)
 		return;
 
-- 
2.29.0.rc1.297.gfa9743e501-goog

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

[Can Guo]

On 2020-10-21 03:52, 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.
> 

You misunderstood my point. I meant you need to give more info about why
we are adding WQ_HIGHPRI flag but not why WQ_MEM_RECLAIM must be there.

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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index feb10ebf7a35..0858c0b55eac 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1867,7 +1867,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 v2 5/5] scsi: ufs: fix clkgating on/off correctly

[Can Guo]

On 2020-10-21 03:52, Jaegeuk Kim wrote:
> The below call stack prevents clk_gating at every IO completion.
> We can remove the condition, ufshcd_any_tag_in_use(), since 
> clkgating_work
> will check it again.
> 

I think checking ufshcd_any_tag_in_use() in either ufshcd_release() or
gate_work() can break UFS clk gating's functionality.

ufshcd_any_tag_in_use() was introduced to replace hba->lrb_in_use. 
However,
they are not exactly same - ufshcd_any_tag_in_use() returns true if any 
tag
assigned from block layer is still in use, but tags are released 
asynchronously
(through block softirq), meaning it does not reflect the real occupation 
of UFS host.
That is after UFS host finishes all tasks, ufshcd_any_tag_in_use() can 
still return true.

This change only removes the check of ufshcd_any_tag_in_use() in 
ufshcd_release(),
but having the check of it in gate_work() can still prevent gating from 
happening.
The current change works for you maybe because the tags are release 
before
hba->clk_gating.delay_ms expires, but if hba->clk_gating.delay_ms is 
shorter or
somehow block softirq is retarded, gate_work() may have chance to see 
ufshcd_any_tag_in_use()
returns true. What do you think?

Thanks,

Can Guo.

In __ufshcd_transfer_req_compl
Ihba->lrb_in_use is cleared immediately when UFS driver
finishes all tasks

> ufshcd_complete_requests(struct ufs_hba *hba)
>   ufshcd_transfer_req_compl()
>     __ufshcd_transfer_req_compl()
>       __ufshcd_release(hba)
>         if (ufshcd_any_tag_in_use() == 1)
>            return;
>   ufshcd_tmc_handler(hba);
>     blk_mq_tagset_busy_iter();
> 
> 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@kernel.org>
> ---
>  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 b5ca0effe636..cecbd4ace8b4 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1746,7 +1746,7 @@ static void __ufshcd_release(struct ufs_hba *hba)
> 
>  	if (hba->clk_gating.active_reqs || hba->clk_gating.is_suspended ||
>  	    hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL ||
> -	    ufshcd_any_tag_in_use(hba) || hba->outstanding_tasks ||
> +	    hba->outstanding_tasks ||
>  	    hba->active_uic_cmd || hba->uic_async_done)
>  		return;

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

[Can Guo]

On 2020-10-21 03:52, 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>

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

Next time can you have a cover letter in case of multiple patches?

Thanks,

Can Guo.

> ---
>  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 b8f573a02713..7344353a9167 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1807,19 +1807,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)
> +		__ufshcd_release(hba);
> +	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;
>  }

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

[jaegeuk]

On 10/21, Can Guo wrote:
> On 2020-10-21 03:52, 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>
> 
> Reviewed-by: Can Guo <cang@codeaurora.org>
> 
> Next time can you have a cover letter in case of multiple patches?

Ah, it seems I had to cc you here as well.

https://lore.kernel.org/linux-scsi/20201020195258.2005605-1-jaegeuk@kernel.org/T/#mb4e43f3bd03a6b7bc136bea21ac805041c1417a2

> 
> Thanks,
> 
> Can Guo.
> 
> > ---
> >  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 b8f573a02713..7344353a9167 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -1807,19 +1807,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)
> > +		__ufshcd_release(hba);
> > +	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;
> >  }

Re: [PATCH v2 5/5] scsi: ufs: fix clkgating on/off correctly

[jaegeuk]

On 10/21, Can Guo wrote:
> On 2020-10-21 03:52, Jaegeuk Kim wrote:
> > The below call stack prevents clk_gating at every IO completion.
> > We can remove the condition, ufshcd_any_tag_in_use(), since
> > clkgating_work
> > will check it again.
> > 
> 
> I think checking ufshcd_any_tag_in_use() in either ufshcd_release() or
> gate_work() can break UFS clk gating's functionality.
> 
> ufshcd_any_tag_in_use() was introduced to replace hba->lrb_in_use. However,
> they are not exactly same - ufshcd_any_tag_in_use() returns true if any tag
> assigned from block layer is still in use, but tags are released
> asynchronously
> (through block softirq), meaning it does not reflect the real occupation of
> UFS host.
> That is after UFS host finishes all tasks, ufshcd_any_tag_in_use() can still
> return true.
> 
> This change only removes the check of ufshcd_any_tag_in_use() in
> ufshcd_release(),
> but having the check of it in gate_work() can still prevent gating from
> happening.
> The current change works for you maybe because the tags are release before
> hba->clk_gating.delay_ms expires, but if hba->clk_gating.delay_ms is shorter
> or
> somehow block softirq is retarded, gate_work() may have chance to see
> ufshcd_any_tag_in_use()
> returns true. What do you think?

I don't think this breaks clkgating, but fix the wrong condition check which
prevented gate_work at all. As you mentioned, even if this schedules gate_work
by racy conditions, gate_work will handle it as a last resort.

> 
> Thanks,
> 
> Can Guo.
> 
> In __ufshcd_transfer_req_compl
> Ihba->lrb_in_use is cleared immediately when UFS driver
> finishes all tasks
> 
> > ufshcd_complete_requests(struct ufs_hba *hba)
> >   ufshcd_transfer_req_compl()
> >     __ufshcd_transfer_req_compl()
> >       __ufshcd_release(hba)
> >         if (ufshcd_any_tag_in_use() == 1)
> >            return;
> >   ufshcd_tmc_handler(hba);
> >     blk_mq_tagset_busy_iter();
> > 
> > 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@kernel.org>
> > ---
> >  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 b5ca0effe636..cecbd4ace8b4 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -1746,7 +1746,7 @@ static void __ufshcd_release(struct ufs_hba *hba)
> > 
> >  	if (hba->clk_gating.active_reqs || hba->clk_gating.is_suspended ||
> >  	    hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL ||
> > -	    ufshcd_any_tag_in_use(hba) || hba->outstanding_tasks ||
> > +	    hba->outstanding_tasks ||
> >  	    hba->active_uic_cmd || hba->uic_async_done)
> >  		return;

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

[jaegeuk]

On 10/21, Can Guo wrote:
> On 2020-10-21 03:52, 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.
> > 
> 
> You misunderstood my point. I meant you need to give more info about why
> we are adding WQ_HIGHPRI flag but not why WQ_MEM_RECLAIM must be there.

Oh, I thought that WQ_HIGHPRI is telling everything tho.

> 
> 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 | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index feb10ebf7a35..0858c0b55eac 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -1867,7 +1867,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 v2 5/5] scsi: ufs: fix clkgating on/off correctly

[Can Guo]

On 2020-10-21 12:52, jaegeuk@kernel.org wrote:
> On 10/21, Can Guo wrote:
>> On 2020-10-21 03:52, Jaegeuk Kim wrote:
>> > The below call stack prevents clk_gating at every IO completion.
>> > We can remove the condition, ufshcd_any_tag_in_use(), since
>> > clkgating_work
>> > will check it again.
>> >
>> 
>> I think checking ufshcd_any_tag_in_use() in either ufshcd_release() or
>> gate_work() can break UFS clk gating's functionality.
>> 
>> ufshcd_any_tag_in_use() was introduced to replace hba->lrb_in_use. 
>> However,
>> they are not exactly same - ufshcd_any_tag_in_use() returns true if 
>> any tag
>> assigned from block layer is still in use, but tags are released
>> asynchronously
>> (through block softirq), meaning it does not reflect the real 
>> occupation of
>> UFS host.
>> That is after UFS host finishes all tasks, ufshcd_any_tag_in_use() can 
>> still
>> return true.
>> 
>> This change only removes the check of ufshcd_any_tag_in_use() in
>> ufshcd_release(),
>> but having the check of it in gate_work() can still prevent gating 
>> from
>> happening.
>> The current change works for you maybe because the tags are release 
>> before
>> hba->clk_gating.delay_ms expires, but if hba->clk_gating.delay_ms is 
>> shorter
>> or
>> somehow block softirq is retarded, gate_work() may have chance to see
>> ufshcd_any_tag_in_use()
>> returns true. What do you think?
> 
> I don't think this breaks clkgating, but fix the wrong condition check 
> which
> prevented gate_work at all. As you mentioned, even if this schedules 
> gate_work
> by racy conditions, gate_work will handle it as a last resort.
> 

If clocks cannot be gated after the last task is cleared from UFS host, 
then clk gating
is broken, no? Assume UFS has completed the last task in its queue, as 
this change says,
ufshcd_any_tag_in_use() is preventing ufshcd_release() from invoking 
gate_work().
Similarly, ufshcd_any_tag_in_use() can prevent gate_work() from doing 
its real work -
disabling the clocks. Do you agree?

         if (hba->clk_gating.active_reqs
                 || hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL
                 || ufshcd_any_tag_in_use(hba) || hba->outstanding_tasks
                 || hba->active_uic_cmd || hba->uic_async_done)
                 goto rel_lock;

Thanks,

Can Guo.

>> 
>> Thanks,
>> 
>> Can Guo.
>> 
>> In __ufshcd_transfer_req_compl
>> Ihba->lrb_in_use is cleared immediately when UFS driver
>> finishes all tasks
>> 
>> > ufshcd_complete_requests(struct ufs_hba *hba)
>> >   ufshcd_transfer_req_compl()
>> >     __ufshcd_transfer_req_compl()
>> >       __ufshcd_release(hba)
>> >         if (ufshcd_any_tag_in_use() == 1)
>> >            return;
>> >   ufshcd_tmc_handler(hba);
>> >     blk_mq_tagset_busy_iter();
>> >
>> > 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@kernel.org>
>> > ---
>> >  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 b5ca0effe636..cecbd4ace8b4 100644
>> > --- a/drivers/scsi/ufs/ufshcd.c
>> > +++ b/drivers/scsi/ufs/ufshcd.c
>> > @@ -1746,7 +1746,7 @@ static void __ufshcd_release(struct ufs_hba *hba)
>> >
>> >  	if (hba->clk_gating.active_reqs || hba->clk_gating.is_suspended ||
>> >  	    hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL ||
>> > -	    ufshcd_any_tag_in_use(hba) || hba->outstanding_tasks ||
>> > +	    hba->outstanding_tasks ||
>> >  	    hba->active_uic_cmd || hba->uic_async_done)
>> >  		return;

Re: [PATCH v2 5/5] scsi: ufs: fix clkgating on/off correctly

[Jaegeuk Kim]

On 10/21, Can Guo wrote:
> On 2020-10-21 12:52, jaegeuk@kernel.org wrote:
> > On 10/21, Can Guo wrote:
> > > On 2020-10-21 03:52, Jaegeuk Kim wrote:
> > > > The below call stack prevents clk_gating at every IO completion.
> > > > We can remove the condition, ufshcd_any_tag_in_use(), since
> > > > clkgating_work
> > > > will check it again.
> > > >
> > > 
> > > I think checking ufshcd_any_tag_in_use() in either ufshcd_release() or
> > > gate_work() can break UFS clk gating's functionality.
> > > 
> > > ufshcd_any_tag_in_use() was introduced to replace hba->lrb_in_use.
> > > However,
> > > they are not exactly same - ufshcd_any_tag_in_use() returns true if
> > > any tag
> > > assigned from block layer is still in use, but tags are released
> > > asynchronously
> > > (through block softirq), meaning it does not reflect the real
> > > occupation of
> > > UFS host.
> > > That is after UFS host finishes all tasks, ufshcd_any_tag_in_use()
> > > can still
> > > return true.
> > > 
> > > This change only removes the check of ufshcd_any_tag_in_use() in
> > > ufshcd_release(),
> > > but having the check of it in gate_work() can still prevent gating
> > > from
> > > happening.
> > > The current change works for you maybe because the tags are release
> > > before
> > > hba->clk_gating.delay_ms expires, but if hba->clk_gating.delay_ms is
> > > shorter
> > > or
> > > somehow block softirq is retarded, gate_work() may have chance to see
> > > ufshcd_any_tag_in_use()
> > > returns true. What do you think?
> > 
> > I don't think this breaks clkgating, but fix the wrong condition check
> > which
> > prevented gate_work at all. As you mentioned, even if this schedules
> > gate_work
> > by racy conditions, gate_work will handle it as a last resort.
> > 
> 
> If clocks cannot be gated after the last task is cleared from UFS host, then
> clk gating
> is broken, no? Assume UFS has completed the last task in its queue, as this
> change says,
> ufshcd_any_tag_in_use() is preventing ufshcd_release() from invoking
> gate_work().
> Similarly, ufshcd_any_tag_in_use() can prevent gate_work() from doing its
> real work -
> disabling the clocks. Do you agree?
> 
>         if (hba->clk_gating.active_reqs
>                 || hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL
>                 || ufshcd_any_tag_in_use(hba) || hba->outstanding_tasks
>                 || hba->active_uic_cmd || hba->uic_async_done)
>                 goto rel_lock;

I see the point, but this happens only when clkgate_delay_ms is too short
to give enough time for releasing tag. If it's correctly set, I think there'd
be no problem, unless softirq was delayed by other RT threads which is just
a corner case tho.

> 
> Thanks,
> 
> Can Guo.
> 
> > > 
> > > Thanks,
> > > 
> > > Can Guo.
> > > 
> > > In __ufshcd_transfer_req_compl
> > > Ihba->lrb_in_use is cleared immediately when UFS driver
> > > finishes all tasks
> > > 
> > > > ufshcd_complete_requests(struct ufs_hba *hba)
> > > >   ufshcd_transfer_req_compl()
> > > >     __ufshcd_transfer_req_compl()
> > > >       __ufshcd_release(hba)
> > > >         if (ufshcd_any_tag_in_use() == 1)
> > > >            return;
> > > >   ufshcd_tmc_handler(hba);
> > > >     blk_mq_tagset_busy_iter();
> > > >
> > > > 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@kernel.org>
> > > > ---
> > > >  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 b5ca0effe636..cecbd4ace8b4 100644
> > > > --- a/drivers/scsi/ufs/ufshcd.c
> > > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > > @@ -1746,7 +1746,7 @@ static void __ufshcd_release(struct ufs_hba *hba)
> > > >
> > > >  	if (hba->clk_gating.active_reqs || hba->clk_gating.is_suspended ||
> > > >  	    hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL ||
> > > > -	    ufshcd_any_tag_in_use(hba) || hba->outstanding_tasks ||
> > > > +	    hba->outstanding_tasks ||
> > > >  	    hba->active_uic_cmd || hba->uic_async_done)
> > > >  		return;

Re: [PATCH v2 5/5] scsi: ufs: fix clkgating on/off correctly

[Can Guo]

On 2020-10-23 08:53, Jaegeuk Kim wrote:
> On 10/21, Can Guo wrote:
>> On 2020-10-21 12:52, jaegeuk@kernel.org wrote:
>> > On 10/21, Can Guo wrote:
>> > > On 2020-10-21 03:52, Jaegeuk Kim wrote:
>> > > > The below call stack prevents clk_gating at every IO completion.
>> > > > We can remove the condition, ufshcd_any_tag_in_use(), since
>> > > > clkgating_work
>> > > > will check it again.
>> > > >
>> > >
>> > > I think checking ufshcd_any_tag_in_use() in either ufshcd_release() or
>> > > gate_work() can break UFS clk gating's functionality.
>> > >
>> > > ufshcd_any_tag_in_use() was introduced to replace hba->lrb_in_use.
>> > > However,
>> > > they are not exactly same - ufshcd_any_tag_in_use() returns true if
>> > > any tag
>> > > assigned from block layer is still in use, but tags are released
>> > > asynchronously
>> > > (through block softirq), meaning it does not reflect the real
>> > > occupation of
>> > > UFS host.
>> > > That is after UFS host finishes all tasks, ufshcd_any_tag_in_use()
>> > > can still
>> > > return true.
>> > >
>> > > This change only removes the check of ufshcd_any_tag_in_use() in
>> > > ufshcd_release(),
>> > > but having the check of it in gate_work() can still prevent gating
>> > > from
>> > > happening.
>> > > The current change works for you maybe because the tags are release
>> > > before
>> > > hba->clk_gating.delay_ms expires, but if hba->clk_gating.delay_ms is
>> > > shorter
>> > > or
>> > > somehow block softirq is retarded, gate_work() may have chance to see
>> > > ufshcd_any_tag_in_use()
>> > > returns true. What do you think?
>> >
>> > I don't think this breaks clkgating, but fix the wrong condition check
>> > which
>> > prevented gate_work at all. As you mentioned, even if this schedules
>> > gate_work
>> > by racy conditions, gate_work will handle it as a last resort.
>> >
>> 
>> If clocks cannot be gated after the last task is cleared from UFS 
>> host, then
>> clk gating
>> is broken, no? Assume UFS has completed the last task in its queue, as 
>> this
>> change says,
>> ufshcd_any_tag_in_use() is preventing ufshcd_release() from invoking
>> gate_work().
>> Similarly, ufshcd_any_tag_in_use() can prevent gate_work() from doing 
>> its
>> real work -
>> disabling the clocks. Do you agree?
>> 
>>         if (hba->clk_gating.active_reqs
>>                 || hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL
>>                 || ufshcd_any_tag_in_use(hba) || 
>> hba->outstanding_tasks
>>                 || hba->active_uic_cmd || hba->uic_async_done)
>>                 goto rel_lock;
> 
> I see the point, but this happens only when clkgate_delay_ms is too 
> short
> to give enough time for releasing tag. If it's correctly set, I think 
> there'd
> be no problem, unless softirq was delayed by other RT threads which is 
> just
> a corner case tho.
> 

Yes, we are fixing corner cases, aren't we? I thought you would like to
address it since you are fixing clk gating.

Regards,

Can Guo.

>> 
>> Thanks,
>> 
>> Can Guo.
>> 
>> > >
>> > > Thanks,
>> > >
>> > > Can Guo.
>> > >
>> > > In __ufshcd_transfer_req_compl
>> > > Ihba->lrb_in_use is cleared immediately when UFS driver
>> > > finishes all tasks
>> > >
>> > > > ufshcd_complete_requests(struct ufs_hba *hba)
>> > > >   ufshcd_transfer_req_compl()
>> > > >     __ufshcd_transfer_req_compl()
>> > > >       __ufshcd_release(hba)
>> > > >         if (ufshcd_any_tag_in_use() == 1)
>> > > >            return;
>> > > >   ufshcd_tmc_handler(hba);
>> > > >     blk_mq_tagset_busy_iter();
>> > > >
>> > > > 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@kernel.org>
>> > > > ---
>> > > >  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 b5ca0effe636..cecbd4ace8b4 100644
>> > > > --- a/drivers/scsi/ufs/ufshcd.c
>> > > > +++ b/drivers/scsi/ufs/ufshcd.c
>> > > > @@ -1746,7 +1746,7 @@ static void __ufshcd_release(struct ufs_hba *hba)
>> > > >
>> > > >  	if (hba->clk_gating.active_reqs || hba->clk_gating.is_suspended ||
>> > > >  	    hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL ||
>> > > > -	    ufshcd_any_tag_in_use(hba) || hba->outstanding_tasks ||
>> > > > +	    hba->outstanding_tasks ||
>> > > >  	    hba->active_uic_cmd || hba->uic_async_done)
>> > > >  		return;

Re: [PATCH v2 5/5] scsi: ufs: fix clkgating on/off correctly

[Jaegeuk Kim]

On 10/26, Can Guo wrote:
> On 2020-10-23 08:53, Jaegeuk Kim wrote:
> > On 10/21, Can Guo wrote:
> > > On 2020-10-21 12:52, jaegeuk@kernel.org wrote:
> > > > On 10/21, Can Guo wrote:
> > > > > On 2020-10-21 03:52, Jaegeuk Kim wrote:
> > > > > > The below call stack prevents clk_gating at every IO completion.
> > > > > > We can remove the condition, ufshcd_any_tag_in_use(), since
> > > > > > clkgating_work
> > > > > > will check it again.
> > > > > >
> > > > >
> > > > > I think checking ufshcd_any_tag_in_use() in either ufshcd_release() or
> > > > > gate_work() can break UFS clk gating's functionality.
> > > > >
> > > > > ufshcd_any_tag_in_use() was introduced to replace hba->lrb_in_use.
> > > > > However,
> > > > > they are not exactly same - ufshcd_any_tag_in_use() returns true if
> > > > > any tag
> > > > > assigned from block layer is still in use, but tags are released
> > > > > asynchronously
> > > > > (through block softirq), meaning it does not reflect the real
> > > > > occupation of
> > > > > UFS host.
> > > > > That is after UFS host finishes all tasks, ufshcd_any_tag_in_use()
> > > > > can still
> > > > > return true.
> > > > >
> > > > > This change only removes the check of ufshcd_any_tag_in_use() in
> > > > > ufshcd_release(),
> > > > > but having the check of it in gate_work() can still prevent gating
> > > > > from
> > > > > happening.
> > > > > The current change works for you maybe because the tags are release
> > > > > before
> > > > > hba->clk_gating.delay_ms expires, but if hba->clk_gating.delay_ms is
> > > > > shorter
> > > > > or
> > > > > somehow block softirq is retarded, gate_work() may have chance to see
> > > > > ufshcd_any_tag_in_use()
> > > > > returns true. What do you think?
> > > >
> > > > I don't think this breaks clkgating, but fix the wrong condition check
> > > > which
> > > > prevented gate_work at all. As you mentioned, even if this schedules
> > > > gate_work
> > > > by racy conditions, gate_work will handle it as a last resort.
> > > >
> > > 
> > > If clocks cannot be gated after the last task is cleared from UFS
> > > host, then
> > > clk gating
> > > is broken, no? Assume UFS has completed the last task in its queue,
> > > as this
> > > change says,
> > > ufshcd_any_tag_in_use() is preventing ufshcd_release() from invoking
> > > gate_work().
> > > Similarly, ufshcd_any_tag_in_use() can prevent gate_work() from
> > > doing its
> > > real work -
> > > disabling the clocks. Do you agree?
> > > 
> > >         if (hba->clk_gating.active_reqs
> > >                 || hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL
> > >                 || ufshcd_any_tag_in_use(hba) ||
> > > hba->outstanding_tasks
> > >                 || hba->active_uic_cmd || hba->uic_async_done)
> > >                 goto rel_lock;
> > 
> > I see the point, but this happens only when clkgate_delay_ms is too
> > short
> > to give enough time for releasing tag. If it's correctly set, I think
> > there'd
> > be no problem, unless softirq was delayed by other RT threads which is
> > just
> > a corner case tho.
> > 
> 
> Yes, we are fixing corner cases, aren't we? I thought you would like to
> address it since you are fixing clk gating.

I think that can be fixed by a separate patch which controls delay_ms when
user tries to change it from default 150 ms?

> 
> Regards,
> 
> Can Guo.
> 
> > > 
> > > Thanks,
> > > 
> > > Can Guo.
> > > 
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Can Guo.
> > > > >
> > > > > In __ufshcd_transfer_req_compl
> > > > > Ihba->lrb_in_use is cleared immediately when UFS driver
> > > > > finishes all tasks
> > > > >
> > > > > > ufshcd_complete_requests(struct ufs_hba *hba)
> > > > > >   ufshcd_transfer_req_compl()
> > > > > >     __ufshcd_transfer_req_compl()
> > > > > >       __ufshcd_release(hba)
> > > > > >         if (ufshcd_any_tag_in_use() == 1)
> > > > > >            return;
> > > > > >   ufshcd_tmc_handler(hba);
> > > > > >     blk_mq_tagset_busy_iter();
> > > > > >
> > > > > > 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@kernel.org>
> > > > > > ---
> > > > > >  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 b5ca0effe636..cecbd4ace8b4 100644
> > > > > > --- a/drivers/scsi/ufs/ufshcd.c
> > > > > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > > > > @@ -1746,7 +1746,7 @@ static void __ufshcd_release(struct ufs_hba *hba)
> > > > > >
> > > > > >  	if (hba->clk_gating.active_reqs || hba->clk_gating.is_suspended ||
> > > > > >  	    hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL ||
> > > > > > -	    ufshcd_any_tag_in_use(hba) || hba->outstanding_tasks ||
> > > > > > +	    hba->outstanding_tasks ||
> > > > > >  	    hba->active_uic_cmd || hba->uic_async_done)
> > > > > >  		return;

Re: [PATCH v2 5/5] scsi: ufs: fix clkgating on/off correctly

[asutoshd]

On 2020-10-22 17:53, Jaegeuk Kim wrote:
> On 10/21, Can Guo wrote:
>> On 2020-10-21 12:52, jaegeuk@kernel.org wrote:
>> > On 10/21, Can Guo wrote:
>> > > On 2020-10-21 03:52, Jaegeuk Kim wrote:
>> > > > The below call stack prevents clk_gating at every IO completion.
>> > > > We can remove the condition, ufshcd_any_tag_in_use(), since
>> > > > clkgating_work
>> > > > will check it again.
>> > > >
>> > >
>> > > I think checking ufshcd_any_tag_in_use() in either ufshcd_release() or
>> > > gate_work() can break UFS clk gating's functionality.
>> > >
>> > > ufshcd_any_tag_in_use() was introduced to replace hba->lrb_in_use.
>> > > However,
>> > > they are not exactly same - ufshcd_any_tag_in_use() returns true if
>> > > any tag
>> > > assigned from block layer is still in use, but tags are released
>> > > asynchronously
>> > > (through block softirq), meaning it does not reflect the real
>> > > occupation of
>> > > UFS host.
>> > > That is after UFS host finishes all tasks, ufshcd_any_tag_in_use()
>> > > can still
>> > > return true.
>> > >
>> > > This change only removes the check of ufshcd_any_tag_in_use() in
>> > > ufshcd_release(),
>> > > but having the check of it in gate_work() can still prevent gating
>> > > from
>> > > happening.
>> > > The current change works for you maybe because the tags are release
>> > > before
>> > > hba->clk_gating.delay_ms expires, but if hba->clk_gating.delay_ms is
>> > > shorter
>> > > or
>> > > somehow block softirq is retarded, gate_work() may have chance to see
>> > > ufshcd_any_tag_in_use()
>> > > returns true. What do you think?
>> >
>> > I don't think this breaks clkgating, but fix the wrong condition check
>> > which
>> > prevented gate_work at all. As you mentioned, even if this schedules
>> > gate_work
>> > by racy conditions, gate_work will handle it as a last resort.
>> >
>> 
>> If clocks cannot be gated after the last task is cleared from UFS 
>> host, then
>> clk gating
>> is broken, no? Assume UFS has completed the last task in its queue, as 
>> this
>> change says,
>> ufshcd_any_tag_in_use() is preventing ufshcd_release() from invoking
>> gate_work().
>> Similarly, ufshcd_any_tag_in_use() can prevent gate_work() from doing 
>> its
>> real work -
>> disabling the clocks. Do you agree?
>> 
>>         if (hba->clk_gating.active_reqs
>>                 || hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL
>>                 || ufshcd_any_tag_in_use(hba) || 
>> hba->outstanding_tasks
>>                 || hba->active_uic_cmd || hba->uic_async_done)
>>                 goto rel_lock;
> 
> I see the point, but this happens only when clkgate_delay_ms is too 
> short
> to give enough time for releasing tag. If it's correctly set, I think 
> there'd
> be no problem, unless softirq was delayed by other RT threads which is 
> just
> a corner case tho.
> 
>> 
>> Thanks,
>> 
>> Can Guo.
>> 
>> > >
>> > > Thanks,
>> > >
>> > > Can Guo.
>> > >
>> > > In __ufshcd_transfer_req_compl
>> > > Ihba->lrb_in_use is cleared immediately when UFS driver
>> > > finishes all tasks
>> > >
>> > > > ufshcd_complete_requests(struct ufs_hba *hba)
>> > > >   ufshcd_transfer_req_compl()
>> > > >     __ufshcd_transfer_req_compl()
>> > > >       __ufshcd_release(hba)
>> > > >         if (ufshcd_any_tag_in_use() == 1)
>> > > >            return;
>> > > >   ufshcd_tmc_handler(hba);
>> > > >     blk_mq_tagset_busy_iter();
>> > > >
>> > > > 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@kernel.org>
>> > > > ---
>> > > >  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 b5ca0effe636..cecbd4ace8b4 100644
>> > > > --- a/drivers/scsi/ufs/ufshcd.c
>> > > > +++ b/drivers/scsi/ufs/ufshcd.c
>> > > > @@ -1746,7 +1746,7 @@ static void __ufshcd_release(struct ufs_hba *hba)
>> > > >
>> > > >  	if (hba->clk_gating.active_reqs || hba->clk_gating.is_suspended ||
>> > > >  	    hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL ||
>> > > > -	    ufshcd_any_tag_in_use(hba) || hba->outstanding_tasks ||
>> > > > +	    hba->outstanding_tasks ||
>> > > >  	    hba->active_uic_cmd || hba->uic_async_done)
>> > > >  		return;

How about checking outstanding_reqs as well, say in 
ufshcd_any_tag_in_use() ?

-asd