[PATCH v3 0/7] SATA ZPODD support

[PATCH v3 0/7] SATA ZPODD support

[Lin Ming]

Hi all,

This is the v3 patches to add SATA ZPODD support.

I split the ACPI D3Cold state support patches out.
You can find it here:

[PATCH v2 0/2] ACPI D3Cold state support
https://lkml.org/lkml/2012/3/27/52

v3:
- Split the ACPI D3Cold state support patches out
- Adds "Device Attention" bit check

v2:
- _PR3 indicates D3Cold support
- move can_power_off flag to pm_subsys_data
- allow all combinations of power resource and device
- split patches into smaller ones to make review easy

v1:
https://lkml.org/lkml/2012/2/13/86

[PATCH v3 1/7] libata-acpi: set acpi state for SATA port
[PATCH v3 2/7] libata-acpi: add ata port runtime D3Cold support
[PATCH v3 3/7] libata-acpi: register/unregister device to/from power resource
[PATCH v3 4/7] libata: detech Device Attention support
[PATCH v3 5/7] libata: tell scsi layer device supports runtime power off
[PATCH v3 6/7] PM / Runtime: Add can_power_off flag to subsys data
[PATCH v3 7/7] [SCSI] sr: adds Zero-power ODD support

 drivers/ata/libata-acpi.c  |  137 +++++++++++++++++++++++++++++++++++++++----
 drivers/ata/libata-core.c  |    3 +
 drivers/ata/libata-scsi.c  |    8 ++-
 drivers/ata/libata.h       |    8 +-
 drivers/scsi/scsi_pm.c     |    8 +++
 drivers/scsi/sr.c          |   44 ++++++++++++++
 drivers/scsi/sr.h          |    2 +
 include/linux/ata.h        |    1 +
 include/linux/libata.h     |    2 +
 include/linux/pm.h         |    1 +
 include/scsi/scsi_device.h |    3 +
 11 files changed, 197 insertions(+), 20 deletions(-)

[PATCH v3 1/7] libata-acpi: set acpi state for SATA port

[Lin Ming]

Currently, ata_acpi_set_state() only sets acpi sate for IDE port.
Remove this limitation.

Acked-by: Aaron Lu <aaron.lu@amd.com>
Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/ata/libata-acpi.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index b03e468..104c1d0 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -841,23 +841,26 @@ void ata_acpi_on_resume(struct ata_port *ap)
 void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
 {
 	struct ata_device *dev;
-
-	if (!ata_ap_acpi_handle(ap) || (ap->flags & ATA_FLAG_ACPI_SATA))
-		return;
+	acpi_handle handle;
+	int acpi_state;
 
 	/* channel first and then drives for power on and vica versa
 	   for power off */
-	if (state.event == PM_EVENT_ON)
-		acpi_bus_set_power(ata_ap_acpi_handle(ap), ACPI_STATE_D0);
+	handle = ata_ap_acpi_handle(ap);
+	if (handle && state.event == PM_EVENT_ON)
+		acpi_bus_set_power(handle, ACPI_STATE_D0);
 
 	ata_for_each_dev(dev, &ap->link, ENABLED) {
-		if (ata_dev_acpi_handle(dev))
-			acpi_bus_set_power(ata_dev_acpi_handle(dev),
+		handle = ata_dev_acpi_handle(dev);
+		if (handle)
+			acpi_bus_set_power(handle,
 				state.event == PM_EVENT_ON ?
 					ACPI_STATE_D0 : ACPI_STATE_D3);
 	}
-	if (state.event != PM_EVENT_ON)
-		acpi_bus_set_power(ata_ap_acpi_handle(ap), ACPI_STATE_D3);
+
+	handle = ata_ap_acpi_handle(ap);
+	if (handle && state.event != PM_EVENT_ON)
+		acpi_bus_set_power(handle, ACPI_STATE_D3);
 }
 
 /**
-- 
1.7.2.5

Re: [PATCH v3 1/7] libata-acpi: set acpi state for SATA port

[Aaron Lu]

Hi,

The patch does not apply, are you using libata-devel/ALL branch?

-Aaron

On Wed, Mar 28, 2012 at 02:21:56PM +0800, Lin Ming wrote:
> Currently, ata_acpi_set_state() only sets acpi sate for IDE port.
> Remove this limitation.
> 
> Acked-by: Aaron Lu <aaron.lu@amd.com>
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> ---
>  drivers/ata/libata-acpi.c |   21 ++++++++++++---------
>  1 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index b03e468..104c1d0 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -841,23 +841,26 @@ void ata_acpi_on_resume(struct ata_port *ap)
>  void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
>  {
>  	struct ata_device *dev;
> -
> -	if (!ata_ap_acpi_handle(ap) || (ap->flags & ATA_FLAG_ACPI_SATA))
> -		return;
> +	acpi_handle handle;
> +	int acpi_state;
>  
>  	/* channel first and then drives for power on and vica versa
>  	   for power off */
> -	if (state.event == PM_EVENT_ON)
> -		acpi_bus_set_power(ata_ap_acpi_handle(ap), ACPI_STATE_D0);
> +	handle = ata_ap_acpi_handle(ap);
> +	if (handle && state.event == PM_EVENT_ON)
> +		acpi_bus_set_power(handle, ACPI_STATE_D0);
>  
>  	ata_for_each_dev(dev, &ap->link, ENABLED) {
> -		if (ata_dev_acpi_handle(dev))
> -			acpi_bus_set_power(ata_dev_acpi_handle(dev),
> +		handle = ata_dev_acpi_handle(dev);
> +		if (handle)
> +			acpi_bus_set_power(handle,
>  				state.event == PM_EVENT_ON ?
>  					ACPI_STATE_D0 : ACPI_STATE_D3);
>  	}
> -	if (state.event != PM_EVENT_ON)
> -		acpi_bus_set_power(ata_ap_acpi_handle(ap), ACPI_STATE_D3);
> +
> +	handle = ata_ap_acpi_handle(ap);
> +	if (handle && state.event != PM_EVENT_ON)
> +		acpi_bus_set_power(handle, ACPI_STATE_D3);
>  }
>  
>  /**
> -- 
> 1.7.2.5
> 
> 

Re: [PATCH v3 1/7] libata-acpi: set acpi state for SATA port

[Lin Ming]

On Thu, 2012-03-29 at 03:55 +0800, Aaron Lu wrote:
> Hi,
> 
> The patch does not apply, are you using libata-devel/ALL branch?

This patch set depends on below patches,

[PATCHv2 0/7] acpi/libata: Express dependencies for devices on dock
stations and bays

which were in github.com libata-dev tree.

But I didn't find these patches in git.kernel.org libata-dev tree.
http://marc.info/?l=linux-scsi&m=133283600431697&w=2

Lin Ming

> 
> -Aaron
> 
> On Wed, Mar 28, 2012 at 02:21:56PM +0800, Lin Ming wrote:
> > Currently, ata_acpi_set_state() only sets acpi sate for IDE port.
> > Remove this limitation.
> > 
> > Acked-by: Aaron Lu <aaron.lu@amd.com>
> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > ---
> >  drivers/ata/libata-acpi.c |   21 ++++++++++++---------
> >  1 files changed, 12 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> > index b03e468..104c1d0 100644
> > --- a/drivers/ata/libata-acpi.c
> > +++ b/drivers/ata/libata-acpi.c
> > @@ -841,23 +841,26 @@ void ata_acpi_on_resume(struct ata_port *ap)
> >  void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
> >  {
> >  	struct ata_device *dev;
> > -
> > -	if (!ata_ap_acpi_handle(ap) || (ap->flags & ATA_FLAG_ACPI_SATA))
> > -		return;
> > +	acpi_handle handle;
> > +	int acpi_state;
> >  
> >  	/* channel first and then drives for power on and vica versa
> >  	   for power off */
> > -	if (state.event == PM_EVENT_ON)
> > -		acpi_bus_set_power(ata_ap_acpi_handle(ap), ACPI_STATE_D0);
> > +	handle = ata_ap_acpi_handle(ap);
> > +	if (handle && state.event == PM_EVENT_ON)
> > +		acpi_bus_set_power(handle, ACPI_STATE_D0);
> >  
> >  	ata_for_each_dev(dev, &ap->link, ENABLED) {
> > -		if (ata_dev_acpi_handle(dev))
> > -			acpi_bus_set_power(ata_dev_acpi_handle(dev),
> > +		handle = ata_dev_acpi_handle(dev);
> > +		if (handle)
> > +			acpi_bus_set_power(handle,
> >  				state.event == PM_EVENT_ON ?
> >  					ACPI_STATE_D0 : ACPI_STATE_D3);
> >  	}
> > -	if (state.event != PM_EVENT_ON)
> > -		acpi_bus_set_power(ata_ap_acpi_handle(ap), ACPI_STATE_D3);
> > +
> > +	handle = ata_ap_acpi_handle(ap);
> > +	if (handle && state.event != PM_EVENT_ON)
> > +		acpi_bus_set_power(handle, ACPI_STATE_D3);
> >  }
> >  
> >  /**
> > -- 
> > 1.7.2.5
> > 
> > 
> 

[PATCH v3 2/7] libata-acpi: add ata port runtime D3Cold support

[Lin Ming]

ATA port may support runtime D3Cold state, for example, Zero-power ODD case.
This patch adds wakeup notifier and enable/disable run_wake during
supend/resume.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/ata/libata-acpi.c |   82 +++++++++++++++++++++++++++++++++++++++++---
 drivers/ata/libata-scsi.c |    6 ++--
 drivers/ata/libata.h      |    8 ++--
 3 files changed, 83 insertions(+), 13 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 104c1d0..ba9382a 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -16,6 +16,7 @@
 #include <linux/libata.h>
 #include <linux/pci.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 #include <scsi/scsi_device.h>
 #include "libata.h"
 
@@ -852,10 +853,23 @@ void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
 
 	ata_for_each_dev(dev, &ap->link, ENABLED) {
 		handle = ata_dev_acpi_handle(dev);
-		if (handle)
-			acpi_bus_set_power(handle,
-				state.event == PM_EVENT_ON ?
-					ACPI_STATE_D0 : ACPI_STATE_D3);
+		if (!handle)
+			continue;
+
+		if (state.event != PM_EVENT_ON) {
+			acpi_state = acpi_pm_device_sleep_state(
+				&dev->sdev->sdev_gendev, NULL);
+			if (acpi_state > 0)
+				acpi_bus_set_power(handle, acpi_state);
+			if (ap->tdev.power.request == RPM_REQ_SUSPEND)
+				acpi_pm_device_run_wake(
+					&dev->sdev->sdev_gendev, true);
+		} else {
+			if (ap->tdev.power.request == RPM_REQ_RESUME)
+				acpi_pm_device_run_wake(
+					&dev->sdev->sdev_gendev, false);
+			acpi_bus_set_power(handle, ACPI_STATE_D0);
+		}
 	}
 
 	handle = ata_ap_acpi_handle(ap);
@@ -955,7 +969,7 @@ void ata_acpi_on_disable(struct ata_device *dev)
 	ata_acpi_clear_gtf(dev);
 }
 
-void ata_acpi_bind_dock(struct ata_device *dev)
+static void ata_acpi_bind_dock(struct ata_device *dev)
 {
 	struct device **docks;
 
@@ -965,7 +979,7 @@ void ata_acpi_bind_dock(struct ata_device *dev)
 	kfree(docks);
 }
 
-void ata_acpi_unbind_dock(struct ata_device *dev)
+static void ata_acpi_unbind_dock(struct ata_device *dev)
 {
 	struct device **docks;
 
@@ -975,6 +989,62 @@ void ata_acpi_unbind_dock(struct ata_device *dev)
 	kfree(docks);
 }
 
+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)
+		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_SUCCESS(status)) {
+		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_SUCCESS(status)) {
+		device_set_run_wake(&dev->sdev->sdev_gendev, false);
+		acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
+			ata_acpi_wake_dev);
+	}
+}
+
+void ata_acpi_bind(struct ata_device *dev)
+{
+	ata_acpi_bind_dock(dev);
+	ata_acpi_add_pm_notifier(dev);
+}
+
+void ata_acpi_unbind(struct ata_device *dev)
+{
+	ata_acpi_unbind_dock(dev);
+	ata_acpi_remove_pm_notifier(dev);
+}
+
 static int is_pci_ata(struct device *dev)
 {
 	struct pci_dev *pdev;
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index f08c447..231c3ec 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3444,7 +3444,7 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
 			if (!IS_ERR(sdev)) {
 				dev->sdev = sdev;
 				scsi_device_put(sdev);
-				ata_acpi_bind_dock(dev);
+				ata_acpi_bind(dev);
 			} else {
 				dev->sdev = NULL;
 			}
@@ -3541,12 +3541,12 @@ static void ata_scsi_remove_dev(struct ata_device *dev)
 	mutex_lock(&ap->scsi_host->scan_mutex);
 	spin_lock_irqsave(ap->lock, flags);
 
+	ata_acpi_unbind(dev);
+
 	/* clearing dev->sdev is protected by host lock */
 	sdev = dev->sdev;
 	dev->sdev = NULL;
 
-	ata_acpi_unbind_dock(dev);
-
 	if (sdev) {
 		/* If user initiated unplug races with us, sdev can go
 		 * away underneath us after the host lock and
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index e61ba10..2ec7c38 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -117,8 +117,8 @@ extern void ata_acpi_on_disable(struct ata_device *dev);
 extern void ata_acpi_set_state(struct ata_port *ap, pm_message_t state);
 extern int ata_acpi_register(void);
 extern void ata_acpi_unregister(void);
-extern void ata_acpi_bind_dock(struct ata_device *dev);
-extern void ata_acpi_unbind_dock(struct ata_device *dev);
+extern void ata_acpi_bind(struct ata_device *dev);
+extern void ata_acpi_unbind(struct ata_device *dev);
 #else
 static inline void ata_acpi_dissociate(struct ata_host *host) { }
 static inline int ata_acpi_on_suspend(struct ata_port *ap) { return 0; }
@@ -129,8 +129,8 @@ static inline void ata_acpi_set_state(struct ata_port *ap,
 				      pm_message_t state) { }
 static inline int ata_acpi_register(void) { return 0; }
 static void ata_acpi_unregister(void) { }
-static void ata_acpi_bind_dock(struct ata_device *dev) { }
-static void ata_acpi_unbind_dock(struct ata_device *dev) { }
+static void ata_acpi_bind(struct ata_device *dev) { }
+static void ata_acpi_unbind(struct ata_device *dev) { }
 #endif
 
 /* libata-scsi.c */
-- 
1.7.2.5

[PATCH v3 3/7] libata-acpi: register/unregister device to/from power resource

[Lin Ming]

Uses the interface introduced in earlier patch.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/ata/libata-acpi.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index ba9382a..81ef0f5 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -1033,16 +1033,48 @@ static void ata_acpi_remove_pm_notifier(struct ata_device *dev)
 	}
 }
 
+static void ata_acpi_register_power_resource(struct ata_device *dev)
+{
+	struct scsi_device *sdev = dev->sdev;
+	acpi_handle handle;
+	struct device *device;
+
+	handle = ata_dev_acpi_handle(dev);
+	if (!handle)
+		return;
+
+	device = &sdev->sdev_gendev;
+
+	acpi_power_resource_register_device(device, handle);
+}
+
+static void ata_acpi_unregister_power_resource(struct ata_device *dev)
+{
+	struct scsi_device *sdev = dev->sdev;
+	acpi_handle handle;
+	struct device *device;
+
+	handle = ata_dev_acpi_handle(dev);
+	if (!handle)
+		return;
+
+	device = &sdev->sdev_gendev;
+
+	acpi_power_resource_unregister_device(device, handle);
+}
+
 void ata_acpi_bind(struct ata_device *dev)
 {
 	ata_acpi_bind_dock(dev);
 	ata_acpi_add_pm_notifier(dev);
+	ata_acpi_register_power_resource(dev);
 }
 
 void ata_acpi_unbind(struct ata_device *dev)
 {
 	ata_acpi_unbind_dock(dev);
 	ata_acpi_remove_pm_notifier(dev);
+	ata_acpi_unregister_power_resource(dev);
 }
 
 static int is_pci_ata(struct device *dev)
-- 
1.7.2.5

[PATCH v3 4/7] libata: detech Device Attention support

[Lin Ming]

Add a new flag ATA_DFLAG_DA to indicate that device supports "Device
Attention".

Acked-by: Aaron Lu <aaron.lu@amd.com>
Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/ata/libata-core.c |    3 +++
 include/linux/ata.h       |    1 +
 include/linux/libata.h    |    2 ++
 3 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 6742cc4..2a10701 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2368,6 +2368,9 @@ int ata_dev_configure(struct ata_device *dev)
 			dma_dir_string = ", DMADIR";
 		}
 
+		if (atapi_id_has_da(dev->id))
+			dev->flags |= ATA_DFLAG_DA;
+
 		/* print device info to dmesg */
 		if (ata_msg_drv(ap) && print_info)
 			ata_dev_info(dev,
diff --git a/include/linux/ata.h b/include/linux/ata.h
index 32df2b6..6ee41cc 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -578,6 +578,7 @@ static inline int ata_is_data(u8 prot)
 	  ((u64) (id)[(n) + 0]) )
 
 #define ata_id_cdb_intr(id)	(((id)[ATA_ID_CONFIG] & 0x60) == 0x20)
+#define atapi_id_has_da(id)	((id)[77] & (1 << 4))
 
 static inline bool ata_id_has_hipm(const u16 *id)
 {
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 36970c6..3085bdc 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -161,6 +161,8 @@ enum {
 	ATA_DFLAG_DETACH	= (1 << 24),
 	ATA_DFLAG_DETACHED	= (1 << 25),
 
+	ATA_DFLAG_DA		= (1 << 26), /* device supports Device Attention */
+
 	ATA_DEV_UNKNOWN		= 0,	/* unknown device */
 	ATA_DEV_ATA		= 1,	/* ATA device */
 	ATA_DEV_ATA_UNSUP	= 2,	/* ATA device (unsupported) */
-- 
1.7.2.5

Re: [PATCH v3 4/7] libata: detech Device Attention support

[Sergei Shtylyov]

Hello.

On 28-03-2012 10:21, Lin Ming wrote:

> Add a new flag ATA_DFLAG_DA to indicate that device supports "Device
> Attention".

> Acked-by: Aaron Lu<aaron.lu@amd.com>
> Signed-off-by: Lin Ming<ming.m.lin@intel.com>
[...]

> diff --git a/include/linux/ata.h b/include/linux/ata.h
> index 32df2b6..6ee41cc 100644
> --- a/include/linux/ata.h
> +++ b/include/linux/ata.h
> @@ -578,6 +578,7 @@ static inline int ata_is_data(u8 prot)
>   	  ((u64) (id)[(n) + 0]) )
>
>   #define ata_id_cdb_intr(id)	(((id)[ATA_ID_CONFIG]&  0x60) == 0x20)
> +#define atapi_id_has_da(id)	((id)[77]&  (1<<  4))

    We prefix these macros with 'ata_' despite them being applied to IDENTIFY 
DEVICE data or IDENTIFY PAKCET DEVICE data. ata_id_cdb_intr(), for example, 
applies only to IDENTIFY PAKCET DEVICE data.

MBR, Sergei

Re: [PATCH v3 4/7] libata: detech Device Attention support

[Lin Ming]

On Wed, 2012-03-28 at 15:22 +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 28-03-2012 10:21, Lin Ming wrote:
> 
> > Add a new flag ATA_DFLAG_DA to indicate that device supports "Device
> > Attention".
> 
> > Acked-by: Aaron Lu<aaron.lu@amd.com>
> > Signed-off-by: Lin Ming<ming.m.lin@intel.com>
> [...]
> 
> > diff --git a/include/linux/ata.h b/include/linux/ata.h
> > index 32df2b6..6ee41cc 100644
> > --- a/include/linux/ata.h
> > +++ b/include/linux/ata.h
> > @@ -578,6 +578,7 @@ static inline int ata_is_data(u8 prot)
> >   	  ((u64) (id)[(n) + 0]) )
> >
> >   #define ata_id_cdb_intr(id)	(((id)[ATA_ID_CONFIG]&  0x60) == 0x20)
> > +#define atapi_id_has_da(id)	((id)[77]&  (1<<  4))
> 
>     We prefix these macros with 'ata_' despite them being applied to IDENTIFY 
> DEVICE data or IDENTIFY PAKCET DEVICE data. ata_id_cdb_intr(), for example, 
> applies only to IDENTIFY PAKCET DEVICE data.

OK, will change it.

> 
> MBR, Sergei

Re: [PATCH v3 4/7] libata: detech Device Attention support

[Sergei Shtylyov]

Hello.

On 29-03-2012 9:06, Lin Ming wrote:

>>> Add a new flag ATA_DFLAG_DA to indicate that device supports "Device
>>> Attention".

>>> Acked-by: Aaron Lu<aaron.lu@amd.com>
>>> Signed-off-by: Lin Ming<ming.m.lin@intel.com>
>> [...]

>>> diff --git a/include/linux/ata.h b/include/linux/ata.h
>>> index 32df2b6..6ee41cc 100644
>>> --- a/include/linux/ata.h
>>> +++ b/include/linux/ata.h
>>> @@ -578,6 +578,7 @@ static inline int ata_is_data(u8 prot)
>>>    	  ((u64) (id)[(n) + 0]) )
>>>
>>>    #define ata_id_cdb_intr(id)	(((id)[ATA_ID_CONFIG]&   0x60) == 0x20)
>>> +#define atapi_id_has_da(id)	((id)[77]&   (1<<   4))

>>      We prefix these macros with 'ata_' despite them being applied to IDENTIFY
>> DEVICE data or IDENTIFY PAKCET DEVICE data. ata_id_cdb_intr(), for example,
>> applies only to IDENTIFY PAKCET DEVICE data.

> OK, will change it.

    Also, what does the word "detech" mean in the subject?

MBR, Sergei

Re: [PATCH v3 4/7] libata: detech Device Attention support

[Lin Ming]

On Thu, Mar 29, 2012 at 7:14 PM, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> Hello.
>
>
> On 29-03-2012 9:06, Lin Ming wrote:
>
>>>> Add a new flag ATA_DFLAG_DA to indicate that device supports "Device
>>>> Attention".
>
>
>>>> Acked-by: Aaron Lu<aaron.lu@amd.com>
>>>> Signed-off-by: Lin Ming<ming.m.lin@intel.com>
>>>
>>> [...]
>
>
>>>> diff --git a/include/linux/ata.h b/include/linux/ata.h
>>>> index 32df2b6..6ee41cc 100644
>>>> --- a/include/linux/ata.h
>>>> +++ b/include/linux/ata.h
>>>> @@ -578,6 +578,7 @@ static inline int ata_is_data(u8 prot)
>>>>          ((u64) (id)[(n) + 0]) )
>>>>
>>>>   #define ata_id_cdb_intr(id)  (((id)[ATA_ID_CONFIG]&   0x60) == 0x20)
>>>> +#define atapi_id_has_da(id)    ((id)[77]&   (1<<   4))
>
>
>>>     We prefix these macros with 'ata_' despite them being applied to
>>> IDENTIFY
>>> DEVICE data or IDENTIFY PAKCET DEVICE data. ata_id_cdb_intr(), for
>>> example,
>>> applies only to IDENTIFY PAKCET DEVICE data.
>
>
>> OK, will change it.
>
>
>   Also, what does the word "detech" mean in the subject?

Typo! It should be "detect"

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

[PATCH v3 5/7] libata: tell scsi layer device supports runtime power off

[Lin Ming]

If ATA device supports "Device Attention", then tell scsi layer that
the device supports runtime power off.

Acked-by: Aaron Lu <aaron.lu@amd.com>
Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/ata/libata-scsi.c  |    2 ++
 include/scsi/scsi_device.h |    1 +
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 231c3ec..0bfbe41 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3445,6 +3445,8 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
 				dev->sdev = sdev;
 				scsi_device_put(sdev);
 				ata_acpi_bind(dev);
+				if (dev->flags & ATA_DFLAG_DA)
+					sdev->power_off = 1;
 			} else {
 				dev->sdev = NULL;
 			}
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 77273f2..0f69612 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -151,6 +151,7 @@ struct scsi_device {
 	unsigned no_read_disc_info:1;	/* Avoid READ_DISC_INFO cmds */
 	unsigned no_read_capacity_16:1; /* Avoid READ_CAPACITY_16 cmds */
 	unsigned is_visible:1;	/* is the device visible in sysfs */
+	unsigned power_off:1; /* Device supports runtime power off */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */
-- 
1.7.2.5

[PATCH v3 6/7] PM / Runtime: Add can_power_off flag to subsys data

[Lin Ming]

Parent device will check child's can_power_off flag to see if child device can
be powered off.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 include/linux/pm.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/linux/pm.h b/include/linux/pm.h
index e4982ac..2e74531 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -465,6 +465,7 @@ struct pm_subsys_data {
 #ifdef CONFIG_PM_GENERIC_DOMAINS
 	struct pm_domain_data *domain_data;
 #endif
+	bool can_power_off;
 };
 
 struct dev_pm_info {
-- 
1.7.2.5

[PATCH v3 7/7] [SCSI] sr: adds Zero-power ODD support

[Lin Ming]

ZPODD(Zero Power Optical Disk Drive) is a new feature in
SATA 3.1 specification. It provides a way to power off unused ODD.

ZPODD support is checked in in sr_probe().
can_power_off flag is set during suspend if ZPODD is supported.

ATA port's runtime suspend callback will actually power off the ODD
and its runtime resume callback will actually power on the ODD.

When ODD is powered off(D3Cold state), inserting disk will trigger a
wakeup event(GPE). GPE AML handler notifies the associated device. Then
ODD is resumed in the notify handler.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/ata/libata-acpi.c  |    8 +++++++-
 drivers/scsi/scsi_pm.c     |    8 ++++++++
 drivers/scsi/sr.c          |   44 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/sr.h          |    2 ++
 include/scsi/scsi_device.h |    2 ++
 5 files changed, 63 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 81ef0f5..3744761 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -859,8 +859,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);
-			if (acpi_state > 0)
+			if (acpi_state > 0) {
+				/* Check if the device is ready for power off */
+				if (acpi_state == ACPI_STATE_D3_COLD &&
+					!scsi_device_can_power_off(dev->sdev))
+					acpi_state = ACPI_STATE_D3;
+
 				acpi_bus_set_power(handle, acpi_state);
+			}
 			if (ap->tdev.power.request == RPM_REQ_SUSPEND)
 				acpi_pm_device_run_wake(
 					&dev->sdev->sdev_gendev, true);
diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index c467064..0dc7e74 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -224,6 +224,14 @@ void scsi_autopm_put_host(struct Scsi_Host *shost)
 	pm_runtime_put_sync(&shost->shost_gendev);
 }
 
+bool scsi_device_can_power_off(struct scsi_device *sdev)
+{
+	struct device *dev = &sdev->sdev_gendev;
+
+	return !dev->power.subsys_data ? false :
+		dev->power.subsys_data->can_power_off;
+}
+
 #else
 
 #define scsi_runtime_suspend	NULL
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 5fc97d2..265a8ea 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -45,6 +45,8 @@
 #include <linux/blkdev.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
+#include <linux/acpi.h>
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>
@@ -80,12 +82,38 @@ 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 *dev, pm_message_t mesg)
+{
+	struct scsi_cd *cd;
+
+	cd = dev_get_drvdata(dev);
+	if (cd->device->power_off)
+		dev->power.subsys_data->can_power_off = true;
+
+	return 0;
+}
+
+static int sr_resume(struct device *dev)
+{
+	struct scsi_cd *cd;
+
+	cd = dev_get_drvdata(dev);
+	if (cd->device->power_off) {
+		dev->power.subsys_data->can_power_off = false;
+		cd->poweroff_event = 0;
+	}
+
+	return 0;
+}
+
 static struct scsi_driver sr_template = {
 	.owner			= THIS_MODULE,
 	.gendrv = {
 		.name   	= "sr",
 		.probe		= sr_probe,
 		.remove		= sr_remove,
+		.suspend	= sr_suspend,
+		.resume		= sr_resume,
 	},
 	.done			= sr_done,
 };
@@ -216,6 +244,11 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 	unsigned int events;
 	int ret;
 
+	/* Not necessary to check events if enter ZPODD state */
+	if (cd->device->power_off &&
+			pm_runtime_suspended(&cd->device->sdev_gendev))
+		return 0;
+
 	/* no changer support */
 	if (CDSL_CURRENT != slot)
 		return 0;
@@ -260,6 +293,11 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 	cd->media_present = scsi_status_is_good(ret) ||
 		(scsi_sense_valid(&sshdr) && sshdr.asc != 0x3a);
 
+	if (!cd->media_present && cd->device->power_off && !cd->poweroff_event) {
+		scsi_autopm_put_device(cd->device);
+		cd->poweroff_event = 1;
+	}
+
 	if (last_present != cd->media_present)
 		cd->device->changed = 1;
 
@@ -716,6 +754,9 @@ static int sr_probe(struct device *dev)
 	disk->flags |= GENHD_FL_REMOVABLE;
 	add_disk(disk);
 
+	if (sdev->power_off)
+		dev_pm_get_subsys_data(dev);
+
 	sdev_printk(KERN_DEBUG, sdev,
 		    "Attached scsi CD-ROM %s\n", cd->cdi.name);
 	return 0;
@@ -972,6 +1013,9 @@ static int sr_remove(struct device *dev)
 	kref_put(&cd->kref, sr_kref_release);
 	mutex_unlock(&sr_ref_mutex);
 
+	if (cd->device->power_off)
+		dev_pm_put_subsys_data(dev);
+
 	return 0;
 }
 
diff --git a/drivers/scsi/sr.h b/drivers/scsi/sr.h
index 37c8f6b..8b2840b 100644
--- a/drivers/scsi/sr.h
+++ b/drivers/scsi/sr.h
@@ -42,6 +42,8 @@ typedef struct scsi_cd {
 	unsigned readcd_cdda:1;	/* reading audio data using READ_CD */
 	unsigned media_present:1;	/* media is present */
 
+	unsigned poweroff_event:1;
+
 	/* GET_EVENT spurious event handling, blk layer guarantees exclusion */
 	int tur_mismatch;		/* nr of get_event TUR mismatches */
 	bool tur_changed:1;		/* changed according to TUR */
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 0f69612..4805d5a 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -389,9 +389,11 @@ extern int scsi_execute_req(struct scsi_device *sdev, const unsigned char *cmd,
 #ifdef CONFIG_PM_RUNTIME
 extern int scsi_autopm_get_device(struct scsi_device *);
 extern void scsi_autopm_put_device(struct scsi_device *);
+extern bool scsi_device_can_power_off(struct scsi_device *);
 #else
 static inline int scsi_autopm_get_device(struct scsi_device *d) { return 0; }
 static inline void scsi_autopm_put_device(struct scsi_device *d) {}
+static inline bool scsi_device_can_power_off(struct scsi_device *d) { return false; };
 #endif /* CONFIG_PM_RUNTIME */
 
 static inline int __must_check scsi_device_reprobe(struct scsi_device *sdev)
-- 
1.7.2.5

Re: [PATCH v3 7/7] [SCSI] sr: adds Zero-power ODD support

[Alan Stern]

On Wed, 28 Mar 2012, Lin Ming wrote:

> ZPODD(Zero Power Optical Disk Drive) is a new feature in
> SATA 3.1 specification. It provides a way to power off unused ODD.
> 
> ZPODD support is checked in in sr_probe().
> can_power_off flag is set during suspend if ZPODD is supported.
> 
> ATA port's runtime suspend callback will actually power off the ODD
> and its runtime resume callback will actually power on the ODD.
> 
> When ODD is powered off(D3Cold state), inserting disk will trigger a
> wakeup event(GPE). GPE AML handler notifies the associated device. Then
> ODD is resumed in the notify handler.

> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c

> @@ -80,12 +82,38 @@ 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 *dev, pm_message_t mesg)
> +{
> +	struct scsi_cd *cd;
> +
> +	cd = dev_get_drvdata(dev);
> +	if (cd->device->power_off)
> +		dev->power.subsys_data->can_power_off = true;
> +
> +	return 0;
> +}
> +
> +static int sr_resume(struct device *dev)
> +{
> +	struct scsi_cd *cd;
> +
> +	cd = dev_get_drvdata(dev);
> +	if (cd->device->power_off) {
> +		dev->power.subsys_data->can_power_off = false;
> +		cd->poweroff_event = 0;
> +	}
> +
> +	return 0;
> +}

Calling this flag "can_power_off" makes these routines look very 
strange.  Either the device can power off or it can't, i.e., either it 
supports ZPODD or it doesn't.  This doesn't change over time.

If you rename the flag "may_power_off" then its meaning will be more 
clear.

> @@ -216,6 +244,11 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
>  	unsigned int events;
>  	int ret;
>  
> +	/* Not necessary to check events if enter ZPODD state */
> +	if (cd->device->power_off &&
> +			pm_runtime_suspended(&cd->device->sdev_gendev))
> +		return 0;

The comment is wrong and the new code does the wrong thing.  You _do_
have to check for events even in the ZPODD state, which means
sr_check_events must power-up the device if necessary.  
sd_check_events in James Bottomley's scsi-misc tree now does the right
thing; see commit 4e2247b2bd289f079349d6c69755f8cff4e31f2b.

> +
>  	/* no changer support */
>  	if (CDSL_CURRENT != slot)
>  		return 0;
> @@ -260,6 +293,11 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
>  	cd->media_present = scsi_status_is_good(ret) ||
>  		(scsi_sense_valid(&sshdr) && sshdr.asc != 0x3a);
>  
> +	if (!cd->media_present && cd->device->power_off && !cd->poweroff_event) {
> +		scsi_autopm_put_device(cd->device);

You can see your mistake here.  You call scsi_autopm_put_device here
without calling scsi_autopm_get_device earlier.

Alan Stern

Re: [PATCH v3 7/7] [SCSI] sr: adds Zero-power ODD support

[Lin Ming]

On Wed, Mar 28, 2012 at 10:33 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 28 Mar 2012, Lin Ming wrote:
>
>> ZPODD(Zero Power Optical Disk Drive) is a new feature in
>> SATA 3.1 specification. It provides a way to power off unused ODD.
>>
>> ZPODD support is checked in in sr_probe().
>> can_power_off flag is set during suspend if ZPODD is supported.
>>
>> ATA port's runtime suspend callback will actually power off the ODD
>> and its runtime resume callback will actually power on the ODD.
>>
>> When ODD is powered off(D3Cold state), inserting disk will trigger a
>> wakeup event(GPE). GPE AML handler notifies the associated device. Then
>> ODD is resumed in the notify handler.
>
>> --- a/drivers/scsi/sr.c
>> +++ b/drivers/scsi/sr.c
>
>> @@ -80,12 +82,38 @@ 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 *dev, pm_message_t mesg)
>> +{
>> +     struct scsi_cd *cd;
>> +
>> +     cd = dev_get_drvdata(dev);
>> +     if (cd->device->power_off)
>> +             dev->power.subsys_data->can_power_off = true;
>> +
>> +     return 0;
>> +}
>> +
>> +static int sr_resume(struct device *dev)
>> +{
>> +     struct scsi_cd *cd;
>> +
>> +     cd = dev_get_drvdata(dev);
>> +     if (cd->device->power_off) {
>> +             dev->power.subsys_data->can_power_off = false;
>> +             cd->poweroff_event = 0;
>> +     }
>> +
>> +     return 0;
>> +}
>
> Calling this flag "can_power_off" makes these routines look very
> strange.  Either the device can power off or it can't, i.e., either it
> supports ZPODD or it doesn't.  This doesn't change over time.
>
> If you rename the flag "may_power_off" then its meaning will be more
> clear.

OK, will rename it.

>
>> @@ -216,6 +244,11 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
>>       unsigned int events;
>>       int ret;
>>
>> +     /* Not necessary to check events if enter ZPODD state */
>> +     if (cd->device->power_off &&
>> +                     pm_runtime_suspended(&cd->device->sdev_gendev))
>> +             return 0;
>
> The comment is wrong and the new code does the wrong thing.  You _do_
> have to check for events even in the ZPODD state, which means
> sr_check_events must power-up the device if necessary.
> sd_check_events in James Bottomley's scsi-misc tree now does the right
> thing; see commit 4e2247b2bd289f079349d6c69755f8cff4e31f2b.

The problem is userspace(GNOME, for example) will check for events
every seconds.
If sr is power up so frequently then we lost the expected power
savings from ZPODD.

There are 2 events:

DISK_EVENT_MEDIA_CHANGE
DISK_EVENT_EJECT_REQUEST

In current implementation, if sr is in ZPODD state, then it means
there is no disk in the CDROM.
So if sr is in ZPODD state, MEDIA_CHANGE would never happen.

EJECT_REQUEST seems can be ignored, since there is no disk in the CDROM at all.

>
>> +
>>       /* no changer support */
>>       if (CDSL_CURRENT != slot)
>>               return 0;
>> @@ -260,6 +293,11 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
>>       cd->media_present = scsi_status_is_good(ret) ||
>>               (scsi_sense_valid(&sshdr) && sshdr.asc != 0x3a);
>>
>> +     if (!cd->media_present && cd->device->power_off && !cd->poweroff_event) {
>> +             scsi_autopm_put_device(cd->device);
>
> You can see your mistake here.  You call scsi_autopm_put_device here
> without calling scsi_autopm_get_device earlier.

Let me check this.

Thanks for the review.
Lin Ming

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

Re: [PATCH v3 7/7] [SCSI] sr: adds Zero-power ODD support

[Aaron Lu]

Hi,

On Thu, Mar 29, 2012 at 1:45 PM, Lin Ming <ming.m.lin@intel.com> wrote:
> On Wed, Mar 28, 2012 at 10:33 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> On Wed, 28 Mar 2012, Lin Ming wrote:
>>
>>> ZPODD(Zero Power Optical Disk Drive) is a new feature in
>>> SATA 3.1 specification. It provides a way to power off unused ODD.
>>>
>>> ZPODD support is checked in in sr_probe().
>>> can_power_off flag is set during suspend if ZPODD is supported.
>>>
>>> ATA port's runtime suspend callback will actually power off the ODD
>>> and its runtime resume callback will actually power on the ODD.
>>>
>>> When ODD is powered off(D3Cold state), inserting disk will trigger a
>>> wakeup event(GPE). GPE AML handler notifies the associated device. Then
>>> ODD is resumed in the notify handler.
>>
>>> --- a/drivers/scsi/sr.c
>>> +++ b/drivers/scsi/sr.c
>>
>>> @@ -80,12 +82,38 @@ 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 *dev, pm_message_t mesg)
>>> +{
>>> +     struct scsi_cd *cd;
>>> +
>>> +     cd = dev_get_drvdata(dev);
>>> +     if (cd->device->power_off)
>>> +             dev->power.subsys_data->can_power_off = true;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int sr_resume(struct device *dev)
>>> +{
>>> +     struct scsi_cd *cd;
>>> +
>>> +     cd = dev_get_drvdata(dev);
>>> +     if (cd->device->power_off) {
>>> +             dev->power.subsys_data->can_power_off = false;
>>> +             cd->poweroff_event = 0;
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>
>> Calling this flag "can_power_off" makes these routines look very
>> strange.  Either the device can power off or it can't, i.e., either it
>> supports ZPODD or it doesn't.  This doesn't change over time.
>>
>> If you rename the flag "may_power_off" then its meaning will be more
>> clear.
>
> OK, will rename it.
>
>>
>>> @@ -216,6 +244,11 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
>>>       unsigned int events;
>>>       int ret;
>>>
>>> +     /* Not necessary to check events if enter ZPODD state */
>>> +     if (cd->device->power_off &&
>>> +                     pm_runtime_suspended(&cd->device->sdev_gendev))
>>> +             return 0;
>>
>> The comment is wrong and the new code does the wrong thing.  You _do_
>> have to check for events even in the ZPODD state, which means
>> sr_check_events must power-up the device if necessary.
>> sd_check_events in James Bottomley's scsi-misc tree now does the right
>> thing; see commit 4e2247b2bd289f079349d6c69755f8cff4e31f2b.
>
> The problem is userspace(GNOME, for example) will check for events
> every seconds.
> If sr is power up so frequently then we lost the expected power
> savings from ZPODD.

Agreed.
BTW, it's udevd on my Linux box that changed the default poll msecs kernel param
which caused sr_check_events being called every 2 seconds.

>
> There are 2 events:
>
> DISK_EVENT_MEDIA_CHANGE
> DISK_EVENT_EJECT_REQUEST
>
> In current implementation, if sr is in ZPODD state, then it means
> there is no disk in the CDROM.
> So if sr is in ZPODD state, MEDIA_CHANGE would never happen.
>
> EJECT_REQUEST seems can be ignored, since there is no disk in the CDROM at all.
>

For the ODD to be put into suspend state, the conditions should be:
1 tray closed
2 no media inside
I think we missed the condition 1 check now.

And if we follow the two conditions, the events can be safely ignored.

What do you think of blocking events for it when going to suspend and unblocking
when resume? This could erase the unnecessary calls of the check events function
when ODD is suspended.
But disk_(un)block_events are not exported and can't be used in sr
module. So I'm
not sure how to do this.

Another thing to consider is, user might want to eject the tray by
software like the
eject /dev/sr0 command or some UI mouse clicks against the cdrom icon. I'm still
thinking how to do this correctly.

>>
>>> +
>>>       /* no changer support */
>>>       if (CDSL_CURRENT != slot)
>>>               return 0;
>>> @@ -260,6 +293,11 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
>>>       cd->media_present = scsi_status_is_good(ret) ||
>>>               (scsi_sense_valid(&sshdr) && sshdr.asc != 0x3a);
>>>
>>> +     if (!cd->media_present && cd->device->power_off && !cd->poweroff_event) {
>>> +             scsi_autopm_put_device(cd->device);
>>
>> You can see your mistake here.  You call scsi_autopm_put_device here
>> without calling scsi_autopm_get_device earlier.
>
> Let me check this.

I guess the earlier call of get device is this?

 869 int scsi_sysfs_add_sdev(struct scsi_device *sdev)
... ...
 891
 892        /* The following call will keep sdev active indefinitely, until
 893         * its driver does a corresponding scsi_autopm_pm_device().  Only
 894         * drivers supporting autosuspend will do this.
 895         */
 896        scsi_autopm_get_device(sdev);
 897

-Aaron

>
> Thanks for the review.
> Lin Ming
>
>>
>> Alan Stern
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 7/7] [SCSI] sr: adds Zero-power ODD support

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 4402 bytes --]

On Thu, 29 Mar 2012, Aaron Lu wrote:

> >>> @@ -216,6 +244,11 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
> >>>       unsigned int events;
> >>>       int ret;
> >>>
> >>> +     /* Not necessary to check events if enter ZPODD state */
> >>> +     if (cd->device->power_off &&
> >>> +                     pm_runtime_suspended(&cd->device->sdev_gendev))
> >>> +             return 0;
> >>
> >> The comment is wrong and the new code does the wrong thing.  You _do_
> >> have to check for events even in the ZPODD state, which means
> >> sr_check_events must power-up the device if necessary.
> >> sd_check_events in James Bottomley's scsi-misc tree now does the right
> >> thing; see commit 4e2247b2bd289f079349d6c69755f8cff4e31f2b.
> >
> > The problem is userspace(GNOME, for example) will check for events
> > every seconds.
> > If sr is power up so frequently then we lost the expected power
> > savings from ZPODD.

That's true.  There's nothing you can do about it in the kernel; it's 
up to userspace to change the frequency of event polling.

> Agreed.
> BTW, it's udevd on my Linux box that changed the default poll msecs kernel param
> which caused sr_check_events being called every 2 seconds.

Indeed.

> > There are 2 events:
> >
> > DISK_EVENT_MEDIA_CHANGE
> > DISK_EVENT_EJECT_REQUEST
> >
> > In current implementation, if sr is in ZPODD state, then it means
> > there is no disk in the CDROM.
> > So if sr is in ZPODD state, MEDIA_CHANGE would never happen.

Sure it will: when the user inserts a disc.

> > EJECT_REQUEST seems can be ignored, since there is no disk in the CDROM at all.
> >
> 
> For the ODD to be put into suspend state, the conditions should be:
> 1 tray closed
> 2 no media inside
> I think we missed the condition 1 check now.
> 
> And if we follow the two conditions, the events can be safely ignored.

No, they can't.  Otherwise the device won't power back up when the user 
inserts a new disc.

> What do you think of blocking events for it when going to suspend and unblocking
> when resume? This could erase the unnecessary calls of the check events function
> when ODD is suspended.
> But disk_(un)block_events are not exported and can't be used in sr
> module. So I'm
> not sure how to do this.
> 
> Another thing to consider is, user might want to eject the tray by
> software like the
> eject /dev/sr0 command or some UI mouse clicks against the cdrom icon. I'm still
> thinking how to do this correctly.

What's the problem?  Can't the user already eject the tray via 
software?

> >>> @@ -260,6 +293,11 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
> >>>       cd->media_present = scsi_status_is_good(ret) ||
> >>>               (scsi_sense_valid(&sshdr) && sshdr.asc != 0x3a);
> >>>
> >>> +     if (!cd->media_present && cd->device->power_off && !cd->poweroff_event) {
> >>> +             scsi_autopm_put_device(cd->device);
> >>
> >> You can see your mistake here.  You call scsi_autopm_put_device here
> >> without calling scsi_autopm_get_device earlier.
> >
> > Let me check this.
> 
> I guess the earlier call of get device is this?
> 
>  869 int scsi_sysfs_add_sdev(struct scsi_device *sdev)
> ... ...
>  891
>  892        /* The following call will keep sdev active indefinitely, until
>  893         * its driver does a corresponding scsi_autopm_pm_device().  Only
>  894         * drivers supporting autosuspend will do this.
>  895         */
>  896        scsi_autopm_get_device(sdev);
>  897

You have to do this correctly.  Currently sr doesn't support runtime PM 
at all.  If you do want to support it, then the driver should call 
scsi_autopm_put_device at the end of the probe routine (or whenever it 
is ready to allow runtime suspends) and scsi_autopm_get_device at the 
start of the remove routine (or whenever it wants to prevent runtime 
suspends).

The idea is that the driver is probed with the PM usage counter set to
1, so a runtime suspend won't occur until you do a put.  Similarly, in
the remove routine, you must make sure that your gets and puts balance
out.

Alan Stern

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

Re: [PATCH v3 7/7] [SCSI] sr: adds Zero-power ODD support

[Aaron Lu]

On Thu, Mar 29, 2012 at 11:00:18AM -0400, Alan Stern wrote:
> On Thu, 29 Mar 2012, Aaron Lu wrote:
> 
> > >>> @@ -216,6 +244,11 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
> > >>> � � � unsigned int events;
> > >>> � � � int ret;
> > >>>
> > >>> + � � /* Not necessary to check events if enter ZPODD state */
> > >>> + � � if (cd->device->power_off &&
> > >>> + � � � � � � � � � � pm_runtime_suspended(&cd->device->sdev_gendev))
> > >>> + � � � � � � return 0;
> > >>
> > >> The comment is wrong and the new code does the wrong thing. �You _do_
> > >> have to check for events even in the ZPODD state, which means
> > >> sr_check_events must power-up the device if necessary.
> > >> sd_check_events in James Bottomley's scsi-misc tree now does the right
> > >> thing; see commit 4e2247b2bd289f079349d6c69755f8cff4e31f2b.
> > >
> > > The problem is userspace(GNOME, for example) will check for events
> > > every seconds.
> > > If sr is power up so frequently then we lost the expected power
> > > savings from ZPODD.
> 
> That's true.  There's nothing you can do about it in the kernel; it's 
> up to userspace to change the frequency of event polling.
> 
> > Agreed.
> > BTW, it's udevd on my Linux box that changed the default poll msecs kernel param
> > which caused sr_check_events being called every 2 seconds.
> 
> Indeed.
> 
> > > There are 2 events:
> > >
> > > DISK_EVENT_MEDIA_CHANGE
> > > DISK_EVENT_EJECT_REQUEST
> > >
> > > In current implementation, if sr is in ZPODD state, then it means
> > > there is no disk in the CDROM.
> > > So if sr is in ZPODD state, MEDIA_CHANGE would never happen.
> 
> Sure it will: when the user inserts a disc.
> 
> > > EJECT_REQUEST seems can be ignored, since there is no disk in the CDROM at all.
> > >
> > 
> > For the ODD to be put into suspend state, the conditions should be:
> > 1 tray closed
> > 2 no media inside
> > I think we missed the condition 1 check now.
> > 
> > And if we follow the two conditions, the events can be safely ignored.
> 
> No, they can't.  Otherwise the device won't power back up when the user 
> inserts a new disc.
> 

The ODD is put to zero power state, so it can't react to the eject
button without being powered up back first ;-)
On current implementation, ACPI is used to power back up the ODD like this:
1 User press the eject button;
2 A gpe event fired, and ACPI interrupt, and the corresponding gpe
handler runs and the ODD's ACPI handle is notified about DEVICE_WAKE_UP;
3 In ODD's acpi notify handler, we power back up it by scsi_autopm_get_device.
This is handled in the following Lin Ming's patch:
[PATCH v3 2/7] libata-acpi: add ata port runtime D3Cold support

> > What do you think of blocking events for it when going to suspend and unblocking
> > when resume? This could erase the unnecessary calls of the check events function
> > when ODD is suspended.
> > But disk_(un)block_events are not exported and can't be used in sr
> > module. So I'm
> > not sure how to do this.
> > 
> > Another thing to consider is, user might want to eject the tray by
> > software like the
> > eject /dev/sr0 command or some UI mouse clicks against the cdrom icon. I'm still
> > thinking how to do this correctly.
> 
> What's the problem?  Can't the user already eject the tray via 
> software?
> 

When the ODD is put to zero power state, it will not be able to handle
commands like CDROMEJECT. We have to power it up first and then handle
the ioctl request. Currently, we only power back up the ODD when user
press the eject button as explained above.

> > >>> @@ -260,6 +293,11 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
> > >>> � � � cd->media_present = scsi_status_is_good(ret) ||
> > >>> � � � � � � � (scsi_sense_valid(&sshdr) && sshdr.asc != 0x3a);
> > >>>
> > >>> + � � if (!cd->media_present && cd->device->power_off && !cd->poweroff_event) {
> > >>> + � � � � � � scsi_autopm_put_device(cd->device);
> > >>
> > >> You can see your mistake here. �You call scsi_autopm_put_device here
> > >> without calling scsi_autopm_get_device earlier.
> > >
> > > Let me check this.
> > 
> > I guess the earlier call of get device is this?
> > 
> >  869 int scsi_sysfs_add_sdev(struct scsi_device *sdev)
> > ... ...
> >  891
> >  892        /* The following call will keep sdev active indefinitely, until
> >  893         * its driver does a corresponding scsi_autopm_pm_device().  Only
> >  894         * drivers supporting autosuspend will do this.
> >  895         */
> >  896        scsi_autopm_get_device(sdev);
> >  897
> 
> You have to do this correctly.  Currently sr doesn't support runtime PM 
> at all.  If you do want to support it, then the driver should call 
> scsi_autopm_put_device at the end of the probe routine (or whenever it 
> is ready to allow runtime suspends) and scsi_autopm_get_device at the 
> start of the remove routine (or whenever it wants to prevent runtime 
> suspends).
> 
> The idea is that the driver is probed with the PM usage counter set to
> 1, so a runtime suspend won't occur until you do a put.  Similarly, in
> the remove routine, you must make sure that your gets and puts balance
> out.
> 

Oh, I see, thanks for your clear explanation.

-Aaron


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

Re: [PATCH v3 7/7] [SCSI] sr: adds Zero-power ODD support

[Alan Stern]

On Fri, 30 Mar 2012, Aaron Lu wrote:

> > > For the ODD to be put into suspend state, the conditions should be:
> > > 1 tray closed
> > > 2 no media inside
> > > I think we missed the condition 1 check now.
> > > 
> > > And if we follow the two conditions, the events can be safely ignored.
> > 
> > No, they can't.  Otherwise the device won't power back up when the user 
> > inserts a new disc.
> > 
> 
> The ODD is put to zero power state, so it can't react to the eject
> button without being powered up back first ;-)
> On current implementation, ACPI is used to power back up the ODD like this:
> 1 User press the eject button;
> 2 A gpe event fired, and ACPI interrupt, and the corresponding gpe
> handler runs and the ODD's ACPI handle is notified about DEVICE_WAKE_UP;
> 3 In ODD's acpi notify handler, we power back up it by scsi_autopm_get_device.
> This is handled in the following Lin Ming's patch:
> [PATCH v3 2/7] libata-acpi: add ata port runtime D3Cold support

I see.  Then you are right; it's not necessary to check for events 
while the drive is at zero power.

> When the ODD is put to zero power state, it will not be able to handle
> commands like CDROMEJECT. We have to power it up first and then handle
> the ioctl request. Currently, we only power back up the ODD when user
> press the eject button as explained above.

Then you may want to avoid going to zero-power while a program is
holding the device file open.  After all, the program might issue a
CDROMEJECT ioctl.

Alan Stern

Re: [PATCH v3 7/7] [SCSI] sr: adds Zero-power ODD support

[Jan Ceuleers]

On 30/03/12 03:07, Aaron Lu wrote:
> When the ODD is put to zero power state, it will not be able to handle
> commands like CDROMEJECT. We have to power it up first and then handle
> the ioctl request. Currently, we only power back up the ODD when user
> press the eject button as explained above.

What about slot-loading optical drives, where the user does not first 
have to press a button to open the tray (due to a lack of tray)?

Jan

Re: [PATCH v3 7/7] [SCSI] sr: adds Zero-power ODD support

[Aaron Lu]

On Sat, Mar 31, 2012 at 12:45:40PM +0200, Jan Ceuleers wrote:
> 
> What about slot-loading optical drives, where the user does not
> first have to press a button to open the tray (due to a lack of
> tray)?

For slot type ODD, the insert of a disk will fire the gpe event by
means of the sata slimline connector's device attention pin.

That means, the insert of a disk into a slot type ODD and the
press of the eject button of a tray type ODD has the same effect to
trigger the resume process.

Thanks,
Aaron

Re: [PATCH v3 7/7] [SCSI] sr: adds Zero-power ODD support

[Lin Ming]

On Thu, Mar 29, 2012 at 9:50 PM, Aaron Lu <aaron.lu@amd.com> wrote:

[.....]

> For the ODD to be put into suspend state, the conditions should be:
> 1 tray closed
> 2 no media inside
> I think we missed the condition 1 check now.
>
> And if we follow the two conditions, the events can be safely ignored.
>
> What do you think of blocking events for it when going to suspend and unblocking
> when resume? This could erase the unnecessary calls of the check events function
> when ODD is suspended.

Good idea.
Will try this.

> But disk_(un)block_events are not exported and can't be used in sr
> module. So I'm
> not sure how to do this.

We can simply export it.

>
> Another thing to consider is, user might want to eject the tray by
> software like the
> eject /dev/sr0 command or some UI mouse clicks against the cdrom icon. I'm still
> thinking how to do this correctly.

Assume eject /dev/sr0 is implemented as:

int fd = open("/dev/sr0", ...)
ioctl(fd, CDROMEJECT)

We may need to resume ODD in the ioctl handler(scsi_cmd_ioctl).

Thanks,
Lin Ming

Re: [PATCH v3 7/7] [SCSI] sr: adds Zero-power ODD support

[Aaron Lu]

Hi,

On Thu, Apr 05, 2012 at 02:00:57PM +0800, Lin Ming wrote:
> >
> > Another thing to consider is, user might want to eject the tray by
> > software like the
> > eject /dev/sr0 command or some UI mouse clicks against the cdrom icon. I'm still
> > thinking how to do this correctly.
> 
> Assume eject /dev/sr0 is implemented as:
> 
> int fd = open("/dev/sr0", ...)
> ioctl(fd, CDROMEJECT)
> 

Indeed, it is implemented as this :-)

> We may need to resume ODD in the ioctl handler(scsi_cmd_ioctl).
> 

I prefer we do this in sr_block_ioctl.
Suppose the ODD is now runtime suspended and received an ioctl:
if the ioctl's cmd is CDROMEJECT, resume it.
For other cases, return an error code like EPERM.
When done, according to the result of ioctl: if success, leave it resumed;
if failed, put it back to sleep.

Something like this:

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 5fc97d2..aa6e920 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>
@@ -538,10 +539,21 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
        struct scsi_cd *cd = scsi_cd(bdev->bd_disk);
        struct scsi_device *sdev = cd->device;
        void __user *argp = (void __user *)arg;
-       int ret;
+       int ret, rpm_resumed = 0;
 
        mutex_lock(&sr_mutex);
 
+       if (pm_runtime_suspended(&sdev->sdev_gendev)) {
+               if (cmd == CDROMEJECT) {
+                       scsi_autopm_get_device(sdev);
+                       rpm_resumed = 1;
+               }
+               else {
+                       ret = -EPERM;
+                       goto out;
+               }
+       }
+
        /*
         * Send SCSI addressing ioctls directly to mid level, send other
         * ioctls to cdrom/block level.
@@ -570,6 +582,9 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
        ret = scsi_ioctl(sdev, cmd, argp);
 
 out:
+       if (rpm_resumed && ret)
+               scsi_autopm_put_device(sdev);
+
        mutex_unlock(&sr_mutex);
        return ret;
 }

Does this work?

-Aaron

Re: [PATCH v3 7/7] [SCSI] sr: adds Zero-power ODD support

[Lin Ming]

On Thu, Apr 5, 2012 at 4:32 PM, Aaron Lu <aaron.lu@amd.com> wrote:
> Hi,
>
> On Thu, Apr 05, 2012 at 02:00:57PM +0800, Lin Ming wrote:
>> >
>> > Another thing to consider is, user might want to eject the tray by
>> > software like the
>> > eject /dev/sr0 command or some UI mouse clicks against the cdrom icon. I'm still
>> > thinking how to do this correctly.
>>
>> Assume eject /dev/sr0 is implemented as:
>>
>> int fd = open("/dev/sr0", ...)
>> ioctl(fd, CDROMEJECT)
>>
>
> Indeed, it is implemented as this :-)
>
>> We may need to resume ODD in the ioctl handler(scsi_cmd_ioctl).
>>
>
> I prefer we do this in sr_block_ioctl.

That's better.

> Suppose the ODD is now runtime suspended and received an ioctl:
> if the ioctl's cmd is CDROMEJECT, resume it.
> For other cases, return an error code like EPERM.
> When done, according to the result of ioctl: if success, leave it resumed;
> if failed, put it back to sleep.
>
> Something like this:
>
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 5fc97d2..aa6e920 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>
> @@ -538,10 +539,21 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
>        struct scsi_cd *cd = scsi_cd(bdev->bd_disk);
>        struct scsi_device *sdev = cd->device;
>        void __user *argp = (void __user *)arg;
> -       int ret;
> +       int ret, rpm_resumed = 0;
>
>        mutex_lock(&sr_mutex);
>
> +       if (pm_runtime_suspended(&sdev->sdev_gendev)) {
> +               if (cmd == CDROMEJECT) {
> +                       scsi_autopm_get_device(sdev);
> +                       rpm_resumed = 1;
> +               }
> +               else {
> +                       ret = -EPERM;
> +                       goto out;
> +               }
> +       }
> +
>        /*
>         * Send SCSI addressing ioctls directly to mid level, send other
>         * ioctls to cdrom/block level.
> @@ -570,6 +582,9 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
>        ret = scsi_ioctl(sdev, cmd, argp);
>
>  out:
> +       if (rpm_resumed && ret)
> +               scsi_autopm_put_device(sdev);
> +
>        mutex_unlock(&sr_mutex);
>        return ret;
>  }
>
> Does this work?

I think so.
Will add this patch.

Thanks,
Lin Ming

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

Re: [PATCH v3 7/7] [SCSI] sr: adds Zero-power ODD support

[Alan Stern]

On Thu, 5 Apr 2012, Aaron Lu wrote:

> Hi,
> 
> On Thu, Apr 05, 2012 at 02:00:57PM +0800, Lin Ming wrote:
> > >
> > > Another thing to consider is, user might want to eject the tray by
> > > software like the
> > > eject /dev/sr0 command or some UI mouse clicks against the cdrom icon. I'm still
> > > thinking how to do this correctly.
> > 
> > Assume eject /dev/sr0 is implemented as:
> > 
> > int fd = open("/dev/sr0", ...)
> > ioctl(fd, CDROMEJECT)
> > 
> 
> Indeed, it is implemented as this :-)
> 
> > We may need to resume ODD in the ioctl handler(scsi_cmd_ioctl).
> > 
> 
> I prefer we do this in sr_block_ioctl.
> Suppose the ODD is now runtime suspended and received an ioctl:
> if the ioctl's cmd is CDROMEJECT, resume it.
> For other cases, return an error code like EPERM.
> When done, according to the result of ioctl: if success, leave it resumed;
> if failed, put it back to sleep.

Alternatively, you may want to do the runtime resume in sr_open and 
sr_block_open and the suspend in the corresponding release routines.  
It's up to you.

Alan Stern

Re: [PATCH v3 7/7] [SCSI] sr: adds Zero-power ODD support

[Lin Ming]

On Thu, Apr 5, 2012 at 10:03 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 5 Apr 2012, Aaron Lu wrote:
>
>> Hi,
>>
>> On Thu, Apr 05, 2012 at 02:00:57PM +0800, Lin Ming wrote:
>> > >
>> > > Another thing to consider is, user might want to eject the tray by
>> > > software like the
>> > > eject /dev/sr0 command or some UI mouse clicks against the cdrom icon. I'm still
>> > > thinking how to do this correctly.
>> >
>> > Assume eject /dev/sr0 is implemented as:
>> >
>> > int fd = open("/dev/sr0", ...)
>> > ioctl(fd, CDROMEJECT)
>> >
>>
>> Indeed, it is implemented as this :-)
>>
>> > We may need to resume ODD in the ioctl handler(scsi_cmd_ioctl).
>> >
>>
>> I prefer we do this in sr_block_ioctl.
>> Suppose the ODD is now runtime suspended and received an ioctl:
>> if the ioctl's cmd is CDROMEJECT, resume it.
>> For other cases, return an error code like EPERM.
>> When done, according to the result of ioctl: if success, leave it resumed;
>> if failed, put it back to sleep.
>
> Alternatively, you may want to do the runtime resume in sr_open and
> sr_block_open and the suspend in the corresponding release routines.
> It's up to you.

GNOME opens cdrom every seconds to check status.
So we won't get the desired power savings if we do the runtime resume
in sr_block_open.
See my previous mail.

>
> Alan Stern

Re: [PATCH v3 7/7] [SCSI] sr: adds Zero-power ODD support

[Lin Ming]

On Thu, Apr 5, 2012 at 4:32 PM, Aaron Lu <aaron.lu@amd.com> wrote:
> Hi,
>
> On Thu, Apr 05, 2012 at 02:00:57PM +0800, Lin Ming wrote:
>> >
>> > Another thing to consider is, user might want to eject the tray by
>> > software like the
>> > eject /dev/sr0 command or some UI mouse clicks against the cdrom icon. I'm still
>> > thinking how to do this correctly.
>>
>> Assume eject /dev/sr0 is implemented as:
>>
>> int fd = open("/dev/sr0", ...)
>> ioctl(fd, CDROMEJECT)
>>
>
> Indeed, it is implemented as this :-)
>
>> We may need to resume ODD in the ioctl handler(scsi_cmd_ioctl).
>>
>
> I prefer we do this in sr_block_ioctl.
> Suppose the ODD is now runtime suspended and received an ioctl:
> if the ioctl's cmd is CDROMEJECT, resume it.
> For other cases, return an error code like EPERM.
> When done, according to the result of ioctl: if success, leave it resumed;
> if failed, put it back to sleep.
>
> Something like this:
>
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 5fc97d2..aa6e920 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>
> @@ -538,10 +539,21 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
>        struct scsi_cd *cd = scsi_cd(bdev->bd_disk);
>        struct scsi_device *sdev = cd->device;
>        void __user *argp = (void __user *)arg;
> -       int ret;
> +       int ret, rpm_resumed = 0;
>
>        mutex_lock(&sr_mutex);
>
> +       if (pm_runtime_suspended(&sdev->sdev_gendev)) {
> +               if (cmd == CDROMEJECT) {
> +                       scsi_autopm_get_device(sdev);
> +                       rpm_resumed = 1;
> +               }
> +               else {
> +                       ret = -EPERM;
> +                       goto out;
> +               }
> +       }
> +
>        /*
>         * Send SCSI addressing ioctls directly to mid level, send other
>         * ioctls to cdrom/block level.
> @@ -570,6 +582,9 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
>        ret = scsi_ioctl(sdev, cmd, argp);
>
>  out:
> +       if (rpm_resumed && ret)
> +               scsi_autopm_put_device(sdev);
> +
>        mutex_unlock(&sr_mutex);
>        return ret;
>  }
>
> Does this work?

I have tested this patch.
Unfortunately, this does not work.

int fd = open("/dev/sr0", ...)
ioctl(fd, COMMAND)

Actually, the open() code path requires CDROM to be in active state, for example

sr_block_open
  cdrom_open
    open_for_data
       cdo->drive_status
         scsi_test_unit_ready
           scsi_execute_req

I'm thinking 3 solutions.

Solution 1:
sr_block_open() always return failure in ZPODD state.
But "eject /dev/sr0" won't work in this solution.

Solution 2:
sr_block_open() always return success in ZPODD state.
Then we have to insert ODD status check in many entries accessing it.
For example, sr_drive_status(), sr_check_events(),  sr_lock_door() etc.
And only runtime resume ODD for some special case, for example,
CDROMEJECT ioctl.

Solution 3:
Runtime resume ODD in sr_block_open(), as Alan suggested.
But the big problem is userspace will open ODD every seconds, so ODD
is frequently
resumed and we lose the expected power savings from ZPODD.

Any other idea?

Thanks,
Lin Ming

Re: [PATCH v3 7/7] [SCSI] sr: adds Zero-power ODD support

[Aaron Lu]

Hi,

On Fri, Apr 06, 2012 at 02:10:31PM +0800, Lin Ming wrote:
> >
> > I prefer we do this in sr_block_ioctl.
> > Suppose the ODD is now runtime suspended and received an ioctl:
> > if the ioctl's cmd is CDROMEJECT, resume it.
> > For other cases, return an error code like EPERM.
> > When done, according to the result of ioctl: if success, leave it resumed;
> > if failed, put it back to sleep.
> >
> > Something like this:
> >
> > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> > index 5fc97d2..aa6e920 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>
> > @@ -538,10 +539,21 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
> >        struct scsi_cd *cd = scsi_cd(bdev->bd_disk);
> >        struct scsi_device *sdev = cd->device;
> >        void __user *argp = (void __user *)arg;
> > -       int ret;
> > +       int ret, rpm_resumed = 0;
> >
> >        mutex_lock(&sr_mutex);
> >
> > +       if (pm_runtime_suspended(&sdev->sdev_gendev)) {
> > +               if (cmd == CDROMEJECT) {
> > +                       scsi_autopm_get_device(sdev);
> > +                       rpm_resumed = 1;
> > +               }
> > +               else {
> > +                       ret = -EPERM;
> > +                       goto out;
> > +               }
> > +       }
> > +
> >        /*
> >         * Send SCSI addressing ioctls directly to mid level, send other
> >         * ioctls to cdrom/block level.
> > @@ -570,6 +582,9 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
> >        ret = scsi_ioctl(sdev, cmd, argp);
> >
> >  out:
> > +       if (rpm_resumed && ret)
> > +               scsi_autopm_put_device(sdev);
> > +
> >        mutex_unlock(&sr_mutex);
> >        return ret;
> >  }
> >
> > Does this work?
> 
> I have tested this patch.
> Unfortunately, this does not work.
> 
> int fd = open("/dev/sr0", ...)
> ioctl(fd, COMMAND)
> 
> Actually, the open() code path requires CDROM to be in active state, for example
> 
> sr_block_open
>   cdrom_open
>     open_for_data
>        cdo->drive_status
>          scsi_test_unit_ready
>            scsi_execute_req
> 
Hmm... This is not the case on my Linux box.
eject will use O_NONBLOCK when open, which would results in the
following code path:
sr_block_open
  cdrom_open
    cdi->ops->open(sr_open)
      done

Maybe your eject is old and doesn't use the O_NONBLOCK flag?
If it is the case, then we may need to avoid this solution since we
can't depend on the user space tools.

> I'm thinking 3 solutions.
> 
> Solution 1:
> sr_block_open() always return failure in ZPODD state.
> But "eject /dev/sr0" won't work in this solution.
> 
Right, so, not an option.

> Solution 2:
> sr_block_open() always return success in ZPODD state.
> Then we have to insert ODD status check in many entries accessing it.
> For example, sr_drive_status(), sr_check_events(),  sr_lock_door() etc.
> And only runtime resume ODD for some special case, for example,
> CDROMEJECT ioctl.
Sounds like pretty complicated :-)

> 
> Solution 3:
> Runtime resume ODD in sr_block_open(), as Alan suggested.
> But the big problem is userspace will open ODD every seconds, so ODD
> is frequently
> resumed and we lose the expected power savings from ZPODD.
Yes, the udisks-daemon will constantly open the ODD's block device,
dunno why it would do this, need to check its code later.

Another problem I can think of doing suspend/resume in open/release is,
it's not that easy to determine if we should suspend the ODD in release
function. We may need to do some extra house keeping to achieve this
based on information like if the door is open and/or if there is media
inside, etc.

Thanks,
Aaron

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