Re: [PATCH v1 5/5] scsi: ufs: Complete pending requests in host reset and restore path

[cang]

On 2019-11-13 10:24, Alim Akhtar wrote:
> Hi Can,
>
> On Fri, Nov 8, 2019 at 1:50 PM Can Guo <cang@xxxxxxxxxxxxxx> wrote:
> > In UFS host reset and restore path, before probe, we stop and start 
> > the
> > host controller once. After host controller is stopped, the pending
> > requests, if any, are cleared from the doorbell, but no completion IRQ
> > would be raised due to the hba is stopped.
> > These pending requests shall be completed along with the first NOP_OUT
> > command(as it is the first command which can raise a transfer 
> > completion
> > IRQ) sent during probe.
> > Since the OCSs of these pending requests are not SUCCESS(because they 
> > are
> > not yet literally finished), their UPIUs shall be dumped. When there 
> > are
> > multiple pending requests, the UPIU dump can be overwhelming and may 
> > lead
> > to stability issues because it is in atomic context.
> > Therefore, before probe, complete these pending requests right after 
> > host
> > controller is stopped.
> >
> > Signed-off-by: Can Guo <cang@xxxxxxxxxxxxxx>
> > ---
> Looks good, I hope this is tested on your platform.
> Reviewed-by: Alim Akhtar <alim.akhtar@xxxxxxxxxxx>

Hi Alim,

Thanks for the review. We've tested it out on our platforms.

Best regards,
Can Guo.

> >  drivers/scsi/ufs/ufshcd.c | 20 +++++++-------------
> >  1 file changed, 7 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 5950a7c..4df4136 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -5404,8 +5404,8 @@ static void ufshcd_err_handler(struct 
> > work_struct *work)
> >
> >         /*
> >          * if host reset is required then skip clearing the pending
> > -        * transfers forcefully because they will automatically get
> > -        * cleared after link startup.
> > +        * transfers forcefully because they will get cleared during
> > +        * host reset and restore
> >          */
> >         if (needs_reset)
> >                 goto skip_pending_xfer_clear;
> > @@ -6333,9 +6333,13 @@ static int ufshcd_host_reset_and_restore(struct 
> > ufs_hba *hba)
> >         int err;
> >         unsigned long flags;
> >
> > -       /* Reset the host controller */
> > +       /*
> > +        * Stop the host controller and complete the requests
> > +        * cleared by h/w
> > +        */
> >         spin_lock_irqsave(hba->host->host_lock, flags);
> >         ufshcd_hba_stop(hba, false);
> > +       ufshcd_complete_requests(hba);
> >         spin_unlock_irqrestore(hba->host->host_lock, flags);
> >
> >         /* scale up clocks to max frequency before full 
> > reinitialization */
> > @@ -6369,7 +6373,6 @@ static int ufshcd_host_reset_and_restore(struct 
> > ufs_hba *hba)
> >  static int ufshcd_reset_and_restore(struct ufs_hba *hba)
> >  {
> >         int err = 0;
> > -       unsigned long flags;
> >         int retries = MAX_HOST_RESET_RETRIES;
> >
> >         do {
> > @@ -6379,15 +6382,6 @@ static int ufshcd_reset_and_restore(struct 
> > ufs_hba *hba)
> >                 err = ufshcd_host_reset_and_restore(hba);
> >         } while (err && --retries);
> >
> > -       /*
> > -        * After reset the door-bell might be cleared, complete
> > -        * outstanding requests in s/w here.
> > -        */
> > -       spin_lock_irqsave(hba->host->host_lock, flags);
> > -       ufshcd_transfer_req_compl(hba);
> > -       ufshcd_tmc_handler(hba);
> > -       spin_unlock_irqrestore(hba->host->host_lock, flags);
> > -
> >         return err;
> >  }
> >
> > --
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
> > Forum,
> > a Linux Foundation Collaborative Project