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

[cang]

On 2019-11-18 21:23, Alim Akhtar wrote:
> On Mon, Nov 18, 2019 at 9:22 AM Can Guo <cang@xxxxxxxxxxxxxxxx> wrote:
> > From: Can Guo <cang@xxxxxxxxxxxxxx>
> >
> > 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 and silence the UPIU dump from them.
> >
> > Signed-off-by: Can Guo <cang@xxxxxxxxxxxxxx>
> > ---
>
> Tested-by: Alim Akhtar <alim.akhtar@xxxxxxxxxxx>
>
> Please add all previous Ack/reviewed and tested-by tags so that we are
> aware what need to be done for this patch.
> Thanks

Hi Alim,

Thanks for pointing out it. I updated the patch a little bit so I think 
the
prevoius tags are not valid any more.

Best Regards,
Can Guo.

> >  drivers/scsi/ufs/ufshcd.c | 24 ++++++++++--------------
> >  drivers/scsi/ufs/ufshcd.h |  2 ++
> >  2 files changed, 12 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 5950a7c..b92a3f4 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -4845,7 +4845,7 @@ static void ufshcd_slave_destroy(struct 
> > scsi_device *sdev)
> >                 break;
> >         } /* end of switch */
> >
> > -       if (host_byte(result) != DID_OK)
> > +       if ((host_byte(result) != DID_OK) && !hba->silence_err_logs)
> >                 ufshcd_print_trs(hba, 1 << lrbp->task_tag, true);
> >         return result;
> >  }
> > @@ -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,15 @@ 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);
> > +       hba->silence_err_logs = true;
> > +       ufshcd_complete_requests(hba);
> > +       hba->silence_err_logs = false;
> >         spin_unlock_irqrestore(hba->host->host_lock, flags);
> >
> >         /* scale up clocks to max frequency before full 
> > reinitialization */
> > @@ -6369,7 +6375,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 +6384,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;
> >  }
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> > index e0fe247..1e51034 100644
> > --- a/drivers/scsi/ufs/ufshcd.h
> > +++ b/drivers/scsi/ufs/ufshcd.h
> > @@ -513,6 +513,7 @@ struct ufs_stats {
> >   * @uic_error: UFS interconnect layer error status
> >   * @saved_err: sticky error mask
> >   * @saved_uic_err: sticky UIC error mask
> > + * @silence_err_logs: flag to silence error logs
> >   * @dev_cmd: ufs device management command information
> >   * @last_dme_cmd_tstamp: time stamp of the last completed DME command
> >   * @auto_bkops_enabled: to track whether bkops is enabled in device
> > @@ -670,6 +671,7 @@ struct ufs_hba {
> >         u32 saved_err;
> >         u32 saved_uic_err;
> >         struct ufs_stats ufs_stats;
> > +       bool silence_err_logs;
> >
> >         /* Device management request data */
> >         struct ufs_dev_cmd dev_cmd;
> > --
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
> > Forum,
> > a Linux Foundation Collaborative Project