* [PATCH]libata-acpi: add ACPI _PSx method
@ 2007-09-14 2:45 Shaohua Li
2007-09-14 3:21 ` Jeff Garzik
2007-09-20 22:00 ` Jeff Garzik
0 siblings, 2 replies; 16+ messages in thread
From: Shaohua Li @ 2007-09-14 2:45 UTC (permalink / raw)
To: linux-ide, linux acpi; +Cc: jgarzik, Len Brown
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 */
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH]libata-acpi: add ACPI _PSx method
2007-09-14 3:21 ` Jeff Garzik
@ 2007-09-14 3:11 ` Shaohua Li
0 siblings, 0 replies; 16+ messages in thread
From: Shaohua Li @ 2007-09-14 3:11 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-ide, linux acpi, Len Brown
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH]libata-acpi: add ACPI _PSx method
2007-09-14 2:45 [PATCH]libata-acpi: add ACPI _PSx method Shaohua Li
@ 2007-09-14 3:21 ` Jeff Garzik
2007-09-14 3:11 ` Shaohua Li
2007-09-20 22:00 ` Jeff Garzik
1 sibling, 1 reply; 16+ messages in thread
From: Jeff Garzik @ 2007-09-14 3:21 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-ide, linux acpi, Len Brown
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH]libata-acpi: add ACPI _PSx method
2007-09-14 2:45 [PATCH]libata-acpi: add ACPI _PSx method Shaohua Li
2007-09-14 3:21 ` Jeff Garzik
@ 2007-09-20 22:00 ` Jeff Garzik
2007-09-21 1:16 ` Shaohua Li
1 sibling, 1 reply; 16+ messages in thread
From: Jeff Garzik @ 2007-09-20 22:00 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-ide, linux acpi, Len Brown, Andrew Morton, Tejun Heo
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH]libata-acpi: add ACPI _PSx method
2007-09-20 22:00 ` Jeff Garzik
@ 2007-09-21 1:16 ` Shaohua Li
2007-09-21 2:20 ` Tejun Heo
0 siblings, 1 reply; 16+ messages in thread
From: Shaohua Li @ 2007-09-21 1:16 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-ide, linux acpi, Len Brown, Andrew Morton, Tejun Heo
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 */
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH]libata-acpi: add ACPI _PSx method
2007-09-21 2:20 ` Tejun Heo
@ 2007-09-21 2:17 ` Shaohua Li
2007-10-31 2:27 ` Len Brown
0 siblings, 1 reply; 16+ messages in thread
From: Shaohua Li @ 2007-09-21 2:17 UTC (permalink / raw)
To: Tejun Heo; +Cc: Jeff Garzik, linux-ide, linux acpi, Len Brown, Andrew Morton
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 */
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH]libata-acpi: add ACPI _PSx method
2007-09-21 1:16 ` Shaohua Li
@ 2007-09-21 2:20 ` Tejun Heo
2007-09-21 2:17 ` Shaohua Li
0 siblings, 1 reply; 16+ messages in thread
From: Tejun Heo @ 2007-09-21 2:20 UTC (permalink / raw)
To: Shaohua Li; +Cc: Jeff Garzik, linux-ide, linux acpi, Len Brown, Andrew Morton
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH]libata-acpi: add ACPI _PSx method
2007-09-21 2:17 ` Shaohua Li
@ 2007-10-31 2:27 ` Len Brown
2007-10-31 13:26 ` Rafael J. Wysocki
0 siblings, 1 reply; 16+ messages in thread
From: Len Brown @ 2007-10-31 2:27 UTC (permalink / raw)
To: Shaohua Li; +Cc: Tejun Heo, Jeff Garzik, linux-ide, linux acpi, Andrew Morton
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 */
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH]libata-acpi: add ACPI _PSx method
2007-10-31 2:27 ` Len Brown
@ 2007-10-31 13:26 ` Rafael J. Wysocki
2007-11-01 2:50 ` Shaohua Li
0 siblings, 1 reply; 16+ messages in thread
From: Rafael J. Wysocki @ 2007-10-31 13:26 UTC (permalink / raw)
To: Len Brown
Cc: Shaohua Li, Tejun Heo, Jeff Garzik, linux-ide, linux acpi,
Andrew Morton
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH]libata-acpi: add ACPI _PSx method
2007-10-31 13:26 ` Rafael J. Wysocki
@ 2007-11-01 2:50 ` Shaohua Li
2007-11-01 10:04 ` Alan Cox
0 siblings, 1 reply; 16+ messages in thread
From: Shaohua Li @ 2007-11-01 2:50 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Len Brown, Tejun Heo, Jeff Garzik, linux-ide, linux acpi,
Andrew Morton
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 */
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH]libata-acpi: add ACPI _PSx method
2007-11-01 2:50 ` Shaohua Li
@ 2007-11-01 10:04 ` Alan Cox
2007-11-02 1:32 ` Shaohua Li
0 siblings, 1 reply; 16+ messages in thread
From: Alan Cox @ 2007-11-01 10:04 UTC (permalink / raw)
To: Shaohua Li
Cc: Rafael J. Wysocki, Len Brown, Tejun Heo, Jeff Garzik, linux-ide,
linux acpi, Andrew Morton
> + 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH]libata-acpi: add ACPI _PSx method
2007-11-01 10:04 ` Alan Cox
@ 2007-11-02 1:32 ` Shaohua Li
2007-11-02 11:16 ` Rafael J. Wysocki
2007-11-03 13:00 ` Jeff Garzik
0 siblings, 2 replies; 16+ messages in thread
From: Shaohua Li @ 2007-11-02 1:32 UTC (permalink / raw)
To: Alan Cox
Cc: Rafael J. Wysocki, Len Brown, Tejun Heo, Jeff Garzik, linux-ide,
linux acpi, Andrew Morton
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 */
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH]libata-acpi: add ACPI _PSx method
2007-11-02 1:32 ` Shaohua Li
@ 2007-11-02 11:16 ` Rafael J. Wysocki
2007-11-05 1:26 ` Shaohua Li
2007-11-03 13:00 ` Jeff Garzik
1 sibling, 1 reply; 16+ messages in thread
From: Rafael J. Wysocki @ 2007-11-02 11:16 UTC (permalink / raw)
To: Shaohua Li
Cc: Alan Cox, Len Brown, Tejun Heo, Jeff Garzik, linux-ide,
linux acpi, Andrew Morton
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH]libata-acpi: add ACPI _PSx method
2007-11-02 1:32 ` Shaohua Li
2007-11-02 11:16 ` Rafael J. Wysocki
@ 2007-11-03 13:00 ` Jeff Garzik
2007-11-05 1:12 ` Shaohua Li
1 sibling, 1 reply; 16+ messages in thread
From: Jeff Garzik @ 2007-11-03 13:00 UTC (permalink / raw)
To: Shaohua Li
Cc: Alan Cox, Rafael J. Wysocki, Len Brown, Tejun Heo, linux-ide,
linux acpi, Andrew Morton
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH]libata-acpi: add ACPI _PSx method
2007-11-03 13:00 ` Jeff Garzik
@ 2007-11-05 1:12 ` Shaohua Li
0 siblings, 0 replies; 16+ messages in thread
From: Shaohua Li @ 2007-11-05 1:12 UTC (permalink / raw)
To: Jeff Garzik
Cc: Alan Cox, Rafael J. Wysocki, Len Brown, Tejun Heo, linux-ide,
linux acpi, Andrew Morton
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH]libata-acpi: add ACPI _PSx method
2007-11-02 11:16 ` Rafael J. Wysocki
@ 2007-11-05 1:26 ` Shaohua Li
0 siblings, 0 replies; 16+ messages in thread
From: Shaohua Li @ 2007-11-05 1:26 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Alan Cox, Len Brown, Tejun Heo, Jeff Garzik, linux-ide,
linux acpi, Andrew Morton
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
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2007-11-05 1:32 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-09-14 2:45 [PATCH]libata-acpi: add ACPI _PSx method Shaohua Li
2007-09-14 3:21 ` Jeff Garzik
2007-09-14 3:11 ` Shaohua Li
2007-09-20 22:00 ` Jeff Garzik
2007-09-21 1:16 ` Shaohua Li
2007-09-21 2:20 ` Tejun Heo
2007-09-21 2:17 ` Shaohua Li
2007-10-31 2:27 ` Len Brown
2007-10-31 13:26 ` Rafael J. Wysocki
2007-11-01 2:50 ` Shaohua Li
2007-11-01 10:04 ` Alan Cox
2007-11-02 1:32 ` Shaohua Li
2007-11-02 11:16 ` Rafael J. Wysocki
2007-11-05 1:26 ` Shaohua Li
2007-11-03 13:00 ` Jeff Garzik
2007-11-05 1:12 ` Shaohua Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).