[PATCH v5 0/7] ZPODD patches

[PATCH v5 0/7] ZPODD patches

[Aaron Lu]

v5:
Add may_power_off flag to scsi device.
Alan Stern suggested that I should not mess runtime suspend with
runtime power off, but the current zpodd implementation made it not
easy to seperate. So I re-wrote the zpodd implementation, the end
result is, normal ODD can also enter runtime suspended state, but
their power won't be removed.

v4:
Rebase on top of Linus' tree, due to this, the problem of a missing
flag in v3 is gone;
Add a new function scsi_autopm_put_device_autosuspend to first mark
last busy for the device and then put autosuspend it as suggested by
Oliver Neukum.
Typo fix as pointed by Sergei Shtylyov.
Check can_power_off flag before any runtime pm operations in sr.

v3:
Rebase on top of scsi-misc tree;
Add the sr related patches previously in Jeff's libata tree;
Re-organize the sr patches.
A problem for now: for patch
scsi: sr: support zero power ODD(ZPODD)
I can't set a flag in libata-acpi.c since a related function is
missing in scsi-misc tree. Will fix this when 3.6-rc1 released.

v2:
Bug fix for v1;
Use scsi_autopm_* in sr driver instead of pm_runtime_*;

v1:
Here are some patches to make ZPODD easier to use for end users and
a fix for using ZPODD with system suspend.


Aaron Lu (7):
  scsi: sr: support runtime pm for ODD
  block: genhd: export disk_(un)block_events
  scsi: sr: block events checking when suspended for zpodd
  libata: acpi: set can_power_off for both ODD and HD
  scsi: pm: add may_power_off flag
  scsi: sr: use may_power_off
  libata: acpi: respect may_power_off flag

 block/genhd.c              |  2 ++
 drivers/ata/libata-acpi.c  | 35 +++++++++++++------
 drivers/scsi/scsi_sysfs.c  | 10 +++---
 drivers/scsi/sr.c          | 85 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/sr.h          |  1 +
 include/scsi/scsi_device.h |  2 ++
 6 files changed, 120 insertions(+), 15 deletions(-)

-- 
1.7.11.5

[PATCH v5 1/7] scsi: sr: support runtime pm for ODD

[Aaron Lu]

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

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          | 72 ++++++++++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_device.h |  1 +
 3 files changed, 77 insertions(+), 2 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;
+		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..04a658b 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,7 +281,9 @@ 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);
+	scsi_autopm_put_device(cd->device);
 	cd->get_event_changed |= events & DISK_EVENT_MEDIA_CHANGE;
 
 	/*
@@ -250,7 +313,9 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 do_tur:
 	/* let's see whether the media is there with TUR */
 	last_present = cd->media_present;
+	scsi_autopm_get_device(cd->device);
 	ret = scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
+	scsi_autopm_put_device(cd->device);
 
 	/*
 	 * Media is considered to be present if TUR succeeds or fails with
@@ -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 */
-- 
1.7.11.5

[PATCH v5 2/7] block: genhd: export disk_(un)block_events

[Aaron Lu]

When ODD is runtime powered off, there is no meaning to check events for
it, so disk_(un)block_events will be called in its suspend/resume
callback.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 block/genhd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/genhd.c b/block/genhd.c
index cac7366..f630150 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1468,6 +1468,7 @@ void disk_block_events(struct gendisk *disk)
 
 	mutex_unlock(&ev->block_mutex);
 }
+EXPORT_SYMBOL(disk_block_events);
 
 static void __disk_unblock_events(struct gendisk *disk, bool check_now)
 {
@@ -1512,6 +1513,7 @@ void disk_unblock_events(struct gendisk *disk)
 	if (disk->ev)
 		__disk_unblock_events(disk, false);
 }
+EXPORT_SYMBOL(disk_unblock_events);
 
 /**
  * disk_flush_events - schedule immediate event checking and flushing
-- 
1.7.11.5

[PATCH v5 3/7] scsi: sr: block events checking when suspended for zpodd

[Aaron Lu]

When ODD is runtime suspended and if it can be powered off(reflected by
the can_power_off flag), we will block events checking for it so that it
can stay in powered off state until resumed by:
1 user pressing the eject button or inserting a disc;
2 software opening the block device.
But not by events checking that normally occurs every 2 seconds.

Old versions of udisk will poll the ODD periodically, so it is advised
to inhibit the polling:
$ udisks --inhibit-polling /dev/sr0

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/scsi/sr.c | 10 ++++++++++
 drivers/scsi/sr.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 04a658b..1acfe0e 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -204,6 +204,11 @@ static int sr_suspend(struct device *dev, pm_message_t msg)
 	if (!suspend)
 		return -EBUSY;
 
+	if (cd->device->can_power_off) {
+		cd->disk_events_blocked = 1;
+		disk_block_events(cd->disk);
+	}
+
 	return 0;
 }
 
@@ -224,6 +229,11 @@ static int sr_resume(struct device *dev)
 			sr_tray_move(&cd->cdi, 1);
 	}
 
+	if (cd->disk_events_blocked) {
+		cd->disk_events_blocked = 0;
+		disk_unblock_events(cd->disk);
+	}
+
 	return 0;
 }
 
diff --git a/drivers/scsi/sr.h b/drivers/scsi/sr.h
index 37c8f6b..9d9a76f 100644
--- a/drivers/scsi/sr.h
+++ b/drivers/scsi/sr.h
@@ -41,6 +41,7 @@ typedef struct scsi_cd {
 	unsigned readcd_known:1;	/* drive supports READ_CD (0xbe) */
 	unsigned readcd_cdda:1;	/* reading audio data using READ_CD */
 	unsigned media_present:1;	/* media is present */
+	unsigned disk_events_blocked:1; /* disk events is blocked */
 
 	/* GET_EVENT spurious event handling, blk layer guarantees exclusion */
 	int tur_mismatch;		/* nr of get_event TUR mismatches */
-- 
1.7.11.5

[PATCH v5 4/7] libata: acpi: set can_power_off for both ODD and HD

[Aaron Lu]

Hard disk may also be runtime powered off, so set can_power_off flag
for it too if condition satisfies.

Signed-off-by: Aaron Lu <aaron.lu@intel.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 64ba43d..6c8f89c 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -1005,7 +1005,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);
@@ -1026,7 +1026,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);
@@ -1130,14 +1130,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.11.5

[PATCH v5 5/7] scsi: pm: add may_power_off flag

[Aaron Lu]

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

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

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

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

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 093d4f6..92ea17c 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -484,9 +484,6 @@ sdev_store_##field (struct device *dev, struct device_attribute *attr,	\
 }									\
 static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, sdev_show_##field, sdev_store_##field);
 
-/* Currently we don't export bit fields, but we might in future,
- * so leave this code in */
-#if 0
 /*
  * sdev_rd_attr: create a function and attribute variable for a
  * read/write bit field.
@@ -526,7 +523,7 @@ static int scsi_sdev_check_buf_bit(const char *buf)
 	} else
 		return -EINVAL;
 }
-#endif
+
 /*
  * Create the actual show/store functions and data structures.
  */
@@ -860,6 +857,8 @@ static struct device_attribute sdev_attr_queue_type_rw =
 	__ATTR(queue_type, S_IRUGO | S_IWUSR, show_queue_type_field,
 	       sdev_store_queue_type_rw);
 
+sdev_rw_attr_bit (may_power_off);
+
 /**
  * scsi_sysfs_add_sdev - add scsi device to sysfs
  * @sdev:	scsi_device to add
@@ -950,6 +949,9 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 		}
 	}
 
+	if (sdev->can_power_off)
+		device_create_file(&sdev->sdev_gendev, &dev_attr_may_power_off);
+
 	return error;
 }
 
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 72d946f..41cec7f 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -157,6 +157,7 @@ struct scsi_device {
 	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 */
+	unsigned may_power_off:1;	/* Power off is allowed by user */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */
-- 
1.7.11.5

[PATCH v5 6/7] scsi: sr: use may_power_off

[Aaron Lu]

With the introduction of may_power_off, when deciding if we can block
events checking, may_power_off should be used.

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

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 1acfe0e..6b843d1 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -204,7 +204,7 @@ static int sr_suspend(struct device *dev, pm_message_t msg)
 	if (!suspend)
 		return -EBUSY;
 
-	if (cd->device->can_power_off) {
+	if (cd->device->may_power_off) {
 		cd->disk_events_blocked = 1;
 		disk_block_events(cd->disk);
 	}
@@ -794,6 +794,9 @@ static int sr_probe(struct device *dev)
 	sdev_printk(KERN_DEBUG, sdev,
 		    "Attached scsi CD-ROM %s\n", cd->cdi.name);
 
+	if (cd->device->can_power_off)
+		cd->device->may_power_off = 1;
+
 	/* enable runtime pm */
 	scsi_autopm_put_device(cd->device);
 
-- 
1.7.11.5

[PATCH v5 7/7] libata: acpi: respect may_power_off flag

[Aaron Lu]

If user does not want the device being powered off when runtime
suspended by setting may_power_off flag to 0, we will not choose D3 cold
ACPI D-State for it.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/ata/libata-acpi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 6c8f89c..774180d 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -869,7 +869,9 @@ void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
 
 		if (state.event != PM_EVENT_ON) {
 			acpi_state = acpi_pm_device_sleep_state(
-				&dev->sdev->sdev_gendev, NULL, ACPI_STATE_D3);
+				&dev->sdev->sdev_gendev, NULL,
+				dev->sdev->may_power_off ?
+				ACPI_STATE_D3_COLD : ACPI_STATE_D3_HOT);
 			if (acpi_state > 0)
 				acpi_bus_set_power(handle, acpi_state);
 			/* TBD: need to check if it's runtime pm request */
-- 
1.7.11.5

Re: [PATCH v5 0/7] ZPODD patches

[Alan Stern]

On Fri, 31 Aug 2012, Aaron Lu wrote:

> v5:
> Add may_power_off flag to scsi device.
> Alan Stern suggested that I should not mess runtime suspend with
> runtime power off, but the current zpodd implementation made it not
> easy to seperate. So I re-wrote the zpodd implementation, the end
> result is, normal ODD can also enter runtime suspended state, but
> their power won't be removed.

This looks good.  I noticed only a few things:

In patch 5/7, your implementation of may_power_off is written in such a
way that if the drive is already powered off when userspace clears the
flag, the drive is not automatically powered back on.  Maybe this is
what you want?

In patch 1/7 you call both scsi_autopm_get_device() and 
scsi_autopm_put_device() twice in sr_check_events().  With a little 
rewriting it should be possible to call them only once.  Just replace
the "return events" lines with gotos.

What happens if you have an idle ZPODD with may_power_off clear?  A
regular ODD would get runtime-suspended.  In the same way, a ZPODD
should also be runtime-suspended but left at full power.  Does patch
7/7 work this way?  It seems to, but I can't tell for sure.

Alan Stern

Re: [PATCH v5 0/7] ZPODD patches

[Aaron Lu]

On Fri, Aug 31, 2012 at 10:33:40AM -0400, Alan Stern wrote:
> On Fri, 31 Aug 2012, Aaron Lu wrote:
> 
> > v5:
> > Add may_power_off flag to scsi device.
> > Alan Stern suggested that I should not mess runtime suspend with
> > runtime power off, but the current zpodd implementation made it not
> > easy to seperate. So I re-wrote the zpodd implementation, the end
> > result is, normal ODD can also enter runtime suspended state, but
> > their power won't be removed.
> 
> This looks good.  I noticed only a few things:
> 
> In patch 5/7, your implementation of may_power_off is written in such a
> way that if the drive is already powered off when userspace clears the
> flag, the drive is not automatically powered back on.  Maybe this is
> what you want?

No, actually I didn't consider this.

What about I do this when may_power_off is set to 0:
In its store function, if the device is runtime suspended(which means it
is in powered off state since may_power_off was 1 before this store call)
I'll set the flag to 0 and then runtime resume this device.

> 
> In patch 1/7 you call both scsi_autopm_get_device() and 
> scsi_autopm_put_device() twice in sr_check_events().  With a little 
> rewriting it should be possible to call them only once.  Just replace
> the "return events" lines with gotos.

Thanks, will change this.

> 
> What happens if you have an idle ZPODD with may_power_off clear?  A
> regular ODD would get runtime-suspended.  In the same way, a ZPODD
> should also be runtime-suspended but left at full power.  Does patch
> 7/7 work this way?  It seems to, but I can't tell for sure.

Yes, if may_power_off is cleared, d3 cold state will not be allowed,
which means it won't be powered off.

Thanks,
Aaron

Re: [PATCH v5 0/7] ZPODD patches

[Alan Stern]

On Fri, 31 Aug 2012, Aaron Lu wrote:

> > In patch 5/7, your implementation of may_power_off is written in such a
> > way that if the drive is already powered off when userspace clears the
> > flag, the drive is not automatically powered back on.  Maybe this is
> > what you want?
> 
> No, actually I didn't consider this.
> 
> What about I do this when may_power_off is set to 0:
> In its store function, if the device is runtime suspended(which means it
> is in powered off state since may_power_off was 1 before this store call)
> I'll set the flag to 0 and then runtime resume this device.

You might also want to runtime resume the device if may_power_off is
changed from 0 to 1.  Then it will power down when it suspends again
instead of just remaining powered up.

Alan Stern