| 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 |
@ , 안녕하세요. 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
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 --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;