ZPODD Patches
Subsystems (5)
v8 cover ZPODD Patches
v8:
This version is a redesign, it doesn't have much to do with previous versions. The ZPODD implementation is done almost entirely in ATA layer now, except 2 helper functions from SCSI sr driver to block disk events.
The basic idea is that, when ata port is runtime suspended, it will check if the ODD is ready to be powered off. And if yes, events is blocked and power omitted; if not, ODD's power supply remains unchanged by keeping ACPI state at D0.
Some background knowledge about ZPODD is added below v1 history log.
v7:
Re work of runtime pm of sr driver, based on ideas of Alan Stern and Oliver Neukum.
Jeff, due to the ready_to_power_off flag added, there is a small change in [PATCH v7 6/6] libata: acpi: respect may_power_off flag, please check if I can still get your ack, thanks.
v6:
When user changes may_power_off flag through sysfs entry and if device is already runtime suspended, resume resume it so that it can respect this flag next time it is runtime suspended as suggested by Alan Stern. Call scsi_autopm_get/put_device once in sr_check_events as suggested by Alan Stern.
v5:
Add may_power_off flag to scsi device.
Alan Stern suggested that I should not mess runtime suspend with runtime power off, but the current zpodd implementation made it not easy to seperate. So I re-wrote the zpodd implementation, the end result is, normal ODD can also enter runtime suspended state, but their power won't be removed.
v4:
Rebase on top of Linus' tree, due to this, the problem of a missing flag in v3 is gone;
Add a new function scsi_autopm_put_device_autosuspend to first mark last busy for the device and then put autosuspend it as suggested by Oliver Neukum.
Typo fix as pointed by Sergei Shtylyov.
Check can_power_off flag before any runtime pm operations in sr.
v3:
Rebase on top of scsi-misc tree;
Add the sr related patches previously in Jeff's libata tree; Re-organize the sr patches.
A problem for now: for patch
scsi: sr: support zero power ODD(ZPODD)
I can't set a flag in libata-acpi.c since a related function is missing in scsi-misc tree. Will fix this when 3.6-rc1 released.
v2:
Bug fix for v1;
Use scsi_autopm_* in sr driver instead of pm_runtime_*;
v1:
Here are some patches to make ZPODD easier to use for end users and a fix for using ZPODD with system suspend.
Some background knowledge about ZPODD:
ODD means Optical Disc Drive.
ZPODD means Zero Power ODD, it is a mechanism to place the ODD into zero power state when the system is running at S0 system state without user's awareness.
It achieved this by ACPI and SATA device attention pin. For power off, normal ACPI control method is used to place the device into D3 cold ACPI device state, aka. device power supply omitted. For power on, when user press the eject button of a drawer type ODD or when user inserts an ODD into a slot type ODD, the device attention pin will trigger. In the current x86 implementation, this pin will connect to a GPE, and the GPE will trigger an ACPI interrupt. With our pre-registered ACPI notification code, the device can be runtime resumed, and we place the device back to full power state by setting its ACPI state to D0. The whole process is transparent to the end user.
Aaron Lu (11):
scsi: sr: support runtime pm
ata: zpodd: Add CONFIG_SATA_ZPODD
ata: zpodd: identify and init ZPODD devices
libata: acpi: move acpi notification code to sata_zpodd
libata-eh: allow defer in ata_exec_internal
ata: zpodd: check loading mechanism for ODD
libata: separate ATAPI code
ata: zpodd: check zero power ready status
block: add a new interface to block events
scsi: sr: support (un)block events
ata: zpodd: handle power transition of ODD
block/genhd.c | 26 ++++ drivers/ata/Kconfig | 12 ++ drivers/ata/Makefile | 3 +- drivers/ata/libata-acpi.c | 143 +++++++++----------- drivers/ata/libata-atapi.c | 88 +++++++++++++ drivers/ata/libata-atapi.h | 7 + drivers/ata/libata-core.c | 54 ++++++-- drivers/ata/libata-eh.c | 86 +----------- drivers/ata/libata-scsi.c | 5 + drivers/ata/sata_zpodd.c | 321 +++++++++++++++++++++++++++++++++++++++++++++ drivers/ata/sata_zpodd.h | 37 ++++++ drivers/scsi/Makefile | 1 + drivers/scsi/sr.c | 30 ++++- drivers/scsi/sr_zpodd.c | 21 +++ drivers/scsi/sr_zpodd.h | 9 ++ include/linux/cdrom.h | 35 +++++ include/linux/genhd.h | 1 + 17 files changed, 695 insertions(+), 184 deletions(-) create mode 100644 drivers/ata/libata-atapi.c create mode 100644 drivers/ata/libata-atapi.h create mode 100644 drivers/ata/sata_zpodd.c create mode 100644 drivers/ata/sata_zpodd.h create mode 100644 drivers/scsi/sr_zpodd.c create mode 100644 drivers/scsi/sr_zpodd.h
Signature · 1 line
1.7.12.4
PATCH v8 1/11 scsi: sr: support runtime pm
This patch adds runtime pm support for sr.
It did this by increasing the runtime usage_count of the device when:
- its block device is opened;
- the events checking is to run.
And decreasing the runtime usage_count of the device when:
- its block device is closed;
- After the events checking is done.
The idea is discussed in this mail thread:
http://thread.gmane.org/gmane.linux.acpi.devel/55243/focus=52703
--- drivers/scsi/sr.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)
drivers/scsi/sr.c +25 -5
@@ -45,6 +45,7 @@#include <linux/blkdev.h> #include <linux/mutex.h> #include <linux/slab.h> +#include <linux/pm_runtime.h> #include <asm/uaccess.h> #include <scsi/scsi.h>
@@ -146,7 +147,8 @@ static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)kref_get (&cd->kref);if (scsi_device_get (cd->device)) goto out_put; - goto out; +if (!scsi_autopm_get_device (cd->device)) + goto out; out_put:kref_put (&cd->kref, sr_kref_release);
@@ -162,6 +164,7 @@ static void scsi_cd_put(struct scsi_cd *cd)mutex_lock (&sr_ref_mutex);kref_put (&cd->kref, sr_kref_release); +scsi_autopm_put_device (sdev);scsi_device_put (sdev);mutex_unlock (&sr_ref_mutex); }
@@ -211,7 +214,7 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi, unsigned int clearing, int slot) {struct scsi_cd *cd = cdi->handle; -bool last_present; +bool last_present = cd->media_present;struct scsi_sense_hdr sshdr;unsigned int events;int ret;
@@ -220,6 +223,8 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,if (CDSL_CURRENT != slot)return 0 ; +scsi_autopm_get_device (cd->device); + events =sr_get_events (cd->device); cd->get_event_changed |= events &DISK_EVENT_MEDIA_CHANGE ;
@@ -246,10 +251,9 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi, }if (!(clearing &DISK_EVENT_MEDIA_CHANGE )) -return events; + goto out; do_tur:/* let's see whether the media is there with TUR */ - last_present = cd->media_present; ret =scsi_test_unit_ready (cd->device,SR_TIMEOUT ,MAX_RETRIES , &sshdr);/*
@@ -270,7 +274,7 @@ do_tur: }if (cd->ignore_get_event) -return events; + goto out;/* check whether GET_EVENT is reporting spurious MEDIA_CHANGE */ if (!cd->tur_changed) {
@@ -287,6 +291,18 @@ do_tur: cd->tur_changed = false; cd->get_event_changed = false; +out: +/* +* If there is no medium detected or the medium has been there +* since last poll, try to suspend the device. Otherwise, keep +* it active for one more poll interval so that if user space +* application opens the block device, we can avoid a runtime +* status change. +*/ +pm_runtime_put_noidle (&cd->device->sdev_gendev); +if (!cd->media_present || last_present) +pm_runtime_suspend (&cd->device->sdev_gendev); +return events; }
@@ -718,6 +734,8 @@ static int sr_probe(struct device *dev)sdev_printk (KERN_DEBUG , sdev,"Attached scsi CD-ROM %s\n" , cd->cdi.name); +scsi_autopm_put_device (cd->device); +return 0 ; fail_put:
@@ -965,6 +983,8 @@ static int sr_remove(struct device *dev) {struct scsi_cd *cd =dev_get_drvdata (dev); +scsi_autopm_get_device (cd->device); +blk_queue_prep_rq (cd->device->request_queue, scsi_prep_fn);del_gendisk (cd->disk);
Signature · 1 line
1.7.12.4
PATCH v8 2/11 ata: zpodd: Add CONFIG_SATA_ZPODD
In reply to Aaron LuAdded a new config CONFIG_SATA_ZPODD, which is ued to support SATA based zero power ODD. It depends on ACPI, and selects BLK_DEV_SR as the implementation of ZPODD depends on SCSI sr driver.
2 new files are added, which will be used to host ZPODD related code. They are empty for this commit.
--- drivers/ata/Kconfig | 12 ++++++++++++ drivers/ata/Makefile | 1 + 2 files changed, 13 insertions(+) create mode 100644 drivers/ata/sata_zpodd.c create mode 100644 drivers/ata/sata_zpodd.h
drivers/ata/Kconfig +12 -0
@@ -58,6 +58,18 @@ config ATA_ACPI You can disable this at kernel boot time by using the option libata.noacpi=1 +config SATA_ZPODD + bool "SATA Zero Power ODD Support" + depends on ACPI + select BLK_DEV_SR + default n + help + This option adds support for SATA ZPODD. It requires both + ODD and the platform support, and if enabled, will automatically + power on/off the ODD when certain condition is satisfied. This + does not impact user's experience of the ODD, only power is saved + when ODD is not in use(i.e. no disc inside). + config SATA_PMP bool "SATA Port Multiplier support" default y
drivers/ata/Makefile +1 -0
@@ -107,3 +107,4 @@ libata-y := libata-core.o libata-scsi.o libata-eh.o libata-transport.o libata-$(CONFIG_ATA_SFF) += libata-sff.o libata-$(CONFIG_SATA_PMP) += libata-pmp.o libata-$(CONFIG_ATA_ACPI) += libata-acpi.o +obj-$(CONFIG_SATA_ZPODD) += sata_zpodd.o
drivers/ata/sata_zpodd.c new file +0 -0
drivers/ata/sata_zpodd.h new file +0 -0
Signature · 1 line
1.7.12.4
James Bottomley
maintainerRe: [PATCH v8 02/11] ata: zpodd: Add CONFIG_SATA_ZPODD
Quoted 6 lines · Aaron Lu
Added a new config
CONFIG_SATA_ZPODD, which is ued to support SATA based zero power ODD. It depends on ACPI, and selectsBLK_DEV_SRas the implementation of ZPODD depends on SCSI sr driver.2 new files are added, which will be used to host ZPODD related code. They are empty for this commit.
I think you actually need to combine this with patch 3 to have it compile ...
James
Quoted 46 lines
Signed-off-by Aaron Lu <[email protected]>--- drivers/ata/Kconfig | 12 ++++++++++++ drivers/ata/Makefile | 1 + 2 files changed, 13 insertions(+) create mode 100644 drivers/ata/sata_zpodd.c create mode 100644 drivers/ata/sata_zpodd.hdrivers/ata/Kconfig +12
-0@@ -58,6 +58,18 @@ config ATA_ACPI You can disable this at kernel boot time by using the option libata.noacpi=1 +config SATA_ZPODD + bool "SATA Zero Power ODD Support" + depends on ACPI + select BLK_DEV_SR + default n + help + This option adds support for SATA ZPODD. It requires both + ODD and the platform support, and if enabled, will automatically + power on/off the ODD when certain condition is satisfied. This + does not impact user's experience of the ODD, only power is saved + when ODD is not in use(i.e. no disc inside). + config SATA_PMP bool "SATA Port Multiplier support" default ydrivers/ata/Makefile +1
-0@@ -107,3 +107,4 @@ libata-y := libata-core.o libata-scsi.o libata-eh.o libata-transport.o libata-$(CONFIG_ATA_SFF) += libata-sff.o libata-$(CONFIG_SATA_PMP) += libata-pmp.o libata-$(CONFIG_ATA_ACPI) += libata-acpi.o +obj-$(CONFIG_SATA_ZPODD) += sata_zpodd.odrivers/ata/sata_zpodd.c new file +0
-0drivers/ata/sata_zpodd.h new file +0
-0
Re: [PATCH v8 02/11] ata: zpodd: Add CONFIG_SATA_ZPODD
Quoted 10 lines · James Bottomley
Quoted 6 lines · Aaron Lu
Added a new config
CONFIG_SATA_ZPODD, which is ued to support SATA based zero power ODD. It depends on ACPI, and selectsBLK_DEV_SRas the implementation of ZPODD depends on SCSI sr driver.2 new files are added, which will be used to host ZPODD related code. They are empty for this commit.
I think you actually need to combine this with patch 3 to have it compile ...
I remembered I tested this, and I just tested the compile again, with the CONFIG_SATA_ZPODD both on and off, and it didn't have any compile issue, so I'm not sure if this is a problem...
-Aaron
Quoted 52 lines
James
Quoted 46 lines
Signed-off-by Aaron Lu <[email protected]>--- drivers/ata/Kconfig | 12 ++++++++++++ drivers/ata/Makefile | 1 + 2 files changed, 13 insertions(+) create mode 100644 drivers/ata/sata_zpodd.c create mode 100644 drivers/ata/sata_zpodd.hdrivers/ata/Kconfig +12
-0@@ -58,6 +58,18 @@ config ATA_ACPI You can disable this at kernel boot time by using the option libata.noacpi=1 +config SATA_ZPODD + bool "SATA Zero Power ODD Support" + depends on ACPI + select BLK_DEV_SR + default n + help + This option adds support for SATA ZPODD. It requires both + ODD and the platform support, and if enabled, will automatically + power on/off the ODD when certain condition is satisfied. This + does not impact user's experience of the ODD, only power is saved + when ODD is not in use(i.e. no disc inside). + config SATA_PMP bool "SATA Port Multiplier support" default ydrivers/ata/Makefile +1
-0@@ -107,3 +107,4 @@ libata-y := libata-core.o libata-scsi.o libata-eh.o libata-transport.o libata-$(CONFIG_ATA_SFF) += libata-sff.o libata-$(CONFIG_SATA_PMP) += libata-pmp.o libata-$(CONFIG_ATA_ACPI) += libata-acpi.o +obj-$(CONFIG_SATA_ZPODD) += sata_zpodd.odrivers/ata/sata_zpodd.c new file +0
-0drivers/ata/sata_zpodd.h new file +0
-0
PATCH v8 3/11 ata: zpodd: identify and init ZPODD devices
In reply to Aaron LuIf the ODD supports device attention and the platform can runtime power off it through ACPI, it means this ODD together with this platform is ZPODD capable. For this case, zpodd_init is called and a new structure is allocated for the device to store ZPODD related stuffs.
And the zpodd_dev_enabled function is used to test if ZPODD is currently enabled for this ODD.
--- drivers/ata/libata-acpi.c | 32 ++++++++++++++++++++++++++++++++ drivers/ata/sata_zpodd.c | 23 +++++++++++++++++++++++ drivers/ata/sata_zpodd.h | 24 ++++++++++++++++++++++++ 3 files changed, 79 insertions(+)
drivers/ata/libata-acpi.c +32 -0
@@ -19,6 +19,7 @@#include <linux/pm_runtime.h> #include <scsi/scsi_device.h> #include "libata.h" +#include "sata_zpodd.h" #include <acpi/acpi_bus.h>
@@ -1051,14 +1052,45 @@ static void ata_acpi_unregister_power_resource(struct ata_device *dev)acpi_power_resource_unregister_device (device, handle); } +static bool ata_acpi_device_poweroff (struct ata_device *ata_dev) +{ +acpi_handle handle; +acpi_status status; +struct acpi_device_power_state *states; +struct acpi_device *acpi_dev; + + handle =DEVICE_ACPI_HANDLE (&ata_dev->sdev->sdev_gendev); +if (!handle) +return false; + + status =acpi_bus_get_device (handle, &acpi_dev); +if (ACPI_FAILURE (status)) +return false; + +/* +* If firmware has _PS3 or _PR3 for this device, +* it means this device can be runtime powered off +*/ + states = acpi_dev->power.states; +if (states[ACPI_STATE_D3_HOT ].flags.valid || + states[ACPI_STATE_D3_COLD ].flags.explicit_set) +return true; +else +return false; +} +void ata_acpi_bind (struct ata_device *dev) {ata_acpi_add_pm_notifier (dev);ata_acpi_register_power_resource (dev); +if (dev->flags &ATA_DFLAG_DA &&ata_acpi_device_poweroff (dev)) +zpodd_init (dev); }void ata_acpi_unbind (struct ata_device *dev) { +if (zpodd_dev_enabled (dev)) +zpodd_deinit (dev);ata_acpi_remove_pm_notifier (dev);ata_acpi_unregister_power_resource (dev); }
drivers/ata/sata_zpodd.c +23 -0
@@ -0,0 +1,23 @@ +#include <linux/libata.h> + +struct zpodd { +struct ata_device *dev; +}; + + +void zpodd_init (struct ata_device *dev) +{ +struct zpodd *zpodd; + zpodd =kzalloc (sizeof (struct zpodd ),GFP_KERNEL ); +if (!zpodd) +return ; + + zpodd->dev = dev; + dev->private_data = zpodd; +} + +void zpodd_deinit (struct ata_device *dev) +{ +kfree (dev->private_data); + dev->private_data =NULL ; +}
drivers/ata/sata_zpodd.h +24 -0
@@ -0,0 +1,24 @@ +#ifndef __SATA_ZPODD_H__ +#define __SATA_ZPODD_H__ + +#include <linux/libata.h> + +#ifdef CONFIG_SATA_ZPODD +void zpodd_init (struct ata_device *); +void zpodd_deinit (struct ata_device *); + +static bool zpodd_dev_enabled (struct ata_device *dev) +{ +if (dev->flags &ATA_DFLAG_DA && dev->private_data) +return true; +else +return false; +} + +#else +static inline void zpodd_init (struct ata_device *dev) {} +static inline void zpodd_deinit (struct ata_device *dev) {} +static inline bool zpodd_dev_enabled (struct ata_device *dev) {return false; } +#endif + +#endif
Signature · 1 line
1.7.12.4
PATCH v8 4/11 libata: acpi: move acpi notification code to sata_zpodd
In reply to Aaron LuSince the ata acpi notification code introduced in commit 3bd46600a7a7e938c54df8cdbac9910668c7dfb0 is solely for zero power ODD, and we now have a dedicated place for ZPODD, move these code there.
And the add/remove_pm_notifier code is simplified a little bit that it does not check things like if the handle is NULL and if a corresponding acpi_device is there for the handle as they are guaranteed by the ata_acpi_device_poweroff already.
--- drivers/ata/libata-acpi.c | 71 ----------------------------------------------- drivers/ata/sata_zpodd.c | 32 +++++++++++++++++++++ 2 files changed, 32 insertions(+), 71 deletions(-)
drivers/ata/libata-acpi.c +0 -71
@@ -971,57 +971,6 @@ void ata_acpi_on_disable(struct ata_device *dev)ata_acpi_clear_gtf (dev); } -static void ata_acpi_wake_dev (acpi_handle handle,u32 event,void *context) -{ -struct ata_device *ata_dev = context; - -if (event ==ACPI_NOTIFY_DEVICE_WAKE && ata_dev && -pm_runtime_suspended (&ata_dev->sdev->sdev_gendev)) -scsi_autopm_get_device (ata_dev->sdev); -} - -static void ata_acpi_add_pm_notifier (struct ata_device *dev) -{ -struct acpi_device *acpi_dev; -acpi_handle handle; -acpi_status status; - - handle =ata_dev_acpi_handle (dev); -if (!handle) -return ; - - status =acpi_bus_get_device (handle, &acpi_dev); -if (ACPI_FAILURE (status)) -return ; - -if (dev->sdev->can_power_off) { -acpi_install_notify_handler (handle,ACPI_SYSTEM_NOTIFY , - ata_acpi_wake_dev, dev); -device_set_run_wake (&dev->sdev->sdev_gendev, true); - } -} - -static void ata_acpi_remove_pm_notifier (struct ata_device *dev) -{ -struct acpi_device *acpi_dev; -acpi_handle handle; -acpi_status status; - - handle =ata_dev_acpi_handle (dev); -if (!handle) -return ; - - status =acpi_bus_get_device (handle, &acpi_dev); -if (ACPI_FAILURE (status)) -return ; - -if (dev->sdev->can_power_off) { -device_set_run_wake (&dev->sdev->sdev_gendev, false); -acpi_remove_notify_handler (handle,ACPI_SYSTEM_NOTIFY , - ata_acpi_wake_dev); - } -} -static void ata_acpi_register_power_resource (struct ata_device *dev) {struct scsi_device *sdev = dev->sdev;
@@ -1081,7 +1030,6 @@ static bool ata_acpi_device_poweroff(struct ata_device *ata_dev)void ata_acpi_bind (struct ata_device *dev) { -ata_acpi_add_pm_notifier (dev);ata_acpi_register_power_resource (dev);if (dev->flags &ATA_DFLAG_DA &&ata_acpi_device_poweroff (dev))zpodd_init (dev);
@@ -1091,7 +1039,6 @@ void ata_acpi_unbind(struct ata_device *dev) {if (zpodd_dev_enabled (dev))zpodd_deinit (dev); -ata_acpi_remove_pm_notifier (dev);ata_acpi_unregister_power_resource (dev); }
@@ -1133,9 +1080,6 @@ static int ata_acpi_bind_device(struct ata_port *ap, struct scsi_device *sdev, acpi_handle *handle) {struct ata_device *ata_dev; -acpi_status status; -struct acpi_device *acpi_dev; -struct acpi_device_power_state *states;if (ap->flags &ATA_FLAG_ACPI_SATA ) ata_dev = &ap->link.device[sdev->channel];
@@ -1147,21 +1091,6 @@ static int ata_acpi_bind_device(struct ata_port *ap, struct scsi_device *sdev,if (!*handle)return -ENODEV ; - status =acpi_bus_get_device (*handle, &acpi_dev); -if (ACPI_FAILURE (status)) -return 0 ; - -/* -* If firmware has _PS3 or _PR3 for this device, -* and this ata ODD device support device attention, -* it means this device can be powered off -*/ - states = acpi_dev->power.states; -if ((states[ACPI_STATE_D3_HOT ].flags.valid || - states[ACPI_STATE_D3_COLD ].flags.explicit_set) && - ata_dev->flags &ATA_DFLAG_DA ) - sdev->can_power_off =1 ; -return 0 ; }
drivers/ata/sata_zpodd.c +32 -0
@@ -1,9 +1,39 @@#include <linux/libata.h> +#include <linux/pm_runtime.h> +#include <scsi/scsi_device.h> struct zpodd { +bool from_notify:1 ;/* resumed as a result of acpi notification */ +struct ata_device *dev; }; +static void zpodd_wake_dev (acpi_handle handle,u32 event,void *context) +{ +struct ata_device *ata_dev = context; +struct zpodd *zpodd = ata_dev->private_data; +struct device *dev = &ata_dev->sdev->sdev_gendev; + +if (event ==ACPI_NOTIFY_DEVICE_WAKE &&pm_runtime_suspended (dev)) { + zpodd->from_notify = true; +pm_runtime_resume (dev); + } +} + +static void zpodd_acpi_add_pm_notifier (struct ata_device *dev) +{ +acpi_handle handle =DEVICE_ACPI_HANDLE (&dev->sdev->sdev_gendev); +acpi_install_notify_handler (handle,ACPI_SYSTEM_NOTIFY , + zpodd_wake_dev, dev); +device_set_run_wake (&dev->sdev->sdev_gendev, true); +} + +static void zpodd_acpi_remove_pm_notifier (struct ata_device *dev) +{ +acpi_handle handle =DEVICE_ACPI_HANDLE (&dev->sdev->sdev_gendev); +device_set_run_wake (&dev->sdev->sdev_gendev, false); +acpi_remove_notify_handler (handle,ACPI_SYSTEM_NOTIFY , zpodd_wake_dev); +}void zpodd_init (struct ata_device *dev) {
@@ -12,12 +42,14 @@ void zpodd_init(struct ata_device *dev)if (!zpodd)return ; +zpodd_acpi_add_pm_notifier (dev); zpodd->dev = dev; dev->private_data = zpodd; }void zpodd_deinit (struct ata_device *dev) { +zpodd_acpi_remove_pm_notifier (dev);kfree (dev->private_data); dev->private_data =NULL ; }
Signature · 1 line
1.7.12.4
PATCH v8 5/11 libata-eh: allow defer in ata_exec_internal
In reply to Aaron Luata_exec_internal will preempt the ata link's active_tag and ata port's qc_active flags, this is OK for error recovery, but if normal code path wants to use ata_exec_internal, there is a problem: we need to check if it is OK to issue a new command with the help of port_ops->defer.
In ZPODD, I'll need to find out the loading mechanism of the ODD by issuing a GET_CONFIGURATION command. And this command may very well race with commands issued from SCSI layer. So instead of preempt the current command, defer the new command if it's not OK to issue it, as it is always wrong to issue a non-NCQ command when there is command(s) in processing.
So ata_exec_internal is modified to check if it is in eh recovery environment, and if yes, act as before; if not, check if this command should be defered with the help of port_ops->defer.
--- drivers/ata/libata-core.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-)
drivers/ata/libata-core.c +22 -12
@@ -1557,6 +1557,7 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,unsigned long flags;unsigned int err_mask;int rc; +bool eh_in_recover;spin_lock_irqsave (ap->lock, flags);
@@ -1588,14 +1589,21 @@ unsigned ata_exec_internal_sg(struct ata_device *dev, qc->dev = dev;ata_qc_reinit (qc); - preempted_tag = link->active_tag; - preempted_sactive = link->sactive; - preempted_qc_active = ap->qc_active; - preempted_nr_active_links = ap->nr_active_links; - link->active_tag =ATA_TAG_POISON ; - link->sactive =0 ; - ap->qc_active =0 ; - ap->nr_active_links =0 ; + eh_in_recover = ap->pflags &ATA_PFLAG_EH_IN_PROGRESS ; +if (eh_in_recover) { + preempted_tag = link->active_tag; + preempted_sactive = link->sactive; + preempted_qc_active = ap->qc_active; + preempted_nr_active_links = ap->nr_active_links; + link->active_tag =ATA_TAG_POISON ; + link->sactive =0 ; + ap->qc_active =0 ; + ap->nr_active_links =0 ; + }else if (ap->ops->qc_defer && ap->ops->qc_defer (qc)) { +ata_qc_free (qc); +spin_unlock_irqrestore (ap->lock, flags); +return -EBUSY ; + }/* prepare & issue qc */ qc->tf = *tf;
@@ -1687,10 +1695,12 @@ unsigned ata_exec_internal_sg(struct ata_device *dev, err_mask = qc->err_mask;ata_qc_free (qc); - link->active_tag = preempted_tag; - link->sactive = preempted_sactive; - ap->qc_active = preempted_qc_active; - ap->nr_active_links = preempted_nr_active_links; +if (eh_in_recover) { + link->active_tag = preempted_tag; + link->sactive = preempted_sactive; + ap->qc_active = preempted_qc_active; + ap->nr_active_links = preempted_nr_active_links; + }spin_unlock_irqrestore (ap->lock, flags);
Signature · 1 line
1.7.12.4
Re: [PATCH v8 05/11] libata-eh: allow defer in ata_exec_internal
Quoted 11 lines · Aaron Lu
ata_exec_internalwill preempt the ata link'sactive_tagand ata port'sqc_activeflags, this is OK for error recovery, but if normal code path wants to useata_exec_internal, there is a problem: we need to check if it is OK to issue a new command with the help ofport_ops->defer.In ZPODD, I'll need to find out the loading mechanism of the ODD by issuing a
GET_CONFIGURATIONcommand. And this command may very well race with commands issued from SCSI layer. So instead of preempt the current command, defer the new command if it's not OK to issue it, as it is always wrong to issue a non-NCQ command when there is command(s) in processing.
Why not do the discovery from EH?
Thanks.
Signature · 1 line
tejun
Re: [PATCH v8 05/11] libata-eh: allow defer in ata_exec_internal
Quoted 6 lines · Tejun Heo
Quoted 5 lines · Aaron Lu
ata_exec_internalwill preempt the ata link'sactive_tagand ata port'sqc_activeflags, this is OK for error recovery, but if normal code path wants to useata_exec_internal, there is a problem: we need to check if it is OK to issue a new command with the help ofport_ops->defer.
Quoted 8 lines
Quoted 6 lines
In ZPODD, I'll need to find out the loading mechanism of the ODD by issuing a
GET_CONFIGURATIONcommand. And this command may very well race with commands issued from SCSI layer. So instead of preempt the current command, defer the new command if it's not OK to issue it, as it is always wrong to issue a non-NCQ command when there is command(s) in processing.Why not do the discovery from EH?
Do you mean set a device level EH flag and then schedule EH to do the discovery?
Thanks,
Aaron
Re: [PATCH v8 05/11] libata-eh: allow defer in ata_exec_internal
Hello,
Quoted 4 lines · Aaron Lu
Quoted 1 line
Why not do the discovery from EH?
Do you mean set a device level EH flag and then schedule EH to do the discovery?
Yeah, something like that or why can't it be done while probing the device in the first place?
Thanks.
Signature · 1 line
tejun
Re: [PATCH v8 05/11] libata-eh: allow defer in ata_exec_internal
Quoted 10 lines · Tejun Heo
Hello,
Quoted 4 lines · Aaron Lu
Quoted 1 line
Why not do the discovery from EH?
Do you mean set a device level EH flag and then schedule EH to do the discovery?
Yeah, something like that or why can't it be done while probing the device in the first place?
I'm not aware of a place to store such ODD specific information when probing the device.
I'm currently storing the loading mech type in structure zpodd, which gets created after the corresponding SCSI device gets created in ata_scsi_scan_host, so at the probe time, the zpodd structure does not exist yet. And the reason I create the zpodd sturcture this late is that, it is only created when the ODD together the platform is ZPODD capable, and to find out if this platform is ZPODD capbale, ACPI binding has to occur first, and ACPI binding happens when SCSI device is added to the device tree.
Thanks,
Aaron
Re: [PATCH v8 05/11] libata-eh: allow defer in ata_exec_internal
Hello,
Quoted 2 lines · Aaron Lu
I'm not aware of a place to store such ODD specific information when probing the device.
You can always add some fields. :)
Quoted 8 lines
I'm currently storing the loading mech type in structure zpodd, which gets created after the corresponding SCSI device gets created in
ata_scsi_scan_host, so at the probe time, the zpodd structure does not exist yet. And the reason I create the zpodd sturcture this late is that, it is only created when the ODD together the platform is ZPODD capable, and to find out if this platform is ZPODD capbale, ACPI binding has to occur first, and ACPI binding happens when SCSI device is added to the device tree.
Hmm... I see. Which ACPI binding is it? The ATA ACPI binding happens during probing. It's a different one, I presume?
Thanks.
Signature · 1 line
tejun
Re: [PATCH v8 05/11] libata-eh: allow defer in ata_exec_internal
Quoted 7 lines · Tejun Heo
Hello,
Quoted 2 lines · Aaron Lu
I'm not aware of a place to store such ODD specific information when probing the device.
You can always add some fields. :)
OK. My concern is that, such information is only useful to ZPODD capable device+platforms, so checking this loading mechanism thing for all ATAPI devices during probe time doesn't seem a good idea.
Quoted 10 lines
Quoted 9 lines
I'm currently storing the loading mech type in structure zpodd, which gets created after the corresponding SCSI device gets created in
ata_scsi_scan_host, so at the probe time, the zpodd structure does not exist yet. And the reason I create the zpodd sturcture this late is that, it is only created when the ODD together the platform is ZPODD capable, and to find out if this platform is ZPODD capbale, ACPI binding has to occur first, and ACPI binding happens when SCSI device is added to the device tree.
Quoted 2 lines
Hmm... I see. Which ACPI binding is it? The ATA ACPI binding happens during probing. It's a different one, I presume?
Since commit 6b66d95895c149cbc04d4fac5a2f5477c543a8ae:
libata: bind the Linux device tree to the ACPI device tree ACPI binding happens when SCSI devices are added to the device tree. The ata port/device software structure does not have a acpi_handle field anymore.
Thanks,
Aaron
Re: [PATCH v8 05/11] libata-eh: allow defer in ata_exec_internal
Hello, Aaron.
Quoted 5 lines · Aaron Lu
Quoted 1 line
You can always add some fields. :)
OK. My concern is that, such information is only useful to ZPODD capable device+platforms, so checking this loading mechanism thing for all ATAPI devices during probe time doesn't seem a good idea.
Hmmm.. but it's not like querying acpi is high cost or anything. Maybe I'm missing something but if it can be simpler that way, please do so by all means. I don't care whether you add some extra fields or some processing overhead during probing. It doesn't really matter.
Quoted 8 lines
Quoted 2 lines
Hmm... I see. Which ACPI binding is it? The ATA ACPI binding happens during probing. It's a different one, I presume?
Since commit
6b66d95895c149cbc04d4fac5a2f5477c543a8ae:
libata: bind the Linux device tree to the ACPI device tree ACPI binding happens when SCSI devices are added to the device tree. The ata port/device software structure does not have aacpi_handlefield anymore.
Please bear with me. I haven't paid much attention to zpodd, so it probably is my ignorance but at least the ATA <-> ACPI association happens during probing by calling ata_acpi_on_devcfg() from ata_dev_configure(), and it pretty much has to happen then because _SDD/_GTF should be executed after hardreset which happens during probing. So, yeah, I'm confused.
Thanks.
Signature · 1 line
tejun
Re: [PATCH v8 05/11] libata-eh: allow defer in ata_exec_internal
Quoted 5 lines · Tejun Heo
Hello, Aaron.
Quoted 2 lines · Aaron Lu
Quoted 1 line
You can always add some fields. :)
Quoted 8 lines
Quoted 3 lines
OK. My concern is that, such information is only useful to ZPODD capable device+platforms, so checking this loading mechanism thing for all ATAPI devices during probe time doesn't seem a good idea.
Hmmm.. but it's not like querying acpi is high cost or anything. Maybe I'm missing something but if it can be simpler that way, please do so by all means. I don't care whether you add some extra fields or some processing overhead during probing. It doesn't really matter.
OK, thanks for the suggestion.
Quoted 10 lines
Quoted 9 lines
Quoted 3 lines
Hmm... I see. Which ACPI binding is it? The ATA ACPI binding happens during probing. It's a different one, I presume?
Since commit
6b66d95895c149cbc04d4fac5a2f5477c543a8ae:
libata: bind the Linux device tree to the ACPI device tree ACPI binding happens when SCSI devices are added to the device tree. The ata port/device software structure does not have aacpi_handlefield anymore.
Quoted 6 lines
Please bear with me. I haven't paid much attention to zpodd, so it probably is my ignorance but at least the ATA <-> ACPI association happens during probing by calling
ata_acpi_on_devcfg()fromata_dev_configure(), and it pretty much has to happen then because _SDD/_GTF should be executed after hardreset which happens during probing. So, yeah, I'm confused.
Oh, yes, ACPI handle can be retrieved any time by quering some ACPI method. And during probe time, it is done that way, while the binding happens much later... But the ACPI handle can indeed be fetched at that time :-)
So all the conditions I need to test ZPODD support is ready during probe time and I'll move the check loading mechanism code there, thanks for your suggestion.
Thanks,
Aaron
PATCH v8 6/11 ata: zpodd: check loading mechanism for ODD
In reply to Aaron LuPer the Mount Fuji spec on SATA ZPODD, only slot type or drawer type ODD can support ZPODD. So add a function to check this, and if fails, skip setting of dev->private_data so that zpodd_dev_enabled will return false.
--- drivers/ata/sata_zpodd.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/cdrom.h | 35 +++++++++++++++++++ 2 files changed, 122 insertions(+)
drivers/ata/sata_zpodd.c +87 -0
@@ -1,13 +1,89 @@#include <linux/libata.h> #include <linux/pm_runtime.h> +#include <linux/cdrom.h> #include <scsi/scsi_device.h> +#include "libata.h" +struct zpodd { +bool slot:1 ; +bool drawer:1 ;bool from_notify:1 ;/* resumed as a result of acpi notification */ struct ata_device *dev; }; +static int zpodd_run_atapi_cmd (struct ata_device *dev,const char *cdb, +unsigned short cdb_len,char *buf,unsigned int buf_len) +{ +int ret =0 ; +struct ata_taskfile tf = {0 }; + + tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; + tf.command =ATA_CMD_PACKET ; + +if (buf) { + tf.protocol =ATAPI_PROT_PIO ; + tf.lbam = buf_len; + }else { + tf.protocol =ATAPI_PROT_NODATA ; + } + +ata_eh_acquire (dev->link->ap); + ret =ata_exec_internal (dev, &tf, cdb, buf ?DMA_FROM_DEVICE :DMA_NONE , + buf, buf_len,0 ); +ata_eh_release (dev->link->ap); + +return ret; +} + +/* +* Per the spec, only slot type and drawer type ODD can be supported +* +* Return 0 for slot, 1 for drawer, and -ENODEV for others or on error. +*/ +static int check_loading_mechanism (struct ata_device *dev) +{ +char buf[16 ]; +unsigned int ret, i, retry =10 ; +struct rm_feature_desc *desc = (void *)(buf +8 ); + +char cdb[] = {GPCMD_GET_CONFIGURATION , +2 ,/* only 1 feature descriptor requested */ +0 ,3 ,/* 3, removable medium feature */ +0 ,0 ,0 ,/* reserved */ +0 ,sizeof (buf), +0 ,0 ,0 , + }; + +/* +* When we issue this command in ATA layer, SCSI layer may also +* issue some commands to the ODD. To avoid this command failed +* due to defer, retry for several times. +*/ +for (i =0 ; i < retry; i++) { + ret =zpodd_run_atapi_cmd (dev, cdb,sizeof (cdb), + buf,sizeof (buf)); +if (!ret || ret != -EBUSY ) +break ; +msleep (20 ); + } +if (ret) +return -ENODEV ; + +if (be16_to_cpu (desc->feature_code) !=3 ) +return -ENODEV ; + +if (desc->mech_type ==0 && desc->load ==0 && desc->eject ==1 ) + ret =0 ;/* slot */ +else if (desc->mech_type ==1 && desc->load ==0 && desc->eject ==1 ) + ret =1 ;/* drawer */ +else + ret = -ENODEV ; + +return ret; +} +static void zpodd_wake_dev (acpi_handle handle,u32 event,void *context) {struct ata_device *ata_dev = context;
@@ -38,10 +114,21 @@ static void zpodd_acpi_remove_pm_notifier(struct ata_device *dev)void zpodd_init (struct ata_device *dev) {struct zpodd *zpodd; +int ret; + zpodd =kzalloc (sizeof (struct zpodd ),GFP_KERNEL );if (!zpodd)return ; +if ((ret =check_loading_mechanism (dev)) == -ENODEV ) { +kfree (zpodd); +return ; + } +if (ret ==0 ) + zpodd->slot = true; +else + zpodd->drawer = true; +zpodd_acpi_add_pm_notifier (dev); zpodd->dev = dev; dev->private_data = zpodd;
include/linux/cdrom.h +35 -0
@@ -806,6 +806,41 @@ struct rwrt_feature_desc {__u8 reserved3; }; +/* removable medium feature descriptor */ +struct rm_feature_desc { +__be16 feature_code; +#if defined(__BIG_ENDIAN_BITFIELD) +__u8 reserved1:2 ; +__u8 feature_version:4 ; +__u8 persistent:1 ; +__u8 curr:1 ; +#elif defined(__LITTLE_ENDIAN_BITFIELD) +__u8 curr:1 ; +__u8 persistent:1 ; +__u8 feature_version:4 ; +__u8 reserved1:2 ; +#endif +__u8 add_len; +#if defined(__BIG_ENDIAN_BITFIELD) +__u8 mech_type:3 ; +__u8 load:1 ; +__u8 eject:1 ; +__u8 pvnt_jmpr:1 ; +__u8 dbml:1 ; +__u8 lock:1 ; +#elif defined(__LITTLE_ENDIAN_BITFIELD) +__u8 lock:1 ; +__u8 dbml:1 ; +__u8 pvnt_jmpr:1 ; +__u8 eject:1 ; +__u8 load:1 ; +__u8 mech_type:3 ; +#endif +__u8 reserved2; +__u8 reserved3; +__u8 reserved4; +}; + typedef struct { __be16 disc_information_length; #if defined(__BIG_ENDIAN_BITFIELD)
Signature · 1 line
1.7.12.4
PATCH v8 7/11 libata: separate ATAPI code
In reply to Aaron LuThe atapi_eh_tur and atapi_eh_request_sense can be reused by ZPODD code, so separate them out to a file named libata-atapi.c. A header file libata-atapi.h is added and the Makefile is modified accordingly.
--- drivers/ata/Makefile | 2 +- drivers/ata/libata-atapi.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++ drivers/ata/libata-atapi.h | 7 ++++ drivers/ata/libata-eh.c | 86 +------------------------------------------- 4 files changed, 97 insertions(+), 86 deletions(-) create mode 100644 drivers/ata/libata-atapi.c create mode 100644 drivers/ata/libata-atapi.h
drivers/ata/Makefile +1 -1
@@ -103,7 +103,7 @@ obj-$(CONFIG_ATA_GENERIC) += ata_generic.o # Should be last libata driver obj-$(CONFIG_PATA_LEGACY) += pata_legacy.o -libata-y := libata-core.o libata-scsi.o libata-eh.o libata-transport.o +libata-y := libata-core.o libata-scsi.o libata-eh.o libata-atapi.o libata-transport.o libata-$(CONFIG_ATA_SFF) += libata-sff.o libata-$(CONFIG_SATA_PMP) += libata-pmp.o libata-$(CONFIG_ATA_ACPI) += libata-acpi.o
drivers/ata/libata-atapi.c new file +88 -0
88 lines not on this page · raw message
drivers/ata/libata-atapi.h new file +7 -0
@@ -0,0 +1,7 @@ +#ifndef __LIBATA_ATAPI_H__ +#define __LIBATA_ATAPI_H__ + +unsigned int atapi_eh_tur (struct ata_device *dev,u8 *r_sense_key); +unsigned int atapi_eh_request_sense (struct ata_device *dev,u8 *sense_buf,u8 dfl_sense_key); + +#endif
drivers/ata/libata-eh.c +1 -85
98 lines not on this page · raw message
Signature · 1 line
1.7.12.4
PATCH v8 8/11 ata: zpodd: check zero power ready status
In reply to Aaron LuPer the Mount Fuji spec, the ODD is considered zero power ready when:
- For slot type ODD, no media inside;
- For tray type ODD, no media inside and tray closed.
The information can be retrieved by either the returned information of command GET_EVENT_STATUS_NOTIFICATION(the command is used to poll for media event) or sense code.
In this implementation, the zero power ready status is determined by the following factors:
1 polled media status byte, and this info is recorded in status_ready
field of zpodd structure;
2 sense code by issuing a TEST_UNIT_READY command after status_ready
is found to be true.
The information provided by the media status byte is not accurate, it is possible that after a new disc is just inserted, the status byte still returns media not present. So this information can not be used as the final deciding factor. But since SCSI ODD driver sr will always poll the ODD every 2 seconds, this information is readily available without any much cost. So it is used as a hint: when we find zero power ready status in the media status byte, we will see if it is really the case with the sense code method. This way, we can avoid sending too many TEST_UNIT_READY commands to the ODD.
When we first sensed the ODD in the zero power ready state, the timestamp will be recoreded. And after ODD stayed in this state for some pre-defined period, the ODD is considered as power off ready and the zp_ready flag will be set. The zp_ready flag serves as the deciding factor other code will use to see if power off is OK for the ODD. The Mount Fuji spec suggests a delay should be used here, to avoid the case user ejects the ODD and then instantly inserts a new one again, so that we can avoid a power transition. And some ODDs may be slow to place its head to the home position after disc is ejected, so a delay here is generally a good idea.
The zero power ready status check is performed in the ata port's runtime suspend code path, when port is not frozen yet, as we need to issue some IOs to the ODD.
--- drivers/ata/libata-acpi.c | 8 +++- drivers/ata/libata-scsi.c | 5 ++ drivers/ata/sata_zpodd.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++ drivers/ata/sata_zpodd.h | 5 ++ 4 files changed, 134 insertions(+), 1 deletion(-)
drivers/ata/libata-acpi.c +7 -1
@@ -785,7 +785,13 @@ static int ata_acpi_push_id(struct ata_device *dev)*/ int ata_acpi_on_suspend (struct ata_port *ap) { -/* nada */ +struct ata_device *dev; + +ata_for_each_dev (dev, &ap->link,ENABLED ) { +if (zpodd_dev_enabled (dev)) +zpodd_check_zpready (dev); + } +return 0 ; }
drivers/ata/libata-scsi.c +5 -0
@@ -53,6 +53,7 @@#include "libata.h" #include "libata-transport.h" +#include "sata_zpodd.h" #define ATA_SCSI_RBUF_SIZE 4096
@@ -2665,6 +2666,10 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)ata_scsi_rbuf_put (cmd, true, &flags); } +if (zpodd_dev_enabled (qc->dev) && + scsicmd[0 ] ==GET_EVENT_STATUS_NOTIFICATION ) +zpodd_snoop_status (qc->dev, cmd); + cmd->result =SAM_STAT_GOOD ; }
drivers/ata/sata_zpodd.c +117 -0
136 lines not on this page · raw message
drivers/ata/sata_zpodd.h +5 -0
@@ -2,6 +2,7 @@#define __SATA_ZPODD_H__#include <linux/libata.h> +#include <scsi/scsi_cmnd.h> #ifdef CONFIG_SATA_ZPODD void zpodd_init (struct ata_device *);
@@ -15,10 +16,14 @@ static bool zpodd_dev_enabled(struct ata_device *dev)return false; } +void zpodd_snoop_status (struct ata_device *,struct scsi_cmnd *); +void zpodd_check_zpready (struct ata_device *);#else static inline void zpodd_init (struct ata_device *dev) {}static inline void zpodd_deinit (struct ata_device *dev) {}static inline bool zpodd_dev_enabled (struct ata_device *dev) {return false; } +static inline void zpodd_snoop_status (struct ata_device *dev,struct scsi_cmnd *cmd) {} +static inline void zpodd_check_zpready (struct ata_device *dev) {}#endif #endif
Signature · 1 line
1.7.12.4
PATCH v8 9/11 block: add a new interface to block events
In reply to Aaron LuA new interface to block disk events is added, this interface is meant to eliminate a race between PM runtime callback and disk events checking.
Suppose the following device tree:device_sata_port (the parent)
device_ODD (the child)
When ODD is runtime suspended, sata port will have a chance to enter runtime suspended state. And in sata port's runtime suspend callback, it will check if it is OK to omit the power of the ODD. And if yes, the periodically running events checking work will be stopped, as the ODD will be waken up by that check and cancel it can make the ODD stay in zero power state much longer(no worry about how the ODD gets media change event in ZPODD's case).
I used disk_block_events to do the events blocking, but there is a race and can lead to a deadlock: when I call disk_block_events in sata port's runtime suspend callback, the events checking work may already be running and it will resume the ODD synchronously, and PM core will try to resume its parent, the sata port first. The PM core makes sure that runtime resume callback does not run concurrently with runtime suspend callback, and if the runtime suspend callback is running, the runtime resume callback will wait for it done. So the events checking work will wait for sata port's runtime suspend callback, while the sata port's runtime suspend callback is waiting for the disk events work to finish. Deadlock.
ODD_suspend disk_events_workfn
ata_port_suspend check_events
disk_block_events resume ODD
cancel_delayed_work_sync resume parent
(waiting for disk_events_workfn) (waiting for suspend callback)
So a new function disk_try_block_events is added, it will try to cancel the delayed work if the work is currently pending, which means it has been queued but the timer is not expired yet. If the work is not pending, we return false, as we do not know if the task is already running or not queued at all. And if the queued work is successfully cancelled, disk_block_events will be called and we are done. But if we failed to cancel the work, false will be returned, and the calling thread knows that it's not safe to block events at the moment and should act accordingly. In ZPODD's case, poweroff will not be attempted.
Note:
ODD means: Optical Disc Drive.
ZPODD means: Zero Power ODD. It's a mechnism to place the ODD into zero power state without user's awareness when the system is at S0 system state.
--- block/genhd.c | 26 ++++++++++++++++++++++++++ include/linux/genhd.h | 1 + 2 files changed, 27 insertions(+)
block/genhd.c +26 -0
@@ -1469,6 +1469,32 @@ void disk_block_events(struct gendisk *disk)mutex_unlock (&ev->block_mutex); } +/* +* Under some circumstances, there is a race between the calling thread +* of disk_block_events and the events checking function. To avoid such a race, +* this function will check if the delayed work is pending. If not, it means +* the work is either not queued or is already running, false is returned. +* And if yes, try to cancel the delayed work. If succedded, disk_block_events +* will be called and there is no worry that cancel_delayed_work_sync will +* deadlock the events checking function. And if failed, false is returned. +*/ +bool disk_try_block_events (struct gendisk *disk) +{ +struct disk_events *ev = disk->ev; + +if (!ev) +return false; + +if (delayed_work_pending (&ev->dwork)) { +if (cancel_delayed_work (&disk->ev->dwork)) { +disk_block_events (disk); +return true; + } + } + +return false; +} +static void __disk_unblock_events (struct gendisk *disk,bool check_now) {struct disk_events *ev = disk->ev;
include/linux/genhd.h +1 -0
@@ -420,6 +420,7 @@ static inline int get_disk_ro(struct gendisk *disk) }extern void disk_block_events (struct gendisk *disk); +extern bool disk_try_block_events (struct gendisk *disk);extern void disk_unblock_events (struct gendisk *disk);extern void disk_flush_events (struct gendisk *disk,unsigned int mask);extern unsigned int disk_clear_events (struct gendisk *disk,unsigned int mask);
Signature · 1 line
1.7.12.4
Re: [PATCH v8 09/11] block: add a new interface to block events
Hello,
Quoted 5 lines · Aaron Lu
ODD_suspenddisk_events_workfn
ata_port_suspendcheck_events
disk_block_eventsresume ODD
cancel_delayed_work_syncresume parent
(waiting fordisk_events_workfn) (waiting for suspend callback)
I don't understand why solving it needs to be this elaborate. check_event() can retry. Just add a per-sr mutex which is try-locked by sr_block_check_events() and grab it when entering zero power.
Quoted 7 lines
block/genhd.c +7
-0+/* +* Under some circumstances, there is a race between the calling thread +* of disk_block_events and the events checking function. To avoid such a race, +* this function will check if the delayed work is pending. If not, it means +* the work is either not queued or is already running, false is returned. +* And if yes, try to cancel the delayed work. If succedded, disk_block_events +* will be called and there is no worry that cancel_delayed_work_sync will
block/genhd.c void disk_block_events(struct gendisk *disk) · line 1488
Quoted 10 lines
block/genhd.c +10
-0+* deadlock the events checking function. And if failed, false is returned. +*/ +bool disk_try_block_events (struct gendisk *disk) +{ +struct disk_events *ev = disk->ev; + +if (!ev) +return false; + +if (delayed_work_pending (&ev->dwork)) {
And please don't use delayed_work_pending() like this. It doesn't add anything. cancel_delayed_work() already needs to perform all the necessary tests.
block/genhd.c void disk_block_events(struct gendisk *disk) · line 1496
Quoted 8 lines
block/genhd.c +8
-0+if (cancel_delayed_work (&disk->ev->dwork)) { +disk_block_events (disk); +return true; + } + } + +return false; +}
Thanks.
Signature · 1 line
tejun
Re: [PATCH v8 09/11] block: add a new interface to block events
Quoted 10 lines · Tejun Heo
Hello,
Quoted 5 lines · Aaron Lu
ODD_suspenddisk_events_workfn
ata_port_suspendcheck_events
disk_block_eventsresume ODD
cancel_delayed_work_syncresume parent
(waiting fordisk_events_workfn) (waiting for suspend callback)I don't understand why solving it needs to be this elaborate.
:-)
Quoted 2 lines
check_event()can retry. Just add a per-sr mutex which is try-locked bysr_block_check_events()and grab it when entering zero power.
Good suggestion. I didn't think about solving it this way.
Many people suggest me that ZPODD is pure SATA/ACPI stuff, and should not pollute sr driver, so I was trying hard not to touch sr while preparing these patches, unless there is no other choice(like the blocking event interface).
So I'm not sure if your suggestion is the way to go.
James, what do you think? Is it OK if I add a mutex into the scsi_cd structure to do this? Of course I'll define this only under CONFIG_SATA_ZPODD.
Quoted 14 lines
Quoted 14 lines
block/genhd.c +13
-0+/* +* Under some circumstances, there is a race between the calling thread +* of disk_block_events and the events checking function. To avoid such a race, +* this function will check if the delayed work is pending. If not, it means +* the work is either not queued or is already running, false is returned. +* And if yes, try to cancel the delayed work. If succedded, disk_block_events +* will be called and there is no worry that cancel_delayed_work_sync will +* deadlock the events checking function. And if failed, false is returned. +*/ +bool disk_try_block_events (struct gendisk *disk) +{ +struct disk_events *ev = disk->ev; +
Quoted 8 lines
block/genhd.c void disk_block_events(struct gendisk *disk) · line 1488
Quoted 4 lines
block/genhd.c +4
-0+if (!ev) +return false; + +if (delayed_work_pending (&ev->dwork)) {And please don't use
delayed_work_pending()like this. It doesn't add anything.cancel_delayed_work()already needs to perform all the necessary tests.
OK, thanks for the suggestion.
-Aaron
Quoted 12 lines
block/genhd.c void disk_block_events(struct gendisk *disk) · line 1496
Quoted 9 lines
block/genhd.c +8
-0+if (cancel_delayed_work (&disk->ev->dwork)) { +disk_block_events (disk); +return true; + } + } + +return false; +}Thanks.
Re: [PATCH v8 09/11] block: add a new interface to block events
Hello,
Quoted 10 lines · Aaron Lu
Quoted 2 lines
check_event()can retry. Just add a per-sr mutex which is try-locked bysr_block_check_events()and grab it when entering zero power.Good suggestion. I didn't think about solving it this way.
Many people suggest me that ZPODD is pure SATA/ACPI stuff, and should not pollute sr driver, so I was trying hard not to touch sr while preparing these patches, unless there is no other choice(like the blocking event interface).
Quoted 5 lines
So I'm not sure if your suggestion is the way to go.
James, what do you think? Is it OK if I add a mutex into the
scsi_cdstructure to do this? Of course I'll define this only underCONFIG_SATA_ZPODD.
I don't think what James' and my suggestions are that different. Just silence check_event() while zpodd is kicked in somehow. There's no reason to synchronize across multiple subsystems.
Thanks.
Signature · 1 line
tejun
Re: [PATCH v8 09/11] block: add a new interface to block events
Hi,
Quoted 13 lines · Tejun Heo
Hello,
Quoted 10 lines · Aaron Lu
Quoted 2 lines
check_event()can retry. Just add a per-sr mutex which is try-locked bysr_block_check_events()and grab it when entering zero power.Good suggestion. I didn't think about solving it this way.
Many people suggest me that ZPODD is pure SATA/ACPI stuff, and should not pollute sr driver, so I was trying hard not to touch sr while preparing these patches, unless there is no other choice(like the blocking event interface).
Quoted 8 lines
Quoted 5 lines
So I'm not sure if your suggestion is the way to go.
James, what do you think? Is it OK if I add a mutex into the
scsi_cdstructure to do this? Of course I'll define this only underCONFIG_SATA_ZPODD.I don't think what James' and my suggestions are that different. Just silence
check_event()while zpodd is kicked in somehow. There's no
Well, since sr is not supposed to know anything about ZPODD, I don't see another way to silence check_event() in ATA layer.
What I hope to achieve is when zero power ready status is sensed in ATA layer, no more sr_check_events should be called, as that function will runtime resume the ODD.
Thanks,
Aaron
Quoted 4 lines
reason to synchronize across multiple subsystems.
Thanks.
PATCH v8 10/11 scsi: sr: support (un)block events
In reply to Aaron Lu2 interfaces are added to block/unblock events for the disk sr manages. This is used by SATA ZPODD, when ODD is runtime powered off, the events poll is no longer needed so better be blocked. And once powered on, events poll will be unblocked.
These 2 interfaces are needed here because SATA layer does not have access to the gendisk structure sr manages.
--- drivers/scsi/Makefile | 1 + drivers/scsi/sr_zpodd.c | 21 +++++++++++++++++++++ drivers/scsi/sr_zpodd.h | 9 +++++++++ 3 files changed, 31 insertions(+) create mode 100644 drivers/scsi/sr_zpodd.c create mode 100644 drivers/scsi/sr_zpodd.h
drivers/scsi/Makefile +1 -0
@@ -177,6 +177,7 @@ sd_mod-objs := sd.o sd_mod-$(CONFIG_BLK_DEV_INTEGRITY) += sd_dif.o sr_mod-objs := sr.o sr_ioctl.o sr_vendor.o +sr_mod-$(CONFIG_SATA_ZPODD) += sr_zpodd.o ncr53c8xx-flags-$(CONFIG_SCSI_ZALON) \ := -DCONFIG_NCR53C8XX_PREFETCH -DSCSI_NCR_BIG_ENDIAN \ -DCONFIG_SCSI_NCR53C8XX_NO_WORD_TRANSFERS
drivers/scsi/sr_zpodd.c new file +21 -0
@@ -0,0 +1,21 @@ +#include <linux/module.h> +#include <linux/genhd.h> +#include <linux/cdrom.h> +#include "sr.h" + +bool sr_block_events (struct device *dev) +{ +struct scsi_cd *cd =dev_get_drvdata (dev); +if (cd) +return disk_try_block_events (cd->disk); +return false; +} +EXPORT_SYMBOL (sr_block_events); + +void sr_unblock_events (struct device *dev) +{ +struct scsi_cd *cd =dev_get_drvdata (dev); +if (cd) +disk_unblock_events (cd->disk); +} +EXPORT_SYMBOL (sr_unblock_events);
drivers/scsi/sr_zpodd.h new file +9 -0
@@ -0,0 +1,9 @@ +#ifndef __SR_ZPODD_H__ +#define __SR_ZPODD_H__ + +#include <linux/device.h> + +extern bool sr_block_events (struct device *dev); +extern void sr_unblock_events (struct device *dev); + +#endif
Signature · 1 line
1.7.12.4
James Bottomley
maintainerRe: [PATCH v8 10/11] scsi: sr: support (un)block events
Quoted 7 lines · Aaron Lu
2 interfaces are added to block/unblock events for the disk sr manages. This is used by SATA ZPODD, when ODD is runtime powered off, the events poll is no longer needed so better be blocked. And once powered on, events poll will be unblocked.
These 2 interfaces are needed here because SATA layer does not have access to the gendisk structure sr manages.
I'm afraid this is a nasty layering violation. You can't have a low level driver have knowledge of a call back in an upper layer one (in this case sr_block_events).
This is all done it looks like because of the problem of getting access to the scsi_cd structure from libata, but it's not the right way to solve it.
I also don't really think you need to do any blocking or unblocking. As I said in the previous thread, just fake the events the standard tells you to, so userspace can probe to its heart's content and you can keep the device powered off.
James
Quoted 63 lines
Signed-off-by Aaron Lu <[email protected]>--- drivers/scsi/Makefile | 1 + drivers/scsi/sr_zpodd.c | 21 +++++++++++++++++++++ drivers/scsi/sr_zpodd.h | 9 +++++++++ 3 files changed, 31 insertions(+) create mode 100644 drivers/scsi/sr_zpodd.c create mode 100644 drivers/scsi/sr_zpodd.hdrivers/scsi/Makefile +1
-0@@ -177,6 +177,7 @@ sd_mod-objs := sd.o sd_mod-$(CONFIG_BLK_DEV_INTEGRITY) += sd_dif.o sr_mod-objs := sr.o sr_ioctl.o sr_vendor.o +sr_mod-$(CONFIG_SATA_ZPODD) += sr_zpodd.o ncr53c8xx-flags-$(CONFIG_SCSI_ZALON) \ := -DCONFIG_NCR53C8XX_PREFETCH -DSCSI_NCR_BIG_ENDIAN \ -DCONFIG_SCSI_NCR53C8XX_NO_WORD_TRANSFERSdrivers/scsi/sr_zpodd.c new file +21
-0@@ -0,0 +1,21 @@ +#include <linux/module.h> +#include <linux/genhd.h> +#include <linux/cdrom.h> +#include "sr.h" + +bool sr_block_events (struct device *dev) +{ +struct scsi_cd *cd =dev_get_drvdata (dev); +if (cd) +return disk_try_block_events (cd->disk); +return false; +} +EXPORT_SYMBOL (sr_block_events); + +void sr_unblock_events (struct device *dev) +{ +struct scsi_cd *cd =dev_get_drvdata (dev); +if (cd) +disk_unblock_events (cd->disk); +} +EXPORT_SYMBOL (sr_unblock_events);drivers/scsi/sr_zpodd.h new file +9
-0@@ -0,0 +1,9 @@ +#ifndef __SR_ZPODD_H__ +#define __SR_ZPODD_H__ + +#include <linux/device.h> + +extern bool sr_block_events (struct device *dev); +extern void sr_unblock_events (struct device *dev); + +#endif
Re: [PATCH v8 10/11] scsi: sr: support (un)block events
Quoted 13 lines · James Bottomley
Quoted 7 lines · Aaron Lu
2 interfaces are added to block/unblock events for the disk sr manages. This is used by SATA ZPODD, when ODD is runtime powered off, the events poll is no longer needed so better be blocked. And once powered on, events poll will be unblocked.
These 2 interfaces are needed here because SATA layer does not have access to the gendisk structure sr manages.
I'm afraid this is a nasty layering violation. You can't have a low level driver have knowledge of a call back in an upper layer one (in this case
sr_block_events).
Quoted 8 lines
This is all done it looks like because of the problem of getting access to the
scsi_cdstructure from libata, but it's not the right way to solve it.I also don't really think you need to do any blocking or unblocking. As I said in the previous thread, just fake the events the standard tells you to, so userspace can probe to its heart's content and you can keep the device powered off.
James, one of us has misunderstood the event-polling mechanism. As far as I know, the polling calls that Aaron is concerned with are generated by the kernel, not by userspace.
Alan Stern
James Bottomley
maintainerRe: [PATCH v8 10/11] scsi: sr: support (un)block events
Quoted 19 lines · Alan Stern
Quoted 17 lines · James Bottomley
Quoted 7 lines · Aaron Lu
2 interfaces are added to block/unblock events for the disk sr manages. This is used by SATA ZPODD, when ODD is runtime powered off, the events poll is no longer needed so better be blocked. And once powered on, events poll will be unblocked.
These 2 interfaces are needed here because SATA layer does not have access to the gendisk structure sr manages.
I'm afraid this is a nasty layering violation. You can't have a low level driver have knowledge of a call back in an upper layer one (in this case
sr_block_events).This is all done it looks like because of the problem of getting access to the
scsi_cdstructure from libata, but it's not the right way to solve it.
Quoted 8 lines
Quoted 4 lines
I also don't really think you need to do any blocking or unblocking. As I said in the previous thread, just fake the events the standard tells you to, so userspace can probe to its heart's content and you can keep the device powered off.
James, one of us has misunderstood the event-polling mechanism. As far as I know, the polling calls that Aaron is concerned with are generated by the kernel, not by userspace.
I don't think the origin of the poll changes anything in what I said above, does it?
James
Re: [PATCH v8 10/11] scsi: sr: support (un)block events
In reply to James BottomleyQuoted 6 lines · James Bottomley
Quoted 5 lines · Aaron Lu
2 interfaces are added to block/unblock events for the disk sr manages. This is used by SATA ZPODD, when ODD is runtime powered off, the events poll is no longer needed so better be blocked. And once powered on, events poll will be unblocked.
Quoted 6 lines
Quoted 2 lines
These 2 interfaces are needed here because SATA layer does not have access to the gendisk structure sr manages.
I'm afraid this is a nasty layering violation. You can't have a low level driver have knowledge of a call back in an upper layer one (in this case
sr_block_events).
I agree. But since ZPODD is now implemented in ATA layer now, this seems the only way to do it.
Quoted 4 lines
This is all done it looks like because of the problem of getting access to the
scsi_cdstructure from libata, but it's not the right way to solve it.
Indeed, and any suggestion on a right way?
Quoted 5 lines
I also don't really think you need to do any blocking or unblocking. As I said in the previous thread, just fake the events the standard tells you to, so userspace can probe to its heart's content and you can keep the device powered off.
No problem for the device, the problem is for its ancestor devices in my current implementation. If the event is not blocked, the ODD will be runtime resumed every 2 seconds. And along the way, all its ancestor devices will have to be runtime resumed every 2 seconds, even I did some trick to avoid power on the ODD itself. This is not a good thing from power management perspective.
And there are two reasons I want to align runtime PM with ZPODD:
1 Simpler code.
And if I do not align ZPODD with runtime PM, I will need to maintain the ODD's software status myself, and deal with some race conditions like when I was to power off the ODD, a new command may arrive; or when the ODD is powering on, all arriving commands need to be failed for the moment untill the ODD is fully recovered from EH, etc. And all these things can be avoided if I rely on runtime PM framework.
2 Integrate with the whole system
With runtime PM, when ODD is suspended, its ancestor devices will have a chance to enter low power state.
Basically, I think power off is a special suspend state, it should happen after the device enters runtime suspended state, and this is the idea I used to implement ZPODD.
Thanks,
Aaron
Quoted 69 lines
James
Quoted 63 lines
Signed-off-by Aaron Lu <[email protected]>--- drivers/scsi/Makefile | 1 + drivers/scsi/sr_zpodd.c | 21 +++++++++++++++++++++ drivers/scsi/sr_zpodd.h | 9 +++++++++ 3 files changed, 31 insertions(+) create mode 100644 drivers/scsi/sr_zpodd.c create mode 100644 drivers/scsi/sr_zpodd.hdrivers/scsi/Makefile +1
-0@@ -177,6 +177,7 @@ sd_mod-objs := sd.o sd_mod-$(CONFIG_BLK_DEV_INTEGRITY) += sd_dif.o sr_mod-objs := sr.o sr_ioctl.o sr_vendor.o +sr_mod-$(CONFIG_SATA_ZPODD) += sr_zpodd.o ncr53c8xx-flags-$(CONFIG_SCSI_ZALON) \ := -DCONFIG_NCR53C8XX_PREFETCH -DSCSI_NCR_BIG_ENDIAN \ -DCONFIG_SCSI_NCR53C8XX_NO_WORD_TRANSFERSdrivers/scsi/sr_zpodd.c new file +21
-0@@ -0,0 +1,21 @@ +#include <linux/module.h> +#include <linux/genhd.h> +#include <linux/cdrom.h> +#include "sr.h" + +bool sr_block_events (struct device *dev) +{ +struct scsi_cd *cd =dev_get_drvdata (dev); +if (cd) +return disk_try_block_events (cd->disk); +return false; +} +EXPORT_SYMBOL (sr_block_events); + +void sr_unblock_events (struct device *dev) +{ +struct scsi_cd *cd =dev_get_drvdata (dev); +if (cd) +disk_unblock_events (cd->disk); +} +EXPORT_SYMBOL (sr_unblock_events);drivers/scsi/sr_zpodd.h new file +9
-0@@ -0,0 +1,9 @@ +#ifndef __SR_ZPODD_H__ +#define __SR_ZPODD_H__ + +#include <linux/device.h> + +extern bool sr_block_events (struct device *dev); +extern void sr_unblock_events (struct device *dev); + +#endif
PATCH v8 11/11 ata: zpodd: handle power transition of ODD
In reply to Aaron LuThe implementation of ZPODD aligns with runtime pm, which means the ODD will only be runtime powered off after it is runtime suspended.
And each time in ata port's suspend callback, a check is made to see if the ODD is ready to be powered off:
1 zp_ready is true;
2 events is successfully blocked.
If the 2 conditions are not met, ACPI_STATE_D0 is chosen since as far as ACPI is concerned about ODD's power management, only D0 and D3 is meanningful. There is no definition of D1/D2/D3hot state for ODD, so avoid setting those ACPI states.
And on resume, it will re-gain power and go through the recovery process. When EH for the ata port is done, the ODD is considered functional, and post processing like eject tray if the ODD is drawer type is done there.
--- drivers/ata/libata-acpi.c | 34 +++++++++++++++++++------- drivers/ata/libata-core.c | 20 ++++++++++++++- drivers/ata/sata_zpodd.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++ drivers/ata/sata_zpodd.h | 8 ++++++ 4 files changed, 114 insertions(+), 10 deletions(-)
drivers/ata/libata-acpi.c +25 -9
@@ -839,6 +839,24 @@ void ata_acpi_on_resume(struct ata_port *ap) } } +static int ata_acpi_choose_state (struct ata_device *dev) +{ +int acpi_state; + +/* Always choose D3 for PATA devices */ +if (!(dev->link->ap->flags &ATA_FLAG_ACPI_SATA )) +return ACPI_STATE_D3 ; + + acpi_state =acpi_pm_device_sleep_state (&dev->sdev->sdev_gendev, +NULL ,ACPI_STATE_D3 ); + +if (acpi_state ==ACPI_STATE_D3 &&zpodd_dev_enabled (dev) && + !zpodd_poweroff_ready (dev)) + acpi_state =ACPI_STATE_D0 ; + +return acpi_state; +} +/** * ata_acpi_set_state - set the port power state * @ap: target ATA port
@@ -865,17 +883,15 @@ void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)continue ;if (state.event !=PM_EVENT_ON ) { - acpi_state =acpi_pm_device_sleep_state ( - &dev->sdev->sdev_gendev,NULL ,ACPI_STATE_D3 ); -if (acpi_state >0 ) + acpi_state =ata_acpi_choose_state (dev); +if (acpi_state >0 ) {acpi_bus_set_power (handle, acpi_state); -/* TBD: need to check if it's runtime pm request */ -acpi_pm_device_run_wake ( - &dev->sdev->sdev_gendev, true); +if (zpodd_dev_enabled (dev)) +zpodd_post_poweroff (dev); + } }else { -/* Ditto */ -acpi_pm_device_run_wake ( - &dev->sdev->sdev_gendev, false); +if (zpodd_dev_enabled (dev)) +zpodd_pre_poweron (dev);acpi_bus_set_power (handle,ACPI_STATE_D0 ); } }
drivers/ata/libata-core.c +19 -1
@@ -70,6 +70,7 @@#include "libata.h" #include "libata-transport.h" +#include "sata_zpodd.h" /* debounce timing parameters in msecs { interval, duration, timeout } */ const unsigned long sata_deb_timing_normal[] = {5 ,100 ,2000 };
@@ -5399,6 +5400,23 @@ static int ata_port_runtime_idle(struct device *dev)return pm_runtime_suspend (dev); } +static int ata_port_runtime_resume (struct device *dev) +{ +int rc; +struct ata_device *ata_dev; +struct ata_port *ap =to_ata_port (dev); + + rc =ata_port_resume_common (dev); +if (!rc) { +ata_for_each_dev (ata_dev, &ap->link,ENABLED ) { +if (zpodd_dev_enabled (ata_dev)) +zpodd_post_resume (ata_dev); + } + } + +return rc; +} +static const struct dev_pm_ops ata_port_pm_ops = { .suspend = ata_port_suspend, .resume = ata_port_resume,
@@ -5408,7 +5426,7 @@ static const struct dev_pm_ops ata_port_pm_ops = { .restore = ata_port_resume, .runtime_suspend = ata_port_suspend, - .runtime_resume = ata_port_resume_common, + .runtime_resume = ata_port_runtime_resume, .runtime_idle = ata_port_runtime_idle, };
drivers/ata/sata_zpodd.c +62 -0
86 lines not on this page · raw message
drivers/ata/sata_zpodd.h +8 -0
@@ -18,12 +18,20 @@ static bool zpodd_dev_enabled(struct ata_device *dev)void zpodd_snoop_status (struct ata_device *,struct scsi_cmnd *);void zpodd_check_zpready (struct ata_device *); +bool zpodd_poweroff_ready (struct ata_device *); +void zpodd_post_poweroff (struct ata_device *); +void zpodd_pre_poweron (struct ata_device *); +void zpodd_post_resume (struct ata_device *);#else static inline void zpodd_init (struct ata_device *dev) {}static inline void zpodd_deinit (struct ata_device *dev) {}static inline bool zpodd_dev_enabled (struct ata_device *dev) {return false; }static inline void zpodd_snoop_status (struct ata_device *dev,struct scsi_cmnd *cmd) {}static inline void zpodd_check_zpready (struct ata_device *dev) {} +static inline bool zpodd_poweroff_ready (struct ata_device *dev) {return false; } +static inline void zpodd_post_poweroff (struct ata_device *dev) {} +static inline void zpodd_pre_poweron (struct ata_device *dev) {} +static inline void zpodd_post_resume (struct ata_device *dev) {}#endif #endif
Signature · 1 line
1.7.12.4