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;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Thu, 06 Aug 2020 17:03:35 +0800
From:   Can Guo <cang@codeaurora.org>
To:     Bean Huo <huobean@gmail.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
Subject: Re: [PATCH v1] scsi: ufs: no need to send one Abort Task TM in case
 the task in DB was cleared
In-Reply-To: <20200804123534.29104-1-huobean@gmail.com>
References: <20200804123534.29104-1-huobean@gmail.com>
Message-ID: <a68a1bdf74bdf8ada29808537290b35b@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Bean,

On 2020-08-04 20:35, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> 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>
> ---
>  drivers/scsi/ufs/ufshcd.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 307622284239..581b4ab52bf4 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6425,19 +6425,16 @@ 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;
> -	}
> 
> +	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 out;

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 */