RE: [PATCH v6 05/10] scsi: ufs: add quirk to fix abnormal ocs fatal error

["Alim Akhtar" <alim.akhtar@samsung.com>]

Hi Avri

> -----Original Message-----
> From: Avri Altman <Avri.Altman@wdc.com>
> Sent: 21 April 2020 17:37
> To: Kiwoong Kim <kwmad.kim@samsung.com>; 'Alim Akhtar'
> <alim.akhtar@samsung.com>; robh@kernel.org; cpgs@samsung.com
> Cc: devicetree@vger.kernel.org; linux-scsi@vger.kernel.org; krzk@kernel.org;
> martin.petersen@oracle.com; cang@codeaurora.org; linux-samsung-
> soc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Subject: RE: [PATCH v6 05/10] scsi: ufs: add quirk to fix abnormal ocs fatal error
> 
> >
> > > -----Original Message-----
> > > From: Avri Altman <Avri.Altman@wdc.com>
> > > Sent: Monday, April 20, 2020 5:56 PM
> > > To: Alim Akhtar <alim.akhtar@samsung.com>; robh@kernel.org
> > > Cc: devicetree@vger.kernel.org; linux-scsi@vger.kernel.org;
> > > krzk@kernel.org; martin.petersen@oracle.com;
> > kwmad.kim@samsung.com;
> > > stanley.chu@mediatek.com; cang@codeaurora.org; linux-samsung-
> > > soc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> > > kernel@vger.kernel.org
> > > Subject: RE: [PATCH v6 05/10] scsi: ufs: add quirk to fix abnormal
> > > ocs fatal error
> > >
> > > >
> > > > From: Kiwoong Kim <kwmad.kim@samsung.com>
> > > >
> > > > Some architectures determines if fatal error for OCS occurrs to
> > > > check status in response upiu. This patch
> > > Typo - occurs
> > >
> > > > is to prevent from reporting command results with that.
> > > >
> > > > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> > > > Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> > > > ---
> > > >  drivers/scsi/ufs/ufshcd.c | 6 ++++++  drivers/scsi/ufs/ufshcd.h |
> > > > 6 ++++++
> > > >  2 files changed, 12 insertions(+)
> > > >
> > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > > index b32fcedcdcb9..8c07caff0a5c 100644
> > > > --- a/drivers/scsi/ufs/ufshcd.c
> > > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > > @@ -4794,6 +4794,12 @@ ufshcd_transfer_rsp_status(struct ufs_hba
> > *hba,
> > > > struct ufshcd_lrb *lrbp)
> > > >         /* overall command status of utrd */
> > > >         ocs = ufshcd_get_tr_ocs(lrbp);
> > > >
> > > > +       if (hba->quirks & UFSHCD_QUIRK_BROKEN_OCS_FATAL_ERROR) {
> > > > +               if (be32_to_cpu(lrbp->ucd_rsp_ptr->header.dword_1) &
> > > > +                                       MASK_RSP_UPIU_RESULT)
> > > > +                       ocs = OCS_SUCCESS;
> > > > +       }
> > > > +
> > > Not sure that I follow what this quirk is all about.
> > > Your code overrides ocs by open coding ufshcd_get_rsp_upiu_result.
> > >
> > > Normally OCS is in utp transfer req descriptor, dword 2, bits 0..7.
> > > My understanding from your description, is that some fatal error
> > > might occur, But the host controller does not report it, and it
> > > still needs to be checked in the response upiu.
> > > Evidently you are not doing so.
> > > Please elaborate your description.
> > >
> > > P.S.
> > > The ocs is being evaluated in device management commands as well,
> > > Isn't this something you need to attend?
> > >
> > > Thanks,
> > > Avri
> > >
> > > >         switch (ocs) {
> > > >         case OCS_SUCCESS:
> > > >                 result = ufshcd_get_req_rsp(lrbp->ucd_rsp_ptr);
> > > > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> > > > index a9b9ace9fc72..e1d09c2c4302 100644
> > > > --- a/drivers/scsi/ufs/ufshcd.h
> > > > +++ b/drivers/scsi/ufs/ufshcd.h
> > > > @@ -541,6 +541,12 @@ enum ufshcd_quirks {
> > > >          * resolution of the values of PRDTO and PRDTL in UTRD as byte.
> > > >          */
> > > >         UFSHCD_QUIRK_PRDT_BYTE_GRAN                     = 1 << 9,
> > > > +
> > > > +       /*
> > > > +        * This quirk needs to be enabled if the host controller reports
> > > > +        * OCS FATAL ERROR with device error through sense data
> > > > +        */
> > > > +       UFSHCD_QUIRK_BROKEN_OCS_FATAL_ERROR             = 1 << 10,
> > > >  };
> > > >
> > > >  enum ufshcd_caps {
> > > > --
> > > > 2.17.1
> > Avri
> >
> > As specified in the spec, OCS isn't supposed to refer to the contents
> > of RESPONSE UPIU.
> > But, Exynos host behaves like that in some cases, e.g. a value of
> > 'state' in is isn't GOOD(00h).
> OK.
> I still think that you might consider rewording your commit, explaining this quirk
> better.
> Specifically you might not want to say "if fatal..." because fatal code (0x7) is just
> one error code out of many.
> Also you might want to use ufshcd_get_rsp_upiu_result() in the quirk body
> instead of open coding it.
> 
> >
> > For QUERY RESPONSE, its offset, i.e. " dword_1" is reserved, so
> > currently no impact, I think.
> > But if you feel another condition is necessary to identify if this
> > request is QUERY REQEUST or not, we can add more.
> No need, as long as you are ok with whatever ufshcd_get_tr_ocs() returns in
> ufshcd_wait_for_dev_cmd().
> 
I will update the commit message to make it clear in the next version of the patch set.

> Thanks,
> Avri
> 
> >
> > Thanks