[PATCH v2 0/4] Support runtime power off of HDD

[PATCH v2 0/4] Support runtime power off of HDD

[Aaron Lu]

v2:
Introduced a new flag sync_before_stop as scsi_stop =
ata_flush_cache + ata_standby_immediate. This flag specifies
that when stop command is to send to this device, please issue
an additional synchronize cache command to it before the stop
command.
Put device into stop power condition on runtime suspend.

v1:
This patch set is baed on v7 ZPODD patches.

Aaron Lu (4):
  scsi: introduce sync_before_stop flag
  scsi: sd: enter stop power condition on runtime suspend
  scsi: sd: set ready_to_power_off for scsi disk
  libata: acpi: set can_power_off for both ODD and HDD

 drivers/ata/libata-acpi.c  | 25 +++++++++++++++++--------
 drivers/ata/libata-scsi.c  |  1 +
 drivers/scsi/sd.c          | 11 +++++++++--
 include/scsi/scsi_device.h |  1 +
 4 files changed, 28 insertions(+), 10 deletions(-)

-- 
1.7.12.21.g871e293

[PATCH v2 1/4] scsi: introduce sync_before_stop flag

[Aaron Lu]

When scsi device received stop command, it will take care of its
internal cache before enters stopped power condition. This command is
translated to standby immediate in libata-scsi, but standby doesn't
imply flush cache for ATA device, so to issue stop command to ATA
device, an additional flush cache has to be issued.

Introduce this flag so that when we are to stop the ATA disk in scsi
disk driver, also flush its internal cache.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 include/scsi/scsi_device.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 4712aa1..26c3621 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -160,6 +160,7 @@ struct scsi_device {
 	unsigned ready_to_power_off:1;	/* Device is ready to be powered off */
 	unsigned powered_off:1;	/* Device is powered off */
 	unsigned may_power_off:1; /* Power off is allowed by user */
+	unsigned sync_before_stop:1; /* Sync cache before stop */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */
-- 
1.7.12.21.g871e293

Re: [PATCH v2 1/4] scsi: introduce sync_before_stop flag

[Oliver Neukum]

On Tuesday 18 September 2012 15:00:28 Aaron Lu wrote:
> When scsi device received stop command, it will take care of its
> internal cache before enters stopped power condition. This command is
> translated to standby immediate in libata-scsi, but standby doesn't
> imply flush cache for ATA device, so to issue stop command to ATA
> device, an additional flush cache has to be issued.

Why not just set WCE?

	Regards
		Oliver

Re: [PATCH v2 1/4] scsi: introduce sync_before_stop flag

[Aaron Lu]

On Tue, Sep 18, 2012 at 09:30:11AM +0200, Oliver Neukum wrote:
> On Tuesday 18 September 2012 15:00:28 Aaron Lu wrote:
> > When scsi device received stop command, it will take care of its
> > internal cache before enters stopped power condition. This command is
> > translated to standby immediate in libata-scsi, but standby doesn't
> > imply flush cache for ATA device, so to issue stop command to ATA
> > device, an additional flush cache has to be issued.
> 
> Why not just set WCE?

This flag is used for devices whose WCE bit is set.

This flag means, when we are to issue a scsi stop command, we need to
issue an additional sync cache command first.

For scsi device, per the spec, there is no such need as the device will
take care of its internal cache when going to stop power condition.

But for ata device, the stop command is translated to standby immediate,
and we have to flush the internal cache before enter standby.

-Aaron

Re: [PATCH v2 1/4] scsi: introduce sync_before_stop flag

[James Bottomley]

On Tue, 2012-09-18 at 15:00 +0800, Aaron Lu wrote:
> When scsi device received stop command, it will take care of its
> internal cache before enters stopped power condition. This command is
> translated to standby immediate in libata-scsi, but standby doesn't
> imply flush cache for ATA device, so to issue stop command to ATA
> device, an additional flush cache has to be issued.
> 
> Introduce this flag so that when we are to stop the ATA disk in scsi
> disk driver, also flush its internal cache.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  include/scsi/scsi_device.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 4712aa1..26c3621 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -160,6 +160,7 @@ struct scsi_device {
>  	unsigned ready_to_power_off:1;	/* Device is ready to be powered off */
>  	unsigned powered_off:1;	/* Device is powered off */
>  	unsigned may_power_off:1; /* Power off is allowed by user */
> +	unsigned sync_before_stop:1; /* Sync cache before stop */

Why do you need this?

Surely it's all conditioned on the WCE flag.  If WCE isn't set, the
cache is write through (or uncached) and there's no need for a sync
before power down.

James

Re: [PATCH v2 1/4] scsi: introduce sync_before_stop flag

[Aaron Lu]

On Tue, Sep 18, 2012 at 08:56:55AM +0100, James Bottomley wrote:
> On Tue, 2012-09-18 at 15:00 +0800, Aaron Lu wrote:
> > When scsi device received stop command, it will take care of its
> > internal cache before enters stopped power condition. This command is
> > translated to standby immediate in libata-scsi, but standby doesn't
> > imply flush cache for ATA device, so to issue stop command to ATA
> > device, an additional flush cache has to be issued.
> > 
> > Introduce this flag so that when we are to stop the ATA disk in scsi
> > disk driver, also flush its internal cache.
> > 
> > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > ---
> >  include/scsi/scsi_device.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> > index 4712aa1..26c3621 100644
> > --- a/include/scsi/scsi_device.h
> > +++ b/include/scsi/scsi_device.h
> > @@ -160,6 +160,7 @@ struct scsi_device {
> >  	unsigned ready_to_power_off:1;	/* Device is ready to be powered off */
> >  	unsigned powered_off:1;	/* Device is powered off */
> >  	unsigned may_power_off:1; /* Power off is allowed by user */
> > +	unsigned sync_before_stop:1; /* Sync cache before stop */
> 
> Why do you need this?
> 
> Surely it's all conditioned on the WCE flag.  If WCE isn't set, the
> cache is write through (or uncached) and there's no need for a sync
> before power down.

The set of this flag doesn't mean we will sync cache for sure.

It's only meaningful when WCE is set, and in that case, it means when we
are to send a stop command to the device, do we need to send an
additional flush cache command first?

In sd_suspend, the cache will be synchronized when:
1 For devices do not support start_stop, always;
2 For devices support start_stop, if it is standard scsi device, never;
  and if it is an ata device(reflected by this newly introduced flag),
  always.

The reason for this is, the scsi's stop command = flush_cache + standby
for ata, so I have to introduce this flag to explicitely do a cache
flush.

Makes sense?

-Aaron

Re: [PATCH v2 1/4] scsi: introduce sync_before_stop flag

[James Bottomley]

On Tue, 2012-09-18 at 16:09 +0800, Aaron Lu wrote:
> On Tue, Sep 18, 2012 at 08:56:55AM +0100, James Bottomley wrote:
> > On Tue, 2012-09-18 at 15:00 +0800, Aaron Lu wrote:
> > > When scsi device received stop command, it will take care of its
> > > internal cache before enters stopped power condition. This command is
> > > translated to standby immediate in libata-scsi, but standby doesn't
> > > imply flush cache for ATA device, so to issue stop command to ATA
> > > device, an additional flush cache has to be issued.
> > > 
> > > Introduce this flag so that when we are to stop the ATA disk in scsi
> > > disk driver, also flush its internal cache.
> > > 
> > > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > > ---
> > >  include/scsi/scsi_device.h | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> > > index 4712aa1..26c3621 100644
> > > --- a/include/scsi/scsi_device.h
> > > +++ b/include/scsi/scsi_device.h
> > > @@ -160,6 +160,7 @@ struct scsi_device {
> > >  	unsigned ready_to_power_off:1;	/* Device is ready to be powered off */
> > >  	unsigned powered_off:1;	/* Device is powered off */
> > >  	unsigned may_power_off:1; /* Power off is allowed by user */
> > > +	unsigned sync_before_stop:1; /* Sync cache before stop */
> > 
> > Why do you need this?
> > 
> > Surely it's all conditioned on the WCE flag.  If WCE isn't set, the
> > cache is write through (or uncached) and there's no need for a sync
> > before power down.
> 
> The set of this flag doesn't mean we will sync cache for sure.
> 
> It's only meaningful when WCE is set, and in that case, it means when we
> are to send a stop command to the device, do we need to send an
> additional flush cache command first?

Then surely it indicates support for ACPI power down and it's wrongly
named?

> In sd_suspend, the cache will be synchronized when:
> 1 For devices do not support start_stop, always;
> 2 For devices support start_stop, if it is standard scsi device, never;
>   and if it is an ata device(reflected by this newly introduced flag),
>   always.

This doesn't look right to me.  I think it's probably just a layering
violation.  For sd, we need to use the standard SCSI commands.  That
means we treat power states as the SCSI standard says.  The fact that
ATA devices may be required to translate START_STOP_UNIT with STANDBY as
a flush followed by one of the ATA standby commands.  This is very
important: if we construct a libata SATL that doesn't conform to the
standards, things will eventually explode when we try to interact with
devices with their own internal SATL (like the LSI card, or various USB
devices) because eventually we'll make one unexpected interaction too
many.

> The reason for this is, the scsi's stop command = flush_cache +
> standby
> for ata, so I have to introduce this flag to explicitely do a cache
> flush.
> 
> Makes sense?

Really, no: your flag equates to "this is an ATA device, treat it
differently" that's knowledge that's not supposed to be in sd unless
absolutely necessary.

James

Re: [PATCH v2 1/4] scsi: introduce sync_before_stop flag

[Aaron Lu]

On Tue, Sep 18, 2012 at 09:20:20AM +0100, James Bottomley wrote:
> On Tue, 2012-09-18 at 16:09 +0800, Aaron Lu wrote:
> > On Tue, Sep 18, 2012 at 08:56:55AM +0100, James Bottomley wrote:
> > > On Tue, 2012-09-18 at 15:00 +0800, Aaron Lu wrote:
> > > > When scsi device received stop command, it will take care of its
> > > > internal cache before enters stopped power condition. This command is
> > > > translated to standby immediate in libata-scsi, but standby doesn't
> > > > imply flush cache for ATA device, so to issue stop command to ATA
> > > > device, an additional flush cache has to be issued.
> > > > 
> > > > Introduce this flag so that when we are to stop the ATA disk in scsi
> > > > disk driver, also flush its internal cache.
> > > > 
> > > > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > > > ---
> > > >  include/scsi/scsi_device.h | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> > > > index 4712aa1..26c3621 100644
> > > > --- a/include/scsi/scsi_device.h
> > > > +++ b/include/scsi/scsi_device.h
> > > > @@ -160,6 +160,7 @@ struct scsi_device {
> > > >  	unsigned ready_to_power_off:1;	/* Device is ready to be powered off */
> > > >  	unsigned powered_off:1;	/* Device is powered off */
> > > >  	unsigned may_power_off:1; /* Power off is allowed by user */
> > > > +	unsigned sync_before_stop:1; /* Sync cache before stop */
> > > 
> > > Why do you need this?
> > > 
> > > Surely it's all conditioned on the WCE flag.  If WCE isn't set, the
> > > cache is write through (or uncached) and there's no need for a sync
> > > before power down.
> > 
> > The set of this flag doesn't mean we will sync cache for sure.
> > 
> > It's only meaningful when WCE is set, and in that case, it means when we
> > are to send a stop command to the device, do we need to send an
> > additional flush cache command first?
> 
> Then surely it indicates support for ACPI power down and it's wrongly
> named?

It's generic ATA requirement that before standby, cache has to be
flushed and unlike scsi, standby does not imply cache flush.

> 
> > In sd_suspend, the cache will be synchronized when:
> > 1 For devices do not support start_stop, always;
> > 2 For devices support start_stop, if it is standard scsi device, never;
> >   and if it is an ata device(reflected by this newly introduced flag),
> >   always.
> 
> This doesn't look right to me.  I think it's probably just a layering
> violation.  For sd, we need to use the standard SCSI commands.  That
> means we treat power states as the SCSI standard says.  The fact that
> ATA devices may be required to translate START_STOP_UNIT with STANDBY as
> a flush followed by one of the ATA standby commands.  This is very
> important: if we construct a libata SATL that doesn't conform to the
> standards, things will eventually explode when we try to interact with
> devices with their own internal SATL (like the LSI card, or various USB
> devices) because eventually we'll make one unexpected interaction too
> many.

I agree that it is better handled in libata's SALT, I tried to do this
but didn't find a good way so I introduced this flag. The SALT is 1-1
mapping, I'm not sure how to handle this 1-2 mapping.

I'll check this again to see how to do it there.

Thanks for your comments.

-Aaron

Re: [PATCH v2 1/4] scsi: introduce sync_before_stop flag

[Jeff Garzik]

On 09/18/2012 04:31 AM, Aaron Lu wrote:
> I agree that it is better handled in libata's SALT, I tried to do this
> but didn't find a good way so I introduced this flag. The SALT is 1-1
> mapping, I'm not sure how to handle this 1-2 mapping.

That is the crux of the matter:  it is rather difficult within the 
current code, if there is not a 1:1 correspondence between SCSI command 
and ATA command.

The solution that works within libata-scsi.c involves creating two 
ata_queued_cmd's, and managing the issuance and completion yourself.

But really -- the most practical route seems to be simply to set WCE, to 
trigger the desired behavior.

	Jeff

[PATCH v2 2/4] scsi: sd: enter stop power condition on runtime suspend

[Aaron Lu]

On runtime suspend, put device into stop power condition.

For standard scsi device, no need to synchronize cache.
For ata devices, issue synchronize cache before enter standby state
as reflected by the sync_before_stop flag.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/ata/libata-scsi.c |  1 +
 drivers/scsi/sd.c         | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 8ec81ca..9307f52 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1117,6 +1117,7 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
 	} else {
 		sdev->sector_size = ata_id_logical_sector_size(dev->id);
 		sdev->manage_start_stop = 1;
+		sdev->sync_before_stop = 1;
 	}
 
 	/*
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 4df73e5..7b0f07a 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2866,18 +2866,24 @@ static int sd_suspend(struct device *dev, pm_message_t mesg)
 {
 	struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
 	int ret = 0;
+	bool sync;
 
 	if (!sdkp)
 		return 0;	/* this can happen */
 
-	if (sdkp->WCE) {
+	if (!sdkp->device->manage_start_stop)
+		sync = !!sdkp->WCE;
+	else
+		sync = sdkp->device->sync_before_stop && !!sdkp->WCE;
+
+	if (sync) {
 		sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
 		ret = sd_sync_cache(sdkp);
 		if (ret)
 			goto done;
 	}
 
-	if ((mesg.event & PM_EVENT_SLEEP) && sdkp->device->manage_start_stop) {
+	if (sdkp->device->manage_start_stop) {
 		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
 		ret = sd_start_stop_device(sdkp, 0);
 	}
-- 
1.7.12.21.g871e293

[PATCH v2 3/4] scsi: sd: set ready_to_power_off for scsi disk

[Aaron Lu]

The ready_to_power_off flag is used to give indication to ATA layer
if this device's power can be removed after runtime suspended from the
perspective of scsi driver.

It is introduced to support zero power ODD. When ODD is runtime
suspended, it may not be OK to remove its power.

But for disk, there doesn't exist a scenario that when the disk is
runtime suspended, it can't be powered off.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/scsi/sd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 7b0f07a..009d30c 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2638,6 +2638,7 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
 
 	sd_printk(KERN_NOTICE, sdkp, "Attached SCSI %sdisk\n",
 		  sdp->removable ? "removable " : "");
+	sdp->ready_to_power_off = 1;
 	scsi_autopm_put_device(sdp);
 	put_device(&sdkp->dev);
 }
-- 
1.7.12.21.g871e293

[PATCH v2 4/4] libata: acpi: set can_power_off for both ODD and HDD

[Aaron Lu]

Hard disk may also be runtime powered off, so set can_power_off flag
for it too if condition satisfies so that the may_power_off sysfs file
will be created for it to give user a chance to disable runtime power
off.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Acked-by: Jeff Garzik <jgarzik@redhat.com>
---
 drivers/ata/libata-acpi.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 24347e0..443c3f2 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -1015,7 +1015,7 @@ static void ata_acpi_add_pm_notifier(struct ata_device *dev)
 	if (ACPI_FAILURE(status))
 		return;
 
-	if (dev->sdev->can_power_off) {
+	if (dev->class == ATA_DEV_ATAPI && dev->sdev->can_power_off) {
 		acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
 			ata_acpi_wake_dev, dev);
 		device_set_run_wake(&dev->sdev->sdev_gendev, true);
@@ -1036,7 +1036,7 @@ static void ata_acpi_remove_pm_notifier(struct ata_device *dev)
 	if (ACPI_FAILURE(status))
 		return;
 
-	if (dev->sdev->can_power_off) {
+	if (dev->class == ATA_DEV_ATAPI && dev->sdev->can_power_off) {
 		device_set_run_wake(&dev->sdev->sdev_gendev, false);
 		acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
 			ata_acpi_wake_dev);
@@ -1140,14 +1140,23 @@ static int ata_acpi_bind_device(struct ata_port *ap, struct scsi_device *sdev,
 
 	/*
 	 * If firmware has _PS3 or _PR3 for this device,
-	 * and this ata ODD device support device attention,
-	 * it means this device can be powered off
+	 * it means this device can be powered off runtime
 	 */
 	states = acpi_dev->power.states;
-	if ((states[ACPI_STATE_D3_HOT].flags.valid ||
-			states[ACPI_STATE_D3_COLD].flags.explicit_set) &&
-			ata_dev->flags & ATA_DFLAG_DA)
-		sdev->can_power_off = 1;
+	if (states[ACPI_STATE_D3_HOT].flags.valid ||
+			states[ACPI_STATE_D3_COLD].flags.explicit_set) {
+		/*
+		 * For ODD, it needs to support device attention or
+		 * it can't be powered up back by user
+		 */
+		if (ata_dev->class == ATA_DEV_ATAPI &&
+				ata_dev->flags & ATA_DFLAG_DA)
+			sdev->can_power_off = 1;
+
+		/* No requirement for hard disk */
+		if (ata_dev->class == ATA_DEV_ATA)
+			sdev->can_power_off = 1;
+	}
 
 	return 0;
 }
-- 
1.7.12.21.g871e293