diff mbox series

[v4] ufs: poll pmc until another pa request is completed

Message ID 1694051306-172962-1-git-send-email-kwmad.kim@samsung.com (mailing list archive)
State Changes Requested
Headers show
Series [v4] ufs: poll pmc until another pa request is completed | expand

Commit Message

Kiwoong Kim Sept. 7, 2023, 1:48 a.m. UTC
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.

And this patch requires the following patch because it assumes
__ufshcd_send_uic_cmd doesn't require host_lock.
https://lore.kernel.org/linux-scsi/bec84ee1ce8f5c5971b98d8e501aeabb9b5b26d1.1686716811.git.kwmad.kim@samsung.com/

Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>

---
v3 -> v4
1) change description

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.

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

Comments

Kiwoong Kim Sept. 11, 2023, 7:31 a.m. UTC | #1
@ , 안녕하세요.


Thanks.
Kiwoong Kim


> -----Original Message-----
> From: Kiwoong Kim <kwmad.kim@samsung.com>
> Sent: Thursday, September 7, 2023 10:48 AM
> To: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org;
> alim.akhtar@samsung.com; avri.altman@wdc.com; bvanassche@acm.org;
> jejb@linux.ibm.com; martin.petersen@oracle.com; beanhuo@micron.com;
> adrian.hunter@intel.com; sc.suh@samsung.com; hy50.seo@samsung.com;
> sh425.lee@samsung.com; kwangwon.min@samsung.com; junwoo80.lee@samsung.com;
> wkon.kim@samsung.com
> Cc: Kiwoong Kim <kwmad.kim@samsung.com>
> Subject: [PATCH v4] ufs: poll pmc until another pa request is completed
> 
> 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.
> 
> And this patch requires the following patch because it assumes
> __ufshcd_send_uic_cmd doesn't require host_lock.
> https://lore.kernel.org/linux-
> scsi/bec84ee1ce8f5c5971b98d8e501aeabb9b5b26d1.1686716811.git.kwmad.kim@sam
> sung.com/
> 
> Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> 
> ---
> v3 -> v4
> 1) change description
> 
> 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.
> 
> 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
> 7bc3fc4..f6b8659 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. */  static bool use_mcq_mode
> = true;
> 
> @@ -4120,6 +4123,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.
> @@ -4142,33 +4201,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,
> @@ -4205,14 +4244,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
Kiwoong Kim Sept. 11, 2023, 7:34 a.m. UTC | #2
Thinking again, removing the mutex completely doesn't seem properly
because this could hurting atomicity of UIC command process.
Let me modify and post it again.
diff mbox series

Patch

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 7bc3fc4..f6b8659 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. */
 static bool use_mcq_mode = true;
 
@@ -4120,6 +4123,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.
@@ -4142,33 +4201,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,
@@ -4205,14 +4244,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;