| Message ID | 1682385635-43601-1-git-send-email-kwmad.kim@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] ufs: poll pmc until another pa request is completed | expand |
On 4/24/23 18:20, Kiwoong Kim wrote: > @@ -4138,6 +4141,61 @@ 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) What does the name "cnf" mean? To me it seems to be a weird name for a completion function pointer. > +{ > + unsigned long flags; > + int ret; > + ktime_t timeout; > + u32 mode = cmd->argument3; Is my understanding correct that __ufshcd_send_uic_cmd() does not modify cmd->argument3? If so, why does this function copy cmd->argument3 and re-assign cmd->argument3? > + timeout = ktime_add_ms(ktime_get(), UIC_PA_RDY_TIMEOUT); "deadline" is probably a better name for this variable than "timeout". Additionally, please consider using jiffies since I think that the accuracy of the jiffies counter is sufficient in this context. > + do { > + spin_lock_irqsave(hba->host->host_lock, flags); > + hba->active_uic_cmd = NULL; Is my understanding correct that it is guaranteed that hba->active_uic_cmd is NULL here? If so, what is the purpose of the above statement? > + ret = __ufshcd_send_uic_cmd(hba, cmd, true); > + spin_unlock_irqrestore(hba->host->host_lock, flags); > + 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))) { Please align msecs_to_jiffies(5) with the first argument ("&cmd->done"). > + 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 for only busy cases */ Please fix the word order in the above comment (for only -> only for) Thanks, Bart.
> > @@ -4138,6 +4141,61 @@ 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) > > What does the name "cnf" mean? To me it seems to be a weird name for a > completion function pointer. 'cnf' is a term used in Unipro spec and I thought it's good to use terms in the spec, especially in this file. ufshcd.c is an implementation of UFS and its related specifications. It's a notification meaning that UFS host's Unipro HW receives a UIC request from the host. I guess maybe 'cnf' stands for 'confirm' but I thought 'confirm' look a little bit abstract. If you have an better idea of naming it, please let me know. > > > +{ > > + unsigned long flags; > > + int ret; > > + ktime_t timeout; > > + u32 mode = cmd->argument3; > > Is my understanding correct that __ufshcd_send_uic_cmd() does not modify > cmd->argument3? If so, why does this function copy cmd->argument3 and > re-assign cmd->argument3? This is for the case when unipro responds w/ busy(09h). When IS.UCCS is enabled and is raised, UFS driver updates cmd->argumen3. With this patch, it will go through the loop again w/ an unexpected value of cmd->argumen3. > > > + timeout = ktime_add_ms(ktime_get(), UIC_PA_RDY_TIMEOUT); > > "deadline" is probably a better name for this variable than "timeout". > Additionally, please consider using jiffies since I think that the > accuracy of the jiffies counter is sufficient in this context. > > > + do { > > + spin_lock_irqsave(hba->host->host_lock, flags); > > + hba->active_uic_cmd = NULL; > > Is my understanding correct that it is guaranteed that > hba->active_uic_cmd is NULL here? If so, what is the purpose of the > above statement? Yeah, putting 'hba->active_uic_cmd = NULL' after wait_for_completion_timeout looks natural. But you can see there is one goto case w/ a UIC command not issued for UIC not ready, i.e. !ufshcd_ready_for_uic_cmd. To cover it together, 'hba->active_uic_cmd = NULL' has to be also put at the end of this function and even wrapped w/ the spin lock. I wanted to reduce LOC and found a period already wrapped by the spin lock. That is, it has the same result, I thought. > > > + ret = __ufshcd_send_uic_cmd(hba, cmd, true); > > + spin_unlock_irqrestore(hba->host->host_lock, flags); > > + 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))) { > > Please align msecs_to_jiffies(5) with the first argument ("&cmd->done"). > > > + 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 for only busy cases */ > > Please fix the word order in the above comment (for only -> only for) > > Thanks, > > Bart. For others, let me change it. Thanks. Kiwoong Kim
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 9434328..96ce6af 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; @@ -4138,6 +4141,61 @@ 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; + ktime_t timeout; + u32 mode = cmd->argument3; + + timeout = ktime_add_ms(ktime_get(), 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; + cmd->argument2 = 0; + cmd->argument3 = mode; + ret = __ufshcd_send_uic_cmd(hba, cmd, true); + spin_unlock_irqrestore(hba->host->host_lock, flags); + 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 for only 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 (ktime_before(ktime_get(), timeout)); +out: + return ret; +} + /** * ufshcd_uic_pwr_ctrl - executes UIC commands (which affects the link power * state) and waits for it to take effect. @@ -4160,33 +4218,16 @@ 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; - } - ret = __ufshcd_send_uic_cmd(hba, cmd, false); - spin_unlock_irqrestore(hba->host->host_lock, flags); + 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, @@ -4223,14 +4264,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;
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 | 89 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 64 insertions(+), 25 deletions(-)