linux-kernel - Re: [PATCH v1] scsi: ufs: no need to send one Abort Task TM in case the task in DB was cleared

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1597101233.19734.0.camel@mtkswgap22>
Date:   Tue, 11 Aug 2020 07:13:53 +0800
From:   Stanley Chu <stanley.chu@...iatek.com>
To:     Bean Huo <huobean@...il.com>
CC:     Can Guo <cang@...eaurora.org>, <alim.akhtar@...sung.com>,
        <avri.altman@....com>, <asutoshd@...eaurora.org>,
        <jejb@...ux.ibm.com>, <martin.petersen@...cle.com>,
        <beanhuo@...ron.com>, <bvanassche@....org>,
        <tomas.winkler@...el.com>, <linux-scsi@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1] scsi: ufs: no need to send one Abort Task TM in case
 the task in DB was cleared

Hi Bean,

On Mon, 2020-08-10 at 17:41 +0200, Bean Huo wrote:
> On Thu, 2020-08-06 at 18:07 +0800, Can Guo wrote:
> > Hi Bean,
> > 
> > On 2020-08-06 17:50, Bean Huo wrote:
> > > > 
> > > > Please check Stanley's recent change to ufshcd_abort, you may
> > > > want to rebase your change on his and do goto cleanup here.
> > > > @Stanley correct me if I am wrong.
> > > > 
> > > > But even if you do a goto cleanup here, we still lost the
> > > > chances to dump host infos/regs like it does in the old code.
> > > > If a cmd was completed but without a notifying intr, this is
> > > > kind of a problem that we/host should look into, because it's
> > > > pasted at least 30 sec since the cmd was sent, so those dumps
> > > > are necessary to debug the problem. How about moving blow prints
> > > > in front of this part?
> > > > 
> > > > Thanks,
> > > > 
> > > > Can Guo.
> > > > 
> > > > >  	}
> > > > > 
> > > > >  	/* Print Transfer Request of aborted task */
> > > 
> > > Hi Can
> > > 
> > > Thanks, do you mean that change to like this:
> > > 
> > > 
> > > Author: Bean Huo <beanhuo@...ron.com>
> > > Date:   Thu Aug 6 11:34:45 2020 +0200
> > > 
> > >     scsi: ufs: no need to send one Abort Task TM in case the task
> > > in
> > >   was cleared
> > > 
> > >     If the bit corresponds to a task in the Doorbell register has
> > > been
> > >     cleared, no need to poll the status of the task on the device
> > > side
> > >     and to send an Abort Task TM.
> > >     This patch also deletes dispensable dev_err() in case of the
> > > task
> > >     already completed.
> > > 
> > >     Signed-off-by: Bean Huo <beanhuo@...ron.com>
> > > 
> > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > index 307622284239..f7c91ce9e294 100644
> > > --- a/drivers/scsi/ufs/ufshcd.c
> > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > @@ -6425,23 +6425,9 @@ static int ufshcd_abort(struct scsi_cmnd
> > > *cmd)
> > >                 return ufshcd_eh_host_reset_handler(cmd);
> > > 
> > >         ufshcd_hold(hba, false);
> > > -       reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> > >         /* If command is already aborted/completed, return SUCCESS
> > > */
> > > -       if (!(test_bit(tag, &hba->outstanding_reqs))) {
> > > -               dev_err(hba->dev,
> > > -                       "%s: cmd at tag %d already completed,
> > > outstanding=0x%lx, doorbell=0x%x\n",
> > > -                       __func__, tag, hba->outstanding_reqs, reg);
> > > +       if (!(test_bit(tag, &hba->outstanding_reqs)))
> > >                 goto out;
> > > -       }
> > > -
> > > -       if (!(reg & (1 << tag))) {
> > > -               dev_err(hba->dev,
> > > -               "%s: cmd was completed, but without a notifying
> > > intr,
> > > tag = %d",
> > > -               __func__, tag);
> > > -       }
> > > -
> > > -       /* Print Transfer Request of aborted task */
> > > -       dev_err(hba->dev, "%s: Device abort task at tag %d\n",
> > > __func__, tag);
> > > 
> > >         /*
> > >          * Print detailed info about aborted request.
> > > @@ -6462,6 +6448,17 @@ static int ufshcd_abort(struct scsi_cmnd
> > > *cmd)
> > >         }
> > >         hba->req_abort_count++;
> > > 
> > > +       reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> > > +       if (!(reg & (1 << tag))) {
> > > +               dev_err(hba->dev,
> > > +               "%s: cmd was completed, but without a notifying
> > > intr,
> > > tag = %d",
> > > +               __func__, tag);
> > > +               goto cleanup;
> > > +       }
> > > +
> > > +       /* Print Transfer Request of aborted task */
> > > +       dev_err(hba->dev, "%s: Device abort task at tag %d\n",
> > > __func__, tag);
> > > +
> > 
> > The rest looks good but let below two lines stay where they were.
> > 
> >         /* Print Transfer Request of aborted task */
> >         dev_err(hba->dev, "%s: Device abort task at tag %d\n",
> > __func__, tag);
> > 
> > 
> > Thanks,
> > 
> > Can Guo.
> > 
> Hi Can
> I will change it in the next version.
> 
> 
> Hi Stanly
> would you mind I take your patch into my next version patchset? Since
> we both will add a new same goto label. I will keep your patch
> authorship.


Sure, OK to me : )

Thanks,

Stanley Chu