[PATCH v11 0/9] ZPODD Patches

[PATCH v11 0/9] ZPODD Patches

[Aaron Lu]

v11:
Introduce event_driven flag in scsi_device to silence the media event
poll after ODD is powered off;
Removed ata layer PM QOS control, instead, simply limit ACPI state to
D3_HOT when choosing state;
Make the power off delay a module param named zpodd_poweroff_delay,
defaults to 30 seconds.

v10:
Introduce PM_QOS_NO_POLL flag to skip calling disk's events_check
callback;
Do not use zero power ready hint information from event poll;
Check attached device in port's runtime idle callback to decide if
suspend is desired;
Address various comments from Tejun Heo.

v9:
Build ZPODD as part of libata instead of another standalone module
as it is tightly related to other libata files.
Identify and init ZPODD during probe time instead of after SCSI
device is created as suggested by Tejun Heo.
Make use of pm qos flag to give ACPI hint when choosing ACPI state.
Expose qos flag to give user control of whether power off is allowed.

This patchset used Rafael's pm-qos work:
git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git pm-qos

v8:
This version is a redesign, it doesn't have much to do with previous
versions. The ZPODD implementation is done almost entirely in ATA layer
now, except 2 helper functions from SCSI sr driver to block disk events.

The basic idea is that, when ata port is runtime suspended, it will
check if the ODD is ready to be powered off. And if yes, events is
blocked and power omitted; if not, ODD's power supply remains unchanged
by keeping ACPI state at D0.

Some background knowledge about ZPODD is added below v1 history log.

v7:
Re work of runtime pm of sr driver, based on ideas of Alan Stern and
Oliver Neukum.

Jeff, due to the ready_to_power_off flag added, there is a small
change in [PATCH v7 6/6] libata: acpi: respect may_power_off flag,
please check if I can still get your ack, thanks.

v6:
When user changes may_power_off flag through sysfs entry and if device
is already runtime suspended, resume resume it so that it can respect
this flag next time it is runtime suspended as suggested by Alan Stern.
Call scsi_autopm_get/put_device once in sr_check_events as suggested by
Alan Stern.

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.

Some background knowledge about ZPODD:
ODD means Optical Disc Drive.
ZPODD means Zero Power ODD, it is a mechanism to place the ODD into
zero power state when the system is running at S0 system state without
user's awareness.
It achieved this by ACPI and SATA device attention pin. For power off,
normal ACPI control method is used to place the device into D3 cold
ACPI device state, aka. device power supply omitted. For power on, when
user press the eject button of a drawer type ODD or when user inserts
an ODD into a slot type ODD, the device attention pin will trigger. In
the current x86 implementation, this pin will connect to a GPE, and the
GPE will trigger an ACPI interrupt. With our pre-registered ACPI
notification code, the device can be runtime resumed, and we place the
device back to full power state by setting its ACPI state to D0. The
whole process is transparent to the end user.

Aaron Lu (9):
  scsi: sr: support runtime pm
  libata: Add CONFIG_SATA_ZPODD
  libata: identify and init ZPODD devices
  libata: move acpi notification code to zpodd
  libata: check zero power ready status for ZPODD
  libata: handle power transition of ODD
  libata: expose pm qos flags for ata device
  libata: no poll when ODD is powered off
  libata: do not suspend port if normal ODD is attached

 drivers/ata/Kconfig        |  13 +++
 drivers/ata/Makefile       |   1 +
 drivers/ata/libata-acpi.c  | 111 +++++-------------
 drivers/ata/libata-core.c  |  23 +++-
 drivers/ata/libata-eh.c    |  12 +-
 drivers/ata/libata-zpodd.c | 281 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata.h       |  27 +++++
 drivers/scsi/sr.c          |  36 +++++-
 include/linux/libata.h     |   3 +
 include/scsi/scsi_device.h |   1 +
 include/uapi/linux/cdrom.h |  34 ++++++
 11 files changed, 452 insertions(+), 90 deletions(-)
 create mode 100644 drivers/ata/libata-zpodd.c

-- 
1.7.11.7

[PATCH v11 1/9] scsi: sr: support runtime pm

[Aaron Lu]

This patch adds runtime pm support for sr.

It did this by increasing the runtime usage_count of the device when:
- its block device is opened;
- the events checking is to run.

And decreasing the runtime usage_count of the device when:
- its block device is closed;
- After the events checking is done.

The idea is discussed in this mail thread:
http://thread.gmane.org/gmane.linux.acpi.devel/55243/focus=52703

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

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 5fc97d2..4d1a610 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,7 +147,8 @@ static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
 	kref_get(&cd->kref);
 	if (scsi_device_get(cd->device))
 		goto out_put;
-	goto out;
+	if (!scsi_autopm_get_device(cd->device))
+		goto out;
 
  out_put:
 	kref_put(&cd->kref, sr_kref_release);
@@ -162,6 +164,7 @@ static void scsi_cd_put(struct scsi_cd *cd)
 
 	mutex_lock(&sr_ref_mutex);
 	kref_put(&cd->kref, sr_kref_release);
+	scsi_autopm_put_device(sdev);
 	scsi_device_put(sdev);
 	mutex_unlock(&sr_ref_mutex);
 }
@@ -211,7 +214,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 +223,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 +251,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 +274,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 +291,18 @@ do_tur:
 	cd->tur_changed = false;
 	cd->get_event_changed = false;
 
+out:
+	/*
+	 * If there is no medium detected or the medium has been there
+	 * since last poll, try to suspend the device. Otherwise, keep
+	 * it active for one more poll interval so that if user space
+	 * application opens the block device, we can avoid a runtime
+	 * status change.
+	 */
+	pm_runtime_put_noidle(&cd->device->sdev_gendev);
+	if (!cd->media_present || last_present)
+		pm_runtime_suspend(&cd->device->sdev_gendev);
+
 	return events;
 }
 
@@ -718,6 +734,8 @@ static int sr_probe(struct device *dev)
 
 	sdev_printk(KERN_DEBUG, sdev,
 		    "Attached scsi CD-ROM %s\n", cd->cdi.name);
+	scsi_autopm_put_device(cd->device);
+
 	return 0;
 
 fail_put:
@@ -965,6 +983,8 @@ static int sr_remove(struct device *dev)
 {
 	struct scsi_cd *cd = dev_get_drvdata(dev);
 
+	scsi_autopm_get_device(cd->device);
+
 	blk_queue_prep_rq(cd->device->request_queue, scsi_prep_fn);
 	del_gendisk(cd->disk);
 
-- 
1.7.11.7

[PATCH v11 2/9] libata: Add CONFIG_SATA_ZPODD

[Aaron Lu]

Added a new config CONFIG_SATA_ZPODD, which is used to support
SATA based zero power ODD(ZPODD), and depends on ATA_ACPI.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/ata/Kconfig  | 13 +++++++++++++
 drivers/ata/Makefile |  1 +
 2 files changed, 14 insertions(+)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index e08d322..996d16c 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -58,6 +58,19 @@ config ATA_ACPI
 	  You can disable this at kernel boot time by using the
 	  option libata.noacpi=1
 
+config SATA_ZPODD
+	bool "SATA Zero Power ODD Support"
+	depends on ATA_ACPI
+	default n
+	help
+	  This option adds support for SATA ZPODD. It requires both
+	  ODD and the platform support, and if enabled, will automatically
+	  power on/off the ODD when certain condition is satisfied. This
+	  does not impact user's experience of the ODD, only power is saved
+	  when ODD is not in use(i.e. no disc inside).
+
+	  If unsure, say N.
+
 config SATA_PMP
 	bool "SATA Port Multiplier support"
 	default y
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 9329daf..85e3de4 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -107,3 +107,4 @@ libata-y	:= libata-core.o libata-scsi.o libata-eh.o libata-transport.o
 libata-$(CONFIG_ATA_SFF)	+= libata-sff.o
 libata-$(CONFIG_SATA_PMP)	+= libata-pmp.o
 libata-$(CONFIG_ATA_ACPI)	+= libata-acpi.o
+libata-$(CONFIG_SATA_ZPODD)	+= libata-zpodd.o
-- 
1.7.11.7

[PATCH v11 3/9] libata: identify and init ZPODD devices

[Aaron Lu]

The ODD can be enabled for ZPODD if the following three conditions are
satisfied:
1 The ODD supports device attention;
2 The platform can runtime power off the ODD through ACPI;
3 The ODD is either slot type or drawer type.
For such ODDs, zpodd_init is called and a new structure is allocated for
it to store ZPODD related stuffs.

And the zpodd_dev_enabled function is used to test if ZPODD is currently
enabled for this ODD.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/ata/libata-acpi.c  |   2 +
 drivers/ata/libata-core.c  |   4 +-
 drivers/ata/libata-zpodd.c | 114 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata.h       |  14 ++++++
 include/linux/libata.h     |   3 ++
 include/uapi/linux/cdrom.h |  34 ++++++++++++++
 6 files changed, 170 insertions(+), 1 deletion(-)
 create mode 100644 drivers/ata/libata-zpodd.c

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index ef01ac0..5aa7322 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -1063,6 +1063,8 @@ void ata_acpi_bind(struct ata_device *dev)
 
 void ata_acpi_unbind(struct ata_device *dev)
 {
+	if (zpodd_dev_enabled(dev))
+		zpodd_exit(dev);
 	ata_acpi_remove_pm_notifier(dev);
 	ata_acpi_unregister_power_resource(dev);
 }
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 9e8b99a..65a362e 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2396,8 +2396,10 @@ int ata_dev_configure(struct ata_device *dev)
 			dma_dir_string = ", DMADIR";
 		}
 
-		if (ata_id_has_da(dev->id))
+		if (ata_id_has_da(dev->id)) {
 			dev->flags |= ATA_DFLAG_DA;
+			zpodd_init(dev);
+		}
 
 		/* print device info to dmesg */
 		if (ata_msg_drv(ap) && print_info)
diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
new file mode 100644
index 0000000..58bf891
--- /dev/null
+++ b/drivers/ata/libata-zpodd.c
@@ -0,0 +1,114 @@
+#include <linux/libata.h>
+#include <linux/cdrom.h>
+
+#include "libata.h"
+
+struct zpodd {
+	bool slot:1;
+	bool drawer:1;
+
+	struct ata_device *dev;
+};
+
+static int run_atapi_cmd(struct ata_device *dev, const char *cdb,
+		unsigned short cdb_len, char *buf, unsigned int buf_len)
+{
+	struct ata_taskfile tf = {0};
+
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.command = ATA_CMD_PACKET;
+
+	if (buf) {
+		tf.protocol = ATAPI_PROT_PIO;
+		tf.lbam = buf_len;
+	} else {
+		tf.protocol = ATAPI_PROT_NODATA;
+	}
+
+	return ata_exec_internal(dev, &tf, cdb,
+			buf ? DMA_FROM_DEVICE : DMA_NONE, buf, buf_len, 0);
+}
+
+/*
+ * Per the spec, only slot type and drawer type ODD can be supported
+ *
+ * Return 0 for slot type, 1 for drawer, -ENODEV for other types or on error.
+ */
+static int check_loading_mechanism(struct ata_device *dev)
+{
+	char buf[16];
+	unsigned int ret;
+	struct rm_feature_desc *desc = (void *)(buf + 8);
+
+	char cdb[] = {  GPCMD_GET_CONFIGURATION,
+			2,      /* only 1 feature descriptor requested */
+			0, 3,   /* 3, removable medium feature */
+			0, 0, 0,/* reserved */
+			0, sizeof(buf),
+			0, 0, 0,
+	};
+
+	ret = run_atapi_cmd(dev, cdb, sizeof(cdb), buf, sizeof(buf));
+	if (ret)
+		return -ENODEV;
+
+	if (be16_to_cpu(desc->feature_code) != 3)
+		return -ENODEV;
+
+	if (desc->mech_type == 0 && desc->load == 0 && desc->eject == 1)
+		return 0; /* slot */
+	else if (desc->mech_type == 1 && desc->load == 0 && desc->eject == 1)
+		return 1; /* drawer */
+	else
+		return -ENODEV;
+}
+
+static bool odd_can_poweroff(struct ata_device *ata_dev)
+{
+	acpi_handle handle;
+	acpi_status status;
+	struct acpi_device *acpi_dev;
+
+	handle = ata_dev_acpi_handle(ata_dev);
+	if (!handle)
+		return false;
+
+	status = acpi_bus_get_device(handle, &acpi_dev);
+	if (ACPI_FAILURE(status))
+		return false;
+
+	return acpi_device_can_poweroff(acpi_dev);
+}
+
+void zpodd_init(struct ata_device *dev)
+{
+	int ret;
+	struct zpodd *zpodd;
+
+	if (dev->zpodd)
+		return;
+
+	if (!odd_can_poweroff(dev))
+		return;
+
+	if ((ret = check_loading_mechanism(dev)) == -ENODEV)
+		return;
+
+	zpodd = kzalloc(sizeof(struct zpodd), GFP_KERNEL);
+	if (!zpodd)
+		return;
+
+	if (ret)
+		zpodd->drawer = true;
+	else
+		zpodd->slot = true;
+
+	zpodd->dev = dev;
+	dev->zpodd = zpodd;
+}
+
+void zpodd_exit(struct ata_device *dev)
+{
+	kfree(dev->zpodd);
+	dev->zpodd = NULL;
+}
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 7148a58..8cb4372 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -230,4 +230,18 @@ static inline void ata_sff_exit(void)
 { }
 #endif /* CONFIG_ATA_SFF */
 
+/* libata-zpodd.c */
+#ifdef CONFIG_SATA_ZPODD
+void zpodd_init(struct ata_device *dev);
+void zpodd_exit(struct ata_device *dev);
+static inline bool zpodd_dev_enabled(struct ata_device *dev)
+{
+	return dev->zpodd ? true : false;
+}
+#else /* CONFIG_SATA_ZPODD */
+static inline void zpodd_init(struct ata_device *dev) {}
+static inline void zpodd_exit(struct ata_device *dev) {}
+static inline bool zpodd_dev_enabled(struct ata_device *dev) { return false; }
+#endif /* CONFIG_SATA_ZPODD */
+
 #endif /* __LIBATA_H__ */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 83ba0ab..f88f909 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -620,6 +620,9 @@ struct ata_device {
 	union acpi_object	*gtf_cache;
 	unsigned int		gtf_filter;
 #endif
+#ifdef CONFIG_SATA_ZPODD
+	void			*zpodd;
+#endif
 	struct device		tdev;
 	/* n_sector is CLEAR_BEGIN, read comment above CLEAR_BEGIN */
 	u64			n_sectors;	/* size of device, if ATA */
diff --git a/include/uapi/linux/cdrom.h b/include/uapi/linux/cdrom.h
index 898b866..bd17ad5 100644
--- a/include/uapi/linux/cdrom.h
+++ b/include/uapi/linux/cdrom.h
@@ -908,5 +908,39 @@ struct mode_page_header {
 	__be16 desc_length;
 };
 
+/* removable medium feature descriptor */
+struct rm_feature_desc {
+	__be16 feature_code;
+#if defined(__BIG_ENDIAN_BITFIELD)
+	__u8 reserved1:2;
+	__u8 feature_version:4;
+	__u8 persistent:1;
+	__u8 curr:1;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+	__u8 curr:1;
+	__u8 persistent:1;
+	__u8 feature_version:4;
+	__u8 reserved1:2;
+#endif
+	__u8 add_len;
+#if defined(__BIG_ENDIAN_BITFIELD)
+	__u8 mech_type:3;
+	__u8 load:1;
+	__u8 eject:1;
+	__u8 pvnt_jmpr:1;
+	__u8 dbml:1;
+	__u8 lock:1;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+	__u8 lock:1;
+	__u8 dbml:1;
+	__u8 pvnt_jmpr:1;
+	__u8 eject:1;
+	__u8 load:1;
+	__u8 mech_type:3;
+#endif
+	__u8 reserved2;
+	__u8 reserved3;
+	__u8 reserved4;
+};
 
 #endif /* _UAPI_LINUX_CDROM_H */
-- 
1.7.11.7

[PATCH v11 4/9] libata: move acpi notification code to zpodd

[Aaron Lu]

Since the ata acpi notification code introduced in commit
3bd46600a7a7e938c54df8cdbac9910668c7dfb0 is solely for ZPODD, and we
now have a dedicated place for it, move these code there.

And the ata_acpi_add_pm_notifier code is changed a little bit in that it
is now invoked when scsi device is not bound with ACPI yet, so the way
to get the acpi handle is different with the previous version. And the
ata_acpi_add/remove_pm_notifier is also simplified a little bit in that
it doesn't check if the acpi_device for the handle exists or not as the
odd_can_poweroff function already checked that.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/ata/libata-acpi.c  | 71 ----------------------------------------------
 drivers/ata/libata-zpodd.c | 33 +++++++++++++++++++++
 2 files changed, 33 insertions(+), 71 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 5aa7322..b4788dd 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -974,57 +974,6 @@ void ata_acpi_on_disable(struct ata_device *dev)
 	ata_acpi_clear_gtf(dev);
 }
 
-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);
-}
-
-static void ata_acpi_add_pm_notifier(struct ata_device *dev)
-{
-	struct acpi_device *acpi_dev;
-	acpi_handle handle;
-	acpi_status status;
-
-	handle = ata_dev_acpi_handle(dev);
-	if (!handle)
-		return;
-
-	status = acpi_bus_get_device(handle, &acpi_dev);
-	if (ACPI_FAILURE(status))
-		return;
-
-	if (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);
-	}
-}
-
-static void ata_acpi_remove_pm_notifier(struct ata_device *dev)
-{
-	struct acpi_device *acpi_dev;
-	acpi_handle handle;
-	acpi_status status;
-
-	handle = ata_dev_acpi_handle(dev);
-	if (!handle)
-		return;
-
-	status = acpi_bus_get_device(handle, &acpi_dev);
-	if (ACPI_FAILURE(status))
-		return;
-
-	if (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);
-	}
-}
-
 static void ata_acpi_register_power_resource(struct ata_device *dev)
 {
 	struct scsi_device *sdev = dev->sdev;
@@ -1057,7 +1006,6 @@ static void ata_acpi_unregister_power_resource(struct ata_device *dev)
 
 void ata_acpi_bind(struct ata_device *dev)
 {
-	ata_acpi_add_pm_notifier(dev);
 	ata_acpi_register_power_resource(dev);
 }
 
@@ -1065,7 +1013,6 @@ void ata_acpi_unbind(struct ata_device *dev)
 {
 	if (zpodd_dev_enabled(dev))
 		zpodd_exit(dev);
-	ata_acpi_remove_pm_notifier(dev);
 	ata_acpi_unregister_power_resource(dev);
 }
 
@@ -1107,9 +1054,6 @@ static int ata_acpi_bind_device(struct ata_port *ap, struct scsi_device *sdev,
 				acpi_handle *handle)
 {
 	struct ata_device *ata_dev;
-	acpi_status status;
-	struct acpi_device *acpi_dev;
-	struct acpi_device_power_state *states;
 
 	if (ap->flags & ATA_FLAG_ACPI_SATA) {
 		if (!sata_pmp_attached(ap))
@@ -1126,21 +1070,6 @@ static int ata_acpi_bind_device(struct ata_port *ap, struct scsi_device *sdev,
 	if (!*handle)
 		return -ENODEV;
 
-	status = acpi_bus_get_device(*handle, &acpi_dev);
-	if (ACPI_FAILURE(status))
-		return 0;
-
-	/*
-	 * 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
-	 */
-	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;
-
 	return 0;
 }
 
diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
index 58bf891..c8c7b74 100644
--- a/drivers/ata/libata-zpodd.c
+++ b/drivers/ata/libata-zpodd.c
@@ -1,5 +1,7 @@
 #include <linux/libata.h>
 #include <linux/cdrom.h>
+#include <linux/pm_runtime.h>
+#include <scsi/scsi_device.h>
 
 #include "libata.h"
 
@@ -7,6 +9,9 @@ struct zpodd {
 	bool slot:1;
 	bool drawer:1;
 
+	/* The following bits are synchronized by PM core */
+	bool from_notify:1; /* resumed as a result of acpi notification */
+
 	struct ata_device *dev;
 };
 
@@ -80,6 +85,32 @@ static bool odd_can_poweroff(struct ata_device *ata_dev)
 	return acpi_device_can_poweroff(acpi_dev);
 }
 
+static void zpodd_wake_dev(acpi_handle handle, u32 event, void *context)
+{
+	struct ata_device *ata_dev = context;
+	struct zpodd *zpodd = ata_dev->zpodd;
+	struct device *dev = &ata_dev->sdev->sdev_gendev;
+
+	if (event == ACPI_NOTIFY_DEVICE_WAKE && ata_dev &&
+			pm_runtime_suspended(dev)) {
+		zpodd->from_notify = true;
+		pm_runtime_resume(dev);
+	}
+}
+
+static void ata_acpi_add_pm_notifier(struct ata_device *dev)
+{
+	acpi_handle handle = ata_dev_acpi_handle(dev);
+	acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
+				    zpodd_wake_dev, dev);
+}
+
+static void ata_acpi_remove_pm_notifier(struct ata_device *dev)
+{
+	acpi_handle handle = DEVICE_ACPI_HANDLE(&dev->sdev->sdev_gendev);
+	acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY, zpodd_wake_dev);
+}
+
 void zpodd_init(struct ata_device *dev)
 {
 	int ret;
@@ -103,12 +134,14 @@ void zpodd_init(struct ata_device *dev)
 	else
 		zpodd->slot = true;
 
+	ata_acpi_add_pm_notifier(dev);
 	zpodd->dev = dev;
 	dev->zpodd = zpodd;
 }
 
 void zpodd_exit(struct ata_device *dev)
 {
+	ata_acpi_remove_pm_notifier(dev);
 	kfree(dev->zpodd);
 	dev->zpodd = NULL;
 }
-- 
1.7.11.7

[PATCH v11 5/9] libata: check zero power ready status for ZPODD

[Aaron Lu]

Per the Mount Fuji spec, the ODD is considered zero power ready when:
  - For slot type ODD, no media inside;
  - For tray type ODD, no media inside and tray closed.

The information can be retrieved by either the returned information of
command GET_EVENT_STATUS_NOTIFICATION(the command is used to poll for
media event) or sense code.

The information provided by the media status byte is not accurate, it
is possible that after a new disc is just inserted, the status byte
still returns media not present. So this information can not be used as
the deciding factor, we use sense code to decide if zpready status is
true.

When we first sensed the ODD in the zero power ready state, the
timestamp will be recoreded. And after ODD stayed in this state for
some pre-defined period, the ODD is considered as power off ready and
the zp_ready flag will be set. The zp_ready flag serves as the deciding
factor other code will use to see if power off is OK for the ODD.

The Mount Fuji spec suggests a delay should be used here, to avoid the
case user ejects the ODD and then instantly inserts a new one again, so
that we can avoid a power transition. And some ODDs may be slow to place
its head to the home position after disc is ejected, so a delay here is
generally a good idea. And the delay time can be changed via the module
param zpodd_poweroff_delay.

The zero power ready status check is performed in the ata port's runtime
suspend code path, when port is not frozen yet, as we need to issue some
IOs to the ODD.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/ata/libata-eh.c    | 10 +++++--
 drivers/ata/libata-zpodd.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata.h       |  5 ++++
 3 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index bf039b0..d85934c 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1591,7 +1591,7 @@ static int ata_eh_read_log_10h(struct ata_device *dev,
  *	RETURNS:
  *	0 on success, AC_ERR_* mask on failure.
  */
-static unsigned int atapi_eh_tur(struct ata_device *dev, u8 *r_sense_key)
+unsigned int atapi_eh_tur(struct ata_device *dev, u8 *r_sense_key)
 {
 	u8 cdb[ATAPI_CDB_LEN] = { TEST_UNIT_READY, 0, 0, 0, 0, 0 };
 	struct ata_taskfile tf;
@@ -1624,7 +1624,7 @@ static unsigned int atapi_eh_tur(struct ata_device *dev, u8 *r_sense_key)
  *	RETURNS:
  *	0 on success, AC_ERR_* mask on failure
  */
-static unsigned int atapi_eh_request_sense(struct ata_device *dev,
+unsigned int atapi_eh_request_sense(struct ata_device *dev,
 					   u8 *sense_buf, u8 dfl_sense_key)
 {
 	u8 cdb[ATAPI_CDB_LEN] =
@@ -4022,6 +4022,7 @@ static void ata_eh_handle_port_suspend(struct ata_port *ap)
 {
 	unsigned long flags;
 	int rc = 0;
+	struct ata_device *dev;
 
 	/* are we suspending? */
 	spin_lock_irqsave(ap->lock, flags);
@@ -4034,6 +4035,11 @@ static void ata_eh_handle_port_suspend(struct ata_port *ap)
 
 	WARN_ON(ap->pflags & ATA_PFLAG_SUSPENDED);
 
+	ata_for_each_dev(dev, &ap->link, ENABLED) {
+		if (zpodd_dev_enabled(dev))
+			zpodd_on_suspend(dev);
+	}
+
 	/* tell ACPI we're suspending */
 	rc = ata_acpi_on_suspend(ap);
 	if (rc)
diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
index c8c7b74..7ee49bb 100644
--- a/drivers/ata/libata-zpodd.c
+++ b/drivers/ata/libata-zpodd.c
@@ -1,16 +1,24 @@
 #include <linux/libata.h>
 #include <linux/cdrom.h>
 #include <linux/pm_runtime.h>
+#include <linux/module.h>
 #include <scsi/scsi_device.h>
 
 #include "libata.h"
 
+static int zpodd_poweroff_delay = 30; /* 30 seconds for power off delay */
+module_param(zpodd_poweroff_delay, int, 0644);
+MODULE_PARM_DESC(zpodd_poweroff_delay, "Poweroff delay for ZPODD in seconds");
+
 struct zpodd {
 	bool slot:1;
 	bool drawer:1;
 
 	/* The following bits are synchronized by PM core */
 	bool from_notify:1; /* resumed as a result of acpi notification */
+	bool zp_ready:1;	/* zero power ready state */
+
+	unsigned long last_ready; /* last zero power ready timestamp */
 
 	struct ata_device *dev;
 };
@@ -85,6 +93,68 @@ static bool odd_can_poweroff(struct ata_device *ata_dev)
 	return acpi_device_can_poweroff(acpi_dev);
 }
 
+/* Test if ODD is zero power ready by sense code */
+static bool zpready(struct ata_device *dev)
+{
+	u8 sense_key, *sense_buf;
+	unsigned int ret, asc, ascq, add_len;
+	struct zpodd *zpodd = dev->zpodd;
+
+	ret = atapi_eh_tur(dev, &sense_key);
+
+	if (!ret || sense_key != NOT_READY)
+		return false;
+
+	sense_buf = dev->link->ap->sector_buf;
+	ret = atapi_eh_request_sense(dev, sense_buf, sense_key);
+	if (ret)
+		return false;
+
+	/* sense valid */
+	if ((sense_buf[0] & 0x7f) != 0x70)
+		return false;
+
+	add_len = sense_buf[7];
+	/* has asc and ascq */
+	if (add_len < 6)
+		return false;
+
+	asc = sense_buf[12];
+	ascq = sense_buf[13];
+
+	if (zpodd->slot)
+		/* no media inside */
+		return asc == 0x3a;
+	else
+		/* no media inside and door closed */
+		return asc == 0x3a && ascq == 0x01;
+}
+
+/* Check zero power ready status */
+void zpodd_on_suspend(struct ata_device *dev)
+{
+	struct zpodd *zpodd = dev->zpodd;
+	unsigned long expires;
+
+	if (!zpready(dev)) {
+		zpodd->zp_ready = false;
+		zpodd->last_ready = 0;
+		return;
+	}
+
+	if (!zpodd->last_ready) {
+		zpodd->last_ready = jiffies;
+		return;
+	}
+
+	expires = zpodd->last_ready +
+		  msecs_to_jiffies(zpodd_poweroff_delay * 1000);
+	if (time_before(jiffies, expires))
+		return;
+
+	zpodd->zp_ready = true;
+}
+
 static void zpodd_wake_dev(acpi_handle handle, u32 event, void *context)
 {
 	struct ata_device *ata_dev = context;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 8cb4372..f9235dc 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -182,6 +182,9 @@ extern void ata_eh_finish(struct ata_port *ap);
 extern int ata_ering_map(struct ata_ering *ering,
 			 int (*map_fn)(struct ata_ering_entry *, void *),
 		  	 void *arg);
+extern unsigned int atapi_eh_tur(struct ata_device *dev, u8 *r_sense_key);
+extern unsigned int atapi_eh_request_sense(struct ata_device *dev,
+					   u8 *sense_buf, u8 dfl_sense_key);
 
 /* libata-pmp.c */
 #ifdef CONFIG_SATA_PMP
@@ -238,10 +241,12 @@ static inline bool zpodd_dev_enabled(struct ata_device *dev)
 {
 	return dev->zpodd ? true : false;
 }
+void zpodd_on_suspend(struct ata_device *dev);
 #else /* CONFIG_SATA_ZPODD */
 static inline void zpodd_init(struct ata_device *dev) {}
 static inline void zpodd_exit(struct ata_device *dev) {}
 static inline bool zpodd_dev_enabled(struct ata_device *dev) { return false; }
+static inline void zpodd_on_suspend(struct ata_device *dev) {}
 #endif /* CONFIG_SATA_ZPODD */
 
 #endif /* __LIBATA_H__ */
-- 
1.7.11.7

[PATCH v11 6/9] libata: handle power transition of ODD

[Aaron Lu]

When ata port is runtime suspended, it will check if the ODD attched to
it is a zero power(ZP) capable ODD and if the ZP capable ODD is in zero
power ready state. And if this is not the case, the highest acpi state
will be limited to ACPI_STATE_D3_HOT to avoid powering off the ODD.

And on resume, it will re-gain power and go through the recovery
process. When reset for the ata port is done, the ODD is considered
functional, and post processing like eject tray if the ODD is drawer
type is done there.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/ata/libata-acpi.c  | 35 ++++++++++++++++++++---------
 drivers/ata/libata-eh.c    |  2 ++
 drivers/ata/libata-zpodd.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata.h       |  8 +++++++
 4 files changed, 91 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index b4788dd..e832bf6 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -835,6 +835,22 @@ void ata_acpi_on_resume(struct ata_port *ap)
 	}
 }
 
+static int ata_acpi_choose_suspend_state(struct ata_device *dev)
+{
+	int d_max_in = ACPI_STATE_D3_COLD;
+
+	/*
+	 * For ATAPI, runtime D3 cold is only allowed
+	 * for ZPODD in zero power ready state
+	 */
+	if (dev->class == ATA_DEV_ATAPI &&
+	    !(zpodd_dev_enabled(dev) && zpodd_zpready(dev)))
+		d_max_in = ACPI_STATE_D3_HOT;
+
+	return acpi_pm_device_sleep_state(&dev->sdev->sdev_gendev,
+					  NULL, d_max_in);
+}
+
 /**
  * ata_acpi_set_state - set the port power state
  * @ap: target ATA port
@@ -861,17 +877,16 @@ void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
 			continue;
 
 		if (state.event != PM_EVENT_ON) {
-			acpi_state = acpi_pm_device_sleep_state(
-				&dev->sdev->sdev_gendev, NULL, ACPI_STATE_D3);
-			if (acpi_state > 0)
-				acpi_bus_set_power(handle, acpi_state);
-			/* TBD: need to check if it's runtime pm request */
-			acpi_pm_device_run_wake(
-				&dev->sdev->sdev_gendev, true);
+			acpi_state = ata_acpi_choose_suspend_state(dev);
+			if (acpi_state == ACPI_STATE_D0)
+				continue;
+			if (zpodd_dev_enabled(dev) &&
+			    acpi_state == ACPI_STATE_D3_COLD)
+				zpodd_pre_poweroff(dev);
+			acpi_bus_set_power(handle, acpi_state);
 		} else {
-			/* Ditto */
-			acpi_pm_device_run_wake(
-				&dev->sdev->sdev_gendev, false);
+			if (zpodd_dev_enabled(dev))
+				zpodd_pre_poweron(dev);
 			acpi_bus_set_power(handle, ACPI_STATE_D0);
 		}
 	}
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index d85934c..d772d66 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -3857,6 +3857,8 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
 				rc = atapi_eh_clear_ua(dev);
 				if (rc)
 					goto rest_fail;
+				if (zpodd_dev_enabled(dev))
+					zpodd_post_resume(dev);
 			}
 		}
 
diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
index 7ee49bb..3c39987 100644
--- a/drivers/ata/libata-zpodd.c
+++ b/drivers/ata/libata-zpodd.c
@@ -17,6 +17,7 @@ struct zpodd {
 	/* The following bits are synchronized by PM core */
 	bool from_notify:1; /* resumed as a result of acpi notification */
 	bool zp_ready:1;	/* zero power ready state */
+	bool powered_off:1;	/* ODD is powered off */
 
 	unsigned long last_ready; /* last zero power ready timestamp */
 
@@ -42,6 +43,17 @@ static int run_atapi_cmd(struct ata_device *dev, const char *cdb,
 			buf ? DMA_FROM_DEVICE : DMA_NONE, buf, buf_len, 0);
 }
 
+static int eject_tray(struct ata_device *dev)
+{
+	const char cdb[] = {  GPCMD_START_STOP_UNIT,
+			      0, 0, 0,
+			      0x02,     /* LoEj */
+			      0, 0, 0, 0, 0, 0, 0,
+	};
+
+	return run_atapi_cmd(dev, cdb, sizeof(cdb), NULL, 0);
+}
+
 /*
  * Per the spec, only slot type and drawer type ODD can be supported
  *
@@ -155,6 +167,50 @@ void zpodd_on_suspend(struct ata_device *dev)
 	zpodd->zp_ready = true;
 }
 
+bool zpodd_zpready(struct ata_device *dev)
+{
+	struct zpodd *zpodd = dev->zpodd;
+	return zpodd->zp_ready;
+}
+
+void zpodd_pre_poweroff(struct ata_device *dev)
+{
+	struct zpodd *zpodd = dev->zpodd;
+
+	zpodd->powered_off = true;
+	device_set_run_wake(&dev->sdev->sdev_gendev, true);
+	acpi_pm_device_run_wake(&dev->sdev->sdev_gendev, true);
+}
+
+void zpodd_pre_poweron(struct ata_device *dev)
+{
+	struct zpodd *zpodd = dev->zpodd;
+
+	if (zpodd->powered_off) {
+		acpi_pm_device_run_wake(&dev->sdev->sdev_gendev, false);
+		device_set_run_wake(&dev->sdev->sdev_gendev, false);
+	}
+}
+
+void zpodd_post_resume(struct ata_device *dev)
+{
+	struct zpodd *zpodd = dev->zpodd;
+
+	if (!zpodd->powered_off)
+		return;
+
+	zpodd->powered_off = false;
+
+	if (zpodd->from_notify) {
+		zpodd->from_notify = false;
+		if (zpodd->drawer)
+			eject_tray(dev);
+	}
+
+	zpodd->last_ready = 0;
+	zpodd->zp_ready = false;
+}
+
 static void zpodd_wake_dev(acpi_handle handle, u32 event, void *context)
 {
 	struct ata_device *ata_dev = context;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index f9235dc..4b20e71 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -242,11 +242,19 @@ static inline bool zpodd_dev_enabled(struct ata_device *dev)
 	return dev->zpodd ? true : false;
 }
 void zpodd_on_suspend(struct ata_device *dev);
+bool zpodd_zpready(struct ata_device *dev);
+void zpodd_pre_poweroff(struct ata_device *dev);
+void zpodd_pre_poweron(struct ata_device *dev);
+void zpodd_post_resume(struct ata_device *dev);
 #else /* CONFIG_SATA_ZPODD */
 static inline void zpodd_init(struct ata_device *dev) {}
 static inline void zpodd_exit(struct ata_device *dev) {}
 static inline bool zpodd_dev_enabled(struct ata_device *dev) { return false; }
 static inline void zpodd_on_suspend(struct ata_device *dev) {}
+static inline bool zpodd_zpready(struct ata_device *dev) { return false; }
+static inline void zpodd_pre_poweroff(struct ata_device *dev) {}
+static inline void zpodd_pre_poweron(struct ata_device *dev) {}
+static inline void zpodd_post_resume(struct ata_device *dev) {}
 #endif /* CONFIG_SATA_ZPODD */
 
 #endif /* __LIBATA_H__ */
-- 
1.7.11.7

[PATCH v11 7/9] libata: expose pm qos flags for ata device

[Aaron Lu]

Expose pm qos flags to user space so that user has a chance to disable
pm features like power off, if he/she has a broken platform or devices
or simply does not like this pm feature.

This flag is exposed to user space only for ata device or atapi device
that is zero power capable. For normal atapi device, it will never be
powered off.

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

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index e832bf6..5b67be3 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -17,6 +17,7 @@
 #include <linux/pci.h>
 #include <linux/slab.h>
 #include <linux/pm_runtime.h>
+#include <linux/pm_qos.h>
 #include <scsi/scsi_device.h>
 #include "libata.h"
 
@@ -1022,6 +1023,8 @@ static void ata_acpi_unregister_power_resource(struct ata_device *dev)
 void ata_acpi_bind(struct ata_device *dev)
 {
 	ata_acpi_register_power_resource(dev);
+	if (dev->class == ATA_DEV_ATA || zpodd_dev_enabled(dev))
+		dev_pm_qos_expose_flags(&dev->sdev->sdev_gendev, 0);
 }
 
 void ata_acpi_unbind(struct ata_device *dev)
-- 
1.7.11.7

[PATCH v11 8/9] libata: no poll when ODD is powered off

[Aaron Lu]

When the ODD is powered off, any action the user did to the ODD that
would generate a media event will trigger an ACPI interrupt, so the
poll for media event is no longer necessary. And the poll will also
cause a runtime status change, which will stop the ODD from staying in
powered off state, so the poll should better be stopped.

But since we don't have access to the gendisk structure in LLDs, here
comes the event_driven flag for scsi device. This flag serves as a
capability of the device, conveyed by the LLDs to upper layer. It is set
when LLDs know that this device will no longer generate any media
related events, so that the check_events can simply return 0 without
bothering the device, effectively silence the poll.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/ata/libata-zpodd.c | 8 ++++++++
 drivers/scsi/sr.c          | 6 +++++-
 include/scsi/scsi_device.h | 1 +
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
index 3c39987..5843a18 100644
--- a/drivers/ata/libata-zpodd.c
+++ b/drivers/ata/libata-zpodd.c
@@ -177,6 +177,13 @@ void zpodd_pre_poweroff(struct ata_device *dev)
 {
 	struct zpodd *zpodd = dev->zpodd;
 
+	/*
+	 * Silence the media change poll, as we will be notified when
+	 * user wants to use the ODD so there is no meaning to poll
+	 * it when it is powered off
+	 */
+	dev->sdev->event_driven = true;
+
 	zpodd->powered_off = true;
 	device_set_run_wake(&dev->sdev->sdev_gendev, true);
 	acpi_pm_device_run_wake(&dev->sdev->sdev_gendev, true);
@@ -209,6 +216,7 @@ void zpodd_post_resume(struct ata_device *dev)
 
 	zpodd->last_ready = 0;
 	zpodd->zp_ready = false;
+	dev->sdev->event_driven = false;
 }
 
 static void zpodd_wake_dev(acpi_handle handle, u32 event, void *context)
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 4d1a610..b034274 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -594,7 +594,11 @@ static unsigned int sr_block_check_events(struct gendisk *disk,
 					  unsigned int clearing)
 {
 	struct scsi_cd *cd = scsi_cd(disk);
-	return cdrom_check_events(&cd->cdi, clearing);
+
+	if (!cd->device->event_driven)
+		return cdrom_check_events(&cd->cdi, clearing);
+	else
+		return 0;
 }
 
 static int sr_block_revalidate_disk(struct gendisk *disk)
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index e65c62e..1756151 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -160,6 +160,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 no_dif:1;	/* T10 PI (DIF) should be disabled */
+	unsigned event_driven:1; /* No need to poll the device */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */
-- 
1.7.11.7

[PATCH v11 9/9] libata: do not suspend port if normal ODD is attached

[Aaron Lu]

For ODDs, the upper layer will poll for media change every a few
seconds, which will make it enter and leave suspend state very
oftern. And as each suspend will also cause a hard/soft reset,
the gain of runtime suspend is very little while the ODD may
mis-function after constantly being reset. So the idle callback
here will not proceed to suspend if a non-ZPODD capable ODD is
attached to the port.

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

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 65a362e..f664a90 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5408,8 +5408,27 @@ static int ata_port_resume(struct device *dev)
 	return rc;
 }
 
+/*
+ * For ODDs, the upper layer will poll for media change every a few seconds,
+ * which will make it enter and leave suspend state every a few seconds. And
+ * as each suspend will cause a hard/soft reset, the gain of runtime suspend
+ * is very little and the ODD may mis-function after constantly being reset.
+ * So the idle callback here will not proceed to suspend if a non-ZPODD capable
+ * ODD is attached to the port.
+ */
 static int ata_port_runtime_idle(struct device *dev)
 {
+	struct ata_port *ap = to_ata_port(dev);
+	struct ata_link *link;
+	struct ata_device *adev;
+
+	ata_for_each_link(link, ap, HOST_FIRST) {
+		ata_for_each_dev(adev, link, ENABLED)
+			if (adev->class == ATA_DEV_ATAPI &&
+			    !zpodd_dev_enabled(adev))
+				return -EBUSY;
+	}
+
 	return pm_runtime_suspend(dev);
 }
 
-- 
1.7.11.7

Re: [PATCH v11 9/9] libata: do not suspend port if normal ODD is attached

[Sergei Shtylyov]

Hello.

On 06-01-2013 6:48, Aaron Lu wrote:

> For ODDs, the upper layer will poll for media change every a few
> seconds, which will make it enter and leave suspend state very
> oftern. And as each suspend will also cause a hard/soft reset,

    s/oftern/often/

> the gain of runtime suspend is very little while the ODD may
> mis-function after constantly being reset. So the idle callback
> here will not proceed to suspend if a non-ZPODD capable ODD is
> attached to the port.

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

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 65a362e..f664a90 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5408,8 +5408,27 @@ static int ata_port_resume(struct device *dev)
>   	return rc;
>   }
>
> +/*
> + * For ODDs, the upper layer will poll for media change every a few seconds,
> + * which will make it enter and leave suspend state every a few seconds. And

    Article is not needed before "few". (Same comment to the changelog.)

> + * as each suspend will cause a hard/soft reset, the gain of runtime suspend
> + * is very little and the ODD may mis-function after constantly being reset.

    s/mis-function/malfunction/ perhaps? (Same in the changelog.)

MBR, Sergei

Re: [PATCH v11 9/9] libata: do not suspend port if normal ODD is attached

[Aaron Lu]

Hi Sergei,

Thanks for reviewing, will update in next revision.

-Aaron

On 01/06/2013 10:34 PM, Sergei Shtylyov wrote:
> Hello.
> 
> On 06-01-2013 6:48, Aaron Lu wrote:
> 
>> For ODDs, the upper layer will poll for media change every a few
>> seconds, which will make it enter and leave suspend state very
>> oftern. And as each suspend will also cause a hard/soft reset,
> 
>     s/oftern/often/
> 
>> the gain of runtime suspend is very little while the ODD may
>> mis-function after constantly being reset. So the idle callback
>> here will not proceed to suspend if a non-ZPODD capable ODD is
>> attached to the port.
> 
>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
>> ---
>>   drivers/ata/libata-core.c | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
> 
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 65a362e..f664a90 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -5408,8 +5408,27 @@ static int ata_port_resume(struct device *dev)
>>   	return rc;
>>   }
>>
>> +/*
>> + * For ODDs, the upper layer will poll for media change every a few seconds,
>> + * which will make it enter and leave suspend state every a few seconds. And
> 
>     Article is not needed before "few". (Same comment to the changelog.)
> 
>> + * as each suspend will cause a hard/soft reset, the gain of runtime suspend
>> + * is very little and the ODD may mis-function after constantly being reset.
> 
>     s/mis-function/malfunction/ perhaps? (Same in the changelog.)
> 
> MBR, Sergei
> 

Re: [PATCH v11 2/9] libata: Add CONFIG_SATA_ZPODD

[Tejun Heo]

On Sun, Jan 06, 2013 at 10:48:22AM +0800, Aaron Lu wrote:
> Added a new config CONFIG_SATA_ZPODD, which is used to support
> SATA based zero power ODD(ZPODD), and depends on ATA_ACPI.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>

Maybe just fold this into the next patch?  It's rather unconventional
to introduce the config option and makefile changes by themselves.

Thanks.

-- 
tejun

Re: [PATCH v11 3/9] libata: identify and init ZPODD devices

[Tejun Heo]

On Sun, Jan 06, 2013 at 10:48:23AM +0800, Aaron Lu wrote:
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index ef01ac0..5aa7322 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -1063,6 +1063,8 @@ void ata_acpi_bind(struct ata_device *dev)
>  
>  void ata_acpi_unbind(struct ata_device *dev)
>  {
> +	if (zpodd_dev_enabled(dev))
> +		zpodd_exit(dev);
>  	ata_acpi_remove_pm_notifier(dev);
>  	ata_acpi_unregister_power_resource(dev);
>  }

Wouldn't it make more sense to invoke zpodd_exit() from
ata_scsi_remove_dev() which is approximate counterpart of
dev_configure?

> +struct zpodd {
> +	bool slot:1;
> +	bool drawer:1;
> +
> +	struct ata_device *dev;
> +};

Field names are usually indented.  It would be nice to have a comment
explaining synchronization.  Bitfields w/ their implicit RMW ops tend
to make people wonder about what the access rules are.

> +static int run_atapi_cmd(struct ata_device *dev, const char *cdb,
> +		unsigned short cdb_len, char *buf, unsigned int buf_len)
> +{
> +	struct ata_taskfile tf = {0};

No need for 0.  { } is enough and more generic.

> +
> +	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
> +	tf.command = ATA_CMD_PACKET;
> +
> +	if (buf) {
> +		tf.protocol = ATAPI_PROT_PIO;
> +		tf.lbam = buf_len;
> +	} else {
> +		tf.protocol = ATAPI_PROT_NODATA;
> +	}
> +
> +	return ata_exec_internal(dev, &tf, cdb,
> +			buf ? DMA_FROM_DEVICE : DMA_NONE, buf, buf_len, 0);
> +}

So, the function name is a bit of misnomer given that ATAPI commands
are not limited to PIO or DMA_FROM_DEVICE.  Also, this function ends
up being used twice - once w/ read buffer and once w/o.  Do we really
want this function?  It's not like exec_internal is difficult to use.

> +/*
> + * Per the spec, only slot type and drawer type ODD can be supported
> + *
> + * Return 0 for slot type, 1 for drawer, -ENODEV for other types or on error.
> + */

Maybe bool odd_has_drawer() is better?

> +static int check_loading_mechanism(struct ata_device *dev)
> +{
> +	char buf[16];
> +	unsigned int ret;
> +	struct rm_feature_desc *desc = (void *)(buf + 8);
> +
> +	char cdb[] = {  GPCMD_GET_CONFIGURATION,
> +			2,      /* only 1 feature descriptor requested */
> +			0, 3,   /* 3, removable medium feature */
> +			0, 0, 0,/* reserved */
> +			0, sizeof(buf),
> +			0, 0, 0,
> +	};
> +
> +	ret = run_atapi_cmd(dev, cdb, sizeof(cdb), buf, sizeof(buf));
> +	if (ret)
> +		return -ENODEV;
> +
> +	if (be16_to_cpu(desc->feature_code) != 3)
> +		return -ENODEV;
> +
> +	if (desc->mech_type == 0 && desc->load == 0 && desc->eject == 1)
> +		return 0; /* slot */
> +	else if (desc->mech_type == 1 && desc->load == 0 && desc->eject == 1)
> +		return 1; /* drawer */
> +	else
> +		return -ENODEV;
> +}
> +
> +static bool odd_can_poweroff(struct ata_device *ata_dev)
> +{
> +	acpi_handle handle;
> +	acpi_status status;
> +	struct acpi_device *acpi_dev;
> +
> +	handle = ata_dev_acpi_handle(ata_dev);
> +	if (!handle)
> +		return false;
> +
> +	status = acpi_bus_get_device(handle, &acpi_dev);
> +	if (ACPI_FAILURE(status))
> +		return false;
> +
> +	return acpi_device_can_poweroff(acpi_dev);
> +}
> +
> +void zpodd_init(struct ata_device *dev)
> +{
> +	int ret;
> +	struct zpodd *zpodd;
> +
> +	if (dev->zpodd)
> +		return;
> +
> +	if (!odd_can_poweroff(dev))
> +		return;
> +
> +	if ((ret = check_loading_mechanism(dev)) == -ENODEV)
> +		return;
> +
> +	zpodd = kzalloc(sizeof(struct zpodd), GFP_KERNEL);
> +	if (!zpodd)
> +		return;
> +
> +	if (ret)
> +		zpodd->drawer = true;
> +	else
> +		zpodd->slot = true;
> +
> +	zpodd->dev = dev;
> +	dev->zpodd = zpodd;
> +}
> +
> +void zpodd_exit(struct ata_device *dev)
> +{
> +	kfree(dev->zpodd);
> +	dev->zpodd = NULL;
> +}
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 7148a58..8cb4372 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -230,4 +230,18 @@ static inline void ata_sff_exit(void)
>  { }
>  #endif /* CONFIG_ATA_SFF */
>  
> +/* libata-zpodd.c */
> +#ifdef CONFIG_SATA_ZPODD
> +void zpodd_init(struct ata_device *dev);
> +void zpodd_exit(struct ata_device *dev);
> +static inline bool zpodd_dev_enabled(struct ata_device *dev)
> +{
> +	return dev->zpodd ? true : false;

	return dev->zpodd or return dev->zpodd != NULL?

Other than the above nits, looks okay to me.

Thanks.

-- 
tejun

Re: [PATCH v11 4/9] libata: move acpi notification code to zpodd

[Tejun Heo]

On Sun, Jan 06, 2013 at 10:48:24AM +0800, Aaron Lu wrote:
> @@ -7,6 +9,9 @@ struct zpodd {
>  	bool slot:1;
>  	bool drawer:1;
>  
> +	/* The following bits are synchronized by PM core */
> +	bool from_notify:1; /* resumed as a result of acpi notification */
> +
>  	struct ata_device *dev;
>  };

So, you can't put bitfields which belong to different synchronization
domains next to each other.  They may (and here will) be put into the
same word and RMW cycles may race with each other.  Please either use
unsigned integer flags per sync domain or just put them in separate
bools.  At any rate, please comment on how RWs are supposed to be
synchronized.

Thanks.

-- 
tejun

Re: [PATCH v11 5/9] libata: check zero power ready status for ZPODD

[Tejun Heo]

Hello, Aaron.

On Sun, Jan 06, 2013 at 10:48:25AM +0800, Aaron Lu wrote:
> +/* Check zero power ready status */
> +void zpodd_on_suspend(struct ata_device *dev)
> +{
> +	struct zpodd *zpodd = dev->zpodd;
> +	unsigned long expires;
> +
> +	if (!zpready(dev)) {
> +		zpodd->zp_ready = false;
> +		zpodd->last_ready = 0;
> +		return;
> +	}
> +
> +	if (!zpodd->last_ready) {
> +		zpodd->last_ready = jiffies;
> +		return;
> +	}
> +
> +	expires = zpodd->last_ready +
> +		  msecs_to_jiffies(zpodd_poweroff_delay * 1000);
> +	if (time_before(jiffies, expires))
> +		return;
> +
> +	zpodd->zp_ready = true;
> +}

Using 0 jiffies as special value is generally considered a bad form.
It should be mostly ok here but it's not like avoiding that is
difficult, so let's please not use 0 jiffies as special value.  If you
have to, add another variable zp_sample_cnt or whatever.

Thanks.

-- 
tejun

Re: [PATCH v11 6/9] libata: handle power transition of ODD

[Tejun Heo]

On Sun, Jan 06, 2013 at 10:48:26AM +0800, Aaron Lu wrote:
> +bool zpodd_zpready(struct ata_device *dev)
> +{
> +	struct zpodd *zpodd = dev->zpodd;
> +	return zpodd->zp_ready;
> +}
> +
> +void zpodd_pre_poweroff(struct ata_device *dev)
> +{
> +	struct zpodd *zpodd = dev->zpodd;
> +
> +	zpodd->powered_off = true;
> +	device_set_run_wake(&dev->sdev->sdev_gendev, true);
> +	acpi_pm_device_run_wake(&dev->sdev->sdev_gendev, true);
> +}
> +
> +void zpodd_pre_poweron(struct ata_device *dev)
> +{
> +	struct zpodd *zpodd = dev->zpodd;
> +
> +	if (zpodd->powered_off) {
> +		acpi_pm_device_run_wake(&dev->sdev->sdev_gendev, false);
> +		device_set_run_wake(&dev->sdev->sdev_gendev, false);
> +	}
> +}
> +
> +void zpodd_post_resume(struct ata_device *dev)
> +{
> +	struct zpodd *zpodd = dev->zpodd;
> +
> +	if (!zpodd->powered_off)
> +		return;
> +
> +	zpodd->powered_off = false;
> +
> +	if (zpodd->from_notify) {
> +		zpodd->from_notify = false;
> +		if (zpodd->drawer)
> +			eject_tray(dev);
> +	}
> +
> +	zpodd->last_ready = 0;
> +	zpodd->zp_ready = false;
> +}

I would really appreciate some comments at least on functions visible
outside zpodd.c.  Please add proper function comments explaining what
they're doing to achieve what.

Thanks.

-- 
tejun

Re: [PATCH v11 7/9] libata: expose pm qos flags for ata device

[Tejun Heo]

On Sun, Jan 06, 2013 at 10:48:27AM +0800, Aaron Lu wrote:
> Expose pm qos flags to user space so that user has a chance to disable
> pm features like power off, if he/she has a broken platform or devices
> or simply does not like this pm feature.
> 
> This flag is exposed to user space only for ata device or atapi device
> that is zero power capable. For normal atapi device, it will never be
> powered off.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  drivers/ata/libata-acpi.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index e832bf6..5b67be3 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -17,6 +17,7 @@
>  #include <linux/pci.h>
>  #include <linux/slab.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pm_qos.h>
>  #include <scsi/scsi_device.h>
>  #include "libata.h"
>  
> @@ -1022,6 +1023,8 @@ static void ata_acpi_unregister_power_resource(struct ata_device *dev)
>  void ata_acpi_bind(struct ata_device *dev)
>  {
>  	ata_acpi_register_power_resource(dev);
> +	if (dev->class == ATA_DEV_ATA || zpodd_dev_enabled(dev))
> +		dev_pm_qos_expose_flags(&dev->sdev->sdev_gendev, 0);
>  }

Why from ata_acpi_bind()?

-- 
tejun

Re: [PATCH v11 8/9] libata: no poll when ODD is powered off

[Tejun Heo]

On Sun, Jan 06, 2013 at 10:48:28AM +0800, Aaron Lu wrote:
> When the ODD is powered off, any action the user did to the ODD that
> would generate a media event will trigger an ACPI interrupt, so the
> poll for media event is no longer necessary. And the poll will also
> cause a runtime status change, which will stop the ODD from staying in
> powered off state, so the poll should better be stopped.
> 
> But since we don't have access to the gendisk structure in LLDs, here
> comes the event_driven flag for scsi device. This flag serves as a
> capability of the device, conveyed by the LLDs to upper layer. It is set
> when LLDs know that this device will no longer generate any media
> related events, so that the check_events can simply return 0 without
> bothering the device, effectively silence the poll.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> @@ -160,6 +160,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 no_dif:1;	/* T10 PI (DIF) should be disabled */
> +	unsigned event_driven:1; /* No need to poll the device */

Again, synchronization.  Also, wouldn't something more explicit like
disable_disk_events better suited?

Thanks.

-- 
tejun

Re: [PATCH v11 0/9] ZPODD Patches

[Tejun Heo]

Hello, Aaron.

On Sun, Jan 06, 2013 at 10:48:20AM +0800, Aaron Lu wrote:
> v11:
> Introduce event_driven flag in scsi_device to silence the media event
> poll after ODD is powered off;
> Removed ata layer PM QOS control, instead, simply limit ACPI state to
> D3_HOT when choosing state;
> Make the power off delay a module param named zpodd_poweroff_delay,
> defaults to 30 seconds.

Other than the nitpicks.  The series generally looks good to me.

Thanks a lot!

-- 
tejun

Re: [PATCH v11 3/9] libata: identify and init ZPODD devices

[Aaron Lu]

On 01/08/2013 02:20 AM, Tejun Heo wrote:
> On Sun, Jan 06, 2013 at 10:48:23AM +0800, Aaron Lu wrote:
>> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
>> index ef01ac0..5aa7322 100644
>> --- a/drivers/ata/libata-acpi.c
>> +++ b/drivers/ata/libata-acpi.c
>> @@ -1063,6 +1063,8 @@ void ata_acpi_bind(struct ata_device *dev)
>>  
>>  void ata_acpi_unbind(struct ata_device *dev)
>>  {
>> +	if (zpodd_dev_enabled(dev))
>> +		zpodd_exit(dev);
>>  	ata_acpi_remove_pm_notifier(dev);
>>  	ata_acpi_unregister_power_resource(dev);
>>  }
> 
> Wouldn't it make more sense to invoke zpodd_exit() from
> ata_scsi_remove_dev() which is approximate counterpart of
> dev_configure?

Yes, I agree.

> 
>> +struct zpodd {
>> +	bool slot:1;
>> +	bool drawer:1;
>> +
>> +	struct ata_device *dev;
>> +};
> 
> Field names are usually indented.  It would be nice to have a comment

checkscript.sh doesn't seem like this if I put a tab around the ':'

ERROR: spaces prohibited around that ':' (ctx:VxW)
#222: FILE: include/uapi/linux/cdrom.h:915:
+	__u8 reserved1:		2;
 	              ^

Which style should I follow?

> explaining synchronization.  Bitfields w/ their implicit RMW ops tend
> to make people wonder about what the access rules are.

The slot and drawer bit field is assigned only once during ata probe
time in EH context, and accessed later in PM's callback context.
Not sure what access rule should I describe...

> 
>> +static int run_atapi_cmd(struct ata_device *dev, const char *cdb,
>> +		unsigned short cdb_len, char *buf, unsigned int buf_len)
>> +{
>> +	struct ata_taskfile tf = {0};
> 
> No need for 0.  { } is enough and more generic.

Thanks for the info.

> 
>> +
>> +	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
>> +	tf.command = ATA_CMD_PACKET;
>> +
>> +	if (buf) {
>> +		tf.protocol = ATAPI_PROT_PIO;
>> +		tf.lbam = buf_len;
>> +	} else {
>> +		tf.protocol = ATAPI_PROT_NODATA;
>> +	}
>> +
>> +	return ata_exec_internal(dev, &tf, cdb,
>> +			buf ? DMA_FROM_DEVICE : DMA_NONE, buf, buf_len, 0);
>> +}
> 
> So, the function name is a bit of misnomer given that ATAPI commands
> are not limited to PIO or DMA_FROM_DEVICE.  Also, this function ends
> up being used twice - once w/ read buffer and once w/o.  Do we really
> want this function?  It's not like exec_internal is difficult to use.

Then I'll remove this function.

> 
>> +/*
>> + * Per the spec, only slot type and drawer type ODD can be supported
>> + *
>> + * Return 0 for slot type, 1 for drawer, -ENODEV for other types or on error.
>> + */
> 
> Maybe bool odd_has_drawer() is better?

There are other types of ODD other than slot and drawer, and both slot
and drawer type ODDs can be supported for ZPODD. So a bool can't convey
such information :-)

> 
>> +static int check_loading_mechanism(struct ata_device *dev)
>> +{
>> +	char buf[16];
>> +	unsigned int ret;
>> +	struct rm_feature_desc *desc = (void *)(buf + 8);
>> +
>> +	char cdb[] = {  GPCMD_GET_CONFIGURATION,
>> +			2,      /* only 1 feature descriptor requested */
>> +			0, 3,   /* 3, removable medium feature */
>> +			0, 0, 0,/* reserved */
>> +			0, sizeof(buf),
>> +			0, 0, 0,
>> +	};
>> +
>> +	ret = run_atapi_cmd(dev, cdb, sizeof(cdb), buf, sizeof(buf));
>> +	if (ret)
>> +		return -ENODEV;
>> +
>> +	if (be16_to_cpu(desc->feature_code) != 3)
>> +		return -ENODEV;
>> +
>> +	if (desc->mech_type == 0 && desc->load == 0 && desc->eject == 1)
>> +		return 0; /* slot */
>> +	else if (desc->mech_type == 1 && desc->load == 0 && desc->eject == 1)
>> +		return 1; /* drawer */
>> +	else
>> +		return -ENODEV;
>> +}
>> +
>> +static bool odd_can_poweroff(struct ata_device *ata_dev)
>> +{
>> +	acpi_handle handle;
>> +	acpi_status status;
>> +	struct acpi_device *acpi_dev;
>> +
>> +	handle = ata_dev_acpi_handle(ata_dev);
>> +	if (!handle)
>> +		return false;
>> +
>> +	status = acpi_bus_get_device(handle, &acpi_dev);
>> +	if (ACPI_FAILURE(status))
>> +		return false;
>> +
>> +	return acpi_device_can_poweroff(acpi_dev);
>> +}
>> +
>> +void zpodd_init(struct ata_device *dev)
>> +{
>> +	int ret;
>> +	struct zpodd *zpodd;
>> +
>> +	if (dev->zpodd)
>> +		return;
>> +
>> +	if (!odd_can_poweroff(dev))
>> +		return;
>> +
>> +	if ((ret = check_loading_mechanism(dev)) == -ENODEV)
>> +		return;
>> +
>> +	zpodd = kzalloc(sizeof(struct zpodd), GFP_KERNEL);
>> +	if (!zpodd)
>> +		return;
>> +
>> +	if (ret)
>> +		zpodd->drawer = true;
>> +	else
>> +		zpodd->slot = true;
>> +
>> +	zpodd->dev = dev;
>> +	dev->zpodd = zpodd;
>> +}
>> +
>> +void zpodd_exit(struct ata_device *dev)
>> +{
>> +	kfree(dev->zpodd);
>> +	dev->zpodd = NULL;
>> +}
>> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
>> index 7148a58..8cb4372 100644
>> --- a/drivers/ata/libata.h
>> +++ b/drivers/ata/libata.h
>> @@ -230,4 +230,18 @@ static inline void ata_sff_exit(void)
>>  { }
>>  #endif /* CONFIG_ATA_SFF */
>>  
>> +/* libata-zpodd.c */
>> +#ifdef CONFIG_SATA_ZPODD
>> +void zpodd_init(struct ata_device *dev);
>> +void zpodd_exit(struct ata_device *dev);
>> +static inline bool zpodd_dev_enabled(struct ata_device *dev)
>> +{
>> +	return dev->zpodd ? true : false;
> 
> 	return dev->zpodd or return dev->zpodd != NULL?
> 
> Other than the above nits, looks okay to me.

Thanks a lot for the review.

-Aaron

Re: [PATCH v11 5/9] libata: check zero power ready status for ZPODD

[Aaron Lu]

On 01/08/2013 02:36 AM, Tejun Heo wrote:
> Hello, Aaron.
> 
> On Sun, Jan 06, 2013 at 10:48:25AM +0800, Aaron Lu wrote:
>> +/* Check zero power ready status */
>> +void zpodd_on_suspend(struct ata_device *dev)
>> +{
>> +	struct zpodd *zpodd = dev->zpodd;
>> +	unsigned long expires;
>> +
>> +	if (!zpready(dev)) {
>> +		zpodd->zp_ready = false;
>> +		zpodd->last_ready = 0;
>> +		return;
>> +	}
>> +
>> +	if (!zpodd->last_ready) {
>> +		zpodd->last_ready = jiffies;
>> +		return;
>> +	}
>> +
>> +	expires = zpodd->last_ready +
>> +		  msecs_to_jiffies(zpodd_poweroff_delay * 1000);
>> +	if (time_before(jiffies, expires))
>> +		return;
>> +
>> +	zpodd->zp_ready = true;
>> +}
> 
> Using 0 jiffies as special value is generally considered a bad form.
> It should be mostly ok here but it's not like avoiding that is
> difficult, so let's please not use 0 jiffies as special value.  If you
> have to, add another variable zp_sample_cnt or whatever.

No problem, thanks!

-Aaron

Re: [PATCH v11 6/9] libata: handle power transition of ODD

[Aaron Lu]

On 01/08/2013 02:42 AM, Tejun Heo wrote:
> On Sun, Jan 06, 2013 at 10:48:26AM +0800, Aaron Lu wrote:
>> +bool zpodd_zpready(struct ata_device *dev)
>> +{
>> +	struct zpodd *zpodd = dev->zpodd;
>> +	return zpodd->zp_ready;
>> +}
>> +
>> +void zpodd_pre_poweroff(struct ata_device *dev)
>> +{
>> +	struct zpodd *zpodd = dev->zpodd;
>> +
>> +	zpodd->powered_off = true;
>> +	device_set_run_wake(&dev->sdev->sdev_gendev, true);
>> +	acpi_pm_device_run_wake(&dev->sdev->sdev_gendev, true);
>> +}
>> +
>> +void zpodd_pre_poweron(struct ata_device *dev)
>> +{
>> +	struct zpodd *zpodd = dev->zpodd;
>> +
>> +	if (zpodd->powered_off) {
>> +		acpi_pm_device_run_wake(&dev->sdev->sdev_gendev, false);
>> +		device_set_run_wake(&dev->sdev->sdev_gendev, false);
>> +	}
>> +}
>> +
>> +void zpodd_post_resume(struct ata_device *dev)
>> +{
>> +	struct zpodd *zpodd = dev->zpodd;
>> +
>> +	if (!zpodd->powered_off)
>> +		return;
>> +
>> +	zpodd->powered_off = false;
>> +
>> +	if (zpodd->from_notify) {
>> +		zpodd->from_notify = false;
>> +		if (zpodd->drawer)
>> +			eject_tray(dev);
>> +	}
>> +
>> +	zpodd->last_ready = 0;
>> +	zpodd->zp_ready = false;
>> +}
> 
> I would really appreciate some comments at least on functions visible
> outside zpodd.c.  Please add proper function comments explaining what
> they're doing to achieve what.

Will add them in next version, thanks.

-Aaron

Re: [PATCH v11 3/9] libata: identify and init ZPODD devices

[Tejun Heo]

Hello, Aaron.

On Tue, Jan 08, 2013 at 05:07:46PM +0800, Aaron Lu wrote:
> >> +struct zpodd {
> >> +	bool slot:1;
> >> +	bool drawer:1;
> >> +
> >> +	struct ata_device *dev;
> >> +};
> > 
> > Field names are usually indented.  It would be nice to have a comment
> 
> checkscript.sh doesn't seem like this if I put a tab around the ':'
> 
> ERROR: spaces prohibited around that ':' (ctx:VxW)
> #222: FILE: include/uapi/linux/cdrom.h:915:
> +	__u8 reserved1:		2;
>  	              ^
> 
> Which style should I follow?

struct zpodd {
	bool			slot:1;
	bool			drawer:1;

	struct ata_device	*dev;
};

> > explaining synchronization.  Bitfields w/ their implicit RMW ops tend
> > to make people wonder about what the access rules are.
> 
> The slot and drawer bit field is assigned only once during ata probe
> time in EH context, and accessed later in PM's callback context.
> Not sure what access rule should I describe...

/* init during probe, RO afterwards */ should do but I'd prefer if you
stay away from bitfields altogether.  There are cases where bitfields
are okay but when you're working across multiple locking domains, it
usually is a bad idea because the code which accesses those fields
look completely independent while still being able to interact with
each other.  They look properly synchronized until you realized they
live on the same machine word.

> >> +/*
> >> + * Per the spec, only slot type and drawer type ODD can be supported
> >> + *
> >> + * Return 0 for slot type, 1 for drawer, -ENODEV for other types or on error.
> >> + */
> > 
> > Maybe bool odd_has_drawer() is better?
> 
> There are other types of ODD other than slot and drawer, and both slot
> and drawer type ODDs can be supported for ZPODD. So a bool can't convey
> such information :-)

Then please make it a proper ATA enum and use it in struct zpodd too.

Thanks!

-- 
tejun

Re: [PATCH v11 3/9] libata: identify and init ZPODD devices

[Aaron Lu]

Hi Tejun,

On 01/09/2013 01:52 AM, Tejun Heo wrote:
> Hello, Aaron.
> 
> On Tue, Jan 08, 2013 at 05:07:46PM +0800, Aaron Lu wrote:
>>>> +struct zpodd {
>>>> +	bool slot:1;
>>>> +	bool drawer:1;
>>>> +
>>>> +	struct ata_device *dev;
>>>> +};
>>>
>>> Field names are usually indented.  It would be nice to have a comment
>>
>> checkscript.sh doesn't seem like this if I put a tab around the ':'
>>
>> ERROR: spaces prohibited around that ':' (ctx:VxW)
>> #222: FILE: include/uapi/linux/cdrom.h:915:
>> +	__u8 reserved1:		2;
>>  	              ^
>>
>> Which style should I follow?
> 
> struct zpodd {
> 	bool			slot:1;
> 	bool			drawer:1;
> 
> 	struct ata_device	*dev;
> };

Oh, got it :-)

> 
>>> explaining synchronization.  Bitfields w/ their implicit RMW ops tend
>>> to make people wonder about what the access rules are.
>>
>> The slot and drawer bit field is assigned only once during ata probe
>> time in EH context, and accessed later in PM's callback context.
>> Not sure what access rule should I describe...
> 
> /* init during probe, RO afterwards */ should do but I'd prefer if you
> stay away from bitfields altogether.  There are cases where bitfields
> are okay but when you're working across multiple locking domains, it
> usually is a bad idea because the code which accesses those fields
> look completely independent while still being able to interact with
> each other.  They look properly synchronized until you realized they
> live on the same machine word.

Then I will not use bitfields, I was just thinking to save some space
and didn't realize it would bring such nasty problems...

> 
>>>> +/*
>>>> + * Per the spec, only slot type and drawer type ODD can be supported
>>>> + *
>>>> + * Return 0 for slot type, 1 for drawer, -ENODEV for other types or on error.
>>>> + */
>>>
>>> Maybe bool odd_has_drawer() is better?
>>
>> There are other types of ODD other than slot and drawer, and both slot
>> and drawer type ODDs can be supported for ZPODD. So a bool can't convey
>> such information :-)
> 
> Then please make it a proper ATA enum and use it in struct zpodd too.

Sure, and thanks for all the suggestions!

-Aaron

Re: [PATCH v11 7/9] libata: expose pm qos flags for ata device

[Aaron Lu]

On 01/08/2013 02:43 AM, Tejun Heo wrote:
> On Sun, Jan 06, 2013 at 10:48:27AM +0800, Aaron Lu wrote:
>> Expose pm qos flags to user space so that user has a chance to disable
>> pm features like power off, if he/she has a broken platform or devices
>> or simply does not like this pm feature.
>>
>> This flag is exposed to user space only for ata device or atapi device
>> that is zero power capable. For normal atapi device, it will never be
>> powered off.
>>
>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
>> ---
>>  drivers/ata/libata-acpi.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
>> index e832bf6..5b67be3 100644
>> --- a/drivers/ata/libata-acpi.c
>> +++ b/drivers/ata/libata-acpi.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/pci.h>
>>  #include <linux/slab.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/pm_qos.h>
>>  #include <scsi/scsi_device.h>
>>  #include "libata.h"
>>  
>> @@ -1022,6 +1023,8 @@ static void ata_acpi_unregister_power_resource(struct ata_device *dev)
>>  void ata_acpi_bind(struct ata_device *dev)
>>  {
>>  	ata_acpi_register_power_resource(dev);
>> +	if (dev->class == ATA_DEV_ATA || zpodd_dev_enabled(dev))
>> +		dev_pm_qos_expose_flags(&dev->sdev->sdev_gendev, 0);
>>  }
> 
> Why from ata_acpi_bind()?

I think I should add a check to see if platform can power off the device
before export this qos flag. The check is done by ACPI, and the qos flag
is for the scsi device. So looks like this is the proper place?

Thanks,
Aaron

RE: [PATCH v11 0/9] ZPODD Patches

[Wu, Jeff]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Aaron Lu
> Sent: Sunday, January 06, 2013 10:48 AM
> To: Jeff Garzik; James Bottomley; Rafael J. Wysocki; Alan Stern; Tejun Heo
> Cc: Aaron Lu; Wu, Jeff; linux-ide@vger.kernel.org; linux-pm@vger.kernel.org;
> linux-scsi@vger.kernel.org; linux-acpi@vger.kernel.org; Aaron Lu
> Subject: [PATCH v11 0/9] ZPODD Patches
> 
> v11:
> Introduce event_driven flag in scsi_device to silence the media event poll
> after ODD is powered off; Removed ata layer PM QOS control, instead,
> simply limit ACPI state to D3_HOT when choosing state; Make the power off
> delay a module param named zpodd_poweroff_delay, defaults to 30
> seconds.
> 

1. Tray type ZPODD , test pass;
2. Slot type ZPODD , test pass;

Tested-by: Jeff Wu  <jeff.wu@amd.com>


> v10:
> Introduce PM_QOS_NO_POLL flag to skip calling disk's events_check callback;
> Do not use zero power ready hint information from event poll; Check
> attached device in port's runtime idle callback to decide if suspend is desired;
> Address various comments from Tejun Heo.
> 
> v9:
> Build ZPODD as part of libata instead of another standalone module as it is
> tightly related to other libata files.
> Identify and init ZPODD during probe time instead of after SCSI device is
> created as suggested by Tejun Heo.
> Make use of pm qos flag to give ACPI hint when choosing ACPI state.
> Expose qos flag to give user control of whether power off is allowed.
> 
> This patchset used Rafael's pm-qos work:
> git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git pm-qos
> 
> v8:
> This version is a redesign, it doesn't have much to do with previous versions.
> The ZPODD implementation is done almost entirely in ATA layer now, except
> 2 helper functions from SCSI sr driver to block disk events.
> 
> The basic idea is that, when ata port is runtime suspended, it will check if the
> ODD is ready to be powered off. And if yes, events is blocked and power
> omitted; if not, ODD's power supply remains unchanged by keeping ACPI
> state at D0.
> 
> Some background knowledge about ZPODD is added below v1 history log.
> 
> v7:
> Re work of runtime pm of sr driver, based on ideas of Alan Stern and Oliver
> Neukum.
> 
> Jeff, due to the ready_to_power_off flag added, there is a small change in
> [PATCH v7 6/6] libata: acpi: respect may_power_off flag, please check if I can
> still get your ack, thanks.
> 
> v6:
> When user changes may_power_off flag through sysfs entry and if device is
> already runtime suspended, resume resume it so that it can respect this flag
> next time it is runtime suspended as suggested by Alan Stern.
> Call scsi_autopm_get/put_device once in sr_check_events as suggested by
> Alan Stern.
> 
> 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.
> 
> Some background knowledge about ZPODD:
> ODD means Optical Disc Drive.
> ZPODD means Zero Power ODD, it is a mechanism to place the ODD into zero
> power state when the system is running at S0 system state without user's
> awareness.
> It achieved this by ACPI and SATA device attention pin. For power off, normal
> ACPI control method is used to place the device into D3 cold ACPI device
> state, aka. device power supply omitted. For power on, when user press the
> eject button of a drawer type ODD or when user inserts an ODD into a slot
> type ODD, the device attention pin will trigger. In the current x86
> implementation, this pin will connect to a GPE, and the GPE will trigger an
> ACPI interrupt. With our pre-registered ACPI notification code, the device
> can be runtime resumed, and we place the device back to full power state by
> setting its ACPI state to D0. The whole process is transparent to the end user.
> 
> Aaron Lu (9):
>   scsi: sr: support runtime pm
>   libata: Add CONFIG_SATA_ZPODD
>   libata: identify and init ZPODD devices
>   libata: move acpi notification code to zpodd
>   libata: check zero power ready status for ZPODD
>   libata: handle power transition of ODD
>   libata: expose pm qos flags for ata device
>   libata: no poll when ODD is powered off
>   libata: do not suspend port if normal ODD is attached
> 
>  drivers/ata/Kconfig        |  13 +++
>  drivers/ata/Makefile       |   1 +
>  drivers/ata/libata-acpi.c  | 111 +++++-------------  drivers/ata/libata-core.c  |
> 23 +++-
>  drivers/ata/libata-eh.c    |  12 +-
>  drivers/ata/libata-zpodd.c | 281
> +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/ata/libata.h       |  27 +++++
>  drivers/scsi/sr.c          |  36 +++++-
>  include/linux/libata.h     |   3 +
>  include/scsi/scsi_device.h |   1 +
>  include/uapi/linux/cdrom.h |  34 ++++++
>  11 files changed, 452 insertions(+), 90 deletions(-)  create mode 100644
> drivers/ata/libata-zpodd.c
> 
> --
> 1.7.11.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the
> body of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

Re: [PATCH v11 0/9] ZPODD Patches

[Aaron Lu]

On 01/09/2013 03:55 PM, Wu, Jeff wrote:
> 
> 
>> -----Original Message-----
>> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
>> owner@vger.kernel.org] On Behalf Of Aaron Lu
>> Sent: Sunday, January 06, 2013 10:48 AM
>> To: Jeff Garzik; James Bottomley; Rafael J. Wysocki; Alan Stern; Tejun Heo
>> Cc: Aaron Lu; Wu, Jeff; linux-ide@vger.kernel.org; linux-pm@vger.kernel.org;
>> linux-scsi@vger.kernel.org; linux-acpi@vger.kernel.org; Aaron Lu
>> Subject: [PATCH v11 0/9] ZPODD Patches
>>
>> v11:
>> Introduce event_driven flag in scsi_device to silence the media event poll
>> after ODD is powered off; Removed ata layer PM QOS control, instead,
>> simply limit ACPI state to D3_HOT when choosing state; Make the power off
>> delay a module param named zpodd_poweroff_delay, defaults to 30
>> seconds.
>>
> 
> 1. Tray type ZPODD , test pass;
> 2. Slot type ZPODD , test pass;
> 
> Tested-by: Jeff Wu  <jeff.wu@amd.com>

Cool! Thanks a lot for your test.

-Aaron

> 
> 
>> v10:
>> Introduce PM_QOS_NO_POLL flag to skip calling disk's events_check callback;
>> Do not use zero power ready hint information from event poll; Check
>> attached device in port's runtime idle callback to decide if suspend is desired;
>> Address various comments from Tejun Heo.
>>
>> v9:
>> Build ZPODD as part of libata instead of another standalone module as it is
>> tightly related to other libata files.
>> Identify and init ZPODD during probe time instead of after SCSI device is
>> created as suggested by Tejun Heo.
>> Make use of pm qos flag to give ACPI hint when choosing ACPI state.
>> Expose qos flag to give user control of whether power off is allowed.
>>
>> This patchset used Rafael's pm-qos work:
>> git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git pm-qos
>>
>> v8:
>> This version is a redesign, it doesn't have much to do with previous versions.
>> The ZPODD implementation is done almost entirely in ATA layer now, except
>> 2 helper functions from SCSI sr driver to block disk events.
>>
>> The basic idea is that, when ata port is runtime suspended, it will check if the
>> ODD is ready to be powered off. And if yes, events is blocked and power
>> omitted; if not, ODD's power supply remains unchanged by keeping ACPI
>> state at D0.
>>
>> Some background knowledge about ZPODD is added below v1 history log.
>>
>> v7:
>> Re work of runtime pm of sr driver, based on ideas of Alan Stern and Oliver
>> Neukum.
>>
>> Jeff, due to the ready_to_power_off flag added, there is a small change in
>> [PATCH v7 6/6] libata: acpi: respect may_power_off flag, please check if I can
>> still get your ack, thanks.
>>
>> v6:
>> When user changes may_power_off flag through sysfs entry and if device is
>> already runtime suspended, resume resume it so that it can respect this flag
>> next time it is runtime suspended as suggested by Alan Stern.
>> Call scsi_autopm_get/put_device once in sr_check_events as suggested by
>> Alan Stern.
>>
>> 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.
>>
>> Some background knowledge about ZPODD:
>> ODD means Optical Disc Drive.
>> ZPODD means Zero Power ODD, it is a mechanism to place the ODD into zero
>> power state when the system is running at S0 system state without user's
>> awareness.
>> It achieved this by ACPI and SATA device attention pin. For power off, normal
>> ACPI control method is used to place the device into D3 cold ACPI device
>> state, aka. device power supply omitted. For power on, when user press the
>> eject button of a drawer type ODD or when user inserts an ODD into a slot
>> type ODD, the device attention pin will trigger. In the current x86
>> implementation, this pin will connect to a GPE, and the GPE will trigger an
>> ACPI interrupt. With our pre-registered ACPI notification code, the device
>> can be runtime resumed, and we place the device back to full power state by
>> setting its ACPI state to D0. The whole process is transparent to the end user.
>>
>> Aaron Lu (9):
>>   scsi: sr: support runtime pm
>>   libata: Add CONFIG_SATA_ZPODD
>>   libata: identify and init ZPODD devices
>>   libata: move acpi notification code to zpodd
>>   libata: check zero power ready status for ZPODD
>>   libata: handle power transition of ODD
>>   libata: expose pm qos flags for ata device
>>   libata: no poll when ODD is powered off
>>   libata: do not suspend port if normal ODD is attached
>>
>>  drivers/ata/Kconfig        |  13 +++
>>  drivers/ata/Makefile       |   1 +
>>  drivers/ata/libata-acpi.c  | 111 +++++-------------  drivers/ata/libata-core.c  |
>> 23 +++-
>>  drivers/ata/libata-eh.c    |  12 +-
>>  drivers/ata/libata-zpodd.c | 281
>> +++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/ata/libata.h       |  27 +++++
>>  drivers/scsi/sr.c          |  36 +++++-
>>  include/linux/libata.h     |   3 +
>>  include/scsi/scsi_device.h |   1 +
>>  include/uapi/linux/cdrom.h |  34 ++++++
>>  11 files changed, 452 insertions(+), 90 deletions(-)  create mode 100644
>> drivers/ata/libata-zpodd.c
>>
>> --
>> 1.7.11.7
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the
>> body of a message to majordomo@vger.kernel.org More majordomo info at
>> http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH v11 0/9] ZPODD Patches

[Aaron Lu]

On 01/08/2013 02:49 AM, Tejun Heo wrote:
> Hello, Aaron.
> 
> On Sun, Jan 06, 2013 at 10:48:20AM +0800, Aaron Lu wrote:
>> v11:
>> Introduce event_driven flag in scsi_device to silence the media event
>> poll after ODD is powered off;
>> Removed ata layer PM QOS control, instead, simply limit ACPI state to
>> D3_HOT when choosing state;
>> Make the power off delay a module param named zpodd_poweroff_delay,
>> defaults to 30 seconds.
> 
> Other than the nitpicks.  The series generally looks good to me.

This is very encouraging, thanks a lot!

Best regards,
Aaron