[PATCH v8 00/11] ZPODD Patches

[PATCH v8 00/11] ZPODD Patches

[Aaron Lu]

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 (11):
  scsi: sr: support runtime pm
  ata: zpodd: Add CONFIG_SATA_ZPODD
  ata: zpodd: identify and init ZPODD devices
  libata: acpi: move acpi notification code to sata_zpodd
  libata-eh: allow defer in ata_exec_internal
  ata: zpodd: check loading mechanism for ODD
  libata: separate ATAPI code
  ata: zpodd: check zero power ready status
  block: add a new interface to block events
  scsi: sr: support (un)block events
  ata: zpodd: handle power transition of ODD

 block/genhd.c              |  26 ++++
 drivers/ata/Kconfig        |  12 ++
 drivers/ata/Makefile       |   3 +-
 drivers/ata/libata-acpi.c  | 143 +++++++++-----------
 drivers/ata/libata-atapi.c |  88 +++++++++++++
 drivers/ata/libata-atapi.h |   7 +
 drivers/ata/libata-core.c  |  54 ++++++--
 drivers/ata/libata-eh.c    |  86 +-----------
 drivers/ata/libata-scsi.c  |   5 +
 drivers/ata/sata_zpodd.c   | 321 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/sata_zpodd.h   |  37 ++++++
 drivers/scsi/Makefile      |   1 +
 drivers/scsi/sr.c          |  30 ++++-
 drivers/scsi/sr_zpodd.c    |  21 +++
 drivers/scsi/sr_zpodd.h    |   9 ++
 include/linux/cdrom.h      |  35 +++++
 include/linux/genhd.h      |   1 +
 17 files changed, 695 insertions(+), 184 deletions(-)
 create mode 100644 drivers/ata/libata-atapi.c
 create mode 100644 drivers/ata/libata-atapi.h
 create mode 100644 drivers/ata/sata_zpodd.c
 create mode 100644 drivers/ata/sata_zpodd.h
 create mode 100644 drivers/scsi/sr_zpodd.c
 create mode 100644 drivers/scsi/sr_zpodd.h

-- 
1.7.12.4

[PATCH v8 01/11] 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.12.4

[PATCH v8 02/11] ata: zpodd: Add CONFIG_SATA_ZPODD

[Aaron Lu]

Added a new config CONFIG_SATA_ZPODD, which is ued to support
SATA based zero power ODD. It depends on ACPI, and selects BLK_DEV_SR
as the implementation of ZPODD depends on SCSI sr driver.

2 new files are added, which will be used to host ZPODD related code.
They are empty for this commit.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/ata/Kconfig  | 12 ++++++++++++
 drivers/ata/Makefile |  1 +
 2 files changed, 13 insertions(+)
 create mode 100644 drivers/ata/sata_zpodd.c
 create mode 100644 drivers/ata/sata_zpodd.h

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index e08d322..2cdecee 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -58,6 +58,18 @@ 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 ACPI
+	select BLK_DEV_SR
+	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).
+
 config SATA_PMP
 	bool "SATA Port Multiplier support"
 	default y
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 9329daf..a5120ff 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
+obj-$(CONFIG_SATA_ZPODD)	+= sata_zpodd.o
diff --git a/drivers/ata/sata_zpodd.c b/drivers/ata/sata_zpodd.c
new file mode 100644
index 0000000..e69de29
diff --git a/drivers/ata/sata_zpodd.h b/drivers/ata/sata_zpodd.h
new file mode 100644
index 0000000..e69de29
-- 
1.7.12.4

[PATCH v8 03/11] ata: zpodd: identify and init ZPODD devices

[Aaron Lu]

If the ODD supports device attention and the platform can runtime
power off it through ACPI, it means this ODD together with this platform
is ZPODD capable. For this case, zpodd_init is called and a new
structure is allocated for the device 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 | 32 ++++++++++++++++++++++++++++++++
 drivers/ata/sata_zpodd.c  | 23 +++++++++++++++++++++++
 drivers/ata/sata_zpodd.h  | 24 ++++++++++++++++++++++++
 3 files changed, 79 insertions(+)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index fd9ecf7..53b2f10 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -19,6 +19,7 @@
 #include <linux/pm_runtime.h>
 #include <scsi/scsi_device.h>
 #include "libata.h"
+#include "sata_zpodd.h"
 
 #include <acpi/acpi_bus.h>
 
@@ -1051,14 +1052,45 @@ static void ata_acpi_unregister_power_resource(struct ata_device *dev)
 	acpi_power_resource_unregister_device(device, handle);
 }
 
+static bool ata_acpi_device_poweroff(struct ata_device *ata_dev)
+{
+	acpi_handle handle;
+	acpi_status status;
+	struct acpi_device_power_state *states;
+	struct acpi_device *acpi_dev;
+
+	handle = DEVICE_ACPI_HANDLE(&ata_dev->sdev->sdev_gendev);
+	if (!handle)
+		return false;
+
+	status = acpi_bus_get_device(handle, &acpi_dev);
+	if (ACPI_FAILURE(status))
+		return false;
+
+	/*
+	 * If firmware has _PS3 or _PR3 for this device,
+	 * it means this device can be runtime powered off
+	 */
+	states = acpi_dev->power.states;
+	if (states[ACPI_STATE_D3_HOT].flags.valid ||
+	    states[ACPI_STATE_D3_COLD].flags.explicit_set)
+		return true;
+	else
+		return false;
+}
+
 void ata_acpi_bind(struct ata_device *dev)
 {
 	ata_acpi_add_pm_notifier(dev);
 	ata_acpi_register_power_resource(dev);
+	if (dev->flags & ATA_DFLAG_DA && ata_acpi_device_poweroff(dev))
+		zpodd_init(dev);
 }
 
 void ata_acpi_unbind(struct ata_device *dev)
 {
+	if (zpodd_dev_enabled(dev))
+		zpodd_deinit(dev);
 	ata_acpi_remove_pm_notifier(dev);
 	ata_acpi_unregister_power_resource(dev);
 }
diff --git a/drivers/ata/sata_zpodd.c b/drivers/ata/sata_zpodd.c
index e69de29..b37db2f 100644
--- a/drivers/ata/sata_zpodd.c
+++ b/drivers/ata/sata_zpodd.c
@@ -0,0 +1,23 @@
+#include <linux/libata.h>
+
+struct zpodd {
+	struct ata_device *dev;
+};
+
+
+void zpodd_init(struct ata_device *dev)
+{
+	struct zpodd *zpodd;
+	zpodd = kzalloc(sizeof(struct zpodd), GFP_KERNEL);
+	if (!zpodd)
+		return;
+
+	zpodd->dev = dev;
+	dev->private_data = zpodd;
+}
+
+void zpodd_deinit(struct ata_device *dev)
+{
+	kfree(dev->private_data);
+	dev->private_data = NULL;
+}
diff --git a/drivers/ata/sata_zpodd.h b/drivers/ata/sata_zpodd.h
index e69de29..e320c6f 100644
--- a/drivers/ata/sata_zpodd.h
+++ b/drivers/ata/sata_zpodd.h
@@ -0,0 +1,24 @@
+#ifndef __SATA_ZPODD_H__
+#define __SATA_ZPODD_H__
+
+#include <linux/libata.h>
+
+#ifdef CONFIG_SATA_ZPODD
+void zpodd_init(struct ata_device *);
+void zpodd_deinit(struct ata_device *);
+
+static bool zpodd_dev_enabled(struct ata_device *dev)
+{
+	if (dev->flags & ATA_DFLAG_DA && dev->private_data)
+		return true;
+	else
+		return false;
+}
+
+#else
+static inline void zpodd_init(struct ata_device *dev) {}
+static inline void zpodd_deinit(struct ata_device *dev) {}
+static inline bool zpodd_dev_enabled(struct ata_device *dev) { return false; }
+#endif
+
+#endif
-- 
1.7.12.4

[PATCH v8 04/11] libata: acpi: move acpi notification code to sata_zpodd

[Aaron Lu]

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

And the add/remove_pm_notifier code is simplified a little bit that it
does not check things like if the handle is NULL and if a corresponding
acpi_device is there for the handle as they are guaranteed by the
ata_acpi_device_poweroff already.

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

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 53b2f10..3167fc2 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -971,57 +971,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;
@@ -1081,7 +1030,6 @@ static bool ata_acpi_device_poweroff(struct ata_device *ata_dev)
 
 void ata_acpi_bind(struct ata_device *dev)
 {
-	ata_acpi_add_pm_notifier(dev);
 	ata_acpi_register_power_resource(dev);
 	if (dev->flags & ATA_DFLAG_DA && ata_acpi_device_poweroff(dev))
 		zpodd_init(dev);
@@ -1091,7 +1039,6 @@ void ata_acpi_unbind(struct ata_device *dev)
 {
 	if (zpodd_dev_enabled(dev))
 		zpodd_deinit(dev);
-	ata_acpi_remove_pm_notifier(dev);
 	ata_acpi_unregister_power_resource(dev);
 }
 
@@ -1133,9 +1080,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)
 		ata_dev = &ap->link.device[sdev->channel];
@@ -1147,21 +1091,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/sata_zpodd.c b/drivers/ata/sata_zpodd.c
index b37db2f..009b1e4 100644
--- a/drivers/ata/sata_zpodd.c
+++ b/drivers/ata/sata_zpodd.c
@@ -1,9 +1,39 @@
 #include <linux/libata.h>
+#include <linux/pm_runtime.h>
+#include <scsi/scsi_device.h>
 
 struct zpodd {
+	bool from_notify:1;	/* resumed as a result of acpi notification */
+
 	struct ata_device *dev;
 };
 
+static void zpodd_wake_dev(acpi_handle handle, u32 event, void *context)
+{
+	struct ata_device *ata_dev = context;
+	struct zpodd *zpodd = ata_dev->private_data;
+	struct device *dev = &ata_dev->sdev->sdev_gendev;
+
+	if (event == ACPI_NOTIFY_DEVICE_WAKE && pm_runtime_suspended(dev)) {
+		zpodd->from_notify = true;
+		pm_runtime_resume(dev);
+	}
+}
+
+static void zpodd_acpi_add_pm_notifier(struct ata_device *dev)
+{
+	acpi_handle handle = DEVICE_ACPI_HANDLE(&dev->sdev->sdev_gendev);
+	acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
+				    zpodd_wake_dev, dev);
+	device_set_run_wake(&dev->sdev->sdev_gendev, true);
+}
+
+static void zpodd_acpi_remove_pm_notifier(struct ata_device *dev)
+{
+	acpi_handle handle = DEVICE_ACPI_HANDLE(&dev->sdev->sdev_gendev);
+	device_set_run_wake(&dev->sdev->sdev_gendev, false);
+	acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY, zpodd_wake_dev);
+}
 
 void zpodd_init(struct ata_device *dev)
 {
@@ -12,12 +42,14 @@ void zpodd_init(struct ata_device *dev)
 	if (!zpodd)
 		return;
 
+	zpodd_acpi_add_pm_notifier(dev);
 	zpodd->dev = dev;
 	dev->private_data = zpodd;
 }
 
 void zpodd_deinit(struct ata_device *dev)
 {
+	zpodd_acpi_remove_pm_notifier(dev);
 	kfree(dev->private_data);
 	dev->private_data = NULL;
 }
-- 
1.7.12.4

[PATCH v8 05/11] libata-eh: allow defer in ata_exec_internal

[Aaron Lu]

ata_exec_internal will preempt the ata link's active_tag and ata port's
qc_active flags, this is OK for error recovery, but if normal code path
wants to use ata_exec_internal, there is a problem: we need to check if
it is OK to issue a new command with the help of port_ops->defer.

In ZPODD, I'll need to find out the loading mechanism of the ODD by
issuing a GET_CONFIGURATION command. And this command may very well
race with commands issued from SCSI layer. So instead of preempt the
current command, defer the new command if it's not OK to issue it, as
it is always wrong to issue a non-NCQ command when there is command(s)
in processing.

So ata_exec_internal is modified to check if it is in eh recovery
environment, and if yes, act as before; if not, check if this command
should be defered with the help of port_ops->defer.

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

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 611050d..95fb7b8 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1557,6 +1557,7 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 	unsigned long flags;
 	unsigned int err_mask;
 	int rc;
+	bool eh_in_recover;
 
 	spin_lock_irqsave(ap->lock, flags);
 
@@ -1588,14 +1589,21 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 	qc->dev = dev;
 	ata_qc_reinit(qc);
 
-	preempted_tag = link->active_tag;
-	preempted_sactive = link->sactive;
-	preempted_qc_active = ap->qc_active;
-	preempted_nr_active_links = ap->nr_active_links;
-	link->active_tag = ATA_TAG_POISON;
-	link->sactive = 0;
-	ap->qc_active = 0;
-	ap->nr_active_links = 0;
+	eh_in_recover = ap->pflags & ATA_PFLAG_EH_IN_PROGRESS;
+	if (eh_in_recover) {
+		preempted_tag = link->active_tag;
+		preempted_sactive = link->sactive;
+		preempted_qc_active = ap->qc_active;
+		preempted_nr_active_links = ap->nr_active_links;
+		link->active_tag = ATA_TAG_POISON;
+		link->sactive = 0;
+		ap->qc_active = 0;
+		ap->nr_active_links = 0;
+	} else if (ap->ops->qc_defer && ap->ops->qc_defer(qc)) {
+		ata_qc_free(qc);
+		spin_unlock_irqrestore(ap->lock, flags);
+		return -EBUSY;
+	}
 
 	/* prepare & issue qc */
 	qc->tf = *tf;
@@ -1687,10 +1695,12 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 	err_mask = qc->err_mask;
 
 	ata_qc_free(qc);
-	link->active_tag = preempted_tag;
-	link->sactive = preempted_sactive;
-	ap->qc_active = preempted_qc_active;
-	ap->nr_active_links = preempted_nr_active_links;
+	if (eh_in_recover) {
+		link->active_tag = preempted_tag;
+		link->sactive = preempted_sactive;
+		ap->qc_active = preempted_qc_active;
+		ap->nr_active_links = preempted_nr_active_links;
+	}
 
 	spin_unlock_irqrestore(ap->lock, flags);
 
-- 
1.7.12.4

[PATCH v8 06/11] ata: zpodd: check loading mechanism for ODD

[Aaron Lu]

Per the Mount Fuji spec on SATA ZPODD, only slot type or drawer type
ODD can support ZPODD. So add a function to check this, and if fails,
skip setting of dev->private_data so that zpodd_dev_enabled will return
false.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/ata/sata_zpodd.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/cdrom.h    | 35 +++++++++++++++++++
 2 files changed, 122 insertions(+)

diff --git a/drivers/ata/sata_zpodd.c b/drivers/ata/sata_zpodd.c
index 009b1e4..c7c63a1 100644
--- a/drivers/ata/sata_zpodd.c
+++ b/drivers/ata/sata_zpodd.c
@@ -1,13 +1,89 @@
 #include <linux/libata.h>
 #include <linux/pm_runtime.h>
+#include <linux/cdrom.h>
 #include <scsi/scsi_device.h>
 
+#include "libata.h"
+
 struct zpodd {
+	bool slot:1;
+	bool drawer:1;
 	bool from_notify:1;	/* resumed as a result of acpi notification */
 
 	struct ata_device *dev;
 };
 
+static int zpodd_run_atapi_cmd(struct ata_device *dev, const char *cdb,
+		unsigned short cdb_len, char *buf, unsigned int buf_len)
+{
+	int ret = 0;
+	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;
+	}
+
+	ata_eh_acquire(dev->link->ap);
+	ret = ata_exec_internal(dev, &tf, cdb, buf ? DMA_FROM_DEVICE : DMA_NONE,
+				buf, buf_len, 0);
+	ata_eh_release(dev->link->ap);
+
+	return ret;
+}
+
+/*
+ * Per the spec, only slot type and drawer type ODD can be supported
+ *
+ * Return 0 for slot, 1 for drawer, and -ENODEV for others or on error.
+ */
+static int check_loading_mechanism(struct ata_device *dev)
+{
+	char buf[16];
+	unsigned int ret, i, retry = 10;
+	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,
+	};
+
+	/*
+	 * When we issue this command in ATA layer, SCSI layer may also
+	 * issue some commands to the ODD. To avoid this command failed
+	 * due to defer, retry for several times.
+	 */
+	for (i = 0; i < retry; i++) {
+		ret = zpodd_run_atapi_cmd(dev, cdb, sizeof(cdb),
+					  buf, sizeof(buf));
+		if (!ret || ret != -EBUSY)
+			break;
+		msleep(20);
+	}
+	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)
+		ret = 0; /* slot */
+	else if (desc->mech_type == 1 && desc->load == 0 && desc->eject == 1)
+		ret = 1; /* drawer */
+	else
+		ret = -ENODEV;
+
+	return ret;
+}
+
 static void zpodd_wake_dev(acpi_handle handle, u32 event, void *context)
 {
 	struct ata_device *ata_dev = context;
@@ -38,10 +114,21 @@ static void zpodd_acpi_remove_pm_notifier(struct ata_device *dev)
 void zpodd_init(struct ata_device *dev)
 {
 	struct zpodd *zpodd;
+	int ret;
+
 	zpodd = kzalloc(sizeof(struct zpodd), GFP_KERNEL);
 	if (!zpodd)
 		return;
 
+	if ((ret = check_loading_mechanism(dev)) == -ENODEV) {
+		kfree(zpodd);
+		return;
+	}
+	if (ret == 0)
+		zpodd->slot = true;
+	else
+		zpodd->drawer = true;
+
 	zpodd_acpi_add_pm_notifier(dev);
 	zpodd->dev = dev;
 	dev->private_data = zpodd;
diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
index dfd7f18..755f2b4 100644
--- a/include/linux/cdrom.h
+++ b/include/linux/cdrom.h
@@ -806,6 +806,41 @@ struct rwrt_feature_desc {
 	__u8 reserved3;
 };
 
+/* 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;
+};
+
 typedef struct {
 	__be16 disc_information_length;
 #if defined(__BIG_ENDIAN_BITFIELD)
-- 
1.7.12.4

[PATCH v8 07/11] libata: separate ATAPI code

[Aaron Lu]

The atapi_eh_tur and atapi_eh_request_sense can be reused by ZPODD
code, so separate them out to a file named libata-atapi.c. A header
file libata-atapi.h is added and the Makefile is modified accordingly.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/ata/Makefile       |  2 +-
 drivers/ata/libata-atapi.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata-atapi.h |  7 ++++
 drivers/ata/libata-eh.c    | 86 +-------------------------------------------
 4 files changed, 97 insertions(+), 86 deletions(-)
 create mode 100644 drivers/ata/libata-atapi.c
 create mode 100644 drivers/ata/libata-atapi.h

diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index a5120ff..335407c 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -103,7 +103,7 @@ obj-$(CONFIG_ATA_GENERIC)	+= ata_generic.o
 # Should be last libata driver
 obj-$(CONFIG_PATA_LEGACY)	+= pata_legacy.o
 
-libata-y	:= libata-core.o libata-scsi.o libata-eh.o libata-transport.o
+libata-y	:= libata-core.o libata-scsi.o libata-eh.o libata-atapi.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
diff --git a/drivers/ata/libata-atapi.c b/drivers/ata/libata-atapi.c
new file mode 100644
index 0000000..28684ae
--- /dev/null
+++ b/drivers/ata/libata-atapi.c
@@ -0,0 +1,88 @@
+#include <linux/libata.h>
+#include <scsi/scsi_cmnd.h>
+#include "libata.h"
+
+/**
+ *	atapi_eh_tur - perform ATAPI TEST_UNIT_READY
+ *	@dev: target ATAPI device
+ *	@r_sense_key: out parameter for sense_key
+ *
+ *	Perform ATAPI TEST_UNIT_READY.
+ *
+ *	LOCKING:
+ *	EH context (may sleep).
+ *
+ *	RETURNS:
+ *	0 on success, AC_ERR_* mask on failure.
+ */
+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;
+	unsigned int err_mask;
+
+	ata_tf_init(dev, &tf);
+
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.command = ATA_CMD_PACKET;
+	tf.protocol = ATAPI_PROT_NODATA;
+
+	err_mask = ata_exec_internal(dev, &tf, cdb, DMA_NONE, NULL, 0, 0);
+	if (err_mask == AC_ERR_DEV)
+		*r_sense_key = tf.feature >> 4;
+	return err_mask;
+}
+
+/**
+ *	atapi_eh_request_sense - perform ATAPI REQUEST_SENSE
+ *	@dev: device to perform REQUEST_SENSE to
+ *	@sense_buf: result sense data buffer (SCSI_SENSE_BUFFERSIZE bytes long)
+ *	@dfl_sense_key: default sense key to use
+ *
+ *	Perform ATAPI REQUEST_SENSE after the device reported CHECK
+ *	SENSE.  This function is EH helper.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep).
+ *
+ *	RETURNS:
+ *	0 on success, AC_ERR_* mask on failure
+ */
+unsigned int atapi_eh_request_sense(struct ata_device *dev,
+				    u8 *sense_buf, u8 dfl_sense_key)
+{
+	u8 cdb[ATAPI_CDB_LEN] =	{
+		REQUEST_SENSE, 0, 0, 0, SCSI_SENSE_BUFFERSIZE, 0 };
+	struct ata_port *ap = dev->link->ap;
+	struct ata_taskfile tf;
+
+	DPRINTK("ATAPI request sense\n");
+
+	/* FIXME: is this needed? */
+	memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
+
+	/* initialize sense_buf with the error register,
+	 * for the case where they are -not- overwritten
+	 */
+	sense_buf[0] = 0x70;
+	sense_buf[2] = dfl_sense_key;
+
+	/* some devices time out if garbage left in tf */
+	ata_tf_init(dev, &tf);
+
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.command = ATA_CMD_PACKET;
+
+	/* is it pointless to prefer PIO for "safety reasons"? */
+	if (ap->flags & ATA_FLAG_PIO_DMA) {
+		tf.protocol = ATAPI_PROT_DMA;
+		tf.feature |= ATAPI_PKT_DMA;
+	} else {
+		tf.protocol = ATAPI_PROT_PIO;
+		tf.lbam = SCSI_SENSE_BUFFERSIZE;
+		tf.lbah = 0;
+	}
+
+	return ata_exec_internal(dev, &tf, cdb, DMA_FROM_DEVICE,
+				 sense_buf, SCSI_SENSE_BUFFERSIZE, 0);
+}
diff --git a/drivers/ata/libata-atapi.h b/drivers/ata/libata-atapi.h
new file mode 100644
index 0000000..4d2e441
--- /dev/null
+++ b/drivers/ata/libata-atapi.h
@@ -0,0 +1,7 @@
+#ifndef __LIBATA_ATAPI_H__
+#define __LIBATA_ATAPI_H__
+
+unsigned int atapi_eh_tur(struct ata_device *dev, u8 *r_sense_key);
+unsigned int atapi_eh_request_sense(struct ata_device *dev, u8 *sense_buf, u8 dfl_sense_key);
+
+#endif
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 2659894..c2f2357 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -47,6 +47,7 @@
 #include <linux/libata.h>
 
 #include "libata.h"
+#include "libata-atapi.h"
 
 enum {
 	/* speed down verdicts */
@@ -1579,91 +1580,6 @@ static int ata_eh_read_log_10h(struct ata_device *dev,
 }
 
 /**
- *	atapi_eh_tur - perform ATAPI TEST_UNIT_READY
- *	@dev: target ATAPI device
- *	@r_sense_key: out parameter for sense_key
- *
- *	Perform ATAPI TEST_UNIT_READY.
- *
- *	LOCKING:
- *	EH context (may sleep).
- *
- *	RETURNS:
- *	0 on success, AC_ERR_* mask on failure.
- */
-static 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;
-	unsigned int err_mask;
-
-	ata_tf_init(dev, &tf);
-
-	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
-	tf.command = ATA_CMD_PACKET;
-	tf.protocol = ATAPI_PROT_NODATA;
-
-	err_mask = ata_exec_internal(dev, &tf, cdb, DMA_NONE, NULL, 0, 0);
-	if (err_mask == AC_ERR_DEV)
-		*r_sense_key = tf.feature >> 4;
-	return err_mask;
-}
-
-/**
- *	atapi_eh_request_sense - perform ATAPI REQUEST_SENSE
- *	@dev: device to perform REQUEST_SENSE to
- *	@sense_buf: result sense data buffer (SCSI_SENSE_BUFFERSIZE bytes long)
- *	@dfl_sense_key: default sense key to use
- *
- *	Perform ATAPI REQUEST_SENSE after the device reported CHECK
- *	SENSE.  This function is EH helper.
- *
- *	LOCKING:
- *	Kernel thread context (may sleep).
- *
- *	RETURNS:
- *	0 on success, AC_ERR_* mask on failure
- */
-static unsigned int atapi_eh_request_sense(struct ata_device *dev,
-					   u8 *sense_buf, u8 dfl_sense_key)
-{
-	u8 cdb[ATAPI_CDB_LEN] =
-		{ REQUEST_SENSE, 0, 0, 0, SCSI_SENSE_BUFFERSIZE, 0 };
-	struct ata_port *ap = dev->link->ap;
-	struct ata_taskfile tf;
-
-	DPRINTK("ATAPI request sense\n");
-
-	/* FIXME: is this needed? */
-	memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
-
-	/* initialize sense_buf with the error register,
-	 * for the case where they are -not- overwritten
-	 */
-	sense_buf[0] = 0x70;
-	sense_buf[2] = dfl_sense_key;
-
-	/* some devices time out if garbage left in tf */
-	ata_tf_init(dev, &tf);
-
-	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
-	tf.command = ATA_CMD_PACKET;
-
-	/* is it pointless to prefer PIO for "safety reasons"? */
-	if (ap->flags & ATA_FLAG_PIO_DMA) {
-		tf.protocol = ATAPI_PROT_DMA;
-		tf.feature |= ATAPI_PKT_DMA;
-	} else {
-		tf.protocol = ATAPI_PROT_PIO;
-		tf.lbam = SCSI_SENSE_BUFFERSIZE;
-		tf.lbah = 0;
-	}
-
-	return ata_exec_internal(dev, &tf, cdb, DMA_FROM_DEVICE,
-				 sense_buf, SCSI_SENSE_BUFFERSIZE, 0);
-}
-
-/**
  *	ata_eh_analyze_serror - analyze SError for a failed port
  *	@link: ATA link to analyze SError for
  *
-- 
1.7.12.4

[PATCH v8 08/11] ata: zpodd: check zero power ready status

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

In this implementation, the zero power ready status is determined by
the following factors:
1 polled media status byte, and this info is recorded in status_ready
  field of zpodd structure;
2 sense code by issuing a TEST_UNIT_READY command after status_ready
  is found to be true.

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
final deciding factor. But since SCSI ODD driver sr will always poll the
ODD every 2 seconds, this information is readily available without any
much cost. So it is used as a hint: when we find zero power ready status
in the media status byte, we will see if it is really the case with the
sense code method. This way, we can avoid sending too many
TEST_UNIT_READY commands to the ODD.

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.

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-acpi.c |   8 +++-
 drivers/ata/libata-scsi.c |   5 ++
 drivers/ata/sata_zpodd.c  | 117 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/sata_zpodd.h  |   5 ++
 4 files changed, 134 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 3167fc2..5a78cb3 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -785,7 +785,13 @@ static int ata_acpi_push_id(struct ata_device *dev)
  */
 int ata_acpi_on_suspend(struct ata_port *ap)
 {
-	/* nada */
+	struct ata_device *dev;
+
+	ata_for_each_dev(dev, &ap->link, ENABLED) {
+		if (zpodd_dev_enabled(dev))
+			zpodd_check_zpready(dev);
+	}
+
 	return 0;
 }
 
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index e3bda07..ed3e4d3 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -53,6 +53,7 @@
 
 #include "libata.h"
 #include "libata-transport.h"
+#include "sata_zpodd.h"
 
 #define ATA_SCSI_RBUF_SIZE	4096
 
@@ -2665,6 +2666,10 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
 			ata_scsi_rbuf_put(cmd, true, &flags);
 		}
 
+		if (zpodd_dev_enabled(qc->dev) &&
+				scsicmd[0] == GET_EVENT_STATUS_NOTIFICATION)
+			zpodd_snoop_status(qc->dev, cmd);
+
 		cmd->result = SAM_STAT_GOOD;
 	}
 
diff --git a/drivers/ata/sata_zpodd.c b/drivers/ata/sata_zpodd.c
index c7c63a1..0ec62f3 100644
--- a/drivers/ata/sata_zpodd.c
+++ b/drivers/ata/sata_zpodd.c
@@ -2,13 +2,22 @@
 #include <linux/pm_runtime.h>
 #include <linux/cdrom.h>
 #include <scsi/scsi_device.h>
+#include <scsi/scsi_cmnd.h>
 
 #include "libata.h"
+#include "libata-atapi.h"
+
+#define POWEROFF_DELAY  (30 * 1000)     /* 30 seconds for power off delay */
 
 struct zpodd {
 	bool slot:1;
 	bool drawer:1;
 	bool from_notify:1;	/* resumed as a result of acpi notification */
+	bool status_ready:1;	/* ready status derived from media event poll,
+				   it is not accurate, but serves as a hint */
+	bool zp_ready:1;	/* zero power ready state */
+
+	unsigned long last_ready; /* last zero power ready timestamp */
 
 	struct ata_device *dev;
 };
@@ -84,6 +93,114 @@ static int check_loading_mechanism(struct ata_device *dev)
 	return ret;
 }
 
+/*
+ * Snoop the result of GET_STATUS_NOTIFICATION_EVENT, the media
+ * status byte has information on media present/door closed.
+ *
+ * This information serves only as a hint, as it is not accurate.
+ * The sense code method will be used when deciding if the ODD is
+ * really zero power ready.
+ */
+void zpodd_snoop_status(struct ata_device *dev, struct scsi_cmnd *scmd)
+{
+	bool ready;
+	char buf[8];
+	struct event_header *eh = (void *)buf;
+	struct media_event_desc *med = (void *)(buf + 4);
+	struct sg_table *table = &scmd->sdb.table;
+	struct zpodd *zpodd = dev->private_data;
+
+	if (sg_copy_to_buffer(table->sgl, table->nents, buf, 8) != 8)
+		return;
+
+	if (be16_to_cpu(eh->data_len) < sizeof(*med))
+		return;
+
+	if (eh->nea || eh->notification_class != 0x4)
+		return;
+
+	if (zpodd->slot)
+		ready = !med->media_present;
+	else
+		ready = !(med->media_present || med->door_open);
+
+	zpodd->status_ready = ready;
+}
+
+/* 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->private_data;
+
+	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 ODD's zero power ready status.
+ *
+ * This function is called during ATA port's suspend path,
+ * when the port is not frozen yet, so that we can still make
+ * some IO to the ODD to decide if it is zero power ready.
+ *
+ * The ODD is regarded as zero power ready when it is in zero
+ * power ready state for some time(defined by POWEROFF_DELAY).
+ */
+void zpodd_check_zpready(struct ata_device *dev)
+{
+	bool zp_ready;
+	unsigned long expires;
+	struct zpodd *zpodd = dev->private_data;
+
+	if (!zpodd->status_ready) {
+		zpodd->last_ready = 0;
+		return;
+	}
+
+	if (!zpodd->last_ready) {
+		zp_ready = zpready(dev);
+		if (zp_ready)
+			zpodd->last_ready = jiffies;
+		return;
+	}
+
+	expires = zpodd->last_ready + msecs_to_jiffies(POWEROFF_DELAY);
+	if (time_before(jiffies, expires))
+		return;
+
+	zpodd->zp_ready = zpready(dev);
+	if (!zpodd->zp_ready)
+		zpodd->last_ready = 0;
+}
+
 static void zpodd_wake_dev(acpi_handle handle, u32 event, void *context)
 {
 	struct ata_device *ata_dev = context;
diff --git a/drivers/ata/sata_zpodd.h b/drivers/ata/sata_zpodd.h
index e320c6f..03d6b4d 100644
--- a/drivers/ata/sata_zpodd.h
+++ b/drivers/ata/sata_zpodd.h
@@ -2,6 +2,7 @@
 #define __SATA_ZPODD_H__
 
 #include <linux/libata.h>
+#include <scsi/scsi_cmnd.h>
 
 #ifdef CONFIG_SATA_ZPODD
 void zpodd_init(struct ata_device *);
@@ -15,10 +16,14 @@ static bool zpodd_dev_enabled(struct ata_device *dev)
 		return false;
 }
 
+void zpodd_snoop_status(struct ata_device *, struct scsi_cmnd *);
+void zpodd_check_zpready(struct ata_device *);
 #else
 static inline void zpodd_init(struct ata_device *dev) {}
 static inline void zpodd_deinit(struct ata_device *dev) {}
 static inline bool zpodd_dev_enabled(struct ata_device *dev) { return false; }
+static inline void zpodd_snoop_status(struct ata_device *dev, struct scsi_cmnd *cmd) {}
+static inline void zpodd_check_zpready(struct ata_device *dev) {}
 #endif
 
 #endif
-- 
1.7.12.4

[PATCH v8 09/11] block: add a new interface to block events

[Aaron Lu]

A new interface to block disk events is added, this interface is
meant to eliminate a race between PM runtime callback and disk events
checking.

Suppose the following device tree:
device_sata_port  (the parent)
  device_ODD      (the child)

When ODD is runtime suspended, sata port will have a chance to enter
runtime suspended state. And in sata port's runtime suspend callback,
it will check if it is OK to omit the power of the ODD. And if yes, the
periodically running events checking work will be stopped, as the ODD
will be waken up by that check and cancel it can make the ODD stay in
zero power state much longer(no worry about how the ODD gets media
change event in ZPODD's case).

I used disk_block_events to do the events blocking, but there is a race
and can lead to a deadlock: when I call disk_block_events in sata port's
runtime suspend callback, the events checking work may already be running
and it will resume the ODD synchronously, and PM core will try to resume
its parent, the sata port first. The PM core makes sure that runtime
resume callback does not run concurrently with runtime suspend callback,
and if the runtime suspend callback is running, the runtime resume
callback will wait for it done. So the events checking work will wait
for sata port's runtime suspend callback, while the sata port's runtime
suspend callback is waiting for the disk events work to finish. Deadlock.

ODD_suspend                        disk_events_workfn
  ata_port_suspend                   check_events
    disk_block_events                  resume ODD
      cancel_delayed_work_sync           resume parent
      (waiting for disk_events_workfn)   (waiting for suspend callback)

So a new function disk_try_block_events is added, it will try to
cancel the delayed work if the work is currently pending, which means
it has been queued but the timer is not expired yet. If the work is not
pending, we return false, as we do not know if the task is already
running or not queued at all. And if the queued work is successfully
cancelled, disk_block_events will be called and we are done. But if we
failed to cancel the work, false will be returned, and the calling
thread knows that it's not safe to block events at the moment and should
act accordingly. In ZPODD's case, poweroff will not be attempted.

Note:
ODD means: Optical Disc Drive.
ZPODD means: Zero Power ODD. It's a mechnism to place the ODD into zero
power state without user's awareness when the system is at S0 system
state.

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

diff --git a/block/genhd.c b/block/genhd.c
index cac7366..c6af755 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1469,6 +1469,32 @@ void disk_block_events(struct gendisk *disk)
 	mutex_unlock(&ev->block_mutex);
 }
 
+/*
+ * Under some circumstances, there is a race between the calling thread
+ * of disk_block_events and the events checking function. To avoid such a race,
+ * this function will check if the delayed work is pending. If not, it means
+ * the work is either not queued or is already running, false is returned.
+ * And if yes, try to cancel the delayed work. If succedded, disk_block_events
+ * will be called and there is no worry that cancel_delayed_work_sync will
+ * deadlock the events checking function. And if failed, false is returned.
+ */
+bool disk_try_block_events(struct gendisk *disk)
+{
+	struct disk_events *ev = disk->ev;
+
+	if (!ev)
+		return false;
+
+	if (delayed_work_pending(&ev->dwork)) {
+		if (cancel_delayed_work(&disk->ev->dwork)) {
+			disk_block_events(disk);
+			return true;
+		}
+	}
+
+	return false;
+}
+
 static void __disk_unblock_events(struct gendisk *disk, bool check_now)
 {
 	struct disk_events *ev = disk->ev;
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 4f440b3..b67247f 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -420,6 +420,7 @@ static inline int get_disk_ro(struct gendisk *disk)
 }
 
 extern void disk_block_events(struct gendisk *disk);
+extern bool disk_try_block_events(struct gendisk *disk);
 extern void disk_unblock_events(struct gendisk *disk);
 extern void disk_flush_events(struct gendisk *disk, unsigned int mask);
 extern unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask);
-- 
1.7.12.4

[PATCH v8 10/11] scsi: sr: support (un)block events

[Aaron Lu]

2 interfaces are added to block/unblock events for the disk sr manages.
This is used by SATA ZPODD, when ODD is runtime powered off, the events
poll is no longer needed so better be blocked. And once powered on,
events poll will be unblocked.

These 2 interfaces are needed here because SATA layer does not have
access to the gendisk structure sr manages.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/scsi/Makefile   |  1 +
 drivers/scsi/sr_zpodd.c | 21 +++++++++++++++++++++
 drivers/scsi/sr_zpodd.h |  9 +++++++++
 3 files changed, 31 insertions(+)
 create mode 100644 drivers/scsi/sr_zpodd.c
 create mode 100644 drivers/scsi/sr_zpodd.h

diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index 888f73a..474efe2 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -177,6 +177,7 @@ sd_mod-objs	:= sd.o
 sd_mod-$(CONFIG_BLK_DEV_INTEGRITY) += sd_dif.o
 
 sr_mod-objs	:= sr.o sr_ioctl.o sr_vendor.o
+sr_mod-$(CONFIG_SATA_ZPODD) += sr_zpodd.o
 ncr53c8xx-flags-$(CONFIG_SCSI_ZALON) \
 		:= -DCONFIG_NCR53C8XX_PREFETCH -DSCSI_NCR_BIG_ENDIAN \
 			-DCONFIG_SCSI_NCR53C8XX_NO_WORD_TRANSFERS
diff --git a/drivers/scsi/sr_zpodd.c b/drivers/scsi/sr_zpodd.c
new file mode 100644
index 0000000..0686e8c
--- /dev/null
+++ b/drivers/scsi/sr_zpodd.c
@@ -0,0 +1,21 @@
+#include <linux/module.h>
+#include <linux/genhd.h>
+#include <linux/cdrom.h>
+#include "sr.h"
+
+bool sr_block_events(struct device *dev)
+{
+	struct scsi_cd *cd = dev_get_drvdata(dev);
+	if (cd)
+		return disk_try_block_events(cd->disk);
+	return false;
+}
+EXPORT_SYMBOL(sr_block_events);
+
+void sr_unblock_events(struct device *dev)
+{
+	struct scsi_cd *cd = dev_get_drvdata(dev);
+	if (cd)
+		disk_unblock_events(cd->disk);
+}
+EXPORT_SYMBOL(sr_unblock_events);
diff --git a/drivers/scsi/sr_zpodd.h b/drivers/scsi/sr_zpodd.h
new file mode 100644
index 0000000..49c6a55
--- /dev/null
+++ b/drivers/scsi/sr_zpodd.h
@@ -0,0 +1,9 @@
+#ifndef __SR_ZPODD_H__
+#define __SR_ZPODD_H__
+
+#include <linux/device.h>
+
+extern bool sr_block_events(struct device *dev);
+extern void sr_unblock_events(struct device *dev);
+
+#endif
-- 
1.7.12.4

[PATCH v8 11/11] ata: zpodd: handle power transition of ODD

[Aaron Lu]

The implementation of ZPODD aligns with runtime pm, which means the ODD
will only be runtime powered off after it is runtime suspended.

And each time in ata port's suspend callback, a check is made to see if
the ODD is ready to be powered off:
1 zp_ready is true;
2 events is successfully blocked.

If the 2 conditions are not met, ACPI_STATE_D0 is chosen since as far
as ACPI is concerned about ODD's power management, only D0 and D3 is
meanningful. There is no definition of D1/D2/D3hot state for ODD, so
avoid setting those ACPI states.

And on resume, it will re-gain power and go through the recovery
process. When EH 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 | 34 +++++++++++++++++++-------
 drivers/ata/libata-core.c | 20 ++++++++++++++-
 drivers/ata/sata_zpodd.c  | 62 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/sata_zpodd.h  |  8 ++++++
 4 files changed, 114 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 5a78cb3..a8c9199 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -839,6 +839,24 @@ void ata_acpi_on_resume(struct ata_port *ap)
 	}
 }
 
+static int ata_acpi_choose_state(struct ata_device *dev)
+{
+	int acpi_state;
+
+	/* Always choose D3 for PATA devices */
+	if (!(dev->link->ap->flags & ATA_FLAG_ACPI_SATA))
+		return ACPI_STATE_D3;
+
+	acpi_state = acpi_pm_device_sleep_state(&dev->sdev->sdev_gendev,
+						NULL, ACPI_STATE_D3);
+
+	if (acpi_state == ACPI_STATE_D3 && zpodd_dev_enabled(dev) &&
+			!zpodd_poweroff_ready(dev))
+		acpi_state = ACPI_STATE_D0;
+
+	return acpi_state;
+}
+
 /**
  * ata_acpi_set_state - set the port power state
  * @ap: target ATA port
@@ -865,17 +883,15 @@ 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_state = ata_acpi_choose_state(dev);
+			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);
+				if (zpodd_dev_enabled(dev))
+					zpodd_post_poweroff(dev);
+			}
 		} 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-core.c b/drivers/ata/libata-core.c
index 95fb7b8..d3be1b6 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -70,6 +70,7 @@
 
 #include "libata.h"
 #include "libata-transport.h"
+#include "sata_zpodd.h"
 
 /* debounce timing parameters in msecs { interval, duration, timeout } */
 const unsigned long sata_deb_timing_normal[]		= {   5,  100, 2000 };
@@ -5399,6 +5400,23 @@ static int ata_port_runtime_idle(struct device *dev)
 	return pm_runtime_suspend(dev);
 }
 
+static int ata_port_runtime_resume(struct device *dev)
+{
+	int rc;
+	struct ata_device *ata_dev;
+	struct ata_port *ap = to_ata_port(dev);
+
+	rc = ata_port_resume_common(dev);
+	if (!rc) {
+		ata_for_each_dev(ata_dev, &ap->link, ENABLED) {
+			if (zpodd_dev_enabled(ata_dev))
+				zpodd_post_resume(ata_dev);
+		}
+	}
+
+	return rc;
+}
+
 static const struct dev_pm_ops ata_port_pm_ops = {
 	.suspend = ata_port_suspend,
 	.resume = ata_port_resume,
@@ -5408,7 +5426,7 @@ static const struct dev_pm_ops ata_port_pm_ops = {
 	.restore = ata_port_resume,
 
 	.runtime_suspend = ata_port_suspend,
-	.runtime_resume = ata_port_resume_common,
+	.runtime_resume = ata_port_runtime_resume,
 	.runtime_idle = ata_port_runtime_idle,
 };
 
diff --git a/drivers/ata/sata_zpodd.c b/drivers/ata/sata_zpodd.c
index 0ec62f3..aca2553 100644
--- a/drivers/ata/sata_zpodd.c
+++ b/drivers/ata/sata_zpodd.c
@@ -6,6 +6,7 @@
 
 #include "libata.h"
 #include "libata-atapi.h"
+#include "../scsi/sr_zpodd.h"
 
 #define POWEROFF_DELAY  (30 * 1000)     /* 30 seconds for power off delay */
 
@@ -16,6 +17,7 @@ struct zpodd {
 	bool status_ready:1;	/* ready status derived from media event poll,
 				   it is not accurate, but serves as a hint */
 	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 */
 
@@ -46,6 +48,17 @@ static int zpodd_run_atapi_cmd(struct ata_device *dev, const char *cdb,
 	return ret;
 }
 
+static int zpodd_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 zpodd_run_atapi_cmd(dev, cdb, sizeof(cdb), NULL, 0);
+}
+
 /*
  * Per the spec, only slot type and drawer type ODD can be supported
  *
@@ -201,6 +214,55 @@ void zpodd_check_zpready(struct ata_device *dev)
 		zpodd->last_ready = 0;
 }
 
+/*
+ * Test if ODD is ready to be powered off.
+ * Determined by zp_ready and if events is successfully blocked
+ */
+bool zpodd_poweroff_ready(struct ata_device *dev)
+{
+	struct zpodd *zpodd = dev->private_data;
+
+	if (zpodd->zp_ready && sr_block_events(&dev->sdev->sdev_gendev))
+		return true;
+	else
+		return false;
+}
+
+void zpodd_post_poweroff(struct ata_device *dev)
+{
+	struct zpodd *zpodd = dev->private_data;
+
+	zpodd->powered_off = true;
+	acpi_pm_device_run_wake(&dev->sdev->sdev_gendev, true);
+}
+
+void zpodd_pre_poweron(struct ata_device *dev)
+{
+	struct zpodd *zpodd = dev->private_data;
+	if (zpodd->powered_off)
+		acpi_pm_device_run_wake(&dev->sdev->sdev_gendev, false);
+}
+
+void zpodd_post_resume(struct ata_device *dev)
+{
+	struct zpodd *zpodd = dev->private_data;
+
+	if (!zpodd->powered_off)
+		return;
+
+	zpodd->powered_off = false;
+
+	if (zpodd->from_notify) {
+		zpodd->from_notify = false;
+		if (zpodd->drawer)
+			zpodd_eject_tray(dev);
+	}
+
+	zpodd->last_ready = 0;
+	zpodd->zp_ready = false;
+	sr_unblock_events(&dev->sdev->sdev_gendev);
+}
+
 static void zpodd_wake_dev(acpi_handle handle, u32 event, void *context)
 {
 	struct ata_device *ata_dev = context;
diff --git a/drivers/ata/sata_zpodd.h b/drivers/ata/sata_zpodd.h
index 03d6b4d..87f6f53 100644
--- a/drivers/ata/sata_zpodd.h
+++ b/drivers/ata/sata_zpodd.h
@@ -18,12 +18,20 @@ static bool zpodd_dev_enabled(struct ata_device *dev)
 
 void zpodd_snoop_status(struct ata_device *, struct scsi_cmnd *);
 void zpodd_check_zpready(struct ata_device *);
+bool zpodd_poweroff_ready(struct ata_device *);
+void zpodd_post_poweroff(struct ata_device *);
+void zpodd_pre_poweron(struct ata_device *);
+void zpodd_post_resume(struct ata_device *);
 #else
 static inline void zpodd_init(struct ata_device *dev) {}
 static inline void zpodd_deinit(struct ata_device *dev) {}
 static inline bool zpodd_dev_enabled(struct ata_device *dev) { return false; }
 static inline void zpodd_snoop_status(struct ata_device *dev, struct scsi_cmnd *cmd) {}
 static inline void zpodd_check_zpready(struct ata_device *dev) {}
+static inline bool zpodd_poweroff_ready(struct ata_device *dev) { return false; }
+static inline void zpodd_post_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
 
 #endif
-- 
1.7.12.4

Re: [PATCH v8 05/11] libata-eh: allow defer in ata_exec_internal

[Tejun Heo]

On Mon, Oct 29, 2012 at 05:01:32PM +0800, Aaron Lu wrote:
> ata_exec_internal will preempt the ata link's active_tag and ata port's
> qc_active flags, this is OK for error recovery, but if normal code path
> wants to use ata_exec_internal, there is a problem: we need to check if
> it is OK to issue a new command with the help of port_ops->defer.
> 
> In ZPODD, I'll need to find out the loading mechanism of the ODD by
> issuing a GET_CONFIGURATION command. And this command may very well
> race with commands issued from SCSI layer. So instead of preempt the
> current command, defer the new command if it's not OK to issue it, as
> it is always wrong to issue a non-NCQ command when there is command(s)
> in processing.

Why not do the discovery from EH?

Thanks.

-- 
tejun

Re: [PATCH v8 09/11] block: add a new interface to block events

[Tejun Heo]

Hello,

On Mon, Oct 29, 2012 at 05:01:36PM +0800, Aaron Lu wrote:
> ODD_suspend                        disk_events_workfn
>   ata_port_suspend                   check_events
>     disk_block_events                  resume ODD
>       cancel_delayed_work_sync           resume parent
>       (waiting for disk_events_workfn)   (waiting for suspend callback)

I don't understand why solving it needs to be this elaborate.
check_event() can retry.  Just add a per-sr mutex which is try-locked
by sr_block_check_events() and grab it when entering zero power.

> +/*
> + * Under some circumstances, there is a race between the calling thread
> + * of disk_block_events and the events checking function. To avoid such a race,
> + * this function will check if the delayed work is pending. If not, it means
> + * the work is either not queued or is already running, false is returned.
> + * And if yes, try to cancel the delayed work. If succedded, disk_block_events
> + * will be called and there is no worry that cancel_delayed_work_sync will
> + * deadlock the events checking function. And if failed, false is returned.
> + */
> +bool disk_try_block_events(struct gendisk *disk)
> +{
> +	struct disk_events *ev = disk->ev;
> +
> +	if (!ev)
> +		return false;
> +
> +	if (delayed_work_pending(&ev->dwork)) {

And please don't use delayed_work_pending() like this.  It doesn't add
anything.  cancel_delayed_work() already needs to perform all the
necessary tests.

> +		if (cancel_delayed_work(&disk->ev->dwork)) {
> +			disk_block_events(disk);
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}

Thanks.

-- 
tejun

Re: [PATCH v8 02/11] ata: zpodd: Add CONFIG_SATA_ZPODD

[James Bottomley]

On Mon, 2012-10-29 at 17:01 +0800, Aaron Lu wrote:
> Added a new config CONFIG_SATA_ZPODD, which is ued to support
> SATA based zero power ODD. It depends on ACPI, and selects BLK_DEV_SR
> as the implementation of ZPODD depends on SCSI sr driver.
> 
> 2 new files are added, which will be used to host ZPODD related code.
> They are empty for this commit.

I think you actually need to combine this with patch 3 to have it
compile ...

James


> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  drivers/ata/Kconfig  | 12 ++++++++++++
>  drivers/ata/Makefile |  1 +
>  2 files changed, 13 insertions(+)
>  create mode 100644 drivers/ata/sata_zpodd.c
>  create mode 100644 drivers/ata/sata_zpodd.h
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index e08d322..2cdecee 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -58,6 +58,18 @@ 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 ACPI
> +	select BLK_DEV_SR
> +	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).
> +
>  config SATA_PMP
>  	bool "SATA Port Multiplier support"
>  	default y
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index 9329daf..a5120ff 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
> +obj-$(CONFIG_SATA_ZPODD)	+= sata_zpodd.o
> diff --git a/drivers/ata/sata_zpodd.c b/drivers/ata/sata_zpodd.c
> new file mode 100644
> index 0000000..e69de29
> diff --git a/drivers/ata/sata_zpodd.h b/drivers/ata/sata_zpodd.h
> new file mode 100644
> index 0000000..e69de29

Re: [PATCH v8 10/11] scsi: sr: support (un)block events

[James Bottomley]

On Mon, 2012-10-29 at 17:01 +0800, Aaron Lu wrote:
> 2 interfaces are added to block/unblock events for the disk sr manages.
> This is used by SATA ZPODD, when ODD is runtime powered off, the events
> poll is no longer needed so better be blocked. And once powered on,
> events poll will be unblocked.
> 
> These 2 interfaces are needed here because SATA layer does not have
> access to the gendisk structure sr manages.

I'm afraid this is a nasty layering violation.  You can't have a low
level driver have knowledge of a call back in an upper layer one (in
this case sr_block_events).

This is all done it looks like because of the problem of getting access
to the scsi_cd structure from libata, but it's not the right way to
solve it.

I also don't really think you need to do any blocking or unblocking.  As
I said in the previous thread, just fake the events the standard tells
you to, so userspace can probe to its heart's content and you can keep
the device powered off.

James


> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  drivers/scsi/Makefile   |  1 +
>  drivers/scsi/sr_zpodd.c | 21 +++++++++++++++++++++
>  drivers/scsi/sr_zpodd.h |  9 +++++++++
>  3 files changed, 31 insertions(+)
>  create mode 100644 drivers/scsi/sr_zpodd.c
>  create mode 100644 drivers/scsi/sr_zpodd.h
> 
> diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
> index 888f73a..474efe2 100644
> --- a/drivers/scsi/Makefile
> +++ b/drivers/scsi/Makefile
> @@ -177,6 +177,7 @@ sd_mod-objs	:= sd.o
>  sd_mod-$(CONFIG_BLK_DEV_INTEGRITY) += sd_dif.o
>  
>  sr_mod-objs	:= sr.o sr_ioctl.o sr_vendor.o
> +sr_mod-$(CONFIG_SATA_ZPODD) += sr_zpodd.o
>  ncr53c8xx-flags-$(CONFIG_SCSI_ZALON) \
>  		:= -DCONFIG_NCR53C8XX_PREFETCH -DSCSI_NCR_BIG_ENDIAN \
>  			-DCONFIG_SCSI_NCR53C8XX_NO_WORD_TRANSFERS
> diff --git a/drivers/scsi/sr_zpodd.c b/drivers/scsi/sr_zpodd.c
> new file mode 100644
> index 0000000..0686e8c
> --- /dev/null
> +++ b/drivers/scsi/sr_zpodd.c
> @@ -0,0 +1,21 @@
> +#include <linux/module.h>
> +#include <linux/genhd.h>
> +#include <linux/cdrom.h>
> +#include "sr.h"
> +
> +bool sr_block_events(struct device *dev)
> +{
> +	struct scsi_cd *cd = dev_get_drvdata(dev);
> +	if (cd)
> +		return disk_try_block_events(cd->disk);
> +	return false;
> +}
> +EXPORT_SYMBOL(sr_block_events);
> +
> +void sr_unblock_events(struct device *dev)
> +{
> +	struct scsi_cd *cd = dev_get_drvdata(dev);
> +	if (cd)
> +		disk_unblock_events(cd->disk);
> +}
> +EXPORT_SYMBOL(sr_unblock_events);
> diff --git a/drivers/scsi/sr_zpodd.h b/drivers/scsi/sr_zpodd.h
> new file mode 100644
> index 0000000..49c6a55
> --- /dev/null
> +++ b/drivers/scsi/sr_zpodd.h
> @@ -0,0 +1,9 @@
> +#ifndef __SR_ZPODD_H__
> +#define __SR_ZPODD_H__
> +
> +#include <linux/device.h>
> +
> +extern bool sr_block_events(struct device *dev);
> +extern void sr_unblock_events(struct device *dev);
> +
> +#endif

Re: [PATCH v8 10/11] scsi: sr: support (un)block events

[Alan Stern]

On Mon, 29 Oct 2012, James Bottomley wrote:

> On Mon, 2012-10-29 at 17:01 +0800, Aaron Lu wrote:
> > 2 interfaces are added to block/unblock events for the disk sr manages.
> > This is used by SATA ZPODD, when ODD is runtime powered off, the events
> > poll is no longer needed so better be blocked. And once powered on,
> > events poll will be unblocked.
> > 
> > These 2 interfaces are needed here because SATA layer does not have
> > access to the gendisk structure sr manages.
> 
> I'm afraid this is a nasty layering violation.  You can't have a low
> level driver have knowledge of a call back in an upper layer one (in
> this case sr_block_events).
> 
> This is all done it looks like because of the problem of getting access
> to the scsi_cd structure from libata, but it's not the right way to
> solve it.
> 
> I also don't really think you need to do any blocking or unblocking.  As
> I said in the previous thread, just fake the events the standard tells
> you to, so userspace can probe to its heart's content and you can keep
> the device powered off.

James, one of us has misunderstood the event-polling mechanism.  As far
as I know, the polling calls that Aaron is concerned with are generated
by the kernel, not by userspace.

Alan Stern

Re: [PATCH v8 05/11] libata-eh: allow defer in ata_exec_internal

[Aaron Lu]

On 10/29/2012 11:20 PM, Tejun Heo wrote:
> On Mon, Oct 29, 2012 at 05:01:32PM +0800, Aaron Lu wrote:
>> ata_exec_internal will preempt the ata link's active_tag and ata port's
>> qc_active flags, this is OK for error recovery, but if normal code path
>> wants to use ata_exec_internal, there is a problem: we need to check if
>> it is OK to issue a new command with the help of port_ops->defer.
>>
>> In ZPODD, I'll need to find out the loading mechanism of the ODD by
>> issuing a GET_CONFIGURATION command. And this command may very well
>> race with commands issued from SCSI layer. So instead of preempt the
>> current command, defer the new command if it's not OK to issue it, as
>> it is always wrong to issue a non-NCQ command when there is command(s)
>> in processing.
> 
> Why not do the discovery from EH?

Do you mean set a device level EH flag and then schedule EH to do the
discovery?

Thanks,
Aaron

Re: [PATCH v8 05/11] libata-eh: allow defer in ata_exec_internal

[Tejun Heo]

Hello,

On Tue, Oct 30, 2012 at 11:00:16AM +0800, Aaron Lu wrote:
> > Why not do the discovery from EH?
> 
> Do you mean set a device level EH flag and then schedule EH to do the
> discovery?

Yeah, something like that or why can't it be done while probing the
device in the first place?

Thanks.

-- 
tejun

Re: [PATCH v8 05/11] libata-eh: allow defer in ata_exec_internal

[Aaron Lu]

On 10/30/2012 11:01 AM, Tejun Heo wrote:
> Hello,
> 
> On Tue, Oct 30, 2012 at 11:00:16AM +0800, Aaron Lu wrote:
>>> Why not do the discovery from EH?
>>
>> Do you mean set a device level EH flag and then schedule EH to do the
>> discovery?
> 
> Yeah, something like that or why can't it be done while probing the
> device in the first place?

I'm not aware of a place to store such ODD specific information when
probing the device.

I'm currently storing the loading mech type in structure zpodd, which
gets created after the corresponding SCSI device gets created in
ata_scsi_scan_host, so at the probe time, the zpodd structure does not
exist yet. And the reason I create the zpodd sturcture this late is
that, it is only created when the ODD together the platform is ZPODD
capable, and to find out if this platform is ZPODD capbale, ACPI binding
has to occur first, and ACPI binding happens when SCSI device is added
to the device tree.

Thanks,
Aaron

Re: [PATCH v8 02/11] ata: zpodd: Add CONFIG_SATA_ZPODD

[Aaron Lu]

On 10/30/2012 02:11 AM, James Bottomley wrote:
> On Mon, 2012-10-29 at 17:01 +0800, Aaron Lu wrote:
>> Added a new config CONFIG_SATA_ZPODD, which is ued to support
>> SATA based zero power ODD. It depends on ACPI, and selects BLK_DEV_SR
>> as the implementation of ZPODD depends on SCSI sr driver.
>>
>> 2 new files are added, which will be used to host ZPODD related code.
>> They are empty for this commit.
> 
> I think you actually need to combine this with patch 3 to have it
> compile ...

I remembered I tested this, and I just tested the compile again, with
the CONFIG_SATA_ZPODD both on and off, and it didn't have any compile
issue, so I'm not sure if this is a problem...

-Aaron

> 
> James
> 
> 
>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
>> ---
>>  drivers/ata/Kconfig  | 12 ++++++++++++
>>  drivers/ata/Makefile |  1 +
>>  2 files changed, 13 insertions(+)
>>  create mode 100644 drivers/ata/sata_zpodd.c
>>  create mode 100644 drivers/ata/sata_zpodd.h
>>
>> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
>> index e08d322..2cdecee 100644
>> --- a/drivers/ata/Kconfig
>> +++ b/drivers/ata/Kconfig
>> @@ -58,6 +58,18 @@ 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 ACPI
>> +	select BLK_DEV_SR
>> +	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).
>> +
>>  config SATA_PMP
>>  	bool "SATA Port Multiplier support"
>>  	default y
>> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
>> index 9329daf..a5120ff 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
>> +obj-$(CONFIG_SATA_ZPODD)	+= sata_zpodd.o
>> diff --git a/drivers/ata/sata_zpodd.c b/drivers/ata/sata_zpodd.c
>> new file mode 100644
>> index 0000000..e69de29
>> diff --git a/drivers/ata/sata_zpodd.h b/drivers/ata/sata_zpodd.h
>> new file mode 100644
>> index 0000000..e69de29
> 
> 

Re: [PATCH v8 10/11] scsi: sr: support (un)block events

[James Bottomley]

On Mon, 2012-10-29 at 18:22 -0400, Alan Stern wrote:
> On Mon, 29 Oct 2012, James Bottomley wrote:
> 
> > On Mon, 2012-10-29 at 17:01 +0800, Aaron Lu wrote:
> > > 2 interfaces are added to block/unblock events for the disk sr manages.
> > > This is used by SATA ZPODD, when ODD is runtime powered off, the events
> > > poll is no longer needed so better be blocked. And once powered on,
> > > events poll will be unblocked.
> > > 
> > > These 2 interfaces are needed here because SATA layer does not have
> > > access to the gendisk structure sr manages.
> > 
> > I'm afraid this is a nasty layering violation.  You can't have a low
> > level driver have knowledge of a call back in an upper layer one (in
> > this case sr_block_events).
> > 
> > This is all done it looks like because of the problem of getting access
> > to the scsi_cd structure from libata, but it's not the right way to
> > solve it.
> > 
> > I also don't really think you need to do any blocking or unblocking.  As
> > I said in the previous thread, just fake the events the standard tells
> > you to, so userspace can probe to its heart's content and you can keep
> > the device powered off.
> 
> James, one of us has misunderstood the event-polling mechanism.  As far
> as I know, the polling calls that Aaron is concerned with are generated
> by the kernel, not by userspace.

I don't think the origin of the poll changes anything in what I said
above, does it?

James

Re: [PATCH v8 10/11] scsi: sr: support (un)block events

[Aaron Lu]

On 10/30/2012 02:11 AM, James Bottomley wrote:
> On Mon, 2012-10-29 at 17:01 +0800, Aaron Lu wrote:
>> 2 interfaces are added to block/unblock events for the disk sr manages.
>> This is used by SATA ZPODD, when ODD is runtime powered off, the events
>> poll is no longer needed so better be blocked. And once powered on,
>> events poll will be unblocked.
>>
>> These 2 interfaces are needed here because SATA layer does not have
>> access to the gendisk structure sr manages.
> 
> I'm afraid this is a nasty layering violation.  You can't have a low
> level driver have knowledge of a call back in an upper layer one (in
> this case sr_block_events).

I agree. But since ZPODD is now implemented in ATA layer now, this seems
the only way to do it.

> 
> This is all done it looks like because of the problem of getting access
> to the scsi_cd structure from libata, but it's not the right way to
> solve it.

Indeed, and any suggestion on a right way?

> 
> I also don't really think you need to do any blocking or unblocking.  As
> I said in the previous thread, just fake the events the standard tells
> you to, so userspace can probe to its heart's content and you can keep
> the device powered off.

No problem for the device, the problem is for its ancestor devices in my
current implementation. If the event is not blocked, the ODD will be
runtime resumed every 2 seconds. And along the way, all its ancestor
devices will have to be runtime resumed every 2 seconds, even I did some
trick to avoid power on the ODD itself. This is not a good thing from
power management perspective.

And there are two reasons I want to align runtime PM with ZPODD:

1 Simpler code.
And if I do not align ZPODD with runtime PM, I will need to maintain the
ODD's software status myself, and deal with some race conditions like
when I was to power off the ODD, a new command may arrive; or when the
ODD is powering on, all arriving commands need to be failed for the
moment untill the ODD is fully recovered from EH, etc. And all these
things can be avoided if I rely on runtime PM framework.

2 Integrate with the whole system
With runtime PM, when ODD is suspended, its ancestor devices will have a
chance to enter low power state.

Basically, I think power off is a special suspend state, it should
happen after the device enters runtime suspended state, and this is the
idea I used to implement ZPODD.

Thanks,
Aaron

> 
> James
> 
> 
>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
>> ---
>>  drivers/scsi/Makefile   |  1 +
>>  drivers/scsi/sr_zpodd.c | 21 +++++++++++++++++++++
>>  drivers/scsi/sr_zpodd.h |  9 +++++++++
>>  3 files changed, 31 insertions(+)
>>  create mode 100644 drivers/scsi/sr_zpodd.c
>>  create mode 100644 drivers/scsi/sr_zpodd.h
>>
>> diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
>> index 888f73a..474efe2 100644
>> --- a/drivers/scsi/Makefile
>> +++ b/drivers/scsi/Makefile
>> @@ -177,6 +177,7 @@ sd_mod-objs	:= sd.o
>>  sd_mod-$(CONFIG_BLK_DEV_INTEGRITY) += sd_dif.o
>>  
>>  sr_mod-objs	:= sr.o sr_ioctl.o sr_vendor.o
>> +sr_mod-$(CONFIG_SATA_ZPODD) += sr_zpodd.o
>>  ncr53c8xx-flags-$(CONFIG_SCSI_ZALON) \
>>  		:= -DCONFIG_NCR53C8XX_PREFETCH -DSCSI_NCR_BIG_ENDIAN \
>>  			-DCONFIG_SCSI_NCR53C8XX_NO_WORD_TRANSFERS
>> diff --git a/drivers/scsi/sr_zpodd.c b/drivers/scsi/sr_zpodd.c
>> new file mode 100644
>> index 0000000..0686e8c
>> --- /dev/null
>> +++ b/drivers/scsi/sr_zpodd.c
>> @@ -0,0 +1,21 @@
>> +#include <linux/module.h>
>> +#include <linux/genhd.h>
>> +#include <linux/cdrom.h>
>> +#include "sr.h"
>> +
>> +bool sr_block_events(struct device *dev)
>> +{
>> +	struct scsi_cd *cd = dev_get_drvdata(dev);
>> +	if (cd)
>> +		return disk_try_block_events(cd->disk);
>> +	return false;
>> +}
>> +EXPORT_SYMBOL(sr_block_events);
>> +
>> +void sr_unblock_events(struct device *dev)
>> +{
>> +	struct scsi_cd *cd = dev_get_drvdata(dev);
>> +	if (cd)
>> +		disk_unblock_events(cd->disk);
>> +}
>> +EXPORT_SYMBOL(sr_unblock_events);
>> diff --git a/drivers/scsi/sr_zpodd.h b/drivers/scsi/sr_zpodd.h
>> new file mode 100644
>> index 0000000..49c6a55
>> --- /dev/null
>> +++ b/drivers/scsi/sr_zpodd.h
>> @@ -0,0 +1,9 @@
>> +#ifndef __SR_ZPODD_H__
>> +#define __SR_ZPODD_H__
>> +
>> +#include <linux/device.h>
>> +
>> +extern bool sr_block_events(struct device *dev);
>> +extern void sr_unblock_events(struct device *dev);
>> +
>> +#endif
> 
> 

Re: [PATCH v8 09/11] block: add a new interface to block events

[Aaron Lu]

On 10/29/2012 11:35 PM, Tejun Heo wrote:
> Hello,
> 
> On Mon, Oct 29, 2012 at 05:01:36PM +0800, Aaron Lu wrote:
>> ODD_suspend                        disk_events_workfn
>>   ata_port_suspend                   check_events
>>     disk_block_events                  resume ODD
>>       cancel_delayed_work_sync           resume parent
>>       (waiting for disk_events_workfn)   (waiting for suspend callback)
> 
> I don't understand why solving it needs to be this elaborate.
							:-)

> check_event() can retry.  Just add a per-sr mutex which is try-locked
> by sr_block_check_events() and grab it when entering zero power.

Good suggestion. I didn't think about solving it this way.

Many people suggest me that ZPODD is pure SATA/ACPI stuff, and should
not pollute sr driver, so I was trying hard not to touch sr while
preparing these patches, unless there is no other choice(like the
blocking event interface).

So I'm not sure if your suggestion is the way to go.

James, what do you think? Is it OK if I add a mutex into the scsi_cd
structure to do this? Of course I'll define this only under
CONFIG_SATA_ZPODD.

> 
>> +/*
>> + * Under some circumstances, there is a race between the calling thread
>> + * of disk_block_events and the events checking function. To avoid such a race,
>> + * this function will check if the delayed work is pending. If not, it means
>> + * the work is either not queued or is already running, false is returned.
>> + * And if yes, try to cancel the delayed work. If succedded, disk_block_events
>> + * will be called and there is no worry that cancel_delayed_work_sync will
>> + * deadlock the events checking function. And if failed, false is returned.
>> + */
>> +bool disk_try_block_events(struct gendisk *disk)
>> +{
>> +	struct disk_events *ev = disk->ev;
>> +
>> +	if (!ev)
>> +		return false;
>> +
>> +	if (delayed_work_pending(&ev->dwork)) {
> 
> And please don't use delayed_work_pending() like this.  It doesn't add
> anything.  cancel_delayed_work() already needs to perform all the
> necessary tests.

OK, thanks for the suggestion.

-Aaron

> 
>> +		if (cancel_delayed_work(&disk->ev->dwork)) {
>> +			disk_block_events(disk);
>> +			return true;
>> +		}
>> +	}
>> +
>> +	return false;
>> +}
> 
> Thanks.
> 

Re: [PATCH v8 09/11] block: add a new interface to block events

[Tejun Heo]

Hello,

On Tue, Oct 30, 2012 at 03:04:17PM +0800, Aaron Lu wrote:
> > check_event() can retry.  Just add a per-sr mutex which is try-locked
> > by sr_block_check_events() and grab it when entering zero power.
> 
> Good suggestion. I didn't think about solving it this way.
> 
> Many people suggest me that ZPODD is pure SATA/ACPI stuff, and should
> not pollute sr driver, so I was trying hard not to touch sr while
> preparing these patches, unless there is no other choice(like the
> blocking event interface).
> 
> So I'm not sure if your suggestion is the way to go.
> 
> James, what do you think? Is it OK if I add a mutex into the scsi_cd
> structure to do this? Of course I'll define this only under
> CONFIG_SATA_ZPODD.

I don't think what James' and my suggestions are that different.  Just
silence check_event() while zpodd is kicked in somehow.  There's no
reason to synchronize across multiple subsystems.

Thanks.

-- 
tejun

Re: [PATCH v8 05/11] libata-eh: allow defer in ata_exec_internal

[Tejun Heo]

Hello,

On Tue, Oct 30, 2012 at 11:09:21AM +0800, Aaron Lu wrote:
> I'm not aware of a place to store such ODD specific information when
> probing the device.

You can always add some fields. :)

> I'm currently storing the loading mech type in structure zpodd, which
> gets created after the corresponding SCSI device gets created in
> ata_scsi_scan_host, so at the probe time, the zpodd structure does not
> exist yet. And the reason I create the zpodd sturcture this late is
> that, it is only created when the ODD together the platform is ZPODD
> capable, and to find out if this platform is ZPODD capbale, ACPI binding
> has to occur first, and ACPI binding happens when SCSI device is added
> to the device tree.

Hmm... I see.  Which ACPI binding is it?  The ATA ACPI binding happens
during probing.  It's a different one, I presume?

Thanks.

-- 
tejun

Re: [PATCH v8 05/11] libata-eh: allow defer in ata_exec_internal

[Aaron Lu]

On 11/01/2012 05:52 AM, Tejun Heo wrote:
> Hello,
> 
> On Tue, Oct 30, 2012 at 11:09:21AM +0800, Aaron Lu wrote:
>> I'm not aware of a place to store such ODD specific information when
>> probing the device.
> 
> You can always add some fields. :)

OK. My concern is that, such information is only useful to ZPODD
capable device+platforms, so checking this loading mechanism thing for
all ATAPI devices during probe time doesn't seem a good idea.

> 
>> I'm currently storing the loading mech type in structure zpodd, which
>> gets created after the corresponding SCSI device gets created in
>> ata_scsi_scan_host, so at the probe time, the zpodd structure does not
>> exist yet. And the reason I create the zpodd sturcture this late is
>> that, it is only created when the ODD together the platform is ZPODD
>> capable, and to find out if this platform is ZPODD capbale, ACPI binding
>> has to occur first, and ACPI binding happens when SCSI device is added
>> to the device tree.
> 
> Hmm... I see.  Which ACPI binding is it?  The ATA ACPI binding happens
> during probing.  It's a different one, I presume?

Since commit 6b66d95895c149cbc04d4fac5a2f5477c543a8ae:
libata: bind the Linux device tree to the ACPI device tree
ACPI binding happens when SCSI devices are added to the device tree. The
ata port/device software structure does not have a acpi_handle field
anymore.

Thanks,
Aaron

Re: [PATCH v8 09/11] block: add a new interface to block events

[Aaron Lu]

Hi,

On 11/01/2012 05:51 AM, Tejun Heo wrote:
> Hello,
> 
> On Tue, Oct 30, 2012 at 03:04:17PM +0800, Aaron Lu wrote:
>>> check_event() can retry.  Just add a per-sr mutex which is try-locked
>>> by sr_block_check_events() and grab it when entering zero power.
>>
>> Good suggestion. I didn't think about solving it this way.
>>
>> Many people suggest me that ZPODD is pure SATA/ACPI stuff, and should
>> not pollute sr driver, so I was trying hard not to touch sr while
>> preparing these patches, unless there is no other choice(like the
>> blocking event interface).
>>
>> So I'm not sure if your suggestion is the way to go.
>>
>> James, what do you think? Is it OK if I add a mutex into the scsi_cd
>> structure to do this? Of course I'll define this only under
>> CONFIG_SATA_ZPODD.
> 
> I don't think what James' and my suggestions are that different.  Just
> silence check_event() while zpodd is kicked in somehow.  There's no

Well, since sr is not supposed to know anything about ZPODD, I don't see
another way to silence check_event() in ATA layer.

What I hope to achieve is when zero power ready status is sensed in ATA
layer, no more sr_check_events should be called, as that function will
runtime resume the ODD.

Thanks,
Aaron

> reason to synchronize across multiple subsystems.
> 
> Thanks.
> 

Re: [PATCH v8 05/11] libata-eh: allow defer in ata_exec_internal

[Tejun Heo]

Hello, Aaron.

On Thu, Nov 01, 2012 at 10:35:10AM +0800, Aaron Lu wrote:
> > You can always add some fields. :)
> 
> OK. My concern is that, such information is only useful to ZPODD
> capable device+platforms, so checking this loading mechanism thing for
> all ATAPI devices during probe time doesn't seem a good idea.

Hmmm.. but it's not like querying acpi is high cost or anything.
Maybe I'm missing something but if it can be simpler that way, please
do so by all means.  I don't care whether you add some extra fields or
some processing overhead during probing.  It doesn't really matter.

> > Hmm... I see.  Which ACPI binding is it?  The ATA ACPI binding happens
> > during probing.  It's a different one, I presume?
> 
> Since commit 6b66d95895c149cbc04d4fac5a2f5477c543a8ae:
> libata: bind the Linux device tree to the ACPI device tree
> ACPI binding happens when SCSI devices are added to the device tree. The
> ata port/device software structure does not have a acpi_handle field
> anymore.

Please bear with me.  I haven't paid much attention to zpodd, so it
probably is my ignorance but at least the ATA <-> ACPI association
happens during probing by calling ata_acpi_on_devcfg() from
ata_dev_configure(), and it pretty much has to happen then because
_SDD/_GTF should be executed after hardreset which happens during
probing.  So, yeah, I'm confused.

Thanks.

-- 
tejun

Re: [PATCH v8 05/11] libata-eh: allow defer in ata_exec_internal

[Aaron Lu]

On 11/02/2012 12:03 AM, Tejun Heo wrote:
> Hello, Aaron.
> 
> On Thu, Nov 01, 2012 at 10:35:10AM +0800, Aaron Lu wrote:
>>> You can always add some fields. :)
>>
>> OK. My concern is that, such information is only useful to ZPODD
>> capable device+platforms, so checking this loading mechanism thing for
>> all ATAPI devices during probe time doesn't seem a good idea.
> 
> Hmmm.. but it's not like querying acpi is high cost or anything.
> Maybe I'm missing something but if it can be simpler that way, please
> do so by all means.  I don't care whether you add some extra fields or
> some processing overhead during probing.  It doesn't really matter.

OK, thanks for the suggestion.

> 
>>> Hmm... I see.  Which ACPI binding is it?  The ATA ACPI binding happens
>>> during probing.  It's a different one, I presume?
>>
>> Since commit 6b66d95895c149cbc04d4fac5a2f5477c543a8ae:
>> libata: bind the Linux device tree to the ACPI device tree
>> ACPI binding happens when SCSI devices are added to the device tree. The
>> ata port/device software structure does not have a acpi_handle field
>> anymore.
> 
> Please bear with me.  I haven't paid much attention to zpodd, so it
> probably is my ignorance but at least the ATA <-> ACPI association
> happens during probing by calling ata_acpi_on_devcfg() from
> ata_dev_configure(), and it pretty much has to happen then because
> _SDD/_GTF should be executed after hardreset which happens during
> probing.  So, yeah, I'm confused.

Oh, yes, ACPI handle can be retrieved any time by quering some ACPI
method. And during probe time, it is done that way, while the binding
happens much later... But the ACPI handle can indeed be fetched at that
time :-)
So all the conditions I need to test ZPODD support is ready during probe
time and I'll move the check loading mechanism code there, thanks for
your suggestion.

Thanks,
Aaron