On Wed, Nov 26, 2025 at 11:47 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 11/26/25 1:30 PM, Rafael J. Wysocki wrote:
> > On Wed, Nov 26, 2025 at 10:11 PM Bart Van Assche <bvanassche@acm.org> wrote:
> >>
> >> On 11/26/25 12:17 PM, Rafael J. Wysocki wrote:
> >>> --- a/block/blk-core.c
> >>> +++ b/block/blk-core.c
> >>> @@ -309,6 +309,8 @@ int blk_queue_enter(struct request_queue
> >>>                if (flags & BLK_MQ_REQ_NOWAIT)
> >>>                        return -EAGAIN;
> >>>
> >>> +             /* if necessary, resume .dev (assume success). */
> >>> +             blk_pm_resume_queue(pm, q);
> >>>                /*
> >>>                 * read pair of barrier in blk_freeze_queue_start(), we need to
> >>>                 * order reading __PERCPU_REF_DEAD flag of .q_usage_counter and
> >>
> >> blk_queue_enter() may be called from the suspend path so I don't think
> >> that the above change will work.
> >
> > Why would the existing code work then?
>
> The existing code works reliably on a very large number of devices.

Well, except that it doesn't work during system suspend and
hibernation when the PM workqueue is frozen.  I think that we agree
here.

This needs to be addressed because it may very well cause system
suspend to deadlock.

There are two possible ways to address it I can think of:

1. Changing blk_pm_resume_queue() and its users to carry out a
synchronous resume of q->dev instead of calling pm_request_resume()
and (effectively) waiting for the queued-up runtime resume of q->dev
to take effect.

This would be my preferred option, but at this point I'm not sure if
it's viable.

2. Stop freezing the PM workqueue before system suspend/hibernation
and adapt device_suspend_late() to that.

This should be doable, even though it is a bit risky because it may
uncover some latent bugs (the freezing of the PM workqueue has been
there forever), but it wouldn't address the problem entirely because
device_suspend_late() would still need to disable runtime PM for the
device (and for some devices it is disabled earlier), so
pm_request_resume() would just start to fail at that point and if
blk_queue_enter() were called after that point for a device supporting
runtime PM, it might deadlock.

> Maybe there is a misunderstanding? RQF_PM / BLK_MQ_REQ_PM are set for
> requests that should be processed even if the power status is changing
> (RPM_SUSPENDING or RPM_RESUMING). The meaning of the 'pm' variable is
> as follows: process this request even if a power state change is
> ongoing.

I see.

The behavior depends on whether or not q->pm_only is set.  If it is
not set, both blk_queue_enter() and __bio_queue_enter() will allow the
request to be processed.

If q->pm_only is set, __bio_queue_enter() will wait until it gets
cleared and in that case pm_request_resume(q->dev) is called to make
that happen (did I get it right?).  This is a bit fragile because what
if the async resume of q->dev fails for some reason?  You deadlock
instead of failing the request.

Unlike __bio_queue_enter(), blk_queue_enter() additionally checks the
runtime PM status of the queue if q->pm_only is set and it will allow
the request to be processed in that case so long as q->rpm_status is
not RPM_SUSPENDED.  However, if the queue status is RPM_SUSPENDED,
pm_request_resume(q->dev) will be called like in the
__bio_queue_enter() case.

I'm not sure why pm_request_resume(q->dev) needs to be called from
within blk_pm_resume_queue().  Arguably, it should be sufficient to
call it once before using the wait_event() macro, if the conditions
checked by blk_pm_resume_queue() are not met.

> > Are you suggesting that q->rpm_status should still be checked before
> > calling pm_runtime_resume() or do you mean something else?
> The purpose of the code changes from a previous email is not entirely
> clear to me so I'm not sure what the code should look like. But to
> answer your question, calling blk_pm_resume_queue() if the runtime
> status is RPM_SUSPENDED should be safe.
> >> As an example, the UFS driver submits a
> >> SCSI START STOP UNIT command from its runtime suspend callback. The call
> >> chain is as follows:
> >>
> >>     ufshcd_wl_runtime_suspend()
> >>       __ufshcd_wl_suspend()
> >>         ufshcd_set_dev_pwr_mode()
> >>           ufshcd_execute_start_stop()
> >>             scsi_execute_cmd()
> >>               scsi_alloc_request()
> >>                 blk_queue_enter()
> >>               blk_execute_rq()
> >>               blk_mq_free_request()
> >>                 blk_queue_exit()
> >
> > In any case, calling pm_request_resume() from blk_pm_resume_queue() in
> > the !pm case is a mistake.
>   Hmm ... we may disagree about this. Does what I wrote above make clear
> why blk_pm_resume_queue() is called if pm == false?

Yes, it does, thanks!