| Message ID | 1347438597-5903-3-git-send-email-aaron.lu@intel.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| 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-ide" 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-ide" 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-ide" 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-ide" 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-ide" 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-ide" 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-ide" 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-ide" 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-ide" 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-ide" 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-ide" 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-ide" 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-ide" 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-ide" 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-ide" 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-ide" 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-ide" 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-ide" 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-ide" 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-ide" 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(-)