| Message ID | 1347438597-5903-3-git-send-email-aaron.lu@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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? -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
[CC list trimmed.] On Wednesday, September 26, 2012, Aaron Lu wrote: > 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: [...] > >> 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. Well, I'm not super familiar with the SCSI subsystem internals, as you have probably noticed already, and I'd like to know how the tray eject works without the ZPODD feature (i.e. in the current mainline). Say the user has pressed the eject button. What does need to happen so that the tray is physically ejected? 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
On 09/27/2012 05:37 AM, Rafael J. Wysocki wrote: > [CC list trimmed.] > > On Wednesday, September 26, 2012, Aaron Lu wrote: >> 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: > > [...] > >>>> 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. > > Well, I'm not super familiar with the SCSI subsystem internals, as you have > probably noticed already, and I'd like to know how the tray eject works > without the ZPODD feature (i.e. in the current mainline). First of all, I must say thank you for your kind help. I know this is not your area, so it's really kind of you to help me here. I wish I can handle this myself, but obviously I'm not capable enough... And thank you Oliver and Alan, for your kind suggestions and solutions to the problems that I've encountered. > > 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? To conclude: - ODD will eject the tray when user presses the eject button, unless host software has locked the door; - When ODD is in zero power state, it will no longer eject the tray when the eject button is pressed, host software will need to eject the tray after the ODD regains power, or user will be confused. I hope I've explained this clearly. Thanks, Aaron -- 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
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 -- 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
On Thursday, September 27, 2012, Aaron Lu wrote: > On 09/27/2012 05:37 AM, Rafael J. Wysocki wrote: > > [CC list trimmed.] > > > > On Wednesday, September 26, 2012, Aaron Lu wrote: > >> 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: > > > > [...] > > > >>>> 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. > > > > Well, I'm not super familiar with the SCSI subsystem internals, as you have > > probably noticed already, and I'd like to know how the tray eject works > > without the ZPODD feature (i.e. in the current mainline). > > First of all, I must say thank you for your kind help. > I know this is not your area, so it's really kind of you to help me > here. I wish I can handle this myself, but obviously I'm not capable > enough... Well, no problem. I'm learning from that too. :-) > And thank you Oliver and Alan, for your kind suggestions and solutions > to the problems that I've encountered. > > > > > 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 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. > To conclude: > - ODD will eject the tray when user presses the eject button, unless > host software has locked the door; > - When ODD is in zero power state, it will no longer eject the tray when > the eject button is pressed, host software will need to eject the tray > after the ODD regains power, or user will be confused. > > I hope I've explained this clearly. Yes, you have, 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
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. And what do you think of moving the acpi notification code to sr? http://marc.info/?l=linux-pm&m=134873904332704&w=4 Thanks, Aaron -- 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
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 > -- 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
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. > 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. Ideally, the whole ZPODD handling should not be visible to the SCSI layer, perhaps except the "no_polling" flag disabling the polling that may be useful for other purposes in principle. I'm not sure if it's possible at this point, but if not we need to know very precisely why not. 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
[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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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
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 > > -- 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
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);
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(-)