[PATCH] block: Keep RPM_ACTIVE in blk_pre_runtime_resume() when already active

[PATCH] block: Keep RPM_ACTIVE in blk_pre_runtime_resume() when already active

[Stanley Jhu]

Since commit eef1d74bf88f ("PM: runtime: Only set runtime_error on
suspend callback failures"), a failed runtime resume leaves
dev->power.runtime_error at 0 with runtime_status set to RPM_SUSPENDED,
so later runtime resume requests run sdev_runtime_resume() again:

1. A runtime suspend increments q->pm_only from 0 to 1 and sets
   q->rpm_status to RPM_SUSPENDED.
2. The first runtime resume fails in the driver callback, and
   blk_post_runtime_resume() sets q->rpm_status to RPM_ACTIVE and calls
   blk_clear_pm_only(), decrementing q->pm_only from 1 to 0 so recovery
   commands can enter the queue (intended since
   commit 6e1fcab00a23 ("scsi: block: pm: Always set request queue
   runtime active in blk_post_runtime_resume()")).
3. A retried runtime resume calls blk_pre_runtime_resume(), which
   overwrites q->rpm_status from RPM_ACTIVE to RPM_RESUMING, so
   blk_post_runtime_resume() sees old_status != RPM_ACTIVE and calls
   blk_clear_pm_only() a second time without a matching
   blk_set_pm_only().
4. q->pm_only underflows from 0 to -1 (triggering WARN_ON_ONCE in
   blk_clear_pm_only()), blocking non-PM requests in blk_queue_enter()
   and __bio_queue_enter() while the device is RPM_ACTIVE, and the next
   runtime suspend increments q->pm_only from -1 to 0, letting non-PM
   requests dispatch while RPM_SUSPENDED. This hits the UFS device
   well-known LUN by default and SCSI disks with
   manage_runtime_start_stop enabled (ATA and FireWire sbp2 defaults).

Set q->rpm_status to RPM_RESUMING in blk_pre_runtime_resume() only when
q->rpm_status is not already RPM_ACTIVE, so blk_post_runtime_resume()
skips blk_clear_pm_only() on retries.

Fixes: eef1d74bf88f ("PM: runtime: Only set runtime_error on suspend callback failures")
Cc: stable@vger.kernel.org # v7.3+
Signed-off-by: Stanley Jhu <stanleyjhu@google.com>
---
 block/blk-pm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/blk-pm.c b/block/blk-pm.c
index 8d3e052f91da..31598dd03a3f 100644
--- a/block/blk-pm.c
+++ b/block/blk-pm.c
@@ -142,7 +142,8 @@ EXPORT_SYMBOL(blk_post_runtime_suspend);
  *
  * Description:
  *    Update the queue's runtime status to RESUMING in preparation for the
- *    runtime resume of the device.
+ *    runtime resume of the device, or keep RPM_ACTIVE if a previous failed
+ *    runtime resume already reactivated the queue.
  *
  *    This function should be called near the start of the device's
  *    runtime_resume callback.
@@ -153,7 +154,9 @@ void blk_pre_runtime_resume(struct request_queue *q)
 		return;
 
 	spin_lock_irq(&q->queue_lock);
-	q->rpm_status = RPM_RESUMING;
+	/* Keep RPM_ACTIVE if a prior failed resume reactivated the queue. */
+	if (q->rpm_status != RPM_ACTIVE)
+		q->rpm_status = RPM_RESUMING;
 	spin_unlock_irq(&q->queue_lock);
 }
 EXPORT_SYMBOL(blk_pre_runtime_resume);
-- 
2.56.0.313.g2479322e68-goog

Re: [PATCH] block: Keep RPM_ACTIVE in blk_pre_runtime_resume() when already active

[Bart Van Assche]

On 10/29/26 12:08 AM, Stanley Jhu wrote:
> 2. The first runtime resume fails in the driver callback, and
>     blk_post_runtime_resume() sets q->rpm_status to RPM_ACTIVE and calls
>     blk_clear_pm_only(), decrementing q->pm_only from 1 to 0 so recovery
>     commands can enter the queue (intended since
>     commit 6e1fcab00a23 ("scsi: block: pm: Always set request queue
>     runtime active in blk_post_runtime_resume()")).

q->rpm_status should be changed back to RPM_SUSPENDED if a runtime
resume fails. The changes made my this patch don't look right to me.

Bart.

Re: [PATCH] block: Keep RPM_ACTIVE in blk_pre_runtime_resume() when already active

[Brian Norris]

Hi Bart,

On Thu, Oct 29, 2026 at 09:22:23AM -0700, Bart Van Assche wrote:
> On 10/29/26 12:08 AM, Stanley Jhu wrote:
> > 2. The first runtime resume fails in the driver callback, and
> >     blk_post_runtime_resume() sets q->rpm_status to RPM_ACTIVE and calls
> >     blk_clear_pm_only(), decrementing q->pm_only from 1 to 0 so recovery
> >     commands can enter the queue (intended since
> >     commit 6e1fcab00a23 ("scsi: block: pm: Always set request queue
> >     runtime active in blk_post_runtime_resume()")).
> 
> q->rpm_status should be changed back to RPM_SUSPENDED if a runtime
> resume fails. The changes made my this patch don't look right to me.

Pardon me if I'm misreading/misunderstanding, but I believe your
suggestion is essentially to unwind the quoted commit 6e1fcab00a23,
which might reintroduce a deadlock.

Brian

Re: [PATCH] block: Keep RPM_ACTIVE in blk_pre_runtime_resume() when already active

[Bart Van Assche]

On 11/3/26 11:49 AM, Brian Norris wrote:
> Pardon me if I'm misreading/misunderstanding, but I believe your
> suggestion is essentially to unwind the quoted commit 6e1fcab00a23,
> which might reintroduce a deadlock.

Hmm ... I think that you are right. Let me take another look at this
patch.

Bart.

Re: [PATCH] block: Keep RPM_ACTIVE in blk_pre_runtime_resume() when already active

[Bart Van Assche]

On 10/29/26 12:08 AM, Stanley Jhu wrote:
> @@ -153,7 +154,9 @@ void blk_pre_runtime_resume(struct request_queue *q)
>   		return;
>   
>   	spin_lock_irq(&q->queue_lock);
> -	q->rpm_status = RPM_RESUMING;
> +	/* Keep RPM_ACTIVE if a prior failed resume reactivated the queue. */
> +	if (q->rpm_status != RPM_ACTIVE)
> +		q->rpm_status = RPM_RESUMING;
>   	spin_unlock_irq(&q->queue_lock);
>   }
>   EXPORT_SYMBOL(blk_pre_runtime_resume);


Stanley, the analysis looks right to me. Could you make the invariant 
explicit? Only move SUSPENDED -> RESUMING in blk_pre_runtime_resume(), 
and only call blk_clear_pm_only() in blk_post_runtime_resume() if 
old_status == RPM_RESUMING. Please also describe in the commit message 
how this was tested. With those changes:

Reviewed-by: Bart Van Assche bvanassche@acm.org

Separately: since commit eef1d74bf88f, ufshcd_recover_pm_error() no 
longer sees power.runtime_error after a resume failure. Its handling of 
that case is probably dead code now.

Thanks,

Bart.