ZPODD patches

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

---
 block/genhd.c         | 23 +++++++++++++++++------
 include/linux/genhd.h |  1 +
 2 files changed, 18 insertions(+), 6 deletions(-)

block/genhd.c +17 -6

index cac7366..4244256 100644
@@ -1627,6 +1627,19 @@ static void disk_events_workfn(struct work_struct *work)
 		kobject_uevent_env(&disk_to_dev(disk)->kobj, KOBJ_CHANGE, envp);
 }
 
+int disk_events_set_poll_msecs(struct gendisk *disk, long intv)
+{
+	if (intv < 0 && intv != -1)
+		return -EINVAL;
+
+	disk_block_events(disk);
+	disk->ev->poll_msecs = intv;
+	__disk_unblock_events(disk, true);
+
+	return 0;
+}
+EXPORT_SYMBOL(disk_events_set_poll_msecs);
+
 /*
  * A disk events enabled device has the following sysfs nodes under
  * its /sys/block/X/ directory.

@@ -1683,16 +1696,14 @@ static ssize_t disk_events_poll_msecs_store(struct device *dev,
 {
 	struct gendisk *disk = dev_to_disk(dev);
 	long intv;
+	int ret;
 
 	if (!count || !sscanf(buf, "%ld", &intv))
 		return -EINVAL;
 
-	if (intv < 0 && intv != -1)
-		return -EINVAL;
-
-	disk_block_events(disk);
-	disk->ev->poll_msecs = intv;
-	__disk_unblock_events(disk, true);
+	ret = disk_events_set_poll_msecs(disk, intv);
+	if (ret)
+		return ret;
 
 	return count;
 }

include/linux/genhd.h +1 -0

index 4f440b3..63409e5 100644
@@ -423,6 +423,7 @@ extern void disk_block_events(struct gendisk *disk);
 extern void disk_unblock_events(struct gendisk *disk);
 extern void disk_flush_events(struct gendisk *disk, unsigned int mask);
 extern unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask);
+extern int disk_events_set_poll_msecs(struct gendisk *disk, long intv);
 
 /* drivers/char/random.c */
 extern void add_disk_randomness(struct gendisk *disk);

Place the ODD into runtime suspend state as soon as there is nobody using it.

The only exception is, if we just find that a new medium is inserted, we wait for the next events checking to idle it.

Based on ideas of Alan Stern and Oliver Neukum.

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

---
 drivers/scsi/sr.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

drivers/scsi/sr.c +1 -0

index 5fc97d2..7a8222f 100644
@@ -45,6 +45,7 @@
 #include <linux/blkdev.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>

drivers/scsi/sr.c +8 -4

+ out_pm:
+	scsi_device_put(cd->device);
  out_put:
 	kref_put(&cd->kref, sr_kref_release);
 	cd = NULL;

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

@@ -211,7 +217,7 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 				    unsigned int clearing, int slot)
 {
 	struct scsi_cd *cd = cdi->handle;
-	bool last_present;
+	bool last_present = cd->media_present;
 	struct scsi_sense_hdr sshdr;
 	unsigned int events;
 	int ret;

@@ -220,6 +226,8 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 	if (CDSL_CURRENT != slot)
 		return 0;
 
+	scsi_autopm_get_device(cd->device);
+
 	events = sr_get_events(cd->device);
 	cd->get_event_changed |= events & DISK_EVENT_MEDIA_CHANGE;
 

@@ -246,10 +254,9 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 	}
 
 	if (!(clearing & DISK_EVENT_MEDIA_CHANGE))
-		return events;
+		goto out;
 do_tur:
 	/* let's see whether the media is there with TUR */
-	last_present = cd->media_present;
 	ret = scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
 
 	/*

@@ -270,7 +277,7 @@ do_tur:
 	}
 
 	if (cd->ignore_get_event)
-		return events;
+		goto out;
 
 	/* check whether GET_EVENT is reporting spurious MEDIA_CHANGE */
 	if (!cd->tur_changed) {

Quoted 2 lines · Aaron Lu

Place the ODD into runtime suspend state as soon as there is nobody using it.

OK, so how is ODD related to the sr driver?

Quoted 2 lines · Aaron Lu

Place the ODD into runtime suspend state as soon as there is nobody using it.

OK, so how is ODD related to the sr driver?

Quoted 3 lines

The only exception is, if we just find that a new medium is inserted, we wait for the next events checking to idle it.

What exactly do you mean by "to idle it"?

Does this patch have any functional effect without the following patches?

Quoted 21 lines

Based on ideas of Alan Stern and Oliver Neukum.

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

---
 drivers/scsi/sr.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

drivers/scsi/sr.c +1 -0

index 5fc97d2..7a8222f 100644
@@ -45,6 +45,7 @@
 #include <linux/blkdev.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>

drivers/scsi/sr.c static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk) · line 152

Quoted 6 lines

drivers/scsi/sr.c +2 -0

@@ -146,8 +147,12 @@ static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
 	kref_get(&cd->kref);
 	if (scsi_device_get(cd->device))
 		goto out_put;
+	if (scsi_autopm_get_device(cd->device))
+		goto out_pm;
 	goto out;

Why don't you do

drivers/scsi/sr.c static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk) · line 154

Quoted 2 lines

drivers/scsi/sr.c +2 -0

+	if (!scsi_autopm_get_device(cd->device))
+		goto out;

without the new label?

Quoted 55 lines

drivers/scsi/sr.c +8 -4

+ out_pm:
+	scsi_device_put(cd->device);
  out_put:
 	kref_put(&cd->kref, sr_kref_release);
 	cd = NULL;

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

@@ -211,7 +217,7 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 				    unsigned int clearing, int slot)
 {
 	struct scsi_cd *cd = cdi->handle;
-	bool last_present;
+	bool last_present = cd->media_present;
 	struct scsi_sense_hdr sshdr;
 	unsigned int events;
 	int ret;

@@ -220,6 +226,8 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 	if (CDSL_CURRENT != slot)
 		return 0;
 
+	scsi_autopm_get_device(cd->device);
+
 	events = sr_get_events(cd->device);
 	cd->get_event_changed |= events & DISK_EVENT_MEDIA_CHANGE;
 

@@ -246,10 +254,9 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 	}
 
 	if (!(clearing & DISK_EVENT_MEDIA_CHANGE))
-		return events;
+		goto out;
 do_tur:
 	/* let's see whether the media is there with TUR */
-	last_present = cd->media_present;
 	ret = scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
 
 	/*

@@ -270,7 +277,7 @@ do_tur:
 	}
 
 	if (cd->ignore_get_event)
-		return events;
+		goto out;
 
 	/* check whether GET_EVENT is reporting spurious MEDIA_CHANGE */
 	if (!cd->tur_changed) {

drivers/scsi/sr.c do_tur: · line 302

Quoted 9 lines

drivers/scsi/sr.c +6 -0

@@ -287,6 +294,12 @@ do_tur:
 	cd->tur_changed = false;
 	cd->get_event_changed = false;
 
+out:
+	if (cd->media_present && !last_present)
+		pm_runtime_put_noidle(&cd->device->sdev_gendev);
+	else
+		scsi_autopm_put_device(cd->device);
+

This thing is asking for a comment.

It looks like you're kind of avoiding to call _idle() for the device, but why? What might go wrong if pm_runtime_put() is used instead of the whole conditional, among other things?

drivers/scsi/sr.c static int sr_probe(struct device *dev) · line 731

Quoted 9 lines

drivers/scsi/sr.c +1 -0

 	return events;
 }
 

@@ -715,9 +728,14 @@ static int sr_probe(struct device *dev)
 	dev_set_drvdata(dev, cd);
 	disk->flags |= GENHD_FL_REMOVABLE;
 	add_disk(disk);
+	disk_events_set_poll_msecs(disk, 5000);

Why do you need this and why is the poll interval universally suitable?

drivers/scsi/sr.c static int sr_probe(struct device *dev) · line 736

Quoted 6 lines

drivers/scsi/sr.c +2 -0

 	sdev_printk(KERN_DEBUG, sdev,
 		    "Attached scsi CD-ROM %s\n", cd->cdi.name);
+
+	/* enable runtime pm */

Not really. What it does is to enable the device to be suspended.

Quoted 10 lines

drivers/scsi/sr.c +2 -0

+	scsi_autopm_put_device(cd->device);
+
 	return 0;
 
 fail_put:

@@ -965,6 +983,9 @@ static int sr_remove(struct device *dev)
 {
 	struct scsi_cd *cd = dev_get_drvdata(dev);
 

drivers/scsi/sr.c static int sr_remove(struct device *dev) · line 986

Quoted 1 line

drivers/scsi/sr.c +1 -0

+	/* disable runtime pm */

And that prevents the device from being suspended (which means that it's going to be resumed at this point in case it was suspended before).

drivers/scsi/sr.c static int sr_remove(struct device *dev) · line 991

Quoted 7 lines

drivers/scsi/sr.c +2 -0

+	scsi_autopm_get_device(cd->device);
+
 	blk_queue_prep_rq(cd->device->request_queue, scsi_prep_fn);
 	del_gendisk(cd->disk);
 

Thanks,
Rafael

Quoted 5 lines · Rafael J. Wysocki

Quoted 2 lines · Aaron Lu

Place the ODD into runtime suspend state as soon as there is nobody using it.

OK, so how is ODD related to the sr driver?

As Alan has explained, ODD(optical disk drive) is driven by scsi sr driver.

Quoted 4 lines

Quoted 2 lines

The only exception is, if we just find that a new medium is inserted, we wait for the next events checking to idle it.

What exactly do you mean by "to idle it"?

I mean to put its usage count so that its idle callback will kick in.

Quoted 1 line

Does this patch have any functional effect without the following patches?

Yes, this one alone takes care of ODD's runtime pm

while the following
patches take care of removing its power after it's runtime suspended. But it doesn't have any real benefit without the following patches.

Quoted 29 lines

Quoted 20 lines

Based on ideas of Alan Stern and Oliver Neukum.

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

---
 drivers/scsi/sr.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

drivers/scsi/sr.c +1 -0

index 5fc97d2..7a8222f 100644
@@ -45,6 +45,7 @@
 #include <linux/blkdev.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>

drivers/scsi/sr.c static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk) · line 152

Quoted 6 lines

drivers/scsi/sr.c +2 -0

@@ -146,8 +147,12 @@ static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
 	kref_get(&cd->kref);
 	if (scsi_device_get(cd->device))
 		goto out_put;
+	if (scsi_autopm_get_device(cd->device))
+		goto out_pm;
 	goto out;

Why don't you do

Quoted 4 lines

drivers/scsi/sr.c static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk) · line 154

Quoted 2 lines

drivers/scsi/sr.c +2 -0

+	if (!scsi_autopm_get_device(cd->device))
+		goto out;

without the new label?

I was just stupidly following the pattern.
Thanks and I'll change this.

Quoted 67 lines

Quoted 55 lines

drivers/scsi/sr.c +8 -4

+ out_pm:
+	scsi_device_put(cd->device);
  out_put:
 	kref_put(&cd->kref, sr_kref_release);
 	cd = NULL;

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

@@ -211,7 +217,7 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 				    unsigned int clearing, int slot)
 {
 	struct scsi_cd *cd = cdi->handle;
-	bool last_present;
+	bool last_present = cd->media_present;
 	struct scsi_sense_hdr sshdr;
 	unsigned int events;
 	int ret;

@@ -220,6 +226,8 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 	if (CDSL_CURRENT != slot)
 		return 0;
 
+	scsi_autopm_get_device(cd->device);
+
 	events = sr_get_events(cd->device);
 	cd->get_event_changed |= events & DISK_EVENT_MEDIA_CHANGE;
 

@@ -246,10 +254,9 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 	}
 
 	if (!(clearing & DISK_EVENT_MEDIA_CHANGE))
-		return events;
+		goto out;
 do_tur:
 	/* let's see whether the media is there with TUR */
-	last_present = cd->media_present;
 	ret = scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
 
 	/*

@@ -270,7 +277,7 @@ do_tur:
 	}
 
 	if (cd->ignore_get_event)
-		return events;
+		goto out;
 
 	/* check whether GET_EVENT is reporting spurious MEDIA_CHANGE */
 	if (!cd->tur_changed) {

drivers/scsi/sr.c do_tur: · line 302

Quoted 9 lines

drivers/scsi/sr.c +6 -0

@@ -287,6 +294,12 @@ do_tur:
 	cd->tur_changed = false;
 	cd->get_event_changed = false;
 
+out:
+	if (cd->media_present && !last_present)
+		pm_runtime_put_noidle(&cd->device->sdev_gendev);
+	else
+		scsi_autopm_put_device(cd->device);
+

This thing is asking for a comment.

Quoted 3 lines

It looks like you're kind of avoiding to call _idle() for the device, but why? What might go wrong if pm_runtime_put() is used instead of the whole conditional, among other things?

The above code means, if we found that a disc is just inserted(reflected by cd->media_present is true and last_present is false), we do not want to put the device into suspend state immediately until next poll. In the interval, some programs may decide to use this device by opening it.

Nothing will go wrong, but it can possibly avoid a runtime status change.

Quoted 10 lines

drivers/scsi/sr.c static int sr_probe(struct device *dev) · line 731

Quoted 8 lines

drivers/scsi/sr.c +1 -0

 	return events;
 }
 

@@ -715,9 +728,14 @@ static int sr_probe(struct device *dev)
 	dev_set_drvdata(dev, cd);
 	disk->flags |= GENHD_FL_REMOVABLE;
 	add_disk(disk);
+	disk_events_set_poll_msecs(disk, 5000);

Why do you need this and why is the poll interval universally suitable?

For a system with udev, the block module parameter events_dfl_poll_msecs will be set to 2s. If disk's events_poll_msecs is not set, that will be used. So the disk will be polled every 2s, that means it will be runtime suspended/resumed every 2s if there is no user. I set it to 5s so that the device can stay in runtime suspended state longer.

And the sysfs interface is still there, if udev thinks a device needs special setting, it will do that and I'll not overwrite that setting.

Quoted 7 lines

drivers/scsi/sr.c static int sr_probe(struct device *dev) · line 736

Quoted 5 lines

drivers/scsi/sr.c +2 -0

 	sdev_printk(KERN_DEBUG, sdev,
 		    "Attached scsi CD-ROM %s\n", cd->cdi.name);
+
+	/* enable runtime pm */

Not really. What it does is to enable the device to be suspended.

OK, will change this.

Quoted 10 lines

Quoted 10 lines

drivers/scsi/sr.c +2 -0

+	scsi_autopm_put_device(cd->device);
+
 	return 0;
 
 fail_put:

@@ -965,6 +983,9 @@ static int sr_remove(struct device *dev)
 {
 	struct scsi_cd *cd = dev_get_drvdata(dev);
 

Quoted 4 lines

drivers/scsi/sr.c static int sr_remove(struct device *dev) · line 986

Quoted 1 line

drivers/scsi/sr.c +1 -0

+	/* disable runtime pm */

And that prevents the device from being suspended (which means that it's going to be resumed at this point in case it was suspended before).

Yes, that's what I want.
We are removing its driver and I think we should undo what we have done to it.

Quoted 9 lines · Aaron Lu

Quoted 5 lines · Rafael J. Wysocki

Quoted 2 lines · Aaron Lu

Place the ODD into runtime suspend state as soon as there is nobody using it.

OK, so how is ODD related to the sr driver?

As Alan has explained, ODD(optical disk drive) is driven by scsi sr driver.

OK, but what about writing "ODD (Optical Disk Drive)" in the changelog?

People reading git logs may not know all of the hardware acronyms and the "0" message doesn't go into the git log. :-)

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

The only exception is, if we just find that a new medium is inserted, we wait for the next events checking to idle it.

What exactly do you mean by "to idle it"?

I mean to put its usage count so that its idle callback will kick in.

So I'd just write that directly in the changelog.

Quoted 3 lines

Quoted 1 line

Does this patch have any functional effect without the following patches?

Yes, this one alone takes care of ODD's runtime pm

I suppose you mean the runtime PM status and usage counter? I.e. the "software state"?

Quoted 4 lines

while the following
patches take care of removing its power after it's runtime suspended. But it doesn't have any real benefit without the following patches.

Please put that information into the changelog too.

Quoted 114 lines

Quoted 30 lines

Quoted 21 lines

Based on ideas of Alan Stern and Oliver Neukum.

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

---
 drivers/scsi/sr.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

drivers/scsi/sr.c +1 -0

index 5fc97d2..7a8222f 100644
@@ -45,6 +45,7 @@
 #include <linux/blkdev.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>

drivers/scsi/sr.c static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk) · line 152

Quoted 6 lines

drivers/scsi/sr.c +2 -0

@@ -146,8 +147,12 @@ static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
 	kref_get(&cd->kref);
 	if (scsi_device_get(cd->device))
 		goto out_put;
+	if (scsi_autopm_get_device(cd->device))
+		goto out_pm;
 	goto out;

Why don't you do

Quoted 4 lines

drivers/scsi/sr.c static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk) · line 154

Quoted 2 lines

drivers/scsi/sr.c +2 -0

+	if (!scsi_autopm_get_device(cd->device))
+		goto out;

without the new label?

I was just stupidly following the pattern.
Thanks and I'll change this.

Quoted 67 lines

Quoted 55 lines

drivers/scsi/sr.c +8 -4

+ out_pm:
+	scsi_device_put(cd->device);
  out_put:
 	kref_put(&cd->kref, sr_kref_release);
 	cd = NULL;

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

@@ -211,7 +217,7 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 				    unsigned int clearing, int slot)
 {
 	struct scsi_cd *cd = cdi->handle;
-	bool last_present;
+	bool last_present = cd->media_present;
 	struct scsi_sense_hdr sshdr;
 	unsigned int events;
 	int ret;

@@ -220,6 +226,8 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 	if (CDSL_CURRENT != slot)
 		return 0;
 
+	scsi_autopm_get_device(cd->device);
+
 	events = sr_get_events(cd->device);
 	cd->get_event_changed |= events & DISK_EVENT_MEDIA_CHANGE;
 

@@ -246,10 +254,9 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 	}
 
 	if (!(clearing & DISK_EVENT_MEDIA_CHANGE))
-		return events;
+		goto out;
 do_tur:
 	/* let's see whether the media is there with TUR */
-	last_present = cd->media_present;
 	ret = scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
 
 	/*

@@ -270,7 +277,7 @@ do_tur:
 	}
 
 	if (cd->ignore_get_event)
-		return events;
+		goto out;
 
 	/* check whether GET_EVENT is reporting spurious MEDIA_CHANGE */
 	if (!cd->tur_changed) {

drivers/scsi/sr.c do_tur: · line 302

Quoted 9 lines

drivers/scsi/sr.c +6 -0

@@ -287,6 +294,12 @@ do_tur:
 	cd->tur_changed = false;
 	cd->get_event_changed = false;
 
+out:
+	if (cd->media_present && !last_present)
+		pm_runtime_put_noidle(&cd->device->sdev_gendev);
+	else
+		scsi_autopm_put_device(cd->device);
+

This thing is asking for a comment.

Quoted 3 lines

It looks like you're kind of avoiding to call _idle() for the device, but why? What might go wrong if pm_runtime_put() is used instead of the whole conditional, among other things?

The above code means, if we found that a disc is just inserted(reflected by cd->media_present is true and last_present is false), we do not want to put the device into suspend state immediately until next poll. In the interval, some programs may decide to use this device by opening it.

Quoted 1 line

Nothing will go wrong, but it can possibly avoid a runtime status change.

OK, so suppose the condition is true and we do the _noidle() put. Who's going to suspend the device in that case if no one actually uses the device?

Quoted 18 lines

Quoted 11 lines

drivers/scsi/sr.c static int sr_probe(struct device *dev) · line 731

Quoted 9 lines

drivers/scsi/sr.c +1 -0

 	return events;
 }
 

@@ -715,9 +728,14 @@ static int sr_probe(struct device *dev)
 	dev_set_drvdata(dev, cd);
 	disk->flags |= GENHD_FL_REMOVABLE;
 	add_disk(disk);
+	disk_events_set_poll_msecs(disk, 5000);

Why do you need this and why is the poll interval universally suitable?

For a system with udev, the block module parameter events_dfl_poll_msecs will be set to 2s. If disk's events_poll_msecs is not set, that will be used. So the disk will be polled every 2s, that means it will be runtime suspended/resumed every 2s if there is no user. I set it to 5s so that the device can stay in runtime suspended state longer.

Quoted 2 lines

And the sysfs interface is still there, if udev thinks a device needs special setting, it will do that and I'll not overwrite that setting.

I'm not quite convinced this is the right approach here.

So if you set it to 5 s this way, the user space will have no idea that the polling happens less often than it thinks, or am I misunderstanding what you said above?

Moreover, what about changing the code so that the polling doesn't actually resume the device?

Quoted 26 lines

Quoted 8 lines

drivers/scsi/sr.c static int sr_probe(struct device *dev) · line 736

Quoted 6 lines

drivers/scsi/sr.c +2 -0

 	sdev_printk(KERN_DEBUG, sdev,
 		    "Attached scsi CD-ROM %s\n", cd->cdi.name);
+
+	/* enable runtime pm */

Not really. What it does is to enable the device to be suspended.

OK, will change this.

Quoted 14 lines

drivers/scsi/sr.c static int sr_remove(struct device *dev) · line 986

Quoted 11 lines

drivers/scsi/sr.c +3 -0

+	scsi_autopm_put_device(cd->device);
+
 	return 0;
 
 fail_put:

@@ -965,6 +983,9 @@ static int sr_remove(struct device *dev)
 {
 	struct scsi_cd *cd = dev_get_drvdata(dev);
 
+	/* disable runtime pm */

And that prevents the device from being suspended (which means that it's going to be resumed at this point in case it was suspended before).

Quoted 3 lines

Yes, that's what I want.
We are removing its driver and I think we should undo what we have done to it.

Yes, I agree. Only the comment wording can better reflect what really happens here (runtime PM is not disabled, in particular).

Quoted 23 lines · Rafael J. Wysocki

Quoted 9 lines · Aaron Lu

Quoted 5 lines · Rafael J. Wysocki

Quoted 2 lines · Aaron Lu

Place the ODD into runtime suspend state as soon as there is nobody using it.

OK, so how is ODD related to the sr driver?

As Alan has explained, ODD(optical disk drive) is driven by scsi sr driver.

OK, but what about writing "ODD (Optical Disk Drive)" in the changelog?

People reading git logs may not know all of the hardware acronyms and the "0" message doesn't go into the git log. :-)

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

The only exception is, if we just find that a new medium is inserted, we wait for the next events checking to idle it.

What exactly do you mean by "to idle it"?

I mean to put its usage count so that its idle callback will kick in.

Quoted 8 lines

So I'd just write that directly in the changelog.

Quoted 3 lines

Quoted 1 line

Does this patch have any functional effect without the following patches?

Yes, this one alone takes care of ODD's runtime pm

I suppose you mean the runtime PM status and usage counter? I.e. the "software state"?

Yes.

Quoted 5 lines

Quoted 4 lines

while the following
patches take care of removing its power after it's runtime suspended. But it doesn't have any real benefit without the following patches.

Quoted 1 line

Please put that information into the changelog too.

As Alan explained, I think I would say:
Though currently it doesn't have any benefit, it allows its parent devices enter runtime suspend state which may save some power.

Quoted 115 lines

Quoted 114 lines

Quoted 29 lines

Quoted 20 lines

Based on ideas of Alan Stern and Oliver Neukum.

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

---
 drivers/scsi/sr.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

drivers/scsi/sr.c +1 -0

index 5fc97d2..7a8222f 100644
@@ -45,6 +45,7 @@
 #include <linux/blkdev.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>

drivers/scsi/sr.c static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk) · line 152

Quoted 6 lines

drivers/scsi/sr.c +2 -0

@@ -146,8 +147,12 @@ static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
 	kref_get(&cd->kref);
 	if (scsi_device_get(cd->device))
 		goto out_put;
+	if (scsi_autopm_get_device(cd->device))
+		goto out_pm;
 	goto out;

Why don't you do

Quoted 4 lines

drivers/scsi/sr.c static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk) · line 154

Quoted 2 lines

drivers/scsi/sr.c +2 -0

+	if (!scsi_autopm_get_device(cd->device))
+		goto out;

without the new label?

I was just stupidly following the pattern.
Thanks and I'll change this.

Quoted 67 lines

Quoted 55 lines

drivers/scsi/sr.c +8 -4

+ out_pm:
+	scsi_device_put(cd->device);
  out_put:
 	kref_put(&cd->kref, sr_kref_release);
 	cd = NULL;

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

@@ -211,7 +217,7 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 				    unsigned int clearing, int slot)
 {
 	struct scsi_cd *cd = cdi->handle;
-	bool last_present;
+	bool last_present = cd->media_present;
 	struct scsi_sense_hdr sshdr;
 	unsigned int events;
 	int ret;

@@ -220,6 +226,8 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 	if (CDSL_CURRENT != slot)
 		return 0;
 
+	scsi_autopm_get_device(cd->device);
+
 	events = sr_get_events(cd->device);
 	cd->get_event_changed |= events & DISK_EVENT_MEDIA_CHANGE;
 

@@ -246,10 +254,9 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 	}
 
 	if (!(clearing & DISK_EVENT_MEDIA_CHANGE))
-		return events;
+		goto out;
 do_tur:
 	/* let's see whether the media is there with TUR */
-	last_present = cd->media_present;
 	ret = scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
 
 	/*

@@ -270,7 +277,7 @@ do_tur:
 	}
 
 	if (cd->ignore_get_event)
-		return events;
+		goto out;
 
 	/* check whether GET_EVENT is reporting spurious MEDIA_CHANGE */
 	if (!cd->tur_changed) {

drivers/scsi/sr.c do_tur: · line 302

Quoted 9 lines

drivers/scsi/sr.c +6 -0

@@ -287,6 +294,12 @@ do_tur:
 	cd->tur_changed = false;
 	cd->get_event_changed = false;
 
+out:
+	if (cd->media_present && !last_present)
+		pm_runtime_put_noidle(&cd->device->sdev_gendev);
+	else
+		scsi_autopm_put_device(cd->device);
+

This thing is asking for a comment.

Quoted 3 lines

It looks like you're kind of avoiding to call _idle() for the device, but why? What might go wrong if pm_runtime_put() is used instead of the whole conditional, among other things?

The above code means, if we found that a disc is just inserted(reflected by cd->media_present is true and last_present is false), we do not want to put the device into suspend state immediately until next poll. In the interval, some programs may decide to use this device by opening it.

Nothing will go wrong, but it can possibly avoid a runtime status change.

Quoted 2 lines

OK, so suppose the condition is true and we do the _noidle() put. Who's going to suspend the device in that case if no one actually uses the device?

Next time when the check_events poll runs, it will find that:
1 Either the disc is still there, then both last_present and
media_present would be true, we will do the put to idle it;
2 Or the disc is ejected, we will do the put to idle it.

The poll runs periodically, until the device is powered off(reflected by the powered_off flag), in which case, the poll will simply return 0 without touching this device.

Quoted 22 lines

Quoted 11 lines

Quoted 10 lines

drivers/scsi/sr.c static int sr_probe(struct device *dev) · line 731

Quoted 8 lines

drivers/scsi/sr.c +1 -0

 	return events;
 }
 

@@ -715,9 +728,14 @@ static int sr_probe(struct device *dev)
 	dev_set_drvdata(dev, cd);
 	disk->flags |= GENHD_FL_REMOVABLE;
 	add_disk(disk);
+	disk_events_set_poll_msecs(disk, 5000);

Why do you need this and why is the poll interval universally suitable?

Quoted 8 lines

For a system with udev, the block module parameter events_dfl_poll_msecs will be set to 2s. If disk's events_poll_msecs is not set, that will be used. So the disk will be polled every 2s, that means it will be runtime suspended/resumed every 2s if there is no user. I set it to 5s so that the device can stay in runtime suspended state longer.

And the sysfs interface is still there, if udev thinks a device needs special setting, it will do that and I'll not overwrite that setting.

I'm not quite convinced this is the right approach here.

Quoted 3 lines

So if you set it to 5 s this way, the user space will have no idea that the polling happens less often than it thinks, or am I misunderstanding what you said above?

That's correct.
AFAIK, user space does not care how often the device is polled, it cares only one thing: when there is a media change event, please let me know(through uevent).

I agree that we can't make user wait for too long before seeing something happen(auto play, etc.) after he inserted a disc, and 5 seconds doesn't seem too long to me.

Quoted 3 lines

Moreover, what about changing the code so that the polling doesn't actually resume the device?

Since the device is going to do IO(executing a scsi command), I think I should resume the device.

But there is a case for ZPODD, when the ODD is powered off(reflected by the powered_off flag), the events checking will simply return without resuming the device.

Quoted 29 lines

Quoted 28 lines

Quoted 7 lines

drivers/scsi/sr.c static int sr_probe(struct device *dev) · line 736

Quoted 5 lines

drivers/scsi/sr.c +2 -0

 	sdev_printk(KERN_DEBUG, sdev,
 		    "Attached scsi CD-ROM %s\n", cd->cdi.name);
+
+	/* enable runtime pm */

Not really. What it does is to enable the device to be suspended.

OK, will change this.

Quoted 10 lines

Quoted 10 lines

drivers/scsi/sr.c +2 -0

+	scsi_autopm_put_device(cd->device);
+
 	return 0;
 
 fail_put:

@@ -965,6 +983,9 @@ static int sr_remove(struct device *dev)
 {
 	struct scsi_cd *cd = dev_get_drvdata(dev);
 

Quoted 4 lines

drivers/scsi/sr.c static int sr_remove(struct device *dev) · line 986

Quoted 1 line

drivers/scsi/sr.c +1 -0

+	/* disable runtime pm */

And that prevents the device from being suspended (which means that it's going to be resumed at this point in case it was suspended before).

Yes, that's what I want.
We are removing its driver and I think we should undo what we have done to it.

Quoted 2 lines

Yes, I agree. Only the comment wording can better reflect what really happens here (runtime PM is not disabled, in particular).

OK, looks like you are saying by disable, disable_depth is the subject while I'm playing with usage_count. I'll pay attention to these words, thanks for the remind.

Quoted 40 lines · Aaron Lu

Quoted 23 lines · Rafael J. Wysocki

Quoted 9 lines · Aaron Lu

Quoted 5 lines · Rafael J. Wysocki

Quoted 2 lines · Aaron Lu

Place the ODD into runtime suspend state as soon as there is nobody using it.

OK, so how is ODD related to the sr driver?

As Alan has explained, ODD(optical disk drive) is driven by scsi sr driver.

OK, but what about writing "ODD (Optical Disk Drive)" in the changelog?

People reading git logs may not know all of the hardware acronyms and the "0" message doesn't go into the git log. :-)

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

The only exception is, if we just find that a new medium is inserted, we wait for the next events checking to idle it.

What exactly do you mean by "to idle it"?

I mean to put its usage count so that its idle callback will kick in.

Quoted 8 lines

So I'd just write that directly in the changelog.

Quoted 3 lines

Quoted 1 line

Does this patch have any functional effect without the following patches?

Yes, this one alone takes care of ODD's runtime pm

I suppose you mean the runtime PM status and usage counter? I.e. the "software state"?

Yes.

Quoted 5 lines

Quoted 4 lines

while the following
patches take care of removing its power after it's runtime suspended. But it doesn't have any real benefit without the following patches.

Quoted 5 lines

Quoted 1 line

Please put that information into the changelog too.

As Alan explained, I think I would say:
Though currently it doesn't have any benefit, it allows its parent devices enter runtime suspend state which may save some power.

Well, please say that in the changelog, then. :-)

Quoted 115 lines

Quoted 115 lines

Quoted 114 lines

Quoted 29 lines

Quoted 20 lines

Based on ideas of Alan Stern and Oliver Neukum.

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

---
 drivers/scsi/sr.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

drivers/scsi/sr.c +1 -0

index 5fc97d2..7a8222f 100644
@@ -45,6 +45,7 @@
 #include <linux/blkdev.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>

drivers/scsi/sr.c static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk) · line 152

Quoted 6 lines

drivers/scsi/sr.c +2 -0

@@ -146,8 +147,12 @@ static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
 	kref_get(&cd->kref);
 	if (scsi_device_get(cd->device))
 		goto out_put;
+	if (scsi_autopm_get_device(cd->device))
+		goto out_pm;
 	goto out;

Why don't you do

Quoted 4 lines

drivers/scsi/sr.c static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk) · line 154

Quoted 2 lines

drivers/scsi/sr.c +2 -0

+	if (!scsi_autopm_get_device(cd->device))
+		goto out;

without the new label?

I was just stupidly following the pattern.
Thanks and I'll change this.

Quoted 67 lines

Quoted 55 lines

drivers/scsi/sr.c +8 -4

+ out_pm:
+	scsi_device_put(cd->device);
  out_put:
 	kref_put(&cd->kref, sr_kref_release);
 	cd = NULL;

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

@@ -211,7 +217,7 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 				    unsigned int clearing, int slot)
 {
 	struct scsi_cd *cd = cdi->handle;
-	bool last_present;
+	bool last_present = cd->media_present;
 	struct scsi_sense_hdr sshdr;
 	unsigned int events;
 	int ret;

@@ -220,6 +226,8 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 	if (CDSL_CURRENT != slot)
 		return 0;
 
+	scsi_autopm_get_device(cd->device);
+
 	events = sr_get_events(cd->device);
 	cd->get_event_changed |= events & DISK_EVENT_MEDIA_CHANGE;
 

@@ -246,10 +254,9 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 	}
 
 	if (!(clearing & DISK_EVENT_MEDIA_CHANGE))
-		return events;
+		goto out;
 do_tur:
 	/* let's see whether the media is there with TUR */
-	last_present = cd->media_present;
 	ret = scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
 
 	/*

@@ -270,7 +277,7 @@ do_tur:
 	}
 
 	if (cd->ignore_get_event)
-		return events;
+		goto out;
 
 	/* check whether GET_EVENT is reporting spurious MEDIA_CHANGE */
 	if (!cd->tur_changed) {

drivers/scsi/sr.c do_tur: · line 302

Quoted 9 lines

drivers/scsi/sr.c +6 -0

@@ -287,6 +294,12 @@ do_tur:
 	cd->tur_changed = false;
 	cd->get_event_changed = false;
 
+out:
+	if (cd->media_present && !last_present)
+		pm_runtime_put_noidle(&cd->device->sdev_gendev);
+	else
+		scsi_autopm_put_device(cd->device);
+

This thing is asking for a comment.

Quoted 3 lines

It looks like you're kind of avoiding to call _idle() for the device, but why? What might go wrong if pm_runtime_put() is used instead of the whole conditional, among other things?

The above code means, if we found that a disc is just inserted(reflected by cd->media_present is true and last_present is false), we do not want to put the device into suspend state immediately until next poll. In the interval, some programs may decide to use this device by opening it.

Nothing will go wrong, but it can possibly avoid a runtime status change.

Quoted 7 lines

Quoted 2 lines

OK, so suppose the condition is true and we do the _noidle() put. Who's going to suspend the device in that case if no one actually uses the device?

Next time when the check_events poll runs, it will find that:
1 Either the disc is still there, then both last_present and
media_present would be true, we will do the put to idle it;
2 Or the disc is ejected, we will do the put to idle it.

In that case I would do:

pm_runtime_put_noidle(&cd->device->sdev_gendev);
if (cd->media_present && !last_present)

    pm_runtime_suspend(&cd->device->sdev_gendev);

And I'd add a comment about the next poll.

This appears somewhat racy, though, because in theory a media may be inserted while we are removing power from the device. Isn't that a problem?

Quoted 4 lines

The poll runs periodically, until the device is powered off(reflected by the powered_off flag), in which case, the poll will simply return 0 without touching this device.

Is it useful to poll the device after it has been suspended, but not powered off?

Quoted 27 lines

Quoted 26 lines

Quoted 12 lines

Quoted 11 lines

drivers/scsi/sr.c static int sr_probe(struct device *dev) · line 731

Quoted 9 lines

drivers/scsi/sr.c +1 -0

 	return events;
 }
 

@@ -715,9 +728,14 @@ static int sr_probe(struct device *dev)
 	dev_set_drvdata(dev, cd);
 	disk->flags |= GENHD_FL_REMOVABLE;
 	add_disk(disk);
+	disk_events_set_poll_msecs(disk, 5000);

Why do you need this and why is the poll interval universally suitable?

Quoted 8 lines

For a system with udev, the block module parameter events_dfl_poll_msecs will be set to 2s. If disk's events_poll_msecs is not set, that will be used. So the disk will be polled every 2s, that means it will be runtime suspended/resumed every 2s if there is no user. I set it to 5s so that the device can stay in runtime suspended state longer.

And the sysfs interface is still there, if udev thinks a device needs special setting, it will do that and I'll not overwrite that setting.

I'm not quite convinced this is the right approach here.

So if you set it to 5 s this way, the user space will have no idea that the polling happens less often than it thinks, or am I misunderstanding what you said above?

Quoted 4 lines

That's correct.
AFAIK, user space does not care how often the device is polled, it cares only one thing: when there is a media change event, please let me know(through uevent).

So that's why we do the polling, right?

Quoted 12 lines

I agree that we can't make user wait for too long before seeing something happen(auto play, etc.) after he inserted a disc, and 5 seconds doesn't seem too long to me.

Quoted 3 lines

Moreover, what about changing the code so that the polling doesn't actually resume the device?

Since the device is going to do IO(executing a scsi command), I think I should resume the device.

Quoted 3 lines

But there is a case for ZPODD, when the ODD is powered off(reflected by the powered_off flag), the events checking will simply return without resuming the device.

Yes, I understand that. My question is whether or not we still need to poll if the device hasn't been powered off, although it has been suspended.

Quoted 30 lines

Quoted 30 lines

Quoted 29 lines

Quoted 8 lines

drivers/scsi/sr.c static int sr_probe(struct device *dev) · line 736

Quoted 6 lines

drivers/scsi/sr.c +2 -0

 	sdev_printk(KERN_DEBUG, sdev,
 		    "Attached scsi CD-ROM %s\n", cd->cdi.name);
+
+	/* enable runtime pm */

Not really. What it does is to enable the device to be suspended.

OK, will change this.

Quoted 10 lines

Quoted 10 lines

drivers/scsi/sr.c +2 -0

+	scsi_autopm_put_device(cd->device);
+
 	return 0;
 
 fail_put:

@@ -965,6 +983,9 @@ static int sr_remove(struct device *dev)
 {
 	struct scsi_cd *cd = dev_get_drvdata(dev);
 

Quoted 4 lines

drivers/scsi/sr.c static int sr_remove(struct device *dev) · line 986

Quoted 1 line

drivers/scsi/sr.c +1 -0

+	/* disable runtime pm */

And that prevents the device from being suspended (which means that it's going to be resumed at this point in case it was suspended before).

Yes, that's what I want.
We are removing its driver and I think we should undo what we have done to it.

Quoted 6 lines

Quoted 2 lines

Yes, I agree. Only the comment wording can better reflect what really happens here (runtime PM is not disabled, in particular).

OK, looks like you are saying by disable, disable_depth is the subject while I'm playing with usage_count. I'll pay attention to these words, thanks for the remind.

Please do.

Thanks,
Rafael

Quoted 5 lines · Rafael J. Wysocki

Quoted 4 lines · Aaron Lu

Quoted 3 lines · Rafael J. Wysocki

Quoted 2 lines · Aaron Lu

Quoted 1 line · Rafael J. Wysocki

On Wednesday, September 12, 2012, Aaron Lu wrote:

Quoted 34 lines

Quoted 33 lines

Quoted 24 lines

Quoted 24 lines

Quoted 15 lines

Review do_tur: · line 302

Quoted 12 lines

(quoted diff) +6 -0

 	/* check whether GET_EVENT is reporting spurious MEDIA_CHANGE */
 	if (!cd->tur_changed) {

@@ -287,6 +294,12 @@ do_tur:
 	cd->tur_changed = false;
 	cd->get_event_changed = false;
 
+out:
+	if (cd->media_present && !last_present)
+		pm_runtime_put_noidle(&cd->device->sdev_gendev);
+	else
+		scsi_autopm_put_device(cd->device);
+

This thing is asking for a comment.

Quoted 3 lines

It looks like you're kind of avoiding to call _idle() for the device, but why? What might go wrong if pm_runtime_put() is used instead of the whole conditional, among other things?

The above code means, if we found that a disc is just inserted(reflected by cd->media_present is true and last_present is false), we do not want to put the device into suspend state immediately until next poll. In the interval, some programs may decide to use this device by opening it.

Quoted 4 lines

Quoted 1 line

Nothing will go wrong, but it can possibly avoid a runtime status change.

OK, so suppose the condition is true and we do the _noidle() put. Who's going to suspend the device in that case if no one actually uses the device?

Next time when the check_events poll runs, it will find that:
1 Either the disc is still there, then both last_present and
media_present would be true, we will do the put to idle it;
2 Or the disc is ejected, we will do the put to idle it.

Quoted 5 lines

In that case I would do:

pm_runtime_put_noidle(&cd->device->sdev_gendev);
if (cd->media_present && !last_present)

    pm_runtime_suspend(&cd->device->sdev_gendev);

This doesn't cover the !cd->media_present(media not present) case. If there is no media present, we will also need to idle it.

Quoted 4 lines

And I'd add a comment about the next poll.

This appears somewhat racy, though, because in theory a media may be inserted while we are removing power from the device. Isn't that a problem?

Yes, this is a problem.
To avoid this race, I think the following needs to be done:
- For slot type ODD, make it such that user can't insert a disc;
- For tray type ODD, make it such that when user presses the eject button, the tray doesn't open.
I'll see if this is possible, thanks for the remind.

Quoted 6 lines

Quoted 3 lines

The poll runs periodically, until the device is powered off(reflected by the powered_off flag), in which case, the poll will simply return 0 without touching this device.

Is it useful to poll the device after it has been suspended, but not powered off?

Yes, it is necessary to poll the device when it has been suspended with power left. The reason is, we need to check if there is a media change event happened, and the way to check this is to issue a GET_EVENT_STATUS_NOTIFICATION comand.

Please note that when the drive is not powered off, it will not send us a notification when its eject button is pressed.

Quoted 26 lines

Quoted 26 lines

Quoted 25 lines

Quoted 11 lines

Quoted 10 lines

Review static int sr_probe(struct device *dev) · line 731

Quoted 8 lines

(quoted diff) +1 -0

 	return events;
 }
 

@@ -715,9 +728,14 @@ static int sr_probe(struct device *dev)
 	dev_set_drvdata(dev, cd);
 	disk->flags |= GENHD_FL_REMOVABLE;
 	add_disk(disk);
+	disk_events_set_poll_msecs(disk, 5000);

Why do you need this and why is the poll interval universally suitable?

Quoted 8 lines

For a system with udev, the block module parameter events_dfl_poll_msecs will be set to 2s. If disk's events_poll_msecs is not set, that will be used. So the disk will be polled every 2s, that means it will be runtime suspended/resumed every 2s if there is no user. I set it to 5s so that the device can stay in runtime suspended state longer.

And the sysfs interface is still there, if udev thinks a device needs special setting, it will do that and I'll not overwrite that setting.

I'm not quite convinced this is the right approach here.

So if you set it to 5 s this way, the user space will have no idea that the polling happens less often than it thinks, or am I misunderstanding what you said above?

Quoted 6 lines

Quoted 4 lines

That's correct.
AFAIK, user space does not care how often the device is polled, it cares only one thing: when there is a media change event, please let me know(through uevent).

So that's why we do the polling, right?

Yes.

Quoted 6 lines · Aaron Lu

Quoted 4 lines · Rafael J. Wysocki

And I'd add a comment about the next poll.

This appears somewhat racy, though, because in theory a media may be inserted while we are removing power from the device. Isn't that a problem?

Quoted 6 lines

Yes, this is a problem.
To avoid this race, I think the following needs to be done:
- For slot type ODD, make it such that user can't insert a disc;
- For tray type ODD, make it such that when user presses the eject button, the tray doesn't open.
I'll see if this is possible, thanks for the remind.

OK

Quoted 13 lines

Quoted 7 lines

Quoted 4 lines

The poll runs periodically, until the device is powered off(reflected by the powered_off flag), in which case, the poll will simply return 0 without touching this device.

Is it useful to poll the device after it has been suspended, but not powered off?

Yes, it is necessary to poll the device when it has been suspended with power left. The reason is, we need to check if there is a media change event happened, and the way to check this is to issue a GET_EVENT_STATUS_NOTIFICATION comand.

Quoted 2 lines

Please note that when the drive is not powered off, it will not send us a notification when its eject button is pressed.

I'm not sure about that, actually. If it doesn't notify us, that whole thing is inherently racy pretty much beyond fixing, because it is always possible that the user will press the eject button right after we've checked the status last time and right before power removal. We're going to lose that event, so the user will have to push the button once again in that case.

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

That's correct.
AFAIK, user space does not care how often the device is polled, it cares only one thing: when there is a media change event, please let me know(through uevent).

So that's why we do the polling, right?

Quoted 1 line

Yes.

Well, that's difficult.

If the remote wakeup is signaled through a GPE, it should be possible to enable it before we actually put the device into D3cold. That may allow us to eliminate the races.

Quoted 7 lines · Rafael J. Wysocki

Quoted 6 lines · Aaron Lu

Quoted 4 lines · Rafael J. Wysocki

And I'd add a comment about the next poll.

This appears somewhat racy, though, because in theory a media may be inserted while we are removing power from the device. Isn't that a problem?

Quoted 8 lines

Quoted 6 lines

Yes, this is a problem.
To avoid this race, I think the following needs to be done:
- For slot type ODD, make it such that user can't insert a disc;
- For tray type ODD, make it such that when user presses the eject button, the tray doesn't open.
I'll see if this is possible, thanks for the remind.

OK

Looks like this is not the right thing to do, if I lock the door user will be confused.

Quoted 13 lines

Quoted 13 lines

Quoted 7 lines

Quoted 4 lines

The poll runs periodically, until the device is powered off(reflected by the powered_off flag), in which case, the poll will simply return 0 without touching this device.

Is it useful to poll the device after it has been suspended, but not powered off?

Yes, it is necessary to poll the device when it has been suspended with power left. The reason is, we need to check if there is a media change event happened, and the way to check this is to issue a GET_EVENT_STATUS_NOTIFICATION comand.

Quoted 8 lines

Quoted 2 lines

Please note that when the drive is not powered off, it will not send us a notification when its eject button is pressed.

I'm not sure about that, actually. If it doesn't notify us, that whole thing is inherently racy pretty much beyond fixing, because it is always possible that the user will press the eject button right after we've checked the status last time and right before power removal. We're going to lose that event, so the user will have to push the button once again in that case.

I just checked the spec again and tested, when the ODD has power, it will also send out notifications on pressing the eject button/inserting a disc. So we should be able to capture such a event.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Quoted 4 lines

That's correct.
AFAIK, user space does not care how often the device is polled, it cares only one thing: when there is a media change event, please let me know(through uevent).

So that's why we do the polling, right?

Quoted 7 lines

Quoted 1 line

Yes.

Well, that's difficult.

If the remote wakeup is signaled through a GPE, it should be possible to enable it before we actually put the device into D3cold. That may allow us to eliminate the races.

Thanks for the suggestion, I'll update the code.

I'm thinking of enabling this GPE in sr_suspend once we decided that it is ready to be powered off, so the time frame between sr_suspend and when the power is actually removed in libata should be taken care of by the GPE. If GPE fires, the notification function will request a runtime resume of the device. Does this sound OK?

Quoted 5 lines · Aaron Lu

I'm thinking of enabling this GPE in sr_suspend once we decided that it is ready to be powered off, so the time frame between sr_suspend and when the power is actually removed in libata should be taken care of by the GPE. If GPE fires, the notification function will request a runtime resume of the device. Does this sound OK?

Well, depending on the implementation. sr_suspend() should be rather generic, but the ACPI association (including the GPE thing) is specific to ATA.

Quoted 7 lines · Rafael J. Wysocki

Quoted 5 lines · Aaron Lu

I'm thinking of enabling this GPE in sr_suspend once we decided that it is ready to be powered off, so the time frame between sr_suspend and when the power is actually removed in libata should be taken care of by the GPE. If GPE fires, the notification function will request a runtime resume of the device. Does this sound OK?

Quoted 2 lines

Well, depending on the implementation. sr_suspend() should be rather generic, but the ACPI association (including the GPE thing) is specific to ATA.

Sorry, but don't quite understand this.

We have ACPI bindings for scsi devices, isn't that for us to use ACPI when needed in scsi?

BTW, if sr_suspend should be generic, that would suggest I shouldn't write any ZPODD related code there, right? Any suggestion where these code should go then?

Quoted 11 lines · On Tuesday 25 September 2012 22:20:21 Aaron Lu wrote:

Quoted 9 lines · Rafael J. Wysocki

Quoted 5 lines · Aaron Lu

I'm thinking of enabling this GPE in sr_suspend once we decided that it is ready to be powered off, so the time frame between sr_suspend and when the power is actually removed in libata should be taken care of by the GPE. If GPE fires, the notification function will request a runtime resume of the device. Does this sound OK?

Well, depending on the implementation. sr_suspend() should be rather generic, but the ACPI association (including the GPE thing) is specific to ATA.

Quoted 4 lines

Sorry, but don't quite understand this.

We have ACPI bindings for scsi devices, isn't that for us to use ACPI when needed in scsi?

We don't have ACPI bindings for generic SCSI devices. We have such bindings for SATA drives. You can put such things only in sr if it applies to all (maybe most) types of drives.

Quoted 4 lines

BTW, if sr_suspend should be generic, that would suggest I shouldn't write any ZPODD related code there, right? Any suggestion where these code should go then?

libata. Maybe some generic hooks can be called in sr_suspend().

PS: Are you sure sr_suspend() handles DVD-RAMs correctly?

Quoted 12 lines · Oliver Neukum

Quoted 11 lines · On Tuesday 25 September 2012 22:20:21 Aaron Lu wrote:

Quoted 9 lines · Rafael J. Wysocki

Quoted 5 lines · Aaron Lu

I'm thinking of enabling this GPE in sr_suspend once we decided that it is ready to be powered off, so the time frame between sr_suspend and when the power is actually removed in libata should be taken care of by the GPE. If GPE fires, the notification function will request a runtime resume of the device. Does this sound OK?

Well, depending on the implementation. sr_suspend() should be rather generic, but the ACPI association (including the GPE thing) is specific to ATA.

Quoted 8 lines

Quoted 4 lines

Sorry, but don't quite understand this.

We have ACPI bindings for scsi devices, isn't that for us to use ACPI when needed in scsi?

We don't have ACPI bindings for generic SCSI devices. We have such bindings for SATA drives. You can put such things only in sr if it applies to all (maybe most) types of drives.

OK. Then these scsi bindings for sata drives will be pretty much of no use I think.

Quoted 5 lines

Quoted 4 lines

BTW, if sr_suspend should be generic, that would suggest I shouldn't write any ZPODD related code there, right? Any suggestion where these code should go then?

Quoted 1 line

libata. Maybe some generic hooks can be called in sr_suspend().

Thanks for the suggestion.
The problem is, I need to know whether the door is closed and if there is a medium inside. I've no way of getting such information in libata.

Quoted 1 line

PS: Are you sure sr_suspend() handles DVD-RAMs correctly?

No. Is there a spec for it?
Considering there are many different drives sr handle, is it possible to write a generic sr_suspend?
Maybe your suggestion of callback is the way to go.
What about this idea, if we find this is a ZPODD capable drive, we enable runtime suspend for it and write a suspend callback according to ZPODD spec. For other drives that does not have a suspend callback, we do not enable runtime suspend.

Does this sound reasonable?

Quoted 30 lines · Aaron Lu

Quoted 12 lines · Oliver Neukum

Quoted 11 lines · On Tuesday 25 September 2012 22:20:21 Aaron Lu wrote:

Quoted 9 lines · Rafael J. Wysocki

Quoted 5 lines · Aaron Lu

I'm thinking of enabling this GPE in sr_suspend once we decided that it is ready to be powered off, so the time frame between sr_suspend and when the power is actually removed in libata should be taken care of by the GPE. If GPE fires, the notification function will request a runtime resume of the device. Does this sound OK?

Well, depending on the implementation. sr_suspend() should be rather generic, but the ACPI association (including the GPE thing) is specific to ATA.

Quoted 8 lines

Quoted 4 lines

Sorry, but don't quite understand this.

We have ACPI bindings for scsi devices, isn't that for us to use ACPI when needed in scsi?

We don't have ACPI bindings for generic SCSI devices. We have such bindings for SATA drives. You can put such things only in sr if it applies to all (maybe most) types of drives.

OK. Then these scsi bindings for sata drives will be pretty much of no use I think.

Quoted 5 lines

Quoted 4 lines

BTW, if sr_suspend should be generic, that would suggest I shouldn't write any ZPODD related code there, right? Any suggestion where these code should go then?

Quoted 5 lines

Quoted 1 line

libata. Maybe some generic hooks can be called in sr_suspend().

Thanks for the suggestion.
The problem is, I need to know whether the door is closed and if there is a medium inside. I've no way of getting such information in libata.

How does sr get to know it in the libata case?

Quoted 11 lines

Quoted 2 lines

PS: Are you sure sr_suspend() handles DVD-RAMs correctly?

No. Is there a spec for it?
Considering there are many different drives sr handle, is it possible to write a generic sr_suspend?
Maybe your suggestion of callback is the way to go.
What about this idea, if we find this is a ZPODD capable drive, we enable runtime suspend for it and write a suspend callback according to ZPODD spec. For other drives that does not have a suspend callback, we do not enable runtime suspend.

You can enable runtime PM for all kinds of drives, but make the suspend and resume callbacks only do something for ZPODD ones. This may allow their parents to use runtime PM (as Alan said earlier in this thread), even if the drives themseleves are not really physically suspended.

Quoted 2 lines

Does this sound reasonable?

First, we need to know when the drive is not in use. That information we can get from the sr's runtime PM and it looks like we need to notify libata about that somehow. I'm not sure what mechanism is the best for that at the moment.

Second, when the device is resumed by remote wakeup, we need to notify sr about that. A "resume" alone is not sufficient, though, because it may be necessary to open the tray. Perhaps in that case we can use the same mechanism by which user events are processed by libata and delivered to sr?

Quoted 31 lines · Rafael J. Wysocki

Quoted 30 lines · Aaron Lu

Quoted 12 lines · Oliver Neukum

Quoted 11 lines · On Tuesday 25 September 2012 22:20:21 Aaron Lu wrote:

Quoted 9 lines · Rafael J. Wysocki

Quoted 5 lines · Aaron Lu

I'm thinking of enabling this GPE in sr_suspend once we decided that it is ready to be powered off, so the time frame between sr_suspend and when the power is actually removed in libata should be taken care of by the GPE. If GPE fires, the notification function will request a runtime resume of the device. Does this sound OK?

Well, depending on the implementation. sr_suspend() should be rather generic, but the ACPI association (including the GPE thing) is specific to ATA.

Quoted 8 lines

Quoted 4 lines

Sorry, but don't quite understand this.

We have ACPI bindings for scsi devices, isn't that for us to use ACPI when needed in scsi?

We don't have ACPI bindings for generic SCSI devices. We have such bindings for SATA drives. You can put such things only in sr if it applies to all (maybe most) types of drives.

OK. Then these scsi bindings for sata drives will be pretty much of no use I think.

Quoted 5 lines

Quoted 4 lines

BTW, if sr_suspend should be generic, that would suggest I shouldn't write any ZPODD related code there, right? Any suggestion where these code should go then?

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

libata. Maybe some generic hooks can be called in sr_suspend().

Thanks for the suggestion.
The problem is, I need to know whether the door is closed and if there is a medium inside. I've no way of getting such information in libata.

How does sr get to know it in the libata case?

By executing a test_unit_ready command.

Libata does/should not have any routine to do this, it is one of the transport of SCSI devices and it relies on SCSI driver to manage the device(both disk and ODD).

Quoted 12 lines

Quoted 11 lines

Quoted 2 lines

PS: Are you sure sr_suspend() handles DVD-RAMs correctly?

No. Is there a spec for it?
Considering there are many different drives sr handle, is it possible to write a generic sr_suspend?
Maybe your suggestion of callback is the way to go.
What about this idea, if we find this is a ZPODD capable drive, we enable runtime suspend for it and write a suspend callback according to ZPODD spec. For other drives that does not have a suspend callback, we do not enable runtime suspend.

Quoted 4 lines

You can enable runtime PM for all kinds of drives, but make the suspend and resume callbacks only do something for ZPODD ones. This may allow their parents to use runtime PM (as Alan said earlier in this thread), even if the drives themseleves are not really physically suspended.

Sounds good.

Quoted 7 lines

Quoted 2 lines

Does this sound reasonable?

First, we need to know when the drive is not in use. That information we can get from the sr's runtime PM and it looks like we need to notify libata about that somehow. I'm not sure what mechanism is the best for that at the moment.

The current mechanism to notify libata is by rumtime suspend. When scsi device is runtime suspended, its parent device will be suspended. And ata_port is one of the ancestor devices of scsi device, and we will remove its power in ata_port's runtime suspend callback.

Quoted 4 lines

Second, when the device is resumed by remote wakeup, we need to notify sr about that. A "resume" alone is not sufficient, though, because it may be necessary to open the tray. Perhaps in that case we can use the same mechanism by which user events are processed by libata and delivered to sr?

Thanks for the suggestion.
I'm not aware of any user events processed by libata. Do you mean the events_checking poll?

Quoted 73 lines · Aaron Lu

Quoted 31 lines · Rafael J. Wysocki

Quoted 30 lines · Aaron Lu

Quoted 12 lines · Oliver Neukum

Quoted 11 lines · On Tuesday 25 September 2012 22:20:21 Aaron Lu wrote:

Quoted 9 lines · Rafael J. Wysocki

Quoted 5 lines · Aaron Lu

I'm thinking of enabling this GPE in sr_suspend once we decided that it is ready to be powered off, so the time frame between sr_suspend and when the power is actually removed in libata should be taken care of by the GPE. If GPE fires, the notification function will request a runtime resume of the device. Does this sound OK?

Well, depending on the implementation. sr_suspend() should be rather generic, but the ACPI association (including the GPE thing) is specific to ATA.

Quoted 8 lines

Quoted 4 lines

Sorry, but don't quite understand this.

We have ACPI bindings for scsi devices, isn't that for us to use ACPI when needed in scsi?

We don't have ACPI bindings for generic SCSI devices. We have such bindings for SATA drives. You can put such things only in sr if it applies to all (maybe most) types of drives.

OK. Then these scsi bindings for sata drives will be pretty much of no use I think.

Quoted 5 lines

Quoted 4 lines

BTW, if sr_suspend should be generic, that would suggest I shouldn't write any ZPODD related code there, right? Any suggestion where these code should go then?

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

libata. Maybe some generic hooks can be called in sr_suspend().

Thanks for the suggestion.
The problem is, I need to know whether the door is closed and if there is a medium inside. I've no way of getting such information in libata.

How does sr get to know it in the libata case?

By executing a test_unit_ready command.

Libata does/should not have any routine to do this, it is one of the transport of SCSI devices and it relies on SCSI driver to manage the device(both disk and ODD).

Quoted 12 lines

Quoted 11 lines

Quoted 2 lines

PS: Are you sure sr_suspend() handles DVD-RAMs correctly?

No. Is there a spec for it?
Considering there are many different drives sr handle, is it possible to write a generic sr_suspend?
Maybe your suggestion of callback is the way to go.
What about this idea, if we find this is a ZPODD capable drive, we enable runtime suspend for it and write a suspend callback according to ZPODD spec. For other drives that does not have a suspend callback, we do not enable runtime suspend.

Quoted 4 lines

You can enable runtime PM for all kinds of drives, but make the suspend and resume callbacks only do something for ZPODD ones. This may allow their parents to use runtime PM (as Alan said earlier in this thread), even if the drives themseleves are not really physically suspended.

Sounds good.

Quoted 7 lines

Quoted 2 lines

Does this sound reasonable?

First, we need to know when the drive is not in use. That information we can get from the sr's runtime PM and it looks like we need to notify libata about that somehow. I'm not sure what mechanism is the best for that at the moment.

Quoted 4 lines

The current mechanism to notify libata is by rumtime suspend. When scsi device is runtime suspended, its parent device will be suspended. And ata_port is one of the ancestor devices of scsi device, and we will remove its power in ata_port's runtime suspend callback.

The problem, then, is that the ata_port's runtime suspend callback would have to know whether or not power can be removed from the drive.

I'm going to post patches introducing a "no power off" flag for PM QoS, among other things, today or tomorrow. I suppose this flag might be used to tell the ata_port's suspend not to remove power from the attached device.

Quoted 6 lines

Quoted 5 lines

Second, when the device is resumed by remote wakeup, we need to notify sr about that. A "resume" alone is not sufficient, though, because it may be necessary to open the tray. Perhaps in that case we can use the same mechanism by which user events are processed by libata and delivered to sr?

Quoted 3 lines

Thanks for the suggestion.
I'm not aware of any user events processed by libata. Do you mean the events_checking poll?

Yes, basically. In the remote wakeup case libata might report the same status as in the "user pressed the eject button" situation happening normally with power on.

Quoted 5 lines · Oliver Neukum

Quoted 3 lines

BTW, if sr_suspend should be generic, that would suggest I shouldn't write any ZPODD related code there, right? Any suggestion where these code should go then?

libata. Maybe some generic hooks can be called in sr_suspend().

Thanks for the suggestion.
The problem is, I need to know whether the door is closed and if there is a medium inside. I've no way of getting such information in libata.

Quoted 1 line

PS: Are you sure sr_suspend() handles DVD-RAMs correctly?

No. Is there a spec for it?

Considering there are many different drives sr handle, is it possible to write a generic sr_suspend?

From the perspective of power management they are no longer

Maybe your suggestion of callback is the way to go.
What about this idea, if we find this is a ZPODD capable drive, we enable runtime suspend for it and write a suspend callback according to ZPODD spec. For other drives that does not have a suspend callback, we do not enable runtime suspend.
Does this sound reasonable?

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Aaron Lu

On Mon, Sep 24, 2012 at 02:55:31PM +0200, Rafael J. Wysocki wrote:

I just checked the spec again and tested, when the ODD has power, it will also send out notifications on pressing the eject button/inserting a disc. So we should be able to capture such a event.

I'm thinking of enabling this GPE in sr_suspend once we decided that it is ready to be powered off, so the time frame between sr_suspend and when the power is actually removed in libata should be taken care of by the GPE. If GPE fires, the notification function will request a runtime resume of the device. Does this sound OK?

Quoted 7 lines · On Tuesday 25 September 2012 16:01:35 Aaron Lu wrote:

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Aaron Lu

On Mon, Sep 24, 2012 at 02:55:31PM +0200, Rafael J. Wysocki wrote:

I just checked the spec again and tested, when the ODD has power, it will also send out notifications on pressing the eject button/inserting a disc. So we should be able to capture such a event.

In this case there's no need to poll for disk change unless the button has been pressed.

Quoted 6 lines

I'm thinking of enabling this GPE in sr_suspend once we decided that it is ready to be powered off, so the time frame between sr_suspend and when the power is actually removed in libata should be taken care of by the GPE. If GPE fires, the notification function will request a runtime resume of the device. Does this sound OK?

This sounds terribly, needlessly complicated. Just enable it when you detect the presence of a disk drive that supports it.

Furthermore we have a device which can detect that a button has been pressed. It is fundamentally wrong to poll for medium change in such devices. You know that it hasn't been changed.

We should notify the upper layers that we can do medium change detection on our own.

Regards
Oliver

When ODD is runtime suspended, we will check if it is OK to remove its power:
1 For tray type, no medium inside and tray closed;
2 For slot type, no medium inside.
And if yes, we will set the ready_to_power_off flag as an indication to ATA layer that it is safe to place this device into ACPI D3 cold power state.

And when it is powered off, we will set the powered_off flag so that the periodically running check_events will not bother this device by simply return.

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

---
 drivers/ata/libata-acpi.c  | 27 +++++++++++++++--------
 drivers/scsi/sr.c          | 53 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/sr.h          |  1 +
 drivers/scsi/sr_ioctl.c    |  7 +++++-
 include/scsi/scsi_device.h |  3 +++
 5 files changed, 81 insertions(+), 10 deletions(-)

drivers/ata/libata-acpi.c +18 -9

index 902b5a4..9aca057 100644
@@ -854,7 +854,7 @@ void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
 {
 	struct ata_device *dev;
 	acpi_handle handle;
-	int acpi_state;
+	int acpi_state, ret;
 
 	/* channel first and then drives for power on and vica versa
 	   for power off */

@@ -869,17 +869,24 @@ void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
 
 		if (state.event != PM_EVENT_ON) {
 			acpi_state = acpi_pm_device_sleep_state(
-				&dev->sdev->sdev_gendev, NULL, ACPI_STATE_D3);
-			if (acpi_state > 0)
-				acpi_bus_set_power(handle, acpi_state);
-			/* TBD: need to check if it's runtime pm request */
-			acpi_pm_device_run_wake(
-				&dev->sdev->sdev_gendev, true);
+					&dev->sdev->sdev_gendev, NULL,
+					dev->sdev->ready_to_power_off ?
+					ACPI_STATE_D3 : ACPI_STATE_D3_HOT);
+			if (acpi_state > 0) {
+				ret = acpi_bus_set_power(handle, acpi_state);
+				if (!ret && acpi_state == ACPI_STATE_D3)
+					dev->sdev->powered_off = 1;
+
+				/* TODO: check if it's runtime pm request */
+				acpi_pm_device_run_wake(
+						&dev->sdev->sdev_gendev, true);
+			}
 		} else {
 			/* Ditto */
 			acpi_pm_device_run_wake(
 				&dev->sdev->sdev_gendev, false);
 			acpi_bus_set_power(handle, ACPI_STATE_D0);
+			dev->sdev->powered_off = 0;
 		}
 	}
 

@@ -985,8 +992,10 @@ static void ata_acpi_wake_dev(acpi_handle handle, u32 event, void *context)
 	struct ata_device *ata_dev = context;
 
 	if (event == ACPI_NOTIFY_DEVICE_WAKE && ata_dev &&
-			pm_runtime_suspended(&ata_dev->sdev->sdev_gendev))
-		scsi_autopm_get_device(ata_dev->sdev);
+			ata_dev->sdev->powered_off) {
+		ata_dev->sdev->need_eject = 1;
+		pm_runtime_resume(&ata_dev->sdev->sdev_gendev);
+	}
 }
 
 static void ata_acpi_add_pm_notifier(struct ata_device *dev)

drivers/scsi/sr.c +4 -0

index 7a8222f..ef72682 100644
@@ -80,6 +80,8 @@ static DEFINE_MUTEX(sr_mutex);
 static int sr_probe(struct device *);
 static int sr_remove(struct device *);
 static int sr_done(struct scsi_cmnd *);
+static int sr_suspend(struct device *, pm_message_t msg);
+static int sr_resume(struct device *);
 
 static struct scsi_driver sr_template = {
 	.owner			= THIS_MODULE,

@@ -87,6 +89,8 @@ static struct scsi_driver sr_template = {
 		.name   	= "sr",
 		.probe		= sr_probe,
 		.remove		= sr_remove,
+		.suspend        = sr_suspend,
+		.resume         = sr_resume,
 	},
 	.done			= sr_done,
 };

drivers/scsi/sr.c +49 -0

+static int sr_suspend(struct device *dev, pm_message_t msg)
+{
+	struct scsi_sense_hdr sshdr;
+	struct scsi_cd *cd = dev_get_drvdata(dev);
+
+	if (!cd->device->can_power_off)
+		return 0;
+
+	/* See if we can power off this ZPODD device */
+	scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
+	if (cd->cdi.mask & CDC_CLOSE_TRAY)
+		/* no media for caddy/slot type ODD */
+		cd->device->ready_to_power_off = scsi_sense_valid(&sshdr) &&
+							sshdr.asc == 0x3a;
+	else
+		/* no media and door closed for tray type ODD */
+		cd->device->ready_to_power_off = scsi_sense_valid(&sshdr) &&
+					sshdr.asc == 0x3a && sshdr.ascq == 0x01;
+
+	return 0;
+}
+
+static int sr_resume(struct device *dev)
+{
+	struct scsi_cd *cd;
+	struct scsi_sense_hdr sshdr;
+
+	cd = dev_get_drvdata(dev);
+
+	if (!cd->device->powered_off)
+		return 0;
+
+	/* get the disk ready */
+	scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
+
+	/* If user wakes up the ODD, eject the tray */
+	if (cd->device->need_eject) {
+		cd->device->need_eject = 0;
+		/* But only for tray type ODD when door is not locked */
+		if (!(cd->cdi.mask & CDC_CLOSE_TRAY) && !cd->door_locked)
+			sr_tray_move(&cd->cdi, 1);
+	}
+
+	return 0;
+}
+
 static unsigned int sr_get_events(struct scsi_device *sdev)
 {
 	u8 buf[8];

@@ -226,6 +276,9 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 	if (CDSL_CURRENT != slot)
 		return 0;
 
+	if (cd->device->powered_off)
+		return 0;
+
 	scsi_autopm_get_device(cd->device);
 
 	events = sr_get_events(cd->device);

drivers/scsi/sr.h +1 -0

index 37c8f6b..1c84537 100644
@@ -41,6 +41,7 @@ typedef struct scsi_cd {
 	unsigned readcd_known:1;	/* drive supports READ_CD (0xbe) */
 	unsigned readcd_cdda:1;	/* reading audio data using READ_CD */
 	unsigned media_present:1;	/* media is present */
+	unsigned door_locked:1; /* door is locked */
 
 	/* GET_EVENT spurious event handling, blk layer guarantees exclusion */
 	int tur_mismatch;		/* nr of get_event TUR mismatches */

drivers/scsi/sr_ioctl.c +6 -1

index a3911c3..c1275f6 100644
@@ -292,10 +292,15 @@ int sr_tray_move(struct cdrom_device_info *cdi, int pos)
 
 int sr_lock_door(struct cdrom_device_info *cdi, int lock)
 {
+	int ret;
 	Scsi_CD *cd = cdi->handle;
 
-	return scsi_set_medium_removal(cd->device, lock ?
+	ret = scsi_set_medium_removal(cd->device, lock ?
 		       SCSI_REMOVAL_PREVENT : SCSI_REMOVAL_ALLOW);
+	if (!ret)
+		cd->door_locked = lock;
+
+	return ret;
 }
 
 int sr_drive_status(struct cdrom_device_info *cdi, int slot)

include/scsi/scsi_device.h +0 -0

index 9895f69..da5c86f 100644

Quoted 12 lines · Aaron Lu

static struct scsi_driver sr_template = {

drivers/scsi/sr.c +2 -0

 	.owner			= THIS_MODULE,

@@ -87,6 +89,8 @@ static struct scsi_driver sr_template = {
 		.name   	= "sr",
 		.probe		= sr_probe,
 		.remove		= sr_remove,
+		.suspend        = sr_suspend,
+		.resume         = sr_resume,
 	},
 	.done			= sr_done,
 };

drivers/scsi/sr.c static void scsi_cd_put(struct scsi_cd *cd) · line 177

Quoted 2 lines

drivers/scsi/sr.c +0 -0

@@ -172,6 +176,52 @@ static void scsi_cd_put(struct scsi_cd *cd)
 	mutex_unlock(&sr_ref_mutex);
 }

Besides, I need some help to understand how this is supposed to work.

Do I think correctly that sr_suspend(), for example, will be run by the SCSI bus type layer in case of a CD device runtime suspend? However,

won't this routine be used during system suspend as well and won't it cause problems to happen if so?

Quoted 18 lines · Rafael J. Wysocki

Quoted 12 lines · Aaron Lu

static struct scsi_driver sr_template = {

drivers/scsi/sr.c +2 -0

 	.owner			= THIS_MODULE,

@@ -87,6 +89,8 @@ static struct scsi_driver sr_template = {
 		.name   	= "sr",
 		.probe		= sr_probe,
 		.remove		= sr_remove,
+		.suspend        = sr_suspend,
+		.resume         = sr_resume,
 	},
 	.done			= sr_done,
 };

drivers/scsi/sr.c static void scsi_cd_put(struct scsi_cd *cd) · line 177

Quoted 2 lines

drivers/scsi/sr.c +0 -0

@@ -172,6 +176,52 @@ static void scsi_cd_put(struct scsi_cd *cd)
 	mutex_unlock(&sr_ref_mutex);
 }

Besides, I need some help to understand how this is supposed to work.

Quoted 2 lines

Do I think correctly that sr_suspend(), for example, will be run by the SCSI bus type layer in case of a CD device runtime suspend? However,

Yes.

Quoted 2 lines

won't this routine be used during system suspend as well and won't it cause problems to happen if so?

On system suspend, nothing needs to be done.
I'll add the following code in next version.

	if (!PMSG_IS_AUTO(msg))
		return 0;

Quoted 24 lines · Aaron Lu

Quoted 18 lines · Rafael J. Wysocki

Quoted 12 lines · Aaron Lu

static struct scsi_driver sr_template = {

(quoted diff) +2 -0

 	.owner			= THIS_MODULE,

@@ -87,6 +89,8 @@ static struct scsi_driver sr_template = {
 		.name   	= "sr",
 		.probe		= sr_probe,
 		.remove		= sr_remove,
+		.suspend        = sr_suspend,
+		.resume         = sr_resume,
 	},
 	.done			= sr_done,
 };

Review static void scsi_cd_put(struct scsi_cd *cd) · line 177

Quoted 2 lines

(quoted diff) +0 -0

@@ -172,6 +176,52 @@ static void scsi_cd_put(struct scsi_cd *cd)
 	mutex_unlock(&sr_ref_mutex);
 }

Besides, I need some help to understand how this is supposed to work.

Quoted 2 lines

Do I think correctly that sr_suspend(), for example, will be run by the SCSI bus type layer in case of a CD device runtime suspend? However,

Yes.

Quoted 8 lines

Quoted 2 lines

won't this routine be used during system suspend as well and won't it cause problems to happen if so?

On system suspend, nothing needs to be done.
I'll add the following code in next version.

	if (!PMSG_IS_AUTO(msg))
		return 0;

Please don't. The pm_message_t thing is obsolete and shoulnd't really be used by device drivers. I know that ATA relies on it internally, but that's just something that needs to be changed at one point.

Moreover, I'd like to migrate SCSI drivers to the PM handling based on struct dev_pm_ops eventually and your change is kind of going in the opposite direction. I don't know how much effort the migration is going to take, though, so perhaps we can just make this change first.

Quoted 4 lines

Moreover, I'd like to migrate SCSI drivers to the PM handling based on struct dev_pm_ops eventually and your change is kind of going in the opposite direction. I don't know how much effort the migration is going to take, though, so perhaps we can just make this change first.

Does the following change look OK?

drivers/scsi/scsi_pm.c +2 -1

index dc0ad85..1fb7ccc 100644
@@ -143,7 +143,15 @@ static int scsi_runtime_suspend(struct device *dev)
 
 	dev_dbg(dev, "scsi_runtime_suspend\n");
 	if (scsi_is_sdev_device(dev)) {
-		err = scsi_dev_type_suspend(dev, PMSG_AUTO_SUSPEND);
+		err = scsi_device_quiesce(to_scsi_device(dev));
+		if (err)

Quoted 25 lines · Aaron Lu

Quoted 4 lines

Moreover, I'd like to migrate SCSI drivers to the PM handling based on struct dev_pm_ops eventually and your change is kind of going in the opposite direction. I don't know how much effort the migration is going to take, though, so perhaps we can just make this change first.

Does the following change look OK?

drivers/scsi/scsi_pm.c +9 -1

index dc0ad85..1fb7ccc 100644
@@ -143,7 +143,15 @@ static int scsi_runtime_suspend(struct device *dev)
 
 	dev_dbg(dev, "scsi_runtime_suspend\n");
 	if (scsi_is_sdev_device(dev)) {
-		err = scsi_dev_type_suspend(dev, PMSG_AUTO_SUSPEND);
+		err = scsi_device_quiesce(to_scsi_device(dev));
+		if (err)
+			goto out;
+
+		err = pm_generic_runtime_suspend(dev);
+		if (!err)
+			goto out;
+
+		scsi_device_resume(to_scsi_device(dev));

drivers/scsi/scsi_pm.c static int scsi_runtime_suspend(struct device *dev) · line 157

Quoted 3 lines

drivers/scsi/scsi_pm.c +0 -0

 		if (err == -EAGAIN)
 			pm_schedule_suspend(dev, jiffies_to_msecs(
 				round_jiffies_up_relative(HZ/10)));

Maybe in the end it will be better, but for now this looks like unnecessary code duplication. Basically you are copying scsi_dev_type_suspend() into scsi_runtime_suspend(), except that you omitted the debugging statement.

Quoted 27 lines · Alan Stern

Quoted 25 lines · Aaron Lu

Quoted 4 lines

Moreover, I'd like to migrate SCSI drivers to the PM handling based on struct dev_pm_ops eventually and your change is kind of going in the opposite direction. I don't know how much effort the migration is going to take, though, so perhaps we can just make this change first.

Does the following change look OK?

drivers/scsi/scsi_pm.c +9 -1

index dc0ad85..1fb7ccc 100644
@@ -143,7 +143,15 @@ static int scsi_runtime_suspend(struct device *dev)
 
 	dev_dbg(dev, "scsi_runtime_suspend\n");
 	if (scsi_is_sdev_device(dev)) {
-		err = scsi_dev_type_suspend(dev, PMSG_AUTO_SUSPEND);
+		err = scsi_device_quiesce(to_scsi_device(dev));
+		if (err)
+			goto out;
+
+		err = pm_generic_runtime_suspend(dev);
+		if (!err)
+			goto out;
+
+		scsi_device_resume(to_scsi_device(dev));

Quoted 8 lines

drivers/scsi/scsi_pm.c static int scsi_runtime_suspend(struct device *dev) · line 157

Quoted 3 lines

drivers/scsi/scsi_pm.c +0 -0

 		if (err == -EAGAIN)
 			pm_schedule_suspend(dev, jiffies_to_msecs(
 				round_jiffies_up_relative(HZ/10)));

Maybe in the end it will be better, but for now this looks like unnecessary code duplication. Basically you are copying scsi_dev_type_suspend() into scsi_runtime_suspend(), except that you omitted the debugging statement.

And I've used pm_generic_runtime_suspend :-)
That would allow me to write runtime callbacks of dev_pm_ops for indivisual scsi drivers, like sr.

When ODD is runtime suspended, we will check if it is OK to remove its power:
1 For tray type, no medium inside and tray closed;
2 For slot type, no medium inside.
And if yes, we will set the ready_to_power_off flag as an indication to ATA layer that it is safe to place this device into ACPI D3 cold power state.

And when it is powered off, we will set the powered_off flag so that the periodically running check_events will not bother this device by simply return.

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

---
  drivers/ata/libata-acpi.c  | 27 +++++++++++++++--------
  drivers/scsi/sr.c          | 53 ++++++++++++++++++++++++++++++++++++++++++++++
  drivers/scsi/sr.h          |  1 +
  drivers/scsi/sr_ioctl.c    |  7 +++++-
  include/scsi/scsi_device.h |  3 +++
  5 files changed, 81 insertions(+), 10 deletions(-)

drivers/ata/libata-acpi.c +14 -7

index 902b5a4..9aca057 100644
@@ -854,7 +854,7 @@ void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
  {
  	struct ata_device *dev;
  	acpi_handle handle;
-	int acpi_state;
+	int acpi_state, ret;
 
  	/* channel first and then drives for power on and vica versa
  	   for power off */

@@ -869,17 +869,24 @@ void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
 
  		if (state.event != PM_EVENT_ON) {
  			acpi_state = acpi_pm_device_sleep_state(
-				&dev->sdev->sdev_gendev, NULL, ACPI_STATE_D3);
-			if (acpi_state > 0)
-				acpi_bus_set_power(handle, acpi_state);
-			/* TBD: need to check if it's runtime pm request */
-			acpi_pm_device_run_wake(
-				&dev->sdev->sdev_gendev, true);
+					&dev->sdev->sdev_gendev, NULL,
+					dev->sdev->ready_to_power_off ?
+					ACPI_STATE_D3 : ACPI_STATE_D3_HOT);
+			if (acpi_state > 0) {
+				ret = acpi_bus_set_power(handle, acpi_state);
+				if (!ret && acpi_state == ACPI_STATE_D3)
+					dev->sdev->powered_off = 1;
+
+				/* TODO: check if it's runtime pm request */
+				acpi_pm_device_run_wake(
+						&dev->sdev->sdev_gendev, true);
+			}
  		} else {
  			/* Ditto */
  			acpi_pm_device_run_wake(
  				&dev->sdev->sdev_gendev, false);
  			acpi_bus_set_power(handle, ACPI_STATE_D0);
+			dev->sdev->powered_off = 0;
  		}
  	}
 

@@ -985,8 +992,10 @@ static void ata_acpi_wake_dev(acpi_handle handle, u32 event, void *context)
  	struct ata_device *ata_dev = context;

If user does not want the device to be powered off when runtime suspended by setting may_power_off flag to 0, we will not choose D3 cold for it.

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

---
  drivers/ata/libata-acpi.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

drivers/ata/libata-acpi.c +1 -0

index 9aca057..24347e0 100644
@@ -855,6 +855,7 @@ void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
  	struct ata_device *dev;
  	acpi_handle handle;
  	int acpi_state, ret;
+	bool power_off_allowed;
 
  	/* channel first and then drives for power on and vica versa
  	   for power off */

v7:
Re work of runtime pm of sr driver, based on ideas of Alan Stern and Oliver Neukum.

Jeff, due to the ready_to_power_off flag added, there is a small change in [PATCH v7 6/6] libata: acpi: respect may_power_off flag, please check if I can still get your ack, thanks.

v6:
When user changes may_power_off flag through sysfs entry and if device is already runtime suspended, resume resume it so that it can respect this flag next time it is runtime suspended as suggested by Alan Stern. Call scsi_autopm_get/put_device once in sr_check_events as suggested by Alan Stern.

v5:
Add may_power_off flag to scsi device.
Alan Stern suggested that I should not mess runtime suspend with runtime power off, but the current zpodd implementation made it not easy to seperate. So I re-wrote the zpodd implementation, the end result is, normal ODD can also enter runtime suspended state, but their power won't be removed.

v4:
Rebase on top of Linus' tree, due to this, the problem of a missing flag in v3 is gone;
Add a new function scsi_autopm_put_device_autosuspend to first mark last busy for the device and then put autosuspend it as suggested by Oliver Neukum.
Typo fix as pointed by Sergei Shtylyov.
Check can_power_off flag before any runtime pm operations in sr.

v3:
Rebase on top of scsi-misc tree;
Add the sr related patches previously in Jeff's libata tree; Re-organize the sr patches.
A problem for now: for patch
scsi: sr: support zero power ODD(ZPODD)
I can't set a flag in libata-acpi.c since a related function is missing in scsi-misc tree. Will fix this when 3.6-rc1 released.

v2:
Bug fix for v1;
Use scsi_autopm_* in sr driver instead of pm_runtime_*;

v1:
Here are some patches to make ZPODD easier to use for end users and a fix for using ZPODD with system suspend.

Aaron Lu (6):
block: genhd: add an interface to set disk poll interval
scsi: sr: support runtime pm
scsi: sr: support zero power ODD(ZPODD)
scsi: pm: add may_power_off flag
scsi: sr: use may_power_off
libata: acpi: respect may_power_off flag

 block/genhd.c              | 23 +++++++++----
 drivers/ata/libata-acpi.c  | 30 +++++++++++-----
 drivers/scsi/scsi_sysfs.c  | 37 +++++++++++++++++++-
 drivers/scsi/sr.c          | 86 +++++++++++++++++++++++++++++++++++++++++++---
 drivers/scsi/sr.h          |  1 +
 drivers/scsi/sr_ioctl.c    |  7 +++-
 include/linux/genhd.h      |  1 +
 include/scsi/scsi_device.h |  4 +++
 8 files changed, 168 insertions(+), 21 deletions(-)

Signature · 1 line

1.7.12.21.g871e293

Hi James,

May I know if this patchset will enter v3.7?

Quoted 3 lines · Aaron Lu

Hi James,

May I know if this patchset will enter v3.7?

Sigh, well, I was hoping to persuade the PM people to sort this out first.

The first observation is that all this looks to be too specific. ZPO may be ACPI specific, but the property it abstracts: whether the particular device is powered off or not is generic and probably should be known at the generic PM level. Nothing actually really cares about how we power off the device until you get all the way down to the ACPI controller.

I think we could do this with a couple of flags sitting inside struct device itself: one for pm state and capabilities defined at a generic level and one for device specific pm state. The latter would be for things like the door lock information which is very specific to CDs (although not specific to SCSI CDs). Alternatively, even if we can't use these capabilities at the generic pm level, we still need an internal state set of flags because power state stuff traverses the stack and struct device is the only universal object in that stack.

So I definitely think all of the sdev flags should become either generic or specific flags in device.

Quoted 24 lines · James Bottomley

Quoted 3 lines · Aaron Lu

Hi James,

May I know if this patchset will enter v3.7?

Sigh, well, I was hoping to persuade the PM people to sort this out first.

The first observation is that all this looks to be too specific. ZPO may be ACPI specific, but the property it abstracts: whether the particular device is powered off or not is generic and probably should be known at the generic PM level. Nothing actually really cares about how we power off the device until you get all the way down to the ACPI controller.

I think we could do this with a couple of flags sitting inside struct device itself: one for pm state and capabilities defined at a generic level and one for device specific pm state. The latter would be for things like the door lock information which is very specific to CDs (although not specific to SCSI CDs). Alternatively, even if we can't use these capabilities at the generic pm level, we still need an internal state set of flags because power state stuff traverses the stack and struct device is the only universal object in that stack.

Quoted 2 lines

So I definitely think all of the sdev flags should become either generic or specific flags in device.

Well, the problem is that it is kind of irrelevant to the core whether or not the given device can be powered off. Moreover, the actual meaning of what "power off" means depends on the platform (it may be an individual device state or a power domain state, for instance). Also, the set of available low-power states depends on the platform (or the bus type) and generally cannot be universally represented and there are low-power states that aren't "power off" per se, but still require the device state to be restored when putting it back into full power.

We've discussed that for a few times and each time we've ended up agreeing that struct device is not the right place to store this information (for example, PCI stores it in struct pci_dev, USB has its own rules etc.).

I'll have a look at the patchset again and see what can be done about this.

Quoted 37 lines · Rafael J. Wysocki

Quoted 24 lines · James Bottomley

Quoted 3 lines · Aaron Lu

Hi James,

May I know if this patchset will enter v3.7?

Sigh, well, I was hoping to persuade the PM people to sort this out first.

The first observation is that all this looks to be too specific. ZPO may be ACPI specific, but the property it abstracts: whether the particular device is powered off or not is generic and probably should be known at the generic PM level. Nothing actually really cares about how we power off the device until you get all the way down to the ACPI controller.

I think we could do this with a couple of flags sitting inside struct device itself: one for pm state and capabilities defined at a generic level and one for device specific pm state. The latter would be for things like the door lock information which is very specific to CDs (although not specific to SCSI CDs). Alternatively, even if we can't use these capabilities at the generic pm level, we still need an internal state set of flags because power state stuff traverses the stack and struct device is the only universal object in that stack.

Quoted 2 lines

So I definitely think all of the sdev flags should become either generic or specific flags in device.

Well, the problem is that it is kind of irrelevant to the core whether or not the given device can be powered off. Moreover, the actual meaning of what "power off" means depends on the platform (it may be an individual device state or a power domain state, for instance). Also, the set of available low-power states depends on the platform (or the bus type) and generally cannot be universally represented and there are low-power states that aren't "power off" per se, but still require the device state to be restored when putting it back into full power.

Quoted 5 lines

We've discussed that for a few times and each time we've ended up agreeing that struct device is not the right place to store this information (for example, PCI stores it in struct pci_dev, USB has its own rules etc.).

I'll have a look at the patchset again and see what can be done about this.

Thanks Rafael, and if there is any question/problem, please kindly let me know.

Quoted 2 lines · Aaron Lu

Thanks Rafael, and if there is any question/problem, please kindly let me know.

Well, unfortunately my initial review indicates that the patchset is not quite ready to go upstream yet.

I'll send comments in replies to the individual patches, but overall I can say that at this stage of development, when I look at the patches, it should be clear to me not only what is being changed, but _why_ it is being changed in the first place and, secondly, why it is being changed in this particular way. It's far from that, though.

The first patch in the series doesn't even have a changelog. How the changelog of the second patch is related to it's actual contents is more than quite unclear to me. Etc.

Thanks,
Rafael

Quoted 4 lines · Rafael J. Wysocki

Quoted 2 lines · Aaron Lu

Thanks Rafael, and if there is any question/problem, please kindly let me know.

Quoted 8 lines

Well, unfortunately my initial review indicates that the patchset is not quite ready to go upstream yet.

I'll send comments in replies to the individual patches, but overall I can say that at this stage of development, when I look at the patches, it should be clear to me not only what is being changed, but _why_ it is being changed in the first place and, secondly, why it is being changed in this particular way. It's far from that, though.

I'm adding zero power support for optical disk drive(ZPODD), which is made possible with the newly defined device attention(DA) pin introduced in SATA 3.1 spec.

The idea here is to use runtime pm to achieve this, so I basically did 2 things:
1 Add runtime pm support for ODD;
2 Add power off support for ODD after it is runtime suspended.

Patch 2 is runtime pm support for ODD, the reason it is done this way is discussed here:
http://www.spinics.net/lists/linux-scsi/msg61551.html

The basic idea is, the ODD will be runtime suspended as long as there is nobody using it, that is, no programs opening the block device.

The ODD will be polled periodically, so it will be runtime resumed before checking if there is any events pending and suspended when done.

The only exception is, if we found a disc is just inserted, we will not idle it immediately at the end of the poll, reason explained in another mail.

This is the rational I wrote patch 2, and patch 1 is used by patch 2.

Patch 3 is adding power off support for ODD after it is runtime suspended, the condition is specified in section 15:
ftp://ftp.seagate.com/sff/INF-8090.PDF

That is, for tray type ODD: no media inside and door closed; for slot type ODD: no media inside.

The is the reason sr_suspend is written, for non-ZPODD capable devices, it does nothing; for ZPODD devices, it will check the above condition to see if it is ready to be powered off. The ready_to_power_off flag will be used by ATA layer to decide if power can be removed.

When in powered off state, if user presses the eject button or insert a disc, an ACPI event will be generated and our acpi wake handler will pm_runtime_resume the ODD. And if it is a tray type ODD, its tray should be ejected(need_eject flag) after powered on. This is patch 3.

And patch 4-6 introduces a new sysfs file may_power_off to give user a chance to disable the power off of the device due to various reasons like the ODD claims support of device attention while actually it is broken.

I hope it makes more sense to you now.

Now, James says he doesn't like the way ready_to_power_off is used. Sure enough, it is totally irrelevant to the majority of SCSI devices. It actually is totally irrelevant to everything in the SCSI subsystem except for the sr driver and libata. So I wonder if you have considered any alternative way to address the use case at hand?

That sounds reasonable enough, but the role of the powered_off and need_eject flags could be explained a bit better. In particular, it would

be nice to have explained why they have to be present in struct scsi_device, because they don't seem to be particularly useful for many SCSI devices that aren't CD drives (the need_eject one in particular).

User space has an interface to disable runtime PM of any device and it looks like that interface should be sufficient to disable the feature in question. Why do you think the new interface is needed?

Quoted 5 lines · On Friday 21 September 2012 23:18:27 Rafael J. Wysocki wrote:

Now, James says he doesn't like the way ready_to_power_off is used. Sure enough, it is totally irrelevant to the majority of SCSI devices. It actually is totally irrelevant to everything in the SCSI subsystem except for the sr driver and libata. So I wonder if you have considered any alternative way to address the use case at hand?

Strictly speaking, USB on very modern systems could use it, but doesn't in the current implementation.

Quoted 2 lines

That sounds reasonable enough, but the role of the powered_off and need_eject flags could be explained a bit better. In particular, it would

I think need_eject needs to be renamed. Something like "media_change_detected"

Quoted 3 lines

be nice to have explained why they have to be present in struct scsi_device, because they don't seem to be particularly useful for many SCSI devices that aren't CD drives (the need_eject one in particular).

There are sd devices with removable media.

Quoted 3 lines

User space has an interface to disable runtime PM of any device and it looks like that interface should be sufficient to disable the feature in question. Why do you think the new interface is needed?

Because this is not equivalent to doing no runtime PM at all. SCSI now defines some powersaving states which do not involve powering down and thus losing state.

Quoted 1 line

There are sd devices with removable media.

OK. Does the SCSI layer distinguish them from devices without removable media?

Quoted 7 lines

Quoted 3 lines

User space has an interface to disable runtime PM of any device and it looks like that interface should be sufficient to disable the feature in question. Why do you think the new interface is needed?

Because this is not equivalent to doing no runtime PM at all. SCSI now defines some powersaving states which do not involve powering down and thus losing state.

I see. So the sr's runtime suspend may be useful even without the power-off feature, right?

Quoted 3 lines · Rafael J. Wysocki

Quoted 1 line

There are sd devices with removable media.

OK. Does the SCSI layer distinguish them from devices without removable media?

Yes, it does. struct scsi_device has a .removable member, and the Removable flag is part of the response data to the INQUIRY command (which belongs to the primary command set common to all SCSI devices).

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

User space has an interface to disable runtime PM of any device and it looks like that interface should be sufficient to disable the feature in question. Why do you think the new interface is needed?

Because this is not equivalent to doing no runtime PM at all. SCSI now defines some powersaving states which do not involve powering down and thus losing state.

Quoted 2 lines

I see. So the sr's runtime suspend may be useful even without the power-off feature, right?

Exactly. Even though the drive itself may not be powered off, by putting it into runtime suspend we gain the ability to suspend the ancestor devices.

Quoted 6 lines

Quoted 2 lines

I see. So the sr's runtime suspend may be useful even without the power-off feature, right?

Exactly. Even though the drive itself may not be powered off, by putting it into runtime suspend we gain the ability to suspend the ancestor devices.

OK, so what about using a PM QoS-based approach as described (in general terms) in this message in the "USB ports power off" thread:

http://marc.info/?l=linux-pm&m=134831537224566&w=4

Quoted 11 lines · Rafael J. Wysocki

Quoted 6 lines

Quoted 2 lines

I see. So the sr's runtime suspend may be useful even without the power-off feature, right?

Exactly. Even though the drive itself may not be powered off, by putting it into runtime suspend we gain the ability to suspend the ancestor devices.

OK, so what about using a PM QoS-based approach as described (in general terms) in this message in the "USB ports power off" thread:

http://marc.info/?l=linux-pm&m=134831537224566&w=4

I'm not entirely sure. It may well work out better in this case than in the USB ports case.

For the ZPODD stuff, the userspace control amounts to a single flag ("do not allow zero-power") which can easily be represented as a QoS constraint.

For the USB ports, the situation is more complicated. The decision about whether or not to power-off a port depends not just on the port itself but also on the device plugged into the port, and there's no direct relation between the two in the sysfs device tree. (That could be fixed perhaps by adding symbolic links between them.)

This should be discussed in the USB thread, though...

Quoted 18 lines · Aaron Lu

Quoted 4 lines · Rafael J. Wysocki

Quoted 2 lines · Aaron Lu

Thanks Rafael, and if there is any question/problem, please kindly let me know.

Quoted 8 lines

Well, unfortunately my initial review indicates that the patchset is not quite ready to go upstream yet.

I'll send comments in replies to the individual patches, but overall I can say that at this stage of development, when I look at the patches, it should be clear to me not only what is being changed, but _why_ it is being changed in the first place and, secondly, why it is being changed in this particular way. It's far from that, though.

I'm adding zero power support for optical disk drive(ZPODD), which is made possible with the newly defined device attention(DA) pin introduced in SATA 3.1 spec.

Quoted 8 lines

The idea here is to use runtime pm to achieve this, so I basically did 2 things:
1 Add runtime pm support for ODD;
2 Add power off support for ODD after it is runtime suspended.

Patch 2 is runtime pm support for ODD, the reason it is done this way is discussed here:
http://www.spinics.net/lists/linux-scsi/msg61551.html

Why isn't it explained in the patch changelog, then? People should be able to learn why things are done the way they are done from git logs.

Quoted 5 lines

The basic idea is, the ODD will be runtime suspended as long as there is nobody using it, that is, no programs opening the block device.

The ODD will be polled periodically, so it will be runtime resumed before checking if there is any events pending and suspended when done.

OK. So what happens if we power off the drive via runtime PM. Does it it really make sense to resumie it through polling in that case?

Quoted 11 lines

The only exception is, if we found a disc is just inserted, we will not idle it immediately at the end of the poll, reason explained in another mail.

This is the rational I wrote patch 2, and patch 1 is used by patch 2.

Patch 3 is adding power off support for ODD after it is runtime suspended, the condition is specified in section 15:
ftp://ftp.seagate.com/sff/INF-8090.PDF

Quoted 7 lines

That is, for tray type ODD: no media inside and door closed; for slot type ODD: no media inside.

The is the reason sr_suspend is written, for non-ZPODD capable devices, it does nothing; for ZPODD devices, it will check the above condition to see if it is ready to be powered off. The ready_to_power_off flag will be used by ATA layer to decide if power can be removed.

Now, James says he doesn't like the way ready_to_power_off is used. Sure enough, it is totally irrelevant to the majority of SCSI devices. It actually is totally irrelevant to everything in the SCSI subsystem except for the sr driver and libata. So I wonder if you have considered any alternative way to address the use case at hand?

Quoted 4 lines

When in powered off state, if user presses the eject button or insert a disc, an ACPI event will be generated and our acpi wake handler will pm_runtime_resume the ODD. And if it is a tray type ODD, its tray should be ejected(need_eject flag) after powered on. This is patch 3.

That sounds reasonable enough, but the role of the powered_off and need_eject flags could be explained a bit better. In particular, it would

be nice to have explained why they have to be present in struct scsi_device, because they don't seem to be particularly useful for many SCSI devices that aren't CD drives (the need_eject one in particular).

Quoted 5 lines

And patch 4-6 introduces a new sysfs file may_power_off to give user a chance to disable the power off of the device due to various reasons like the ODD claims support of device attention while actually it is broken.

User space has an interface to disable runtime PM of any device and it looks like that interface should be sufficient to disable the feature in question. Why do you think the new interface is needed?

Quoted 1 line

I hope it makes more sense to you now.

Well, thanks for the explanation. :-)

Rafael

Quoted 31 lines · Rafael J. Wysocki

Quoted 18 lines · Aaron Lu

Quoted 4 lines · Rafael J. Wysocki

Quoted 2 lines · Aaron Lu

Thanks Rafael, and if there is any question/problem, please kindly let me know.

Quoted 8 lines

Well, unfortunately my initial review indicates that the patchset is not quite ready to go upstream yet.

I'll send comments in replies to the individual patches, but overall I can say that at this stage of development, when I look at the patches, it should be clear to me not only what is being changed, but _why_ it is being changed in the first place and, secondly, why it is being changed in this particular way. It's far from that, though.

I'm adding zero power support for optical disk drive(ZPODD), which is made possible with the newly defined device attention(DA) pin introduced in SATA 3.1 spec.

Quoted 8 lines

The idea here is to use runtime pm to achieve this, so I basically did 2 things:
1 Add runtime pm support for ODD;
2 Add power off support for ODD after it is runtime suspended.

Patch 2 is runtime pm support for ODD, the reason it is done this way is discussed here:
http://www.spinics.net/lists/linux-scsi/msg61551.html

Why isn't it explained in the patch changelog, then? People should be able to learn why things are done the way they are done from git logs.

Quoted 8 lines

Quoted 5 lines

The basic idea is, the ODD will be runtime suspended as long as there is nobody using it, that is, no programs opening the block device.

The ODD will be polled periodically, so it will be runtime resumed before checking if there is any events pending and suspended when done.

OK. So what happens if we power off the drive via runtime PM. Does it it really make sense to resumie it through polling in that case?

No, this is the reason I introduced the powered_off flag. If set, the poll will simply return without touching the device.

I've tried to do a disk_block_events call on its suspend callback when it is ready to be powered off, but there is a race that I don't know how to solve:
pm_runtime_suspend disk_events_workfn
scsi_dev_type_suspend sr_block_check_events
sr_suspend cdrom_check_events
disk_block_events cdrom_update_events
(this call waits for all sr_check_events
running events_checking function scsi_autopm_get_device
to return)

Suppose sr_suspend runs first, and then sr_check_events comes in. sr_suspend calls disk_block_events, which waits for sr_check_events, while scsi_autopm_get_device wait for suspend callback to finish, deadlock.

Quoted 29 lines

Quoted 10 lines

The only exception is, if we found a disc is just inserted, we will not idle it immediately at the end of the poll, reason explained in another mail.

This is the rational I wrote patch 2, and patch 1 is used by patch 2.

Patch 3 is adding power off support for ODD after it is runtime suspended, the condition is specified in section 15:
ftp://ftp.seagate.com/sff/INF-8090.PDF

Quoted 7 lines

That is, for tray type ODD: no media inside and door closed; for slot type ODD: no media inside.

The is the reason sr_suspend is written, for non-ZPODD capable devices, it does nothing; for ZPODD devices, it will check the above condition to see if it is ready to be powered off. The ready_to_power_off flag will be used by ATA layer to decide if power can be removed.

Now, James says he doesn't like the way ready_to_power_off is used. Sure enough, it is totally irrelevant to the majority of SCSI devices. It actually is totally irrelevant to everything in the SCSI subsystem except for the sr driver and libata. So I wonder if you have considered any alternative way to address the use case at hand?

Quoted 4 lines

When in powered off state, if user presses the eject button or insert a disc, an ACPI event will be generated and our acpi wake handler will pm_runtime_resume the ODD. And if it is a tray type ODD, its tray should be ejected(need_eject flag) after powered on. This is patch 3.

Quoted 2 lines

That sounds reasonable enough, but the role of the powered_off and need_eject flags could be explained a bit better. In particular, it would

powered_off: set when the device is powered off, clear otherwise.

need_eject:
First consider how the device will be runtime resumed:
1 Some program opens the block device;
2 Events checking poll when it's not powered off yet;
3 User presses the eject button or inserts a disc into the slot when the

device is in powered off state.
And the need_eject flag is for case 3, when the device is in powered off state and user presses the eject button, it will be powered on(through acpi wake notification function) and runtime resumed. In its runtime resume callback, its tray needs to be ejected since user just presses the eject button. The whole process of ZPODD is opaque to the user, he/she doesn't know the ODD lost power so the ODD has to behave exactly like it doesn't lose power.

Hi Oliver,
This flag is really to say the tray needs to be ejected after runtime resumed, it's not that media change detected. It is possible that user ejects the tray without putting any disc inside and simply close the tray, which doesn't qualify a media change event. And if user does put a disc in, the sr_check_events will find that and report the media change event to user space. Agree?

Quoted 3 lines

be nice to have explained why they have to be present in struct scsi_device, because they don't seem to be particularly useful for many SCSI devices that aren't CD drives (the need_eject one in particular).

With your suggestion of pm_platform_power_off_allowed, I suppose powered_off can be eliminated similarly with something like pm_platform_powered_off returning true or false(for ACPI platform, return true when device is in D3 cold state).

And for the need_eject flag, I don't know if there is a better place for it. The acpi wake notification code resides in libata(where we need to record that this resume is due to user presses the eject button and the tray needs to be ejected after resumed), and the runtime resume callback resides in scsi driver(where we actually eject the tray). Ideally, this flag should sit in scsi_cd structure, but libata does not have access to it.

Quoted 55 lines · Aaron Lu

Quoted 31 lines · Rafael J. Wysocki

Quoted 18 lines · Aaron Lu

Quoted 4 lines · Rafael J. Wysocki

Quoted 2 lines · Aaron Lu

Thanks Rafael, and if there is any question/problem, please kindly let me know.

Quoted 8 lines

Well, unfortunately my initial review indicates that the patchset is not quite ready to go upstream yet.

I'll send comments in replies to the individual patches, but overall I can say that at this stage of development, when I look at the patches, it should be clear to me not only what is being changed, but _why_ it is being changed in the first place and, secondly, why it is being changed in this particular way. It's far from that, though.

I'm adding zero power support for optical disk drive(ZPODD), which is made possible with the newly defined device attention(DA) pin introduced in SATA 3.1 spec.

Quoted 8 lines

The idea here is to use runtime pm to achieve this, so I basically did 2 things:
1 Add runtime pm support for ODD;
2 Add power off support for ODD after it is runtime suspended.

Patch 2 is runtime pm support for ODD, the reason it is done this way is discussed here:
http://www.spinics.net/lists/linux-scsi/msg61551.html

Why isn't it explained in the patch changelog, then? People should be able to learn why things are done the way they are done from git logs.

Quoted 8 lines

Quoted 5 lines

The basic idea is, the ODD will be runtime suspended as long as there is nobody using it, that is, no programs opening the block device.

The ODD will be polled periodically, so it will be runtime resumed before checking if there is any events pending and suspended when done.

OK. So what happens if we power off the drive via runtime PM. Does it it really make sense to resumie it through polling in that case?

No, this is the reason I introduced the powered_off flag. If set, the poll will simply return without touching the device.

I've tried to do a disk_block_events call on its suspend callback when it is ready to be powered off, but there is a race that I don't know how to solve:
pm_runtime_suspend disk_events_workfn
scsi_dev_type_suspend sr_block_check_events
sr_suspend cdrom_check_events
disk_block_events cdrom_update_events
(this call waits for all sr_check_events
running events_checking function scsi_autopm_get_device
to return)

Quoted 4 lines

Suppose sr_suspend runs first, and then sr_check_events comes in. sr_suspend calls disk_block_events, which waits for sr_check_events, while scsi_autopm_get_device wait for suspend callback to finish, deadlock.

I need some more time to think about this, stay tuned.

Quoted 30 lines

Quoted 30 lines

Quoted 11 lines

The only exception is, if we found a disc is just inserted, we will not idle it immediately at the end of the poll, reason explained in another mail.

This is the rational I wrote patch 2, and patch 1 is used by patch 2.

Patch 3 is adding power off support for ODD after it is runtime suspended, the condition is specified in section 15:
ftp://ftp.seagate.com/sff/INF-8090.PDF

Quoted 7 lines

That is, for tray type ODD: no media inside and door closed; for slot type ODD: no media inside.

The is the reason sr_suspend is written, for non-ZPODD capable devices, it does nothing; for ZPODD devices, it will check the above condition to see if it is ready to be powered off. The ready_to_power_off flag will be used by ATA layer to decide if power can be removed.

Now, James says he doesn't like the way ready_to_power_off is used. Sure enough, it is totally irrelevant to the majority of SCSI devices. It actually is totally irrelevant to everything in the SCSI subsystem except for the sr driver and libata. So I wonder if you have considered any alternative way to address the use case at hand?

Quoted 4 lines

When in powered off state, if user presses the eject button or insert a disc, an ACPI event will be generated and our acpi wake handler will pm_runtime_resume the ODD. And if it is a tray type ODD, its tray should be ejected(need_eject flag) after powered on. This is patch 3.

Quoted 4 lines

Quoted 2 lines

That sounds reasonable enough, but the role of the powered_off and need_eject flags could be explained a bit better. In particular, it would

powered_off: set when the device is powered off, clear otherwise.

That's pretty clear, but I think this flag should be called no_polling or something like this, because that's what it means to the SCSI layer.

Quoted 14 lines

need_eject:
First consider how the device will be runtime resumed:
1 Some program opens the block device;
2 Events checking poll when it's not powered off yet;
3 User presses the eject button or inserts a disc into the slot when the
device is in powered off state.
And the need_eject flag is for case 3, when the device is in powered off state and user presses the eject button, it will be powered on(through acpi wake notification function) and runtime resumed. In its runtime resume callback, its tray needs to be ejected since user just presses the eject button. The whole process of ZPODD is opaque to the user, he/she doesn't know the ODD lost power so the ODD has to behave exactly like it doesn't lose power.

Do you think it can be useful for other types of devices, not necessarily handled through ACPI?

Quoted 13 lines

Hi Oliver,
This flag is really to say the tray needs to be ejected after runtime resumed, it's not that media change detected. It is possible that user ejects the tray without putting any disc inside and simply close the tray, which doesn't qualify a media change event. And if user does put a disc in, the sr_check_events will find that and report the media change event to user space. Agree?

Quoted 3 lines

be nice to have explained why they have to be present in struct scsi_device, because they don't seem to be particularly useful for many SCSI devices that aren't CD drives (the need_eject one in particular).

Quoted 4 lines

With your suggestion of pm_platform_power_off_allowed, I suppose powered_off can be eliminated similarly with something like pm_platform_powered_off returning true or false(for ACPI platform, return true when device is in D3 cold state).

I'm currently thinking that using PM QoS may be a better approach here.

Quoted 8 lines

And for the need_eject flag, I don't know if there is a better place for it. The acpi wake notification code resides in libata(where we need to record that this resume is due to user presses the eject button and the tray needs to be ejected after resumed), and the runtime resume callback resides in scsi driver(where we actually eject the tray). Ideally, this flag should sit in scsi_cd structure, but libata does not have access to it.

Yes, that's the problem that James mentioned.

Thanks,
Rafael

Quoted 56 lines · Rafael J. Wysocki

Quoted 55 lines · Aaron Lu

Quoted 31 lines · Rafael J. Wysocki

Quoted 18 lines · Aaron Lu

Quoted 4 lines · Rafael J. Wysocki

Quoted 2 lines · Aaron Lu

Thanks Rafael, and if there is any question/problem, please kindly let me know.

Quoted 8 lines

Well, unfortunately my initial review indicates that the patchset is not quite ready to go upstream yet.

I'll send comments in replies to the individual patches, but overall I can say that at this stage of development, when I look at the patches, it should be clear to me not only what is being changed, but _why_ it is being changed in the first place and, secondly, why it is being changed in this particular way. It's far from that, though.

I'm adding zero power support for optical disk drive(ZPODD), which is made possible with the newly defined device attention(DA) pin introduced in SATA 3.1 spec.

Quoted 8 lines

The idea here is to use runtime pm to achieve this, so I basically did 2 things:
1 Add runtime pm support for ODD;
2 Add power off support for ODD after it is runtime suspended.

Patch 2 is runtime pm support for ODD, the reason it is done this way is discussed here:
http://www.spinics.net/lists/linux-scsi/msg61551.html

Why isn't it explained in the patch changelog, then? People should be able to learn why things are done the way they are done from git logs.

Quoted 8 lines

Quoted 5 lines

The basic idea is, the ODD will be runtime suspended as long as there is nobody using it, that is, no programs opening the block device.

The ODD will be polled periodically, so it will be runtime resumed before checking if there is any events pending and suspended when done.

OK. So what happens if we power off the drive via runtime PM. Does it it really make sense to resumie it through polling in that case?

No, this is the reason I introduced the powered_off flag. If set, the poll will simply return without touching the device.

I've tried to do a disk_block_events call on its suspend callback when it is ready to be powered off, but there is a race that I don't know how to solve:
pm_runtime_suspend disk_events_workfn
scsi_dev_type_suspend sr_block_check_events
sr_suspend cdrom_check_events
disk_block_events cdrom_update_events
(this call waits for all sr_check_events
running events_checking function scsi_autopm_get_device
to return)