[PATCH v13 0/9] ZPODD Patches

[PATCH v13 0/9] ZPODD Patches

[Aaron Lu]

v13:
Use atomic type for disk_events_disable_depth, so that disabling can
stack and has its own synchronization rule as suggested by Tejun Heo;

Set zp_ready to false whenever we found the ODD is not in ZP ready state
due to support of NO_POWEROFF qos flag. This change is made in patch 6
instead of patch 4 so that it is clear why this change is necessary.

v12:
Suggestions by Tejun Heo:
Fold patch 2 into patch 3;
Do not use bitfields in zpodd structure;
Call zpodd_exit in ata_scsi_remove_dev;
Remove run_atapi_cmd, use ata_exec_internal directly;
Use enum to return ODD loading mechanism type information;
Added some comments.

Typo fix:
Fixed some mis-spellings as pointed out by Sergei Shtylyov.

Several function name changes:
Rename zpodd_pre_poweroff to zpodd_enable_run_wake, zpodd_pre_poweron
to zpodd_disable_run_wake. This feels clearer and people can get what
the two functions are supposed to do by their names;
Rename zpodd_post_resume to zpodd_post_poweron, since this function is
only useful when the ODD is powered off during suspend.
I decided to rename these functions when I'm adding comments for them
as suggested by Tejun Heo, and I felt I didn't name them well...

QOS no_poweroff flag:
Only export qos flag for ZPODD devices, disks that can be runtime
powered off should also export this interface, but it doesn't belong
to this patchset so will be done in other patches.

A new patch:
Added a patch to remove a no more useful scsi_device flag can_power_off.

A git repo for it:
https://github.com/aaronlu/linux.git zpodd_v12

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: 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: scsi: no poll when ODD is powered off
  libata: do not suspend port if normal ODD is attached
  scsi: remove can_power_off flag from scsi_device

 drivers/ata/Kconfig        |  13 ++
 drivers/ata/Makefile       |   1 +
 drivers/ata/libata-acpi.c  | 109 +++++-----------
 drivers/ata/libata-core.c  |  23 +++-
 drivers/ata/libata-eh.c    |  16 ++-
 drivers/ata/libata-scsi.c  |   2 +
 drivers/ata/libata-zpodd.c | 300 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata.h       |  27 ++++
 drivers/scsi/scsi_lib.c    |  14 +++
 drivers/scsi/sr.c          |  36 +++++-
 include/linux/libata.h     |   3 +
 include/scsi/scsi_device.h |   5 +-
 include/uapi/linux/cdrom.h |  34 +++++
 13 files changed, 492 insertions(+), 91 deletions(-)
 create mode 100644 drivers/ata/libata-zpodd.c

-- 
1.7.11.7

[PATCH v13 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 v13 2/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.

A new config CONFIG_SATA_ZPODD is added to selectively build ZPODD code.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Acked-by: Tejun Heo <tj@kernel.org>
---
 drivers/ata/Kconfig        |  13 ++++++
 drivers/ata/Makefile       |   1 +
 drivers/ata/libata-core.c  |   4 +-
 drivers/ata/libata-scsi.c  |   2 +
 drivers/ata/libata-zpodd.c | 100 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata.h       |  14 +++++++
 include/linux/libata.h     |   3 ++
 include/uapi/linux/cdrom.h |  34 +++++++++++++++
 8 files changed, 170 insertions(+), 1 deletion(-)
 create mode 100644 drivers/ata/libata-zpodd.c

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
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 275941b..c7ecd84 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2401,8 +2401,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-scsi.c b/drivers/ata/libata-scsi.c
index 7c337e7..1ff0185 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3755,6 +3755,8 @@ static void ata_scsi_remove_dev(struct ata_device *dev)
 	mutex_lock(&ap->scsi_host->scan_mutex);
 	spin_lock_irqsave(ap->lock, flags);
 
+	if (zpodd_dev_enabled(dev))
+		zpodd_exit(dev);
 	ata_acpi_unbind(dev);
 
 	/* clearing dev->sdev is protected by host lock */
diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
new file mode 100644
index 0000000..27eed2f
--- /dev/null
+++ b/drivers/ata/libata-zpodd.c
@@ -0,0 +1,100 @@
+#include <linux/libata.h>
+#include <linux/cdrom.h>
+
+#include "libata.h"
+
+enum odd_mech_type {
+	ODD_MECH_TYPE_SLOT,
+	ODD_MECH_TYPE_DRAWER,
+	ODD_MECH_TYPE_UNSUPPORTED,
+};
+
+struct zpodd {
+	enum odd_mech_type	mech_type; /* init during probe, RO afterwards */
+	struct ata_device	*dev;
+};
+
+/* Per the spec, only slot type and drawer type ODD can be supported */
+static enum odd_mech_type zpodd_get_mech_type(struct ata_device *dev)
+{
+	char buf[16];
+	unsigned int ret;
+	struct rm_feature_desc *desc = (void *)(buf + 8);
+	struct ata_taskfile tf = {};
+
+	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,
+	};
+
+	tf.flags = ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.command = ATA_CMD_PACKET;
+	tf.protocol = ATAPI_PROT_PIO;
+	tf.lbam = sizeof(buf);
+
+	ret = ata_exec_internal(dev, &tf, cdb, DMA_FROM_DEVICE,
+				buf, sizeof(buf), 0);
+	if (ret)
+		return ODD_MECH_TYPE_UNSUPPORTED;
+
+	if (be16_to_cpu(desc->feature_code) != 3)
+		return ODD_MECH_TYPE_UNSUPPORTED;
+
+	if (desc->mech_type == 0 && desc->load == 0 && desc->eject == 1)
+		return ODD_MECH_TYPE_SLOT;
+	else if (desc->mech_type == 1 && desc->load == 0 && desc->eject == 1)
+		return ODD_MECH_TYPE_DRAWER;
+	else
+		return ODD_MECH_TYPE_UNSUPPORTED;
+}
+
+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)
+{
+	enum odd_mech_type mech_type;
+	struct zpodd *zpodd;
+
+	if (dev->zpodd)
+		return;
+
+	if (!odd_can_poweroff(dev))
+		return;
+
+	mech_type = zpodd_get_mech_type(dev);
+	if (mech_type == ODD_MECH_TYPE_UNSUPPORTED)
+		return;
+
+	zpodd = kzalloc(sizeof(struct zpodd), GFP_KERNEL);
+	if (!zpodd)
+		return;
+
+	zpodd->mech_type = mech_type;
+
+	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..a21740b 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 != NULL;
+}
+#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 7ae207e..65ff67e 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -621,6 +621,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 v13 3/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>
Acked-by: Tejun Heo <tj@kernel.org>
---
 drivers/ata/libata-acpi.c  | 71 ----------------------------------------------
 drivers/ata/libata-zpodd.c | 34 ++++++++++++++++++++++
 2 files changed, 34 insertions(+), 71 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index ef01ac0..446b4e7 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,13 +1006,11 @@ 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);
 }
 
 void ata_acpi_unbind(struct ata_device *dev)
 {
-	ata_acpi_remove_pm_notifier(dev);
 	ata_acpi_unregister_power_resource(dev);
 }
 
@@ -1105,9 +1052,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))
@@ -1124,21 +1068,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 27eed2f..9a0d90d 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"
 
@@ -12,6 +14,10 @@ enum odd_mech_type {
 struct zpodd {
 	enum odd_mech_type	mech_type; /* init during probe, RO afterwards */
 	struct ata_device	*dev;
+
+	/* The following fields are synchronized by PM core. */
+	bool			from_notify; /* resumed as a result of
+					      * acpi wake notification */
 };
 
 /* Per the spec, only slot type and drawer type ODD can be supported */
@@ -68,6 +74,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)
 {
 	enum odd_mech_type mech_type;
@@ -89,12 +121,14 @@ void zpodd_init(struct ata_device *dev)
 
 	zpodd->mech_type = mech_type;
 
+	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 v13 4/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
zp_sampled will be set and 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    | 14 +++++++--
 drivers/ata/libata-zpodd.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata.h       |  5 ++++
 3 files changed, 92 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index bcf4437..a0dddc3 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,15 @@ static void ata_eh_handle_port_suspend(struct ata_port *ap)
 
 	WARN_ON(ap->pflags & ATA_PFLAG_SUSPENDED);
 
+	/*
+	 * If we have a ZPODD attached, check its zero
+	 * power ready status before the port is frozen.
+	 */
+	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 9a0d90d..71dd48c 100644
--- a/drivers/ata/libata-zpodd.c
+++ b/drivers/ata/libata-zpodd.c
@@ -1,10 +1,15 @@
 #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");
+
 enum odd_mech_type {
 	ODD_MECH_TYPE_SLOT,
 	ODD_MECH_TYPE_DRAWER,
@@ -18,6 +23,9 @@ struct zpodd {
 	/* The following fields are synchronized by PM core. */
 	bool			from_notify; /* resumed as a result of
 					      * acpi wake notification */
+	bool			zp_ready; /* ZP ready state */
+	unsigned long		last_ready; /* last ZP ready timestamp */
+	bool			zp_sampled; /* ZP ready state sampled */
 };
 
 /* Per the spec, only slot type and drawer type ODD can be supported */
@@ -74,6 +82,73 @@ 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->mech_type == ODD_MECH_TYPE_SLOT)
+		/* no media inside */
+		return asc == 0x3a;
+	else
+		/* no media inside and door closed */
+		return asc == 0x3a && ascq == 0x01;
+}
+
+/*
+ * Update the zpodd->zp_ready field. This field will only be set
+ * if the ODD has stayed in ZP ready state for zpodd_poweroff_delay
+ * time, and will be used to decide if power off is allowed. If it
+ * is set, it will be cleared during resume from powered off state.
+ */
+void zpodd_on_suspend(struct ata_device *dev)
+{
+	struct zpodd *zpodd = dev->zpodd;
+	unsigned long expires;
+
+	if (!zpready(dev)) {
+		zpodd->zp_sampled = false;
+		return;
+	}
+
+	if (!zpodd->zp_sampled) {
+		zpodd->zp_sampled = true;
+		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 a21740b..b9b2bb1 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 != NULL;
 }
+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 v13 5/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
if the ODD can be powered off, runtime wake capability needs to be
enabled and powered_off flag will be set to let resume code knows that
the ODD was in powered off state.

And on resume, before it is powered on, if it was powered off during
suspend, runtime wake capability needs to be disabled. After it is
recovered, the ODD is considered functional, post power on processing
like eject tray if the ODD is drawer type is done, and several ZPODD
related fields will also be reset.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Acked-by: Tejun Heo <tj@kernel.org>
---
 drivers/ata/libata-acpi.c  | 35 +++++++++++++------
 drivers/ata/libata-eh.c    |  2 ++
 drivers/ata/libata-zpodd.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata.h       |  8 +++++
 4 files changed, 118 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 446b4e7..6f72c64 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_enable_run_wake(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_disable_run_wake(dev);
 			acpi_bus_set_power(handle, ACPI_STATE_D0);
 		}
 	}
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index a0dddc3..50f3ef0 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_poweron(dev);
 			}
 		}
 
diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
index 71dd48c..1f5d52a 100644
--- a/drivers/ata/libata-zpodd.c
+++ b/drivers/ata/libata-zpodd.c
@@ -26,8 +26,26 @@ struct zpodd {
 	bool			zp_ready; /* ZP ready state */
 	unsigned long		last_ready; /* last ZP ready timestamp */
 	bool			zp_sampled; /* ZP ready state sampled */
+	bool			powered_off; /* ODD is powered off
+					      *	during suspend */
 };
 
+static int eject_tray(struct ata_device *dev)
+{
+	struct ata_taskfile tf = {};
+	const char cdb[] = {  GPCMD_START_STOP_UNIT,
+		0, 0, 0,
+		0x02,     /* LoEj */
+		0, 0, 0, 0, 0, 0, 0,
+	};
+
+	tf.flags = ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.command = ATA_CMD_PACKET;
+	tf.protocol = ATAPI_PROT_NODATA;
+
+	return ata_exec_internal(dev, &tf, cdb, DMA_NONE, NULL, 0, 0);
+}
+
 /* Per the spec, only slot type and drawer type ODD can be supported */
 static enum odd_mech_type zpodd_get_mech_type(struct ata_device *dev)
 {
@@ -149,6 +167,71 @@ 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;
+}
+
+/*
+ * Enable runtime wake capability through ACPI and set the powered_off flag,
+ * this flag will be used during resume to decide what operations are needed
+ * to take.
+ */
+void zpodd_enable_run_wake(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);
+}
+
+/* Disable runtime wake capability if it is enabled */
+void zpodd_disable_run_wake(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);
+	}
+}
+
+/*
+ * Post power on processing after the ODD has been recovered. If the
+ * ODD wasn't powered off during suspend, it doesn't do anything.
+ *
+ * For drawer type ODD, if it is powered on due to user pressed the
+ * eject button, the tray needs to be ejected. This can only be done
+ * after the ODD has been recovered, i.e. link is initialized and
+ * device is able to process NON_DATA PIO command, as eject needs to
+ * send command for the ODD to process.
+ *
+ * The from_notify flag set in wake notification handler function
+ * zpodd_wake_dev represents if power on is due to user's action.
+ *
+ * For both types of ODD, several fields need to be reset.
+ */
+void zpodd_post_poweron(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->mech_type == ODD_MECH_TYPE_DRAWER)
+			eject_tray(dev);
+	}
+
+	zpodd->zp_sampled = false;
+	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 b9b2bb1..c949dd3 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 != NULL;
 }
 void zpodd_on_suspend(struct ata_device *dev);
+bool zpodd_zpready(struct ata_device *dev);
+void zpodd_enable_run_wake(struct ata_device *dev);
+void zpodd_disable_run_wake(struct ata_device *dev);
+void zpodd_post_poweron(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_enable_run_wake(struct ata_device *dev) {}
+static inline void zpodd_disable_run_wake(struct ata_device *dev) {}
+static inline void zpodd_post_poweron(struct ata_device *dev) {}
 #endif /* CONFIG_SATA_ZPODD */
 
 #endif /* __LIBATA_H__ */
-- 
1.7.11.7

[PATCH v13 6/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
ZPODD feature, if he/she has a broken platform or devices or simply does
not like this feature.

This flag is exposed to user space only for ZPODD devices.

Due to this flag, it is possible the ODD is ZP ready but we didn't power
it off. So the zp_ready flag will need to be cleared whenever we found
the ODD is not in ZP ready state. Previously, once zp_ready is set, the
ODD will always be powered off and the flag will be cleared in
post_poweron. But this is no longer the case now.

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

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 6f72c64..9709449 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 (zpodd_dev_enabled(dev))
+		dev_pm_qos_expose_flags(&dev->sdev->sdev_gendev, 0);
 }
 
 void ata_acpi_unbind(struct ata_device *dev)
diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
index 1f5d52a..540b0b7 100644
--- a/drivers/ata/libata-zpodd.c
+++ b/drivers/ata/libata-zpodd.c
@@ -150,6 +150,7 @@ void zpodd_on_suspend(struct ata_device *dev)
 
 	if (!zpready(dev)) {
 		zpodd->zp_sampled = false;
+		zpodd->zp_ready = false;
 		return;
 	}
 
-- 
1.7.11.7

[PATCH v13 7/9] libata: scsi: 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 disk_events_disable_depth for scsi device. This field is a
hint set by LLDs to convey information to upper layer drivers. A value
of 0 means media poll is necessary for the device, while values above 0
means media poll is not needed and should better be skipped. So we can
increase its value when we are to power off the ODD in ATA layer and
decrease its value when the ODD is powered on, effectively silence the
media events poll.

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

diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
index 540b0b7..a7df603 100644
--- a/drivers/ata/libata-zpodd.c
+++ b/drivers/ata/libata-zpodd.c
@@ -178,11 +178,16 @@ bool zpodd_zpready(struct ata_device *dev)
  * Enable runtime wake capability through ACPI and set the powered_off flag,
  * this flag will be used during resume to decide what operations are needed
  * to take.
+ *
+ * Also, media poll needs to be silenced, so that it doesn't bring the ODD
+ * back to full power state every few seconds.
  */
 void zpodd_enable_run_wake(struct ata_device *dev)
 {
 	struct zpodd *zpodd = dev->zpodd;
 
+	sdev_disable_disk_events(dev->sdev);
+
 	zpodd->powered_off = true;
 	device_set_run_wake(&dev->sdev->sdev_gendev, true);
 	acpi_pm_device_run_wake(&dev->sdev->sdev_gendev, true);
@@ -231,6 +236,8 @@ void zpodd_post_poweron(struct ata_device *dev)
 
 	zpodd->zp_sampled = false;
 	zpodd->zp_ready = false;
+
+	sdev_enable_disk_events(dev->sdev);
 }
 
 static void zpodd_wake_dev(acpi_handle handle, u32 event, void *context)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f1bf5af..765398c 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2617,3 +2617,17 @@ void scsi_kunmap_atomic_sg(void *virt)
 	kunmap_atomic(virt);
 }
 EXPORT_SYMBOL(scsi_kunmap_atomic_sg);
+
+void sdev_disable_disk_events(struct scsi_device *sdev)
+{
+	atomic_inc(&sdev->disk_events_disable_depth);
+}
+EXPORT_SYMBOL(sdev_disable_disk_events);
+
+void sdev_enable_disk_events(struct scsi_device *sdev)
+{
+	if (WARN_ON_ONCE(atomic_read(&sdev->disk_events_disable_depth) <= 0))
+		return;
+	atomic_dec(&sdev->disk_events_disable_depth);
+}
+EXPORT_SYMBOL(sdev_enable_disk_events);
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 4d1a610..c99ecd4 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 (atomic_read(&cd->device->disk_events_disable_depth) == 0)
+		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..bb1371b 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -161,6 +161,8 @@ struct scsi_device {
 	unsigned wce_default_on:1;	/* Cache is ON by default */
 	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
 
+	atomic_t disk_events_disable_depth; /* disable depth for disk events */
+
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */
 	struct work_struct event_work;
@@ -397,6 +399,8 @@ extern int scsi_execute_req(struct scsi_device *sdev, const unsigned char *cmd,
 			    int data_direction, void *buffer, unsigned bufflen,
 			    struct scsi_sense_hdr *, int timeout, int retries,
 			    int *resid);
+extern void sdev_disable_disk_events(struct scsi_device *sdev);
+extern void sdev_enable_disk_events(struct scsi_device *sdev);
 
 #ifdef CONFIG_PM_RUNTIME
 extern int scsi_autopm_get_device(struct scsi_device *);
-- 
1.7.11.7

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

[Aaron Lu]

For ODDs, the upper layer will poll for media change every few
seconds, which will make it enter and leave suspend state very
often. And as each suspend will also cause a hard/soft reset,
the gain of runtime suspend is very little while the ODD may
malfunction 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>
Acked-by: Tejun Heo <tj@kernel.org>
---
 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 c7ecd84..b7eed82 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5413,8 +5413,27 @@ static int ata_port_resume(struct device *dev)
 	return rc;
 }
 
+/*
+ * For ODDs, the upper layer will poll for media change every few seconds,
+ * which will make it enter and leave suspend state every few seconds. And
+ * as each suspend will cause a hard/soft reset, the gain of runtime suspend
+ * is very little and the ODD may malfunction 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

[PATCH v13 9/9] scsi: remove can_power_off flag from scsi_device

[Aaron Lu]

Commit 166a2967b45ede2e2e56f3ede3cd32053dc17812 "libata: tell scsi layer
device supports runtime power off" introduced the can_power_off flag for
scsi_device and is used to support ZPODD implementation in SCSI layer.
Since ZPODD is now implemented in ATA layer, that flag is no longer
needed, so remove it.

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

diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index bb1371b..a7f9cba 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -157,7 +157,6 @@ struct scsi_device {
 	unsigned no_read_capacity_16:1; /* Avoid READ_CAPACITY_16 cmds */
 	unsigned try_rc_10_first:1;	/* Try READ_CAPACACITY_10 first */
 	unsigned is_visible:1;	/* is the device visible in sysfs */
-	unsigned can_power_off:1; /* Device supports runtime power off */
 	unsigned wce_default_on:1;	/* Cache is ON by default */
 	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
 
-- 
1.7.11.7

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

[James Bottomley]

On Tue, 2013-01-15 at 17:20 +0800, Aaron Lu wrote:
> 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>

I'd like an ack from Alan Stern as well, please, but with that, you can
add my acked-by.

James

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

[Alan Stern]

On Wed, 16 Jan 2013, James Bottomley wrote:

> On Tue, 2013-01-15 at 17:20 +0800, Aaron Lu wrote:
> > 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>
> 
> I'd like an ack from Alan Stern as well, please, but with that, you can
> add my acked-by.

Aaron, have you checked whether this patch works okay when you play a
track on an audio-only CD on the computer?  The block interface looks 
okay but I'm not sure about the cdrom_device interface.

Alan Stern

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

[Aaron Lu]

On Wed, Jan 16, 2013 at 11:31:31AM -0500, Alan Stern wrote:
> On Wed, 16 Jan 2013, James Bottomley wrote:
> 
> > On Tue, 2013-01-15 at 17:20 +0800, Aaron Lu wrote:
> > > 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>
> > 
> > I'd like an ack from Alan Stern as well, please, but with that, you can
> > add my acked-by.
> 
> Aaron, have you checked whether this patch works okay when you play a
> track on an audio-only CD on the computer?  The block interface looks 
> okay but I'm not sure about the cdrom_device interface.

Just verified it works OK with the whole patchset applied using 2 audio
CDs.

After the ODD has been put into zero power state, insert an audio cd.
ODD will be resumed, gvfs will automatically mount the disc, and a
dialog titled "Audio Disc" asks me what to do. Press OK, the rhythmbox
application will get started. Press the Play button of rhythmbox, songs
will start to play, and runtime_usage is 2. Eject the disc, the ODD will
be put to zero power state some time later.

Thanks,
Aaron

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

[Alan Stern]

On Fri, 18 Jan 2013, Aaron Lu wrote:

> > Aaron, have you checked whether this patch works okay when you play a
> > track on an audio-only CD on the computer?  The block interface looks 
> > okay but I'm not sure about the cdrom_device interface.
> 
> Just verified it works OK with the whole patchset applied using 2 audio
> CDs.
> 
> After the ODD has been put into zero power state, insert an audio cd.
> ODD will be resumed, gvfs will automatically mount the disc, and a
> dialog titled "Audio Disc" asks me what to do. Press OK, the rhythmbox
> application will get started. Press the Play button of rhythmbox, songs
> will start to play, and runtime_usage is 2. Eject the disc, the ODD will
> be put to zero power state some time later.

What happens if you use a non-ZP drive?

What happens if you're not running a desktop graphical environment, so
gvfs doesn't mount the disc?  Basically, I'm worried that the drive may
remain suspended after sr_open() returns.

What happens if you use a program other than rhythmbox?  There are (or
used to be) programs which would issue the PLAY AUDIO command and then
exit.  The drive would continue playing even while the device file was
closed.  Do we want to drop support for that kind of behavior?

Alan Stern

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

[Aaron Lu]

On 01/18/2013 11:24 PM, Alan Stern wrote:
> On Fri, 18 Jan 2013, Aaron Lu wrote:
>
>>> Aaron, have you checked whether this patch works okay when you play a
>>> track on an audio-only CD on the computer?  The block interface looks
>>> okay but I'm not sure about the cdrom_device interface.
>>
>> Just verified it works OK with the whole patchset applied using 2 audio
>> CDs.
>>
>> After the ODD has been put into zero power state, insert an audio cd.
>> ODD will be resumed, gvfs will automatically mount the disc, and a
>> dialog titled "Audio Disc" asks me what to do. Press OK, the rhythmbox
>> application will get started. Press the Play button of rhythmbox, songs
>> will start to play, and runtime_usage is 2. Eject the disc, the ODD will
>> be put to zero power state some time later.
>
> What happens if you use a non-ZP drive?
>
> What happens if you're not running a desktop graphical environment, so
> gvfs doesn't mount the disc?  Basically, I'm worried that the drive may
> remain suspended after sr_open() returns.

Tried on my notebook with a normal ODD, using mplayer under console
mode, no GUI environment running, works fine.

The usage count will be incremented by the app, and get decreased
only when it exits. So the ODD will not be in runtime suspended state
when the app is running.

>
> What happens if you use a program other than rhythmbox?  There are (or
> used to be) programs which would issue the PLAY AUDIO command and then
> exit.  The drive would continue playing even while the device file was

Then we indeed have a problem. But I didn't find any such app in
Fedora's repo or by searching the internet. But of course such apps can
exist since the Mount Fuji spec defined such a command.

> closed.  Do we want to drop support for that kind of behavior?

I don't think we should drop such support.
And the safest way to avoid such break is we refine the suspend
condition for ODD, and using what ZPODD defined condition isn't that
bad to me:
- for tray type, no media inside and tray close;
- for slot type, no media inside.
While whether tray is closed or not may not be that important, but at
least we should make sure there is no media inside.

Thoughts?

Thanks,
Aaron

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

[Alan Stern]

On Sat, 19 Jan 2013, Aaron Lu wrote:

> > What happens if you're not running a desktop graphical environment, so
> > gvfs doesn't mount the disc?  Basically, I'm worried that the drive may
> > remain suspended after sr_open() returns.
> 
> Tried on my notebook with a normal ODD, using mplayer under console
> mode, no GUI environment running, works fine.
> 
> The usage count will be incremented by the app, and get decreased
> only when it exits. So the ODD will not be in runtime suspended state
> when the app is running.

You missed my point.  What happens when sr_open() gets called?  It 
doesn't try to resume the device.  Will we run into trouble then?

> > What happens if you use a program other than rhythmbox?  There are (or
> > used to be) programs which would issue the PLAY AUDIO command and then
> > exit.  The drive would continue playing even while the device file was
> 
> Then we indeed have a problem. But I didn't find any such app in
> Fedora's repo or by searching the internet.

http://rpm.pbone.net/index.php3/stat/4/idpl/2392183/dir/redhat_5.x/com/cdp-0.33-10.i386.rpm.html

> > closed.  Do we want to drop support for that kind of behavior?
> 
> I don't think we should drop such support.
> And the safest way to avoid such break is we refine the suspend
> condition for ODD, and using what ZPODD defined condition isn't that
> bad to me:
> - for tray type, no media inside and tray close;
> - for slot type, no media inside.
> While whether tray is closed or not may not be that important, but at
> least we should make sure there is no media inside.
> 
> Thoughts?

That sounds reasonable to me, at least as a first step.  If people want 
their CD drive to suspend, they can eject the disc.

Alan Stern

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

[Julian Calaby]

Hi Alan,

On Sun, Jan 20, 2013 at 5:46 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sat, 19 Jan 2013, Aaron Lu wrote:
>> > closed.  Do we want to drop support for that kind of behavior?
>>
>> I don't think we should drop such support.
>> And the safest way to avoid such break is we refine the suspend
>> condition for ODD, and using what ZPODD defined condition isn't that
>> bad to me:
>> - for tray type, no media inside and tray close;
>> - for slot type, no media inside.
>> While whether tray is closed or not may not be that important, but at
>> least we should make sure there is no media inside.
>>
>> Thoughts?
>
> That sounds reasonable to me, at least as a first step.  If people want
> their CD drive to suspend, they can eject the disc.

Stupid question: does the kernel know if a CD has audio tracks?

(I'm assuming that nobody will access a data track without mounting it
or holding the device open)

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

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

[Aaron Lu]

On Sat, Jan 19, 2013 at 01:46:15PM -0500, Alan Stern wrote:
> On Sat, 19 Jan 2013, Aaron Lu wrote:
> 
> > > What happens if you're not running a desktop graphical environment, so
> > > gvfs doesn't mount the disc?  Basically, I'm worried that the drive may
> > > remain suspended after sr_open() returns.
> > 
> > Tried on my notebook with a normal ODD, using mplayer under console
> > mode, no GUI environment running, works fine.
> > 
> > The usage count will be incremented by the app, and get decreased
> > only when it exits. So the ODD will not be in runtime suspended state
> > when the app is running.
> 
> You missed my point.  What happens when sr_open() gets called?  It 
> doesn't try to resume the device.  Will we run into trouble then?

Oh yes, you mean the cdo->open gets called without going through sr's
block interface.

I just checked the code, it looks to me the cdrom interface code is the
code common to cdrom functionality, to be used by various underlying
block drivers like sr, ide-cd, etc. All the code cdrom.c has provided
requires a cdi parameter, which can only be provided by the underlying
block driver, so I don't think those cdrom_device_ops functions will be
called without going through the block drivers' interface.

But the functions defined in block_device_operations by sr can be called
by other kernel components as long as they have access to the gendisk
structure of the block device, so I'll add get/sync pair to all those
functions(i.e. sr_block_revalidate_disk, sr_block_ioctl in addition to
the open/release/check_events calls).

> 
> > > What happens if you use a program other than rhythmbox?  There are (or
> > > used to be) programs which would issue the PLAY AUDIO command and then
> > > exit.  The drive would continue playing even while the device file was
> > 
> > Then we indeed have a problem. But I didn't find any such app in
> > Fedora's repo or by searching the internet.
> 
> http://rpm.pbone.net/index.php3/stat/4/idpl/2392183/dir/redhat_5.x/com/cdp-0.33-10.i386.rpm.html
> 
> > > closed.  Do we want to drop support for that kind of behavior?
> > 
> > I don't think we should drop such support.
> > And the safest way to avoid such break is we refine the suspend
> > condition for ODD, and using what ZPODD defined condition isn't that
> > bad to me:
> > - for tray type, no media inside and tray close;
> > - for slot type, no media inside.
> > While whether tray is closed or not may not be that important, but at
> > least we should make sure there is no media inside.
> > 
> > Thoughts?
> 
> That sounds reasonable to me, at least as a first step.  If people want 
> their CD drive to suspend, they can eject the disc.

I'm gonna add the cd->media_present check in sr's runtime suspend
callback, if sr thinks there is media inside, suspend will not proceed.

Thanks,
Aaron

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

[Aaron Lu]

On Mon, Jan 21, 2013 at 02:31:50PM +1100, Julian Calaby wrote:
> Hi Alan,
> 
> On Sun, Jan 20, 2013 at 5:46 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Sat, 19 Jan 2013, Aaron Lu wrote:
> >> > closed.  Do we want to drop support for that kind of behavior?
> >>
> >> I don't think we should drop such support.
> >> And the safest way to avoid such break is we refine the suspend
> >> condition for ODD, and using what ZPODD defined condition isn't that
> >> bad to me:
> >> - for tray type, no media inside and tray close;
> >> - for slot type, no media inside.
> >> While whether tray is closed or not may not be that important, but at
> >> least we should make sure there is no media inside.
> >>
> >> Thoughts?
> >
> > That sounds reasonable to me, at least as a first step.  If people want
> > their CD drive to suspend, they can eject the disc.
> 
> Stupid question: does the kernel know if a CD has audio tracks?

Yes, cdrom module knows that, but the block driver(e.g. sr) doesn't.
See cdrom_count_tracks in cdrom.c.

May I know if you have an use case that you want to runtime suspend the
cd drive with a disc inside? That would help us to refine the runtime
suspend condition.

Thanks,
Aaron

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

[Julian Calaby]

Hi Aaron,

On Mon, Jan 21, 2013 at 7:14 PM, Aaron Lu <aaron.lu@intel.com> wrote:
> On Mon, Jan 21, 2013 at 02:31:50PM +1100, Julian Calaby wrote:
>> Hi Alan,
>>
>> On Sun, Jan 20, 2013 at 5:46 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Sat, 19 Jan 2013, Aaron Lu wrote:
>> >> > closed.  Do we want to drop support for that kind of behavior?
>> >>
>> >> I don't think we should drop such support.
>> >> And the safest way to avoid such break is we refine the suspend
>> >> condition for ODD, and using what ZPODD defined condition isn't that
>> >> bad to me:
>> >> - for tray type, no media inside and tray close;
>> >> - for slot type, no media inside.
>> >> While whether tray is closed or not may not be that important, but at
>> >> least we should make sure there is no media inside.
>> >>
>> >> Thoughts?
>> >
>> > That sounds reasonable to me, at least as a first step.  If people want
>> > their CD drive to suspend, they can eject the disc.
>>
>> Stupid question: does the kernel know if a CD has audio tracks?
>
> Yes, cdrom module knows that, but the block driver(e.g. sr) doesn't.
> See cdrom_count_tracks in cdrom.c.
>
> May I know if you have an use case that you want to runtime suspend the
> cd drive with a disc inside? That would help us to refine the runtime
> suspend condition.

The discussion seemed to be restricting when the drive would be
suspended to only occasions where the drive is empty and, where
applicable, closed. I'll admit that I hadn't followed the discussion
enough to know what the restrictions were before that, but this seemed
to be a further restriction, therefore I assumed that you were
originally planning to be able to suspend the drive with a disk
inside.

I asked my question in the hope of someone setting up the compromise:
"we could suspend the drive only if the drive is empty and closed, or
a data disc is inside and nobody's using it."

Personally, providing nobody's using it, and relevant state is stored,
I can't see any reason why you can't suspend a drive with a disc in
it.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

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

[Aaron Lu]

Hi Julian,

On Mon, Jan 21, 2013 at 07:55:20PM +1100, Julian Calaby wrote:
> Hi Aaron,
> 
> On Mon, Jan 21, 2013 at 7:14 PM, Aaron Lu <aaron.lu@intel.com> wrote:
> > On Mon, Jan 21, 2013 at 02:31:50PM +1100, Julian Calaby wrote:
> >> Hi Alan,
> >>
> >> On Sun, Jan 20, 2013 at 5:46 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> >> > On Sat, 19 Jan 2013, Aaron Lu wrote:
> >> >> > closed.  Do we want to drop support for that kind of behavior?
> >> >>
> >> >> I don't think we should drop such support.
> >> >> And the safest way to avoid such break is we refine the suspend
> >> >> condition for ODD, and using what ZPODD defined condition isn't that
> >> >> bad to me:
> >> >> - for tray type, no media inside and tray close;
> >> >> - for slot type, no media inside.
> >> >> While whether tray is closed or not may not be that important, but at
> >> >> least we should make sure there is no media inside.
> >> >>
> >> >> Thoughts?
> >> >
> >> > That sounds reasonable to me, at least as a first step.  If people want
> >> > their CD drive to suspend, they can eject the disc.
> >>
> >> Stupid question: does the kernel know if a CD has audio tracks?
> >
> > Yes, cdrom module knows that, but the block driver(e.g. sr) doesn't.
> > See cdrom_count_tracks in cdrom.c.
> >
> > May I know if you have an use case that you want to runtime suspend the
> > cd drive with a disc inside? That would help us to refine the runtime
> > suspend condition.
> 
> The discussion seemed to be restricting when the drive would be
> suspended to only occasions where the drive is empty and, where
> applicable, closed. I'll admit that I hadn't followed the discussion
> enough to know what the restrictions were before that, but this seemed
> to be a further restriction, therefore I assumed that you were
> originally planning to be able to suspend the drive with a disk
> inside.

Right, that was the original implementation to allow the cd drive to be
runtime suspended even with a disc inside.

> 
> I asked my question in the hope of someone setting up the compromise:
> "we could suspend the drive only if the drive is empty and closed, or
> a data disc is inside and nobody's using it."
> 
> Personally, providing nobody's using it, and relevant state is stored,
> I can't see any reason why you can't suspend a drive with a disc in
> it.

It is not easy for the OS to tell if the drive is being used or not
sometimes :-)

Alan has reminded me it is possible for an app to open the block device
file(/dev/sr0), issue a command(play audio), then close the device file.
>From the OS' point of view, we think nobody is using it. But actually,
the drive is playing cd for the user, so we can't suspend the device.

I think we can always refine the condition check, but as a first step, I
want to be safe and avoid breaking existing functionalities.

Thanks,
Aaron

Re: [PATCH v13 2/9] libata: identify and init ZPODD devices

[Jeff Garzik]

On 01/15/2013 04:20 AM, Aaron Lu wrote:
> 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.
>
> A new config CONFIG_SATA_ZPODD is added to selectively build ZPODD code.
>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> Acked-by: Tejun Heo <tj@kernel.org>

How possible is it to apply patches 2-6 right now?  It appears possible 
according to my read, but I've not tested this guess.

	Jeff

Re: [PATCH v13 2/9] libata: identify and init ZPODD devices

[Aaron Lu]

On 01/21/2013 05:16 PM, Jeff Garzik wrote:
> On 01/15/2013 04:20 AM, Aaron Lu wrote:
>> 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.
>>
>> A new config CONFIG_SATA_ZPODD is added to selectively build ZPODD code.
>>
>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
>> Acked-by: Tejun Heo <tj@kernel.org>
> 
> How possible is it to apply patches 2-6 right now?  It appears possible 
> according to my read, but I've not tested this guess.

Patches 2-6 and 8 can be applied cleanly on top of today's Linus tree
with or without your NEXT branch merged.

Thanks,
Aaron

[RFC PATCH] scsi: sr: support runtime pm

[Aaron Lu]

On Sat, Jan 19, 2013 at 01:46:15PM -0500, Alan Stern wrote:
> On Sat, 19 Jan 2013, Aaron Lu wrote:
> > I don't think we should drop such support.
> > And the safest way to avoid such break is we refine the suspend
> > condition for ODD, and using what ZPODD defined condition isn't that
> > bad to me:
> > - for tray type, no media inside and tray close;
> > - for slot type, no media inside.
> > While whether tray is closed or not may not be that important, but at
> > least we should make sure there is no media inside.
> > 
> > Thoughts?
> 
> That sounds reasonable to me, at least as a first step.  If people want 
> their CD drive to suspend, they can eject the disc.

Here is an updated patch to address the problem, please review, thanks.

Changes to v13:
- Add PM get/put pair functions to all the block device operation
  functions; Move the existing PM get/put pair functions in
  sr_check_events to sr_block_check_events;
- Add sr_runtime_suspend, it will check if there is media inside and if
  yes, avoid suspend.

>From 378bf55810a1118ede481f45132b5c39af891d23 Mon Sep 17 00:00:00 2001
From: Aaron Lu <aaron.lu@intel.com>
Date: Wed, 26 Sep 2012 15:14:56 +0800
Subject: [RFC PATCH] scsi: sr: support runtime pm

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 accessed. And decreasing the runtime usage_count
of the device when the access is done.

The idea is discussed here:
http://thread.gmane.org/gmane.linux.acpi.devel/55243/focus=52703
and here:
http://thread.gmane.org/gmane.linux.ide/53665/focus=58836

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

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 5fc97d2..2e8ddd7 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -45,6 +45,7 @@
 #include <linux/blkdev.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>
@@ -79,6 +80,11 @@ static DEFINE_MUTEX(sr_mutex);
 static int sr_probe(struct device *);
 static int sr_remove(struct device *);
 static int sr_done(struct scsi_cmnd *);
+static int sr_runtime_suspend(struct device *dev);
+
+static struct dev_pm_ops sr_pm_ops = {
+	.runtime_suspend	= sr_runtime_suspend,
+};
 
 static struct scsi_driver sr_template = {
 	.owner			= THIS_MODULE,
@@ -86,6 +92,7 @@ static struct scsi_driver sr_template = {
 		.name   	= "sr",
 		.probe		= sr_probe,
 		.remove		= sr_remove,
+		.pm		= &sr_pm_ops,
 	},
 	.done			= sr_done,
 };
@@ -131,6 +138,16 @@ static inline struct scsi_cd *scsi_cd(struct gendisk *disk)
 	return container_of(disk->private_data, struct scsi_cd, driver);
 }
 
+static int sr_runtime_suspend(struct device *dev)
+{
+	struct scsi_cd *cd = dev_get_drvdata(dev);
+
+	if (cd->media_present)
+		return -EBUSY;
+	else
+		return 0;
+}
+
 /*
  * The get and put routines for the struct scsi_cd.  Note this entity
  * has a scsi_device pointer and owns a reference to this.
@@ -146,7 +163,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 +180,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);
 }
@@ -540,6 +559,8 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 	void __user *argp = (void __user *)arg;
 	int ret;
 
+	scsi_autopm_get_device(cd->device);
+
 	mutex_lock(&sr_mutex);
 
 	/*
@@ -571,6 +592,7 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 
 out:
 	mutex_unlock(&sr_mutex);
+	scsi_autopm_put_device(cd->device);
 	return ret;
 }
 
@@ -578,7 +600,13 @@ 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);
+	unsigned int ret;
+
+	scsi_autopm_get_device(cd->device);
+	ret = cdrom_check_events(&cd->cdi, clearing);
+	scsi_autopm_put_device(cd->device);
+
+	return ret;
 }
 
 static int sr_block_revalidate_disk(struct gendisk *disk)
@@ -586,12 +614,16 @@ static int sr_block_revalidate_disk(struct gendisk *disk)
 	struct scsi_cd *cd = scsi_cd(disk);
 	struct scsi_sense_hdr sshdr;
 
+	scsi_autopm_get_device(cd->device);
+
 	/* if the unit is not ready, nothing more to do */
 	if (scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr))
-		return 0;
+		goto out;
 
 	sr_cd_check(&cd->cdi);
 	get_sectorsize(cd);
+out:
+	scsi_autopm_put_device(cd->device);
 	return 0;
 }
 
@@ -718,6 +750,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 +999,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.8.1

Thanks,
Aaron

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

[Aaron Lu]

On 01/20/2013 02:46 AM, Alan Stern wrote:
> On Sat, 19 Jan 2013, Aaron Lu wrote:
>> Then we indeed have a problem. But I didn't find any such app in
>> Fedora's repo or by searching the internet.
>
> http://rpm.pbone.net/index.php3/stat/4/idpl/2392183/dir/redhat_5.x/com/cdp-0.33-10.i386.rpm.html

Now that with the RFC PATCH, the cd drive's runtime status will always
be active if there is disc inside, so we don't have this problem any
more. Tested on two systems, one with a ZP-drive and the other with a
normal one.

BTW, I can't get the above app run, as it requires an old ncurses lib.
And compiling it is not easy...

For an amusement, I wrote the following stupid simple app intended for
test:

#include <sys/types.h>
#include <sys/stat.h>
#include <sys/ioctl.h>
#include <fcntl.h>
#include <linux/cdrom.h>

int main(void)
{
	int ret = 0;
	struct cdrom_msf msf = { 1, 0, 0, 2, 0, 0 };
	int fd;
	
	fd = open("/dev/sr0", O_RDONLY | O_NONBLOCK);
	if (fd == -1) {
		perror("open");
		return -1;
	}

	ret = ioctl(fd, CDROMPLAYMSF, &msf);
	if (ret) {
		perror("ioctl");
		goto out;
	}
out:
	close(fd);

	return ret;
}

Insert an audio disc, make sure no app is accessing it, run the above
app, then the app will exit and the led on the drive will be blinking
for roughly one minute, as the app has set, but I didn't hear any sound...

And the runtime status is of course active all the time, no matter if
there is app accessing it or not as expected.

Thanks,
Aaron

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

[Oliver Neukum]

On Monday 21 January 2013 17:11:04 Aaron Lu wrote:
> It is not easy for the OS to tell if the drive is being used or not
> sometimes 
> 
> Alan has reminded me it is possible for an app to open the block device
> file(/dev/sr0), issue a command(play audio), then close the device file.
> From the OS' point of view, we think nobody is using it. But actually,
> the drive is playing cd for the user, so we can't suspend the device.

Are there drives that support ZPODD and have an audio output?

	Regards
		Oliver

Re: [RFC PATCH] scsi: sr: support runtime pm

[Alan Stern]

On Mon, 21 Jan 2013, Aaron Lu wrote:

> On Sat, Jan 19, 2013 at 01:46:15PM -0500, Alan Stern wrote:
> > On Sat, 19 Jan 2013, Aaron Lu wrote:
> > > I don't think we should drop such support.
> > > And the safest way to avoid such break is we refine the suspend
> > > condition for ODD, and using what ZPODD defined condition isn't that
> > > bad to me:
> > > - for tray type, no media inside and tray close;
> > > - for slot type, no media inside.
> > > While whether tray is closed or not may not be that important, but at
> > > least we should make sure there is no media inside.
> > > 
> > > Thoughts?
> > 
> > That sounds reasonable to me, at least as a first step.  If people want 
> > their CD drive to suspend, they can eject the disc.
> 
> Here is an updated patch to address the problem, please review, thanks.
> 
> Changes to v13:
> - Add PM get/put pair functions to all the block device operation
>   functions; Move the existing PM get/put pair functions in
>   sr_check_events to sr_block_check_events;
> - Add sr_runtime_suspend, it will check if there is media inside and if
>   yes, avoid suspend.
> 
> From 378bf55810a1118ede481f45132b5c39af891d23 Mon Sep 17 00:00:00 2001
> From: Aaron Lu <aaron.lu@intel.com>
> Date: Wed, 26 Sep 2012 15:14:56 +0800
> Subject: [RFC PATCH] scsi: sr: support runtime pm
> 
> 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 accessed. And decreasing the runtime usage_count
> of the device when the access is done.
> 
> The idea is discussed here:
> http://thread.gmane.org/gmane.linux.acpi.devel/55243/focus=52703
> and here:
> http://thread.gmane.org/gmane.linux.ide/53665/focus=58836
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>

This looks good now.  When you submit the patch, you might want to 
mention the restriction about no media being present in the changelog 
entry.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

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

[Jeff Garzik]

On 01/15/2013 04:21 AM, Aaron Lu wrote:
> For ODDs, the upper layer will poll for media change every few
> seconds, which will make it enter and leave suspend state very
> often. And as each suspend will also cause a hard/soft reset,
> the gain of runtime suspend is very little while the ODD may
> malfunction 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>
> Acked-by: Tejun Heo <tj@kernel.org>
> ---
>   drivers/ata/libata-core.c | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)

applied patches #2-6 and #8

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

[Aaron Lu]

On Mon, Jan 21, 2013 at 03:56:43PM +0100, Oliver Neukum wrote:
> On Monday 21 January 2013 17:11:04 Aaron Lu wrote:
> > It is not easy for the OS to tell if the drive is being used or not
> > sometimes 
> > 
> > Alan has reminded me it is possible for an app to open the block device
> > file(/dev/sr0), issue a command(play audio), then close the device file.
> > From the OS' point of view, we think nobody is using it. But actually,
> > the drive is playing cd for the user, so we can't suspend the device.
> 
> Are there drives that support ZPODD and have an audio output?

I'm afraid I don't know, since there are so many ODD makers.
But at least we can say, the SPEC doesn't forbid it.

Thanks,
Aaron

Re: [RFC PATCH] scsi: sr: support runtime pm

[Aaron Lu]

On Mon, Jan 21, 2013 at 11:59:06AM -0500, Alan Stern wrote:
> On Mon, 21 Jan 2013, Aaron Lu wrote:
> 
> > On Sat, Jan 19, 2013 at 01:46:15PM -0500, Alan Stern wrote:
> > > On Sat, 19 Jan 2013, Aaron Lu wrote:
> > > > I don't think we should drop such support.
> > > > And the safest way to avoid such break is we refine the suspend
> > > > condition for ODD, and using what ZPODD defined condition isn't that
> > > > bad to me:
> > > > - for tray type, no media inside and tray close;
> > > > - for slot type, no media inside.
> > > > While whether tray is closed or not may not be that important, but at
> > > > least we should make sure there is no media inside.
> > > > 
> > > > Thoughts?
> > > 
> > > That sounds reasonable to me, at least as a first step.  If people want 
> > > their CD drive to suspend, they can eject the disc.
> > 
> > Here is an updated patch to address the problem, please review, thanks.
> > 
> > Changes to v13:
> > - Add PM get/put pair functions to all the block device operation
> >   functions; Move the existing PM get/put pair functions in
> >   sr_check_events to sr_block_check_events;
> > - Add sr_runtime_suspend, it will check if there is media inside and if
> >   yes, avoid suspend.
> > 
> > From 378bf55810a1118ede481f45132b5c39af891d23 Mon Sep 17 00:00:00 2001
> > From: Aaron Lu <aaron.lu@intel.com>
> > Date: Wed, 26 Sep 2012 15:14:56 +0800
> > Subject: [RFC PATCH] scsi: sr: support runtime pm
> > 
> > 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 accessed. And decreasing the runtime usage_count
> > of the device when the access is done.
> > 
> > The idea is discussed here:
> > http://thread.gmane.org/gmane.linux.acpi.devel/55243/focus=52703
> > and here:
> > http://thread.gmane.org/gmane.linux.ide/53665/focus=58836
> > 
> > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> 
> This looks good now.  When you submit the patch, you might want to 
> mention the restriction about no media being present in the changelog 
> entry.
> 
> Acked-by: Alan Stern <stern@rowland.harvard.edu>
> 

Will add that in the changelog, thanks a lot for your kind help.

-Aaron

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

[Oliver Neukum]

On Tuesday 22 January 2013 10:25:31 Aaron Lu wrote:
> On Mon, Jan 21, 2013 at 03:56:43PM +0100, Oliver Neukum wrote:
> > On Monday 21 January 2013 17:11:04 Aaron Lu wrote:
> > > It is not easy for the OS to tell if the drive is being used or not
> > > sometimes 
> > > 
> > > Alan has reminded me it is possible for an app to open the block device
> > > file(/dev/sr0), issue a command(play audio), then close the device file.
> > > From the OS' point of view, we think nobody is using it. But actually,
> > > the drive is playing cd for the user, so we can't suspend the device.
> > 
> > Are there drives that support ZPODD and have an audio output?
> 
> I'm afraid I don't know, since there are so many ODD makers.
> But at least we can say, the SPEC doesn't forbid it.

Well, then we have to handle it.

	Regards
		Oliver

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

[Aaron Lu]

On 01/22/2013 05:13 PM, Oliver Neukum wrote:
> On Tuesday 22 January 2013 10:25:31 Aaron Lu wrote:
>> On Mon, Jan 21, 2013 at 03:56:43PM +0100, Oliver Neukum wrote:
>>> On Monday 21 January 2013 17:11:04 Aaron Lu wrote:
>>>> It is not easy for the OS to tell if the drive is being used or not
>>>> sometimes 
>>>>
>>>> Alan has reminded me it is possible for an app to open the block device
>>>> file(/dev/sr0), issue a command(play audio), then close the device file.
>>>> From the OS' point of view, we think nobody is using it. But actually,
>>>> the drive is playing cd for the user, so we can't suspend the device.
>>>
>>> Are there drives that support ZPODD and have an audio output?
>>
>> I'm afraid I don't know, since there are so many ODD makers.
>> But at least we can say, the SPEC doesn't forbid it.
> 
> Well, then we have to handle it.

Yes, and the way we handle it is by checking the cd->media_present: if
it is true, we will not allow runtime suspend as shown in the RFC patch.
http://marc.info/?l=linux-scsi&m=135876099800714&w=2

Thanks,
Aaron

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

[Aaron Lu]

On Mon, Jan 21, 2013 at 03:42:55PM -0500, Jeff Garzik wrote:
> On 01/15/2013 04:21 AM, Aaron Lu wrote:
> >For ODDs, the upper layer will poll for media change every few
> >seconds, which will make it enter and leave suspend state very
> >often. And as each suspend will also cause a hard/soft reset,
> >the gain of runtime suspend is very little while the ODD may
> >malfunction 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>
> >Acked-by: Tejun Heo <tj@kernel.org>
> >---
> >  drivers/ata/libata-core.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> 
> applied patches #2-6 and #8

Thanks!

Now that patch 1 has ack from Alan Stern(and thus James), and consider
the smatch warning of patch #3, I can either send out the remaining 3
patches(#1, #7 and #9) together with a small fix patch for the smatch
warning or I can re-send the whole patchset. Just let me know which way
you prefer, thanks.

-Aaron