[PATCH v2 0/3] change UIC command handling

[PATCH v2 0/3] change UIC command handling

[Kiwoong Kim]

v1 -> v2: remove an unused variable in __ufshcd_send_uic_cmd

There are two issues on UIC command handling related to auto hibern8.
https://lore.kernel.org/linux-scsi/1684208012-114324-1-git-send-email-kwmad.kim@samsung.com/
https://lore.kernel.org/linux-scsi/1684209152-115304-1-git-send-email-kwmad.kim@samsung.com/

Now I combine the two things into this thread.
And one thing added to make part of submitting a UIC command apart
from the critical section w/ host_lock.

Kiwoong Kim (3):
  ufs: make __ufshcd_send_uic_cmd not wrapped by host_lock
  ufs: poll HCS.UCRDY before issuing a UIC command
  ufs: poll pmc until another pa request is completed

 drivers/ufs/core/ufshcd.c | 110 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 77 insertions(+), 33 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] ufs: make __ufshcd_send_uic_cmd not wrapped by host_lock

[Kiwoong Kim]

__ufshcd_send_uic_cmd is wrapped uic_cmd_mutex and
its related contexts are accessed within the period wrappted
by uic_cmd_mutex. Thus, wrapping with host_lock is
redundant.

Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
---
 drivers/ufs/core/ufshcd.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 9434328..a89d39a 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2457,7 +2457,6 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd,
 		      bool completion)
 {
 	lockdep_assert_held(&hba->uic_cmd_mutex);
-	lockdep_assert_held(hba->host->host_lock);
 
 	if (!ufshcd_ready_for_uic_cmd(hba)) {
 		dev_err(hba->dev,
@@ -2484,7 +2483,6 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd,
 int ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
 {
 	int ret;
-	unsigned long flags;
 
 	if (hba->quirks & UFSHCD_QUIRK_BROKEN_UIC_CMD)
 		return 0;
@@ -2493,9 +2491,7 @@ int ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
 	mutex_lock(&hba->uic_cmd_mutex);
 	ufshcd_add_delay_before_dme_cmd(hba);
 
-	spin_lock_irqsave(hba->host->host_lock, flags);
 	ret = __ufshcd_send_uic_cmd(hba, uic_cmd, true);
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
 	if (!ret)
 		ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd);
 
@@ -4180,8 +4176,8 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
 		wmb();
 		reenable_intr = true;
 	}
-	ret = __ufshcd_send_uic_cmd(hba, cmd, false);
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
+	ret = __ufshcd_send_uic_cmd(hba, cmd, false);
 	if (ret) {
 		dev_err(hba->dev,
 			"pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",
-- 
2.7.4

[PATCH v2 2/3] ufs: poll HCS.UCRDY before issuing a UIC command

[Kiwoong Kim]

v1 -> v2: replace usleep_range with udelay
because it's a sleepable period.

With auto hibern8 enabled, UIC could be working
for a while to process a hibern8 operation and HCI
reports UIC not ready for a short term through HCS.UCRDY.
And UFS driver can't recognize the operation.
UFSHCI spec specifies UCRDY like this:
whether the host controller is ready to process UIC COMMAND

The 'ready' could be seen as many different meanings. If the meaning
includes not processing any request from HCI, processing a hibern8
operation can be 'not ready'. In this situation, the driver needs to
wait until the operations is completed.

Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
---
 drivers/ufs/core/ufshcd.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index a89d39a..1f58a20 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2365,7 +2365,18 @@ static inline int ufshcd_hba_capabilities(struct ufs_hba *hba)
  */
 static inline bool ufshcd_ready_for_uic_cmd(struct ufs_hba *hba)
 {
-	return ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY;
+	ktime_t timeout = ktime_add_ms(ktime_get(), UIC_CMD_TIMEOUT);
+	u32 val = 0;
+
+	do {
+		val = ufshcd_readl(hba, REG_CONTROLLER_STATUS) &
+			UIC_COMMAND_READY;
+		if (val)
+			break;
+		udelay(500);
+	} while (ktime_before(ktime_get(), timeout));
+
+	return val ? true : false;
 }
 
 /**
-- 
2.7.4

[PATCH v2 3/3] ufs: poll pmc until another pa request is completed

[Kiwoong Kim]

v2 -> v3
1) check time in the loop with jiffies, instead of miliseconds.
2) change a variable's name and fix a typo and wrong alignment.

v1 -> v2
1) remove clearing hba->active_uic_cmd at the end of __ufshcd_poll_uic_pwr
2) change commit message on the cited clause: 5.7.12.11 -> 5.7.12.1.1
3) add mdelay to avoid too many issueing
4) change the timeout for the polling to 100ms because I found it
sometimes takes much longer than expected.

Regarding 5.7.12.1.1 in Unipro v1.8, PA rejects sebsequent
requests following the first request from upper layer or remote.
In this situation, PA responds w/ BUSY in cases
when they come from upper layer and does nothing for
the requests. So HCI doesn't receive ind, a.k.a. indicator
for its requests and an interrupt, IS.UPMS isn't generated.

When LINERESET occurs, the error handler issues PMC which is
recognized as a request for PA. If a host's PA gets or raises
LINERESET, and a request for PMC, this could be a concurrent
situation mentioned above. And I found that situation w/ my
environment.

[  222.929539]I[0:DefaultDispatch:20410] exynos-ufs 13500000.ufs: ufshcd_update_uic_error: uecdl : 0x80000002
[  222.999009]I[0: arch_disk_io_1:20413] exynos-ufs 13500000.ufs: ufshcd_update_uic_error: uecpa : 0x80000010
[  222.999200] [6:  kworker/u16:2:  132] exynos-ufs 13500000.ufs: ufs_pwr_mode_restore_needed : mode = 0x15, pwr_rx = 1, pwr_tx = 1
[  223.002876]I[0: arch_disk_io_3:20422] exynos-ufs 13500000.ufs: ufshcd_update_uic_error: uecpa : 0x80000010
[  223.501050] [4:  kworker/u16:2:  132] exynos-ufs 13500000.ufs: pwr ctrl cmd 0x2 with mode 0x11 completion timeout
[  223.502305] [4:  kworker/u16:2:  132] exynos-ufs 13500000.ufs: ufshcd_change_power_mode: power mode change failed -110
[  223.502312] [4:  kworker/u16:2:  132] exynos-ufs 13500000.ufs: ufshcd_err_handler: Failed to restore power mode, err = -110
[  223.502392] [4:  kworker/u16:2:  132] exynos-ufs 13500000.ufs: ufshcd_is_pwr_mode_restore_needed : mode = 0x11, pwr_rx = 1, pwr_tx = 1

This patch is to poll PMC's result w/o checking its ind until
the result is not busy, i.e. 09h, to avoid the rejection.

Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
---
 drivers/ufs/core/ufshcd.c | 93 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 65 insertions(+), 28 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 1f58a20..9480233 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -98,6 +98,9 @@
 /* Polling time to wait for fDeviceInit */
 #define FDEVICEINIT_COMPL_TIMEOUT 1500 /* millisecs */
 
+/* Polling time to wait until PA is ready */
+#define UIC_PA_RDY_TIMEOUT	100	/* millisecs */
+
 /* UFSHC 4.0 compliant HC support this mode, refer param_set_mcq_mode() */
 static bool use_mcq_mode = true;
 
@@ -4145,6 +4148,62 @@ int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
 }
 EXPORT_SYMBOL_GPL(ufshcd_dme_get_attr);
 
+static int __ufshcd_poll_uic_pwr(struct ufs_hba *hba, struct uic_command *cmd,
+		struct completion *cnf)
+{
+	unsigned long flags;
+	int ret;
+	u32 mode = cmd->argument3;
+	unsigned long deadline = jiffies +
+		msecs_to_jiffies(UIC_PA_RDY_TIMEOUT);
+
+	do {
+		spin_lock_irqsave(hba->host->host_lock, flags);
+		hba->active_uic_cmd = NULL;
+		if (ufshcd_is_link_broken(hba)) {
+			spin_unlock_irqrestore(hba->host->host_lock, flags);
+			ret = -ENOLINK;
+			goto out;
+		}
+		hba->uic_async_done = cnf;
+		spin_unlock_irqrestore(hba->host->host_lock, flags);
+
+		cmd->argument2 = 0;
+		cmd->argument3 = mode;
+		ret = __ufshcd_send_uic_cmd(hba, cmd, true);
+		if (ret) {
+			dev_err(hba->dev,
+				"pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",
+				cmd->command, cmd->argument3, ret);
+			goto out;
+		}
+
+		/* This value is heuristic */
+		if (!wait_for_completion_timeout(&cmd->done,
+						 msecs_to_jiffies(5))) {
+			ret = -ETIMEDOUT;
+			dev_err(hba->dev,
+				"pwr ctrl cmd 0x%x with mode 0x%x timeout\n",
+				cmd->command, cmd->argument3);
+			if (cmd->cmd_active)
+				goto out;
+
+			dev_info(hba->dev, "%s: pwr ctrl cmd has already been completed\n", __func__);
+		}
+
+		/* retry only for busy cases */
+		ret = cmd->argument2 & MASK_UIC_COMMAND_RESULT;
+		if (ret != UIC_CMD_RESULT_BUSY)
+			break;
+
+		dev_info(hba->dev, "%s: PA is busy and can't handle a requeest, %d\n", __func__, ret);
+		mdelay(1);
+
+	} while (time_before(jiffies, deadline));
+out:
+	return ret;
+}
+
 /**
  * ufshcd_uic_pwr_ctrl - executes UIC commands (which affects the link power
  * state) and waits for it to take effect.
@@ -4167,33 +4226,13 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
 	unsigned long flags;
 	u8 status;
 	int ret;
-	bool reenable_intr = false;
 
-	mutex_lock(&hba->uic_cmd_mutex);
-	ufshcd_add_delay_before_dme_cmd(hba);
-
-	spin_lock_irqsave(hba->host->host_lock, flags);
-	if (ufshcd_is_link_broken(hba)) {
-		ret = -ENOLINK;
-		goto out_unlock;
-	}
-	hba->uic_async_done = &uic_async_done;
-	if (ufshcd_readl(hba, REG_INTERRUPT_ENABLE) & UIC_COMMAND_COMPL) {
-		ufshcd_disable_intr(hba, UIC_COMMAND_COMPL);
-		/*
-		 * Make sure UIC command completion interrupt is disabled before
-		 * issuing UIC command.
-		 */
-		wmb();
-		reenable_intr = true;
-	}
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	ret = __ufshcd_send_uic_cmd(hba, cmd, false);
+	ret = __ufshcd_poll_uic_pwr(hba, cmd, &uic_async_done);
 	if (ret) {
-		dev_err(hba->dev,
-			"pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",
-			cmd->command, cmd->argument3, ret);
-		goto out;
+		if (ret == -ENOLINK)
+			goto out_unlock;
+		else
+			goto out;
 	}
 
 	if (!wait_for_completion_timeout(hba->uic_async_done,
@@ -4230,14 +4269,12 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	hba->active_uic_cmd = NULL;
 	hba->uic_async_done = NULL;
-	if (reenable_intr)
-		ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
 	if (ret) {
 		ufshcd_set_link_broken(hba);
 		ufshcd_schedule_eh_work(hba);
 	}
-out_unlock:
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
+out_unlock:
 	mutex_unlock(&hba->uic_cmd_mutex);
 
 	return ret;
-- 
2.7.4

RE: [PATCH v2 2/3] ufs: poll HCS.UCRDY before issuing a UIC command

[Avri Altman]

 
> v1 -> v2: replace usleep_range with udelay because it's a sleepable period.
> 
> With auto hibern8 enabled, UIC could be working for a while to process a
> hibern8 operation and HCI reports UIC not ready for a short term through
> HCS.UCRDY.
> And UFS driver can't recognize the operation.
> UFSHCI spec specifies UCRDY like this:
> whether the host controller is ready to process UIC COMMAND
> 
> The 'ready' could be seen as many different meanings. If the meaning includes
> not processing any request from HCI, processing a hibern8 operation can be
> 'not ready'. In this situation, the driver needs to wait until the operations is
> completed.
> 
> Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> ---
>  drivers/ufs/core/ufshcd.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
> a89d39a..1f58a20 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2365,7 +2365,18 @@ static inline int ufshcd_hba_capabilities(struct
> ufs_hba *hba)
>   */
>  static inline bool ufshcd_ready_for_uic_cmd(struct ufs_hba *hba)  {
> -       return ufshcd_readl(hba, REG_CONTROLLER_STATUS) &
> UIC_COMMAND_READY;
> +       ktime_t timeout = ktime_add_ms(ktime_get(), UIC_CMD_TIMEOUT);
> +       u32 val = 0;
> +
> +       do {
> +               val = ufshcd_readl(hba, REG_CONTROLLER_STATUS) &
> +                       UIC_COMMAND_READY;
> +               if (val)
> +                       break;
> +               udelay(500);
> +       } while (ktime_before(ktime_get(), timeout));
> +
> +       return val ? true : false;
>  }
Can you use read_poll_timeout() instead?

Thanks,
Avri

> 
>  /**
> --
> 2.7.4

Re: [PATCH v2 1/3] ufs: make __ufshcd_send_uic_cmd not wrapped by host_lock

[Bart Van Assche]

On 6/4/23 18:15, Kiwoong Kim wrote:
> __ufshcd_send_uic_cmd is wrapped uic_cmd_mutex and
> its related contexts are accessed within the period wrappted
> by uic_cmd_mutex. Thus, wrapping with host_lock is
> redundant.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v2 3/3] ufs: poll pmc until another pa request is completed

[Bart Van Assche]

On 6/4/23 18:15, Kiwoong Kim wrote:
> v2 -> v3
> 1) check time in the loop with jiffies, instead of miliseconds.
> 2) change a variable's name and fix a typo and wrong alignment.
> 
> v1 -> v2
> 1) remove clearing hba->active_uic_cmd at the end of __ufshcd_poll_uic_pwr
> 2) change commit message on the cited clause: 5.7.12.11 -> 5.7.12.1.1
> 3) add mdelay to avoid too many issueing
> 4) change the timeout for the polling to 100ms because I found it
> sometimes takes much longer than expected.

A change history like the above should either occur below the "---" 
separator or in a cover letter instead of in the patch description.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
[ ... ]

There are two changes in this patch: the introduction of the 
__ufshcd_poll_uic_pwr() helper function and also the introduction of a 
wait loop. Please split this patch into two patches - one patch that 
introduces the helper function and a second patch that introduces the 
wait loop. That will make this patch series easier to review.

Thanks,

Bart.

RE: [PATCH v2 2/3] ufs: poll HCS.UCRDY before issuing a UIC command

[Kiwoong Kim]

> >  static inline bool ufshcd_ready_for_uic_cmd(struct ufs_hba *hba)  {
> > -       return ufshcd_readl(hba, REG_CONTROLLER_STATUS) &
> > UIC_COMMAND_READY;
> > +       ktime_t timeout = ktime_add_ms(ktime_get(), UIC_CMD_TIMEOUT);
> > +       u32 val = 0;
> > +
> > +       do {
> > +               val = ufshcd_readl(hba, REG_CONTROLLER_STATUS) &
> > +                       UIC_COMMAND_READY;
> > +               if (val)
> > +                       break;
> > +               udelay(500);
> > +       } while (ktime_before(ktime_get(), timeout));
> > +
> > +       return val ? true : false;
> >  }
> Can you use read_poll_timeout() instead?
> 
> Thanks,
> Avri

Okay, thanks.

RE: [PATCH v2 3/3] ufs: poll pmc until another pa request is completed

[Kiwoong Kim]

> A change history like the above should either occur below the "---"
> separator or in a cover letter instead of in the patch description.
> 
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> [ ... ]

Thanks for your information.

> 
> There are two changes in this patch: the introduction of the
> __ufshcd_poll_uic_pwr() helper function and also the introduction of a
> wait loop. Please split this patch into two patches - one patch that
> introduces the helper function and a second patch that introduces the wait
> loop. That will make this patch series easier to review.
> 
> Thanks,
> 
> Bart.

I got it and I have one question.

The reason why I bound all the three things is
because I thought they were tangled each other. I felt that the first patch relies
on both the second one and the third one and the helper named __ufshcd_poll_uic_pwr
in the third one calls ufshcd_ready_for_uic_cmd where the change of the second one is applied.
So I thought I should care this sort of conflict in terms of the driver's working.
Don't submitters need to care this?

Thanks.
Kiwoong Kim

Re: [PATCH v2 3/3] ufs: poll pmc until another pa request is completed

[Bart Van Assche]

On 6/7/23 19:52, Kiwoong Kim wrote:
> I got it and I have one question.
> 
> The reason why I bound all the three things is
> because I thought they were tangled each other. I felt that the first patch relies
> on both the second one and the third one and the helper named __ufshcd_poll_uic_pwr
> in the third one calls ufshcd_ready_for_uic_cmd where the change of the second one is applied.
> So I thought I should care this sort of conflict in terms of the driver's working.
> Don't submitters need to care this?

Sorry but the above is not clear to me so I don't know what to answer.

As you may have noticed, the request for one change per patch is a 
common request. From Documentation/process/submitting-patches.rst: 
"Solve only one problem per patch. If your description starts to get 
long, that's a sign that you probably need to split up your patch.
See :ref:`split_changes`."

Thanks,

Bart.