On Fri, 2018-08-10 at 15:27 +0000, Bart Van Assche wrote:
> On Fri, 2018-08-10 at 10:39 +0800, jianchao.wang wrote:
> > On 08/10/2018 03:41 AM, Bart Van Assche wrote:
> > > +void blk_pm_runtime_exit(struct request_queue *q)
> > > +{
> > > + if (!q->dev)
> > > + return;
> > > +
> > > + pm_runtime_get_sync(q->dev);
> > > + q->dev = NULL;
> > > +}
> > > +EXPORT_SYMBOL(blk_pm_runtime_exit);
> > > +
> > > /**
> > > * blk_pre_runtime_suspend - Pre runtime suspend check
> > > * @q: the queue of the device
> > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > > index 69ab459abb98..5537762dfcfd 100644
> > > --- a/drivers/scsi/sd.c
> > > +++ b/drivers/scsi/sd.c
> > > @@ -3420,12 +3420,11 @@ static int sd_probe(struct device *dev)
> > > **/
> > > static int sd_remove(struct device *dev)
> > > {
> > > - struct scsi_disk *sdkp;
> > > - dev_t devt;
> > > + struct scsi_disk *sdkp = dev_get_drvdata(dev);
> > > + struct scsi_device *sdp = sdkp->device;
> > > + dev_t devt = disk_devt(sdkp->disk);
> > >
> > > - sdkp = dev_get_drvdata(dev);
> > > - devt = disk_devt(sdkp->disk);
> > > - scsi_autopm_get_device(sdkp->device);
> > > + blk_pm_runtime_exit(sdp->request_queue)
> >
> >
> > Based on __scsi_device_remove, sd_remove will be invoked before
> > blk_cleanup_queue.
> > So it should be not safe that we set the q->dev to NULL here.
> >
> > We have following operations in followed patch:
> > +static inline void blk_pm_mark_last_busy(struct request *rq)
> > +{
> > + if (rq->q->dev && !(rq->rq_flags & RQF_PM))
> > + pm_runtime_mark_last_busy(rq->q->dev);
> > +}
>
> How about moving the blk_pm_runtime_exit() call into blk_cleanup_queue() at
> a point where it is sure that there are no requests in progress? That should
> avoid that blk_pm_mark_last_busy() races against blk_pm_runtime_exit().
That wouldn't work because unbinding the SCSI ULD doesn't remove the request
queue. How about changing blk_pm_runtime_exit() into something like the
following?
void blk_pm_runtime_exit(struct request_queue *q)
{
if (!q->dev)
return;
pm_runtime_get_sync(q->dev);
blk_freeze_queue(q);
q->dev = NULL;
blk_unfreeze_queue(q);
}
Bart.
