scsi: sr: use block layer runtime PM

PATCH Superseded Aaron Lu 3 participants 12 messages Sep 5, 2013 – Oct 28, 2013 SCSI

Share & tools

Aaron Lu

author

2013-09-05 05:51 UTC #0 lore raw

PATCH 1/1 scsi: sr: use block layer runtime PM

Migrate SCSI Optical Disk Drive(ODD) driver sr to make use of block layer runtime PM. Accordingly, the SCSI bus layer runtime PM callback is simplified as all SCSI devices that implement runtime PM are now request based.

Signed-off-by Aaron Lu <[email protected]>

---
Note that due to ODD will be polled every 2 seconds, for suspend to actually happen, the autosuspend_delay can not be set to more than 2 seconds.

Also, make sure to use the util-linux utility of version 2.23.2 or later, as there is a bug fix for eject command to correctly update the ODD's locked state. If the fix is not applied, after the ODD is ejected with the eject command, it will not be able to enter runtime suspend state any more due to SCSI EH code will submit a lock door command for the ODD right after its parent ATA port is runtime suspended.
https://git.kernel.org/cgit/utils/util-linux/util-linux.git/commit/?id=12272030e563201e0b06732d8a87d8cea7157a04

 drivers/scsi/scsi_pm.c | 58 ++++++++++++--------------------------------------
 drivers/scsi/sr.c      | 37 +++++++++++---------------------
 2 files changed, 27 insertions(+), 68 deletions(-)

drivers/scsi/scsi_pm.c +14 ~~-44~~

index 4c5aabe..752edf0 100644
@@ -144,38 +144,22 @@ static int scsi_bus_restore(struct device *dev)
 
 #ifdef CONFIG_PM_RUNTIME
 
-static int sdev_blk_runtime_suspend(struct scsi_device *sdev,
-					int (*cb)(struct device *))
+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;
 
 	err = blk_pre_runtime_suspend(sdev->request_queue);
 	if (err)
 		return err;
-	if (cb)
-		err = cb(&sdev->sdev_gendev);
+	if (pm && pm->runtime_suspend)
+		err = pm->runtime_suspend(dev);
 	blk_post_runtime_suspend(sdev->request_queue, err);
 
 	return err;
 }
 
-static int sdev_runtime_suspend(struct device *dev)
-{
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-	int (*cb)(struct device *) = pm ? pm->runtime_suspend : NULL;
-	struct scsi_device *sdev = to_scsi_device(dev);
-	int err;
-
-	if (sdev->request_queue->dev)
-		return sdev_blk_runtime_suspend(sdev, cb);
-
-	err = scsi_dev_type_suspend(dev, cb);
-	if (err == -EAGAIN)
-		pm_schedule_suspend(dev, jiffies_to_msecs(
-					round_jiffies_up_relative(HZ/10)));
-	return err;
-}
-
 static int scsi_runtime_suspend(struct device *dev)
 {
 	int err = 0;

@@ -189,31 +173,20 @@ static int scsi_runtime_suspend(struct device *dev)
 	return err;
 }
 
-static int sdev_blk_runtime_resume(struct scsi_device *sdev,
-					int (*cb)(struct device *))
+static int sdev_runtime_resume(struct device *dev)
 {
+	struct scsi_device *sdev = to_scsi_device(dev);
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int err = 0;
 
 	blk_pre_runtime_resume(sdev->request_queue);
-	if (cb)
-		err = cb(&sdev->sdev_gendev);
+	if (pm && pm->runtime_resume)
+		err = pm->runtime_resume(dev);
 	blk_post_runtime_resume(sdev->request_queue, err);
 
 	return err;
 }
 
-static int sdev_runtime_resume(struct device *dev)
-{
-	struct scsi_device *sdev = to_scsi_device(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-	int (*cb)(struct device *) = pm ? pm->runtime_resume : NULL;
-
-	if (sdev->request_queue->dev)
-		return sdev_blk_runtime_resume(sdev, cb);
-	else
-		return scsi_dev_type_resume(dev, cb);
-}
-
 static int scsi_runtime_resume(struct device *dev)
 {
 	int err = 0;

@@ -234,14 +207,11 @@ static int scsi_runtime_idle(struct device *dev)
 	/* Insert hooks here for targets, hosts, and transport classes */
 
 	if (scsi_is_sdev_device(dev)) {
-		struct scsi_device *sdev = to_scsi_device(dev);
-
-		if (sdev->request_queue->dev) {
-			pm_runtime_mark_last_busy(dev);
-			pm_runtime_autosuspend(dev);
-			return -EBUSY;
-		}
+		pm_runtime_mark_last_busy(dev);
+		pm_runtime_autosuspend(dev);
+		return -EBUSY;
 	}
+
 	return 0;
 }

drivers/scsi/sr.c +13 ~~-24~~

index 119d67f..40d8592 100644
@@ -161,14 +161,10 @@ static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
 		goto out;
 	cd = scsi_cd(disk);
 	kref_get(&cd->kref);
-	if (scsi_device_get(cd->device))
-		goto out_put;
-	if (!scsi_autopm_get_device(cd->device))
-		goto out;
-
- out_put:
-	kref_put(&cd->kref, sr_kref_release);
-	cd = NULL;
+	if (scsi_device_get(cd->device)) {
+		kref_put(&cd->kref, sr_kref_release);
+		cd = NULL;
+	}
  out:
 	mutex_unlock(&sr_ref_mutex);
 	return cd;

@@ -180,7 +176,6 @@ static void scsi_cd_put(struct scsi_cd *cd)
 
 	mutex_lock(&sr_ref_mutex);
 	kref_put(&cd->kref, sr_kref_release);
-	scsi_autopm_put_device(sdev);
 	scsi_device_put(sdev);
 	mutex_unlock(&sr_ref_mutex);
 }

@@ -558,8 +553,6 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 	void __user *argp = (void __user *)arg;
 	int ret;
 
-	scsi_autopm_get_device(cd->device);
-
 	mutex_lock(&sr_mutex);
 
 	/*

@@ -591,7 +584,6 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 
 out:
 	mutex_unlock(&sr_mutex);
-	scsi_autopm_put_device(cd->device);
 	return ret;
 }

@@ -599,17 +591,11 @@ static unsigned int sr_block_check_events(struct gendisk *disk,
 					  unsigned int clearing)
 {
 	struct scsi_cd *cd = scsi_cd(disk);
-	unsigned int ret;
 
-	if (atomic_read(&cd->device->disk_events_disable_depth) == 0) {
-		scsi_autopm_get_device(cd->device);
-		ret = cdrom_check_events(&cd->cdi, clearing);
-		scsi_autopm_put_device(cd->device);
-	} else {
-		ret = 0;
-	}
+	if (atomic_read(&cd->device->disk_events_disable_depth))
+		return 0;
 
-	return ret;
+	return cdrom_check_events(&cd->cdi, clearing);
 }
 
 static int sr_block_revalidate_disk(struct gendisk *disk)

@@ -617,8 +603,6 @@ static int sr_block_revalidate_disk(struct gendisk *disk)
 	struct scsi_cd *cd = scsi_cd(disk);
 	struct scsi_sense_hdr sshdr;
 
-	scsi_autopm_get_device(cd->device);
-
 	/* if the unit is not ready, nothing more to do */
 	if (scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr))
 		goto out;

@@ -626,7 +610,6 @@ static int sr_block_revalidate_disk(struct gendisk *disk)
 	sr_cd_check(&cd->cdi);
 	get_sectorsize(cd);
 out:
-	scsi_autopm_put_device(cd->device);
 	return 0;
 }

@@ -747,6 +730,12 @@ static int sr_probe(struct device *dev)
 	if (register_cdrom(&cd->cdi))
 		goto fail_put;
 
+	/*
+	 * Initialize block layer runtime PM stuffs before the
+	 * periodic event checking request gets started in add_disk.
+	 */
+	blk_pm_runtime_init(sdev->request_queue, dev);
+
 	dev_set_drvdata(dev, cd);
 	disk->flags |= GENHD_FL_REMOVABLE;
 	add_disk(disk);

Signature · 1 line

1.8.3.2.10.g43d11f4

Alan Stern

2013-09-05 14:25 UTC #1 lore raw

Quoted 10 lines · Aaron Lu

Migrate SCSI Optical Disk Drive(ODD) driver sr to make use of block layer runtime PM. Accordingly, the SCSI bus layer runtime PM callback is simplified as all SCSI devices that implement runtime PM are now request based.
Signed-off-by Aaron Lu <[email protected]>
---
Note that due to ODD will be polled every 2 seconds, for suspend to actually happen, the autosuspend_delay can not be set to more than 2 seconds.

Or the polling delay must be increased.

Quoted 7 lines

Also, make sure to use the util-linux utility of version 2.23.2 or later, as there is a bug fix for eject command to correctly update the ODD's locked state. If the fix is not applied, after the ODD is ejected with the eject command, it will not be able to enter runtime suspend state any more due to SCSI EH code will submit a lock door command for the ODD right after its parent ATA port is runtime suspended.
https://git.kernel.org/cgit/utils/util-linux/util-linux.git/commit/?id=12272030e563201e0b06732d8a87d8cea7157a04

I think this patch will cause a compile warning if you have CONFIG_PM_RUNTIME set and CONFIG_PM_SLEEP disabled. The scsi_dev_type_suspend and scsi_dev_type_resume routines will be defined but not used.

Otherwise it seems okay.

Alan Stern

Aaron Lu

author

2013-09-06 01:35 UTC #2 lore raw

Quoted 23 lines · Alan Stern

Quoted 10 lines · Aaron Lu
Migrate SCSI Optical Disk Drive(ODD) driver sr to make use of block layer runtime PM. Accordingly, the SCSI bus layer runtime PM callback is simplified as all SCSI devices that implement runtime PM are now request based.
Signed-off-by Aaron Lu <[email protected]>
---
Note that due to ODD will be polled every 2 seconds, for suspend to actually happen, the autosuspend_delay can not be set to more than 2 seconds.
Or the polling delay must be increased.
Quoted 7 lines
Also, make sure to use the util-linux utility of version 2.23.2 or later, as there is a bug fix for eject command to correctly update the ODD's locked state. If the fix is not applied, after the ODD is ejected with the eject command, it will not be able to enter runtime suspend state any more due to SCSI EH code will submit a lock door command for the ODD right after its parent ATA port is runtime suspended.
https://git.kernel.org/cgit/utils/util-linux/util-linux.git/commit/?id=12272030e563201e0b06732d8a87d8cea7157a04

Quoted 4 lines

I think this patch will cause a compile warning if you have CONFIG_PM_RUNTIME set and CONFIG_PM_SLEEP disabled. The scsi_dev_type_suspend and scsi_dev_type_resume routines will be defined but not used.

Oh, right, thanks for the remind. I'll move CONFIG_PM_SLEEP up to cover scsi_dev_type_suspend/resume in v2.

Quoted 3 lines

Otherwise it seems okay.

Thanks,
Aaron

Aaron Lu

author

2013-09-06 02:00 UTC #3 lore raw

PATCH v2 1/1 scsi: sr: use block layer runtime PM

In reply to Alan Stern

Note that due to ODD will be polled every 2 seconds, for suspend to actually happen, the autosuspend_delay can not be set to more than 2 seconds or the polling interval has to be increased.

Signed-off-by Aaron Lu <[email protected]>

---
v2: scsi_dev_type_suspend/resume should be covered by CONFIG_PM_SLEEP, suggested by Alan Stern.

Make sure to use the util-linux utility of version 2.23.2 or later if a ATA host is in use, as there is a bug fix for eject command to correctly update the ODD's locked state. If the fix is not applied, after the ODD is ejected with the eject command, it will not be able to enter runtime suspend state any more due to SCSI EH code will submit a lock door command for the ODD right after its parent ATA port is runtime suspended.
https://git.kernel.org/cgit/utils/util-linux/util-linux.git/commit/?id=12272030e563201e0b06732d8a87d8cea7157a04

 drivers/scsi/scsi_pm.c | 62 +++++++++++++-------------------------------------
 drivers/scsi/sr.c      | 37 +++++++++++-------------------
 2 files changed, 29 insertions(+), 70 deletions(-)

drivers/scsi/scsi_pm.c +16 ~~-46~~

index 4c5aabe..bb31fc9 100644
@@ -16,6 +16,8 @@
 
 #include "scsi_priv.h"
 
+#ifdef CONFIG_PM_SLEEP
+
 static int scsi_dev_type_suspend(struct device *dev, int (*cb)(struct device *))
 {
 	int err;

@@ -43,8 +45,6 @@ static int scsi_dev_type_resume(struct device *dev, int (*cb)(struct device *))
 	return err;
 }
 
-#ifdef CONFIG_PM_SLEEP
-
 static int
 scsi_bus_suspend_common(struct device *dev, int (*cb)(struct device *))
 {

@@ -144,38 +144,22 @@ static int scsi_bus_restore(struct device *dev)
 
 #ifdef CONFIG_PM_RUNTIME
 
-static int sdev_blk_runtime_suspend(struct scsi_device *sdev,
-					int (*cb)(struct device *))
+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;
 
 	err = blk_pre_runtime_suspend(sdev->request_queue);
 	if (err)
 		return err;
-	if (cb)
-		err = cb(&sdev->sdev_gendev);
+	if (pm && pm->runtime_suspend)
+		err = pm->runtime_suspend(dev);
 	blk_post_runtime_suspend(sdev->request_queue, err);
 
 	return err;
 }
 
-static int sdev_runtime_suspend(struct device *dev)
-{
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-	int (*cb)(struct device *) = pm ? pm->runtime_suspend : NULL;
-	struct scsi_device *sdev = to_scsi_device(dev);
-	int err;
-
-	if (sdev->request_queue->dev)
-		return sdev_blk_runtime_suspend(sdev, cb);
-
-	err = scsi_dev_type_suspend(dev, cb);
-	if (err == -EAGAIN)
-		pm_schedule_suspend(dev, jiffies_to_msecs(
-					round_jiffies_up_relative(HZ/10)));
-	return err;
-}
-
 static int scsi_runtime_suspend(struct device *dev)
 {
 	int err = 0;

@@ -189,31 +173,20 @@ static int scsi_runtime_suspend(struct device *dev)
 	return err;
 }
 
-static int sdev_blk_runtime_resume(struct scsi_device *sdev,
-					int (*cb)(struct device *))
+static int sdev_runtime_resume(struct device *dev)
 {
+	struct scsi_device *sdev = to_scsi_device(dev);
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int err = 0;
 
 	blk_pre_runtime_resume(sdev->request_queue);
-	if (cb)
-		err = cb(&sdev->sdev_gendev);
+	if (pm && pm->runtime_resume)
+		err = pm->runtime_resume(dev);
 	blk_post_runtime_resume(sdev->request_queue, err);
 
 	return err;
 }
 
-static int sdev_runtime_resume(struct device *dev)
-{
-	struct scsi_device *sdev = to_scsi_device(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-	int (*cb)(struct device *) = pm ? pm->runtime_resume : NULL;
-
-	if (sdev->request_queue->dev)
-		return sdev_blk_runtime_resume(sdev, cb);
-	else
-		return scsi_dev_type_resume(dev, cb);
-}
-
 static int scsi_runtime_resume(struct device *dev)
 {
 	int err = 0;

@@ -234,14 +207,11 @@ static int scsi_runtime_idle(struct device *dev)
 	/* Insert hooks here for targets, hosts, and transport classes */
 
 	if (scsi_is_sdev_device(dev)) {
-		struct scsi_device *sdev = to_scsi_device(dev);
-
-		if (sdev->request_queue->dev) {
-			pm_runtime_mark_last_busy(dev);
-			pm_runtime_autosuspend(dev);
-			return -EBUSY;
-		}
+		pm_runtime_mark_last_busy(dev);
+		pm_runtime_autosuspend(dev);
+		return -EBUSY;
 	}
+
 	return 0;
 }

drivers/scsi/sr.c +13 ~~-24~~

index 119d67f..40d8592 100644
@@ -161,14 +161,10 @@ static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
 		goto out;
 	cd = scsi_cd(disk);
 	kref_get(&cd->kref);
-	if (scsi_device_get(cd->device))
-		goto out_put;
-	if (!scsi_autopm_get_device(cd->device))
-		goto out;
-
- out_put:
-	kref_put(&cd->kref, sr_kref_release);
-	cd = NULL;
+	if (scsi_device_get(cd->device)) {
+		kref_put(&cd->kref, sr_kref_release);
+		cd = NULL;
+	}
  out:
 	mutex_unlock(&sr_ref_mutex);
 	return cd;

@@ -180,7 +176,6 @@ static void scsi_cd_put(struct scsi_cd *cd)
 
 	mutex_lock(&sr_ref_mutex);
 	kref_put(&cd->kref, sr_kref_release);
-	scsi_autopm_put_device(sdev);
 	scsi_device_put(sdev);
 	mutex_unlock(&sr_ref_mutex);
 }

@@ -558,8 +553,6 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 	void __user *argp = (void __user *)arg;
 	int ret;
 
-	scsi_autopm_get_device(cd->device);
-
 	mutex_lock(&sr_mutex);
 
 	/*

@@ -591,7 +584,6 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 
 out:
 	mutex_unlock(&sr_mutex);
-	scsi_autopm_put_device(cd->device);
 	return ret;
 }

@@ -599,17 +591,11 @@ static unsigned int sr_block_check_events(struct gendisk *disk,
 					  unsigned int clearing)
 {
 	struct scsi_cd *cd = scsi_cd(disk);
-	unsigned int ret;
 
-	if (atomic_read(&cd->device->disk_events_disable_depth) == 0) {
-		scsi_autopm_get_device(cd->device);
-		ret = cdrom_check_events(&cd->cdi, clearing);
-		scsi_autopm_put_device(cd->device);
-	} else {
-		ret = 0;
-	}
+	if (atomic_read(&cd->device->disk_events_disable_depth))
+		return 0;
 
-	return ret;
+	return cdrom_check_events(&cd->cdi, clearing);
 }
 
 static int sr_block_revalidate_disk(struct gendisk *disk)

@@ -617,8 +603,6 @@ static int sr_block_revalidate_disk(struct gendisk *disk)
 	struct scsi_cd *cd = scsi_cd(disk);
 	struct scsi_sense_hdr sshdr;
 
-	scsi_autopm_get_device(cd->device);
-
 	/* if the unit is not ready, nothing more to do */
 	if (scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr))
 		goto out;

@@ -626,7 +610,6 @@ static int sr_block_revalidate_disk(struct gendisk *disk)
 	sr_cd_check(&cd->cdi);
 	get_sectorsize(cd);
 out:
-	scsi_autopm_put_device(cd->device);
 	return 0;
 }

@@ -747,6 +730,12 @@ static int sr_probe(struct device *dev)
 	if (register_cdrom(&cd->cdi))
 		goto fail_put;
 
+	/*
+	 * Initialize block layer runtime PM stuffs before the
+	 * periodic event checking request gets started in add_disk.
+	 */
+	blk_pm_runtime_init(sdev->request_queue, dev);
+
 	dev_set_drvdata(dev, cd);
 	disk->flags |= GENHD_FL_REMOVABLE;
 	add_disk(disk);

Signature · 1 line

1.8.3.2.10.g43d11f4

Alan Stern

Acked-by on v2 1/1

2013-09-06 14:26 UTC #4 lore raw

Quoted 10 lines · Aaron Lu

Migrate SCSI Optical Disk Drive(ODD) driver sr to make use of block layer runtime PM. Accordingly, the SCSI bus layer runtime PM callback is simplified as all SCSI devices that implement runtime PM are now request based.
Note that due to ODD will be polled every 2 seconds, for suspend to actually happen, the autosuspend_delay can not be set to more than 2 seconds or the polling interval has to be increased.
Signed-off-by Aaron Lu <[email protected]>

Acked-by Alan Stern <[email protected]>

James Bottomley

maintainer

2013-09-06 15:00 UTC #5 lore raw

In reply to Aaron Lu

Quoted 1 line · Aaron Lu

Migrate SCSI Optical Disk Drive(ODD)

I'm not very keen on this description because it's not quite accurate. sr stands for SCSI ROM. You could say optical SCSI ROM perhaps, but Magneto Optical disks are handled by sd not sr.

Quoted 1 line

driver sr to make use of block

I'd just say "Migrate sr to make use of .."

Quoted 3 lines

layer runtime PM. Accordingly, the SCSI bus layer runtime PM callback is simplified as all SCSI devices that implement runtime PM are now request based.

OK, let's now try for a descriptive changelog. All SCSI devices (including sr) are "request based". I think what you mean is "all SCSI devices which implement runtime PM have an exposed block device"?

Quoted 1 line

Note that due to ODD will be polled every 2 seconds,

Since the device will be polled every 2 seconds

Quoted 3 lines

for suspend to
actually happen, the autosuspend_delay can not be set to more than 2 seconds or the polling interval has to be increased.

Is this true? What about event driven devices? Supposing a distro has a different interval.

How about

If your Distribution polls the device, the autosuspend interval cannot be set to longer than the polling interval otherwise the device will never suspend.

James

Alan Stern

2013-09-06 15:56 UTC #6 lore raw

Quoted 7 lines · James Bottomley

Quoted 3 lines
layer runtime PM. Accordingly, the SCSI bus layer runtime PM callback is simplified as all SCSI devices that implement runtime PM are now request based.
OK, let's now try for a descriptive changelog. All SCSI devices (including sr) are "request based". I think what you mean is "all SCSI devices which implement runtime PM have an exposed block device"?

Actually Aaron means "All SCSI drivers implementing runtime PM now use the block layer's request-based mechanism."

Quoted 8 lines

Quoted 1 line
Note that due to ODD will be polled every 2 seconds,
Since the device will be polled every 2 seconds
Quoted 3 lines
for suspend to
actually happen, the autosuspend_delay can not be set to more than 2 seconds or the polling interval has to be increased.

Quoted 8 lines

Is this true? What about event driven devices? Supposing a distro has a different interval.
How about
If your Distribution polls the device, the autosuspend interval cannot be set to longer than the polling interval otherwise the device will never suspend.

The default polling done by the kernel uses 2-second intervals. Of course, distributions and users can change this or disable it entirely.

Alan Stern

Aaron Lu

author

2013-09-07 14:45 UTC #9 lore raw

In reply to James Bottomley

Quoted 10 lines · James Bottomley

Quoted 1 line · Aaron Lu
Migrate SCSI Optical Disk Drive(ODD)
I'm not very keen on this description because it's not quite accurate. sr stands for SCSI ROM. You could say optical SCSI ROM perhaps, but Magneto Optical disks are handled by sd not sr.
Quoted 1 line
driver sr to make use of block
I'd just say "Migrate sr to make use of .."

OK, will update in v3, thanks for the correction.

Thanks,
Aaron

Aaron Lu

author

2013-09-07 15:05 UTC #7 lore raw

In reply to Alan Stern

Quoted 6 lines · Alan Stern

Quoted 4 lines · James Bottomley
Quoted 3 lines
layer runtime PM. Accordingly, the SCSI bus layer runtime PM callback is simplified as all SCSI devices that implement runtime PM are now request based.

Quoted 6 lines

Quoted 3 lines
OK, let's now try for a descriptive changelog. All SCSI devices (including sr) are "request based". I think what you mean is "all SCSI devices which implement runtime PM have an exposed block device"?
Actually Aaron means "All SCSI drivers implementing runtime PM now use the block layer's request-based mechanism."

Exactly, thanks for the clarification Alan. I'll update changlog using the above words in v3.

Quoted 12 lines

Quoted 12 lines
Quoted 2 lines
Note that due to ODD will be polled every 2 seconds,
Since the device will be polled every 2 seconds
Quoted 3 lines
for suspend to
actually happen, the autosuspend_delay can not be set to more than 2 seconds or the polling interval has to be increased.
Is this true? What about event driven devices? Supposing a distro has a different interval.

Quoted 8 lines

Quoted 5 lines
How about
If your Distribution polls the device, the autosuspend interval cannot be set to longer than the polling interval otherwise the device will never suspend.
The default polling done by the kernel uses 2-second intervals. Of course, distributions and users can change this or disable it entirely.

What about:
Note that due to the device will be polled by kernel at a constant interval specified at /sys/module/block/parameters/events_dfl_poll_msecs, the default value is 2 seconds, the autosuspend delay cannot be set to longer than the polling interval otherwise the device will never suspend.

Thanks,
Aaron

Alan Stern

2013-09-07 15:22 UTC #8 lore raw

Quoted 17 lines · Aaron Lu

Quoted 17 lines
Quoted 16 lines
Quoted 1 line
Note that due to ODD will be polled every 2 seconds,
Since the device will be polled every 2 seconds
Quoted 3 lines
for suspend to
actually happen, the autosuspend_delay can not be set to more than 2 seconds or the polling interval has to be increased.
Is this true? What about event driven devices? Supposing a distro has a different interval.
How about
If your Distribution polls the device, the autosuspend interval cannot be set to longer than the polling interval otherwise the device will never suspend.

Quoted 8 lines

Quoted 2 lines
The default polling done by the kernel uses 2-second intervals. Of course, distributions and users can change this or disable it entirely.
What about:
Note that due to the device will be polled by kernel at a constant interval specified at /sys/module/block/parameters/events_dfl_poll_msecs, the default value is 2 seconds, the autosuspend delay cannot be set to longer than the polling interval otherwise the device will never suspend.

Well, I wouldn't say "cannot" -- people can set the delay to whatever they want. Just: If the autosuspend delay is set longer than the polling interval then the device will never suspend.

Also, the actual polling interval is controlled by /sys/block/sdX/events_poll_msec. The file you mentioned merely contains the default polling interval used for new devices.

Alan Stern

Aaron Lu

author

2013-09-09 02:00 UTC #10 lore raw

PATCH v3 1/1 scsi: sr: use block layer runtime PM

In reply to Aaron Lu

Migrate sr to make use of block layer runtime PM. Accordingly, the SCSI bus layer runtime PM callback is simplified as all SCSI drivers implementing runtime PM now use the block layer's request-based mechanism.

Note that due to the device will be polled by kernel at a constant interval, if the autosuspend delay is set longer than the polling interval then the device will never suspend.

Signed-off-by Aaron Lu <[email protected]>

Acked-by Alan Stern <[email protected]>

---
v2: scsi_dev_type_suspend/resume should be covered by CONFIG_PM_SLEEP, suggested by Alan Stern.
v3: Modify changelog as suggested by Alan Stern and James Bottomley to be more accurate and add Alan Stern's Acked-by tag.

The polling interval for a block device is specified by the per-device setting at: /sys/block/sdX/events_poll_msecs. If its value is -1, then /sys/module/block/parameters/events_dfl_poll_msecs is used.

 drivers/scsi/scsi_pm.c | 62 +++++++++++++-------------------------------------
 drivers/scsi/sr.c      | 37 +++++++++++-------------------
 2 files changed, 29 insertions(+), 70 deletions(-)

drivers/scsi/scsi_pm.c +16 ~~-46~~

index 4c5aabe..bb31fc9 100644
@@ -16,6 +16,8 @@
 
 #include "scsi_priv.h"
 
+#ifdef CONFIG_PM_SLEEP
+
 static int scsi_dev_type_suspend(struct device *dev, int (*cb)(struct device *))
 {
 	int err;

@@ -43,8 +45,6 @@ static int scsi_dev_type_resume(struct device *dev, int (*cb)(struct device *))
 	return err;
 }
 
-#ifdef CONFIG_PM_SLEEP
-
 static int
 scsi_bus_suspend_common(struct device *dev, int (*cb)(struct device *))
 {

@@ -144,38 +144,22 @@ static int scsi_bus_restore(struct device *dev)
 
 #ifdef CONFIG_PM_RUNTIME
 
-static int sdev_blk_runtime_suspend(struct scsi_device *sdev,
-					int (*cb)(struct device *))
+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;
 
 	err = blk_pre_runtime_suspend(sdev->request_queue);
 	if (err)
 		return err;
-	if (cb)
-		err = cb(&sdev->sdev_gendev);
+	if (pm && pm->runtime_suspend)
+		err = pm->runtime_suspend(dev);
 	blk_post_runtime_suspend(sdev->request_queue, err);
 
 	return err;
 }
 
-static int sdev_runtime_suspend(struct device *dev)
-{
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-	int (*cb)(struct device *) = pm ? pm->runtime_suspend : NULL;
-	struct scsi_device *sdev = to_scsi_device(dev);
-	int err;
-
-	if (sdev->request_queue->dev)
-		return sdev_blk_runtime_suspend(sdev, cb);
-
-	err = scsi_dev_type_suspend(dev, cb);
-	if (err == -EAGAIN)
-		pm_schedule_suspend(dev, jiffies_to_msecs(
-					round_jiffies_up_relative(HZ/10)));
-	return err;
-}
-
 static int scsi_runtime_suspend(struct device *dev)
 {
 	int err = 0;

@@ -189,31 +173,20 @@ static int scsi_runtime_suspend(struct device *dev)
 	return err;
 }
 
-static int sdev_blk_runtime_resume(struct scsi_device *sdev,
-					int (*cb)(struct device *))
+static int sdev_runtime_resume(struct device *dev)
 {
+	struct scsi_device *sdev = to_scsi_device(dev);
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int err = 0;
 
 	blk_pre_runtime_resume(sdev->request_queue);
-	if (cb)
-		err = cb(&sdev->sdev_gendev);
+	if (pm && pm->runtime_resume)
+		err = pm->runtime_resume(dev);
 	blk_post_runtime_resume(sdev->request_queue, err);
 
 	return err;
 }
 
-static int sdev_runtime_resume(struct device *dev)
-{
-	struct scsi_device *sdev = to_scsi_device(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-	int (*cb)(struct device *) = pm ? pm->runtime_resume : NULL;
-
-	if (sdev->request_queue->dev)
-		return sdev_blk_runtime_resume(sdev, cb);
-	else
-		return scsi_dev_type_resume(dev, cb);
-}
-
 static int scsi_runtime_resume(struct device *dev)
 {
 	int err = 0;

@@ -234,14 +207,11 @@ static int scsi_runtime_idle(struct device *dev)
 	/* Insert hooks here for targets, hosts, and transport classes */
 
 	if (scsi_is_sdev_device(dev)) {
-		struct scsi_device *sdev = to_scsi_device(dev);
-
-		if (sdev->request_queue->dev) {
-			pm_runtime_mark_last_busy(dev);
-			pm_runtime_autosuspend(dev);
-			return -EBUSY;
-		}
+		pm_runtime_mark_last_busy(dev);
+		pm_runtime_autosuspend(dev);
+		return -EBUSY;
 	}
+
 	return 0;
 }

drivers/scsi/sr.c +13 ~~-24~~

index 119d67f..40d8592 100644
@@ -161,14 +161,10 @@ static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
 		goto out;
 	cd = scsi_cd(disk);
 	kref_get(&cd->kref);
-	if (scsi_device_get(cd->device))
-		goto out_put;
-	if (!scsi_autopm_get_device(cd->device))
-		goto out;
-
- out_put:
-	kref_put(&cd->kref, sr_kref_release);
-	cd = NULL;
+	if (scsi_device_get(cd->device)) {
+		kref_put(&cd->kref, sr_kref_release);
+		cd = NULL;
+	}
  out:
 	mutex_unlock(&sr_ref_mutex);
 	return cd;

@@ -180,7 +176,6 @@ static void scsi_cd_put(struct scsi_cd *cd)
 
 	mutex_lock(&sr_ref_mutex);
 	kref_put(&cd->kref, sr_kref_release);
-	scsi_autopm_put_device(sdev);
 	scsi_device_put(sdev);
 	mutex_unlock(&sr_ref_mutex);
 }

@@ -558,8 +553,6 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 	void __user *argp = (void __user *)arg;
 	int ret;
 
-	scsi_autopm_get_device(cd->device);
-
 	mutex_lock(&sr_mutex);
 
 	/*

@@ -591,7 +584,6 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 
 out:
 	mutex_unlock(&sr_mutex);
-	scsi_autopm_put_device(cd->device);
 	return ret;
 }

@@ -599,17 +591,11 @@ static unsigned int sr_block_check_events(struct gendisk *disk,
 					  unsigned int clearing)
 {
 	struct scsi_cd *cd = scsi_cd(disk);
-	unsigned int ret;
 
-	if (atomic_read(&cd->device->disk_events_disable_depth) == 0) {
-		scsi_autopm_get_device(cd->device);
-		ret = cdrom_check_events(&cd->cdi, clearing);
-		scsi_autopm_put_device(cd->device);
-	} else {
-		ret = 0;
-	}
+	if (atomic_read(&cd->device->disk_events_disable_depth))
+		return 0;
 
-	return ret;
+	return cdrom_check_events(&cd->cdi, clearing);
 }
 
 static int sr_block_revalidate_disk(struct gendisk *disk)

@@ -617,8 +603,6 @@ static int sr_block_revalidate_disk(struct gendisk *disk)
 	struct scsi_cd *cd = scsi_cd(disk);
 	struct scsi_sense_hdr sshdr;
 
-	scsi_autopm_get_device(cd->device);
-
 	/* if the unit is not ready, nothing more to do */
 	if (scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr))
 		goto out;

@@ -626,7 +610,6 @@ static int sr_block_revalidate_disk(struct gendisk *disk)
 	sr_cd_check(&cd->cdi);
 	get_sectorsize(cd);
 out:
-	scsi_autopm_put_device(cd->device);
 	return 0;
 }

@@ -747,6 +730,12 @@ static int sr_probe(struct device *dev)
 	if (register_cdrom(&cd->cdi))
 		goto fail_put;
 
+	/*
+	 * Initialize block layer runtime PM stuffs before the
+	 * periodic event checking request gets started in add_disk.
+	 */
+	blk_pm_runtime_init(sdev->request_queue, dev);
+
 	dev_set_drvdata(dev, cd);
 	disk->flags |= GENHD_FL_REMOVABLE;
 	add_disk(disk);

-- 1.8.3.2.10.g43d11f4 --

Aaron Lu

author

2013-10-28 07:27 UTC #11 lore raw

PATCH RESEND v3 1/1 scsi: sr: use block layer runtime PM

Note that due to the device will be polled by kernel at a constant interval, if the autosuspend delay is set longer than the polling interval then the device will never suspend.

Signed-off-by Aaron Lu <[email protected]>

Acked-by Alan Stern <[email protected]>

 drivers/scsi/scsi_pm.c | 62 +++++++++++++-------------------------------------
 drivers/scsi/sr.c      | 37 +++++++++++-------------------
 2 files changed, 29 insertions(+), 70 deletions(-)

drivers/scsi/scsi_pm.c +16 ~~-46~~

index 4c5aabe..bb31fc9 100644
@@ -16,6 +16,8 @@
 
 #include "scsi_priv.h"
 
+#ifdef CONFIG_PM_SLEEP
+
 static int scsi_dev_type_suspend(struct device *dev, int (*cb)(struct device *))
 {
 	int err;

@@ -43,8 +45,6 @@ static int scsi_dev_type_resume(struct device *dev, int (*cb)(struct device *))
 	return err;
 }
 
-#ifdef CONFIG_PM_SLEEP
-
 static int
 scsi_bus_suspend_common(struct device *dev, int (*cb)(struct device *))
 {

@@ -144,38 +144,22 @@ static int scsi_bus_restore(struct device *dev)
 
 #ifdef CONFIG_PM_RUNTIME
 
-static int sdev_blk_runtime_suspend(struct scsi_device *sdev,
-					int (*cb)(struct device *))
+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;
 
 	err = blk_pre_runtime_suspend(sdev->request_queue);
 	if (err)
 		return err;
-	if (cb)
-		err = cb(&sdev->sdev_gendev);
+	if (pm && pm->runtime_suspend)
+		err = pm->runtime_suspend(dev);
 	blk_post_runtime_suspend(sdev->request_queue, err);
 
 	return err;
 }
 
-static int sdev_runtime_suspend(struct device *dev)
-{
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-	int (*cb)(struct device *) = pm ? pm->runtime_suspend : NULL;
-	struct scsi_device *sdev = to_scsi_device(dev);
-	int err;
-
-	if (sdev->request_queue->dev)
-		return sdev_blk_runtime_suspend(sdev, cb);
-
-	err = scsi_dev_type_suspend(dev, cb);
-	if (err == -EAGAIN)
-		pm_schedule_suspend(dev, jiffies_to_msecs(
-					round_jiffies_up_relative(HZ/10)));
-	return err;
-}
-
 static int scsi_runtime_suspend(struct device *dev)
 {
 	int err = 0;

@@ -189,31 +173,20 @@ static int scsi_runtime_suspend(struct device *dev)
 	return err;
 }
 
-static int sdev_blk_runtime_resume(struct scsi_device *sdev,
-					int (*cb)(struct device *))
+static int sdev_runtime_resume(struct device *dev)
 {
+	struct scsi_device *sdev = to_scsi_device(dev);
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int err = 0;
 
 	blk_pre_runtime_resume(sdev->request_queue);
-	if (cb)
-		err = cb(&sdev->sdev_gendev);
+	if (pm && pm->runtime_resume)
+		err = pm->runtime_resume(dev);
 	blk_post_runtime_resume(sdev->request_queue, err);
 
 	return err;
 }
 
-static int sdev_runtime_resume(struct device *dev)
-{
-	struct scsi_device *sdev = to_scsi_device(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-	int (*cb)(struct device *) = pm ? pm->runtime_resume : NULL;
-
-	if (sdev->request_queue->dev)
-		return sdev_blk_runtime_resume(sdev, cb);
-	else
-		return scsi_dev_type_resume(dev, cb);
-}
-
 static int scsi_runtime_resume(struct device *dev)
 {
 	int err = 0;

@@ -234,14 +207,11 @@ static int scsi_runtime_idle(struct device *dev)
 	/* Insert hooks here for targets, hosts, and transport classes */
 
 	if (scsi_is_sdev_device(dev)) {
-		struct scsi_device *sdev = to_scsi_device(dev);
-
-		if (sdev->request_queue->dev) {
-			pm_runtime_mark_last_busy(dev);
-			pm_runtime_autosuspend(dev);
-			return -EBUSY;
-		}
+		pm_runtime_mark_last_busy(dev);
+		pm_runtime_autosuspend(dev);
+		return -EBUSY;
 	}
+
 	return 0;
 }

drivers/scsi/sr.c +13 ~~-24~~

index 119d67f..40d8592 100644
@@ -161,14 +161,10 @@ static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
 		goto out;
 	cd = scsi_cd(disk);
 	kref_get(&cd->kref);
-	if (scsi_device_get(cd->device))
-		goto out_put;
-	if (!scsi_autopm_get_device(cd->device))
-		goto out;
-
- out_put:
-	kref_put(&cd->kref, sr_kref_release);
-	cd = NULL;
+	if (scsi_device_get(cd->device)) {
+		kref_put(&cd->kref, sr_kref_release);
+		cd = NULL;
+	}
  out:
 	mutex_unlock(&sr_ref_mutex);
 	return cd;

@@ -180,7 +176,6 @@ static void scsi_cd_put(struct scsi_cd *cd)
 
 	mutex_lock(&sr_ref_mutex);
 	kref_put(&cd->kref, sr_kref_release);
-	scsi_autopm_put_device(sdev);
 	scsi_device_put(sdev);
 	mutex_unlock(&sr_ref_mutex);
 }

@@ -558,8 +553,6 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 	void __user *argp = (void __user *)arg;
 	int ret;
 
-	scsi_autopm_get_device(cd->device);
-
 	mutex_lock(&sr_mutex);
 
 	/*

@@ -591,7 +584,6 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 
 out:
 	mutex_unlock(&sr_mutex);
-	scsi_autopm_put_device(cd->device);
 	return ret;
 }

@@ -599,17 +591,11 @@ static unsigned int sr_block_check_events(struct gendisk *disk,
 					  unsigned int clearing)
 {
 	struct scsi_cd *cd = scsi_cd(disk);
-	unsigned int ret;
 
-	if (atomic_read(&cd->device->disk_events_disable_depth) == 0) {
-		scsi_autopm_get_device(cd->device);
-		ret = cdrom_check_events(&cd->cdi, clearing);
-		scsi_autopm_put_device(cd->device);
-	} else {
-		ret = 0;
-	}
+	if (atomic_read(&cd->device->disk_events_disable_depth))
+		return 0;
 
-	return ret;
+	return cdrom_check_events(&cd->cdi, clearing);
 }
 
 static int sr_block_revalidate_disk(struct gendisk *disk)

@@ -617,8 +603,6 @@ static int sr_block_revalidate_disk(struct gendisk *disk)
 	struct scsi_cd *cd = scsi_cd(disk);
 	struct scsi_sense_hdr sshdr;
 
-	scsi_autopm_get_device(cd->device);
-
 	/* if the unit is not ready, nothing more to do */
 	if (scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr))
 		goto out;

@@ -626,7 +610,6 @@ static int sr_block_revalidate_disk(struct gendisk *disk)
 	sr_cd_check(&cd->cdi);
 	get_sectorsize(cd);
 out:
-	scsi_autopm_put_device(cd->device);
 	return 0;
 }

@@ -747,6 +730,12 @@ static int sr_probe(struct device *dev)
 	if (register_cdrom(&cd->cdi))
 		goto fail_put;
 
+	/*
+	 * Initialize block layer runtime PM stuffs before the
+	 * periodic event checking request gets started in add_disk.
+	 */
+	blk_pm_runtime_init(sdev->request_queue, dev);
+
 	dev_set_drvdata(dev, cd);
 	disk->flags |= GENHD_FL_REMOVABLE;
 	add_disk(disk);

-- 1.8.3.2.10.g43d11f4 --