| Message ID | 1346768680-7287-2-git-send-email-aaron.lwe@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On Tuesday 04 September 2012 22:24:34 Aaron Lu wrote: > From: Aaron Lu <aaron.lu@intel.com> > > The ODD will be placed into suspend state when: > 1 For tray type ODD, no media inside and door closed; > 2 For slot type ODD, no media inside; > And together with ACPI, when we suspend the ODD's parent(the port it > attached to), we will omit the power altogether to reduce power > consumption(done in libata-acpi.c). Well, this is quite a layering violation. You encode that specific requirement in the generic sr_suspend() > The ODD can be resumed either by user or by software. > > For user to resume the suspended ODD: > 1 For tray type ODD, press the eject button; > 2 For slot type ODD, insert a disc; > Once such events happened, an ACPI notification will be sent and in our > handler, we will resume the ODD(again in libata-acpi.c). > This kind of resume requires platform and device support and is > reflected by the can_power_off flag. > > For software to resume the suspended ODD, we did this in ODD's > open/release and check_event function. > > Signed-off-by: Aaron Lu <aaron.lu@intel.com> > --- > drivers/ata/libata-acpi.c | 6 ++-- > drivers/scsi/sr.c | 76 ++++++++++++++++++++++++++++++++++++++++++++-- > include/scsi/scsi_device.h | 1 + > 3 files changed, 79 insertions(+), 4 deletions(-) > > diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c > index 902b5a4..64ba43d 100644 > --- a/drivers/ata/libata-acpi.c > +++ b/drivers/ata/libata-acpi.c > @@ -985,8 +985,10 @@ static void ata_acpi_wake_dev(acpi_handle handle, u32 event, void *context) > struct ata_device *ata_dev = context; > > if (event == ACPI_NOTIFY_DEVICE_WAKE && ata_dev && > - pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) > - scsi_autopm_get_device(ata_dev->sdev); > + pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) { > + ata_dev->sdev->wakeup_by_user = 1; That flag is badly named. Something like "insert_event_during_suspend" would be better. [..] > +static int sr_suspend(struct device *dev, pm_message_t msg) > +{ > + int suspend; > + struct scsi_sense_hdr sshdr; > + struct scsi_cd *cd = dev_get_drvdata(dev); > + > + /* no action for system pm */ > + if (!PMSG_IS_AUTO(msg)) > + return 0; > + > + /* > + * ODD can be runtime suspended when: > + * tray type: no media inside and tray closed > + * slot type: no media inside > + */ > + scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr); > + > + if (cd->cdi.mask & CDC_CLOSE_TRAY) > + /* no media for caddy/slot type ODD */ > + suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a; > + else > + /* no media and door closed for tray type ODD */ > + suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a && > + sshdr.ascq == 0x01; That requirement is valid for a specific type of disk. You cannot put it into generic sd_suspend(). And even so I don't see why you wouldn't want to suspend for example a drive with an inserted but unopened disk. > + > + if (!suspend) > + return -EBUSY; > + > + return 0; > +} 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 Tue, 4 Sep 2012, Oliver Neukum wrote: > On Tuesday 04 September 2012 22:24:34 Aaron Lu wrote: > > From: Aaron Lu <aaron.lu@intel.com> > > > > The ODD will be placed into suspend state when: > > 1 For tray type ODD, no media inside and door closed; > > 2 For slot type ODD, no media inside; > > And together with ACPI, when we suspend the ODD's parent(the port it > > attached to), we will omit the power altogether to reduce power > > consumption(done in libata-acpi.c). > > Well, this is quite a layering violation. You encode that specific requirement > in the generic sr_suspend() What requirement are you talking about? The "no media and door closed" thing? How is that a layering violation? Are you suggesting this should go into the CD layer instead? > > @@ -985,8 +985,10 @@ static void ata_acpi_wake_dev(acpi_handle handle, u32 event, void *context) > > struct ata_device *ata_dev = context; > > > > if (event == ACPI_NOTIFY_DEVICE_WAKE && ata_dev && > > - pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) > > - scsi_autopm_get_device(ata_dev->sdev); > > + pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) { > > + ata_dev->sdev->wakeup_by_user = 1; > > That flag is badly named. Something like "insert_event_during_suspend" > would be better. What happens on non-ACPI systems when a new disc is inserted into a suspended ODD? How does the drive let the computer know that an insert event has occurred? > > + if (cd->cdi.mask & CDC_CLOSE_TRAY) > > + /* no media for caddy/slot type ODD */ > > + suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a; > > + else > > + /* no media and door closed for tray type ODD */ > > + suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a && > > + sshdr.ascq == 0x01; > > That requirement is valid for a specific type of disk. You cannot put it > into generic sd_suspend(). You mean sr_suspend(). What's not generic about it? > And even so I don't see why you wouldn't > want to suspend for example a drive with an inserted but unopened disk. I assume that Aaron wanted to handle the easiest case first. Adding suspend/resume handling to the open/close routines can be done later. 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 Tuesday 04 September 2012 13:59:38 Alan Stern wrote: > On Tue, 4 Sep 2012, Oliver Neukum wrote: > > > On Tuesday 04 September 2012 22:24:34 Aaron Lu wrote: > > > From: Aaron Lu <aaron.lu@intel.com> > > > > > > The ODD will be placed into suspend state when: > > > 1 For tray type ODD, no media inside and door closed; > > > 2 For slot type ODD, no media inside; > > > And together with ACPI, when we suspend the ODD's parent(the port it > > > attached to), we will omit the power altogether to reduce power > > > consumption(done in libata-acpi.c). > > > > Well, this is quite a layering violation. You encode that specific requirement > > in the generic sr_suspend() > > What requirement are you talking about? The "no media and door closed" > thing? How is that a layering violation? Are you suggesting this There is no reason this requirement should apply to any other drive than the device this is aimed at. It comes from the ability of this specific combination to detect medium changes. > should go into the CD layer instead? No. It needs to be specific to a certain hardware. It needs to be a callback. > > > @@ -985,8 +985,10 @@ static void ata_acpi_wake_dev(acpi_handle handle, u32 event, void *context) > > > struct ata_device *ata_dev = context; > > > > > > if (event == ACPI_NOTIFY_DEVICE_WAKE && ata_dev && > > > - pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) > > > - scsi_autopm_get_device(ata_dev->sdev); > > > + pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) { > > > + ata_dev->sdev->wakeup_by_user = 1; > > > > That flag is badly named. Something like "insert_event_during_suspend" > > would be better. > > What happens on non-ACPI systems when a new disc is inserted into a > suspended ODD? How does the drive let the computer know that an insert > event has occurred? Good question. Again either the kernel polls the drive or there a mechanism specific to the hardware. > > > + if (cd->cdi.mask & CDC_CLOSE_TRAY) > > > + /* no media for caddy/slot type ODD */ > > > + suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a; > > > + else > > > + /* no media and door closed for tray type ODD */ > > > + suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a && > > > + sshdr.ascq == 0x01; > > > > That requirement is valid for a specific type of disk. You cannot put it > > into generic sd_suspend(). > > You mean sr_suspend(). What's not generic about it? Yes. We may encounter drives which cannot register a medium change while suspended, but can be safely suspended while their door is locked. > > And even so I don't see why you wouldn't > > want to suspend for example a drive with an inserted but unopened disk. > > I assume that Aaron wanted to handle the easiest case first. Adding > suspend/resume handling to the open/close routines can be done later. Sure, but this patch blocks that in contrast to just not implementing it. 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 Tue, Sep 04, 2012 at 08:47:15PM +0200, Oliver Neukum wrote: > On Tuesday 04 September 2012 13:59:38 Alan Stern wrote: > > On Tue, 4 Sep 2012, Oliver Neukum wrote: > > > > > On Tuesday 04 September 2012 22:24:34 Aaron Lu wrote: > > > > From: Aaron Lu <aaron.lu@intel.com> > > > > > > > > The ODD will be placed into suspend state when: > > > > 1 For tray type ODD, no media inside and door closed; > > > > 2 For slot type ODD, no media inside; > > > > And together with ACPI, when we suspend the ODD's parent(the port it > > > > attached to), we will omit the power altogether to reduce power > > > > consumption(done in libata-acpi.c). > > > > > > Well, this is quite a layering violation. You encode that specific requirement > > > in the generic sr_suspend() > > > > What requirement are you talking about? The "no media and door closed" > > thing? How is that a layering violation? Are you suggesting this > > There is no reason this requirement should apply to any other drive than the > device this is aimed at. It comes from the ability of this specific combination > to detect medium changes. When suspending, I'll check the "no media inside and door closed" condition. Do you mean this is a special ability for devices driven by sr driver? And after the device is runtime suspended, if the platform has the ability to power off the device and the device supports notifying the host of user action(i.e inserting a disc to a slot type ODD or pressing the eject button of a tray type ODD), it will be powered off to save more power. This is ZPODD device. By detecting medium change, I suppose you mean when the device is runtime suspended, how can it detect medium change? There are 2 situations here: 1 For normal devices, the in kernel poll is still going on; 2 For ZPODD device, it will be powered off when runtime suspended. And the in kernel poll is stopped to let the device stay in powered off state longer(done in patch 3). when user wants to use the ODD by either inserting a disc or pressing the eject button, an ACPI event is generated and the notification code will runtime resume the device and the in kernel poll is restarted. And then the medium change can be detected. > > > should go into the CD layer instead? > > No. It needs to be specific to a certain hardware. It needs to be a callback. > > > > > @@ -985,8 +985,10 @@ static void ata_acpi_wake_dev(acpi_handle handle, u32 event, void *context) > > > > struct ata_device *ata_dev = context; > > > > > > > > if (event == ACPI_NOTIFY_DEVICE_WAKE && ata_dev && > > > > - pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) > > > > - scsi_autopm_get_device(ata_dev->sdev); > > > > + pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) { > > > > + ata_dev->sdev->wakeup_by_user = 1; > > > > > > That flag is badly named. Something like "insert_event_during_suspend" > > > would be better. This flag means, the reason the device gets runtime resumed is due to user as described by the above situation 2, not by software(e.g. by sr_check_events). I don't quite understand insert_event_during_suspend mean here... > > > > What happens on non-ACPI systems when a new disc is inserted into a > > suspended ODD? How does the drive let the computer know that an insert > > event has occurred? > > Good question. Again either the kernel polls the drive or there a mechanism > specific to the hardware. Yes. And the mechanism is called ZPODD :-) And if ZPODD is supported, I'll block kernel polling; If not, kernel polling is not blocked. > > > > > + if (cd->cdi.mask & CDC_CLOSE_TRAY) > > > > + /* no media for caddy/slot type ODD */ > > > > + suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a; > > > > + else > > > > + /* no media and door closed for tray type ODD */ > > > > + suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a && > > > > + sshdr.ascq == 0x01; > > > > > > That requirement is valid for a specific type of disk. You cannot put it > > > into generic sd_suspend(). > > > > You mean sr_suspend(). What's not generic about it? > > Yes. We may encounter drives which cannot register a medium change while > suspended, but can be safely suspended while their door is locked. There is no need to register a medium change while suspended. The current model of periodically poll is not changed. Only when platform and device both support ZPODD will the in kernel poll be stopped. > > > > And even so I don't see why you wouldn't > > > want to suspend for example a drive with an inserted but unopened disk. > > > > I assume that Aaron wanted to handle the easiest case first. Adding > > suspend/resume handling to the open/close routines can be done later. > > Sure, but this patch blocks that in contrast to just not implementing it. It's already implemented. I did it in scsi_cd_get/put. And the reason I didn't allow runtime suspend for medium inside and door closed case are: 1 ZPODD has a spec and it didn't allow that; 2 It's not easy to implement. Imagine user just inserts a disc, and the sr_suspend routine checks that door is closed so that it wants to suspend the device. But actually, after user just inserts a disc, he definitely wants to use the device, so it's not a good thing to do. And if ZPODD is used, the device will be powered off instantly when the door is closed, this is not good. So I think for now, the "no medium inside and door closed" makes thing easier :-) 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 Wed, 5 Sep 2012, Aaron Lu wrote: > > > > That flag is badly named. Something like "insert_event_during_suspend" > > > > would be better. > > This flag means, the reason the device gets runtime resumed is due to > user as described by the above situation 2, not by software(e.g. by > sr_check_events). > > I don't quite understand insert_event_during_suspend mean here... It means: An insert event occurred while the device was suspended. > > > > And even so I don't see why you wouldn't > > > > want to suspend for example a drive with an inserted but unopened disk. > > > > > > I assume that Aaron wanted to handle the easiest case first. Adding > > > suspend/resume handling to the open/close routines can be done later. > > > > Sure, but this patch blocks that in contrast to just not implementing it. > > It's already implemented. I did it in scsi_cd_get/put. > > And the reason I didn't allow runtime suspend for medium inside and door > closed case are: > 1 ZPODD has a spec and it didn't allow that; In theory we should allow runtime suspend in this case too, but leave the power on. (Although in practice, turning the power off would probably work even though it may violate the spec.) > 2 It's not easy to implement. Imagine user just inserts a disc, and the > sr_suspend routine checks that door is closed so that it wants to > suspend the device. But actually, after user just inserts a disc, he > definitely wants to use the device, so it's not a good thing to do. And > if ZPODD is used, the device will be powered off instantly when the door > is closed, this is not good. That's why we have an autosuspend delay. Although for some reason the SCSI subsystem doesn't use it currently... We need to add a call to pm_runtime_use_autosuspend() in scsi_sysfs_add_sdev(). Likewise, the pm_schedule_suspend() call in scsi_runtime_idle() should be changed to pm_runtime_autosuspend(). And there should be calls to pm_runtime_set_autosuspend_delay() in the sd and sr drivers. 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 Wednesday 05 September 2012 23:22:55 Aaron Lu wrote: > On Tue, Sep 04, 2012 at 08:47:15PM +0200, Oliver Neukum wrote: > > On Tuesday 04 September 2012 13:59:38 Alan Stern wrote: > > > What requirement are you talking about? The "no media and door closed" > > > thing? How is that a layering violation? Are you suggesting this > > > > There is no reason this requirement should apply to any other drive than the > > device this is aimed at. It comes from the ability of this specific combination > > to detect medium changes. > > When suspending, I'll check the "no media inside and door closed" > condition. Do you mean this is a special ability for devices driven by > sr driver? No. This is a special requirement for devices supporting ZPODD. ZPODD are driven by sr. By sr drives many devices which are not sr. For them doing the check you need to do for ZPODD devices is wrong. > And after the device is runtime suspended, if the platform has the > ability to power off the device and the device supports notifying the > host of user action(i.e inserting a disc to a slot type ODD or pressing > the eject button of a tray type ODD), it will be powered off to save > more power. This is ZPODD device. Sure. I am not saying that your patches are wrong for ZPODD devices. The problem is with devices that are not ZPODD. > > > What happens on non-ACPI systems when a new disc is inserted into a > > > suspended ODD? How does the drive let the computer know that an insert > > > event has occurred? > > > > Good question. Again either the kernel polls the drive or there a mechanism > > specific to the hardware. > > Yes. And the mechanism is called ZPODD :-) No. There is a mechanism known as ZPODD. There may be others. > And the reason I didn't allow runtime suspend for medium inside and door > closed case are: > 1 ZPODD has a spec and it didn't allow that; Yet there are drives which are not ZPODD. > 2 It's not easy to implement. Imagine user just inserts a disc, and the > sr_suspend routine checks that door is closed so that it wants to > suspend the device. But actually, after user just inserts a disc, he > definitely wants to use the device, so it's not a good thing to do. And > if ZPODD is used, the device will be powered off instantly when the door > is closed, this is not good. Therefore you use a reasonable delay in the driver core. 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 Wed, Sep 05, 2012 at 12:08:25PM -0400, Alan Stern wrote: > On Wed, 5 Sep 2012, Aaron Lu wrote: > > > > > > That flag is badly named. Something like "insert_event_during_suspend" > > > > > would be better. > > > > This flag means, the reason the device gets runtime resumed is due to > > user as described by the above situation 2, not by software(e.g. by > > sr_check_events). > > > > I don't quite understand insert_event_during_suspend mean here... > > It means: An insert event occurred while the device was suspended. Thanks, I got it now. > > > > > > And even so I don't see why you wouldn't > > > > > want to suspend for example a drive with an inserted but unopened disk. > > > > > > > > I assume that Aaron wanted to handle the easiest case first. Adding > > > > suspend/resume handling to the open/close routines can be done later. > > > > > > Sure, but this patch blocks that in contrast to just not implementing it. > > > > It's already implemented. I did it in scsi_cd_get/put. > > > > And the reason I didn't allow runtime suspend for medium inside and door > > closed case are: > > 1 ZPODD has a spec and it didn't allow that; > > In theory we should allow runtime suspend in this case too, but leave > the power on. (Although in practice, turning the power off would > probably work even though it may violate the spec.) > Agree. But it's not easy to implement with autosuspend. Please see below. > > 2 It's not easy to implement. Imagine user just inserts a disc, and the > > sr_suspend routine checks that door is closed so that it wants to > > suspend the device. But actually, after user just inserts a disc, he > > definitely wants to use the device, so it's not a good thing to do. And > > if ZPODD is used, the device will be powered off instantly when the door > > is closed, this is not good. > > That's why we have an autosuspend delay. Although for some reason the > SCSI subsystem doesn't use it currently... We need to add a call to > pm_runtime_use_autosuspend() in scsi_sysfs_add_sdev(). Likewise, the > pm_schedule_suspend() call in scsi_runtime_idle() should be changed to > pm_runtime_autosuspend(). And there should be calls to > pm_runtime_set_autosuspend_delay() in the sd and sr drivers. I tried to use autosuspend when preparing the patch, but the fact that the devices will be polled every 2 seconds make it impossible to enter suspend state if the autosuspend delay is larger than that. -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 Tue, Sep 04, 2012 at 05:57:35PM +0200, Oliver Neukum wrote: > > diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c > > index 902b5a4..64ba43d 100644 > > --- a/drivers/ata/libata-acpi.c > > +++ b/drivers/ata/libata-acpi.c > > @@ -985,8 +985,10 @@ static void ata_acpi_wake_dev(acpi_handle handle, u32 event, void *context) > > struct ata_device *ata_dev = context; > > > > if (event == ACPI_NOTIFY_DEVICE_WAKE && ata_dev && > > - pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) > > - scsi_autopm_get_device(ata_dev->sdev); > > + pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) { > > + ata_dev->sdev->wakeup_by_user = 1; > > That flag is badly named. Something like "insert_event_during_suspend" > would be better. The reason I added the flag is to let sr knows that when the device gets resumed, please eject its tray since user just pressed the eject button when it is in powered off state. If wakeup_by_user is badly named, what about need_eject? I don't think insert_event_during_suspend reflects what happened here. -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 Thu, 6 Sep 2012, Aaron Lu wrote: > > That's why we have an autosuspend delay. Although for some reason the > > SCSI subsystem doesn't use it currently... We need to add a call to > > pm_runtime_use_autosuspend() in scsi_sysfs_add_sdev(). Likewise, the > > pm_schedule_suspend() call in scsi_runtime_idle() should be changed to > > pm_runtime_autosuspend(). And there should be calls to > > pm_runtime_set_autosuspend_delay() in the sd and sr drivers. > > I tried to use autosuspend when preparing the patch, but the fact that > the devices will be polled every 2 seconds make it impossible to enter > suspend state if the autosuspend delay is larger than that. You can always increase the polling interval. But in the long run that wouldn't be a good solution. What I'd really like is a way to do the status polling without having it reset the idle timer. Oliver, what do you think? Would that be a good solution? 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 Thursday 06 September 2012 11:06:49 Alan Stern wrote: > On Thu, 6 Sep 2012, Aaron Lu wrote: > > > > That's why we have an autosuspend delay. Although for some reason the > > > SCSI subsystem doesn't use it currently... We need to add a call to > > > pm_runtime_use_autosuspend() in scsi_sysfs_add_sdev(). Likewise, the > > > pm_schedule_suspend() call in scsi_runtime_idle() should be changed to > > > pm_runtime_autosuspend(). And there should be calls to > > > pm_runtime_set_autosuspend_delay() in the sd and sr drivers. > > > > I tried to use autosuspend when preparing the patch, but the fact that > > the devices will be polled every 2 seconds make it impossible to enter > > suspend state if the autosuspend delay is larger than that. > > You can always increase the polling interval. > > But in the long run that wouldn't be a good solution. What I'd really > like is a way to do the status polling without having it reset the > idle timer. > > Oliver, what do you think? Would that be a good solution? Well, we could introduce a flag into the requests for the polls. But best would be to simply declare a device immediately idle as soon as we learn that it has no medium. No special casing would be needed. 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 Thu, 6 Sep 2012, Oliver Neukum wrote: > On Thursday 06 September 2012 11:06:49 Alan Stern wrote: > > On Thu, 6 Sep 2012, Aaron Lu wrote: > > > > > > That's why we have an autosuspend delay. Although for some reason the > > > > SCSI subsystem doesn't use it currently... We need to add a call to > > > > pm_runtime_use_autosuspend() in scsi_sysfs_add_sdev(). Likewise, the > > > > pm_schedule_suspend() call in scsi_runtime_idle() should be changed to > > > > pm_runtime_autosuspend(). And there should be calls to > > > > pm_runtime_set_autosuspend_delay() in the sd and sr drivers. > > > > > > I tried to use autosuspend when preparing the patch, but the fact that > > > the devices will be polled every 2 seconds make it impossible to enter > > > suspend state if the autosuspend delay is larger than that. > > > > You can always increase the polling interval. > > > > But in the long run that wouldn't be a good solution. What I'd really > > like is a way to do the status polling without having it reset the > > idle timer. > > > > Oliver, what do you think? Would that be a good solution? > > Well, we could introduce a flag into the requests for the polls. > But best would be to simply declare a device immediately idle > as soon as we learn that it has no medium. No special casing > would be needed. We could do that, but what about idle drives that do have media? 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 Thursday 06 September 2012 13:08:18 Alan Stern wrote: > On Thu, 6 Sep 2012, Oliver Neukum wrote: > > > On Thursday 06 September 2012 11:06:49 Alan Stern wrote: > > > On Thu, 6 Sep 2012, Aaron Lu wrote: > > > > > > > > That's why we have an autosuspend delay. Although for some reason the > > > > > SCSI subsystem doesn't use it currently... We need to add a call to > > > > > pm_runtime_use_autosuspend() in scsi_sysfs_add_sdev(). Likewise, the > > > > > pm_schedule_suspend() call in scsi_runtime_idle() should be changed to > > > > > pm_runtime_autosuspend(). And there should be calls to > > > > > pm_runtime_set_autosuspend_delay() in the sd and sr drivers. > > > > > > > > I tried to use autosuspend when preparing the patch, but the fact that > > > > the devices will be polled every 2 seconds make it impossible to enter > > > > suspend state if the autosuspend delay is larger than that. > > > > > > You can always increase the polling interval. > > > > > > But in the long run that wouldn't be a good solution. What I'd really > > > like is a way to do the status polling without having it reset the > > > idle timer. > > > > > > Oliver, what do you think? Would that be a good solution? > > > > Well, we could introduce a flag into the requests for the polls. > > But best would be to simply declare a device immediately idle > > as soon as we learn that it has no medium. No special casing > > would be needed. > > We could do that, but what about idle drives that do have media? Then we do have a problem. To handle this optimally we'd have to make a difference between the first time a new medium is noticed and later polls. 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 Thu, 6 Sep 2012, Oliver Neukum wrote: > > > > But in the long run that wouldn't be a good solution. What I'd really > > > > like is a way to do the status polling without having it reset the > > > > idle timer. > > > > > > > > Oliver, what do you think? Would that be a good solution? > > > > > > Well, we could introduce a flag into the requests for the polls. > > > But best would be to simply declare a device immediately idle > > > as soon as we learn that it has no medium. No special casing > > > would be needed. > > > > We could do that, but what about idle drives that do have media? > > Then we do have a problem. To handle this optimally we'd have to make > a difference between the first time a new medium is noticed and later > polls. That's right. It shouldn't be too difficult to accomplish. One case to watch out for is a ZPODD with no media and an open door. We should put the drive into runtime suspend immediately, but continue polling and leave the power on. The runtime suspend after each poll will to see if the door is shut; when it is then polling can be turned off and power removed. This leads to a question: How should the sr device tell its parent whether or not to turn off the power? Aaron's current patches simply rely on the device being runtime suspended, but that won't work with this scheme. Do we need a "ready_to_power_down" flag? 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 09/06/2012 11:06 PM, Alan Stern wrote: > On Thu, 6 Sep 2012, Aaron Lu wrote: > >>> That's why we have an autosuspend delay. Although for some reason the >>> SCSI subsystem doesn't use it currently... We need to add a call to >>> pm_runtime_use_autosuspend() in scsi_sysfs_add_sdev(). Likewise, the >>> pm_schedule_suspend() call in scsi_runtime_idle() should be changed to >>> pm_runtime_autosuspend(). And there should be calls to >>> pm_runtime_set_autosuspend_delay() in the sd and sr drivers. >> >> I tried to use autosuspend when preparing the patch, but the fact that >> the devices will be polled every 2 seconds make it impossible to enter >> suspend state if the autosuspend delay is larger than that. > > You can always increase the polling interval. > But in the long run that wouldn't be a good solution. Agree. > What I'd really like is a way to do the status polling without having > it reset the idle timer. I like this, I'll try to see if this can be done. If we idle the device immediately, we would suspend/resume the ODD every 2 seconds, which may not be a good idea. And we can't increate the polling interval that much, like to the level of minutes, and if we can't put the device into suspend state long enough, it may not worth the effort. What do you think? -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 Fri, 7 Sep 2012, Aaron Lu wrote: > > What I'd really like is a way to do the status polling without having > > it reset the idle timer. > > I like this, I'll try to see if this can be done. > > If we idle the device immediately, we would suspend/resume the ODD every > 2 seconds, which may not be a good idea. It may be okay. This depends on how long it takes to suspend and resume the drive and its parents. If it takes no more than a few hundred ms then there shouldn't be any problem. Especially if the polling interval is increased to something like 5 seconds. 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
diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c index 902b5a4..64ba43d 100644 --- a/drivers/ata/libata-acpi.c +++ b/drivers/ata/libata-acpi.c @@ -985,8 +985,10 @@ static void ata_acpi_wake_dev(acpi_handle handle, u32 event, void *context) struct ata_device *ata_dev = context; if (event == ACPI_NOTIFY_DEVICE_WAKE && ata_dev && - pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) - scsi_autopm_get_device(ata_dev->sdev); + pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) { + ata_dev->sdev->wakeup_by_user = 1; + pm_runtime_resume(&ata_dev->sdev->sdev_gendev); + } } static void ata_acpi_add_pm_notifier(struct ata_device *dev) diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c index 5fc97d2..637f4ca 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> @@ -79,6 +80,8 @@ static DEFINE_MUTEX(sr_mutex); static int sr_probe(struct device *); static int sr_remove(struct device *); static int sr_done(struct scsi_cmnd *); +static int sr_suspend(struct device *, pm_message_t msg); +static int sr_resume(struct device *); static struct scsi_driver sr_template = { .owner = THIS_MODULE, @@ -86,6 +89,8 @@ static struct scsi_driver sr_template = { .name = "sr", .probe = sr_probe, .remove = sr_remove, + .suspend = sr_suspend, + .resume = sr_resume, }, .done = sr_done, }; @@ -146,8 +151,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,9 +172,61 @@ 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); } +static int sr_suspend(struct device *dev, pm_message_t msg) +{ + int suspend; + struct scsi_sense_hdr sshdr; + struct scsi_cd *cd = dev_get_drvdata(dev); + + /* no action for system pm */ + if (!PMSG_IS_AUTO(msg)) + return 0; + + /* + * ODD can be runtime suspended when: + * tray type: no media inside and tray closed + * slot type: no media inside + */ + scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr); + + if (cd->cdi.mask & CDC_CLOSE_TRAY) + /* no media for caddy/slot type ODD */ + suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a; + else + /* no media and door closed for tray type ODD */ + suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a && + sshdr.ascq == 0x01; + + if (!suspend) + return -EBUSY; + + return 0; +} + +static int sr_resume(struct device *dev) +{ + struct scsi_cd *cd; + struct scsi_sense_hdr sshdr; + + cd = dev_get_drvdata(dev); + + /* get the disk ready */ + scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr); + + /* if user wakes up the ODD, eject the tray */ + if (cd->device->wakeup_by_user) { + cd->device->wakeup_by_user = 0; + if (!(cd->cdi.mask & CDC_CLOSE_TRAY)) + sr_tray_move(&cd->cdi, 1); + } + + return 0; +} + static unsigned int sr_get_events(struct scsi_device *sdev) { u8 buf[8]; @@ -220,6 +281,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,7 +309,7 @@ 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; @@ -270,7 +333,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 +350,8 @@ do_tur: cd->tur_changed = false; cd->get_event_changed = false; +out: + scsi_autopm_put_device(cd->device); return events; } @@ -718,6 +783,10 @@ static int sr_probe(struct device *dev) 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 +1034,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); diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 9895f69..72d946f 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -156,6 +156,7 @@ struct scsi_device { unsigned is_visible:1; /* is the device visible in sysfs */ unsigned can_power_off:1; /* Device supports runtime power off */ unsigned wce_default_on:1; /* Cache is ON by default */ + unsigned wakeup_by_user:1; /* User wakes up the device */ DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */ struct list_head event_list; /* asserted events */