Hi Bart,
On Sun, 2020-08-23 at 20:06 -0700, Bart Van Assche wrote:
> With the current implementation the following race can happen:
> * blk_pre_runtime_suspend() calls blk_freeze_queue_start() and
> blk_mq_unfreeze_queue().
> * blk_queue_enter() calls blk_queue_pm_only() and that function returns
> true.
> * blk_queue_enter() calls blk_pm_request_resume() and that function does
> not call pm_request_resume() because the queue runtime status is
> RPM_ACTIVE.
> * blk_pre_runtime_suspend() changes the queue status into RPM_SUSPENDING.
>
> Fix this race by changing the queue runtime status into RPM_SUSPENDING
> before switching q_usage_counter to atomic mode.
>
> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Cc: Stanley Chu <stanley.chu@xxxxxxxxxxxx>
> Cc: Ming Lei <ming.lei@xxxxxxxxxx>
> Cc: stable <stable@xxxxxxxxxxxxxxx>
> Fixes: 986d413b7c15 ("blk-mq: Enable support for runtime power management")
> Signed-off-by: Can Guo <cang@xxxxxxxxxxxxxx>
> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
> ---
> block/blk-pm.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/block/blk-pm.c b/block/blk-pm.c
> index b85234d758f7..17bd020268d4 100644
> --- a/block/blk-pm.c
> +++ b/block/blk-pm.c
> @@ -67,6 +67,10 @@ int blk_pre_runtime_suspend(struct request_queue *q)
>
> WARN_ON_ONCE(q->rpm_status != RPM_ACTIVE);
>
> + spin_lock_irq(&q->queue_lock);
> + q->rpm_status = RPM_SUSPENDING;
> + spin_unlock_irq(&q->queue_lock);
> +
Has below alternative way been considered that RPM_SUSPENDING is set
after blk_mq_unfreeze_queue()?
blk_freeze_queue_start(q);
+ spin_lock_irq(&q->queue_lock);
+ q->rpm_status = RPM_SUSPENDING;
+ spin_unlock_irq(&q->queue_lock);
Otherwise requests can enter queue while rpm_status is RPM_SUSPENDING
during a small window, i.e., before blk_set_pm_only() is invoked. This
would make the definition of rpm_status ambiguous.
In this way, the racing could be also solved:
- Before blk_freeze_queue_start(), any requests shall be allowed to
enter queue
- blk_freeze_queue_start() freezes the queue and blocks all upcoming
requests (make them wait_event(q->mq_freeze_wq))
- rpm_status is set as RPM_SUSPENDING
- blk_mq_unfreeze_queue() wakes up q->mq_freeze_wq and then
blk_pm_request_resume() can be executed
Thanks,
Stanley Chu
> /*
> * Increase the pm_only counter before checking whether any
> * non-PM blk_queue_enter() calls are in progress to avoid that any
> @@ -89,15 +93,14 @@ int blk_pre_runtime_suspend(struct request_queue *q)
> /* Switch q_usage_counter back to per-cpu mode. */
> blk_mq_unfreeze_queue(q);
>
> - spin_lock_irq(&q->queue_lock);
> - if (ret < 0)
> + if (ret < 0) {
> + spin_lock_irq(&q->queue_lock);
> + q->rpm_status = RPM_ACTIVE;
> pm_runtime_mark_last_busy(q->dev);
> - else
> - q->rpm_status = RPM_SUSPENDING;
> - spin_unlock_irq(&q->queue_lock);
> + spin_unlock_irq(&q->queue_lock);
>
> - if (ret)
> blk_clear_pm_only(q);
> + }
>
> return ret;
> }
