[PATCH]libata-acpi: add ACPI _PSx method

[PATCH]libata-acpi: add ACPI _PSx method

[Shaohua Li]

ACPI spec (ver 3.0a, p289) requires IDE power on/off executes ACPI _PSx
methods. As recently most PATA drivers use libata, this patch adds _PSx
method support in libata. ACPI spec doesn't mention if SATA requires the
same _PSx method, but executing _PSx for SATA should be ok I think.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>

---
 drivers/ata/libata-acpi.c |   34 ++++++++++++++++++++++++++++++++++
 drivers/ata/libata-eh.c   |    3 +++
 drivers/ata/libata.h      |    2 ++
 3 files changed, 39 insertions(+)

Index: linux/drivers/ata/libata-acpi.c
===================================================================
--- linux.orig/drivers/ata/libata-acpi.c	2007-07-17 15:00:32.000000000 +0800
+++ linux/drivers/ata/libata-acpi.c	2007-09-14 10:23:01.000000000 +0800
@@ -523,6 +523,40 @@ void ata_acpi_on_resume(struct ata_port 
 }
 
 /**
+ * ata_acpi_set_state - set the port power state
+ * @ap: target ATA port
+ * @state: state, on/off
+ *
+ * This function executes the _PS0/_PS3 ACPI method to set the power state.
+ * ACPI spec requires _PS0 when IDE power on and _PS3 when power off
+ */
+void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
+{
+	int max_devices = 1, i;
+
+	if (!ap->acpi_handle)
+		return;
+
+	/* channel first and then drives for power on and verse versa for power off */
+	if (state.event == PM_EVENT_ON)
+		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D0);
+
+	if (ap->flags & ATA_FLAG_SLAVE_POSS)
+		max_devices++;
+
+	for (i = 0; i < max_devices; ++i) {
+		struct ata_device *dev = &ap->device[i];
+
+		if (dev->acpi_handle)
+			acpi_bus_set_power(dev->acpi_handle,
+				state.event == PM_EVENT_ON ?
+				ACPI_STATE_D0: ACPI_STATE_D3);
+	}
+	if (state.event != PM_EVENT_ON)
+		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D3);
+}
+
+/**
  * ata_acpi_on_devcfg - ATA ACPI hook called on device donfiguration
  * @dev: target ATA device
  *
Index: linux/drivers/ata/libata-eh.c
===================================================================
--- linux.orig/drivers/ata/libata-eh.c	2007-09-12 11:16:16.000000000 +0800
+++ linux/drivers/ata/libata-eh.c	2007-09-14 08:58:44.000000000 +0800
@@ -2349,6 +2349,7 @@ static void ata_eh_handle_port_suspend(s
 	if (ap->ops->port_suspend)
 		rc = ap->ops->port_suspend(ap, ap->pm_mesg);
 
+	ata_acpi_set_state(ap, PMSG_SUSPEND);
  out:
 	/* report result */
 	spin_lock_irqsave(ap->lock, flags);
@@ -2394,6 +2395,8 @@ static void ata_eh_handle_port_resume(st
 
 	WARN_ON(!(ap->pflags & ATA_PFLAG_SUSPENDED));
 
+	ata_acpi_set_state(ap, PMSG_ON);
+
 	if (ap->ops->port_resume)
 		rc = ap->ops->port_resume(ap);
 
Index: linux/drivers/ata/libata.h
===================================================================
--- linux.orig/drivers/ata/libata.h	2007-07-25 09:14:32.000000000 +0800
+++ linux/drivers/ata/libata.h	2007-09-14 08:57:46.000000000 +0800
@@ -102,11 +102,13 @@ extern void ata_acpi_associate(struct at
 extern int ata_acpi_on_suspend(struct ata_port *ap);
 extern void ata_acpi_on_resume(struct ata_port *ap);
 extern int ata_acpi_on_devcfg(struct ata_device *adev);
+extern void ata_acpi_set_state(struct ata_port *ap, pm_message_t state);
 #else
 static inline void ata_acpi_associate(struct ata_host *host) { }
 static inline int ata_acpi_on_suspend(struct ata_port *ap) { return 0; }
 static inline void ata_acpi_on_resume(struct ata_port *ap) { }
 static inline int ata_acpi_on_devcfg(struct ata_device *adev) { return 0; }
+static inline void ata_acpi_set_state(struct ata_port *ap, pm_message_t state) { }
 #endif
 
 /* libata-scsi.c */

Re: [PATCH]libata-acpi: add ACPI _PSx method

[Jeff Garzik]

Shaohua Li wrote:
> ACPI spec (ver 3.0a, p289) requires IDE power on/off executes ACPI _PSx
> methods. As recently most PATA drivers use libata, this patch adds _PSx
> method support in libata. ACPI spec doesn't mention if SATA requires the
> same _PSx method, but executing _PSx for SATA should be ok I think.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>

Where/how can a need for this change be produced?

	Jeff

Re: [PATCH]libata-acpi: add ACPI _PSx method

[Shaohua Li]

On Thu, 2007-09-13 at 23:21 -0400, Jeff Garzik wrote:
> Shaohua Li wrote:
> > ACPI spec (ver 3.0a, p289) requires IDE power on/off executes ACPI _PSx
> > methods. As recently most PATA drivers use libata, this patch adds _PSx
> > method support in libata. ACPI spec doesn't mention if SATA requires the
> > same _PSx method, but executing _PSx for SATA should be ok I think.
> > 
> > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> 
> Where/how can a need for this change be produced?
I run suspend/resume test in a Thinkpad R40 laptop, which implements
_PSx method in IDE devices. But I'd say I didn't see visible difference
in the suspend/resume circle w/wo the patch in the test laptop. This is
just to make Linux match with ACPI spec. Maybe this changes other
laptops.

Thanks,
Shaohua

Re: [PATCH]libata-acpi: add ACPI _PSx method

[Jeff Garzik]

Shaohua Li wrote:
> ACPI spec (ver 3.0a, p289) requires IDE power on/off executes ACPI _PSx
> methods. As recently most PATA drivers use libata, this patch adds _PSx
> method support in libata. ACPI spec doesn't mention if SATA requires the
> same _PSx method, but executing _PSx for SATA should be ok I think.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>

ahci and sata_sil24 directly power off components, so it seems quite 
unwise for "modern" SATA controllers.

ISTR Tejun, when he cleaned up libata-acpi, finally figured out a good 
way to distinguish ACPI's idea of "IDE" -- which sometimes includes the 
legacy programming mode of SATA controllers, and a controller running a 
modern programming mode.

Your best bet is to use a similar test when deciding when to execute 
_PSx.  Or, you could simply be conservative and only do it for PATA.

outside of those issues, the rest of the patch looks ok.

Re: [PATCH]libata-acpi: add ACPI _PSx method

[Shaohua Li]

On Thu, 2007-09-20 at 18:00 -0400, Jeff Garzik wrote:
> Shaohua Li wrote:
> > ACPI spec (ver 3.0a, p289) requires IDE power on/off executes ACPI _PSx
> > methods. As recently most PATA drivers use libata, this patch adds _PSx
> > method support in libata. ACPI spec doesn't mention if SATA requires the
> > same _PSx method, but executing _PSx for SATA should be ok I think.
> > 
> > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> 
> ahci and sata_sil24 directly power off components, so it seems quite 
> unwise for "modern" SATA controllers.
> 
> ISTR Tejun, when he cleaned up libata-acpi, finally figured out a good 
> way to distinguish ACPI's idea of "IDE" -- which sometimes includes the 
> legacy programming mode of SATA controllers, and a controller running a 
> modern programming mode.
> 
> Your best bet is to use a similar test when deciding when to execute 
> _PSx.  Or, you could simply be conservative and only do it for PATA.
> 
> outside of those issues, the rest of the patch looks ok.
Then how about this one? I added a check in ata_acpi_set_state to skip
SATA device, assume this is what you are talking about.

Thanks,
Shaohua

---
 drivers/ata/libata-acpi.c |   34 ++++++++++++++++++++++++++++++++++
 drivers/ata/libata-eh.c   |    3 +++
 drivers/ata/libata.h      |    2 ++
 3 files changed, 39 insertions(+)

Index: linux/drivers/ata/libata-acpi.c
===================================================================
--- linux.orig/drivers/ata/libata-acpi.c	2007-09-14 10:28:25.000000000 +0800
+++ linux/drivers/ata/libata-acpi.c	2007-09-21 08:58:18.000000000 +0800
@@ -523,6 +523,40 @@ void ata_acpi_on_resume(struct ata_port 
 }
 
 /**
+ * ata_acpi_set_state - set the port power state
+ * @ap: target ATA port
+ * @state: state, on/off
+ *
+ * This function executes the _PS0/_PS3 ACPI method to set the power state.
+ * ACPI spec requires _PS0 when IDE power on and _PS3 when power off
+ */
+void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
+{
+	int max_devices = 1, i;
+
+	if (!ap->acpi_handle || (ap->flags & ATA_FLAG_ACPI_SATA))
+		return;
+
+	/* channel first and then drives for power on and verse versa for power off */
+	if (state.event == PM_EVENT_ON)
+		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D0);
+
+	if (ap->flags & ATA_FLAG_SLAVE_POSS)
+		max_devices++;
+
+	for (i = 0; i < max_devices; ++i) {
+		struct ata_device *dev = &ap->device[i];
+
+		if (dev->acpi_handle)
+			acpi_bus_set_power(dev->acpi_handle,
+				state.event == PM_EVENT_ON ?
+				ACPI_STATE_D0: ACPI_STATE_D3);
+	}
+	if (state.event != PM_EVENT_ON)
+		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D3);
+}
+
+/**
  * ata_acpi_on_devcfg - ATA ACPI hook called on device donfiguration
  * @dev: target ATA device
  *
Index: linux/drivers/ata/libata-eh.c
===================================================================
--- linux.orig/drivers/ata/libata-eh.c	2007-09-14 10:28:25.000000000 +0800
+++ linux/drivers/ata/libata-eh.c	2007-09-21 08:56:40.000000000 +0800
@@ -2349,6 +2349,7 @@ static void ata_eh_handle_port_suspend(s
 	if (ap->ops->port_suspend)
 		rc = ap->ops->port_suspend(ap, ap->pm_mesg);
 
+	ata_acpi_set_state(ap, PMSG_SUSPEND);
  out:
 	/* report result */
 	spin_lock_irqsave(ap->lock, flags);
@@ -2394,6 +2395,8 @@ static void ata_eh_handle_port_resume(st
 
 	WARN_ON(!(ap->pflags & ATA_PFLAG_SUSPENDED));
 
+	ata_acpi_set_state(ap, PMSG_ON);
+
 	if (ap->ops->port_resume)
 		rc = ap->ops->port_resume(ap);
 
Index: linux/drivers/ata/libata.h
===================================================================
--- linux.orig/drivers/ata/libata.h	2007-09-14 10:28:25.000000000 +0800
+++ linux/drivers/ata/libata.h	2007-09-21 08:56:40.000000000 +0800
@@ -102,11 +102,13 @@ extern void ata_acpi_associate(struct at
 extern int ata_acpi_on_suspend(struct ata_port *ap);
 extern void ata_acpi_on_resume(struct ata_port *ap);
 extern int ata_acpi_on_devcfg(struct ata_device *adev);
+extern void ata_acpi_set_state(struct ata_port *ap, pm_message_t state);
 #else
 static inline void ata_acpi_associate(struct ata_host *host) { }
 static inline int ata_acpi_on_suspend(struct ata_port *ap) { return 0; }
 static inline void ata_acpi_on_resume(struct ata_port *ap) { }
 static inline int ata_acpi_on_devcfg(struct ata_device *adev) { return 0; }
+static inline void ata_acpi_set_state(struct ata_port *ap, pm_message_t state) { }
 #endif
 
 /* libata-scsi.c */

Re: [PATCH]libata-acpi: add ACPI _PSx method

[Tejun Heo]

Hello,

Shaohua Li wrote:
> +	/* channel first and then drives for power on and verse versa for power off */
> +	if (state.event == PM_EVENT_ON)
> +		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D0);
> +
> +	if (ap->flags & ATA_FLAG_SLAVE_POSS)
> +		max_devices++;

ata_port_max_devices()?

> Index: linux/drivers/ata/libata-eh.c
> ===================================================================
> --- linux.orig/drivers/ata/libata-eh.c	2007-09-14 10:28:25.000000000 +0800
> +++ linux/drivers/ata/libata-eh.c	2007-09-21 08:56:40.000000000 +0800
> @@ -2349,6 +2349,7 @@ static void ata_eh_handle_port_suspend(s
>  	if (ap->ops->port_suspend)
>  		rc = ap->ops->port_suspend(ap, ap->pm_mesg);
>  
> +	ata_acpi_set_state(ap, PMSG_SUSPEND);
>   out:
>  	/* report result */
>  	spin_lock_irqsave(ap->lock, flags);
> @@ -2394,6 +2395,8 @@ static void ata_eh_handle_port_resume(st
>  
>  	WARN_ON(!(ap->pflags & ATA_PFLAG_SUSPENDED));
>  
> +	ata_acpi_set_state(ap, PMSG_ON);
> +
>  	if (ap->ops->port_resume)
>  		rc = ap->ops->port_resume(ap);

Can these be moved into ata_acpi_on_suspend() and ata_acpi_on_resume()?
 Or do they have to be placed on the other side of ->port_suspend/resume
callbacks?

Thanks.

-- 
tejun

Re: [PATCH]libata-acpi: add ACPI _PSx method

[Shaohua Li]

On Fri, 2007-09-21 at 11:20 +0900, Tejun Heo wrote:
> Hello,
> 
> Shaohua Li wrote:
> > +	/* channel first and then drives for power on and verse versa for power off */
> > +	if (state.event == PM_EVENT_ON)
> > +		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D0);
> > +
> > +	if (ap->flags & ATA_FLAG_SLAVE_POSS)
> > +		max_devices++;
> 
> ata_port_max_devices()?
ok, fixed.
> 
> > Index: linux/drivers/ata/libata-eh.c
> > ===================================================================
> > --- linux.orig/drivers/ata/libata-eh.c	2007-09-14 10:28:25.000000000 +0800
> > +++ linux/drivers/ata/libata-eh.c	2007-09-21 08:56:40.000000000 +0800
> > @@ -2349,6 +2349,7 @@ static void ata_eh_handle_port_suspend(s
> >  	if (ap->ops->port_suspend)
> >  		rc = ap->ops->port_suspend(ap, ap->pm_mesg);
> >  
> > +	ata_acpi_set_state(ap, PMSG_SUSPEND);
> >   out:
> >  	/* report result */
> >  	spin_lock_irqsave(ap->lock, flags);
> > @@ -2394,6 +2395,8 @@ static void ata_eh_handle_port_resume(st
> >  
> >  	WARN_ON(!(ap->pflags & ATA_PFLAG_SUSPENDED));
> >  
> > +	ata_acpi_set_state(ap, PMSG_ON);
> > +
> >  	if (ap->ops->port_resume)
> >  		rc = ap->ops->port_resume(ap);
> 
> Can these be moved into ata_acpi_on_suspend() and ata_acpi_on_resume()?
>  Or do they have to be placed on the other side of ->port_suspend/resume
> callbacks?
I suppose _PSx will shutdown ports. So I want to do PS3 as later as
possible in suspend, and vice versa in resume.

Thanks,
Shaohua


---
 drivers/ata/libata-acpi.c |   34 ++++++++++++++++++++++++++++++++++
 drivers/ata/libata-eh.c   |    3 +++
 drivers/ata/libata.h      |    2 ++
 3 files changed, 39 insertions(+)

Index: linux/drivers/ata/libata-acpi.c
===================================================================
--- linux.orig/drivers/ata/libata-acpi.c	2007-09-21 09:23:13.000000000 +0800
+++ linux/drivers/ata/libata-acpi.c	2007-09-21 10:13:32.000000000 +0800
@@ -523,6 +523,39 @@ void ata_acpi_on_resume(struct ata_port 
 }
 
 /**
+ * ata_acpi_set_state - set the port power state
+ * @ap: target ATA port
+ * @state: state, on/off
+ *
+ * This function executes the _PS0/_PS3 ACPI method to set the power state.
+ * ACPI spec requires _PS0 when IDE power on and _PS3 when power off
+ */
+void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
+{
+	int max_devices, i;
+
+	if (!ap->acpi_handle || (ap->flags & ATA_FLAG_ACPI_SATA))
+		return;
+
+	/* channel first and then drives for power on and verse versa for power off */
+	if (state.event == PM_EVENT_ON)
+		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D0);
+
+	max_devices = ata_port_max_devices(ap);
+
+	for (i = 0; i < max_devices; ++i) {
+		struct ata_device *dev = &ap->device[i];
+
+		if (dev->acpi_handle)
+			acpi_bus_set_power(dev->acpi_handle,
+				state.event == PM_EVENT_ON ?
+				ACPI_STATE_D0: ACPI_STATE_D3);
+	}
+	if (state.event != PM_EVENT_ON)
+		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D3);
+}
+
+/**
  * ata_acpi_on_devcfg - ATA ACPI hook called on device donfiguration
  * @dev: target ATA device
  *
Index: linux/drivers/ata/libata-eh.c
===================================================================
--- linux.orig/drivers/ata/libata-eh.c	2007-09-21 09:23:13.000000000 +0800
+++ linux/drivers/ata/libata-eh.c	2007-09-21 10:09:10.000000000 +0800
@@ -2349,6 +2349,7 @@ static void ata_eh_handle_port_suspend(s
 	if (ap->ops->port_suspend)
 		rc = ap->ops->port_suspend(ap, ap->pm_mesg);
 
+	ata_acpi_set_state(ap, PMSG_SUSPEND);
  out:
 	/* report result */
 	spin_lock_irqsave(ap->lock, flags);
@@ -2394,6 +2395,8 @@ static void ata_eh_handle_port_resume(st
 
 	WARN_ON(!(ap->pflags & ATA_PFLAG_SUSPENDED));
 
+	ata_acpi_set_state(ap, PMSG_ON);
+
 	if (ap->ops->port_resume)
 		rc = ap->ops->port_resume(ap);
 
Index: linux/drivers/ata/libata.h
===================================================================
--- linux.orig/drivers/ata/libata.h	2007-09-21 09:23:13.000000000 +0800
+++ linux/drivers/ata/libata.h	2007-09-21 10:09:10.000000000 +0800
@@ -102,11 +102,13 @@ extern void ata_acpi_associate(struct at
 extern int ata_acpi_on_suspend(struct ata_port *ap);
 extern void ata_acpi_on_resume(struct ata_port *ap);
 extern int ata_acpi_on_devcfg(struct ata_device *adev);
+extern void ata_acpi_set_state(struct ata_port *ap, pm_message_t state);
 #else
 static inline void ata_acpi_associate(struct ata_host *host) { }
 static inline int ata_acpi_on_suspend(struct ata_port *ap) { return 0; }
 static inline void ata_acpi_on_resume(struct ata_port *ap) { }
 static inline int ata_acpi_on_devcfg(struct ata_device *adev) { return 0; }
+static inline void ata_acpi_set_state(struct ata_port *ap, pm_message_t state) { }
 #endif
 
 /* libata-scsi.c */

Re: [PATCH]libata-acpi: add ACPI _PSx method

[Len Brown]

It would be interseting if any of the folks having power consumption
problems when suspended see an improvement with this patch.

Are there plans to refresh this patch and get it upstream?

Acked-by: Len Brown <len.brown@intel.com>

thanks,
-Len

On Thursday 20 September 2007 22:17, Shaohua Li wrote:
> On Fri, 2007-09-21 at 11:20 +0900, Tejun Heo wrote:
> > Hello,
> > 
> > Shaohua Li wrote:
> > > +	/* channel first and then drives for power on and verse versa for power off */
> > > +	if (state.event == PM_EVENT_ON)
> > > +		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D0);
> > > +
> > > +	if (ap->flags & ATA_FLAG_SLAVE_POSS)
> > > +		max_devices++;
> > 
> > ata_port_max_devices()?
> ok, fixed.
> > 
> > > Index: linux/drivers/ata/libata-eh.c
> > > ===================================================================
> > > --- linux.orig/drivers/ata/libata-eh.c	2007-09-14 10:28:25.000000000 +0800
> > > +++ linux/drivers/ata/libata-eh.c	2007-09-21 08:56:40.000000000 +0800
> > > @@ -2349,6 +2349,7 @@ static void ata_eh_handle_port_suspend(s
> > >  	if (ap->ops->port_suspend)
> > >  		rc = ap->ops->port_suspend(ap, ap->pm_mesg);
> > >  
> > > +	ata_acpi_set_state(ap, PMSG_SUSPEND);
> > >   out:
> > >  	/* report result */
> > >  	spin_lock_irqsave(ap->lock, flags);
> > > @@ -2394,6 +2395,8 @@ static void ata_eh_handle_port_resume(st
> > >  
> > >  	WARN_ON(!(ap->pflags & ATA_PFLAG_SUSPENDED));
> > >  
> > > +	ata_acpi_set_state(ap, PMSG_ON);
> > > +
> > >  	if (ap->ops->port_resume)
> > >  		rc = ap->ops->port_resume(ap);
> > 
> > Can these be moved into ata_acpi_on_suspend() and ata_acpi_on_resume()?
> >  Or do they have to be placed on the other side of ->port_suspend/resume
> > callbacks?
> I suppose _PSx will shutdown ports. So I want to do PS3 as later as
> possible in suspend, and vice versa in resume.
> 
> Thanks,
> Shaohua
> 
> 
> ---
>  drivers/ata/libata-acpi.c |   34 ++++++++++++++++++++++++++++++++++
>  drivers/ata/libata-eh.c   |    3 +++
>  drivers/ata/libata.h      |    2 ++
>  3 files changed, 39 insertions(+)
> 
> Index: linux/drivers/ata/libata-acpi.c
> ===================================================================
> --- linux.orig/drivers/ata/libata-acpi.c	2007-09-21 09:23:13.000000000 +0800
> +++ linux/drivers/ata/libata-acpi.c	2007-09-21 10:13:32.000000000 +0800
> @@ -523,6 +523,39 @@ void ata_acpi_on_resume(struct ata_port 
>  }
>  
>  /**
> + * ata_acpi_set_state - set the port power state
> + * @ap: target ATA port
> + * @state: state, on/off
> + *
> + * This function executes the _PS0/_PS3 ACPI method to set the power state.
> + * ACPI spec requires _PS0 when IDE power on and _PS3 when power off
> + */
> +void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
> +{
> +	int max_devices, i;
> +
> +	if (!ap->acpi_handle || (ap->flags & ATA_FLAG_ACPI_SATA))
> +		return;
> +
> +	/* channel first and then drives for power on and verse versa for power off */
> +	if (state.event == PM_EVENT_ON)
> +		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D0);
> +
> +	max_devices = ata_port_max_devices(ap);
> +
> +	for (i = 0; i < max_devices; ++i) {
> +		struct ata_device *dev = &ap->device[i];
> +
> +		if (dev->acpi_handle)
> +			acpi_bus_set_power(dev->acpi_handle,
> +				state.event == PM_EVENT_ON ?
> +				ACPI_STATE_D0: ACPI_STATE_D3);
> +	}
> +	if (state.event != PM_EVENT_ON)
> +		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D3);
> +}
> +
> +/**
>   * ata_acpi_on_devcfg - ATA ACPI hook called on device donfiguration
>   * @dev: target ATA device
>   *
> Index: linux/drivers/ata/libata-eh.c
> ===================================================================
> --- linux.orig/drivers/ata/libata-eh.c	2007-09-21 09:23:13.000000000 +0800
> +++ linux/drivers/ata/libata-eh.c	2007-09-21 10:09:10.000000000 +0800
> @@ -2349,6 +2349,7 @@ static void ata_eh_handle_port_suspend(s
>  	if (ap->ops->port_suspend)
>  		rc = ap->ops->port_suspend(ap, ap->pm_mesg);
>  
> +	ata_acpi_set_state(ap, PMSG_SUSPEND);
>   out:
>  	/* report result */
>  	spin_lock_irqsave(ap->lock, flags);
> @@ -2394,6 +2395,8 @@ static void ata_eh_handle_port_resume(st
>  
>  	WARN_ON(!(ap->pflags & ATA_PFLAG_SUSPENDED));
>  
> +	ata_acpi_set_state(ap, PMSG_ON);
> +
>  	if (ap->ops->port_resume)
>  		rc = ap->ops->port_resume(ap);
>  
> Index: linux/drivers/ata/libata.h
> ===================================================================
> --- linux.orig/drivers/ata/libata.h	2007-09-21 09:23:13.000000000 +0800
> +++ linux/drivers/ata/libata.h	2007-09-21 10:09:10.000000000 +0800
> @@ -102,11 +102,13 @@ extern void ata_acpi_associate(struct at
>  extern int ata_acpi_on_suspend(struct ata_port *ap);
>  extern void ata_acpi_on_resume(struct ata_port *ap);
>  extern int ata_acpi_on_devcfg(struct ata_device *adev);
> +extern void ata_acpi_set_state(struct ata_port *ap, pm_message_t state);
>  #else
>  static inline void ata_acpi_associate(struct ata_host *host) { }
>  static inline int ata_acpi_on_suspend(struct ata_port *ap) { return 0; }
>  static inline void ata_acpi_on_resume(struct ata_port *ap) { }
>  static inline int ata_acpi_on_devcfg(struct ata_device *adev) { return 0; }
> +static inline void ata_acpi_set_state(struct ata_port *ap, pm_message_t state) { }
>  #endif
>  
>  /* libata-scsi.c */
> 

Re: [PATCH]libata-acpi: add ACPI _PSx method

[Rafael J. Wysocki]

On Wednesday, 31 October 2007 03:27, Len Brown wrote:
> It would be interseting if any of the folks having power consumption
> problems when suspended see an improvement with this patch.
> 
> Are there plans to refresh this patch and get it upstream?
> 
> Acked-by: Len Brown <len.brown@intel.com>

There was a discussion regarding this patch, IIRC, and it was argued that
_PSx are only applicable to IDE/PATA devices.

I wonder if this has been addressed.

Greetings,
Rafael


> On Thursday 20 September 2007 22:17, Shaohua Li wrote:
> > On Fri, 2007-09-21 at 11:20 +0900, Tejun Heo wrote:
> > > Hello,
> > > 
> > > Shaohua Li wrote:
> > > > +	/* channel first and then drives for power on and verse versa for power off */
> > > > +	if (state.event == PM_EVENT_ON)
> > > > +		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D0);
> > > > +
> > > > +	if (ap->flags & ATA_FLAG_SLAVE_POSS)
> > > > +		max_devices++;
> > > 
> > > ata_port_max_devices()?
> > ok, fixed.
> > > 
> > > > Index: linux/drivers/ata/libata-eh.c
> > > > ===================================================================
> > > > --- linux.orig/drivers/ata/libata-eh.c	2007-09-14 10:28:25.000000000 +0800
> > > > +++ linux/drivers/ata/libata-eh.c	2007-09-21 08:56:40.000000000 +0800
> > > > @@ -2349,6 +2349,7 @@ static void ata_eh_handle_port_suspend(s
> > > >  	if (ap->ops->port_suspend)
> > > >  		rc = ap->ops->port_suspend(ap, ap->pm_mesg);
> > > >  
> > > > +	ata_acpi_set_state(ap, PMSG_SUSPEND);
> > > >   out:
> > > >  	/* report result */
> > > >  	spin_lock_irqsave(ap->lock, flags);
> > > > @@ -2394,6 +2395,8 @@ static void ata_eh_handle_port_resume(st
> > > >  
> > > >  	WARN_ON(!(ap->pflags & ATA_PFLAG_SUSPENDED));
> > > >  
> > > > +	ata_acpi_set_state(ap, PMSG_ON);
> > > > +
> > > >  	if (ap->ops->port_resume)
> > > >  		rc = ap->ops->port_resume(ap);
> > > 
> > > Can these be moved into ata_acpi_on_suspend() and ata_acpi_on_resume()?
> > >  Or do they have to be placed on the other side of ->port_suspend/resume
> > > callbacks?
> > I suppose _PSx will shutdown ports. So I want to do PS3 as later as
> > possible in suspend, and vice versa in resume.
> > 
> > Thanks,
> > Shaohua
> > 
> > 
> > ---
> >  drivers/ata/libata-acpi.c |   34 ++++++++++++++++++++++++++++++++++
> >  drivers/ata/libata-eh.c   |    3 +++
> >  drivers/ata/libata.h      |    2 ++
> >  3 files changed, 39 insertions(+)
> > 
> > Index: linux/drivers/ata/libata-acpi.c
> > ===================================================================
> > --- linux.orig/drivers/ata/libata-acpi.c	2007-09-21 09:23:13.000000000 +0800
> > +++ linux/drivers/ata/libata-acpi.c	2007-09-21 10:13:32.000000000 +0800
> > @@ -523,6 +523,39 @@ void ata_acpi_on_resume(struct ata_port 
> >  }
> >  
> >  /**
> > + * ata_acpi_set_state - set the port power state
> > + * @ap: target ATA port
> > + * @state: state, on/off
> > + *
> > + * This function executes the _PS0/_PS3 ACPI method to set the power state.
> > + * ACPI spec requires _PS0 when IDE power on and _PS3 when power off
> > + */
> > +void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
> > +{
> > +	int max_devices, i;
> > +
> > +	if (!ap->acpi_handle || (ap->flags & ATA_FLAG_ACPI_SATA))
> > +		return;
> > +
> > +	/* channel first and then drives for power on and verse versa for power off */
> > +	if (state.event == PM_EVENT_ON)
> > +		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D0);
> > +
> > +	max_devices = ata_port_max_devices(ap);
> > +
> > +	for (i = 0; i < max_devices; ++i) {
> > +		struct ata_device *dev = &ap->device[i];
> > +
> > +		if (dev->acpi_handle)
> > +			acpi_bus_set_power(dev->acpi_handle,
> > +				state.event == PM_EVENT_ON ?
> > +				ACPI_STATE_D0: ACPI_STATE_D3);
> > +	}
> > +	if (state.event != PM_EVENT_ON)
> > +		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D3);
> > +}
> > +
> > +/**
> >   * ata_acpi_on_devcfg - ATA ACPI hook called on device donfiguration
> >   * @dev: target ATA device
> >   *
> > Index: linux/drivers/ata/libata-eh.c
> > ===================================================================
> > --- linux.orig/drivers/ata/libata-eh.c	2007-09-21 09:23:13.000000000 +0800
> > +++ linux/drivers/ata/libata-eh.c	2007-09-21 10:09:10.000000000 +0800
> > @@ -2349,6 +2349,7 @@ static void ata_eh_handle_port_suspend(s
> >  	if (ap->ops->port_suspend)
> >  		rc = ap->ops->port_suspend(ap, ap->pm_mesg);
> >  
> > +	ata_acpi_set_state(ap, PMSG_SUSPEND);
> >   out:
> >  	/* report result */
> >  	spin_lock_irqsave(ap->lock, flags);
> > @@ -2394,6 +2395,8 @@ static void ata_eh_handle_port_resume(st
> >  
> >  	WARN_ON(!(ap->pflags & ATA_PFLAG_SUSPENDED));
> >  
> > +	ata_acpi_set_state(ap, PMSG_ON);
> > +
> >  	if (ap->ops->port_resume)
> >  		rc = ap->ops->port_resume(ap);
> >  
> > Index: linux/drivers/ata/libata.h
> > ===================================================================
> > --- linux.orig/drivers/ata/libata.h	2007-09-21 09:23:13.000000000 +0800
> > +++ linux/drivers/ata/libata.h	2007-09-21 10:09:10.000000000 +0800
> > @@ -102,11 +102,13 @@ extern void ata_acpi_associate(struct at
> >  extern int ata_acpi_on_suspend(struct ata_port *ap);
> >  extern void ata_acpi_on_resume(struct ata_port *ap);
> >  extern int ata_acpi_on_devcfg(struct ata_device *adev);
> > +extern void ata_acpi_set_state(struct ata_port *ap, pm_message_t state);
> >  #else
> >  static inline void ata_acpi_associate(struct ata_host *host) { }
> >  static inline int ata_acpi_on_suspend(struct ata_port *ap) { return 0; }
> >  static inline void ata_acpi_on_resume(struct ata_port *ap) { }
> >  static inline int ata_acpi_on_devcfg(struct ata_device *adev) { return 0; }
> > +static inline void ata_acpi_set_state(struct ata_port *ap, pm_message_t state) { }
> >  #endif
> >  
> >  /* libata-scsi.c */
> > 
> -
> 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
> 
> 

-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [PATCH]libata-acpi: add ACPI _PSx method

[Shaohua Li]

On Wed, 2007-10-31 at 14:26 +0100, Rafael J. Wysocki wrote:
> On Wednesday, 31 October 2007 03:27, Len Brown wrote:
> > It would be interseting if any of the folks having power consumption
> > problems when suspended see an improvement with this patch.
> > 
> > Are there plans to refresh this patch and get it upstream?
> > 
> > Acked-by: Len Brown <len.brown@intel.com>
> 
> There was a discussion regarding this patch, IIRC, and it was argued that
> _PSx are only applicable to IDE/PATA devices.
> 
> I wonder if this has been addressed.
Sure, there is a check for PATA. I refreshed the patch to against latest
git tree.

ACPI spec (ver 3.0a, p289) requires IDE power on/off executes ACPI _PSx
methods. As recently most PATA drivers use libata, this patch adds _PSx
method support in libata. ACPI spec doesn't mention if SATA requires the
same _PSx method.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
Acked-by: Len Brown <len.brown@intel.com>
---
 drivers/ata/libata-acpi.c |   33 +++++++++++++++++++++++++++++++++
 drivers/ata/libata-eh.c   |    3 +++
 drivers/ata/libata.h      |    2 ++
 3 files changed, 38 insertions(+)

Index: linux/drivers/ata/libata-acpi.c
===================================================================
--- linux.orig/drivers/ata/libata-acpi.c	2007-10-26 08:49:07.000000000 +0800
+++ linux/drivers/ata/libata-acpi.c	2007-11-01 09:17:12.000000000 +0800
@@ -652,6 +652,39 @@ void ata_acpi_on_resume(struct ata_port 
 }
 
 /**
+ * ata_acpi_set_state - set the port power state
+ * @ap: target ATA port
+ * @state: state, on/off
+ *
+ * This function executes the _PS0/_PS3 ACPI method to set the power state.
+ * ACPI spec requires _PS0 when IDE power on and _PS3 when power off
+ */
+void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
+{
+	int max_devices, i;
+
+	if (!ap->acpi_handle || (ap->flags & ATA_FLAG_ACPI_SATA))
+		return;
+
+	/* channel first and then drives for power on and verse versa for power off */
+	if (state.event == PM_EVENT_ON)
+		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D0);
+
+	max_devices = ata_link_max_devices(&ap->link);
+
+	for (i = 0; i < max_devices; ++i) {
+		struct ata_device *dev = &ap->link.device[i];
+
+		if (dev->acpi_handle)
+			acpi_bus_set_power(dev->acpi_handle,
+				state.event == PM_EVENT_ON ?
+				ACPI_STATE_D0: ACPI_STATE_D3);
+	}
+	if (state.event != PM_EVENT_ON)
+		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D3);
+}
+
+/**
  * ata_acpi_on_devcfg - ATA ACPI hook called on device donfiguration
  * @dev: target ATA device
  *
Index: linux/drivers/ata/libata-eh.c
===================================================================
--- linux.orig/drivers/ata/libata-eh.c	2007-10-31 09:14:06.000000000 +0800
+++ linux/drivers/ata/libata-eh.c	2007-11-01 08:55:05.000000000 +0800
@@ -2796,6 +2796,7 @@ static void ata_eh_handle_port_suspend(s
 	if (ap->ops->port_suspend)
 		rc = ap->ops->port_suspend(ap, ap->pm_mesg);
 
+	ata_acpi_set_state(ap, PMSG_SUSPEND);
  out:
 	/* report result */
 	spin_lock_irqsave(ap->lock, flags);
@@ -2841,6 +2842,8 @@ static void ata_eh_handle_port_resume(st
 
 	WARN_ON(!(ap->pflags & ATA_PFLAG_SUSPENDED));
 
+	ata_acpi_set_state(ap, PMSG_ON);
+
 	if (ap->ops->port_resume)
 		rc = ap->ops->port_resume(ap);
 
Index: linux/drivers/ata/libata.h
===================================================================
--- linux.orig/drivers/ata/libata.h	2007-10-30 14:17:14.000000000 +0800
+++ linux/drivers/ata/libata.h	2007-11-01 08:55:05.000000000 +0800
@@ -111,12 +111,14 @@ extern void ata_acpi_associate(struct at
 extern int ata_acpi_on_suspend(struct ata_port *ap);
 extern void ata_acpi_on_resume(struct ata_port *ap);
 extern int ata_acpi_on_devcfg(struct ata_device *adev);
+extern void ata_acpi_set_state(struct ata_port *ap, pm_message_t state);
 #else
 static inline void ata_acpi_associate_sata_port(struct ata_port *ap) { }
 static inline void ata_acpi_associate(struct ata_host *host) { }
 static inline int ata_acpi_on_suspend(struct ata_port *ap) { return 0; }
 static inline void ata_acpi_on_resume(struct ata_port *ap) { }
 static inline int ata_acpi_on_devcfg(struct ata_device *adev) { return 0; }
+static inline void ata_acpi_set_state(struct ata_port *ap, pm_message_t state) { }
 #endif
 
 /* libata-scsi.c */

Re: [PATCH]libata-acpi: add ACPI _PSx method

[Alan Cox]

> +	max_devices = ata_link_max_devices(&ap->link);
> +
> +	for (i = 0; i < max_devices; ++i) {
> +		struct ata_device *dev = &ap->link.device[i];

Better to use:

	ata_link_for_each_dev(dev, &ap->link) {		

> +
> +		if (dev->acpi_handle)
> +			acpi_bus_set_power(dev->acpi_handle,
> +				state.event == PM_EVENT_ON ?
> +				ACPI_STATE_D0: ACPI_STATE_D3);
> +	}

Also should you not check ata_dev_enabled(dev) ?

Alan

Re: [PATCH]libata-acpi: add ACPI _PSx method

[Shaohua Li]

On Thu, 2007-11-01 at 10:04 +0000, Alan Cox wrote:
> > +	max_devices = ata_link_max_devices(&ap->link);
> > +
> > +	for (i = 0; i < max_devices; ++i) {
> > +		struct ata_device *dev = &ap->link.device[i];
> 
> Better to use:
ok.
> 	ata_link_for_each_dev(dev, &ap->link) {		
> 
> > +
> > +		if (dev->acpi_handle)
> > +			acpi_bus_set_power(dev->acpi_handle,
> > +				state.event == PM_EVENT_ON ?
> > +				ACPI_STATE_D0: ACPI_STATE_D3);
> > +	}
> 
> Also should you not check ata_dev_enabled(dev) ?
Not sure, this is just to call a BIOS routine, but a check should be
safer. Thanks!

ACPI spec (ver 3.0a, p289) requires IDE power on/off executes ACPI _PSx
methods. As recently most PATA drivers use libata, this patch adds _PSx
method support in libata. ACPI spec doesn't mention if SATA requires the
same _PSx method.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
Acked-by: Len Brown <len.brown@intel.com>
---
 drivers/ata/libata-acpi.c |   29 +++++++++++++++++++++++++++++
 drivers/ata/libata-eh.c   |    3 +++
 drivers/ata/libata.h      |    2 ++
 3 files changed, 34 insertions(+)

Index: linux/drivers/ata/libata-acpi.c
===================================================================
--- linux.orig/drivers/ata/libata-acpi.c	2007-11-01 10:54:25.000000000 +0800
+++ linux/drivers/ata/libata-acpi.c	2007-11-02 09:14:12.000000000 +0800
@@ -652,6 +652,35 @@ void ata_acpi_on_resume(struct ata_port 
 }
 
 /**
+ * ata_acpi_set_state - set the port power state
+ * @ap: target ATA port
+ * @state: state, on/off
+ *
+ * This function executes the _PS0/_PS3 ACPI method to set the power state.
+ * ACPI spec requires _PS0 when IDE power on and _PS3 when power off
+ */
+void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
+{
+	struct ata_device *dev;
+
+	if (!ap->acpi_handle || (ap->flags & ATA_FLAG_ACPI_SATA))
+		return;
+
+	/* channel first and then drives for power on and verse versa for power off */
+	if (state.event == PM_EVENT_ON)
+		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D0);
+
+	ata_link_for_each_dev(dev, &ap->link) {
+		if (dev->acpi_handle && ata_dev_enabled(dev))
+			acpi_bus_set_power(dev->acpi_handle,
+				state.event == PM_EVENT_ON ?
+				ACPI_STATE_D0: ACPI_STATE_D3);
+	}
+	if (state.event != PM_EVENT_ON)
+		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D3);
+}
+
+/**
  * ata_acpi_on_devcfg - ATA ACPI hook called on device donfiguration
  * @dev: target ATA device
  *
Index: linux/drivers/ata/libata-eh.c
===================================================================
--- linux.orig/drivers/ata/libata-eh.c	2007-11-01 10:54:25.000000000 +0800
+++ linux/drivers/ata/libata-eh.c	2007-11-02 09:01:41.000000000 +0800
@@ -2796,6 +2796,7 @@ static void ata_eh_handle_port_suspend(s
 	if (ap->ops->port_suspend)
 		rc = ap->ops->port_suspend(ap, ap->pm_mesg);
 
+	ata_acpi_set_state(ap, PMSG_SUSPEND);
  out:
 	/* report result */
 	spin_lock_irqsave(ap->lock, flags);
@@ -2841,6 +2842,8 @@ static void ata_eh_handle_port_resume(st
 
 	WARN_ON(!(ap->pflags & ATA_PFLAG_SUSPENDED));
 
+	ata_acpi_set_state(ap, PMSG_ON);
+
 	if (ap->ops->port_resume)
 		rc = ap->ops->port_resume(ap);
 
Index: linux/drivers/ata/libata.h
===================================================================
--- linux.orig/drivers/ata/libata.h	2007-11-01 10:54:25.000000000 +0800
+++ linux/drivers/ata/libata.h	2007-11-02 09:01:41.000000000 +0800
@@ -111,12 +111,14 @@ extern void ata_acpi_associate(struct at
 extern int ata_acpi_on_suspend(struct ata_port *ap);
 extern void ata_acpi_on_resume(struct ata_port *ap);
 extern int ata_acpi_on_devcfg(struct ata_device *adev);
+extern void ata_acpi_set_state(struct ata_port *ap, pm_message_t state);
 #else
 static inline void ata_acpi_associate_sata_port(struct ata_port *ap) { }
 static inline void ata_acpi_associate(struct ata_host *host) { }
 static inline int ata_acpi_on_suspend(struct ata_port *ap) { return 0; }
 static inline void ata_acpi_on_resume(struct ata_port *ap) { }
 static inline int ata_acpi_on_devcfg(struct ata_device *adev) { return 0; }
+static inline void ata_acpi_set_state(struct ata_port *ap, pm_message_t state) { }
 #endif
 
 /* libata-scsi.c */

Re: [PATCH]libata-acpi: add ACPI _PSx method

[Rafael J. Wysocki]

On Friday, 2 November 2007 02:32, Shaohua Li wrote:
> On Thu, 2007-11-01 at 10:04 +0000, Alan Cox wrote:
> > > +	max_devices = ata_link_max_devices(&ap->link);
> > > +
> > > +	for (i = 0; i < max_devices; ++i) {
> > > +		struct ata_device *dev = &ap->link.device[i];
> > 
> > Better to use:
> ok.
> > 	ata_link_for_each_dev(dev, &ap->link) {		
> > 
> > > +
> > > +		if (dev->acpi_handle)
> > > +			acpi_bus_set_power(dev->acpi_handle,
> > > +				state.event == PM_EVENT_ON ?
> > > +				ACPI_STATE_D0: ACPI_STATE_D3);
> > > +	}
> > 
> > Also should you not check ata_dev_enabled(dev) ?
> Not sure, this is just to call a BIOS routine, but a check should be
> safer. Thanks!
> 
> ACPI spec (ver 3.0a, p289) requires IDE power on/off executes ACPI _PSx
> methods. As recently most PATA drivers use libata, this patch adds _PSx
> method support in libata. ACPI spec doesn't mention if SATA requires the
> same _PSx method.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> Acked-by: Len Brown <len.brown@intel.com>
> ---
>  drivers/ata/libata-acpi.c |   29 +++++++++++++++++++++++++++++
>  drivers/ata/libata-eh.c   |    3 +++
>  drivers/ata/libata.h      |    2 ++
>  3 files changed, 34 insertions(+)
> 
> Index: linux/drivers/ata/libata-acpi.c
> ===================================================================
> --- linux.orig/drivers/ata/libata-acpi.c	2007-11-01 10:54:25.000000000 +0800
> +++ linux/drivers/ata/libata-acpi.c	2007-11-02 09:14:12.000000000 +0800
> @@ -652,6 +652,35 @@ void ata_acpi_on_resume(struct ata_port 
>  }
>  
>  /**
> + * ata_acpi_set_state - set the port power state
> + * @ap: target ATA port
> + * @state: state, on/off
> + *
> + * This function executes the _PS0/_PS3 ACPI method to set the power state.
> + * ACPI spec requires _PS0 when IDE power on and _PS3 when power off
> + */
> +void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)

I wouldn't use pm_message_t here.

We're seriously considering removing it in the future and the caller passes
constants explicitly anyway.

IMO, it might be cleaner to define two separate functions, one for switching
to D0 and one for switching to D3, and call them as appropriate.  Then, you
could get rid of some conditionals below.

> +{
> +	struct ata_device *dev;
> +
> +	if (!ap->acpi_handle || (ap->flags & ATA_FLAG_ACPI_SATA))
> +		return;
> +
> +	/* channel first and then drives for power on and verse versa for power off */
> +	if (state.event == PM_EVENT_ON)
> +		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D0);
> +
> +	ata_link_for_each_dev(dev, &ap->link) {
> +		if (dev->acpi_handle && ata_dev_enabled(dev))
> +			acpi_bus_set_power(dev->acpi_handle,
> +				state.event == PM_EVENT_ON ?
> +				ACPI_STATE_D0: ACPI_STATE_D3);
> +	}
> +	if (state.event != PM_EVENT_ON)
> +		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D3);
> +}
> +
> +/**
>   * ata_acpi_on_devcfg - ATA ACPI hook called on device donfiguration
>   * @dev: target ATA device
>   *
> Index: linux/drivers/ata/libata-eh.c
> ===================================================================
> --- linux.orig/drivers/ata/libata-eh.c	2007-11-01 10:54:25.000000000 +0800
> +++ linux/drivers/ata/libata-eh.c	2007-11-02 09:01:41.000000000 +0800
> @@ -2796,6 +2796,7 @@ static void ata_eh_handle_port_suspend(s
>  	if (ap->ops->port_suspend)
>  		rc = ap->ops->port_suspend(ap, ap->pm_mesg);
>  
> +	ata_acpi_set_state(ap, PMSG_SUSPEND);
>   out:
>  	/* report result */
>  	spin_lock_irqsave(ap->lock, flags);
> @@ -2841,6 +2842,8 @@ static void ata_eh_handle_port_resume(st
>  
>  	WARN_ON(!(ap->pflags & ATA_PFLAG_SUSPENDED));
>  
> +	ata_acpi_set_state(ap, PMSG_ON);
> +
>  	if (ap->ops->port_resume)
>  		rc = ap->ops->port_resume(ap);
>  
> Index: linux/drivers/ata/libata.h
> ===================================================================
> --- linux.orig/drivers/ata/libata.h	2007-11-01 10:54:25.000000000 +0800
> +++ linux/drivers/ata/libata.h	2007-11-02 09:01:41.000000000 +0800
> @@ -111,12 +111,14 @@ extern void ata_acpi_associate(struct at
>  extern int ata_acpi_on_suspend(struct ata_port *ap);
>  extern void ata_acpi_on_resume(struct ata_port *ap);
>  extern int ata_acpi_on_devcfg(struct ata_device *adev);
> +extern void ata_acpi_set_state(struct ata_port *ap, pm_message_t state);
>  #else
>  static inline void ata_acpi_associate_sata_port(struct ata_port *ap) { }
>  static inline void ata_acpi_associate(struct ata_host *host) { }
>  static inline int ata_acpi_on_suspend(struct ata_port *ap) { return 0; }
>  static inline void ata_acpi_on_resume(struct ata_port *ap) { }
>  static inline int ata_acpi_on_devcfg(struct ata_device *adev) { return 0; }
> +static inline void ata_acpi_set_state(struct ata_port *ap, pm_message_t state) { }
>  #endif
>  
>  /* libata-scsi.c */
> -
> 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
> 
> 

Greetings,
Rafael

Re: [PATCH]libata-acpi: add ACPI _PSx method

[Shaohua Li]

On Fri, 2007-11-02 at 12:16 +0100, Rafael J. Wysocki wrote:
> > 
> > Index: linux/drivers/ata/libata-acpi.c
> > ===================================================================
> > --- linux.orig/drivers/ata/libata-acpi.c	2007-11-01 10:54:25.000000000 +0800
> > +++ linux/drivers/ata/libata-acpi.c	2007-11-02 09:14:12.000000000 +0800
> > @@ -652,6 +652,35 @@ void ata_acpi_on_resume(struct ata_port 
> >  }
> >  
> >  /**
> > + * ata_acpi_set_state - set the port power state
> > + * @ap: target ATA port
> > + * @state: state, on/off
> > + *
> > + * This function executes the _PS0/_PS3 ACPI method to set the power state.
> > + * ACPI spec requires _PS0 when IDE power on and _PS3 when power off
> > + */
> > +void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
> 
> I wouldn't use pm_message_t here.
> 
> We're seriously considering removing it in the future and the caller passes
> constants explicitly anyway.
> 
> IMO, it might be cleaner to define two separate functions, one for switching
> to D0 and one for switching to D3, and call them as appropriate.  Then, you
> could get rid of some conditionals below.
It doesn't make sense to add a new one just for ata_acpi and we don't
misuse pm_message_t here. If you will remove it, lets clean up it then.

Thanks,
Shaohua

Re: [PATCH]libata-acpi: add ACPI _PSx method

[Jeff Garzik]

Shaohua Li wrote:
> Not sure, this is just to call a BIOS routine, but a check should be
> safer. Thanks!
> 
> ACPI spec (ver 3.0a, p289) requires IDE power on/off executes ACPI _PSx
> methods. As recently most PATA drivers use libata, this patch adds _PSx
> method support in libata. ACPI spec doesn't mention if SATA requires the
> same _PSx method.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> Acked-by: Len Brown <len.brown@intel.com>


What is the safety factor on this patch?  :)

Since we are into 2.6.24-rc, my preference would be to let this get some 
testing in -mm, and push it upstream for 2.6.25.  Comments?

	Jeff

Re: [PATCH]libata-acpi: add ACPI _PSx method

[Shaohua Li]

On Sat, 2007-11-03 at 09:00 -0400, Jeff Garzik wrote:
> Shaohua Li wrote:
> > Not sure, this is just to call a BIOS routine, but a check should be
> > safer. Thanks!
> > 
> > ACPI spec (ver 3.0a, p289) requires IDE power on/off executes ACPI _PSx
> > methods. As recently most PATA drivers use libata, this patch adds _PSx
> > method support in libata. ACPI spec doesn't mention if SATA requires the
> > same _PSx method.
> > 
> > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> > Acked-by: Len Brown <len.brown@intel.com>
> 
> 
> What is the safety factor on this patch?  :)
Not sure, but it depends on BIOS, I just guess ignoring disabled device might be safe :)
> Since we are into 2.6.24-rc, my preference would be to let this get some 
> testing in -mm, and push it upstream for 2.6.25.  Comments?
Sure, Thanks, taking either this patch or previous patch is ok to me.

Thanks,
Shaohua