Re: [PATCH 2/2] scsi: ufs: core: Fix the code for entering hibernation

[Bean Huo <huobean@gmail.com>]

On Wed, 2024-08-21 at 11:29 -0700, Bart Van Assche wrote:
> drivers/ufs/core/ufshcd.c | 22 ++++++----------------
>  include/ufs/ufshcd.h      |  7 ++++---
>  2 files changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index d0ae6e50becc..e12f30b8a83c 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2585,6 +2585,7 @@ int ufshcd_send_uic_cmd(struct ufs_hba *hba,
> struct uic_command *uic_cmd)
>         ufshcd_hold(hba);
>         mutex_lock(&hba->uic_cmd_mutex);
>         ufshcd_add_delay_before_dme_cmd(hba);
> +       WARN_ON(hba->uic_async_done);
>  
>         ret = __ufshcd_send_uic_cmd(hba, uic_cmd, true);
>         if (!ret)
> @@ -4255,7 +4256,6 @@ 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);
> @@ -4266,15 +4266,6 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba
> *hba, struct uic_command *cmd)
>                 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.
> -                */
> -               ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
> -               reenable_intr = true;
> -       }
>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>         ret = __ufshcd_send_uic_cmd(hba, cmd, false);
>         if (ret) {
> @@ -4318,8 +4309,6 @@ 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);
> @@ -5472,11 +5461,12 @@ static irqreturn_t
> ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
>                 hba->errors |= (UFSHCD_UIC_HIBERN8_MASK &
> intr_status);
>  
>         if (intr_status & UIC_COMMAND_COMPL && cmd) {
> -               cmd->argument2 |= ufshcd_get_uic_cmd_result(hba);
> -               cmd->argument3 = ufshcd_get_dme_attr_val(hba);

Bart, 

My only concern is, removing disabling UIC completion IRQ, and keeping
is.uccs 1, then we don't read its status in case of ufshcd_uic_pwr_ctrl
path, whether this will affect the next UIC access result.

Other things look good to me.

Kind regards,
Bean