Re: [PATCH v4 5/6] scsi: ufs: core: Fix double release in ufshcd_abort()

Re: [PATCH v4 5/6] scsi: ufs: core: Fix double release in ufshcd_abort()

[Peter Wang (王信友)]

On Wed, 2026-10-21 at 08:28 -0500, Stanley Jhu wrote:
> On Wed, 2026-10-21 at 21:12 +0800, Peter Wang wrote:
> > If ufshcd_clear_outstanding_reqs(hba, BIT(tag)) returns 0,
> > should we also set err = SUCCESS? Since the tag is already done
> > — either by the ISR or by ufshcd_abort — shouldn't both cases
> > be treated the same?
> 
> A return value of 0 here means the completion path, rather than
> this abort path, has claimed the tag and may still be running
> ufshcd_release_scsi_cmd() after it dropped outstanding_lock.
> SUCCESS tells the SCSI error handler it can retry or finish the
> command right away, so a retry could map the same command again
> while the completion path is still unmapping it.
> 
> Returning FAILED here also matches the check at the top of
> ufshcd_abort(), which already returns FAILED when the tag is no
> longer set in hba->outstanding_reqs.
> 
> Thanks,
> Stanley

Okay, that is correct. Thanks for the explanation.

Peter

Re: [PATCH v4 5/6] scsi: ufs: core: Fix double release in ufshcd_abort()

[Peter Wang]

On Wed, 2026-10-21 at 09:37 -0700, Bart Van Assche wrote:
> On 10/21/26 6:28 AM, Stanley Jhu wrote:
> > A return value of 0 here means the completion path, rather than
> > this abort path, has claimed the tag and may still be running
> > ufshcd_release_scsi_cmd() after it dropped outstanding_lock.
> Do we really need the code that checks that a command has completed
> without notification interrupt? I have never encountered any logs on
> any test setup that indicates that this happened.
> 
> Thanks,
> 
> Bart.

I remember that when the SSU timeout was only 1 sec, we'd pretty
easily hit the concurrent execution case between the timeout and
the complete path. But after change it to 10 sec along with patch 
9a5f6c09d0fa (scsi: ufs: core: Simplify ufshcd_try_to_abort_task()), 
it shouldn't happen anymore.

Thanks
Peter

Re: [PATCH v4 5/6] scsi: ufs: core: Fix double release in ufshcd_abort()

[Peter Wang]

On Wed, 2026-10-21 at 08:28 -0500, Stanley Jhu wrote:
> On Wed, 2026-10-21 at 21:12 +0800, Peter Wang wrote:
> > If ufshcd_clear_outstanding_reqs(hba, BIT(tag)) returns 0,
> > should we also set err = SUCCESS? Since the tag is already done
> > — either by the ISR or by ufshcd_abort — shouldn't both cases
> > be treated the same?
> 
> A return value of 0 here means the completion path, rather than
> this abort path, has claimed the tag and may still be running
> ufshcd_release_scsi_cmd() after it dropped outstanding_lock.
> SUCCESS tells the SCSI error handler it can retry or finish the
> command right away, so a retry could map the same command again
> while the completion path is still unmapping it.
> 
> Returning FAILED here also matches the check at the top of
> ufshcd_abort(), which already returns FAILED when the tag is no
> longer set in hba->outstanding_reqs.
> 
> Thanks,
> Stanley

Okay, that is correct. Thanks for the explanation.

Peter

Re: [PATCH v4 5/6] scsi: ufs: core: Fix double release in ufshcd_abort()

[Stanley Jhu]

On 10/21/26 9:37 AM, Bart Van Assche wrote:
> Do we really need the code that checks that a command has completed
> without notification interrupt? I have never encountered any logs on
> any test setup that indicates that this happened.

Agreed, we do not need this branch, and v5 removes it. If a completion
interrupt is lost or arrives after the command times out, leaving the
command to the SCSI error handler makes recovery slower in that rare
corner case, which is acceptable since it only happens after a timeout.

I will send v5 shortly.

Re: [PATCH v4 5/6] scsi: ufs: core: Fix double release in ufshcd_abort()

[Bart Van Assche]

On 10/21/26 6:28 AM, Stanley Jhu wrote:
> A return value of 0 here means the completion path, rather than
> this abort path, has claimed the tag and may still be running
> ufshcd_release_scsi_cmd() after it dropped outstanding_lock.
Do we really need the code that checks that a command has completed
without notification interrupt? I have never encountered any logs on
any test setup that indicates that this happened.

Thanks,

Bart.

Re: [PATCH v4 5/6] scsi: ufs: core: Fix double release in ufshcd_abort()

[Stanley Jhu]

On Wed, 2026-10-21 at 21:12 +0800, Peter Wang wrote:
> If ufshcd_clear_outstanding_reqs(hba, BIT(tag)) returns 0,
> should we also set err = SUCCESS? Since the tag is already done
> — either by the ISR or by ufshcd_abort — shouldn't both cases
> be treated the same?

A return value of 0 here means the completion path, rather than
this abort path, has claimed the tag and may still be running
ufshcd_release_scsi_cmd() after it dropped outstanding_lock.
SUCCESS tells the SCSI error handler it can retry or finish the
command right away, so a retry could map the same command again
while the completion path is still unmapping it.

Returning FAILED here also matches the check at the top of
ufshcd_abort(), which already returns FAILED when the tag is no
longer set in hba->outstanding_reqs.

Thanks,
Stanley

Re: [PATCH v4 5/6] scsi: ufs: core: Fix double release in ufshcd_abort()

[Peter Wang]

On Wed, 2026-10-21 at 12:06 +0800, Stanley Jhu wrote:
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index ff5977f7da68..4f9b34c1d112 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8185,7 +8185,11 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>  		dev_err(hba->dev,
>  		"%s: cmd was completed, but without a notifying
> intr, tag = %d",
>  		__func__, tag);
> -		__ufshcd_transfer_req_compl(hba, 1UL << tag);
> +		if (ufshcd_clear_outstanding_reqs(hba, BIT(tag))) {
> +			if (ufshcd_is_scsi_cmd(cmd))
> +				ufshcd_release_scsi_cmd(hba, cmd);
> +			err = SUCCESS;
> +		}
> 

Hi Stanley,

If ufshcd_clear_outstanding_reqs(hba, BIT(tag)) returns 0, 
should we also set err = SUCCESS? Since the tag is already done
— either by the ISR or by ufshcd_abort — shouldn't both cases 
be treated the same?

Thanks
Peter

[PATCH v4 5/6] scsi: ufs: core: Fix double release in ufshcd_abort()

[Stanley Jhu]

In SDB mode, a timed-out command whose completion interrupt has not
been handled yet can be released twice:

1. The command times out and scsi_timeout() schedules ufshcd_abort().
2. The command finishes in hardware. Its doorbell bit is clear, but the
   completion interrupt has not been handled, so BIT(tag) is still set
   in hba->outstanding_reqs.
3. ufshcd_abort() sees the doorbell bit clear and calls
   __ufshcd_transfer_req_compl(). That releases the command but leaves
   BIT(tag) set in hba->outstanding_reqs. ufshcd_abort() returns
   FAILED.
4. The next ufshcd_poll() call, or the logical unit reset that follows
   the FAILED, sees BIT(tag) set in hba->outstanding_reqs and releases
   the same command again.

Claim BIT(tag) in hba->outstanding_reqs with
ufshcd_clear_outstanding_reqs(), release the command if it is a SCSI
command, and return SUCCESS when ufshcd_abort() claims the bit.

Fixes: 169f5eb28869 ("scsi: ufs: Optimize SCSI command processing")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/2e042211ca0ebc974ad5ec4c0de9f3f93b3c75c0.camel@iokpp.de
Suggested-by: Bean Huo <beanhuo@micron.com>
Signed-off-by: Stanley Jhu <stanleyjhu@google.com>
---
 drivers/ufs/core/ufshcd.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index ff5977f7da68..4f9b34c1d112 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8185,7 +8185,11 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 		dev_err(hba->dev,
 		"%s: cmd was completed, but without a notifying intr, tag = %d",
 		__func__, tag);
-		__ufshcd_transfer_req_compl(hba, 1UL << tag);
+		if (ufshcd_clear_outstanding_reqs(hba, BIT(tag))) {
+			if (ufshcd_is_scsi_cmd(cmd))
+				ufshcd_release_scsi_cmd(hba, cmd);
+			err = SUCCESS;
+		}
 		goto release;
 	}
 
-- 
2.56.0.212.g129ede20d9-goog