[PATCH v6 0/7] ZPODD patches

[PATCH v6 0/7] ZPODD patches

[Aaron Lu]

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.

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

 block/genhd.c              |  2 ++
 drivers/ata/libata-acpi.c  | 35 ++++++++++++------
 drivers/scsi/scsi_sysfs.c  | 37 ++++++++++++++++++-
 drivers/scsi/sr.c          | 89 ++++++++++++++++++++++++++++++++++++++++++++--
 drivers/scsi/sr.h          |  1 +
 include/scsi/scsi_device.h |  2 ++
 6 files changed, 152 insertions(+), 14 deletions(-)

-- 
1.7.11.3

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

[Aaron Lu]

From: Aaron Lu <aaron.lu@intel.com>

The ODD will be placed into suspend state when:
1 For tray type ODD, no media inside and door closed;
2 For slot type ODD, no media inside;
And together with ACPI, when we suspend the ODD's parent(the port it
attached to), we will omit the power altogether to reduce power
consumption(done in libata-acpi.c).

The ODD can be resumed either by user or by software.

For user to resume the suspended ODD:
1 For tray type ODD, press the eject button;
2 For slot type ODD, insert a disc;
Once such events happened, an ACPI notification will be sent and in our
handler, we will resume the ODD(again in libata-acpi.c).
This kind of resume requires platform and device support and is
reflected by the can_power_off flag.

For software to resume the suspended ODD, we did this in ODD's
open/release and check_event function.

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

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 902b5a4..64ba43d 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -985,8 +985,10 @@ static void ata_acpi_wake_dev(acpi_handle handle, u32 event, void *context)
 	struct ata_device *ata_dev = context;
 
 	if (event == ACPI_NOTIFY_DEVICE_WAKE && ata_dev &&
-			pm_runtime_suspended(&ata_dev->sdev->sdev_gendev))
-		scsi_autopm_get_device(ata_dev->sdev);
+			pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) {
+		ata_dev->sdev->wakeup_by_user = 1;
+		pm_runtime_resume(&ata_dev->sdev->sdev_gendev);
+	}
 }
 
 static void ata_acpi_add_pm_notifier(struct ata_device *dev)
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 5fc97d2..637f4ca 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -45,6 +45,7 @@
 #include <linux/blkdev.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>
@@ -79,6 +80,8 @@ static DEFINE_MUTEX(sr_mutex);
 static int sr_probe(struct device *);
 static int sr_remove(struct device *);
 static int sr_done(struct scsi_cmnd *);
+static int sr_suspend(struct device *, pm_message_t msg);
+static int sr_resume(struct device *);
 
 static struct scsi_driver sr_template = {
 	.owner			= THIS_MODULE,
@@ -86,6 +89,8 @@ static struct scsi_driver sr_template = {
 		.name   	= "sr",
 		.probe		= sr_probe,
 		.remove		= sr_remove,
+		.suspend	= sr_suspend,
+		.resume		= sr_resume,
 	},
 	.done			= sr_done,
 };
@@ -146,8 +151,12 @@ static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
 	kref_get(&cd->kref);
 	if (scsi_device_get(cd->device))
 		goto out_put;
+	if (scsi_autopm_get_device(cd->device))
+		goto out_pm;
 	goto out;
 
+ out_pm:
+	scsi_device_put(cd->device);
  out_put:
 	kref_put(&cd->kref, sr_kref_release);
 	cd = NULL;
@@ -163,9 +172,61 @@ static void scsi_cd_put(struct scsi_cd *cd)
 	mutex_lock(&sr_ref_mutex);
 	kref_put(&cd->kref, sr_kref_release);
 	scsi_device_put(sdev);
+	scsi_autopm_put_device(sdev);
 	mutex_unlock(&sr_ref_mutex);
 }
 
+static int sr_suspend(struct device *dev, pm_message_t msg)
+{
+	int suspend;
+	struct scsi_sense_hdr sshdr;
+	struct scsi_cd *cd = dev_get_drvdata(dev);
+
+	/* no action for system pm */
+	if (!PMSG_IS_AUTO(msg))
+		return 0;
+
+	/*
+	 * ODD can be runtime suspended when:
+	 * tray type: no media inside and tray closed
+	 * slot type: no media inside
+	 */
+	scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
+
+	if (cd->cdi.mask & CDC_CLOSE_TRAY)
+		/* no media for caddy/slot type ODD */
+		suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a;
+	else
+		/* no media and door closed for tray type ODD */
+		suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a &&
+			sshdr.ascq == 0x01;
+
+	if (!suspend)
+		return -EBUSY;
+
+	return 0;
+}
+
+static int sr_resume(struct device *dev)
+{
+	struct scsi_cd *cd;
+	struct scsi_sense_hdr sshdr;
+
+	cd = dev_get_drvdata(dev);
+
+	/* get the disk ready */
+	scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
+
+	/* if user wakes up the ODD, eject the tray */
+	if (cd->device->wakeup_by_user) {
+		cd->device->wakeup_by_user = 0;
+		if (!(cd->cdi.mask & CDC_CLOSE_TRAY))
+			sr_tray_move(&cd->cdi, 1);
+	}
+
+	return 0;
+}
+
 static unsigned int sr_get_events(struct scsi_device *sdev)
 {
 	u8 buf[8];
@@ -220,6 +281,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,7 +309,7 @@ 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;
@@ -270,7 +333,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 +350,8 @@ do_tur:
 	cd->tur_changed = false;
 	cd->get_event_changed = false;
 
+out:
+	scsi_autopm_put_device(cd->device);
 	return events;
 }
 
@@ -718,6 +783,10 @@ static int sr_probe(struct device *dev)
 
 	sdev_printk(KERN_DEBUG, sdev,
 		    "Attached scsi CD-ROM %s\n", cd->cdi.name);
+
+	/* enable runtime pm */
+	scsi_autopm_put_device(cd->device);
+
 	return 0;
 
 fail_put:
@@ -965,6 +1034,9 @@ static int sr_remove(struct device *dev)
 {
 	struct scsi_cd *cd = dev_get_drvdata(dev);
 
+	/* disable runtime pm */
+	scsi_autopm_get_device(cd->device);
+
 	blk_queue_prep_rq(cd->device->request_queue, scsi_prep_fn);
 	del_gendisk(cd->disk);
 
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 9895f69..72d946f 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -156,6 +156,7 @@ struct scsi_device {
 	unsigned is_visible:1;	/* is the device visible in sysfs */
 	unsigned can_power_off:1; /* Device supports runtime power off */
 	unsigned wce_default_on:1;	/* Cache is ON by default */
+	unsigned wakeup_by_user:1;	/* User wakes up the device */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */
-- 
1.7.11.3

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

[Aaron Lu]

From: Aaron Lu <aaron.lu@intel.com>

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

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

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

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

[Aaron Lu]

From: Aaron Lu <aaron.lu@intel.com>

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

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

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

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

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

[Aaron Lu]

From: Aaron Lu <aaron.lu@intel.com>

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

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

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

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

[Aaron Lu]

From: Aaron Lu <aaron.lu@intel.com>

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

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

When user changes this flag through sysfs entry and if the device is
already runtime suspended, runtime resume it so that it can respect this
flag next time it is runtime suspended.

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

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

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 093d4f6..8c8efd3 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -509,6 +509,7 @@ sdev_store_##field (struct device *dev, struct device_attribute *attr,	\
 	return ret;							\
 }									\
 static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, sdev_show_##field, sdev_store_##field);
+#endif
 
 /*
  * scsi_sdev_check_buf_bit: return 0 if buf is "0", return 1 if buf is "1",
@@ -526,7 +527,7 @@ static int scsi_sdev_check_buf_bit(const char *buf)
 	} else
 		return -EINVAL;
 }
-#endif
+
 /*
  * Create the actual show/store functions and data structures.
  */
@@ -860,6 +861,37 @@ static struct device_attribute sdev_attr_queue_type_rw =
 	__ATTR(queue_type, S_IRUGO | S_IWUSR, show_queue_type_field,
 	       sdev_store_queue_type_rw);
 
+static ssize_t
+sdev_show_may_power_off(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct scsi_device *sdev;
+	sdev = to_scsi_device(dev);
+	return snprintf (buf, 20, "%d\n", sdev->may_power_off);
+}
+
+static ssize_t
+sdev_store_may_power_off(struct device *dev, struct device_attribute *attr,
+			 const char *buf, size_t count)
+{
+	int ret;
+	struct scsi_device *sdev;
+
+	ret = scsi_sdev_check_buf_bit(buf);
+	if (ret >= 0) {
+		sdev = to_scsi_device(dev);
+		if (sdev->may_power_off != ret) {
+			sdev->may_power_off = ret;
+			if (pm_runtime_suspended(dev))
+				pm_runtime_resume(dev);
+		}
+		ret = count;
+	}
+	return ret;
+}
+static DEVICE_ATTR(may_power_off, S_IRUGO | S_IWUSR,
+			sdev_show_may_power_off, sdev_store_may_power_off);
+
 /**
  * scsi_sysfs_add_sdev - add scsi device to sysfs
  * @sdev:	scsi_device to add
@@ -950,6 +982,9 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 		}
 	}
 
+	if (sdev->can_power_off)
+		device_create_file(&sdev->sdev_gendev, &dev_attr_may_power_off);
+
 	return error;
 }
 
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 72d946f..41cec7f 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -157,6 +157,7 @@ struct scsi_device {
 	unsigned can_power_off:1; /* Device supports runtime power off */
 	unsigned wce_default_on:1;	/* Cache is ON by default */
 	unsigned wakeup_by_user:1;	/* User wakes up the device */
+	unsigned may_power_off:1;	/* Power off is allowed by user */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */
-- 
1.7.11.3

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

[Aaron Lu]

From: Aaron Lu <aaron.lu@intel.com>

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

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

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

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

[Aaron Lu]

From: Aaron Lu <aaron.lu@intel.com>

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

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

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

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

[Oliver Neukum]

On Tuesday 04 September 2012 22:24:34 Aaron Lu wrote:
> From: Aaron Lu <aaron.lu@intel.com>
> 
> The ODD will be placed into suspend state when:
> 1 For tray type ODD, no media inside and door closed;
> 2 For slot type ODD, no media inside;
> And together with ACPI, when we suspend the ODD's parent(the port it
> attached to), we will omit the power altogether to reduce power
> consumption(done in libata-acpi.c).

Well, this is quite a layering violation. You encode that specific requirement
in the generic sr_suspend()

> The ODD can be resumed either by user or by software.
> 
> For user to resume the suspended ODD:
> 1 For tray type ODD, press the eject button;
> 2 For slot type ODD, insert a disc;
> Once such events happened, an ACPI notification will be sent and in our
> handler, we will resume the ODD(again in libata-acpi.c).
> This kind of resume requires platform and device support and is
> reflected by the can_power_off flag.
> 
> For software to resume the suspended ODD, we did this in ODD's
> open/release and check_event function.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  drivers/ata/libata-acpi.c  |  6 ++--
>  drivers/scsi/sr.c          | 76 ++++++++++++++++++++++++++++++++++++++++++++--
>  include/scsi/scsi_device.h |  1 +
>  3 files changed, 79 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index 902b5a4..64ba43d 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -985,8 +985,10 @@ static void ata_acpi_wake_dev(acpi_handle handle, u32 event, void *context)
>  	struct ata_device *ata_dev = context;
>  
>  	if (event == ACPI_NOTIFY_DEVICE_WAKE && ata_dev &&
> -			pm_runtime_suspended(&ata_dev->sdev->sdev_gendev))
> -		scsi_autopm_get_device(ata_dev->sdev);
> +			pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) {
> +		ata_dev->sdev->wakeup_by_user = 1;

That flag is badly named. Something like "insert_event_during_suspend"
would be better.

[..]
> +static int sr_suspend(struct device *dev, pm_message_t msg)
> +{
> +	int suspend;
> +	struct scsi_sense_hdr sshdr;
> +	struct scsi_cd *cd = dev_get_drvdata(dev);
> +
> +	/* no action for system pm */
> +	if (!PMSG_IS_AUTO(msg))
> +		return 0;
> +
> +	/*
> +	 * ODD can be runtime suspended when:
> +	 * tray type: no media inside and tray closed
> +	 * slot type: no media inside
> +	 */
> +	scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
> +
> +	if (cd->cdi.mask & CDC_CLOSE_TRAY)
> +		/* no media for caddy/slot type ODD */
> +		suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a;
> +	else
> +		/* no media and door closed for tray type ODD */
> +		suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a &&
> +			sshdr.ascq == 0x01;

That requirement is valid for a specific type of disk. You cannot put it
into generic sd_suspend(). And even so I don't see why you wouldn't
want to suspend for example a drive with an inserted but unopened disk.

> +
> +	if (!suspend)
> +		return -EBUSY;
> +
> +	return 0;
> +}

	Regards
		Oliver

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

[Oliver Neukum]

On Tuesday 04 September 2012 22:24:38 Aaron Lu wrote:
> From: Aaron Lu <aaron.lu@intel.com>
> 
> Add a new flag may_power_off for scsi device, it gives the user a chance
> to control when the device is runtime suspended, can we remove its power
> if possible.
> 
> And if the device can be powered off(reflected by can_power_off flag,
> determined by individual driver), create a sysfs entry named
> may_power_off to let user control the flag.

Do we really need yet another interface to user space?

	Regards
		Oliver

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

[Alan Stern]

On Tue, 4 Sep 2012, Oliver Neukum wrote:

> On Tuesday 04 September 2012 22:24:34 Aaron Lu wrote:
> > From: Aaron Lu <aaron.lu@intel.com>
> > 
> > The ODD will be placed into suspend state when:
> > 1 For tray type ODD, no media inside and door closed;
> > 2 For slot type ODD, no media inside;
> > And together with ACPI, when we suspend the ODD's parent(the port it
> > attached to), we will omit the power altogether to reduce power
> > consumption(done in libata-acpi.c).
> 
> Well, this is quite a layering violation. You encode that specific requirement
> in the generic sr_suspend()

What requirement are you talking about?  The "no media and door closed" 
thing?  How is that a layering violation?  Are you suggesting this 
should go into the CD layer instead?

> > @@ -985,8 +985,10 @@ static void ata_acpi_wake_dev(acpi_handle handle, u32 event, void *context)
> >  	struct ata_device *ata_dev = context;
> >  
> >  	if (event == ACPI_NOTIFY_DEVICE_WAKE && ata_dev &&
> > -			pm_runtime_suspended(&ata_dev->sdev->sdev_gendev))
> > -		scsi_autopm_get_device(ata_dev->sdev);
> > +			pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) {
> > +		ata_dev->sdev->wakeup_by_user = 1;
> 
> That flag is badly named. Something like "insert_event_during_suspend"
> would be better.

What happens on non-ACPI systems when a new disc is inserted into a
suspended ODD?  How does the drive let the computer know that an insert
event has occurred?

> > +	if (cd->cdi.mask & CDC_CLOSE_TRAY)
> > +		/* no media for caddy/slot type ODD */
> > +		suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a;
> > +	else
> > +		/* no media and door closed for tray type ODD */
> > +		suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a &&
> > +			sshdr.ascq == 0x01;
> 
> That requirement is valid for a specific type of disk. You cannot put it
> into generic sd_suspend().

You mean sr_suspend().  What's not generic about it?

>  And even so I don't see why you wouldn't
> want to suspend for example a drive with an inserted but unopened disk.

I assume that Aaron wanted to handle the easiest case first.  Adding 
suspend/resume handling to the open/close routines can be done later.

Alan Stern

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

[Oliver Neukum]

On Tuesday 04 September 2012 13:59:38 Alan Stern wrote:
> On Tue, 4 Sep 2012, Oliver Neukum wrote:
> 
> > On Tuesday 04 September 2012 22:24:34 Aaron Lu wrote:
> > > From: Aaron Lu <aaron.lu@intel.com>
> > > 
> > > The ODD will be placed into suspend state when:
> > > 1 For tray type ODD, no media inside and door closed;
> > > 2 For slot type ODD, no media inside;
> > > And together with ACPI, when we suspend the ODD's parent(the port it
> > > attached to), we will omit the power altogether to reduce power
> > > consumption(done in libata-acpi.c).
> > 
> > Well, this is quite a layering violation. You encode that specific requirement
> > in the generic sr_suspend()
> 
> What requirement are you talking about?  The "no media and door closed" 
> thing?  How is that a layering violation?  Are you suggesting this

There is no reason this requirement should apply to any other drive than the
device this is aimed at. It comes from the ability of this specific combination
to detect medium changes.

> should go into the CD layer instead?

No. It needs to be specific to a certain hardware. It needs to be a callback.
 
> > > @@ -985,8 +985,10 @@ static void ata_acpi_wake_dev(acpi_handle handle, u32 event, void *context)
> > >  	struct ata_device *ata_dev = context;
> > >  
> > >  	if (event == ACPI_NOTIFY_DEVICE_WAKE && ata_dev &&
> > > -			pm_runtime_suspended(&ata_dev->sdev->sdev_gendev))
> > > -		scsi_autopm_get_device(ata_dev->sdev);
> > > +			pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) {
> > > +		ata_dev->sdev->wakeup_by_user = 1;
> > 
> > That flag is badly named. Something like "insert_event_during_suspend"
> > would be better.
> 
> What happens on non-ACPI systems when a new disc is inserted into a
> suspended ODD?  How does the drive let the computer know that an insert
> event has occurred?

Good question. Again either the kernel polls the drive or there a mechanism
specific to the hardware.
 
> > > +	if (cd->cdi.mask & CDC_CLOSE_TRAY)
> > > +		/* no media for caddy/slot type ODD */
> > > +		suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a;
> > > +	else
> > > +		/* no media and door closed for tray type ODD */
> > > +		suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a &&
> > > +			sshdr.ascq == 0x01;
> > 
> > That requirement is valid for a specific type of disk. You cannot put it
> > into generic sd_suspend().
> 
> You mean sr_suspend().  What's not generic about it?

Yes. We may encounter drives which cannot register a medium change while
suspended, but can be safely suspended while their door is locked.

> >  And even so I don't see why you wouldn't
> > want to suspend for example a drive with an inserted but unopened disk.
> 
> I assume that Aaron wanted to handle the easiest case first.  Adding 
> suspend/resume handling to the open/close routines can be done later.

Sure, but this patch blocks that in contrast to just not implementing it.

	Regards
		Oliver

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

[Aaron Lu]

On Tue, Sep 04, 2012 at 08:47:15PM +0200, Oliver Neukum wrote:
> On Tuesday 04 September 2012 13:59:38 Alan Stern wrote:
> > On Tue, 4 Sep 2012, Oliver Neukum wrote:
> > 
> > > On Tuesday 04 September 2012 22:24:34 Aaron Lu wrote:
> > > > From: Aaron Lu <aaron.lu@intel.com>
> > > > 
> > > > The ODD will be placed into suspend state when:
> > > > 1 For tray type ODD, no media inside and door closed;
> > > > 2 For slot type ODD, no media inside;
> > > > And together with ACPI, when we suspend the ODD's parent(the port it
> > > > attached to), we will omit the power altogether to reduce power
> > > > consumption(done in libata-acpi.c).
> > > 
> > > Well, this is quite a layering violation. You encode that specific requirement
> > > in the generic sr_suspend()
> > 
> > What requirement are you talking about?  The "no media and door closed" 
> > thing?  How is that a layering violation?  Are you suggesting this
> 
> There is no reason this requirement should apply to any other drive than the
> device this is aimed at. It comes from the ability of this specific combination
> to detect medium changes.

When suspending, I'll check the "no media inside and door closed"
condition. Do you mean this is a special ability for devices driven by
sr driver?

And after the device is runtime suspended, if the platform has the
ability to power off the device and the device supports notifying the
host of user action(i.e inserting a disc to a slot type ODD or pressing
the eject button of a tray type ODD), it will be powered off to save
more power. This is ZPODD device.

By detecting medium change, I suppose you mean when the device is
runtime suspended, how can it detect medium change?
There are 2 situations here:
1 For normal devices, the in kernel poll is still going on;
2 For ZPODD device, it will be powered off when runtime suspended.
And the in kernel poll is stopped to let the device stay in powered off
state longer(done in patch 3). when user wants to use the ODD by either
inserting a disc or pressing the eject button, an ACPI event is
generated and the notification code will runtime resume the device and
the in kernel poll is restarted. And then the medium change can be
detected.

> 
> > should go into the CD layer instead?
> 
> No. It needs to be specific to a certain hardware. It needs to be a callback.
>  
> > > > @@ -985,8 +985,10 @@ static void ata_acpi_wake_dev(acpi_handle handle, u32 event, void *context)
> > > >  	struct ata_device *ata_dev = context;
> > > >  
> > > >  	if (event == ACPI_NOTIFY_DEVICE_WAKE && ata_dev &&
> > > > -			pm_runtime_suspended(&ata_dev->sdev->sdev_gendev))
> > > > -		scsi_autopm_get_device(ata_dev->sdev);
> > > > +			pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) {
> > > > +		ata_dev->sdev->wakeup_by_user = 1;
> > > 
> > > That flag is badly named. Something like "insert_event_during_suspend"
> > > would be better.

This flag means, the reason the device gets runtime resumed is due to
user as described by the above situation 2, not by software(e.g. by
sr_check_events).

I don't quite understand insert_event_during_suspend mean here...

> > 
> > What happens on non-ACPI systems when a new disc is inserted into a
> > suspended ODD?  How does the drive let the computer know that an insert
> > event has occurred?
> 
> Good question. Again either the kernel polls the drive or there a mechanism
> specific to the hardware.

Yes. And the mechanism is called ZPODD :-)
And if ZPODD is supported, I'll block kernel polling; If not, kernel
polling is not blocked.

>  
> > > > +	if (cd->cdi.mask & CDC_CLOSE_TRAY)
> > > > +		/* no media for caddy/slot type ODD */
> > > > +		suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a;
> > > > +	else
> > > > +		/* no media and door closed for tray type ODD */
> > > > +		suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a &&
> > > > +			sshdr.ascq == 0x01;
> > > 
> > > That requirement is valid for a specific type of disk. You cannot put it
> > > into generic sd_suspend().
> > 
> > You mean sr_suspend().  What's not generic about it?
> 
> Yes. We may encounter drives which cannot register a medium change while
> suspended, but can be safely suspended while their door is locked.

There is no need to register a medium change while suspended.
The current model of periodically poll is not changed.
Only when platform and device both support ZPODD will the in kernel poll
be stopped.

> 
> > >  And even so I don't see why you wouldn't
> > > want to suspend for example a drive with an inserted but unopened disk.
> > 
> > I assume that Aaron wanted to handle the easiest case first.  Adding 
> > suspend/resume handling to the open/close routines can be done later.
> 
> Sure, but this patch blocks that in contrast to just not implementing it.

It's already implemented. I did it in scsi_cd_get/put.

And the reason I didn't allow runtime suspend for medium inside and door
closed case are:
1 ZPODD has a spec and it didn't allow that;
2 It's not easy to implement. Imagine user just inserts a disc, and the
sr_suspend routine checks that door is closed so that it wants to
suspend the device. But actually, after user just inserts a disc, he
definitely wants to use the device, so it's not a good thing to do. And
if ZPODD is used, the device will be powered off instantly when the door
is closed, this is not good.

So I think for now, the "no medium inside and door closed" makes thing
easier :-)

Thanks,
Aaron

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

[Alan Stern]

On Wed, 5 Sep 2012, Aaron Lu wrote:

> > > > That flag is badly named. Something like "insert_event_during_suspend"
> > > > would be better.
> 
> This flag means, the reason the device gets runtime resumed is due to
> user as described by the above situation 2, not by software(e.g. by
> sr_check_events).
> 
> I don't quite understand insert_event_during_suspend mean here...

It means: An insert event occurred while the device was suspended.

> > > >  And even so I don't see why you wouldn't
> > > > want to suspend for example a drive with an inserted but unopened disk.
> > > 
> > > I assume that Aaron wanted to handle the easiest case first.  Adding 
> > > suspend/resume handling to the open/close routines can be done later.
> > 
> > Sure, but this patch blocks that in contrast to just not implementing it.
> 
> It's already implemented. I did it in scsi_cd_get/put.
> 
> And the reason I didn't allow runtime suspend for medium inside and door
> closed case are:
> 1 ZPODD has a spec and it didn't allow that;

In theory we should allow runtime suspend in this case too, but leave 
the power on.  (Although in practice, turning the power off would 
probably work even though it may violate the spec.)

> 2 It's not easy to implement. Imagine user just inserts a disc, and the
> sr_suspend routine checks that door is closed so that it wants to
> suspend the device. But actually, after user just inserts a disc, he
> definitely wants to use the device, so it's not a good thing to do. And
> if ZPODD is used, the device will be powered off instantly when the door
> is closed, this is not good.

That's why we have an autosuspend delay.  Although for some reason the 
SCSI subsystem doesn't use it currently...  We need to add a call to 
pm_runtime_use_autosuspend() in scsi_sysfs_add_sdev().  Likewise, the 
pm_schedule_suspend() call in scsi_runtime_idle() should be changed to 
pm_runtime_autosuspend().  And there should be calls to 
pm_runtime_set_autosuspend_delay() in the sd and sr drivers.

Alan Stern

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

[Oliver Neukum]

On Wednesday 05 September 2012 23:22:55 Aaron Lu wrote:
> On Tue, Sep 04, 2012 at 08:47:15PM +0200, Oliver Neukum wrote:
> > On Tuesday 04 September 2012 13:59:38 Alan Stern wrote:

> > > What requirement are you talking about?  The "no media and door closed" 
> > > thing?  How is that a layering violation?  Are you suggesting this
> > 
> > There is no reason this requirement should apply to any other drive than the
> > device this is aimed at. It comes from the ability of this specific combination
> > to detect medium changes.
> 
> When suspending, I'll check the "no media inside and door closed"
> condition. Do you mean this is a special ability for devices driven by
> sr driver?

No. This is a special requirement for devices supporting ZPODD.
ZPODD are driven by sr. By sr drives many devices which are not sr.
For them doing the check you need to do for ZPODD devices
is wrong.
 
> And after the device is runtime suspended, if the platform has the
> ability to power off the device and the device supports notifying the
> host of user action(i.e inserting a disc to a slot type ODD or pressing
> the eject button of a tray type ODD), it will be powered off to save
> more power. This is ZPODD device.

Sure. I am not saying that your patches are wrong for ZPODD devices.
The problem is with devices that are not ZPODD.

> > > What happens on non-ACPI systems when a new disc is inserted into a
> > > suspended ODD?  How does the drive let the computer know that an insert
> > > event has occurred?
> > 
> > Good question. Again either the kernel polls the drive or there a mechanism
> > specific to the hardware.
> 
> Yes. And the mechanism is called ZPODD :-)

No. There is a mechanism known as ZPODD. There may be others.

> And the reason I didn't allow runtime suspend for medium inside and door
> closed case are:
> 1 ZPODD has a spec and it didn't allow that;

Yet there are drives which are not ZPODD.

> 2 It's not easy to implement. Imagine user just inserts a disc, and the
> sr_suspend routine checks that door is closed so that it wants to
> suspend the device. But actually, after user just inserts a disc, he
> definitely wants to use the device, so it's not a good thing to do. And
> if ZPODD is used, the device will be powered off instantly when the door
> is closed, this is not good.

Therefore you use a reasonable delay in the driver core.

	Regards
		Oliver

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

[Aaron Lu]

On Wed, Sep 05, 2012 at 12:08:25PM -0400, Alan Stern wrote:
> On Wed, 5 Sep 2012, Aaron Lu wrote:
> 
> > > > > That flag is badly named. Something like "insert_event_during_suspend"
> > > > > would be better.
> > 
> > This flag means, the reason the device gets runtime resumed is due to
> > user as described by the above situation 2, not by software(e.g. by
> > sr_check_events).
> > 
> > I don't quite understand insert_event_during_suspend mean here...
> 
> It means: An insert event occurred while the device was suspended.

Thanks, I got it now.

> 
> > > > >  And even so I don't see why you wouldn't
> > > > > want to suspend for example a drive with an inserted but unopened disk.
> > > > 
> > > > I assume that Aaron wanted to handle the easiest case first.  Adding 
> > > > suspend/resume handling to the open/close routines can be done later.
> > > 
> > > Sure, but this patch blocks that in contrast to just not implementing it.
> > 
> > It's already implemented. I did it in scsi_cd_get/put.
> > 
> > And the reason I didn't allow runtime suspend for medium inside and door
> > closed case are:
> > 1 ZPODD has a spec and it didn't allow that;
> 
> In theory we should allow runtime suspend in this case too, but leave 
> the power on.  (Although in practice, turning the power off would 
> probably work even though it may violate the spec.)
> 

Agree. But it's not easy to implement with autosuspend.
Please see below.

> > 2 It's not easy to implement. Imagine user just inserts a disc, and the
> > sr_suspend routine checks that door is closed so that it wants to
> > suspend the device. But actually, after user just inserts a disc, he
> > definitely wants to use the device, so it's not a good thing to do. And
> > if ZPODD is used, the device will be powered off instantly when the door
> > is closed, this is not good.
> 
> That's why we have an autosuspend delay.  Although for some reason the 
> SCSI subsystem doesn't use it currently...  We need to add a call to 
> pm_runtime_use_autosuspend() in scsi_sysfs_add_sdev().  Likewise, the 
> pm_schedule_suspend() call in scsi_runtime_idle() should be changed to 
> pm_runtime_autosuspend().  And there should be calls to 
> pm_runtime_set_autosuspend_delay() in the sd and sr drivers.

I tried to use autosuspend when preparing the patch, but the fact that
the devices will be polled every 2 seconds make it impossible to enter
suspend state if the autosuspend delay is larger than that.

-Aaron

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

[Aaron Lu]

On Tue, Sep 04, 2012 at 06:01:22PM +0200, Oliver Neukum wrote:
> On Tuesday 04 September 2012 22:24:38 Aaron Lu wrote:
> > From: Aaron Lu <aaron.lu@intel.com>
> > 
> > Add a new flag may_power_off for scsi device, it gives the user a chance
> > to control when the device is runtime suspended, can we remove its power
> > if possible.
> > 
> > And if the device can be powered off(reflected by can_power_off flag,
> > determined by individual driver), create a sysfs entry named
> > may_power_off to let user control the flag.
> 
> Do we really need yet another interface to user space?

We do not have a way to let user decide when device is runtime
suspended, can we remove its power, so I added this flag. PCI subsystem
has a similar flag named d3cold_allowed.

This flag can disable the power off of ZPODD capable devices, or put a
rotational disk into powered off if user so choose.

Thanks,
Aaron

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

[Aaron Lu]

On Tue, Sep 04, 2012 at 05:57:35PM +0200, Oliver Neukum wrote:
> > diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> > index 902b5a4..64ba43d 100644
> > --- a/drivers/ata/libata-acpi.c
> > +++ b/drivers/ata/libata-acpi.c
> > @@ -985,8 +985,10 @@ static void ata_acpi_wake_dev(acpi_handle handle, u32 event, void *context)
> >  	struct ata_device *ata_dev = context;
> >  
> >  	if (event == ACPI_NOTIFY_DEVICE_WAKE && ata_dev &&
> > -			pm_runtime_suspended(&ata_dev->sdev->sdev_gendev))
> > -		scsi_autopm_get_device(ata_dev->sdev);
> > +			pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) {
> > +		ata_dev->sdev->wakeup_by_user = 1;
> 
> That flag is badly named. Something like "insert_event_during_suspend"
> would be better.

The reason I added the flag is to let sr knows that when the device
gets resumed, please eject its tray since user just pressed the eject
button when it is in powered off state.

If wakeup_by_user is badly named, what about need_eject? I don't think
insert_event_during_suspend reflects what happened here.

-Aaron

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

[Aaron Lu]

On Wed, Sep 05, 2012 at 08:06:27PM +0200, Oliver Neukum wrote:
> > 2 It's not easy to implement. Imagine user just inserts a disc, and the
> > sr_suspend routine checks that door is closed so that it wants to
> > suspend the device. But actually, after user just inserts a disc, he
> > definitely wants to use the device, so it's not a good thing to do. And
> > if ZPODD is used, the device will be powered off instantly when the door
> > is closed, this is not good.
> 
> Therefore you use a reasonable delay in the driver core.

As I said in another mail to Alan, the device will not be able to
suspend due to the in kernel poll that occurred every 2s if the delay
is larger than that.

And here is a draft version for sr runtime pm that tries to use callback,
for normal ROM type and WORM type devices, there doesn't seem be
anything stopping them from suspending, so I just return 0 in their
suspend routine, they can be put to runtime suspended no matter if
the door is open or close, medium is inside or not, etc.

Please let me know what do you think, thanks.

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 902b5a4..cc3bc29 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -985,8 +985,10 @@ static void ata_acpi_wake_dev(acpi_handle handle, u32 event, void *context)
 	struct ata_device *ata_dev = context;
 
 	if (event == ACPI_NOTIFY_DEVICE_WAKE && ata_dev &&
-			pm_runtime_suspended(&ata_dev->sdev->sdev_gendev))
-		scsi_autopm_get_device(ata_dev->sdev);
+			pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) {
+		ata_dev->sdev->need_eject = 1;
+		pm_runtime_resume(&ata_dev->sdev->sdev_gendev);
+	}
 }
 
 static void ata_acpi_add_pm_notifier(struct ata_device *dev)
diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index 888f73a..fe36db9 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -176,7 +176,7 @@ scsi_tgt-y			+= scsi_tgt_lib.o scsi_tgt_if.o
 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-objs	:= sr.o sr_ioctl.o sr_vendor.o sr_pm_ops.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.c b/drivers/scsi/sr.c
index 5fc97d2..e4cbd6d 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -45,6 +45,7 @@
 #include <linux/blkdev.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>
@@ -79,6 +80,8 @@ static DEFINE_MUTEX(sr_mutex);
 static int sr_probe(struct device *);
 static int sr_remove(struct device *);
 static int sr_done(struct scsi_cmnd *);
+static int sr_suspend(struct device *, pm_message_t msg);
+static int sr_resume(struct device *);
 
 static struct scsi_driver sr_template = {
 	.owner			= THIS_MODULE,
@@ -86,6 +89,8 @@ static struct scsi_driver sr_template = {
 		.name   	= "sr",
 		.probe		= sr_probe,
 		.remove		= sr_remove,
+		.suspend	= sr_suspend,
+		.resume		= sr_resume,
 	},
 	.done			= sr_done,
 };
@@ -146,8 +151,12 @@ static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
 	kref_get(&cd->kref);
 	if (scsi_device_get(cd->device))
 		goto out_put;
+	if (scsi_autopm_get_device(cd->device))
+		goto out_pm;
 	goto out;
 
+ out_pm:
+	scsi_device_put(cd->device);
  out_put:
 	kref_put(&cd->kref, sr_kref_release);
 	cd = NULL;
@@ -163,9 +172,28 @@ static void scsi_cd_put(struct scsi_cd *cd)
 	mutex_lock(&sr_ref_mutex);
 	kref_put(&cd->kref, sr_kref_release);
 	scsi_device_put(sdev);
+	scsi_autopm_put_device(sdev);
 	mutex_unlock(&sr_ref_mutex);
 }
 
+static int sr_suspend(struct device *dev, pm_message_t msg)
+{
+	struct scsi_cd *cd;
+
+	/* no action for system pm */
+	if (!PMSG_IS_AUTO(msg))
+		return 0;
+
+	cd = dev_get_drvdata(dev);
+	return cd->pm_ops->suspend(dev);
+}
+
+static int sr_resume(struct device *dev)
+{
+	struct scsi_cd *cd = dev_get_drvdata(dev);
+	return cd->pm_ops->resume(dev);
+}
+
 static unsigned int sr_get_events(struct scsi_device *sdev)
 {
 	u8 buf[8];
@@ -220,6 +248,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,7 +276,7 @@ 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;
@@ -270,7 +300,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 +317,8 @@ do_tur:
 	cd->tur_changed = false;
 	cd->get_event_changed = false;
 
+out:
+	scsi_autopm_put_device(cd->device);
 	return events;
 }
 
@@ -702,6 +734,7 @@ static int sr_probe(struct device *dev)
 	get_capabilities(cd);
 	blk_queue_prep_rq(sdev->request_queue, sr_prep_fn);
 	sr_vendor_init(cd);
+	sr_pm_ops_init(cd);
 
 	disk->driverfs_dev = &sdev->sdev_gendev;
 	set_capacity(disk, cd->capacity);
@@ -718,6 +751,10 @@ static int sr_probe(struct device *dev)
 
 	sdev_printk(KERN_DEBUG, sdev,
 		    "Attached scsi CD-ROM %s\n", cd->cdi.name);
+
+	/* enable runtime pm */
+	scsi_autopm_put_device(cd->device);
+
 	return 0;
 
 fail_put:
@@ -965,6 +1002,9 @@ static int sr_remove(struct device *dev)
 {
 	struct scsi_cd *cd = dev_get_drvdata(dev);
 
+	/* disable runtime pm */
+	scsi_autopm_get_device(cd->device);
+
 	blk_queue_prep_rq(cd->device->request_queue, scsi_prep_fn);
 	del_gendisk(cd->disk);
 
diff --git a/drivers/scsi/sr.h b/drivers/scsi/sr.h
index 37c8f6b..face0d8 100644
--- a/drivers/scsi/sr.h
+++ b/drivers/scsi/sr.h
@@ -25,6 +25,11 @@
 
 struct scsi_device;
 
+struct sr_pm_ops {
+	int (*suspend)(struct device *dev);
+	int (*resume)(struct device *dev);
+};
+
 /* The CDROM is fairly slow, so we need a little extra time */
 /* In fact, it is very slow if it has to spin up first */
 #define IOCTL_TIMEOUT 30*HZ
@@ -49,6 +54,7 @@ typedef struct scsi_cd {
 	bool ignore_get_event:1;	/* GET_EVENT is unreliable, use TUR */
 
 	struct cdrom_device_info cdi;
+	struct sr_pm_ops *pm_ops;
 	/* We hold gendisk and scsi_device references on probe and use
 	 * the refs on this kref to decide when to release them */
 	struct kref kref;
@@ -74,4 +80,7 @@ void sr_vendor_init(Scsi_CD *);
 int sr_cd_check(struct cdrom_device_info *);
 int sr_set_blocklength(Scsi_CD *, int blocklength);
 
+/* sr_pm_ops.c */
+void sr_pm_ops_init(struct scsi_cd *cd);
+
 #endif
diff --git a/drivers/scsi/sr_pm_ops.c b/drivers/scsi/sr_pm_ops.c
new file mode 100644
index 0000000..610fb74
--- /dev/null
+++ b/drivers/scsi/sr_pm_ops.c
@@ -0,0 +1,104 @@
+#include <linux/cdrom.h>
+#include <scsi/scsi.h>
+#include <scsi/scsi_device.h>
+#include <scsi/scsi_eh.h>
+#include "sr.h"
+
+static int sr_rom_suspend(struct device *dev);
+static int sr_rom_resume(struct device *dev);
+static int sr_zpodd_suspend(struct device *dev);
+static int sr_zpodd_resume(struct device *dev);
+static int sr_worm_suspend(struct device *dev);
+static int sr_worm_resume(struct device *dev);
+
+static struct sr_pm_ops sr_rom_pm_ops = {
+	.suspend	= sr_rom_suspend,
+	.resume		= sr_rom_resume,
+};
+
+static struct sr_pm_ops sr_zpodd_pm_ops = {
+	.suspend	= sr_zpodd_suspend,
+	.resume		= sr_zpodd_resume,
+};
+
+static struct sr_pm_ops sr_worm_pm_ops = {
+	.suspend	= sr_worm_suspend,
+	.resume		= sr_worm_resume,
+};
+
+static int sr_rom_suspend(struct device *dev)
+{
+	/* TODO: add requirement for ROM */
+	return 0;
+}
+
+static int sr_rom_resume(struct device *dev)
+{
+	return 0;
+}
+
+static int sr_worm_suspend(struct device *dev)
+{
+	/* TODO: add requirement for WORM */
+	return 0;
+}
+
+static int sr_worm_resume(struct device *dev)
+{
+	return 0;
+}
+
+static int sr_zpodd_suspend(struct device *dev)
+{
+	int suspend;
+	struct scsi_sense_hdr sshdr;
+	struct scsi_cd *cd = dev_get_drvdata(dev);
+
+	/*
+	 * ZPODD can be runtime suspended when:
+	 * tray type: no media inside and tray closed
+	 * slot type: no media inside
+	 */
+	scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
+
+	if (cd->cdi.mask & CDC_CLOSE_TRAY)
+		/* no media for caddy/slot type ODD */
+		suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a;
+	else
+		/* no media and door closed for tray type ODD */
+		suspend = scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a &&
+			sshdr.ascq == 0x01;
+
+	if (!suspend)
+		return -EBUSY;
+
+	return 0;
+}
+
+static int sr_zpodd_resume(struct device *dev)
+{
+	struct scsi_cd *cd = dev_get_drvdata(dev);
+	struct scsi_sense_hdr sshdr;
+
+	/* get the disk ready */
+	scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
+
+	if (cd->device->need_eject) {
+		cd->device->need_eject = 0;
+		if (!(cd->cdi.mask & CDC_CLOSE_TRAY))
+			sr_tray_move(&cd->cdi, 1);
+	}
+
+	return 0;
+
+}
+
+void sr_pm_ops_init(struct scsi_cd *cd)
+{
+	if (cd->device->type == TYPE_ROM) {
+		cd->pm_ops = &sr_rom_pm_ops;
+		if (cd->device->can_power_off)
+			cd->pm_ops = &sr_zpodd_pm_ops;
+	} else if (cd->device->type == TYPE_WORM)
+		cd->pm_ops = &sr_worm_pm_ops;
+}
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 9895f69..c0019d3 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -156,6 +156,7 @@ struct scsi_device {
 	unsigned is_visible:1;	/* is the device visible in sysfs */
 	unsigned can_power_off:1; /* Device supports runtime power off */
 	unsigned wce_default_on:1;	/* Cache is ON by default */
+	unsigned need_eject:1;	/* Need eject the tray when wakes up */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */

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

[Alan Stern]

On Thu, 6 Sep 2012, Aaron Lu wrote:

> > That's why we have an autosuspend delay.  Although for some reason the 
> > SCSI subsystem doesn't use it currently...  We need to add a call to 
> > pm_runtime_use_autosuspend() in scsi_sysfs_add_sdev().  Likewise, the 
> > pm_schedule_suspend() call in scsi_runtime_idle() should be changed to 
> > pm_runtime_autosuspend().  And there should be calls to 
> > pm_runtime_set_autosuspend_delay() in the sd and sr drivers.
> 
> I tried to use autosuspend when preparing the patch, but the fact that
> the devices will be polled every 2 seconds make it impossible to enter
> suspend state if the autosuspend delay is larger than that.

You can always increase the polling interval.

But in the long run that wouldn't be a good solution.  What I'd really 
like is a way to do the status polling without having it reset the 
idle timer.

Oliver, what do you think?  Would that be a good solution?

Alan Stern

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

[Oliver Neukum]

On Thursday 06 September 2012 11:06:49 Alan Stern wrote:
> On Thu, 6 Sep 2012, Aaron Lu wrote:
> 
> > > That's why we have an autosuspend delay.  Although for some reason the 
> > > SCSI subsystem doesn't use it currently...  We need to add a call to 
> > > pm_runtime_use_autosuspend() in scsi_sysfs_add_sdev().  Likewise, the 
> > > pm_schedule_suspend() call in scsi_runtime_idle() should be changed to 
> > > pm_runtime_autosuspend().  And there should be calls to 
> > > pm_runtime_set_autosuspend_delay() in the sd and sr drivers.
> > 
> > I tried to use autosuspend when preparing the patch, but the fact that
> > the devices will be polled every 2 seconds make it impossible to enter
> > suspend state if the autosuspend delay is larger than that.
> 
> You can always increase the polling interval.
> 
> But in the long run that wouldn't be a good solution.  What I'd really 
> like is a way to do the status polling without having it reset the 
> idle timer.
> 
> Oliver, what do you think?  Would that be a good solution?

Well, we could introduce a flag into the requests for the polls.
But best would be to simply declare a device immediately idle
as soon as we learn that it has no medium. No special casing
would be needed.

	Regards
		Oliver

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

[Alan Stern]

On Thu, 6 Sep 2012, Oliver Neukum wrote:

> On Thursday 06 September 2012 11:06:49 Alan Stern wrote:
> > On Thu, 6 Sep 2012, Aaron Lu wrote:
> > 
> > > > That's why we have an autosuspend delay.  Although for some reason the 
> > > > SCSI subsystem doesn't use it currently...  We need to add a call to 
> > > > pm_runtime_use_autosuspend() in scsi_sysfs_add_sdev().  Likewise, the 
> > > > pm_schedule_suspend() call in scsi_runtime_idle() should be changed to 
> > > > pm_runtime_autosuspend().  And there should be calls to 
> > > > pm_runtime_set_autosuspend_delay() in the sd and sr drivers.
> > > 
> > > I tried to use autosuspend when preparing the patch, but the fact that
> > > the devices will be polled every 2 seconds make it impossible to enter
> > > suspend state if the autosuspend delay is larger than that.
> > 
> > You can always increase the polling interval.
> > 
> > But in the long run that wouldn't be a good solution.  What I'd really 
> > like is a way to do the status polling without having it reset the 
> > idle timer.
> > 
> > Oliver, what do you think?  Would that be a good solution?
> 
> Well, we could introduce a flag into the requests for the polls.
> But best would be to simply declare a device immediately idle
> as soon as we learn that it has no medium. No special casing
> would be needed.

We could do that, but what about idle drives that do have media?

Alan Stern

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

[Oliver Neukum]

On Thursday 06 September 2012 13:08:18 Alan Stern wrote:
> On Thu, 6 Sep 2012, Oliver Neukum wrote:
> 
> > On Thursday 06 September 2012 11:06:49 Alan Stern wrote:
> > > On Thu, 6 Sep 2012, Aaron Lu wrote:
> > > 
> > > > > That's why we have an autosuspend delay.  Although for some reason the 
> > > > > SCSI subsystem doesn't use it currently...  We need to add a call to 
> > > > > pm_runtime_use_autosuspend() in scsi_sysfs_add_sdev().  Likewise, the 
> > > > > pm_schedule_suspend() call in scsi_runtime_idle() should be changed to 
> > > > > pm_runtime_autosuspend().  And there should be calls to 
> > > > > pm_runtime_set_autosuspend_delay() in the sd and sr drivers.
> > > > 
> > > > I tried to use autosuspend when preparing the patch, but the fact that
> > > > the devices will be polled every 2 seconds make it impossible to enter
> > > > suspend state if the autosuspend delay is larger than that.
> > > 
> > > You can always increase the polling interval.
> > > 
> > > But in the long run that wouldn't be a good solution.  What I'd really 
> > > like is a way to do the status polling without having it reset the 
> > > idle timer.
> > > 
> > > Oliver, what do you think?  Would that be a good solution?
> > 
> > Well, we could introduce a flag into the requests for the polls.
> > But best would be to simply declare a device immediately idle
> > as soon as we learn that it has no medium. No special casing
> > would be needed.
> 
> We could do that, but what about idle drives that do have media?

Then we do have a problem. To handle this optimally we'd have to make
a difference between the first time a new medium is noticed and later
polls.

	Regards
		Oliver

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

[Alan Stern]

On Thu, 6 Sep 2012, Oliver Neukum wrote:

> > > > But in the long run that wouldn't be a good solution.  What I'd really 
> > > > like is a way to do the status polling without having it reset the 
> > > > idle timer.
> > > > 
> > > > Oliver, what do you think?  Would that be a good solution?
> > > 
> > > Well, we could introduce a flag into the requests for the polls.
> > > But best would be to simply declare a device immediately idle
> > > as soon as we learn that it has no medium. No special casing
> > > would be needed.
> > 
> > We could do that, but what about idle drives that do have media?
> 
> Then we do have a problem. To handle this optimally we'd have to make
> a difference between the first time a new medium is noticed and later
> polls.

That's right.  It shouldn't be too difficult to accomplish.

One case to watch out for is a ZPODD with no media and an open door.  
We should put the drive into runtime suspend immediately, but continue
polling and leave the power on.  The runtime suspend after each poll
will to see if the door is shut; when it is then polling can be turned
off and power removed.

This leads to a question: How should the sr device tell its parent 
whether or not to turn off the power?  Aaron's current patches simply 
rely on the device being runtime suspended, but that won't work with 
this scheme.  Do we need a "ready_to_power_down" flag?

Alan Stern

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

[Jeff Garzik]

On 09/04/2012 10:24 AM, Aaron Lu wrote:
> From: Aaron Lu <aaron.lu@intel.com>
>
> Hard disk may also be runtime powered off, so set can_power_off flag
> for it too if condition satisfies.
>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>

Acked-by: Jeff Garzik <jgarzik@redhat.com>

Re: [PATCH v6 7/7] libata: acpi: respect may_power_off flag

[Jeff Garzik]

On 09/04/2012 10:24 AM, Aaron Lu wrote:
> From: Aaron Lu <aaron.lu@intel.com>
>
> If user does not want the device being powered off when runtime
> suspended by setting may_power_off flag to 0, we will not choose D3 cold
> ACPI D-State for it.
>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>   drivers/ata/libata-acpi.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index 6c8f89c..774180d 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -869,7 +869,9 @@ void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
>
>   		if (state.event != PM_EVENT_ON) {
>   			acpi_state = acpi_pm_device_sleep_state(
> -				&dev->sdev->sdev_gendev, NULL, ACPI_STATE_D3);
> +				&dev->sdev->sdev_gendev, NULL,
> +				dev->sdev->may_power_off ?
> +				ACPI_STATE_D3_COLD : ACPI_STATE_D3_HOT);
>   			if (acpi_state > 0)
>   				acpi_bus_set_power(handle, acpi_state);

Acked-by: Jeff Garzik <jgarzik@redhat.com>

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

[Aaron Lu]

On 09/06/2012 11:06 PM, Alan Stern wrote:
> On Thu, 6 Sep 2012, Aaron Lu wrote:
>
>>> That's why we have an autosuspend delay.  Although for some reason the
>>> SCSI subsystem doesn't use it currently...  We need to add a call to
>>> pm_runtime_use_autosuspend() in scsi_sysfs_add_sdev().  Likewise, the
>>> pm_schedule_suspend() call in scsi_runtime_idle() should be changed to
>>> pm_runtime_autosuspend().  And there should be calls to
>>> pm_runtime_set_autosuspend_delay() in the sd and sr drivers.
>>
>> I tried to use autosuspend when preparing the patch, but the fact that
>> the devices will be polled every 2 seconds make it impossible to enter
>> suspend state if the autosuspend delay is larger than that.
>
> You can always increase the polling interval.
> But in the long run that wouldn't be a good solution.

Agree.

> What I'd really like is a way to do the status polling without having
> it reset the idle timer.

I like this, I'll try to see if this can be done.

If we idle the device immediately, we would suspend/resume the ODD every
2 seconds, which may not be a good idea.

And we can't increate the polling interval that much, like to the level
of minutes, and if we can't put the device into suspend state long
enough, it may not worth the effort.

What do you think?

-Aaron

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

[Alan Stern]

On Fri, 7 Sep 2012, Aaron Lu wrote:

> > What I'd really like is a way to do the status polling without having
> > it reset the idle timer.
> 
> I like this, I'll try to see if this can be done.
> 
> If we idle the device immediately, we would suspend/resume the ODD every
> 2 seconds, which may not be a good idea.

It may be okay.  This depends on how long it takes to suspend and
resume the drive and its parents.  If it takes no more than a few
hundred ms then there shouldn't be any problem.  Especially if the 
polling interval is increased to something like 5 seconds.

Alan Stern

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

[Aaron Lu]

On 09/07/2012 02:50 AM, Alan Stern wrote:
> On Thu, 6 Sep 2012, Oliver Neukum wrote:
> 
>>>>> But in the long run that wouldn't be a good solution.  What I'd really 
>>>>> like is a way to do the status polling without having it reset the 
>>>>> idle timer.
>>>>>
>>>>> Oliver, what do you think?  Would that be a good solution?
>>>>
>>>> Well, we could introduce a flag into the requests for the polls.
>>>> But best would be to simply declare a device immediately idle
>>>> as soon as we learn that it has no medium. No special casing
>>>> would be needed.
>>>
>>> We could do that, but what about idle drives that do have media?
>>
>> Then we do have a problem. To handle this optimally we'd have to make
>> a difference between the first time a new medium is noticed and later
>> polls.
> 
> That's right.  It shouldn't be too difficult to accomplish.
> 
> One case to watch out for is a ZPODD with no media and an open door.  
> We should put the drive into runtime suspend immediately, but continue
> polling and leave the power on.  The runtime suspend after each poll
> will to see if the door is shut; when it is then polling can be turned
> off and power removed.
> 
> This leads to a question: How should the sr device tell its parent 
> whether or not to turn off the power?  Aaron's current patches simply 
> rely on the device being runtime suspended, but that won't work with 
> this scheme.  Do we need a "ready_to_power_down" flag?


Thanks for the suggestions, I've tried to use these ideas and please
take a look to see if below code did the job.

Note that I didn't call disk_block_events in sr_suspend, as there is a
race that I didn't know how to resolve:

sr_suspend				sr_check_events
  disk_block_events			  scsi_autopm_get_device
    wait for all delayed		    wait for suspend finish
    events checking work
    to finish

So it's possible when sr_suspend is executing, sr_check_events is also
executing and disk_block_events will wait for sr_check_events to finish
while sr_check_events waits for this device's suspend routine finish
before the scsi_autopm_get_device can proceed.

I used a flag called powered_off, if it is set, the sr_check_events will
simply return.

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 902b5a4..f91c922 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -869,9 +869,14 @@ void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
 
 		if (state.event != PM_EVENT_ON) {
 			acpi_state = acpi_pm_device_sleep_state(
-				&dev->sdev->sdev_gendev, NULL, ACPI_STATE_D3);
-			if (acpi_state > 0)
+					&dev->sdev->sdev_gendev, NULL,
+					dev->sdev->ready_to_power_off ?
+					ACPI_STATE_D3 : ACPI_STATE_D0);
+			if (acpi_state > 0) {
 				acpi_bus_set_power(handle, acpi_state);
+				if (acpi_state == ACPI_STATE_D3)
+					dev->sdev->powered_off = 1;
+			}
 			/* TBD: need to check if it's runtime pm request */
 			acpi_pm_device_run_wake(
 				&dev->sdev->sdev_gendev, true);
@@ -880,6 +885,7 @@ void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
 			acpi_pm_device_run_wake(
 				&dev->sdev->sdev_gendev, false);
 			acpi_bus_set_power(handle, ACPI_STATE_D0);
+			dev->sdev->powered_off = 0;
 		}
 	}
 
@@ -985,8 +991,10 @@ static void ata_acpi_wake_dev(acpi_handle handle, u32 event, void *context)
 	struct ata_device *ata_dev = context;
 
 	if (event == ACPI_NOTIFY_DEVICE_WAKE && ata_dev &&
-			pm_runtime_suspended(&ata_dev->sdev->sdev_gendev))
-		scsi_autopm_get_device(ata_dev->sdev);
+			pm_runtime_suspended(&ata_dev->sdev->sdev_gendev)) {
+		ata_dev->sdev->need_eject = 1;
+		pm_runtime_resume(&ata_dev->sdev->sdev_gendev);
+	}
 }
 
 static void ata_acpi_add_pm_notifier(struct ata_device *dev)
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 5fc97d2..890cee2 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -45,6 +45,7 @@
 #include <linux/blkdev.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>
@@ -79,6 +80,8 @@ static DEFINE_MUTEX(sr_mutex);
 static int sr_probe(struct device *);
 static int sr_remove(struct device *);
 static int sr_done(struct scsi_cmnd *);
+static int sr_suspend(struct device *, pm_message_t msg);
+static int sr_resume(struct device *);
 
 static struct scsi_driver sr_template = {
 	.owner			= THIS_MODULE,
@@ -86,6 +89,8 @@ static struct scsi_driver sr_template = {
 		.name   	= "sr",
 		.probe		= sr_probe,
 		.remove		= sr_remove,
+		.suspend	= sr_suspend,
+		.resume		= sr_resume,
 	},
 	.done			= sr_done,
 };
@@ -146,8 +151,12 @@ static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
 	kref_get(&cd->kref);
 	if (scsi_device_get(cd->device))
 		goto out_put;
+	if (scsi_autopm_get_device(cd->device))
+		goto out_pm;
 	goto out;
 
+ out_pm:
+	scsi_device_put(cd->device);
  out_put:
 	kref_put(&cd->kref, sr_kref_release);
 	cd = NULL;
@@ -163,9 +172,38 @@ static void scsi_cd_put(struct scsi_cd *cd)
 	mutex_lock(&sr_ref_mutex);
 	kref_put(&cd->kref, sr_kref_release);
 	scsi_device_put(sdev);
+	scsi_autopm_put_device(sdev);
 	mutex_unlock(&sr_ref_mutex);
 }
 
+static int sr_suspend(struct device *dev, pm_message_t msg)
+{
+	return 0;
+}
+
+static int sr_resume(struct device *dev)
+{
+	struct scsi_cd *cd;
+	struct scsi_sense_hdr sshdr;
+
+	cd = dev_get_drvdata(dev);
+
+	if (!cd->device->powered_off)
+		return 0;
+
+	/* get the disk ready */
+	scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
+
+	/* if user wakes up the ODD, eject the tray */
+	if (cd->device->need_eject) {
+		cd->device->need_eject = 0;
+		if (!(cd->cdi.mask & CDC_CLOSE_TRAY))
+			sr_tray_move(&cd->cdi, 1);
+	}
+
+	return 0;
+}
+
 static unsigned int sr_get_events(struct scsi_device *sdev)
 {
 	u8 buf[8];
@@ -211,7 +249,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 +258,11 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 	if (CDSL_CURRENT != slot)
 		return 0;
 
+	if (cd->device->powered_off)
+		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 +289,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);
 
 	/*
@@ -269,8 +311,19 @@ do_tur:
 		cd->tur_changed = true;
 	}
 
+	/* See if we can power off this ZPODD device */
+	if (cd->device->can_power_off) {
+		if (cd->cdi.mask & CDC_CLOSE_TRAY)
+			/* no media for caddy/slot type ODD */
+			cd->device->ready_to_power_off = !cd->media_present;
+		else
+			/* no media and door closed for tray type ODD */
+			cd->device->ready_to_power_off = !cd->media_present &&
+							 sshdr.ascq == 0x01;
+	}
+
 	if (cd->ignore_get_event)
-		return events;
+		goto out;
 
 	/* check whether GET_EVENT is reporting spurious MEDIA_CHANGE */
 	if (!cd->tur_changed) {
@@ -287,6 +340,12 @@ do_tur:
 	cd->tur_changed = false;
 	cd->get_event_changed = false;
 
+out:
+	if (cd->media_present && !last_present)
+		pm_runtime_put_noidle(&cd->device->sdev_gendev);
+	else
+		scsi_autopm_put_device(cd->device);
+
 	return events;
 }
 
@@ -715,9 +774,14 @@ static int sr_probe(struct device *dev)
 	dev_set_drvdata(dev, cd);
 	disk->flags |= GENHD_FL_REMOVABLE;
 	add_disk(disk);
+	disk_events_set_poll_msecs(disk, 5000);
 
 	sdev_printk(KERN_DEBUG, sdev,
 		    "Attached scsi CD-ROM %s\n", cd->cdi.name);
+
+	/* enable runtime pm */
+	scsi_autopm_put_device(cd->device);
+
 	return 0;
 
 fail_put:
@@ -965,6 +1029,9 @@ static int sr_remove(struct device *dev)
 {
 	struct scsi_cd *cd = dev_get_drvdata(dev);
 
+	/* disable runtime pm */
+	scsi_autopm_get_device(cd->device);
+
 	blk_queue_prep_rq(cd->device->request_queue, scsi_prep_fn);
 	del_gendisk(cd->disk);
 

Thanks,
Aaron

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

[Oliver Neukum]

On Monday 10 September 2012 17:16:22 Aaron Lu wrote:

> +static int sr_resume(struct device *dev)
> +{
> +	struct scsi_cd *cd;
> +	struct scsi_sense_hdr sshdr;
> +
> +	cd = dev_get_drvdata(dev);
> +
> +	if (!cd->device->powered_off)
> +		return 0;
> +
> +	/* get the disk ready */
> +	scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
> +
> +	/* if user wakes up the ODD, eject the tray */
> +	if (cd->device->need_eject) {
> +		cd->device->need_eject = 0;
> +		if (!(cd->cdi.mask & CDC_CLOSE_TRAY))
> +			sr_tray_move(&cd->cdi, 1);

1. Even if the door is locked?
2. sr_tray_move allocates memory with GFP_KERNEL. This smells of a deadlock.

	Regards
		Oliver

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

[Aaron Lu]

On Mon, Sep 10, 2012 at 12:45:51PM +0200, Oliver Neukum wrote:
> On Monday 10 September 2012 17:16:22 Aaron Lu wrote:
> 
> > +static int sr_resume(struct device *dev)
> > +{
> > +	struct scsi_cd *cd;
> > +	struct scsi_sense_hdr sshdr;
> > +
> > +	cd = dev_get_drvdata(dev);
> > +
> > +	if (!cd->device->powered_off)
> > +		return 0;
> > +
> > +	/* get the disk ready */
> > +	scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
> > +
> > +	/* if user wakes up the ODD, eject the tray */
> > +	if (cd->device->need_eject) {
> > +		cd->device->need_eject = 0;
> > +		if (!(cd->cdi.mask & CDC_CLOSE_TRAY))
> > +			sr_tray_move(&cd->cdi, 1);
> 
> 1. Even if the door is locked?

This piece of code of sr_resume is intended for ZPODD devices, for
normal devices it will simply return as the powered_off flag will never
be set for them.

And for ZPODD devices, the door shouldn't be in locked state here since
for it to enter power off state, "no medium inside" has to be satisfied
and when there is no medium inside, we do not lock its door.

> 2. sr_tray_move allocates memory with GFP_KERNEL. This smells of a deadlock.

Sorry, don't get this. Can you please elaborate?

Thanks,
Aaron

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

[Oliver Neukum]

On Tuesday 11 September 2012 14:44:30 Aaron Lu wrote:
> On Mon, Sep 10, 2012 at 12:45:51PM +0200, Oliver Neukum wrote:
> > On Monday 10 September 2012 17:16:22 Aaron Lu wrote:
> > 
> > > +static int sr_resume(struct device *dev)
> > > +{
> > > +	struct scsi_cd *cd;
> > > +	struct scsi_sense_hdr sshdr;
> > > +
> > > +	cd = dev_get_drvdata(dev);
> > > +
> > > +	if (!cd->device->powered_off)
> > > +		return 0;
> > > +
> > > +	/* get the disk ready */
> > > +	scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
> > > +
> > > +	/* if user wakes up the ODD, eject the tray */
> > > +	if (cd->device->need_eject) {
> > > +		cd->device->need_eject = 0;
> > > +		if (!(cd->cdi.mask & CDC_CLOSE_TRAY))
> > > +			sr_tray_move(&cd->cdi, 1);
> > 
> > 1. Even if the door is locked?
> 
> This piece of code of sr_resume is intended for ZPODD devices, for
> normal devices it will simply return as the powered_off flag will never
> be set for them.
> 
> And for ZPODD devices, the door shouldn't be in locked state here since
> for it to enter power off state, "no medium inside" has to be satisfied

This is true only if the device is autosuspended. But sr_resume()
will also be called if the whole system was suspended.
What happens if the user presses the door button on the drive
while the system was suspended?

> and when there is no medium inside, we do not lock its door.
> 
> > 2. sr_tray_move allocates memory with GFP_KERNEL. This smells of a deadlock.
> 
> Sorry, don't get this. Can you please elaborate?

Suppose you need to wake up two devices for the same reason, a ZPODD and a hard
drive. The ZPODD is woken first and sr_resume() requests memory with GFP_KERNEL.
The VM layer decides to page out to the hard drive to be woken up -> deadlock.

	Regards
		Oliver

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

[Aaron Lu]

On Tue, Sep 11, 2012 at 10:55:44AM +0200, Oliver Neukum wrote:
> On Tuesday 11 September 2012 14:44:30 Aaron Lu wrote:
> > On Mon, Sep 10, 2012 at 12:45:51PM +0200, Oliver Neukum wrote:
> > > On Monday 10 September 2012 17:16:22 Aaron Lu wrote:
> > > 
> > > > +static int sr_resume(struct device *dev)
> > > > +{
> > > > +	struct scsi_cd *cd;
> > > > +	struct scsi_sense_hdr sshdr;
> > > > +
> > > > +	cd = dev_get_drvdata(dev);
> > > > +
> > > > +	if (!cd->device->powered_off)
> > > > +		return 0;
> > > > +
> > > > +	/* get the disk ready */
> > > > +	scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
> > > > +
> > > > +	/* if user wakes up the ODD, eject the tray */
> > > > +	if (cd->device->need_eject) {
> > > > +		cd->device->need_eject = 0;
> > > > +		if (!(cd->cdi.mask & CDC_CLOSE_TRAY))
> > > > +			sr_tray_move(&cd->cdi, 1);
> > > 
> > > 1. Even if the door is locked?
> > 
> > This piece of code of sr_resume is intended for ZPODD devices, for
> > normal devices it will simply return as the powered_off flag will never
> > be set for them.
> > 
> > And for ZPODD devices, the door shouldn't be in locked state here since
> > for it to enter power off state, "no medium inside" has to be satisfied
> 
> This is true only if the device is autosuspended. But sr_resume()
> will also be called if the whole system was suspended.

You mean when system resumes, right?

> What happens if the user presses the door button on the drive
> while the system was suspended?

The drive does not have the capability to wake up the system from S3, so
nothing happens.

> 
> > and when there is no medium inside, we do not lock its door.
> > 
> > > 2. sr_tray_move allocates memory with GFP_KERNEL. This smells of a deadlock.
> > 
> > Sorry, don't get this. Can you please elaborate?
> 
> Suppose you need to wake up two devices for the same reason, a ZPODD and a hard
> drive. The ZPODD is woken first and sr_resume() requests memory with GFP_KERNEL.
> The VM layer decides to page out to the hard drive to be woken up -> deadlock.

While, I don't understand what stops the hard disk from being resumed,
the hard drive's runtime resume does not depend on the ODD's, and I
don't think there should be an order enforced on them.

Thanks,
Aaron

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

[Oliver Neukum]

On Tuesday 11 September 2012 17:24:13 Aaron Lu wrote:
> On Tue, Sep 11, 2012 at 10:55:44AM +0200, Oliver Neukum wrote:
> > On Tuesday 11 September 2012 14:44:30 Aaron Lu wrote:
> > > On Mon, Sep 10, 2012 at 12:45:51PM +0200, Oliver Neukum wrote:
> > > > On Monday 10 September 2012 17:16:22 Aaron Lu wrote:
> > > > 
> > > > > +static int sr_resume(struct device *dev)
> > > > > +{
> > > > > +       struct scsi_cd *cd;
> > > > > +       struct scsi_sense_hdr sshdr;
> > > > > +
> > > > > +       cd = dev_get_drvdata(dev);
> > > > > +
> > > > > +       if (!cd->device->powered_off)
> > > > > +               return 0;
> > > > > +
> > > > > +       /* get the disk ready */
> > > > > +       scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
> > > > > +
> > > > > +       /* if user wakes up the ODD, eject the tray */
> > > > > +       if (cd->device->need_eject) {
> > > > > +               cd->device->need_eject = 0;
> > > > > +               if (!(cd->cdi.mask & CDC_CLOSE_TRAY))
> > > > > +                       sr_tray_move(&cd->cdi, 1);
> > > > 
> > > > 1. Even if the door is locked?
> > > 
> > > This piece of code of sr_resume is intended for ZPODD devices, for
> > > normal devices it will simply return as the powered_off flag will never
> > > be set for them.
> > > 
> > > And for ZPODD devices, the door shouldn't be in locked state here since
> > > for it to enter power off state, "no medium inside" has to be satisfied
> > 
> > This is true only if the device is autosuspended. But sr_resume()
> > will also be called if the whole system was suspended.
> 
> You mean when system resumes, right?

Yes, but because the whole system had been suspended.
In that case you can have a locked door.

> > What happens if the user presses the door button on the drive
> > while the system was suspended?
> 
> The drive does not have the capability to wake up the system from S3, so
> nothing happens.

You are assuming that the system is resumed because the door button is pressed.
That need not be the case. What happens if the door button is pressed while
the system is resuming?

	Regards
		Oliver

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

[Aaron Lu]

On Tue, Sep 11, 2012 at 11:30:35AM +0200, Oliver Neukum wrote:
> On Tuesday 11 September 2012 17:24:13 Aaron Lu wrote:
> > On Tue, Sep 11, 2012 at 10:55:44AM +0200, Oliver Neukum wrote:
> > > On Tuesday 11 September 2012 14:44:30 Aaron Lu wrote:
> > > > On Mon, Sep 10, 2012 at 12:45:51PM +0200, Oliver Neukum wrote:
> > > > > On Monday 10 September 2012 17:16:22 Aaron Lu wrote:
> > > > > 
> > > > > > +static int sr_resume(struct device *dev)
> > > > > > +{
> > > > > > +       struct scsi_cd *cd;
> > > > > > +       struct scsi_sense_hdr sshdr;
> > > > > > +
> > > > > > +       cd = dev_get_drvdata(dev);
> > > > > > +
> > > > > > +       if (!cd->device->powered_off)
> > > > > > +               return 0;
> > > > > > +
> > > > > > +       /* get the disk ready */
> > > > > > +       scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
> > > > > > +
> > > > > > +       /* if user wakes up the ODD, eject the tray */
> > > > > > +       if (cd->device->need_eject) {
> > > > > > +               cd->device->need_eject = 0;
> > > > > > +               if (!(cd->cdi.mask & CDC_CLOSE_TRAY))
> > > > > > +                       sr_tray_move(&cd->cdi, 1);
> > > > > 
> > > > > 1. Even if the door is locked?
> > > > 
> > > > This piece of code of sr_resume is intended for ZPODD devices, for
> > > > normal devices it will simply return as the powered_off flag will never
> > > > be set for them.
> > > > 
> > > > And for ZPODD devices, the door shouldn't be in locked state here since
> > > > for it to enter power off state, "no medium inside" has to be satisfied
> > > 
> > > This is true only if the device is autosuspended. But sr_resume()
> > > will also be called if the whole system was suspended.
> > 
> > You mean when system resumes, right?
> 
> Yes, but because the whole system had been suspended.
> In that case you can have a locked door.

By locked, do you mean the door is closed or the door is locked by the
sr_lock_door function with param lock set to 1?

> 
> > > What happens if the user presses the door button on the drive
> > > while the system was suspended?
> > 
> > The drive does not have the capability to wake up the system from S3, so
> > nothing happens.
> 
> You are assuming that the system is resumed because the door button is pressed.

No, as I said, pressing the eject button can't wake up a suspended
system. The whole idea of ZPODD is to put the ODD into powered off state
while the system is at S0.

> That need not be the case. What happens if the door button is pressed while
> the system is resuming?

In that case, if the hardware logic is ready, an ACPI event will fire
and the device will be runtime resumed; if the hardware logic is not
ready yet due to the system is resuming, nothing happens.

Thanks,
Aaron

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

[Oliver Neukum]

On Tuesday 11 September 2012 19:11:08 Aaron Lu wrote:
> On Tue, Sep 11, 2012 at 11:30:35AM +0200, Oliver Neukum wrote:
> > On Tuesday 11 September 2012 17:24:13 Aaron Lu wrote:

> > Yes, but because the whole system had been suspended.
> > In that case you can have a locked door.
> 
> By locked, do you mean the door is closed or the door is locked by the
> sr_lock_door function with param lock set to 1?

sr_lock_door()

> > That need not be the case. What happens if the door button is pressed while
> > the system is resuming?
> 
> In that case, if the hardware logic is ready, an ACPI event will fire
> and the device will be runtime resumed; if the hardware logic is not
> ready yet due to the system is resuming, nothing happens.

So we have this race:

sr_lock_door()
system goes to S3
system starts resuming from S3
user presses door button
sr_resume()
door opens

I guess this should not happen.

	Regards
		Oliver

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

[Aaron Lu]

On Tue, Sep 11, 2012 at 11:30:35AM +0200, Oliver Neukum wrote:
> On Tuesday 11 September 2012 17:24:13 Aaron Lu wrote:
> > On Tue, Sep 11, 2012 at 10:55:44AM +0200, Oliver Neukum wrote:
> > > On Tuesday 11 September 2012 14:44:30 Aaron Lu wrote:
> > > > On Mon, Sep 10, 2012 at 12:45:51PM +0200, Oliver Neukum wrote:
> > > > > On Monday 10 September 2012 17:16:22 Aaron Lu wrote:
> > > > > 
> > > > > > +static int sr_resume(struct device *dev)
> > > > > > +{
> > > > > > +       struct scsi_cd *cd;
> > > > > > +       struct scsi_sense_hdr sshdr;
> > > > > > +
> > > > > > +       cd = dev_get_drvdata(dev);
> > > > > > +
> > > > > > +       if (!cd->device->powered_off)
> > > > > > +               return 0;
> > > > > > +
> > > > > > +       /* get the disk ready */
> > > > > > +       scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
> > > > > > +
> > > > > > +       /* if user wakes up the ODD, eject the tray */
> > > > > > +       if (cd->device->need_eject) {
> > > > > > +               cd->device->need_eject = 0;
> > > > > > +               if (!(cd->cdi.mask & CDC_CLOSE_TRAY))
> > > > > > +                       sr_tray_move(&cd->cdi, 1);
> > > > > 
> > > > > 1. Even if the door is locked?
> > > > 
> > > > This piece of code of sr_resume is intended for ZPODD devices, for
> > > > normal devices it will simply return as the powered_off flag will never
> > > > be set for them.
> > > > 
> > > > And for ZPODD devices, the door shouldn't be in locked state here since
> > > > for it to enter power off state, "no medium inside" has to be satisfied
> > > 
> > > This is true only if the device is autosuspended. But sr_resume()
> > > will also be called if the whole system was suspended.
> > 
> > You mean when system resumes, right?
> 
> Yes, but because the whole system had been suspended.
> In that case you can have a locked door.
> 
> > > What happens if the user presses the door button on the drive
> > > while the system was suspended?
> > 
> > The drive does not have the capability to wake up the system from S3, so
> > nothing happens.
> 
> You are assuming that the system is resumed because the door button is pressed.
> That need not be the case. What happens if the door button is pressed while
> the system is resuming?

I think a little more description about sr_resume is required.

For non-ZPODD devices, sr_resume will always do nothing no matter it is
called due to system resume or runtime resume since the powered_off flag
will never be set.

And for ZPODD devices, when powered_off flag is set, it will check if
this resume is caused by user pressing the eject button, this is reflected
by the need_eject flag. This flag will only be set if an ACPI gpe event
is received and the event is generated by user pressing the eject button
when the ODD is in powered off state and the system is in S0 state. If
this flag is set, the tray will be ejected for tray type ODD; and if it
is not set, nothing is done there.

So only when the system is in S0, ODD is in powered off state and user
pressed the eject button will the tray be ejected on resume. Is there a
problem with this?

Thanks,
Aaron

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

[Aaron Lu]

On Tue, Sep 11, 2012 at 02:10:18PM +0200, Oliver Neukum wrote:
> On Tuesday 11 September 2012 19:11:08 Aaron Lu wrote:
> > On Tue, Sep 11, 2012 at 11:30:35AM +0200, Oliver Neukum wrote:
> > > On Tuesday 11 September 2012 17:24:13 Aaron Lu wrote:
> 
> > > Yes, but because the whole system had been suspended.
> > > In that case you can have a locked door.
> > 
> > By locked, do you mean the door is closed or the door is locked by the
> > sr_lock_door function with param lock set to 1?
> 
> sr_lock_door()
> 
> > > That need not be the case. What happens if the door button is pressed while
> > > the system is resuming?
> > 
> > In that case, if the hardware logic is ready, an ACPI event will fire
> > and the device will be runtime resumed; if the hardware logic is not
> > ready yet due to the system is resuming, nothing happens.
> 
> So we have this race:
> 
> sr_lock_door()
> system goes to S3
> system starts resuming from S3
> user presses door button
> sr_resume()
> door opens
> 
> I guess this should not happen.

OK, I think I got your meaning.

For a simpler scenario:
1 User application did an ioctl to lock the door of the ODD when there
is no medium inside(but why? :-);
2 The ODD is runtime suspended;
3 The ODD is runtime powered off;
4 User presses the eject button, and the door is ejected in sr_resume.

Condition 1 is a must for this to happen, as if there is medium inside,
the ODD will never be powered off; or if the underlying block device is
opened by some process, it will not enter runtime suspend state.

Looks like I need to record if the door is locked in scsi_cd structure
so that I did not mistakenly eject the door. Does this sound OK?

Thanks,
Aaron

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

[Oliver Neukum]

On Tuesday 11 September 2012 20:31:17 Aaron Lu wrote:
> OK, I think I got your meaning.
> 
> For a simpler scenario:
> 1 User application did an ioctl to lock the door of the ODD when there
> is no medium inside(but why? :-);
> 2 The ODD is runtime suspended;
> 3 The ODD is runtime powered off;
> 4 User presses the eject button, and the door is ejected in sr_resume.
> 
> Condition 1 is a must for this to happen, as if there is medium inside,
> the ODD will never be powered off; or if the underlying block device is
> opened by some process, it will not enter runtime suspend state.
> 
> Looks like I need to record if the door is locked in scsi_cd structure
> so that I did not mistakenly eject the door. Does this sound OK?

Yes, that will do.

	Regards
		Oliver

RE: [PATCH v6 0/7] ZPODD patches

[Jack Wang]

Hi Aaron,

This feature looks nice, but how could I test this patch set, I suppose it
need special Hardware which support ZPODD, could suggest the test setup
environment?

For hard disk runtime power off, Is there (hardware?) requirement for hard
drive to support similar feature?

Best regards!

Jack

Re: [PATCH v6 0/7] ZPODD patches

[Aaron Lu]

On 09/13/2012 10:42 AM, Jack Wang wrote:
> Hi Aaron,

Hi Jack,

> 
> This feature looks nice

Glad to hear this :-)

> but how could I test this patch set, I suppose it
> need special Hardware which support ZPODD, could suggest the test setup
> environment?

For ZPODD, there are 2 requirements:
1 The platform, it has to have the ability to power off a sata device
while at S0;
2 The ODD needs support for device attention capability.

> 
> For hard disk runtime power off, Is there (hardware?) requirement for hard
> drive to support similar feature?

No requirement for hard drive, as it does not need to remote wake up
itself while powered off.

Please note that there is a v7 sent yesterday, you may want to take a
look at that instead of v6.

And the runtime power off of hard drive is dropped from that patch set,
I'll submit it in another patch set(pretty small changes needed).

Thanks,
Aaron

RE: [PATCH v6 0/7] ZPODD patches

[Jack Wang]

> > For hard disk runtime power off, Is there (hardware?) requirement for
hard
> > drive to support similar feature?
> 
> No requirement for hard drive, as it does not need to remote wake up
> itself while powered off.
> 
> Please note that there is a v7 sent yesterday, you may want to take a
> look at that instead of v6.
> 
> And the runtime power off of hard drive is dropped from that patch set,
> I'll submit it in another patch set(pretty small changes needed).
> 
> Thanks,
> Aaron
[Jack Wang] Thanks Aaron for quick replay, for disk runtime power off, will
it   a different proach from block runtime power management patch set from
Lin Ming? 

Re: [PATCH v6 0/7] ZPODD patches

[Aaron Lu]

On 09/13/2012 04:15 PM, Jack Wang wrote:
>>> For hard disk runtime power off, Is there (hardware?) requirement for
> hard
>>> drive to support similar feature?
>>
>> No requirement for hard drive, as it does not need to remote wake up
>> itself while powered off.
>>
>> Please note that there is a v7 sent yesterday, you may want to take a
>> look at that instead of v6.
>>
>> And the runtime power off of hard drive is dropped from that patch set,
>> I'll submit it in another patch set(pretty small changes needed).
>>
>> Thanks,
>> Aaron
> [Jack Wang] Thanks Aaron for quick replay, for disk runtime power off, will
> it   a different proach from block runtime power management patch set from
> Lin Ming? 

I suppose you mean the "block layer runtime pm" from Lin Ming?
No, they are different things.

That patch set introduced a mechanism to decide when a device can enter
runtime suspend based on block layer request, while my patch cares about
when it is runtime suspended, how to remove its power.

Thanks,
Aaron

RE: [PATCH v6 0/7] ZPODD patches

[Jack Wang]

> >>> For hard disk runtime power off, Is there (hardware?) requirement for
> > hard
> >>> drive to support similar feature?
> >>
> >> No requirement for hard drive, as it does not need to remote wake up
> >> itself while powered off.
> >>
> >> Please note that there is a v7 sent yesterday, you may want to take a
> >> look at that instead of v6.
> >>
> >> And the runtime power off of hard drive is dropped from that patch set,
> >> I'll submit it in another patch set(pretty small changes needed).
> >>
> >> Thanks,
> >> Aaron
> > [Jack Wang] Thanks Aaron for quick replay, for disk runtime power off,
will
> > it   a different proach from block runtime power management patch set
from
> > Lin Ming?
> 
> I suppose you mean the "block layer runtime pm" from Lin Ming?
> No, they are different things.
> 
> That patch set introduced a mechanism to decide when a device can enter
> runtime suspend based on block layer request, while my patch cares about
> when it is runtime suspended, how to remove its power.
> 
> Thanks,
> Aaron
[Jack Wang] Thanks, but I still not get how to remove hard disk power?
Through acpi? But libata acpi not cover scsi disk right? Sorry if my
question is too stupid.

Re: [PATCH v6 0/7] ZPODD patches

[Aaron Lu]

On 09/13/2012 04:39 PM, Jack Wang wrote:
>>>>> For hard disk runtime power off, Is there (hardware?) requirement for
>>> hard
>>>>> drive to support similar feature?
>>>>
>>>> No requirement for hard drive, as it does not need to remote wake up
>>>> itself while powered off.
>>>>
>>>> Please note that there is a v7 sent yesterday, you may want to take a
>>>> look at that instead of v6.
>>>>
>>>> And the runtime power off of hard drive is dropped from that patch set,
>>>> I'll submit it in another patch set(pretty small changes needed).
>>>>
>>>> Thanks,
>>>> Aaron
>>> [Jack Wang] Thanks Aaron for quick replay, for disk runtime power off,
> will
>>> it   a different proach from block runtime power management patch set
> from
>>> Lin Ming?
>>
>> I suppose you mean the "block layer runtime pm" from Lin Ming?
>> No, they are different things.
>>
>> That patch set introduced a mechanism to decide when a device can enter
>> runtime suspend based on block layer request, while my patch cares about
>> when it is runtime suspended, how to remove its power.
>>
>> Thanks,
>> Aaron
> [Jack Wang] Thanks, but I still not get how to remove hard disk power?
> Through acpi? But libata acpi not cover scsi disk right? Sorry if my
> question is too stupid.

When scsi disk is runtime suspended, the ata port it attached to will be
runtime suspended, and then the power will be removed through ACPI.

Please check libata-acpi.c, function ata_acpi_set_state.

The parent/child relationship of the devices:
ata_host
  ata_port
    scsi_host
      scsi_target
        scsi_device

If you have more questions on how it works, please send email only
to me, other people might be too busy to read such emails :-)

Thanks,
Aaron

Re: [PATCH v6 0/7] ZPODD patches

[James Bottomley]

On Thu, 2012-09-13 at 16:56 +0800, Aaron Lu wrote:
> If you have more questions on how it works, please send email only
> to me, other people might be too busy to read such emails :-)

No, please don't.  One of the points of mailing lists is to accumulate
information.  The reason we always ask for the mailing list to be cc'd
isn't so that everyone reads everything at once (the beauty of email is
that once you're proficient, you can simply skip threads you've lost
interest in), it's so that a year later when someone wonders about a
feature like Optical Disk Drive zero power, they ask google and it turns
up the thread.  If you answer all questions in the open, the chances are
you've already answered whatever it is the future searcher is looking
for.

James

RE:[PATCH v6 0/7] ZPODD patches

[Jack Wang]

> 
> When scsi disk is runtime suspended, the ata port it attached to will be
> runtime suspended, and then the power will be removed through ACPI.
> 
> Please check libata-acpi.c, function ata_acpi_set_state.
> 
> The parent/child relationship of the devices:
> ata_host
>   ata_port
>     scsi_host
>       scsi_target
>         scsi_device
> 
> If you have more questions on how it works, please send email only
> to me, other people might be too busy to read such emails :-)
> 
> Thanks,
> Aaron
[Jack Wang] Thanks, but as I read code, only SATA disk will attached to ata
port, how about SAS disk or FC disk?

I still copy to other people as James suggested :)


> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/7] ZPODD patches

[Aaron Lu]

On 09/13/2012 05:12 PM, Jack Wang wrote:
>>
>> When scsi disk is runtime suspended, the ata port it attached to will be
>> runtime suspended, and then the power will be removed through ACPI.
>>
>> Please check libata-acpi.c, function ata_acpi_set_state.
>>
>> The parent/child relationship of the devices:
>> ata_host
>>   ata_port
>>     scsi_host
>>       scsi_target
>>         scsi_device
>>
>> If you have more questions on how it works, please send email only
>> to me, other people might be too busy to read such emails :-)
>>
>> Thanks,
>> Aaron
> [Jack Wang] Thanks, but as I read code, only SATA disk will attached to ata
> port, how about SAS disk or FC disk?

Don't know much about SAS/FC, I guess they have their own controller.
The patch only work for SATA.

> 
> I still copy to other people as James suggested :)

Sure, please do so.

Thanks,
Aaron