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

[Avri Altman]

> Regarding 5.7.12.11 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.
Can you please elaborate on how this concurrency can happen?
My understanding is that both line reset indication and uic command are protected by host_lock?

> 
> [  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@xxxxxxxxxxx>
> ---
>  drivers/ufs/core/ufshcd.c | 92 ++++++++++++++++++++++++++++++++++---------
> ----
>  1 file changed, 67 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 9434328..3fa58d9 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     30      /* millisecs */
Is this something that is common to all hosts?

Thanks,
Avri

> +
>  /* UFSHC 4.0 compliant HC support this mode, refer param_set_mcq_mode()
> */
>  static bool use_mcq_mode = true;
> 
> @@ -4138,6 +4141,64 @@ 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\n",
> __func__);
> +
> +       } while (ktime_before(ktime_get(), timeout));
> +out:
> +       spin_lock_irqsave(hba->host->host_lock, flags);
> +       hba->active_uic_cmd = NULL;
> +       spin_unlock_irqrestore(hba->host->host_lock, flags);
> +
> +       return ret;
> +}
> +
>  /**
>   * ufshcd_uic_pwr_ctrl - executes UIC commands (which affects the link
> power
>   * state) and waits for it to take effect.
> @@ -4160,33 +4221,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 +4267,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