Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <5c6f1ad9f703cc5721e081452e869a9ee6bc4ab6.camel@gmail.com>
Subject: Re: [PATCH v1] scsi: ufs: no need to send one Abort Task TM in case
 the task in DB was cleared
From:   Bean Huo <huobean@gmail.com>
To:     Can Guo <cang@codeaurora.org>,
        Stanley Chu <stanley.chu@mediatek.com>
Cc:     alim.akhtar@samsung.com, avri.altman@wdc.com,
        asutoshd@codeaurora.org, jejb@linux.ibm.com,
        martin.petersen@oracle.com, stanley.chu@mediatek.com,
        beanhuo@micron.com, bvanassche@acm.org, tomas.winkler@intel.com,
        linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Mon, 10 Aug 2020 17:41:22 +0200
In-Reply-To: <5ad1dbd76a0d5d476641a01bfb8bd435@codeaurora.org>
References: <20200804123534.29104-1-huobean@gmail.com>
         <a68a1bdf74bdf8ada29808537290b35b@codeaurora.org>
         <871fdbc1719d7a3c469bf857071aa2c6bd71ddaf.camel@gmail.com>
         <5ad1dbd76a0d5d476641a01bfb8bd435@codeaurora.org>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@micron.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@micron.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