[PATCH v14 0/3] ZPODD Patches

[PATCH v14 0/3] ZPODD Patches

[Aaron Lu]

v14:
Patches 2-6 and 8 of v13 are already taken by Jeff into libata/upstream,
so we just have 3 patches left:
patch 1 in v13 -> patch 1 in v14
patch 7 in v13 -> patch 2 in v14
patch 9 in v13 -> patch 3 in v14

Update sr's runtime PM code to take care of audio play as pointed out by
Alan Stern in patch 1;
Due to the above changes to patch 1, patch 2 was rediffed accordingly,
with no functionality changes.

Hi Jeff,

I've prepaed several git branches for your convenience:
https://github.com/aaronlu/linux.git
Branch zpodd_v14 has the three patches submitted here;
Branch fix-smatch has the fix patch for a smatch warning;
Branch for-jeff has the above two branches merged;
Branch zpodd has all the 9 patches on top of libata/upstream branch, but
with all zpodd related patches in that branch removed first.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Aaron Lu (3):
  scsi: sr: support runtime pm
  libata: scsi: no poll when ODD is powered off
  scsi: remove can_power_off flag from scsi_device

 drivers/ata/libata-zpodd.c |  7 +++++++
 drivers/scsi/scsi_lib.c    | 14 ++++++++++++++
 drivers/scsi/sr.c          | 46 +++++++++++++++++++++++++++++++++++++++++++---
 include/scsi/scsi_device.h |  5 ++++-
 4 files changed, 68 insertions(+), 4 deletions(-)

-- 
1.8.1

[PATCH v14 1/3] scsi: sr: support runtime pm

[Aaron Lu]

This patch adds runtime pm support for sr.

It did this by increasing the runtime usage_count of the device when
its block device is accessed. And decreasing the runtime usage_count
of the device when the access is done.

If there is media inside, runtime suspend is not allowed as we don't
always know if the ODD is being used or not.

The idea is discussed here:
http://thread.gmane.org/gmane.linux.acpi.devel/55243/focus=52703
and the restriction to check media inside is discussed here:
http://thread.gmane.org/gmane.linux.ide/53665/focus=58836

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Acked-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/sr.c | 42 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

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

[PATCH v14 2/3] libata: scsi: no poll when ODD is powered off

[Aaron Lu]

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

But since we don't have access to the gendisk structure in LLDs, here
comes the disk_events_disable_depth for scsi device. This field is a
hint set by LLDs to convey information to upper layer drivers. A value
of 0 means media poll is necessary for the device, while values above 0
means media poll is not needed and should better be skipped. So we can
increase its value when we are to power off the ODD in ATA layer and
decrease its value when the ODD is powered on, effectively silence the
media events poll.

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

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

[PATCH v14 3/3] scsi: remove can_power_off flag from scsi_device

[Aaron Lu]

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

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

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

Re: [PATCH v14 1/3] scsi: sr: support runtime pm

[Jeff Garzik]

On 01/23/2013 02:09 AM, Aaron Lu wrote:
> This patch adds runtime pm support for sr.
>
> It did this by increasing the runtime usage_count of the device when
> its block device is accessed. And decreasing the runtime usage_count
> of the device when the access is done.
>
> If there is media inside, runtime suspend is not allowed as we don't
> always know if the ODD is being used or not.
>
> The idea is discussed here:
> http://thread.gmane.org/gmane.linux.acpi.devel/55243/focus=52703
> and the restriction to check media inside is discussed here:
> http://thread.gmane.org/gmane.linux.ide/53665/focus=58836
>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> Acked-by: Alan Stern <stern@rowland.harvard.edu>
> Acked-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>   drivers/scsi/sr.c | 42 +++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 39 insertions(+), 3 deletions(-)

applied 1-3

Re: [PATCH v14 1/3] scsi: sr: support runtime pm

[Aaron Lu]

On Fri, Jan 25, 2013 at 03:37:28PM -0500, Jeff Garzik wrote:
> On 01/23/2013 02:09 AM, Aaron Lu wrote:
> >This patch adds runtime pm support for sr.
> >
> >It did this by increasing the runtime usage_count of the device when
> >its block device is accessed. And decreasing the runtime usage_count
> >of the device when the access is done.
> >
> >If there is media inside, runtime suspend is not allowed as we don't
> >always know if the ODD is being used or not.
> >
> >The idea is discussed here:
> >http://thread.gmane.org/gmane.linux.acpi.devel/55243/focus=52703
> >and the restriction to check media inside is discussed here:
> >http://thread.gmane.org/gmane.linux.ide/53665/focus=58836
> >
> >Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> >Acked-by: Alan Stern <stern@rowland.harvard.edu>
> >Acked-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> >---
> >  drivers/scsi/sr.c | 42 +++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 39 insertions(+), 3 deletions(-)
> 
> applied 1-3

Now that the whole patchset is queued, I would like to thank all of you,
especially Alan Stern and Tejun Heo, for your kind help, without that I
would not be able to make it.

Thanks and best regards,
Aaron

Re: [PATCH v14 1/3] scsi: sr: support runtime pm

[Tejun Heo]

Hey, Aaron.

Thank *you* for the persistence.  It was a pleasure to work with you. :)

-- 
tejun