On 2020-06-29 09:15, Alan Stern wrote:
> Aha. Looking at this more closely, it's apparent that the code in
> blk-core.c contains a logic bug: It assumes that if the BLK_MQ_REQ_PREEMPT
> flag is set then the request can be issued regardless of the queue's
> runtime status. That is not correct when the queue is suspended.
Please clarify why this is not correct.
> Index: usb-devel/block/blk-core.c
> ===================================================================
> --- usb-devel.orig/block/blk-core.c
> +++ usb-devel/block/blk-core.c
> @@ -423,7 +423,8 @@ int blk_queue_enter(struct request_queue
> * responsible for ensuring that that counter is
> * globally visible before the queue is unfrozen.
> */
> - if (pm || !blk_queue_pm_only(q)) {
> + if ((pm && q->rpm_status != RPM_SUSPENDED) ||
> + !blk_queue_pm_only(q)) {
> success = true;
> } else {
> percpu_ref_put(&q->q_usage_counter);
Does the above change make it impossible to bring a suspended device
back to the RPM_ACTIVE state if the BLK_MQ_REQ_NOWAIT flag is set?
> @@ -448,8 +449,7 @@ int blk_queue_enter(struct request_queue
>
> wait_event(q->mq_freeze_wq,
> (!q->mq_freeze_depth &&
> - (pm || (blk_pm_request_resume(q),
> - !blk_queue_pm_only(q)))) ||
> + blk_pm_resume_queue(pm, q)) ||
> blk_queue_dying(q));
> if (blk_queue_dying(q))
> return -ENODEV;
> Index: usb-devel/block/blk-pm.h
> ===================================================================
> --- usb-devel.orig/block/blk-pm.h
> +++ usb-devel/block/blk-pm.h
> @@ -6,11 +6,14 @@
> #include <linux/pm_runtime.h>
>
> #ifdef CONFIG_PM
> -static inline void blk_pm_request_resume(struct request_queue *q)
> +static inline int blk_pm_resume_queue(const bool pm, struct request_queue *q)
> {
> - if (q->dev && (q->rpm_status == RPM_SUSPENDED ||
> - q->rpm_status == RPM_SUSPENDING))
> - pm_request_resume(q->dev);
> + if (!q->dev || !blk_queue_pm_only(q))
> + return 1; /* Nothing to do */
> + if (pm && q->rpm_status != RPM_SUSPENDED)
> + return 1; /* Request allowed */
> + pm_request_resume(q->dev);
> + return 0;
> }
Does the above change, especially the " && q->rpm_status !=
RPM_SUSPENDED" part, make it impossible to bring a suspended device back
to the RPM_ACTIVE state?
Thanks,
Bart.
