On Mon, 2018-07-30 at 09:56 +0800, jianchao.wang wrote:
> static int sdev_runtime_suspend(struct device *dev)
> {
> const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> struct scsi_device *sdev = to_scsi_device(dev);
> int err = 0;
>
> err = blk_pre_runtime_suspend(sdev->request_queue);
> if (err)
> return err;
> if (pm && pm->runtime_suspend)
> err = pm->runtime_suspend(dev);
> blk_post_runtime_suspend(sdev->request_queue, err);
>
> return err;
> }
>
> If blk_pre_runtime_suspend returns -EBUSY, blk_post_runtime_suspend will not
> be invoked.
Right, I will fix this.
> > > The request_queue should be set to preempt only mode only when we confirm
> > > we could set
> > > rpm_status to RPM_SUSPENDING or RPM_RESUMING.
> >
> > Why do you think this?
>
> https://marc.info/?l=linux-scsi&m=133727953625963&w=2
> "
> If q->rpm_status is RPM_SUSPENDED, they shouldn't do anything -- act as
> though the queue is
> empty. If q->rpm_status is RPM_SUSPENDING or RPM_RESUMING, they should hand
> over the request
> only if it has the REQ_PM flag set.
> "
I think the blk_pre_runtime_suspend() callers guarantee that q->rpm_status ==
RPM_ACTIVE
before blk_pre_runtime_suspend() is called. I will add a WARN_ON_ONCE()
statement that
verifies that.
> In additon, if we set the preempt only here unconditionally, the normal IO
> will be blocked
> during the blk_pre_runtime_suspend. In your patch, q_usage_counter will be
> switched to atomic mode,
> this could cost some time. Is it really OK ?
I will see what I can do about this.
Bart.
