| 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
| ||
|
Message-ID: <5ad1dbd76a0d5d476641a01bfb8bd435@codeaurora.org>
Date: Thu, 06 Aug 2020 18:07:37 +0800
From: Can Guo <cang@...eaurora.org>
To: Bean Huo <huobean@...il.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
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.
> /* Skip task abort in case previous aborts failed and report
> failure */
> if (lrbp->req_abort_skip) {
> err = -EIO;
> @@ -6526,6 +6523,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
> goto out;
> }
>
> +cleanup:
> scsi_dma_unmap(cmd);
>
> spin_lock_irqsave(host->host_lock, flags);
>
>
>
>
>
> Author: Stanley Chu <stanley.chu@...iatek.com>
> Date: Thu Aug 6 11:48:00 2020 +0200
>
> scsi: ufs: Cleanup completed request without interrupt notification
>
> If somehow no interrupt notification is raised for a completed
> request
> and its doorbell bit is cleared by host, UFS driver needs to
> cleanup
> its outstanding bit in ufshcd_abort(). Otherwise, system may behave
> abnormally by below flow:
>
> After ufshcd_abort() returns, this request will be requeued by SCSI
> layer with its outstanding bit set. Any future completed request
> will trigger ufshcd_transfer_req_compl() to handle all "completed
> outstanding bits". In this time, the "abnormal outstanding bit"
> will be detected and the "requeued request" will be chosen to
> execute
> request post-processing flow. This is wrong because this request is
> still "alive".
>
> Signed-off-by: Stanley Chu <stanley.chu@...iatek.com>
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index f7c91ce9e294..29d5e5e5d0e0 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6489,7 +6489,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
> /* command completed already */
> dev_err(hba->dev, "%s: cmd at tag %d
> successfully cleared from DB.\n",
> __func__, tag);
> - goto out;
> + goto cleanup;
> } else {
> dev_err(hba->dev,
> "%s: no response from device. tag = %d,
> err %d\n",
Powered by blists - more mailing lists