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

[Kiwoong Kim]

@ , 안녕하세요.


Thanks.
Kiwoong Kim


> -----Original Message-----
> From: Kiwoong Kim <kwmad.kim@xxxxxxxxxxx>
> Sent: Thursday, September 7, 2023 10:48 AM
> To: linux-scsi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> alim.akhtar@xxxxxxxxxxx; avri.altman@xxxxxxx; bvanassche@xxxxxxx;
> jejb@xxxxxxxxxxxxx; martin.petersen@xxxxxxxxxx; beanhuo@xxxxxxxxxx;
> adrian.hunter@xxxxxxxxx; sc.suh@xxxxxxxxxxx; hy50.seo@xxxxxxxxxxx;
> sh425.lee@xxxxxxxxxxx; kwangwon.min@xxxxxxxxxxx; junwoo80.lee@xxxxxxxxxxx;
> wkon.kim@xxxxxxxxxxx
> Cc: Kiwoong Kim <kwmad.kim@xxxxxxxxxxx>
> 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@xxxxxxxxxxx>
> 
> ---
> 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@xxxxxxxxxxx>
> ---
>  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