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] [thread-next>] [day] [month] [year] [list]

Message-ID: <5c6f1ad9f703cc5721e081452e869a9ee6bc4ab6.camel@gmail.com>
Date:   Mon, 10 Aug 2020 17:41:22 +0200
From:   Bean Huo <huobean@...il.com>
To:     Can Guo <cang@...eaurora.org>,
        Stanley Chu <stanley.chu@...iatek.com>
Cc:     alim.akhtar@...sung.com, avri.altman@....com,
        asutoshd@...eaurora.org, jejb@...ux.ibm.com,
        martin.petersen@...cle.com, stanley.chu@...iatek.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

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.

Thanks,
Bean