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

[PATCH v4 0/6] scsi: ufs: core: Factor hba->outstanding_reqs helper and fix double completions

[Stanley Jhu]

In Single Doorbell (SDB) mode, multiple completion and error handling
paths can race on the same transfer request when bits in
hba->outstanding_reqs are not claimed atomically with completing the
request. A stale bit in hba->outstanding_reqs lets a later poll or
reset complete the request again and unmap its DMA buffers twice.

Patch 1 removes a WARN_ONCE() in ufshcd_poll() whose condition is always
false.

Patch 2 adds __ufshcd_clear_outstanding_reqs() and
ufshcd_clear_outstanding_reqs() to share the bit-claim logic across
ufshcd_poll() and ufshcd_abort() with no functional change.

Patch 3 uses the helper in ufshcd_clear_lu_cmds() to fix a double
completion during LU reset.

Patch 4 uses the helper in ufshcd_eh_timed_out() and checks its return
value to prevent releasing driver resources twice.

Patch 5 uses the helper in ufshcd_abort() to fix a double release in
the missing-interrupt path and returns SUCCESS when it claims the bit.

Patch 6 skips releasing reserved requests at the end of ufshcd_abort().

This series is based on mkp/for-next (db8b4d2bf2d3).

Tested: QEMU ARM64 in SDB mode across LU reset, abort, and PM timeout
paths without double DMA unmaps or active_reqs underflow.

Changes since v3:
- Document the helper return value in patch 2 and add Cc: stable to
  patch 1 (Bart).
- Squash v3 patches 4 and 5 into patch 4 (Bart).
- Return SUCCESS when ufshcd_abort() claims BIT(tag) in patch 5 (Bart).
- Link to v3:
  https://lore.kernel.org/r/20261020082110.3511797-1-stanleyjhu@google.com

Stanley Jhu (6):
  scsi: ufs: core: Remove an always-false WARN_ONCE()
  scsi: ufs: core: Add a helper for clearing hba->outstanding_reqs
  scsi: ufs: core: Fix double completion in ufshcd_clear_lu_cmds()
  scsi: ufs: core: Fix double release in ufshcd_eh_timed_out()
  scsi: ufs: core: Fix double release in ufshcd_abort()
  scsi: ufs: core: Do not release a reserved request in ufshcd_abort()

 drivers/ufs/core/ufshcd.c | 67 +++++++++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 21 deletions(-)


base-commit: db8b4d2bf2d3e02b4ff78bbf69d00e11795840de
-- 
2.56.0.212.g129ede20d9-goog

[PATCH v4 3/6] scsi: ufs: core: Fix double completion in ufshcd_clear_lu_cmds()

[Stanley Jhu]

In SDB mode, ufshcd_clear_lu_cmds() clears the request slot in the
controller and completes the request, but leaves its bit set in
hba->outstanding_reqs. ufshcd_poll() treats a request whose bit is set
in hba->outstanding_reqs and clear in the doorbell register as finished,
so the next ufshcd_poll() completes the same request again and unmaps
its DMA buffers twice.

Clear the bit with ufshcd_clear_outstanding_reqs() before completing the
request, and complete it only if the bit was still set. If ufshcd_poll()
has already completed the request, the bit is already clear and
ufshcd_clear_lu_cmds() skips it.

Fixes: f18fac1e2b72 ("scsi: ufs: core: Rework ufshcd_eh_device_reset_handler()")
Cc: stable@vger.kernel.org
Reviewed-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Peter Wang <peter.wang@mediatek.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Stanley Jhu <stanleyjhu@google.com>
---
 drivers/ufs/core/ufshcd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 1db572f84971..68b53f89f8c6 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -7987,7 +7987,8 @@ static bool ufshcd_clear_lu_cmds(struct request *req, void *priv)
 		return true;
 	}
 
-	ufshcd_compl_one_cqe(hba, tag, NULL);
+	if (ufshcd_clear_outstanding_reqs(hba, BIT(tag)))
+		ufshcd_compl_one_cqe(hba, tag, NULL);
 	return true;
 }
 
-- 
2.56.0.212.g129ede20d9-goog

[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

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

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()

[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 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()

[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