[PATCH v7 0/6] ZPODD patches

[PATCH v7 0/6] ZPODD patches

[Aaron Lu]

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(-)

-- 
1.7.12.21.g871e293

[PATCH v7 1/6] block: genhd: add an interface to set disk poll interval

[Aaron Lu]

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 block/genhd.c         | 23 +++++++++++++++++------
 include/linux/genhd.h |  1 +
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index cac7366..4244256 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -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;
 }
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 4f440b3..63409e5 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -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);
-- 
1.7.12.21.g871e293

Re: [PATCH v7 1/6] block: genhd: add an interface to set disk poll interval

[Rafael J. Wysocki]

Please add a changelog explaining who's going to use the new interface, in
addition to the original user of that code, and why it is exported.

Thanks,
Rafael


On Wednesday, September 12, 2012, Aaron Lu wrote:
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  block/genhd.c         | 23 +++++++++++++++++------
>  include/linux/genhd.h |  1 +
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index cac7366..4244256 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -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;
>  }
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index 4f440b3..63409e5 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -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);
> 

[PATCH v7 2/6] scsi: sr: support runtime pm

[Aaron Lu]

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 <aaron.lu@intel.com>
---
 drivers/scsi/sr.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 5fc97d2..7a8222f 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -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>
@@ -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;
 
+ 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) {
@@ -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);
+
 	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);
 
 	sdev_printk(KERN_DEBUG, sdev,
 		    "Attached scsi CD-ROM %s\n", cd->cdi.name);
+
+	/* enable runtime pm */
+	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 */
+	scsi_autopm_get_device(cd->device);
+
 	blk_queue_prep_rq(cd->device->request_queue, scsi_prep_fn);
 	del_gendisk(cd->disk);
 
-- 
1.7.12.21.g871e293

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Rafael J. Wysocki]

On Wednesday, September 12, 2012, Aaron Lu wrote:
> 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?

> 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?

> Based on ideas of Alan Stern and Oliver Neukum.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  drivers/scsi/sr.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 5fc97d2..7a8222f 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -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>
> @@ -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

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

without the new label?

>  
> + 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) {
> @@ -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?

>  	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?

>  
>  	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.

> +	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).

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

Thanks,
Rafael

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Alan Stern]

On Thu, 20 Sep 2012, Rafael J. Wysocki wrote:

> On Wednesday, September 12, 2012, Aaron Lu wrote:
> > 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?

Aaron did not make it clear in this patch description, although it may 
have been mentioned in the overall 0/6 description.  "ODD" stands for 
"Optical Disc Drive" -- in other words, a CD or DVD drive.  Once you 
know this, the relation is clear: sr is the SCSI driver for CD/DVD 
drives.

Alan Stern

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Aaron Lu]

On Thu, Sep 20, 2012 at 10:48:10PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, September 12, 2012, Aaron Lu wrote:
> > 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.

> 
> > 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.

> 
> 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.

> 
> > Based on ideas of Alan Stern and Oliver Neukum.
> > 
> > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > ---
> >  drivers/scsi/sr.c | 29 +++++++++++++++++++++++++----
> >  1 file changed, 25 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> > index 5fc97d2..7a8222f 100644
> > --- a/drivers/scsi/sr.c
> > +++ b/drivers/scsi/sr.c
> > @@ -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>
> > @@ -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
> 
> > +	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.

> 
> >  
> > + 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) {
> > @@ -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?

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.

> 
> >  	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.

> 
> >  
> >  	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.

> 
> > +	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).

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

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

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Rafael J. Wysocki]

On Friday, September 21, 2012, Aaron Lu wrote:
> On Thu, Sep 20, 2012 at 10:48:10PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, September 12, 2012, Aaron Lu wrote:
> > > 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. :-)

> > > 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.

> > 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"?

> 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.

> > > Based on ideas of Alan Stern and Oliver Neukum.
> > > 
> > > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > > ---
> > >  drivers/scsi/sr.c | 29 +++++++++++++++++++++++++----
> > >  1 file changed, 25 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> > > index 5fc97d2..7a8222f 100644
> > > --- a/drivers/scsi/sr.c
> > > +++ b/drivers/scsi/sr.c
> > > @@ -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>
> > > @@ -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
> > 
> > > +	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.
> 
> > 
> > >  
> > > + 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) {
> > > @@ -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?
> 
> 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.

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?

> > >  	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.

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?

> > >  
> > >  	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.
> 
> > 
> > > +	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).
> 
> 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).

Thanks,
Rafael

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Aaron Lu]

On Fri, Sep 21, 2012 at 10:49:32PM +0200, Rafael J. Wysocki wrote:
> On Friday, September 21, 2012, Aaron Lu wrote:
> > On Thu, Sep 20, 2012 at 10:48:10PM +0200, Rafael J. Wysocki wrote:
> > > On Wednesday, September 12, 2012, Aaron Lu wrote:
> > > > 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. :-)
> 
> > > > 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.
> 
> > > 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.

> 
> > 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.

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.

> 
> > > > Based on ideas of Alan Stern and Oliver Neukum.
> > > > 
> > > > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > > > ---
> > > >  drivers/scsi/sr.c | 29 +++++++++++++++++++++++++----
> > > >  1 file changed, 25 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> > > > index 5fc97d2..7a8222f 100644
> > > > --- a/drivers/scsi/sr.c
> > > > +++ b/drivers/scsi/sr.c
> > > > @@ -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>
> > > > @@ -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
> > > 
> > > > +	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.
> > 
> > > 
> > > >  
> > > > + 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) {
> > > > @@ -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?
> > 
> > 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.
> 
> 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.

> 
> > > >  	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.
> 
> 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?

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.

> 
> 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.

> 
> > > >  
> > > >  	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.
> > 
> > > 
> > > > +	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).
> > 
> > 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).

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.

-Aaron

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Rafael J. Wysocki]

On Monday, September 24, 2012, Aaron Lu wrote:
> On Fri, Sep 21, 2012 at 10:49:32PM +0200, Rafael J. Wysocki wrote:
> > On Friday, September 21, 2012, Aaron Lu wrote:
> > > On Thu, Sep 20, 2012 at 10:48:10PM +0200, Rafael J. Wysocki wrote:
> > > > On Wednesday, September 12, 2012, Aaron Lu wrote:
> > > > > 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. :-)
> > 
> > > > > 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.
> > 
> > > > 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.
> 
> > 
> > > 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.
> 
> 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. :-)

> > > > > Based on ideas of Alan Stern and Oliver Neukum.
> > > > > 
> > > > > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > > > > ---
> > > > >  drivers/scsi/sr.c | 29 +++++++++++++++++++++++++----
> > > > >  1 file changed, 25 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> > > > > index 5fc97d2..7a8222f 100644
> > > > > --- a/drivers/scsi/sr.c
> > > > > +++ b/drivers/scsi/sr.c
> > > > > @@ -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>
> > > > > @@ -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
> > > > 
> > > > > +	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.
> > > 
> > > > 
> > > > >  
> > > > > + 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) {
> > > > > @@ -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?
> > > 
> > > 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.
> > 
> > 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?

> 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?

> > > > >  	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.
> > 
> > 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?
> 
> 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?

> 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.
> 
> > 
> > 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.

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.

> > > > >  
> > > > >  	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.
> > > 
> > > > 
> > > > > +	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).
> > > 
> > > 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).
> 
> 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

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Aaron Lu]

On Mon, Sep 24, 2012 at 02:55:31PM +0200, Rafael J. Wysocki wrote:
> On Monday, September 24, 2012, Aaron Lu wrote:
> > On Fri, Sep 21, 2012 at 10:49:32PM +0200, Rafael J. Wysocki wrote:
> > > On Friday, September 21, 2012, Aaron Lu wrote:
> > > > On Thu, Sep 20, 2012 at 10:48:10PM +0200, Rafael J. Wysocki wrote:
> > > > > On Wednesday, September 12, 2012, Aaron Lu wrote:
> > > > > > 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. :-)
> > > 
> > > > > > 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.
> > > 
> > > > > 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.
> > 
> > > 
> > > > 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.
> > 
> > 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. :-)
> 
> > > > > > Based on ideas of Alan Stern and Oliver Neukum.
> > > > > > 
> > > > > > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > > > > > ---
> > > > > >  drivers/scsi/sr.c | 29 +++++++++++++++++++++++++----
> > > > > >  1 file changed, 25 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> > > > > > index 5fc97d2..7a8222f 100644
> > > > > > --- a/drivers/scsi/sr.c
> > > > > > +++ b/drivers/scsi/sr.c
> > > > > > @@ -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>
> > > > > > @@ -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
> > > > > 
> > > > > > +	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.
> > > > 
> > > > > 
> > > > > >  
> > > > > > + 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) {
> > > > > > @@ -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?
> > > > 
> > > > 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.
> > > 
> > > 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);

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.

> 
> 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.

> 
> > 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.

> 
> > > > > >  	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.
> > > 
> > > 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?
> > 
> > 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.

> 
> > 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.
> > 
> > > 
> > > 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.
> 
> 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.

Yes, it's necessary.

Thanks,
Aaron

> 
> > > > > >  
> > > > > >  	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.
> > > > 
> > > > > 
> > > > > > +	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).
> > > > 
> > > > 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).
> > 
> > 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

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Rafael J. Wysocki]

On Monday, September 24, 2012, Aaron Lu wrote:
> On Mon, Sep 24, 2012 at 02:55:31PM +0200, Rafael J. Wysocki wrote:
> > On Monday, September 24, 2012, Aaron Lu wrote:
> > > On Fri, Sep 21, 2012 at 10:49:32PM +0200, Rafael J. Wysocki wrote:
> > > > On Friday, September 21, 2012, Aaron Lu wrote:
> > > > > On Thu, Sep 20, 2012 at 10:48:10PM +0200, Rafael J. Wysocki wrote:
> > > > > > On Wednesday, September 12, 2012, Aaron Lu wrote:

[...] 

> > > > > > >  	/* 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.
> > > > > > 
> > > > > > 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.
> > > > 
> > > > 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);
> 
> 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.

Oh, I got the condition backwards.  I meant:

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

which should be equivalent to your original code (if I'm not mistaken again).

> > 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.

OK

> > > 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.

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.

> > > > > > >  	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.
> > > > 
> > > > 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?
> > > 
> > > 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.

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,
Rafael

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Aaron Lu]

On Mon, Sep 24, 2012 at 11:40:18PM +0200, Rafael J. Wysocki wrote:
> On Monday, September 24, 2012, Aaron Lu wrote:
> > On Mon, Sep 24, 2012 at 02:55:31PM +0200, Rafael J. Wysocki wrote:
> > > 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.
> 
> OK

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

> 
> > > > 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.
> 
> 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.

> 
> > > > 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.
> 
> 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?

Thanks,
Aaron

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Rafael J. Wysocki]

On Tuesday, September 25, 2012, Aaron Lu wrote:
> On Mon, Sep 24, 2012 at 11:40:18PM +0200, Rafael J. Wysocki wrote:
> > On Monday, September 24, 2012, Aaron Lu wrote:
> > > On Mon, Sep 24, 2012 at 02:55:31PM +0200, Rafael J. Wysocki wrote:
> > > > 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.
> > 
> > OK
> 
> Looks like this is not the right thing to do, if I lock the door user
> will be confused.
> 
> > 
> > > > > 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.
> > 
> > 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.

Good!

> > > > > 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.
> > 
> > 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?

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

Thanks,
Rafael

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Aaron Lu]

On Tue, Sep 25, 2012 at 01:47:52PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, September 25, 2012, Aaron Lu wrote:
> > 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.

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?

Thanks,
Aaron

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Oliver Neukum]

On Tuesday 25 September 2012 22:20:21 Aaron Lu wrote:
> On Tue, Sep 25, 2012 at 01:47:52PM +0200, Rafael J. Wysocki wrote:
> > On Tuesday, September 25, 2012, Aaron Lu wrote:
> > > 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.
> 
> 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.

> 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().

	Regards
		Oliver

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

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Aaron Lu]

On 09/25/2012 10:23 PM, Oliver Neukum wrote:
> On Tuesday 25 September 2012 22:20:21 Aaron Lu wrote:
>> On Tue, Sep 25, 2012 at 01:47:52PM +0200, Rafael J. Wysocki wrote:
>>> On Tuesday, September 25, 2012, Aaron Lu wrote:
>>>> 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.
>>
>> 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.

> 
>> 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.

> 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?

Thanks,
Aaron

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Rafael J. Wysocki]

On Tuesday, September 25, 2012, Aaron Lu wrote:
> On 09/25/2012 10:23 PM, Oliver Neukum wrote:
> > On Tuesday 25 September 2012 22:20:21 Aaron Lu wrote:
> >> On Tue, Sep 25, 2012 at 01:47:52PM +0200, Rafael J. Wysocki wrote:
> >>> On Tuesday, September 25, 2012, Aaron Lu wrote:
> >>>> 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.
> >>
> >> 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.
> 
> > 
> >> 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.

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

> > 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.

> 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?

Rafael

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Aaron Lu]

On Tue, Sep 25, 2012 at 11:45:34PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, September 25, 2012, Aaron Lu wrote:
> > On 09/25/2012 10:23 PM, Oliver Neukum wrote:
> > > On Tuesday 25 September 2012 22:20:21 Aaron Lu wrote:
> > >> On Tue, Sep 25, 2012 at 01:47:52PM +0200, Rafael J. Wysocki wrote:
> > >>> On Tuesday, September 25, 2012, Aaron Lu wrote:
> > >>>> 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.
> > >>
> > >> 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.
> > 
> > > 
> > >> 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.
> 
> 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).

> 
> > > 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.

Sounds good.

> 
> > 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.

> 
> 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? I'm not sure about this events passing thing, as
in that case, I will need to add code to listen to a socket in sr.

Thanks,
Aaron

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Rafael J. Wysocki]

On Wednesday, September 26, 2012, Aaron Lu wrote:
> On Tue, Sep 25, 2012 at 11:45:34PM +0200, Rafael J. Wysocki wrote:
> > On Tuesday, September 25, 2012, Aaron Lu wrote:
> > > On 09/25/2012 10:23 PM, Oliver Neukum wrote:
> > > > On Tuesday 25 September 2012 22:20:21 Aaron Lu wrote:
> > > >> On Tue, Sep 25, 2012 at 01:47:52PM +0200, Rafael J. Wysocki wrote:
> > > >>> On Tuesday, September 25, 2012, Aaron Lu wrote:
> > > >>>> 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.
> > > >>
> > > >> 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.
> > > 
> > > > 
> > > >> 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.
> > 
> > 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).
> 
> > 
> > > > 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.
> 
> Sounds good.
> 
> > 
> > > 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.

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.

> > 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?

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.

Thanks,
Rafael

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Aaron Lu]

On Wed, Sep 26, 2012 at 7:18 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, September 26, 2012, Aaron Lu wrote:
>> On Tue, Sep 25, 2012 at 11:45:34PM +0200, Rafael J. Wysocki wrote:
>> > On Tuesday, September 25, 2012, Aaron Lu wrote:
>> > > On 09/25/2012 10:23 PM, Oliver Neukum wrote:
>> > > > On Tuesday 25 September 2012 22:20:21 Aaron Lu wrote:
>> > > >> On Tue, Sep 25, 2012 at 01:47:52PM +0200, Rafael J. Wysocki wrote:
>> > > >>> On Tuesday, September 25, 2012, Aaron Lu wrote:
>> > > >>>> 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.
>> > > >>
>> > > >> 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.
>> > >
>> > > >
>> > > >> 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.
>> >
>> > 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).
>>
>> >
>> > > > 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.
>>
>> Sounds good.
>>
>> >
>> > > 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.
>
> 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.

Cool, thanks.

>
>> > 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?
>
> 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.

Maybe I didn't explain it clearly. The "user pressed the eject button"
is reported
by ACPI through GPE, while the events_checking poll sends a command to the
device to let it report events like media_change, etc.

And the events is reported to user space, that doesn't seem can help us in
this case.

Thanks,
Aaron

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Oliver Neukum]

On Tuesday 25 September 2012 22:46:06 Aaron Lu wrote:
> On 09/25/2012 10:23 PM, Oliver Neukum wrote:

> >> 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.

Hooks can be called with the necessary parameters. I suggest
a triplett of medium presence, tray state and door lock state.
That should cover most types of drives.

> > PS: Are you sure sr_suspend() handles DVD-RAMs correctly?
> 
> No. Is there a spec for it?

Mount Fuji I presume.

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

There are two different issues. sr handles some different devices:
CD/DVD/BD-ROMs, -writers and -RAMs. For those you can have different
code paths in sr. That is no problem at all.

In addition devices can be attached by different hardware. In fact
the same drive can be attached in a USB enclosure or by SATA.
>From the perspective of power management they are no longer
the same device.

Those are best handled in callbacks and limited use of special cases in
sr.

> 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?

No. It would badly harm usb-storage.
You need to leave paths open for other device types.

	Regards
		Oliver

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Oliver Neukum]

On Tuesday 25 September 2012 16:01:35 Aaron Lu wrote:
> On Mon, Sep 24, 2012 at 11:40:18PM +0200, Rafael J. Wysocki wrote:
> > On Monday, September 24, 2012, Aaron Lu wrote:
> > > 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.

> 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

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Aaron Lu]

On 09/27/2012 06:46 PM, Oliver Neukum wrote:
> On Tuesday 25 September 2012 16:01:35 Aaron Lu wrote:
>> On Mon, Sep 24, 2012 at 11:40:18PM +0200, Rafael J. Wysocki wrote:
>>> On Monday, September 24, 2012, Aaron Lu wrote:
>>>> 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.

The SATA spec says the device attention pin shall assert when:
- For tray type ODD, its front panel button is released;
- For slot type ODD, media is inserted.

I've a slot type ODD which has a eject button. The notification will be
sent when a disc is inserted, but not when the eject button is pressed,
and this doesn't violate the spec.

But if we disable the poll for disc changes, we will lose an event when
the disc is ejected by the eject button(the device attention pin shall
not trigger this time). I suppose this is a problem?

I think the device attention scheme is not designed to do this job,
while SATA asynchronous notification is.

> 
>> 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.

That may depend on the ODD's capability. For the slot type ODD I
mentioned above, we will not know when the disc is gone.

Thanks,
Aaron

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

[PATCH v7 3/6] scsi: sr: support zero power ODD(ZPODD)

[Aaron Lu]

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 <aaron.lu@intel.com>
---
 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(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 902b5a4..9aca057 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -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)
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 7a8222f..ef72682 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -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,
 };
@@ -172,6 +176,52 @@ static void scsi_cd_put(struct scsi_cd *cd)
 	mutex_unlock(&sr_ref_mutex);
 }
 
+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);
diff --git a/drivers/scsi/sr.h b/drivers/scsi/sr.h
index 37c8f6b..1c84537 100644
--- a/drivers/scsi/sr.h
+++ b/drivers/scsi/sr.h
@@ -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 */
diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
index a3911c3..c1275f6 100644
--- a/drivers/scsi/sr_ioctl.c
+++ b/drivers/scsi/sr_ioctl.c
@@ -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)
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 9895f69..da5c86f 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -156,6 +156,9 @@ struct scsi_device {
 	unsigned is_visible:1;	/* is the device visible in sysfs */
 	unsigned can_power_off:1; /* Device supports runtime power off */
 	unsigned wce_default_on:1;	/* Cache is ON by default */
+	unsigned need_eject:1;	/* Need eject the tray when wakes up */
+	unsigned ready_to_power_off:1;	/* Device is ready to be powered off */
+	unsigned powered_off:1;	/* Device is powered off */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */
-- 
1.7.12.21.g871e293

Re: [PATCH v7 3/6] scsi: sr: support zero power ODD(ZPODD)

[Rafael J. Wysocki]

On Wednesday, September 12, 2012, Aaron Lu wrote:
> 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.

Well, so I agree with James that the PM flags in struct scsi_device are
kind of odd and it would be better to put them somewhere else.

I'd probably prefer them to sit in the ACPI layer with some helper functions
allowing the sr driver to manipulate them, but that depends on the resulting
code complexity.

> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  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(-)
> 
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index 902b5a4..9aca057 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -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)
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 7a8222f..ef72682 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -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,
>  };
> @@ -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?

> +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);
> diff --git a/drivers/scsi/sr.h b/drivers/scsi/sr.h
> index 37c8f6b..1c84537 100644
> --- a/drivers/scsi/sr.h
> +++ b/drivers/scsi/sr.h
> @@ -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 */
> diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
> index a3911c3..c1275f6 100644
> --- a/drivers/scsi/sr_ioctl.c
> +++ b/drivers/scsi/sr_ioctl.c
> @@ -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)
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 9895f69..da5c86f 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -156,6 +156,9 @@ struct scsi_device {
>  	unsigned is_visible:1;	/* is the device visible in sysfs */
>  	unsigned can_power_off:1; /* Device supports runtime power off */
>  	unsigned wce_default_on:1;	/* Cache is ON by default */
> +	unsigned need_eject:1;	/* Need eject the tray when wakes up */
> +	unsigned ready_to_power_off:1;	/* Device is ready to be powered off */
> +	unsigned powered_off:1;	/* Device is powered off */
>  
>  	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
>  	struct list_head event_list;	/* asserted events */

Thanks,
Rafael

Re: [PATCH v7 3/6] scsi: sr: support zero power ODD(ZPODD)

[Aaron Lu]

On Fri, Sep 21, 2012 at 12:07:23AM +0200, Rafael J. Wysocki wrote:
> On Wednesday, September 12, 2012, Aaron Lu wrote:
> >  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,
> >  };
> > @@ -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,

Yes.

> 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;

Thanks,
Aaron

Re: [PATCH v7 3/6] scsi: sr: support zero power ODD(ZPODD)

[Rafael J. Wysocki]

On Friday, September 21, 2012, Aaron Lu wrote:
> On Fri, Sep 21, 2012 at 12:07:23AM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, September 12, 2012, Aaron Lu wrote:
> > >  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,
> > >  };
> > > @@ -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,
> 
> Yes.
> 
> > 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.

On a slightly related note, suppose that we have an "enabled" bit in flags
in struct acpi_device_power_state and suppose that we have a helper
function pm_platform_power_off_allowed(dev, bool), such that if the driver
(or subsystem) does pm_platform_power_off_allowed(dev, false) and the
platform is ACPI, the "enabled" bit for the D3cold state will be cleared.
Then, the ACPI device PM routines will never use D3cold as the device
power state.

In that case, it seems, you won't need the "ready_to_power_off" flag in struct
scsi_device any more.

Thanks,
Rafael

Re: [PATCH v7 3/6] scsi: sr: support zero power ODD(ZPODD)

[Aaron Lu]

On 09/22/2012 05:02 AM, Rafael J. Wysocki wrote:
> On Friday, September 21, 2012, Aaron Lu wrote:
>> On Fri, Sep 21, 2012 at 12:07:23AM +0200, Rafael J. Wysocki wrote:
>>> On Wednesday, September 12, 2012, Aaron Lu wrote:
>>>>  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,
>>>>  };
>>>> @@ -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,
>>
>> Yes.
>>
>>> 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.

Does the following change look OK?

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index dc0ad85..1fb7ccc 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -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));
 		if (err == -EAGAIN)
 			pm_schedule_suspend(dev, jiffies_to_msecs(
 				round_jiffies_up_relative(HZ/10)));
@@ -151,6 +159,7 @@ static int scsi_runtime_suspend(struct device *dev)
 
 	/* Insert hooks here for targets, hosts, and transport classes */
 
+out:
 	return err;
 }
 
@@ -159,11 +168,17 @@ static int scsi_runtime_resume(struct device *dev)
 	int err = 0;
 
 	dev_dbg(dev, "scsi_runtime_resume\n");
-	if (scsi_is_sdev_device(dev))
+	if (scsi_is_sdev_device(dev)) {
+		err = pm_generic_runtime_resume(dev);
+		if (err)
+			goto out;
+
 		err = scsi_dev_type_resume(dev);
+	}
 
 	/* Insert hooks here for targets, hosts, and transport classes */
 
+out:
 	return err;
 }
 

And I'll define runtime callbacks for sr and sd.

Thanks,
Aaron

Re: [PATCH v7 3/6] scsi: sr: support zero power ODD(ZPODD)

[Alan Stern]

On Thu, 27 Sep 2012, Aaron Lu wrote:

> > 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?
> 
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index dc0ad85..1fb7ccc 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -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));
>  		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.

Alan Stern

Re: [PATCH v7 3/6] scsi: sr: support zero power ODD(ZPODD)

[Aaron Lu]

On 09/27/2012 10:42 PM, Alan Stern wrote:
> On Thu, 27 Sep 2012, Aaron Lu wrote:
> 
>>> 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?
>>
>> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
>> index dc0ad85..1fb7ccc 100644
>> --- a/drivers/scsi/scsi_pm.c
>> +++ b/drivers/scsi/scsi_pm.c
>> @@ -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));
>>  		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.

Thanks,
Aaron

Re: [PATCH v7 3/6] scsi: sr: support zero power ODD(ZPODD)

[Rafael J. Wysocki]

On Thursday, September 27, 2012, Aaron Lu wrote:
> On 09/27/2012 10:42 PM, Alan Stern wrote:
> > On Thu, 27 Sep 2012, Aaron Lu wrote:
> > 
> >>> 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?
> >>
> >> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> >> index dc0ad85..1fb7ccc 100644
> >> --- a/drivers/scsi/scsi_pm.c
> >> +++ b/drivers/scsi/scsi_pm.c
> >> @@ -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));
> >>  		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.

Well, actually, I meant something different.

The above patch would make it possible for drivers to provide runtime PM
callbacks through dev->driver.pm, but drv->suspend and drv->resume would
still be used for system suspend/resume (and hibernation).

The transition to struct dev_pm_ops I meant, however, would be to start using
callbacks from dev->driver.pm for _all_ device PM operations by default and
fall back to drv->suspend and drv->resume for the drivers whose dev->driver.pm
is NULL (along the lines of what PCI does).  The next step would be to
convert all drivers to use dev->driver.pm only.

So I wouldn't like any drivers to use dev->driver.pm callbacks for some
operations and drv->suspend and drv->resume for the remaining ones at the
same time.

Thanks,
Rafael

Re: [PATCH v7 3/6] scsi: sr: support zero power ODD(ZPODD)

[Jeff Garzik]

On 09/12/2012 04:29 AM, Aaron Lu wrote:
> 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 <aaron.lu@intel.com>
> ---
>   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(-)
>
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index 902b5a4..9aca057 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -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);
> +	}
>   }
>

these bits are Acked-by: Jeff Garzik <jgarzik@redhat.com>

but obviously that is contingent upon acceptance of the upper level SCSI 
and ACPI changes.

[PATCH v7 4/6] scsi: pm: add may_power_off flag

[Aaron Lu]

Add a new flag may_power_off for scsi device, it gives the user a chance
to control when the device is runtime suspended, can we remove its power
if possible.

And if the device can be powered off(reflected by can_power_off flag,
determined by individual driver), create a sysfs entry named
may_power_off to let user control the flag.

When user changes this flag through sysfs entry and if the device is
already runtime suspended, runtime resume it so that it can respect this
flag next time it is runtime suspended.

I'm planning using this flag for sr and sd.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/scsi/scsi_sysfs.c  | 37 ++++++++++++++++++++++++++++++++++++-
 include/scsi/scsi_device.h |  1 +
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 093d4f6..8c8efd3 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -509,6 +509,7 @@ sdev_store_##field (struct device *dev, struct device_attribute *attr,	\
 	return ret;							\
 }									\
 static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, sdev_show_##field, sdev_store_##field);
+#endif
 
 /*
  * scsi_sdev_check_buf_bit: return 0 if buf is "0", return 1 if buf is "1",
@@ -526,7 +527,7 @@ static int scsi_sdev_check_buf_bit(const char *buf)
 	} else
 		return -EINVAL;
 }
-#endif
+
 /*
  * Create the actual show/store functions and data structures.
  */
@@ -860,6 +861,37 @@ static struct device_attribute sdev_attr_queue_type_rw =
 	__ATTR(queue_type, S_IRUGO | S_IWUSR, show_queue_type_field,
 	       sdev_store_queue_type_rw);
 
+static ssize_t
+sdev_show_may_power_off(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct scsi_device *sdev;
+	sdev = to_scsi_device(dev);
+	return snprintf (buf, 20, "%d\n", sdev->may_power_off);
+}
+
+static ssize_t
+sdev_store_may_power_off(struct device *dev, struct device_attribute *attr,
+			 const char *buf, size_t count)
+{
+	int ret;
+	struct scsi_device *sdev;
+
+	ret = scsi_sdev_check_buf_bit(buf);
+	if (ret >= 0) {
+		sdev = to_scsi_device(dev);
+		if (sdev->may_power_off != ret) {
+			sdev->may_power_off = ret;
+			if (pm_runtime_suspended(dev))
+				pm_runtime_resume(dev);
+		}
+		ret = count;
+	}
+	return ret;
+}
+static DEVICE_ATTR(may_power_off, S_IRUGO | S_IWUSR,
+			sdev_show_may_power_off, sdev_store_may_power_off);
+
 /**
  * scsi_sysfs_add_sdev - add scsi device to sysfs
  * @sdev:	scsi_device to add
@@ -950,6 +982,9 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 		}
 	}
 
+	if (sdev->can_power_off)
+		device_create_file(&sdev->sdev_gendev, &dev_attr_may_power_off);
+
 	return error;
 }
 
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index da5c86f..4712aa1 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -159,6 +159,7 @@ struct scsi_device {
 	unsigned need_eject:1;	/* Need eject the tray when wakes up */
 	unsigned ready_to_power_off:1;	/* Device is ready to be powered off */
 	unsigned powered_off:1;	/* Device is powered off */
+	unsigned may_power_off:1; /* Power off is allowed by user */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */
-- 
1.7.12.21.g871e293

[PATCH v7 5/6] scsi: sr: use may_power_off

[Aaron Lu]

If may_power_off is 0, we do not check if it is ready to be powered off
in its suspend callback.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/scsi/sr.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index ef72682..4c1a182 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -181,7 +181,7 @@ 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)
+	if (!cd->device->may_power_off)
 		return 0;
 
 	/* See if we can power off this ZPODD device */
@@ -786,6 +786,10 @@ static int sr_probe(struct device *dev)
 	sdev_printk(KERN_DEBUG, sdev,
 		    "Attached scsi CD-ROM %s\n", cd->cdi.name);
 
+	/* By default, we allow power off of ZPODD */
+	if (cd->device->can_power_off)
+		cd->device->may_power_off = 1;
+
 	/* enable runtime pm */
 	scsi_autopm_put_device(cd->device);
 
-- 
1.7.12.21.g871e293

[PATCH v7 6/6] libata: acpi: respect may_power_off flag

[Aaron Lu]

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 <aaron.lu@intel.com>
---
 drivers/ata/libata-acpi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 9aca057..24347e0 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -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 */
@@ -868,9 +869,11 @@ void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
 			continue;
 
 		if (state.event != PM_EVENT_ON) {
+			power_off_allowed = dev->sdev->ready_to_power_off &&
+					    dev->sdev->may_power_off;
 			acpi_state = acpi_pm_device_sleep_state(
 					&dev->sdev->sdev_gendev, NULL,
-					dev->sdev->ready_to_power_off ?
+					power_off_allowed ?
 					ACPI_STATE_D3 : ACPI_STATE_D3_HOT);
 			if (acpi_state > 0) {
 				ret = acpi_bus_set_power(handle, acpi_state);
-- 
1.7.12.21.g871e293

Re: [PATCH v7 6/6] libata: acpi: respect may_power_off flag

[Jeff Garzik]

On 09/12/2012 04:29 AM, Aaron Lu wrote:
> 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 <aaron.lu@intel.com>
> ---
>   drivers/ata/libata-acpi.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index 9aca057..24347e0 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -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 */
> @@ -868,9 +869,11 @@ void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
>   			continue;
>
>   		if (state.event != PM_EVENT_ON) {
> +			power_off_allowed = dev->sdev->ready_to_power_off &&
> +					    dev->sdev->may_power_off;
>   			acpi_state = acpi_pm_device_sleep_state(
>   					&dev->sdev->sdev_gendev, NULL,
> -					dev->sdev->ready_to_power_off ?
> +					power_off_allowed ?
>   					ACPI_STATE_D3 : ACPI_STATE_D3_HOT);

Acked-by: Jeff Garzik <jgarzik@redhat.com>

with the same caveat as with the rest of this patchset:  contingent upon 
acceptance of the other API changes.

Re: [PATCH v7 0/6] ZPODD patches

[Aaron Lu]

Hi James,

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

Thanks,
Aaron

On Wed, Sep 12, 2012 at 04:29:51PM +0800, Aaron Lu wrote:
> 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(-)
> 
> -- 
> 1.7.12.21.g871e293
> 

Re: [PATCH v7 0/6] ZPODD patches

[James Bottomley]

On Wed, 2012-09-19 at 16:03 +0800, Aaron Lu wrote:
> 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.

James

Re: [PATCH v7 0/6] ZPODD patches

[Rafael J. Wysocki]

On Wednesday, September 19, 2012, James Bottomley wrote:
> On Wed, 2012-09-19 at 16:03 +0800, Aaron Lu wrote:
> > 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.

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.

Thanks,
Rafael

Re: [PATCH v7 0/6] ZPODD patches

[Aaron Lu]

On 09/19/2012 08:50 PM, Rafael J. Wysocki wrote:
> On Wednesday, September 19, 2012, James Bottomley wrote:
>> On Wed, 2012-09-19 at 16:03 +0800, Aaron Lu wrote:
>>> 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.
> 
> 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.

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

-Aaron

Re: [PATCH v7 0/6] ZPODD patches

[Rafael J. Wysocki]

On Wednesday, September 19, 2012, Aaron Lu wrote:
> On 09/19/2012 08:50 PM, Rafael J. Wysocki wrote:
> > On Wednesday, September 19, 2012, James Bottomley wrote:
> >> On Wed, 2012-09-19 at 16:03 +0800, Aaron Lu wrote:
> >>> 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.
> > 
> > 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.
> 
> 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

Re: [PATCH v7 0/6] ZPODD patches

[Aaron Lu]

On Thu, Sep 20, 2012 at 10:00:51PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, September 19, 2012, Aaron Lu wrote:
> > 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.

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.

Thanks,
Aaron

> 
> 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

Re: [PATCH v7 0/6] ZPODD patches

[Rafael J. Wysocki]

On Friday, September 21, 2012, Aaron Lu wrote:
> On Thu, Sep 20, 2012 at 10:00:51PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, September 19, 2012, Aaron Lu wrote:
> > > 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.
> 
> 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

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.

> 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?

> 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.

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?

> 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).

> 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?

> I hope it makes more sense to you now.

Well, thanks for the explanation. :-)

Rafael

Re: [PATCH v7 0/6] ZPODD patches

[Oliver Neukum]

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.

> 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"

> 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.
 
> 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.

	Regards
		Oliver

Re: [PATCH v7 0/6] ZPODD patches

[Rafael J. Wysocki]

On Saturday, September 22, 2012, Oliver Neukum wrote:
> 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.

OK

Still, why does the flag have to be in struct scsi_device for this purpose?

> > 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"
> 
> > 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.

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

> > 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?

Rafael

Re: [PATCH v7 0/6] ZPODD patches

[Alan Stern]

On Sat, 22 Sep 2012, Rafael J. Wysocki wrote:

> > 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).

> > > 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?

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.

Alan Stern

Re: [PATCH v7 0/6] ZPODD patches

[Rafael J. Wysocki]

On Saturday, September 22, 2012, Alan Stern wrote:
> On Sat, 22 Sep 2012, Rafael J. Wysocki wrote:
> 
> > > 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).
> 
> > > > 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?
> 
> 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

Rafael

Re: [PATCH v7 0/6] ZPODD patches

[Alan Stern]

On Sat, 22 Sep 2012, Rafael J. Wysocki wrote:

> > > 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...

Alan Stern

Re: [PATCH v7 0/6] ZPODD patches

[Rafael J. Wysocki]

On Saturday, September 22, 2012, Alan Stern wrote:
> On Sat, 22 Sep 2012, Rafael J. Wysocki wrote:
> 
> > > > 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.)

That doesn't seem to be a big obstacle as far as PM QoS is concerned, though.
The trick may be to add PM QoS constraints not for the device itself, but
directly for the port it is connected to (it always is possible to add a
constraint for the device too, but that simply may be unnecessary).  Then,
whoever decides whether or not to power off the port would only have to
look at the effective PM QoS requirement bits of the port.

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

Sure.

Thanks,
Rafael

Re: [PATCH v7 0/6] ZPODD patches

[Aaron Lu]

On Fri, Sep 21, 2012 at 11:18:27PM +0200, Rafael J. Wysocki wrote:
> On Friday, September 21, 2012, Aaron Lu wrote:
> > On Thu, Sep 20, 2012 at 10:00:51PM +0200, Rafael J. Wysocki wrote:
> > > On Wednesday, September 19, 2012, Aaron Lu wrote:
> > > > 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.
> > 
> > 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
> 
> 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.
> 
> > 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.

> 
> > 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.
> 
> 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?
> 
> > 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

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?

> 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.

Thanks,
Aaron

> 
> > 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?
> 
> > I hope it makes more sense to you now.
> 
> Well, thanks for the explanation. :-)
> 
> Rafael

Re: [PATCH v7 0/6] ZPODD patches

[Rafael J. Wysocki]

On Monday, September 24, 2012, Aaron Lu wrote:
> On Fri, Sep 21, 2012 at 11:18:27PM +0200, Rafael J. Wysocki wrote:
> > On Friday, September 21, 2012, Aaron Lu wrote:
> > > On Thu, Sep 20, 2012 at 10:00:51PM +0200, Rafael J. Wysocki wrote:
> > > > On Wednesday, September 19, 2012, Aaron Lu wrote:
> > > > > 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.
> > > 
> > > 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
> > 
> > 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.
> > 
> > > 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.

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

> > > 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.
> > 
> > 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?
> > 
> > > 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
> 
> 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.

> 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?

> 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?
> 
> > 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).

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

> 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

Re: [PATCH v7 0/6] ZPODD patches

[Aaron Lu]

On Mon, Sep 24, 2012 at 03:06:11PM +0200, Rafael J. Wysocki wrote:
> On Monday, September 24, 2012, Aaron Lu wrote:
> > On Fri, Sep 21, 2012 at 11:18:27PM +0200, Rafael J. Wysocki wrote:
> > > On Friday, September 21, 2012, Aaron Lu wrote:
> > > > On Thu, Sep 20, 2012 at 10:00:51PM +0200, Rafael J. Wysocki wrote:
> > > > > On Wednesday, September 19, 2012, Aaron Lu wrote:
> > > > > > 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.
> > > > 
> > > > 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
> > > 
> > > 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.
> > > 
> > > > 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.
> 
> I need some more time to think about this, stay tuned.

Thanks.

> 
> > > > 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.
> > > 
> > > 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?
> > > 
> > > > 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
> > 
> > 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.

Agree.

> 
> > 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?

I can only say that it is useful for ZPODD, if ZPODD someday is used on
another platform that does not use ACPI, the need_eject flag should
still be needed.

As for other scsi devices, I'm not sure.

> 
> > 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?
> > 
> > > 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).
> 
> I'm currently thinking that using PM QoS may be a better approach here.

Is it something like a "power_off_allowed" binary constraint?
Then both the sr driver and the user can change the value so that both
the ready_to_power_off and may_power_off is no longer needed.

> 
> > 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.

Right, not easy to find a home for need_eject...

Thanks,
Aaron

> 
> Thanks,
> Rafael

Re: [PATCH v7 0/6] ZPODD patches

[Rafael J. Wysocki]

On Monday, September 24, 2012, Aaron Lu wrote:
> On Mon, Sep 24, 2012 at 03:06:11PM +0200, Rafael J. Wysocki wrote:
> > On Monday, September 24, 2012, Aaron Lu wrote:
> > > On Fri, Sep 21, 2012 at 11:18:27PM +0200, Rafael J. Wysocki wrote:
> > > > On Friday, September 21, 2012, Aaron Lu wrote:
> > > > > On Thu, Sep 20, 2012 at 10:00:51PM +0200, Rafael J. Wysocki wrote:
> > > > > > On Wednesday, September 19, 2012, Aaron Lu wrote:
> > > > > > > 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.
> > > > > 
> > > > > 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
> > > > 
> > > > 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.
> > > > 
> > > > > 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.
> > 
> > I need some more time to think about this, stay tuned.
> 
> Thanks.

Alan has just given you a good suggestion, you can follow it I think.

> > > > > 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.
> > > > 
> > > > 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?
> > > > 
> > > > > 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
> > > 
> > > 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.
> 
> Agree.
> 
> > 
> > > 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?
> 
> I can only say that it is useful for ZPODD, if ZPODD someday is used on
> another platform that does not use ACPI, the need_eject flag should
> still be needed.
> 
> As for other scsi devices, I'm not sure.

I see.  This means we don't really have good arguments for putting that flag
into struct scsi_device ...

> > > 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?
> > > 
> > > > 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).
> > 
> > I'm currently thinking that using PM QoS may be a better approach here.
> 
> Is it something like a "power_off_allowed" binary constraint?
> Then both the sr driver and the user can change the value so that both
> the ready_to_power_off and may_power_off is no longer needed.

Yes, that's the idea.

Thanks,
Rafael

Re: [PATCH v7 0/6] ZPODD patches

[Aaron Lu]

On Mon, Sep 24, 2012 at 11:46:03PM +0200, Rafael J. Wysocki wrote:
> On Monday, September 24, 2012, Aaron Lu wrote:
> > On Mon, Sep 24, 2012 at 03:06:11PM +0200, Rafael J. Wysocki wrote:
> > > On Monday, September 24, 2012, Aaron Lu wrote:
> > > > 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?
> > 
> > I can only say that it is useful for ZPODD, if ZPODD someday is used on
> > another platform that does not use ACPI, the need_eject flag should
> > still be needed.
> > 
> > As for other scsi devices, I'm not sure.
> 
> I see.  This means we don't really have good arguments for putting that flag
> into struct scsi_device ...

OK.

I'm thinking of moving the acpi wake notification code from ata to scsi,
so that the notification function lives in sr module and then I do not
need to set this need_eject flag in scsi_device and scsi_cd structure
needs to host this flag.

A example patch would be something like the following, I didn't seperate
these ACPI calls in sr.c as this is just a concept proof, if this is the
right thing to do, I will separate them into another file sr-acpi.c and
make empty stubs for them in sr.h for systems do not have ACPI configured.


diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index ef72682..94d17f1 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -46,6 +46,7 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/pm_runtime.h>
+#include <linux/acpi.h>
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>
@@ -57,6 +58,8 @@
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_ioctl.h>	/* For the door lock/unlock commands */
 
+#include <acpi/acpi_bus.h>
+
 #include "scsi_logging.h"
 #include "sr.h"
 
@@ -212,8 +220,8 @@ static int sr_resume(struct device *dev)
 	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;
+	if (cd->need_eject) {
+		cd->need_eject = false;
 		/* 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);
@@ -704,6 +711,58 @@ static void sr_release(struct cdrom_device_info *cdi)
 
 }
 
+static void sr_acpi_wake_dev(acpi_handle handle, u32 event, void *context)
+{
+	struct device *dev = context;
+	struct scsi_cd *cd = dev_get_drvdata(dev);
+
+	if (event == ACPI_NOTIFY_DEVICE_WAKE && pm_runtime_suspended(dev)) {
+		cd->need_eject = true;
+		pm_runtime_resume(dev);
+	}
+}
+
+static void sr_acpi_add_pm_notifier(struct device *dev)
+{
+	struct acpi_device *acpi_dev;
+	acpi_handle handle;
+	acpi_status status;
+
+	handle = dev->archdata.acpi_handle;
+	if (!handle)
+		return;
+
+	status = acpi_bus_get_device(handle, &acpi_dev);
+	if (ACPI_FAILURE(status))
+		return;
+
+	acpi_power_resource_register_device(dev, handle);
+	acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
+					sr_acpi_wake_dev, dev);
+	device_set_run_wake(dev, true);
+}
+
+static void sr_acpi_remove_pm_notifier(struct device *dev)
+{
+	struct acpi_device *acpi_dev;
+	acpi_handle handle;
+	acpi_status status;
+
+	handle = dev->archdata.acpi_handle;
+	if (!handle)
+		return;
+
+	status = acpi_bus_get_device(handle, &acpi_dev);
+	if (ACPI_FAILURE(status))
+		return;
+
+	acpi_power_resource_unregister_device(dev, handle);
+	device_set_run_wake(dev, false);
+	acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY, sr_acpi_wake_dev);
+}
+
 static int sr_probe(struct device *dev)
 {
 	struct scsi_device *sdev = to_scsi_device(dev);
@@ -786,7 +845,9 @@ static int sr_probe(struct device *dev)
 	sdev_printk(KERN_DEBUG, sdev,
 		    "Attached scsi CD-ROM %s\n", cd->cdi.name);
 
-	/* enable runtime pm */
+	if (sdev->can_power_off)
+		sr_acpi_add_pm_notifier(dev);
+
 	scsi_autopm_put_device(cd->device);
 
 	return 0;
@@ -1036,8 +1097,9 @@ static int sr_remove(struct device *dev)
 {
 	struct scsi_cd *cd = dev_get_drvdata(dev);
 
-	/* disable runtime pm */
 	scsi_autopm_get_device(cd->device);
+	if (cd->device->can_power_off)
+		sr_acpi_remove_pm_notifier(dev);
 
 	blk_queue_prep_rq(cd->device->request_queue, scsi_prep_fn);
 	del_gendisk(cd->disk);


Thanks,
Aaron

Re: [PATCH v7 0/6] ZPODD patches

[James Bottomley]

On Tue, 2012-09-25 at 16:18 +0800, Aaron Lu wrote:
> A example patch would be something like the following, I didn't seperate
> these ACPI calls in sr.c as this is just a concept proof, if this is the
> right thing to do, I will separate them into another file sr-acpi.c and
> make empty stubs for them in sr.h for systems do not have ACPI configured.

Apart from the needed separation to compile in the !ACPI case

> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index ef72682..94d17f1 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -46,6 +46,7 @@
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/acpi.h>
>  #include <asm/uaccess.h>
>  
>  #include <scsi/scsi.h>
> @@ -57,6 +58,8 @@
>  #include <scsi/scsi_host.h>
>  #include <scsi/scsi_ioctl.h>	/* For the door lock/unlock commands */
>  
> +#include <acpi/acpi_bus.h>
> +
>  #include "scsi_logging.h"
>  #include "sr.h"
>  
> @@ -212,8 +220,8 @@ static int sr_resume(struct device *dev)
>  	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;
> +	if (cd->need_eject) {
> +		cd->need_eject = false;
>  		/* 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);
> @@ -704,6 +711,58 @@ static void sr_release(struct cdrom_device_info *cdi)
>  
>  }
>  
> +static void sr_acpi_wake_dev(acpi_handle handle, u32 event, void *context)
> +{
> +	struct device *dev = context;
> +	struct scsi_cd *cd = dev_get_drvdata(dev);
> +
> +	if (event == ACPI_NOTIFY_DEVICE_WAKE && pm_runtime_suspended(dev)) {
> +		cd->need_eject = true;
> +		pm_runtime_resume(dev);
> +	}
> +}
> +
> +static void sr_acpi_add_pm_notifier(struct device *dev)
> +{
> +	struct acpi_device *acpi_dev;
> +	acpi_handle handle;
> +	acpi_status status;
> +
> +	handle = dev->archdata.acpi_handle;

This is a complete no-no.  archdata is defined to be specific to the
architecture it's supposed to be opaque to non-arch code.  You'll find
that only x86 and ia64 defines an acpi_handle there.  This will
instantly fail to compile on non intel.  If you need the handle, it
should be obtained via some accessor like dev_to_acpi_handle() which
will allow this to continue to function when, say, arm acquires ACPI.

I've got to say, this looks like a fault in ACPI itself.  If the handles
are 1:1 with struct device, then why not have all the functions taking
handles take the device instead and leave the embedded handle safely in
the architecture specific code?

James

Re: [PATCH v7 0/6] ZPODD patches

[Aaron Lu]

On Tue, Sep 25, 2012 at 03:02:29PM +0400, James Bottomley wrote:
> On Tue, 2012-09-25 at 16:18 +0800, Aaron Lu wrote:
> > A example patch would be something like the following, I didn't seperate
> > these ACPI calls in sr.c as this is just a concept proof, if this is the
> > right thing to do, I will separate them into another file sr-acpi.c and
> > make empty stubs for them in sr.h for systems do not have ACPI configured.
> 
> Apart from the needed separation to compile in the !ACPI case
> 
> > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> > index ef72682..94d17f1 100644
> > --- a/drivers/scsi/sr.c
> > +++ b/drivers/scsi/sr.c
> > @@ -46,6 +46,7 @@
> >  #include <linux/mutex.h>
> >  #include <linux/slab.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/acpi.h>
> >  #include <asm/uaccess.h>
> >  
> >  #include <scsi/scsi.h>
> > @@ -57,6 +58,8 @@
> >  #include <scsi/scsi_host.h>
> >  #include <scsi/scsi_ioctl.h>	/* For the door lock/unlock commands */
> >  
> > +#include <acpi/acpi_bus.h>
> > +
> >  #include "scsi_logging.h"
> >  #include "sr.h"
> >  
> > @@ -212,8 +220,8 @@ static int sr_resume(struct device *dev)
> >  	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;
> > +	if (cd->need_eject) {
> > +		cd->need_eject = false;
> >  		/* 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);
> > @@ -704,6 +711,58 @@ static void sr_release(struct cdrom_device_info *cdi)
> >  
> >  }
> >  
> > +static void sr_acpi_wake_dev(acpi_handle handle, u32 event, void *context)
> > +{
> > +	struct device *dev = context;
> > +	struct scsi_cd *cd = dev_get_drvdata(dev);
> > +
> > +	if (event == ACPI_NOTIFY_DEVICE_WAKE && pm_runtime_suspended(dev)) {
> > +		cd->need_eject = true;
> > +		pm_runtime_resume(dev);
> > +	}
> > +}
> > +
> > +static void sr_acpi_add_pm_notifier(struct device *dev)
> > +{
> > +	struct acpi_device *acpi_dev;
> > +	acpi_handle handle;
> > +	acpi_status status;
> > +
> > +	handle = dev->archdata.acpi_handle;
> 
> This is a complete no-no.  archdata is defined to be specific to the
> architecture it's supposed to be opaque to non-arch code.  You'll find
> that only x86 and ia64 defines an acpi_handle there.  This will
> instantly fail to compile on non intel.  If you need the handle, it

If you are OK with this change to solve the need_eject flag, I'll prepare
a formal patch, in which, all of the newly added function will be within
the range of

#ifdef CONFIG_ACPI
... ...
#endif

And for the CONFIG_ACPI not defined case, they will be static inline
empty functions. Then there should be no compile errors.

> should be obtained via some accessor like dev_to_acpi_handle() which
> will allow this to continue to function when, say, arm acquires ACPI.

There is a DEVICE_ACPI_HANDLE macro that I'll use when preparing the
formal patch. I'm rushing out these code to show the idea.
Sorry for not considering these things.

Thanks,
Aaron

Re: [PATCH v7 0/6] ZPODD patches

[Aaron Lu]

On Tue, Sep 25, 2012 at 03:02:29PM +0400, James Bottomley wrote:
> On Tue, 2012-09-25 at 16:18 +0800, Aaron Lu wrote:
> > A example patch would be something like the following, I didn't seperate
> > these ACPI calls in sr.c as this is just a concept proof, if this is the
> > right thing to do, I will separate them into another file sr-acpi.c and
> > make empty stubs for them in sr.h for systems do not have ACPI configured.
> 
> Apart from the needed separation to compile in the !ACPI case
> 
> > +static void sr_acpi_add_pm_notifier(struct device *dev)
> > +{
> > +	struct acpi_device *acpi_dev;
> > +	acpi_handle handle;
> > +	acpi_status status;
> > +
> > +	handle = dev->archdata.acpi_handle;
> 
> This is a complete no-no.  archdata is defined to be specific to the
> architecture it's supposed to be opaque to non-arch code.  You'll find
> that only x86 and ia64 defines an acpi_handle there.  This will
> instantly fail to compile on non intel.  If you need the handle, it
> should be obtained via some accessor like dev_to_acpi_handle() which
> will allow this to continue to function when, say, arm acquires ACPI.
> 
> I've got to say, this looks like a fault in ACPI itself.  If the handles
> are 1:1 with struct device, then why not have all the functions taking
> handles take the device instead and leave the embedded handle safely in
> the architecture specific code?

I've prepared a complete code change for you to take a look, with the
notification code resides in sr, I can move the need_eject flag from
scsi_device to scsi_cd, which should make more sense.


diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index 888f73a..9f0a1da 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -177,6 +177,7 @@ sd_mod-objs	:= sd.o
 sd_mod-$(CONFIG_BLK_DEV_INTEGRITY) += sd_dif.o
 
 sr_mod-objs	:= sr.o sr_ioctl.o sr_vendor.o
+sr_mod-$(CONFIG_ACPI)	+= sr_acpi.o
 ncr53c8xx-flags-$(CONFIG_SCSI_ZALON) \
 		:= -DCONFIG_NCR53C8XX_PREFETCH -DSCSI_NCR_BIG_ENDIAN \
 			-DCONFIG_SCSI_NCR53C8XX_NO_WORD_TRANSFERS
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 4d1a610..cb6703c 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -734,6 +734,7 @@ static int sr_probe(struct device *dev)
 
 	sdev_printk(KERN_DEBUG, sdev,
 		    "Attached scsi CD-ROM %s\n", cd->cdi.name);
+	sr_acpi_add_pm_notifier(dev);
 	scsi_autopm_put_device(cd->device);
 
 	return 0;
@@ -984,6 +985,7 @@ static int sr_remove(struct device *dev)
 	struct scsi_cd *cd = dev_get_drvdata(dev);
 
 	scsi_autopm_get_device(cd->device);
+	sr_acpi_remove_pm_notifier(dev);
 
 	blk_queue_prep_rq(cd->device->request_queue, scsi_prep_fn);
 	del_gendisk(cd->disk);
diff --git a/drivers/scsi/sr.h b/drivers/scsi/sr.h
index 37c8f6b..1f66fa0a 100644
--- a/drivers/scsi/sr.h
+++ b/drivers/scsi/sr.h
@@ -48,6 +48,8 @@ typedef struct scsi_cd {
 	bool get_event_changed:1;	/* changed according to GET_EVENT */
 	bool ignore_get_event:1;	/* GET_EVENT is unreliable, use TUR */
 
+	bool need_eject:1; /* User wakes up the ODD, need eject the tray */
+
 	struct cdrom_device_info cdi;
 	/* We hold gendisk and scsi_device references on probe and use
 	 * the refs on this kref to decide when to release them */
@@ -74,4 +76,13 @@ void sr_vendor_init(Scsi_CD *);
 int sr_cd_check(struct cdrom_device_info *);
 int sr_set_blocklength(Scsi_CD *, int blocklength);
 
+/* sr_acpi.c */
+#ifdef CONFIG_ACPI
+extern void sr_acpi_add_pm_notifier(struct device *);
+extern void sr_acpi_remove_pm_notifier(struct device *);
+#else
+static inline void sr_acpi_add_pm_notifier(struct device *dev) {}
+static inline void sr_acpi_remove_pm_notifier(struct device *dev) {}
+#endif
+
 #endif
diff --git a/drivers/scsi/sr_acpi.c b/drivers/scsi/sr_acpi.c
new file mode 100644
index 0000000..ce6bc11
--- /dev/null
+++ b/drivers/scsi/sr_acpi.c
@@ -0,0 +1,64 @@
+#include <linux/cdrom.h>
+#include <linux/pm_runtime.h>
+#include <scsi/scsi_device.h>
+#include <linux/acpi.h>
+#include <acpi/acpi_bus.h>
+#include "sr.h"
+
+static void sr_acpi_wake_dev(acpi_handle handle, u32 event, void *context)
+{
+	struct device *dev = context;
+	struct scsi_cd *cd = dev_get_drvdata(dev);
+
+	if (event == ACPI_NOTIFY_DEVICE_WAKE && pm_runtime_suspended(dev)) {
+		cd->need_eject = 1;
+		pm_runtime_resume(dev);
+	}
+}
+
+void sr_acpi_add_pm_notifier(struct device *dev)
+{
+	struct acpi_device *acpi_dev;
+	acpi_handle handle;
+	acpi_status status;
+	struct scsi_device *sdev = to_scsi_device(dev);
+
+	if (!sdev->can_power_off)
+		return;
+
+	handle = DEVICE_ACPI_HANDLE(dev);
+	if (!handle)
+		return;
+
+	status = acpi_bus_get_device(handle, &acpi_dev);
+	if (ACPI_FAILURE(status))
+		return;
+
+	acpi_power_resource_register_device(dev, handle);
+	acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
+			sr_acpi_wake_dev, dev);
+	device_set_run_wake(dev, true);
+}
+
+void sr_acpi_remove_pm_notifier(struct device *dev)
+{
+	struct acpi_device *acpi_dev;
+	acpi_handle handle;
+	acpi_status status;
+	struct scsi_device *sdev = to_scsi_device(dev);
+
+	if (!sdev->can_power_off)
+		return;
+
+	handle = DEVICE_ACPI_HANDLE(dev);
+	if (!handle)
+		return;
+
+	status = acpi_bus_get_device(handle, &acpi_dev);
+	if (ACPI_FAILURE(status))
+		return;
+
+	acpi_power_resource_unregister_device(dev, handle);
+	device_set_run_wake(dev, false);
+	acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY, sr_acpi_wake_dev);
+}

Thanks,
Aaron

Re: [PATCH v7 0/6] ZPODD patches

[Alan Stern]

[CC: list trimmed somewhat]

On Mon, 24 Sep 2012, Aaron Lu wrote:

> 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.

The problem is in the scsi_autopm_get_device call.  What you need to do
here is increment the PM usage counter without resuming the drive.  
Then if the runtime PM status is RPM_SUSPENDING, decrement the usage
counter and return immediately (skip the status check); otherwise call
scsi_autopm_get_device as before and do an extra decrement after the
status check is finished.

It's a little more tricky than this because you need to acquire the 
private runtime PM spinlock in addition to incrementing the usage 
counter, for proper synchronization.  I don't think we have any 
interface to do this, although it would be easy to add one.

Alan Stern

Re: [PATCH v7 0/6] ZPODD patches

[James Bottomley]

On Wed, 2012-09-19 at 14:50 +0200, Rafael J. Wysocki wrote:
> On Wednesday, September 19, 2012, James Bottomley wrote:
> > On Wed, 2012-09-19 at 16:03 +0800, Aaron Lu wrote:
> > > 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.
> 
> 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.

So I don't insist on it being generic, but we do need somewhere to hang
the state.

> 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.).

So, here's the problem this causes.  In SCSI, lower level devices have
no access to the drivers (to which the upper layer structures are tied),
so we have no way to go from device/scsi device to the scsi_disk
structure say.  This means that a lot of device specific PM stuff tends
to have flags in scsi_device just so we can get access to it.  A flag in
device would allow us to carry the information farther (say to struct
cdrom for instance).

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

Thanks,

James

Re: [PATCH v7 0/6] ZPODD patches

[Rafael J. Wysocki]

On Wednesday, September 19, 2012, James Bottomley wrote:
> On Wed, 2012-09-19 at 14:50 +0200, Rafael J. Wysocki wrote:
> > On Wednesday, September 19, 2012, James Bottomley wrote:
> > > On Wed, 2012-09-19 at 16:03 +0800, Aaron Lu wrote:
> > > > 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.
> > 
> > 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.
> 
> So I don't insist on it being generic, but we do need somewhere to hang
> the state.
> 
> > 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.).
> 
> So, here's the problem this causes.  In SCSI, lower level devices have
> no access to the drivers (to which the upper layer structures are tied),
> so we have no way to go from device/scsi device to the scsi_disk
> structure say.  This means that a lot of device specific PM stuff tends
> to have flags in scsi_device just so we can get access to it.  A flag in
> device would allow us to carry the information farther (say to struct
> cdrom for instance).
> 
> > I'll have a look at the patchset again and see what can be done about this.

I think I see what you mean.

For example, the sr driver uses its "device" member to pass information
to the libata layer, if I understand things correctly, and the libata
layer cannot go back to its struct scsi_cd, right?

First off, I agree with you that putting those PM fields into struct scsi_device
is not the cleanest approach and it would be good to find some other place
for them.  However, I also don't think that struct device (or struct dev_pm_info
embedded in it for that matter) is any better (those fields in there would make
as little sense as they do in struct scsi_device).

Now, the question is where to store them and I think there are a couple of
places worth considering.  For instance, there's the void *driver_data field
in struct acpi_device that I bet is unused for the ACPI devices associated with
ATA ones and in principle it might be set by libata to point to a PM data
structure (that would require some "platform" helper functions for the
sr, sd etc. drivers to access that structure).  There also is the subsys_data
field in struct dev_pm_info that might be used to point to some libata-specific
PM data (although the question is which struct dev_pm_info to use in that
case, the sdev's one, or the sdev->gendev's one?).

Thanks,
Rafael

Re: [PATCH v7 0/6] ZPODD patches

[Oliver Neukum]

On Wednesday 19 September 2012 13:27:47 James Bottomley wrote:
> On Wed, 2012-09-19 at 16:03 +0800, Aaron Lu wrote:
> > 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.

The SCSI layer however needs to know whether the cache is handled
by the OS or in the device and under which circumstances a device
can detect media change events.

	Regards
		Oliver


-- 
- - - 
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) 
Maxfeldstraße 5                         
90409 Nürnberg 
Germany 
- - - 
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 0/6] ZPODD patches

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1253 bytes --]

On Wed, 2012-09-19 at 13:27 +0100, James Bottomley wrote:
> 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.

FWIW the SATA Device Sleep support, which is currently only used (in a
patch which is pending) automatically as an extension of the AHCI link
power management, can also be triggered manually by a GPIO line; I've
got a platform where it's done with an ACPI call. I suspect we'll see
that kind of manual setup on a few embedded platforms too.

If we're working on hooking this up through the device layers, perhaps
that's worth taking into consideration?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation



[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

RE: [PATCH v7 0/6] ZPODD patches

[Jack Wang]

> 
> On Wed, 2012-09-19 at 13:27 +0100, James Bottomley wrote:
> > 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.
> 
> FWIW the SATA Device Sleep support, which is currently only used (in a
> patch which is pending) automatically as an extension of the AHCI link
> power management, can also be triggered manually by a GPIO line; I've
> got a platform where it's done with an ACPI call. I suspect we'll see
> that kind of manual setup on a few embedded platforms too.
> 
> If we're working on hooking this up through the device layers, perhaps
> that's worth taking into consideration?
> 
> --

Hi David,

For SATA Device sleep, what's the appropriate time to set the Device sleep,
this seems also part of runtime PM?

Best regards!

Jack

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Aaron Lu]

[Adding more people and list back in]

On 09/29/2012 05:46 AM, Rafael J. Wysocki wrote:
> On Friday, September 28, 2012, Aaron Lu wrote:
>> On 09/28/2012 07:15 AM, Rafael J. Wysocki wrote:
>>> On Thursday, September 27, 2012, Aaron Lu wrote:
>>>> On 09/27/2012 05:37 AM, Rafael J. Wysocki wrote:
>>>>>
>>>>> Say the user has pressed the eject button.  What does need to happen so that
>>>>> the tray is physically ejected?
>>>>
>>>> The tray is ejected by the ODD itself, host does not have to do anything.
>>>>
>>>> There is a command(PREVENT_MEDIUM_REMOVAL) to lock the door so that when
>>>> user presses the eject button, the tray will not be ejected. This command
>>>> is usually sent when we have a disc inside and a user space program
>>>> opened the underlying block device(e.g. /dev/sr0) to read/write data.
>>>>
>>>> And host can also eject the tray by sending a START_STOP_UNIT command
>>>> with param LoEj set to 1 and we have a function called sr_tray_move to
>>>> do just this. And this is also what I've used to eject the tray after
>>>> user wakes up the ODD, as when user presses the eject button when the
>>>> ODD is in zero power state, it can't eject the tray as usual, so host
>>>> software will need to do this, that's the reason I need to know such
>>>> information:
>>>> When ODD is resumed, is it because user wakes it up?
>>>
>>> But START_STOP_UNIT eventually causes ata_scsi_start_stop_xlat() to be
>>
>> You are following ata case, while the ODD is an atapi device :-)
>> The translation function is atapi_xlat, but that doesn't affect the idea
>> here.
>>
>>> executed, so I wonder if we really need to go up through the SCSI stack
>>> to send that command to the drive from there?  It should be possible
>>> to issue STANDBY/READ VERIFY to the device directly from libata if
>>> an eject event is signaled through a GPE.
>>
>> Yes, this is possible.
>> Though it doesn't feel very cool, since I have no idea if the ODD is a
>> tray type or slot type in ATA layer and I'll blindly send this command
>> to it then, not a problem maybe.
> 
> It would be good to verify if that works for slot devices, if possible.

The ACPI GPE event is triggered when user inserts a disc into a slot
type ODD, and if I send an eject command to it, the disc will be
ejected, which is wrong.

I need to know the loading mechanism(tray type or slot type) of the ODD
to decide if I should send this command.

> 
>> And what do you think of moving the acpi notification code to sr?
>> http://marc.info/?l=linux-pm&m=134873904332704&w=4
> 
> I don't think this is a good idea, quite frankly.  sr seems to be a too
> generic place for that.

Does this mean sr can only have code that is useful to all devices it
manages? i.e. If a piece of code enables a feature for a special kind of
ODD(like the sata based ZPODD), it shouldn't be done in sr?

> 
> Ideally, the whole ZPODD handling should not be visible to the SCSI layer,

I can see 2 problems:
- Don't know its loading machanism so we have the problem above;
- Need to send command to find out if ODD is zero power ready somewhere
  in ata layer, this implies the device is doing IO after it is runtime
  suspended in scsi layer.

> perhaps except the "no_polling" flag disabling the polling that may be
> useful for other purposes in principle.

I hope so, let's hear what other people has to say.

> 
> I'm not sure if it's possible at this point, but if not we need to know
> very precisely why not.

There is nothing in theory stopping us from doing this in ata layer.
For the loading mechanism, we can always send an ATAPI command to figure
it out.

So gentlemen, I need your opinions on where this ZPODD code should live
before I can continue this work, thanks.

-Aaron

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Alan Stern]

On Sat, 29 Sep 2012, Aaron Lu wrote:

> > I don't think this is a good idea, quite frankly.  sr seems to be a too
> > generic place for that.
> 
> Does this mean sr can only have code that is useful to all devices it
> manages? i.e. If a piece of code enables a feature for a special kind of
> ODD(like the sata based ZPODD), it shouldn't be done in sr?

Drivers are allowed to have special features and quirks that apply only 
to some devices.

> There is nothing in theory stopping us from doing this in ata layer.
> For the loading mechanism, we can always send an ATAPI command to figure
> it out.
> 
> So gentlemen, I need your opinions on where this ZPODD code should live
> before I can continue this work, thanks.

Can arbitrary SCSI devices be ZP, or does this notion apply only to
ATAPI-based drives?  That's the key question, and the answer determines
where the ZP support belongs.

On the other hand, the sr driver certainly deserves to have some form 
of runtime PM support, even for devices that aren't ZP.

Alan Stern

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Aaron Lu]

On 09/29/2012 10:29 PM, Alan Stern wrote:
> On Sat, 29 Sep 2012, Aaron Lu wrote:
> 
>>> I don't think this is a good idea, quite frankly.  sr seems to be a too
>>> generic place for that.
>>
>> Does this mean sr can only have code that is useful to all devices it
>> manages? i.e. If a piece of code enables a feature for a special kind of
>> ODD(like the sata based ZPODD), it shouldn't be done in sr?
> 
> Drivers are allowed to have special features and quirks that apply only 
> to some devices.

I think SATA based zero power capable ODDs are the "some devices".

> 
>> There is nothing in theory stopping us from doing this in ata layer.
>> For the loading mechanism, we can always send an ATAPI command to figure
>> it out.
>>
>> So gentlemen, I need your opinions on where this ZPODD code should live
>> before I can continue this work, thanks.
> 
> Can arbitrary SCSI devices be ZP, or does this notion apply only to
> ATAPI-based drives?  That's the key question, and the answer determines
> where the ZP support belongs.

I don't know if arbitrary SCSI devices can be ZP or not, the SPC spec
doesn't seem to define such a power state.

ZPODD is defined for sata based ATAPI ODD which supports device
attention, but doesn't specify how the ODD is powered off and how the
device attention pin notifies OS. On x86 systems, these are implemented
by ACPI.

Though SCSI devices may not have a general notion of ZP, the fact that
ZPODD are managed by sr driver is enough to make the decision that ZPODD
code can live in sr?

> 
> On the other hand, the sr driver certainly deserves to have some form 
> of runtime PM support, even for devices that aren't ZP.

Agree.

And the following codes need to find a home:
- Code used to handle ACPI wake notification(currently done in ATA, but
  causes the "annoying" need_eject flag in scsi_device);
- Code to check if the ODD is ready to be powered off per the ZPODD
  spec.

Thanks,
Aaron

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Rafael J. Wysocki]

On Saturday, September 29, 2012, Aaron Lu wrote:
> On 09/29/2012 10:29 PM, Alan Stern wrote:
> > On Sat, 29 Sep 2012, Aaron Lu wrote:
> > 
> >>> I don't think this is a good idea, quite frankly.  sr seems to be a too
> >>> generic place for that.
> >>
> >> Does this mean sr can only have code that is useful to all devices it
> >> manages? i.e. If a piece of code enables a feature for a special kind of
> >> ODD(like the sata based ZPODD), it shouldn't be done in sr?
> > 
> > Drivers are allowed to have special features and quirks that apply only 
> > to some devices.
> 
> I think SATA based zero power capable ODDs are the "some devices".
> 
> > 
> >> There is nothing in theory stopping us from doing this in ata layer.
> >> For the loading mechanism, we can always send an ATAPI command to figure
> >> it out.
> >>
> >> So gentlemen, I need your opinions on where this ZPODD code should live
> >> before I can continue this work, thanks.
> > 
> > Can arbitrary SCSI devices be ZP, or does this notion apply only to
> > ATAPI-based drives?  That's the key question, and the answer determines
> > where the ZP support belongs.
> 
> I don't know if arbitrary SCSI devices can be ZP or not, the SPC spec
> doesn't seem to define such a power state.
> 
> ZPODD is defined for sata based ATAPI ODD which supports device
> attention, but doesn't specify how the ODD is powered off and how the
> device attention pin notifies OS. On x86 systems, these are implemented
> by ACPI.
> 
> Though SCSI devices may not have a general notion of ZP, the fact that
> ZPODD are managed by sr driver is enough to make the decision that ZPODD
> code can live in sr?

Well, only a part of it is handled by sr, the high-level part so to speak.
The low-level handling is done by the ATA layer.  Now, since sr is the
high-level part and is supposed to be generic, I don't think it's appropriate
to make it care about some low-level things specific to ATA (because there is
another layer of code supposed to handle those).

> > On the other hand, the sr driver certainly deserves to have some form 
> > of runtime PM support, even for devices that aren't ZP.
> 
> Agree.
> 
> And the following codes need to find a home:
> - Code used to handle ACPI wake notification(currently done in ATA, but
>   causes the "annoying" need_eject flag in scsi_device);

And quite frankly I'd more and more convinced that this flag isn't really
necessary.

What you really want to achieve is to eject the tray of a tray-type ODD
if the eject is signaled through a GPE.  I don't see anything for sr to
do in that.  Moreover, you probably would like to do that even if the
drive is not powered down, right?

I wonder if this mechanism can be used for user space notification
without polling regardless of whether or not the zero-power feature is
used?

> - Code to check if the ODD is ready to be powered off per the ZPODD
>   spec.

If that can be done at the ATA level, I'd prefer it too.

Thanks,
Rafael

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Aaron Lu]

On Sun, Sep 30, 2012 at 12:44:50AM +0200, Rafael J. Wysocki wrote:
> On Saturday, September 29, 2012, Aaron Lu wrote:
> > On 09/29/2012 10:29 PM, Alan Stern wrote:
> > > On Sat, 29 Sep 2012, Aaron Lu wrote:
> > > 
> > >>> I don't think this is a good idea, quite frankly.  sr seems to be a too
> > >>> generic place for that.
> > >>
> > >> Does this mean sr can only have code that is useful to all devices it
> > >> manages? i.e. If a piece of code enables a feature for a special kind of
> > >> ODD(like the sata based ZPODD), it shouldn't be done in sr?
> > > 
> > > Drivers are allowed to have special features and quirks that apply only 
> > > to some devices.
> > 
> > I think SATA based zero power capable ODDs are the "some devices".
> > 
> > > 
> > >> There is nothing in theory stopping us from doing this in ata layer.
> > >> For the loading mechanism, we can always send an ATAPI command to figure
> > >> it out.
> > >>
> > >> So gentlemen, I need your opinions on where this ZPODD code should live
> > >> before I can continue this work, thanks.
> > > 
> > > Can arbitrary SCSI devices be ZP, or does this notion apply only to
> > > ATAPI-based drives?  That's the key question, and the answer determines
> > > where the ZP support belongs.
> > 
> > I don't know if arbitrary SCSI devices can be ZP or not, the SPC spec
> > doesn't seem to define such a power state.
> > 
> > ZPODD is defined for sata based ATAPI ODD which supports device
> > attention, but doesn't specify how the ODD is powered off and how the
> > device attention pin notifies OS. On x86 systems, these are implemented
> > by ACPI.
> > 
> > Though SCSI devices may not have a general notion of ZP, the fact that
> > ZPODD are managed by sr driver is enough to make the decision that ZPODD
> > code can live in sr?
> 
> Well, only a part of it is handled by sr, the high-level part so to speak.
> The low-level handling is done by the ATA layer.  Now, since sr is the
> high-level part and is supposed to be generic, I don't think it's appropriate
> to make it care about some low-level things specific to ATA (because there is
> another layer of code supposed to handle those).

Makes sense to me, but there is a problem if I want to block events
checking for the disk, as I do not have a pointer to the gendisk in ATA
layer.

> 
> > > On the other hand, the sr driver certainly deserves to have some form 
> > > of runtime PM support, even for devices that aren't ZP.
> > 
> > Agree.
> > 
> > And the following codes need to find a home:
> > - Code used to handle ACPI wake notification(currently done in ATA, but
> >   causes the "annoying" need_eject flag in scsi_device);
> 
> And quite frankly I'd more and more convinced that this flag isn't really
> necessary.
> 
> What you really want to achieve is to eject the tray of a tray-type ODD
> if the eject is signaled through a GPE.  I don't see anything for sr to
> do in that.  Moreover, you probably would like to do that even if the
> drive is not powered down, right?

The tray will be ejected by the ODD itself when it has power, I do not
need to do that. Moreover, I don't think I need enable the GPE bit when
it has power.

> 
> I wonder if this mechanism can be used for user space notification
> without polling regardless of whether or not the zero-power feature is
> used?

This may be a reason the GPE should be always enabled no matter if power
is removed or not. But I have concerns that this mechanism is designed
to acheive this, as explained in another mail thread:
http://marc.info/?l=linux-pm&m=134882049212936&w=2

Copied here:

The SATA spec says the device attention pin shall assert when:
- For tray type ODD, its front panel button is released;
- For slot type ODD, media is inserted.

I've a slot type ODD which has a eject button. The notification will be
sent when a disc is inserted, but not when the eject button is pressed,
and this doesn't violate the spec.

But if we disable the poll for disc changes, we will lose an event when
the disc is ejected by the eject button(the device attention pin shall
not trigger this time). I suppose this is a problem?

I think the device attention scheme is not designed to do this job,
while SATA asynchronous notification is.

Thanks,
Aaron

> 
> > - Code to check if the ODD is ready to be powered off per the ZPODD
> >   spec.
> 
> If that can be done at the ATA level, I'd prefer it too.
> 
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Alan Stern]

On Sun, 30 Sep 2012, Aaron Lu wrote:

> Makes sense to me, but there is a problem if I want to block events
> checking for the disk, as I do not have a pointer to the gendisk in ATA
> layer.

> The tray will be ejected by the ODD itself when it has power, I do not
> need to do that. Moreover, I don't think I need enable the GPE bit when
> it has power.

It sounds like you need to add only two things to the sr layer: An 
interface to enable/disable event checking and an interface to request 
an eject.  (And perhaps ejects can be carried out entirely within the 
ATAPI layer, with no need to involve sr.)

Alan Stern

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Aaron Lu]

On 09/30/2012 10:47 PM, Alan Stern wrote:
> On Sun, 30 Sep 2012, Aaron Lu wrote:
> 
>> Makes sense to me, but there is a problem if I want to block events
>> checking for the disk, as I do not have a pointer to the gendisk in ATA
>> layer.
> 
>> The tray will be ejected by the ODD itself when it has power, I do not
>> need to do that. Moreover, I don't think I need enable the GPE bit when
>> it has power.
> 
> It sounds like you need to add only two things to the sr layer: An 
> interface to enable/disable event checking and an interface to request 
> an eject.  (And perhaps ejects can be carried out entirely within the 
> ATAPI layer, with no need to involve sr.)

Thanks for the suggestion, I'll try to do this.

-Aaron

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Jeff Garzik]

On 09/30/2012 10:47 AM, Alan Stern wrote:
> On Sun, 30 Sep 2012, Aaron Lu wrote:
>
>> Makes sense to me, but there is a problem if I want to block events
>> checking for the disk, as I do not have a pointer to the gendisk in ATA
>> layer.

You may discover the gendisk by going the ATA -> SCSI -> block route.


>> The tray will be ejected by the ODD itself when it has power, I do not
>> need to do that. Moreover, I don't think I need enable the GPE bit when
>> it has power.
>
> It sounds like you need to add only two things to the sr layer: An
> interface to enable/disable event checking and an interface to request
> an eject.  (And perhaps ejects can be carried out entirely within the
> ATAPI layer, with no need to involve sr.)

Sounds reasonable.

	Jeff

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Jeff Garzik]

On 09/29/2012 06:44 PM, Rafael J. Wysocki wrote:
> On Saturday, September 29, 2012, Aaron Lu wrote:
>> On 09/29/2012 10:29 PM, Alan Stern wrote:
>>> On Sat, 29 Sep 2012, Aaron Lu wrote:
>>>
>>>>> I don't think this is a good idea, quite frankly.  sr seems to be a too
>>>>> generic place for that.
>>>>
>>>> Does this mean sr can only have code that is useful to all devices it
>>>> manages? i.e. If a piece of code enables a feature for a special kind of
>>>> ODD(like the sata based ZPODD), it shouldn't be done in sr?
>>>
>>> Drivers are allowed to have special features and quirks that apply only
>>> to some devices.
>>
>> I think SATA based zero power capable ODDs are the "some devices".
>>
>>>
>>>> There is nothing in theory stopping us from doing this in ata layer.
>>>> For the loading mechanism, we can always send an ATAPI command to figure
>>>> it out.
>>>>
>>>> So gentlemen, I need your opinions on where this ZPODD code should live
>>>> before I can continue this work, thanks.
>>>
>>> Can arbitrary SCSI devices be ZP, or does this notion apply only to
>>> ATAPI-based drives?  That's the key question, and the answer determines
>>> where the ZP support belongs.
>>
>> I don't know if arbitrary SCSI devices can be ZP or not, the SPC spec
>> doesn't seem to define such a power state.
>>
>> ZPODD is defined for sata based ATAPI ODD which supports device
>> attention, but doesn't specify how the ODD is powered off and how the
>> device attention pin notifies OS. On x86 systems, these are implemented
>> by ACPI.
>>
>> Though SCSI devices may not have a general notion of ZP, the fact that
>> ZPODD are managed by sr driver is enough to make the decision that ZPODD
>> code can live in sr?
>
> Well, only a part of it is handled by sr, the high-level part so to speak.
> The low-level handling is done by the ATA layer.  Now, since sr is the
> high-level part and is supposed to be generic, I don't think it's appropriate
> to make it care about some low-level things specific to ATA (because there is
> another layer of code supposed to handle those).
>
>>> On the other hand, the sr driver certainly deserves to have some form
>>> of runtime PM support, even for devices that aren't ZP.
>>
>> Agree.
>>
>> And the following codes need to find a home:
>> - Code used to handle ACPI wake notification(currently done in ATA, but
>>    causes the "annoying" need_eject flag in scsi_device);
>
> And quite frankly I'd more and more convinced that this flag isn't really
> necessary.
>
> What you really want to achieve is to eject the tray of a tray-type ODD
> if the eject is signaled through a GPE.  I don't see anything for sr to
> do in that.  Moreover, you probably would like to do that even if the
> drive is not powered down, right?
>
> I wonder if this mechanism can be used for user space notification
> without polling regardless of whether or not the zero-power feature is
> used?

Fair questions, and I think this is finally getting to the heart of the 
matter/solution.


>> - Code to check if the ODD is ready to be powered off per the ZPODD
>>    spec.
>
> If that can be done at the ATA level, I'd prefer it too.

Does the ready-to-poweroff check involve SCSI/MMC command set commands?

If no, it probably belongs in libata.

If yes, it probably belongs in sr.

	Jeff

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Rafael J. Wysocki]

On Saturday, September 29, 2012, Alan Stern wrote:
> On Sat, 29 Sep 2012, Aaron Lu wrote:
> 
> > > I don't think this is a good idea, quite frankly.  sr seems to be a too
> > > generic place for that.
> > 
> > Does this mean sr can only have code that is useful to all devices it
> > manages? i.e. If a piece of code enables a feature for a special kind of
> > ODD(like the sata based ZPODD), it shouldn't be done in sr?
> 
> Drivers are allowed to have special features and quirks that apply only 
> to some devices.
> 
> > There is nothing in theory stopping us from doing this in ata layer.
> > For the loading mechanism, we can always send an ATAPI command to figure
> > it out.
> > 
> > So gentlemen, I need your opinions on where this ZPODD code should live
> > before I can continue this work, thanks.
> 
> Can arbitrary SCSI devices be ZP, or does this notion apply only to
> ATAPI-based drives?  That's the key question, and the answer determines
> where the ZP support belongs.

I agree.  That said for now I'm not aware of any other ZP devices.  It also
is unclear whether or not their requirements would be the same for the
ZPODD devices. 

> On the other hand, the sr driver certainly deserves to have some form 
> of runtime PM support, even for devices that aren't ZP.

Yes, it does, but it is unclear to me at this point what it should do in its
callbacks.

Thanks,
Rafael

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Jeff Garzik]

On 09/29/2012 06:31 PM, Rafael J. Wysocki wrote:
> On Saturday, September 29, 2012, Alan Stern wrote:
>> Can arbitrary SCSI devices be ZP, or does this notion apply only to
>> ATAPI-based drives?  That's the key question, and the answer determines
>> where the ZP support belongs.
>
> I agree.  That said for now I'm not aware of any other ZP devices.  It also
> is unclear whether or not their requirements would be the same for the
> ZPODD devices.

Not quite.

The key question is whether or not this operates at the SCSI command set 
level.  ATAPI is simply SCSI MMC command set tunnelling.

The ATA-specific bits that belong in libata include everything below the 
SCSI command set: bus details, delivering the command to the device, 
returning the command response, etc.

sr handles the SCSI command set details.  SATA optical devices are 
aligned with the SCSI MMC command set, which periodically synchronizes 
with USB and ATAPI industry efforts.

There are ugly hacks around the edges, where sometimes ATA or USB 
subsystems may tweak the request or response in passing, but that is the 
general model:  it belongs in libata UNLESS the operation is occurring 
wholly at the SCSI command set level.

Because USB and ATA chose to use the SCSI command set, it is sadly 
inevitable that there might be a few details -- hopefully glossed over 
with layer-hopping hooks and flags -- within 'sr' that are bus-specific.

The simple fact of "only ZPODD devices out there are ATA" is not the 
decision-maker for where the code should live.  It is more a question 
where ZPODD belongs in the device/command set model currently employed.

	Jeff

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Alan Stern]

On Sun, 30 Sep 2012, Jeff Garzik wrote:

> The simple fact of "only ZPODD devices out there are ATA" is not the 
> decision-maker for where the code should live.  It is more a question 
> where ZPODD belongs in the device/command set model currently employed.

I don't really accept this argument.  It's a little like saying: The
tty layer uses ioctl commands to control RS232 line settings; therefore
RS232 settings should be handled in the VFS layer as part of the ioctl
core.

Regardless, according to Aaron the ZP power-off stuff is currently
implemented only in ACPI, tied more closely to the ATA layer than the
SCSI layer (though not part of either).  It is not part of the SCSI
spec in any form.  Now it's true that determining whether a device is
in the right state for power to be removed involves sending a TEST UNIT
READY command, which is of course a SCSI command.  This seems to be
incidental to the overall scheme, however.

Alan Stern

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Jeff Garzik]

On 09/30/2012 03:43 PM, Alan Stern wrote:
> On Sun, 30 Sep 2012, Jeff Garzik wrote:
>
>> The simple fact of "only ZPODD devices out there are ATA" is not the
>> decision-maker for where the code should live.  It is more a question
>> where ZPODD belongs in the device/command set model currently employed.
>
> I don't really accept this argument.  It's a little like saying: The
> tty layer uses ioctl commands to control RS232 line settings; therefore
> RS232 settings should be handled in the VFS layer as part of the ioctl
> core.

Hardly -- sr is an optical device driver, much more aligned.

And libata is probably at least 50% of the entire sr userbase, if not 
much much more.

	Jeff

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Aaron Lu]

On Sun, Sep 30, 2012 at 03:43:27PM -0400, Alan Stern wrote:
> On Sun, 30 Sep 2012, Jeff Garzik wrote:
> 
> > The simple fact of "only ZPODD devices out there are ATA" is not the 
> > decision-maker for where the code should live.  It is more a question 
> > where ZPODD belongs in the device/command set model currently employed.
> 
> I don't really accept this argument.  It's a little like saying: The
> tty layer uses ioctl commands to control RS232 line settings; therefore
> RS232 settings should be handled in the VFS layer as part of the ioctl
> core.
> 
> Regardless, according to Aaron the ZP power-off stuff is currently
> implemented only in ACPI, tied more closely to the ATA layer than the
> SCSI layer (though not part of either).  It is not part of the SCSI
> spec in any form.

The mechanism of SATA ODD zero power model is specified in Mount Fuji
spec v8 section 15 describing what the ODD needs support, how to sense
if the ODD is ready to be powered off and on power up what needs to be
done, etc. And the actual power off and wakeup is implemented in ACPI
and SATA.

> Now it's true that determining whether a device is
> in the right state for power to be removed involves sending a TEST UNIT
> READY command, which is of course a SCSI command.  This seems to be
> incidental to the overall scheme, however.

I need to add that, there are 2 schemes to sense if the ODD is ready to
be powered off:
1 the one I used here, by checking if the door is closed and no media
  inside with test_unit_ready;
2 some ZP capable ODDs can report zero power ready(ZPReady) event to
  host when queried, eliminating the need of host checking the conditions.

Thanks,
Aaron

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[James Bottomley]

On Mon, 2012-10-08 at 17:27 +0800, Aaron Lu wrote:
> On Sun, Sep 30, 2012 at 03:43:27PM -0400, Alan Stern wrote:
> > On Sun, 30 Sep 2012, Jeff Garzik wrote:
> > 
> > > The simple fact of "only ZPODD devices out there are ATA" is not the 
> > > decision-maker for where the code should live.  It is more a question 
> > > where ZPODD belongs in the device/command set model currently employed.
> > 
> > I don't really accept this argument.  It's a little like saying: The
> > tty layer uses ioctl commands to control RS232 line settings; therefore
> > RS232 settings should be handled in the VFS layer as part of the ioctl
> > core.
> > 
> > Regardless, according to Aaron the ZP power-off stuff is currently
> > implemented only in ACPI, tied more closely to the ATA layer than the
> > SCSI layer (though not part of either).  It is not part of the SCSI
> > spec in any form.
> 
> The mechanism of SATA ODD zero power model is specified in Mount Fuji
> spec v8 section 15 describing what the ODD needs support, how to sense
> if the ODD is ready to be powered off and on power up what needs to be
> done, etc. And the actual power off and wakeup is implemented in ACPI
> and SATA.
> 
> > Now it's true that determining whether a device is
> > in the right state for power to be removed involves sending a TEST UNIT
> > READY command, which is of course a SCSI command.  This seems to be
> > incidental to the overall scheme, however.
> 
> I need to add that, there are 2 schemes to sense if the ODD is ready to
> be powered off:
> 1 the one I used here, by checking if the door is closed and no media
>   inside with test_unit_ready;
> 2 some ZP capable ODDs can report zero power ready(ZPReady) event to
>   host when queried, eliminating the need of host checking the conditions.

The way I read the standard is that ZP ODD is a hack to try and get to
off and ZPready is the same hack but integrated into the standardised
power management states (and hence available to normal power saving).
The standard even implies ZP ODD is a less desirable hack by
recommending the use of ZPready.

The ZPready method, being an extension of the usual SCSI power
management model, is pretty much what we support today (especially as
the whole thing is timer driven from values in the mode page and happens
pretty much invisibly to us).

Since the object is to make this as painless as possible, why don't we
just implement ZPODD the way the spec recommends?  i.e. emulate the
timers at an incredibly low level and intercept and emulate the non-disk
commands listed in table 321.  I bet, in Linux, since we moved basically
to GET_EVENT_STATUS_NOTIFICATION, that's the only one that actually
needs an emulation.

The state model seems to work if you snoop the polled media event, so
you wait for no media, then set your internal timer, stop it if we get a
media change and power off the device after interval expiry.  Thereafter
you emulate media event with no change keeping the device powered off.
If a disc gets inserted or the eject button is pressed, you see that via
the SATA PHY events (so wake the drive) and if the Upper Layer sends an
unexpected command, you also power on the drive.

That way all of this should be nicely containable within SATA/ACPI.

James

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Aaron Lu]

On 10/08/2012 06:21 PM, James Bottomley wrote:
> On Mon, 2012-10-08 at 17:27 +0800, Aaron Lu wrote:
>> On Sun, Sep 30, 2012 at 03:43:27PM -0400, Alan Stern wrote:
>>> On Sun, 30 Sep 2012, Jeff Garzik wrote:
>>>
>>>> The simple fact of "only ZPODD devices out there are ATA" is not the 
>>>> decision-maker for where the code should live.  It is more a question 
>>>> where ZPODD belongs in the device/command set model currently employed.
>>>
>>> I don't really accept this argument.  It's a little like saying: The
>>> tty layer uses ioctl commands to control RS232 line settings; therefore
>>> RS232 settings should be handled in the VFS layer as part of the ioctl
>>> core.
>>>
>>> Regardless, according to Aaron the ZP power-off stuff is currently
>>> implemented only in ACPI, tied more closely to the ATA layer than the
>>> SCSI layer (though not part of either).  It is not part of the SCSI
>>> spec in any form.
>>
>> The mechanism of SATA ODD zero power model is specified in Mount Fuji
>> spec v8 section 15 describing what the ODD needs support, how to sense
>> if the ODD is ready to be powered off and on power up what needs to be
>> done, etc. And the actual power off and wakeup is implemented in ACPI
>> and SATA.
>>
>>> Now it's true that determining whether a device is
>>> in the right state for power to be removed involves sending a TEST UNIT
>>> READY command, which is of course a SCSI command.  This seems to be
>>> incidental to the overall scheme, however.
>>
>> I need to add that, there are 2 schemes to sense if the ODD is ready to
>> be powered off:
>> 1 the one I used here, by checking if the door is closed and no media
>>   inside with test_unit_ready;
>> 2 some ZP capable ODDs can report zero power ready(ZPReady) event to
>>   host when queried, eliminating the need of host checking the conditions.
> 
> The way I read the standard is that ZP ODD is a hack to try and get to

Thanks for your time.

> off and ZPready is the same hack but integrated into the standardised
> power management states (and hence available to normal power saving).
> The standard even implies ZP ODD is a less desirable hack by
> recommending the use of ZPready.

There are ZPODDs not supporting ZPready and I want to support them so
the sense scheme 1 is used.

> 
> The ZPready method, being an extension of the usual SCSI power
> management model, is pretty much what we support today (especially as
> the whole thing is timer driven from values in the mode page and happens
> pretty much invisibly to us).
> 
> Since the object is to make this as painless as possible, why don't we
> just implement ZPODD the way the spec recommends?  i.e. emulate the
> timers at an incredibly low level and intercept and emulate the non-disk
> commands listed in table 321.  I bet, in Linux, since we moved basically
> to GET_EVENT_STATUS_NOTIFICATION, that's the only one that actually
> needs an emulation.
> 
> The state model seems to work if you snoop the polled media event, so
> you wait for no media, then set your internal timer, stop it if we get a
> media change and power off the device after interval expiry.  Thereafter
> you emulate media event with no change keeping the device powered off.
> If a disc gets inserted or the eject button is pressed, you see that via
> the SATA PHY events (so wake the drive) and if the Upper Layer sends an
> unexpected command, you also power on the drive.
> 
> That way all of this should be nicely containable within SATA/ACPI.

Thanks for the suggestion, it is really something that I've never
thought of :-)

But I was hoping to use the runtime pm framework to support ZPODD.
With your suggestion, I don't know how to do this. Maybe I can set the
scsi device representing the ODD to runtime suspended once I decided to
power it off and resume it when I power it up. But there is a problem,
that I'm setting a scsi device's runtime status in ATA layer, this
doesn't feel right. And someday, someone may want to add runtime pm
support for sr and the runtime status of the scsi device will be messed.

The reason I want to use runtime PM is, when the scsi device is runtime
suspended, its ancestor devices will have a chance to enter runtime
suspend state, e.g. the sata controller.

Thanks,
Aaron

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[James Bottomley]

On Tue, 2012-10-09 at 15:20 +0800, Aaron Lu wrote:
> On 10/08/2012 06:21 PM, James Bottomley wrote:
> > On Mon, 2012-10-08 at 17:27 +0800, Aaron Lu wrote:
> >> On Sun, Sep 30, 2012 at 03:43:27PM -0400, Alan Stern wrote:
> >>> On Sun, 30 Sep 2012, Jeff Garzik wrote:
> >>>
> >>>> The simple fact of "only ZPODD devices out there are ATA" is not the 
> >>>> decision-maker for where the code should live.  It is more a question 
> >>>> where ZPODD belongs in the device/command set model currently employed.
> >>>
> >>> I don't really accept this argument.  It's a little like saying: The
> >>> tty layer uses ioctl commands to control RS232 line settings; therefore
> >>> RS232 settings should be handled in the VFS layer as part of the ioctl
> >>> core.
> >>>
> >>> Regardless, according to Aaron the ZP power-off stuff is currently
> >>> implemented only in ACPI, tied more closely to the ATA layer than the
> >>> SCSI layer (though not part of either).  It is not part of the SCSI
> >>> spec in any form.
> >>
> >> The mechanism of SATA ODD zero power model is specified in Mount Fuji
> >> spec v8 section 15 describing what the ODD needs support, how to sense
> >> if the ODD is ready to be powered off and on power up what needs to be
> >> done, etc. And the actual power off and wakeup is implemented in ACPI
> >> and SATA.
> >>
> >>> Now it's true that determining whether a device is
> >>> in the right state for power to be removed involves sending a TEST UNIT
> >>> READY command, which is of course a SCSI command.  This seems to be
> >>> incidental to the overall scheme, however.
> >>
> >> I need to add that, there are 2 schemes to sense if the ODD is ready to
> >> be powered off:
> >> 1 the one I used here, by checking if the door is closed and no media
> >>   inside with test_unit_ready;
> >> 2 some ZP capable ODDs can report zero power ready(ZPReady) event to
> >>   host when queried, eliminating the need of host checking the conditions.
> > 
> > The way I read the standard is that ZP ODD is a hack to try and get to
> 
> Thanks for your time.
> 
> > off and ZPready is the same hack but integrated into the standardised
> > power management states (and hence available to normal power saving).
> > The standard even implies ZP ODD is a less desirable hack by
> > recommending the use of ZPready.
> 
> There are ZPODDs not supporting ZPready and I want to support them so
> the sense scheme 1 is used.

Right, but what I'm saying is that ZPODD looks like a hack until ZPready
is fairly universally implemented.  ZPready makes far more sense since
it's integrated into the usual SCSI power management, so ZPODD should
have a limited shelf life.

> > 
> > The ZPready method, being an extension of the usual SCSI power
> > management model, is pretty much what we support today (especially as
> > the whole thing is timer driven from values in the mode page and happens
> > pretty much invisibly to us).
> > 
> > Since the object is to make this as painless as possible, why don't we
> > just implement ZPODD the way the spec recommends?  i.e. emulate the
> > timers at an incredibly low level and intercept and emulate the non-disk
> > commands listed in table 321.  I bet, in Linux, since we moved basically
> > to GET_EVENT_STATUS_NOTIFICATION, that's the only one that actually
> > needs an emulation.
> > 
> > The state model seems to work if you snoop the polled media event, so
> > you wait for no media, then set your internal timer, stop it if we get a
> > media change and power off the device after interval expiry.  Thereafter
> > you emulate media event with no change keeping the device powered off.
> > If a disc gets inserted or the eject button is pressed, you see that via
> > the SATA PHY events (so wake the drive) and if the Upper Layer sends an
> > unexpected command, you also power on the drive.
> > 
> > That way all of this should be nicely containable within SATA/ACPI.
> 
> Thanks for the suggestion, it is really something that I've never
> thought of :-)
> 
> But I was hoping to use the runtime pm framework to support ZPODD.

Well, the runtime pm framework doesn't support the current SCSI power
management states within the drive, that's why it makes sense to treat
what is essentially a hack to them in the same manner.

> With your suggestion, I don't know how to do this. Maybe I can set the
> scsi device representing the ODD to runtime suspended once I decided to
> power it off and resume it when I power it up. But there is a problem,
> that I'm setting a scsi device's runtime status in ATA layer, this
> doesn't feel right. And someday, someone may want to add runtime pm
> support for sr and the runtime status of the scsi device will be messed.

No, if we ever actually supported the standard power management states,
you'd simply be intercepting the SCSI commands that forced the state
transitions (START_STOP_UNIT) and act when yours was forced.

> The reason I want to use runtime PM is, when the scsi device is runtime
> suspended, its ancestor devices will have a chance to enter runtime
> suspend state, e.g. the sata controller.

But this, I think, is why it looks odd.  You're implementing a lower
state than standard SCSI power model which the current runtime pm
doesn't support anyway.

There is, of course, the question of whether we *should* force the SCSI
transitions in runtime pm ...  but that's orthogonal to this discussion.

James

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Aaron Lu]

On Tue, Oct 09, 2012 at 03:58:34PM +0100, James Bottomley wrote:
> On Tue, 2012-10-09 at 15:20 +0800, Aaron Lu wrote:
> > On 10/08/2012 06:21 PM, James Bottomley wrote:
> > > On Mon, 2012-10-08 at 17:27 +0800, Aaron Lu wrote:
> > >> On Sun, Sep 30, 2012 at 03:43:27PM -0400, Alan Stern wrote:
> > >>> On Sun, 30 Sep 2012, Jeff Garzik wrote:
> > >>>
> > >>>> The simple fact of "only ZPODD devices out there are ATA" is not the 
> > >>>> decision-maker for where the code should live.  It is more a question 
> > >>>> where ZPODD belongs in the device/command set model currently employed.
> > >>>
> > >>> I don't really accept this argument.  It's a little like saying: The
> > >>> tty layer uses ioctl commands to control RS232 line settings; therefore
> > >>> RS232 settings should be handled in the VFS layer as part of the ioctl
> > >>> core.
> > >>>
> > >>> Regardless, according to Aaron the ZP power-off stuff is currently
> > >>> implemented only in ACPI, tied more closely to the ATA layer than the
> > >>> SCSI layer (though not part of either).  It is not part of the SCSI
> > >>> spec in any form.
> > >>
> > >> The mechanism of SATA ODD zero power model is specified in Mount Fuji
> > >> spec v8 section 15 describing what the ODD needs support, how to sense
> > >> if the ODD is ready to be powered off and on power up what needs to be
> > >> done, etc. And the actual power off and wakeup is implemented in ACPI
> > >> and SATA.
> > >>
> > >>> Now it's true that determining whether a device is
> > >>> in the right state for power to be removed involves sending a TEST UNIT
> > >>> READY command, which is of course a SCSI command.  This seems to be
> > >>> incidental to the overall scheme, however.
> > >>
> > >> I need to add that, there are 2 schemes to sense if the ODD is ready to
> > >> be powered off:
> > >> 1 the one I used here, by checking if the door is closed and no media
> > >>   inside with test_unit_ready;
> > >> 2 some ZP capable ODDs can report zero power ready(ZPReady) event to
> > >>   host when queried, eliminating the need of host checking the conditions.
> > > 
> > > The way I read the standard is that ZP ODD is a hack to try and get to
> > 
> > Thanks for your time.
> > 
> > > off and ZPready is the same hack but integrated into the standardised
> > > power management states (and hence available to normal power saving).
> > > The standard even implies ZP ODD is a less desirable hack by
> > > recommending the use of ZPready.
> > 
> > There are ZPODDs not supporting ZPready and I want to support them so
> > the sense scheme 1 is used.
> 
> Right, but what I'm saying is that ZPODD looks like a hack until ZPready
> is fairly universally implemented.  ZPready makes far more sense since
> it's integrated into the usual SCSI power management, so ZPODD should
> have a limited shelf life.

I hope so :-)

> 
> > > 
> > > The ZPready method, being an extension of the usual SCSI power
> > > management model, is pretty much what we support today (especially as
> > > the whole thing is timer driven from values in the mode page and happens
> > > pretty much invisibly to us).
> > > 
> > > Since the object is to make this as painless as possible, why don't we
> > > just implement ZPODD the way the spec recommends?  i.e. emulate the
> > > timers at an incredibly low level and intercept and emulate the non-disk
> > > commands listed in table 321.  I bet, in Linux, since we moved basically
> > > to GET_EVENT_STATUS_NOTIFICATION, that's the only one that actually
> > > needs an emulation.
> > > 
> > > The state model seems to work if you snoop the polled media event, so
> > > you wait for no media, then set your internal timer, stop it if we get a
> > > media change and power off the device after interval expiry.  Thereafter
> > > you emulate media event with no change keeping the device powered off.
> > > If a disc gets inserted or the eject button is pressed, you see that via
> > > the SATA PHY events (so wake the drive) and if the Upper Layer sends an
> > > unexpected command, you also power on the drive.
> > > 
> > > That way all of this should be nicely containable within SATA/ACPI.
> > 
> > Thanks for the suggestion, it is really something that I've never
> > thought of :-)
> > 
> > But I was hoping to use the runtime pm framework to support ZPODD.
> 
> Well, the runtime pm framework doesn't support the current SCSI power
> management states within the drive, that's why it makes sense to treat
> what is essentially a hack to them in the same manner.

OK, I agree, and it amused me a little bit :-)

And here are some thoughts on runtime pm regarding SCSI power state in
ODD's case.

The Mount Fuji spec has words like this in section 16.1.1:
When no media is mounted, the logical unit should enter Standby state.
So we do not need to do anything for no media case if Standby is
acceptable.

And when there is media inside, the ODD's power state will either be
controlled by its internal timer or by host's command.

So for runtime pm code to participate, the only place to do some work is
when there is media inside and we decide when the ODD is not in use and
place it into Standby and when the ODD is in use, we place it into Active.
But in this case, it seems better we let the ODD handle this power state
transition by itself by programming its internal timer.

So there doesn't seem to be anything related to SCSI power state that
runtime pm code needs to do for media inside case either.

The runtime pm support for sr may still be desired in that we can give
the pm core a hint that this device is now not in use and its ancestor
devices can be runtime suspended now(as already suggested by some people
several times), but no need to touch the SCSI power state of the device.

Following this thought, I can see why you people are suggesting me to
implement ZPODD in SATA. I think we can totally forget setting power
state in sr's runtime code. The runtime pm code for sr should just serve
as a hint to pm core, and the actual power state should be either
handled by the device itself(for the standard power state case using
internal time) or by the underlying layer(for the ZP case).

Thanks,
Aaron

> 
> > With your suggestion, I don't know how to do this. Maybe I can set the
> > scsi device representing the ODD to runtime suspended once I decided to
> > power it off and resume it when I power it up. But there is a problem,
> > that I'm setting a scsi device's runtime status in ATA layer, this
> > doesn't feel right. And someday, someone may want to add runtime pm
> > support for sr and the runtime status of the scsi device will be messed.
> 
> No, if we ever actually supported the standard power management states,
> you'd simply be intercepting the SCSI commands that forced the state
> transitions (START_STOP_UNIT) and act when yours was forced.
> 
> > The reason I want to use runtime PM is, when the scsi device is runtime
> > suspended, its ancestor devices will have a chance to enter runtime
> > suspend state, e.g. the sata controller.
> 
> But this, I think, is why it looks odd.  You're implementing a lower
> state than standard SCSI power model which the current runtime pm
> doesn't support anyway.
> 
> There is, of course, the question of whether we *should* force the SCSI
> transitions in runtime pm ...  but that's orthogonal to this discussion.
> 
> James
> 
> 

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Rafael J. Wysocki]

On Tuesday 09 of October 2012 15:20:39 Aaron Lu wrote:
> On 10/08/2012 06:21 PM, James Bottomley wrote:
> > On Mon, 2012-10-08 at 17:27 +0800, Aaron Lu wrote:
> >> On Sun, Sep 30, 2012 at 03:43:27PM -0400, Alan Stern wrote:
> >>> On Sun, 30 Sep 2012, Jeff Garzik wrote:
> >>>
> >>>> The simple fact of "only ZPODD devices out there are ATA" is not the 
> >>>> decision-maker for where the code should live.  It is more a question 
> >>>> where ZPODD belongs in the device/command set model currently employed.
> >>>
> >>> I don't really accept this argument.  It's a little like saying: The
> >>> tty layer uses ioctl commands to control RS232 line settings; therefore
> >>> RS232 settings should be handled in the VFS layer as part of the ioctl
> >>> core.
> >>>
> >>> Regardless, according to Aaron the ZP power-off stuff is currently
> >>> implemented only in ACPI, tied more closely to the ATA layer than the
> >>> SCSI layer (though not part of either).  It is not part of the SCSI
> >>> spec in any form.
> >>
> >> The mechanism of SATA ODD zero power model is specified in Mount Fuji
> >> spec v8 section 15 describing what the ODD needs support, how to sense
> >> if the ODD is ready to be powered off and on power up what needs to be
> >> done, etc. And the actual power off and wakeup is implemented in ACPI
> >> and SATA.
> >>
> >>> Now it's true that determining whether a device is
> >>> in the right state for power to be removed involves sending a TEST UNIT
> >>> READY command, which is of course a SCSI command.  This seems to be
> >>> incidental to the overall scheme, however.
> >>
> >> I need to add that, there are 2 schemes to sense if the ODD is ready to
> >> be powered off:
> >> 1 the one I used here, by checking if the door is closed and no media
> >>   inside with test_unit_ready;
> >> 2 some ZP capable ODDs can report zero power ready(ZPReady) event to
> >>   host when queried, eliminating the need of host checking the conditions.
> > 
> > The way I read the standard is that ZP ODD is a hack to try and get to
> 
> Thanks for your time.
> 
> > off and ZPready is the same hack but integrated into the standardised
> > power management states (and hence available to normal power saving).
> > The standard even implies ZP ODD is a less desirable hack by
> > recommending the use of ZPready.
> 
> There are ZPODDs not supporting ZPready and I want to support them so
> the sense scheme 1 is used.
> 
> > 
> > The ZPready method, being an extension of the usual SCSI power
> > management model, is pretty much what we support today (especially as
> > the whole thing is timer driven from values in the mode page and happens
> > pretty much invisibly to us).
> > 
> > Since the object is to make this as painless as possible, why don't we
> > just implement ZPODD the way the spec recommends?  i.e. emulate the
> > timers at an incredibly low level and intercept and emulate the non-disk
> > commands listed in table 321.  I bet, in Linux, since we moved basically
> > to GET_EVENT_STATUS_NOTIFICATION, that's the only one that actually
> > needs an emulation.
> > 
> > The state model seems to work if you snoop the polled media event, so
> > you wait for no media, then set your internal timer, stop it if we get a
> > media change and power off the device after interval expiry.  Thereafter
> > you emulate media event with no change keeping the device powered off.
> > If a disc gets inserted or the eject button is pressed, you see that via
> > the SATA PHY events (so wake the drive) and if the Upper Layer sends an
> > unexpected command, you also power on the drive.
> > 
> > That way all of this should be nicely containable within SATA/ACPI.
> 
> Thanks for the suggestion, it is really something that I've never
> thought of :-)

Well, that's what I wanted to say previously, but James expressed it much
better than I could. ;-)

> But I was hoping to use the runtime pm framework to support ZPODD.
> With your suggestion, I don't know how to do this. Maybe I can set the
> scsi device representing the ODD to runtime suspended once I decided to
> power it off and resume it when I power it up. But there is a problem,
> that I'm setting a scsi device's runtime status in ATA layer, this
> doesn't feel right. And someday, someone may want to add runtime pm
> support for sr and the runtime status of the scsi device will be messed.
> 
> The reason I want to use runtime PM is, when the scsi device is runtime
> suspended, its ancestor devices will have a chance to enter runtime
> suspend state, e.g. the sata controller.

You can add runtime PM support for sr that won't do anything hardware-specific
and in addition to that you can do pm_runtime_get_sync() on the parent directly
from the ATA layer when you know it is needed and pm_runtime_put() when you
know it is safe for it to go to a low-power state.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Rafael J. Wysocki]

On Saturday, September 29, 2012, Aaron Lu wrote:
> [Adding more people and list back in]
> 
> On 09/29/2012 05:46 AM, Rafael J. Wysocki wrote:
> > On Friday, September 28, 2012, Aaron Lu wrote:
> >> On 09/28/2012 07:15 AM, Rafael J. Wysocki wrote:
> >>> On Thursday, September 27, 2012, Aaron Lu wrote:
> >>>> On 09/27/2012 05:37 AM, Rafael J. Wysocki wrote:
> >>>>>
> >>>>> Say the user has pressed the eject button.  What does need to happen so that
> >>>>> the tray is physically ejected?
> >>>>
> >>>> The tray is ejected by the ODD itself, host does not have to do anything.
> >>>>
> >>>> There is a command(PREVENT_MEDIUM_REMOVAL) to lock the door so that when
> >>>> user presses the eject button, the tray will not be ejected. This command
> >>>> is usually sent when we have a disc inside and a user space program
> >>>> opened the underlying block device(e.g. /dev/sr0) to read/write data.
> >>>>
> >>>> And host can also eject the tray by sending a START_STOP_UNIT command
> >>>> with param LoEj set to 1 and we have a function called sr_tray_move to
> >>>> do just this. And this is also what I've used to eject the tray after
> >>>> user wakes up the ODD, as when user presses the eject button when the
> >>>> ODD is in zero power state, it can't eject the tray as usual, so host
> >>>> software will need to do this, that's the reason I need to know such
> >>>> information:
> >>>> When ODD is resumed, is it because user wakes it up?
> >>>
> >>> But START_STOP_UNIT eventually causes ata_scsi_start_stop_xlat() to be
> >>
> >> You are following ata case, while the ODD is an atapi device :-)
> >> The translation function is atapi_xlat, but that doesn't affect the idea
> >> here.
> >>
> >>> executed, so I wonder if we really need to go up through the SCSI stack
> >>> to send that command to the drive from there?  It should be possible
> >>> to issue STANDBY/READ VERIFY to the device directly from libata if
> >>> an eject event is signaled through a GPE.
> >>
> >> Yes, this is possible.
> >> Though it doesn't feel very cool, since I have no idea if the ODD is a
> >> tray type or slot type in ATA layer and I'll blindly send this command
> >> to it then, not a problem maybe.
> > 
> > It would be good to verify if that works for slot devices, if possible.
> 
> The ACPI GPE event is triggered when user inserts a disc into a slot
> type ODD, and if I send an eject command to it, the disc will be
> ejected, which is wrong.
> 
> I need to know the loading mechanism(tray type or slot type) of the ODD
> to decide if I should send this command.
> 
> > 
> >> And what do you think of moving the acpi notification code to sr?
> >> http://marc.info/?l=linux-pm&m=134873904332704&w=4
> > 
> > I don't think this is a good idea, quite frankly.  sr seems to be a too
> > generic place for that.
> 
> Does this mean sr can only have code that is useful to all devices it
> manages? i.e. If a piece of code enables a feature for a special kind of
> ODD(like the sata based ZPODD), it shouldn't be done in sr?

If the feature is specific to one special kind of ODD only, then I don't
think sr is the right place to add support for it.

> > Ideally, the whole ZPODD handling should not be visible to the SCSI layer,
> 
> I can see 2 problems:
> - Don't know its loading machanism so we have the problem above;

Does using the need_eject flag address this problem somehow?

> - Need to send command to find out if ODD is zero power ready somewhere
>   in ata layer, this implies the device is doing IO after it is runtime
>   suspended in scsi layer.

There's nothing wrong with accessig suspended devices as long as we know
that they will respond. :-)

> > perhaps except the "no_polling" flag disabling the polling that may be
> > useful for other purposes in principle.
> 
> I hope so, let's hear what other people has to say.
> 
> > 
> > I'm not sure if it's possible at this point, but if not we need to know
> > very precisely why not.
> 
> There is nothing in theory stopping us from doing this in ata layer.
> For the loading mechanism, we can always send an ATAPI command to figure
> it out.
> 
> So gentlemen, I need your opinions on where this ZPODD code should live
> before I can continue this work, thanks.

I would _try_ to add it at the ATA level.

Thanks,
Rafael

Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Aaron Lu]

On Sun, Sep 30, 2012 at 12:27:41AM +0200, Rafael J. Wysocki wrote:
> On Saturday, September 29, 2012, Aaron Lu wrote:
> > [Adding more people and list back in]
> > 
> > On 09/29/2012 05:46 AM, Rafael J. Wysocki wrote:
> > > On Friday, September 28, 2012, Aaron Lu wrote:
> > >> On 09/28/2012 07:15 AM, Rafael J. Wysocki wrote:
> > >>> On Thursday, September 27, 2012, Aaron Lu wrote:
> > >>>> On 09/27/2012 05:37 AM, Rafael J. Wysocki wrote:
> > >>>>>
> > >>>>> Say the user has pressed the eject button.  What does need to happen so that
> > >>>>> the tray is physically ejected?
> > >>>>
> > >>>> The tray is ejected by the ODD itself, host does not have to do anything.
> > >>>>
> > >>>> There is a command(PREVENT_MEDIUM_REMOVAL) to lock the door so that when
> > >>>> user presses the eject button, the tray will not be ejected. This command
> > >>>> is usually sent when we have a disc inside and a user space program
> > >>>> opened the underlying block device(e.g. /dev/sr0) to read/write data.
> > >>>>
> > >>>> And host can also eject the tray by sending a START_STOP_UNIT command
> > >>>> with param LoEj set to 1 and we have a function called sr_tray_move to
> > >>>> do just this. And this is also what I've used to eject the tray after
> > >>>> user wakes up the ODD, as when user presses the eject button when the
> > >>>> ODD is in zero power state, it can't eject the tray as usual, so host
> > >>>> software will need to do this, that's the reason I need to know such
> > >>>> information:
> > >>>> When ODD is resumed, is it because user wakes it up?
> > >>>
> > >>> But START_STOP_UNIT eventually causes ata_scsi_start_stop_xlat() to be
> > >>
> > >> You are following ata case, while the ODD is an atapi device :-)
> > >> The translation function is atapi_xlat, but that doesn't affect the idea
> > >> here.
> > >>
> > >>> executed, so I wonder if we really need to go up through the SCSI stack
> > >>> to send that command to the drive from there?  It should be possible
> > >>> to issue STANDBY/READ VERIFY to the device directly from libata if
> > >>> an eject event is signaled through a GPE.
> > >>
> > >> Yes, this is possible.
> > >> Though it doesn't feel very cool, since I have no idea if the ODD is a
> > >> tray type or slot type in ATA layer and I'll blindly send this command
> > >> to it then, not a problem maybe.
> > > 
> > > It would be good to verify if that works for slot devices, if possible.
> > 
> > The ACPI GPE event is triggered when user inserts a disc into a slot
> > type ODD, and if I send an eject command to it, the disc will be
> > ejected, which is wrong.
> > 
> > I need to know the loading mechanism(tray type or slot type) of the ODD
> > to decide if I should send this command.
> > 
> > > 
> > >> And what do you think of moving the acpi notification code to sr?
> > >> http://marc.info/?l=linux-pm&m=134873904332704&w=4
> > > 
> > > I don't think this is a good idea, quite frankly.  sr seems to be a too
> > > generic place for that.
> > 
> > Does this mean sr can only have code that is useful to all devices it
> > manages? i.e. If a piece of code enables a feature for a special kind of
> > ODD(like the sata based ZPODD), it shouldn't be done in sr?
> 
> If the feature is specific to one special kind of ODD only, then I don't
> think sr is the right place to add support for it.
> 
> > > Ideally, the whole ZPODD handling should not be visible to the SCSI layer,
> > 
> > I can see 2 problems:
> > - Don't know its loading machanism so we have the problem above;
> 
> Does using the need_eject flag address this problem somehow?

The need_eject flag is used to give sr a hint that on resume, please
eject its tray. And sr knows what loading mechanism this ODD is.

> 
> > - Need to send command to find out if ODD is zero power ready somewhere
> >   in ata layer, this implies the device is doing IO after it is runtime
> >   suspended in scsi layer.
> 
> There's nothing wrong with accessig suspended devices as long as we know
> that they will respond. :-)

Oh, I was reading the comments above the struct dev_pm_ops in
/include/linux/pm.h and thought that was a requirement :-)

Thanks,
Aaron

> 
> > > perhaps except the "no_polling" flag disabling the polling that may be
> > > useful for other purposes in principle.
> > 
> > I hope so, let's hear what other people has to say.
> > 
> > > 
> > > I'm not sure if it's possible at this point, but if not we need to know
> > > very precisely why not.
> > 
> > There is nothing in theory stopping us from doing this in ata layer.
> > For the loading mechanism, we can always send an ATAPI command to figure
> > it out.
> > 
> > So gentlemen, I need your opinions on where this ZPODD code should live
> > before I can continue this work, thanks.
> 
> I would _try_ to add it at the ATA level.
> 
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html