Subject: Enable PNPACPI _PSx Support
(This is an updated patch of http://lkml.org/lkml/2008/11/23/211)
This patch sets the power of PnP ACPI devices to D0 when they
are activated and to D3 when they are disabled. The latter is
in correspondence with the ACPI 3.0 specification, whereas the
former is added in order to be able to power up a device after
it has been previously disabled (or when booting up a system).
(As a consequence, the patch makes the PnP ACPI code more ACPI
compliant.)
Section 6.2.2 of the ACPI Specification (at least versions 1.0b
and 3.0a) states: "Prior to running this control method [_DIS],
the OS[PM] will have already put the device in the D3 state."
Unfortunately, there is no clear statement as to when to put
a device in the D0 state. :-( Therefore, the patch executes the
method calls as _PS3/_DIS and _SRS/_PS0. What is clear: "If the
device is disabled, _SRS enables the device at the specified
resources." (From the ACPI 3.0a Specification.)
The patch fixes a problem with some IBM ThinkPads (at least the
600E and the 600X) where the serial ports have a dedicated
power source that needs to be brought up before the serial port
can be used. Without this patch, the serial port is enabled
but has no power. (In the past, the tpctl utility had to be
utilized to turn on the power, but support for this feature
stopped with version 5.9 as it did not support the more recent
kernel versions.)
No regressions were observed on hardware that does not require
this patch.
The patch is applied against 2.6.27.8 (vanilla).
Signed-off-by: Witold Szczeponik <[email protected]>
Index: linux/drivers/pnp/pnpacpi/core.c
===================================================================
--- linux.orig/drivers/pnp/pnpacpi/core.c
+++ linux/drivers/pnp/pnpacpi/core.c
@@ -98,17 +98,20 @@ static int pnpacpi_set_resources(struct
status = acpi_set_current_resources(handle, &buffer);
if (ACPI_FAILURE(status))
ret = -EINVAL;
+ else
+ acpi_bus_set_power(handle, ACPI_STATE_D0);
kfree(buffer.pointer);
return ret;
}
static int pnpacpi_disable_resources(struct pnp_dev *dev)
{
+ acpi_handle handle = dev->data;
acpi_status status;
/* acpi_unregister_gsi(pnp_irq(dev, 0)); */
- status = acpi_evaluate_object((acpi_handle) dev->data,
- "_DIS", NULL, NULL);
+ acpi_bus_set_power(handle, ACPI_STATE_D3);
+ status = acpi_evaluate_object(handle, "_DIS", NULL, NULL);
return ACPI_FAILURE(status) ? -ENODEV : 0;
}
On Monday 08 December 2008 7:49:10 pm Adam M Belay wrote:
> Hi Witold,
>
> All in all your patch looks good, especially in the context of
> resolving the Thinkpad 600X issue. I have some patches on the
> way that will add more complete power management support to the
> PNP stack and hopefully allow for runtime PM of these system
> devices, but I think this is a good short-term solution.
>
> One quick comment. acpi_bus_set_power() can fail, and
> pnpacpi_set_resources() needs to handle this gracefully. Please
> update the patch to include this.
Hi Adam,
Do you have any proposals for how pnpacpi_set_resources() should
handle failure? Witold started out with code to call acpi_bus_set_power()
only if the device is power-manageable, and he then passed the
return code up. I suggested letting acpi_bus_set_power() do
the "is-power-manageable" check. But then the caller would have
to distinguish the "device isn't power-manageable" error from
others, which is kind of ugly.
Bjorn
> Also, for the follow up to this patch, I'm wondering what sort
> of ACPI behavior we need to support D1 and D2 states. In PCI,
> disabling resource decoding (similar to _DIS) is only required
> for entering D3. The ACPI spec seems to be ambiguous with this
> issue. Len, any thoughts?
>
> Thanks,
> Adam
>
> Quoting Witold Szczeponik <[email protected]>:
> > Subject: Enable PNPACPI _PSx Support
> >
> >
> > (This is an updated patch of http://lkml.org/lkml/2008/11/23/211)
> >
> > This patch sets the power of PnP ACPI devices to D0 when they
> > are activated and to D3 when they are disabled. The latter is
> > in correspondence with the ACPI 3.0 specification, whereas the
> > former is added in order to be able to power up a device after
> > it has been previously disabled (or when booting up a system).
> > (As a consequence, the patch makes the PnP ACPI code more ACPI
> > compliant.)
> >
> > Section 6.2.2 of the ACPI Specification (at least versions 1.0b
> > and 3.0a) states: "Prior to running this control method [_DIS],
> > the OS[PM] will have already put the device in the D3 state."
> > Unfortunately, there is no clear statement as to when to put
> > a device in the D0 state. :-( Therefore, the patch executes the
> > method calls as _PS3/_DIS and _SRS/_PS0. What is clear: "If the
> > device is disabled, _SRS enables the device at the specified
> > resources." (From the ACPI 3.0a Specification.)
> >
> > The patch fixes a problem with some IBM ThinkPads (at least the
> > 600E and the 600X) where the serial ports have a dedicated
> > power source that needs to be brought up before the serial port
> > can be used. Without this patch, the serial port is enabled
> > but has no power. (In the past, the tpctl utility had to be
> > utilized to turn on the power, but support for this feature
> > stopped with version 5.9 as it did not support the more recent
> > kernel versions.)
> >
> > No regressions were observed on hardware that does not require
> > this patch.
> >
> > The patch is applied against 2.6.27.8 (vanilla).
> >
> >
> > Signed-off-by: Witold Szczeponik <[email protected]>
> >
> >
> > Index: linux/drivers/pnp/pnpacpi/core.c
> > ===================================================================
> > --- linux.orig/drivers/pnp/pnpacpi/core.c
> > +++ linux/drivers/pnp/pnpacpi/core.c
> > @@ -98,17 +98,20 @@ static int pnpacpi_set_resources(struct
> > status = acpi_set_current_resources(handle, &buffer);
> > if (ACPI_FAILURE(status))
> > ret = -EINVAL;
> > + else
> > + acpi_bus_set_power(handle, ACPI_STATE_D0);
> > kfree(buffer.pointer);
> > return ret;
> > }
> >
> > static int pnpacpi_disable_resources(struct pnp_dev *dev)
> > {
> > + acpi_handle handle = dev->data;
> > acpi_status status;
> >
> > /* acpi_unregister_gsi(pnp_irq(dev, 0)); */
> > - status = acpi_evaluate_object((acpi_handle) dev->data,
> > - "_DIS", NULL, NULL);
> > + acpi_bus_set_power(handle, ACPI_STATE_D3);
> > + status = acpi_evaluate_object(handle, "_DIS", NULL, NULL);
> > return ACPI_FAILURE(status) ? -ENODEV : 0;
> > }
Hi Witold,
All in all your patch looks good, especially in the context of
resolving the Thinkpad 600X issue. I have some patches on the
way that will add more complete power management support to the
PNP stack and hopefully allow for runtime PM of these system
devices, but I think this is a good short-term solution.
One quick comment. acpi_bus_set_power() can fail, and
pnpacpi_set_resources() needs to handle this gracefully. Please
update the patch to include this.
Also, for the follow up to this patch, I'm wondering what sort
of ACPI behavior we need to support D1 and D2 states. In PCI,
disabling resource decoding (similar to _DIS) is only required
for entering D3. The ACPI spec seems to be ambiguous with this
issue. Len, any thoughts?
Thanks,
Adam
Quoting Witold Szczeponik <[email protected]>:
> Subject: Enable PNPACPI _PSx Support
>
>
> (This is an updated patch of http://lkml.org/lkml/2008/11/23/211)
>
> This patch sets the power of PnP ACPI devices to D0 when they
> are activated and to D3 when they are disabled. The latter is
> in correspondence with the ACPI 3.0 specification, whereas the
> former is added in order to be able to power up a device after
> it has been previously disabled (or when booting up a system).
> (As a consequence, the patch makes the PnP ACPI code more ACPI
> compliant.)
>
> Section 6.2.2 of the ACPI Specification (at least versions 1.0b
> and 3.0a) states: "Prior to running this control method [_DIS],
> the OS[PM] will have already put the device in the D3 state."
> Unfortunately, there is no clear statement as to when to put
> a device in the D0 state. :-( Therefore, the patch executes the
> method calls as _PS3/_DIS and _SRS/_PS0. What is clear: "If the
> device is disabled, _SRS enables the device at the specified
> resources." (From the ACPI 3.0a Specification.)
>
> The patch fixes a problem with some IBM ThinkPads (at least the
> 600E and the 600X) where the serial ports have a dedicated
> power source that needs to be brought up before the serial port
> can be used. Without this patch, the serial port is enabled
> but has no power. (In the past, the tpctl utility had to be
> utilized to turn on the power, but support for this feature
> stopped with version 5.9 as it did not support the more recent
> kernel versions.)
>
> No regressions were observed on hardware that does not require
> this patch.
>
> The patch is applied against 2.6.27.8 (vanilla).
>
>
> Signed-off-by: Witold Szczeponik <[email protected]>
>
>
> Index: linux/drivers/pnp/pnpacpi/core.c
> ===================================================================
> --- linux.orig/drivers/pnp/pnpacpi/core.c
> +++ linux/drivers/pnp/pnpacpi/core.c
> @@ -98,17 +98,20 @@ static int pnpacpi_set_resources(struct
> status = acpi_set_current_resources(handle, &buffer);
> if (ACPI_FAILURE(status))
> ret = -EINVAL;
> + else
> + acpi_bus_set_power(handle, ACPI_STATE_D0);
> kfree(buffer.pointer);
> return ret;
> }
>
> static int pnpacpi_disable_resources(struct pnp_dev *dev)
> {
> + acpi_handle handle = dev->data;
> acpi_status status;
>
> /* acpi_unregister_gsi(pnp_irq(dev, 0)); */
> - status = acpi_evaluate_object((acpi_handle) dev->data,
> - "_DIS", NULL, NULL);
> + acpi_bus_set_power(handle, ACPI_STATE_D3);
> + status = acpi_evaluate_object(handle, "_DIS", NULL, NULL);
> return ACPI_FAILURE(status) ? -ENODEV : 0;
> }
>
>
Quoting Bjorn Helgaas <[email protected]>:
> Hi Adam,
>
> Do you have any proposals for how pnpacpi_set_resources() should
> handle failure? Witold started out with code to call acpi_bus_set_power()
> only if the device is power-manageable, and he then passed the
> return code up. I suggested letting acpi_bus_set_power() do
> the "is-power-manageable" check. But then the caller would have
> to distinguish the "device isn't power-manageable" error from
> others, which is kind of ugly.
>
> Bjorn
Hi Bjorn,
How about one of these options:
1.) Modify acpi_bus_set_power() to return success if the device is not power
manageable but the request is for D0. After all, any device that doesn't
support PM can be assumed to be in D0. (Or maybe more correctly the state
of its parent?)
2.) Call acpi_bus_get_power() and check if the state is not already D0 before
asking acpi_bus_set_power() for the transition. If it is any other
state, then
of the device is power manageable.
Thanks,
Adam
On Mon, 08 Dec 2008, Witold Szczeponik wrote:
> This patch sets the power of PnP ACPI devices to D0 when they
> are activated and to D3 when they are disabled. The latter is
This will likely fix my T43 to power down USB during STR. THANKS!!
Now, I just need to check if the ACPI BIOS is smart enough to not do
it when they are configured as wake devices [in the BIOS].
Although I do wonder WTF we don't do that on our USB UHCI/EHCI kernel
drivers, that ACPI needs to step in to fix it. If it is not a wake
device, why are we leaving it powered up? It is bad enough we don't
have any sort of proper USB power control, but to leave the entire USB
subsystem powered up without reason?!
Vista not only powers down the USB HCIs and ports on STR, it also
powers down ports when you "safely remove the USB device", and seems
to leave the port powered down until you insert a device (or remove
the device already in there, whatever).
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
Am Mittwoch, 10. Dezember 2008 10:53:15 schrieb Henrique de Moraes Holschuh:
> Although I do wonder WTF we don't do that on our USB UHCI/EHCI kernel
> drivers, that ACPI needs to step in to fix it. ?If it is not a wake
EHCI/UHCI have no way to power down host controllers.
It needs external means, namely ACPI.
> device, why are we leaving it powered up? ?It is bad enough we don't
> have any sort of proper USB power control, but to leave the entire USB
> subsystem powered up without reason?!
We don't. If you unplug all devices, USB will power down as much as is
compatible with detecting new devices.
> Vista not only powers down the USB HCIs and ports on STR, it also
> powers down ports when you "safely remove the USB device", and seems
> to leave the port powered down until you insert a device (or remove
> the device already in there, whatever).
If you cut power to a port, you cannot detect a hotplugging. You can
suspend it, which Linux does do.
Regards
Oliver
Henrique de Moraes Holschuh wrote:
> On Mon, 08 Dec 2008, Witold Szczeponik wrote:
>> This patch sets the power of PnP ACPI devices to D0 when they
>> are activated and to D3 when they are disabled. The latter is
>
> This will likely fix my T43 to power down USB during STR. THANKS!!
> Now, I just need to check if the ACPI BIOS is smart enough to not do
> it when they are configured as wake devices [in the BIOS].
>
Most likely it won't. :-( My patch takes only care of devices that are
properly registered as PnP by ACPI: attached USB devices don't fall
under that category.
--- Witold
Adam M Belay wrote:
> Hi Witold,
>
> All in all your patch looks good, especially in the context of
> resolving the Thinkpad 600X issue. I have some patches on the
> way that will add more complete power management support to the
> PNP stack and hopefully allow for runtime PM of these system
> devices, but I think this is a good short-term solution.
>
> One quick comment. acpi_bus_set_power() can fail, and
> pnpacpi_set_resources() needs to handle this gracefully. Please
> update the patch to include this.
Hi Adam,
please take a look at my patch at http://lkml.org/lkml/2008/11/23/211,
a "precursor" to the current patch, which contains the required tests
(it, does, however, also remove a comment that Bjorn asked me to keep).
Would that be the type of tests you're thinking of?
--- Witold
>
> Also, for the follow up to this patch, I'm wondering what sort
> of ACPI behavior we need to support D1 and D2 states. In PCI,
> disabling resource decoding (similar to _DIS) is only required
> for entering D3. The ACPI spec seems to be ambiguous with this
> issue. Len, any thoughts?
>
> Thanks,
> Adam
>
> Quoting Witold Szczeponik <[email protected]>:
>
>> Subject: Enable PNPACPI _PSx Support
>>
>>
>> (This is an updated patch of http://lkml.org/lkml/2008/11/23/211)
>>
>> This patch sets the power of PnP ACPI devices to D0 when they
>> are activated and to D3 when they are disabled. The latter is
>> in correspondence with the ACPI 3.0 specification, whereas the
>> former is added in order to be able to power up a device after
>> it has been previously disabled (or when booting up a system).
>> (As a consequence, the patch makes the PnP ACPI code more ACPI
>> compliant.)
>>
>> Section 6.2.2 of the ACPI Specification (at least versions 1.0b
>> and 3.0a) states: "Prior to running this control method [_DIS],
>> the OS[PM] will have already put the device in the D3 state."
>> Unfortunately, there is no clear statement as to when to put
>> a device in the D0 state. :-( Therefore, the patch executes the
>> method calls as _PS3/_DIS and _SRS/_PS0. What is clear: "If the
>> device is disabled, _SRS enables the device at the specified
>> resources." (From the ACPI 3.0a Specification.)
>>
>> The patch fixes a problem with some IBM ThinkPads (at least the
>> 600E and the 600X) where the serial ports have a dedicated
>> power source that needs to be brought up before the serial port
>> can be used. Without this patch, the serial port is enabled
>> but has no power. (In the past, the tpctl utility had to be
>> utilized to turn on the power, but support for this feature
>> stopped with version 5.9 as it did not support the more recent
>> kernel versions.)
>>
>> No regressions were observed on hardware that does not require
>> this patch.
>>
>> The patch is applied against 2.6.27.8 (vanilla).
>>
>>
>> Signed-off-by: Witold Szczeponik <[email protected]>
>>
>>
>> Index: linux/drivers/pnp/pnpacpi/core.c
>> ===================================================================
>> --- linux.orig/drivers/pnp/pnpacpi/core.c
>> +++ linux/drivers/pnp/pnpacpi/core.c
>> @@ -98,17 +98,20 @@ static int pnpacpi_set_resources(struct
>> status = acpi_set_current_resources(handle, &buffer);
>> if (ACPI_FAILURE(status))
>> ret = -EINVAL;
>> + else
>> + acpi_bus_set_power(handle, ACPI_STATE_D0);
>> kfree(buffer.pointer);
>> return ret;
>> }
>>
>> static int pnpacpi_disable_resources(struct pnp_dev *dev)
>> {
>> + acpi_handle handle = dev->data;
>> acpi_status status;
>>
>> /* acpi_unregister_gsi(pnp_irq(dev, 0)); */
>> - status = acpi_evaluate_object((acpi_handle) dev->data,
>> - "_DIS", NULL, NULL);
>> + acpi_bus_set_power(handle, ACPI_STATE_D3);
>> + status = acpi_evaluate_object(handle, "_DIS", NULL, NULL);
>> return ACPI_FAILURE(status) ? -ENODEV : 0;
>> }
>>
>>
>
>
On Wed, 10 Dec 2008, Witold Szczeponik wrote:
> Henrique de Moraes Holschuh wrote:
>> On Mon, 08 Dec 2008, Witold Szczeponik wrote:
>>> This patch sets the power of PnP ACPI devices to D0 when they
>>> are activated and to D3 when they are disabled. The latter is
>>
>> This will likely fix my T43 to power down USB during STR. THANKS!!
>> Now, I just need to check if the ACPI BIOS is smart enough to not do
>> it when they are configured as wake devices [in the BIOS].
>>
>
> Most likely it won't. :-( My patch takes only care of devices that are
> properly registered as PnP by ACPI: attached USB devices don't fall
> under that category.
I do mean the UHCI and EHCI bridges, not USB devices hanging off them :)
I will do an in-depth study of the AML in question, I just noticed it is
coupled to some EC magic register, which might be doing stuff the USB
subsystem just can't do normally.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Wed, 10 Dec 2008, Oliver Neukum wrote:
> Am Mittwoch, 10. Dezember 2008 10:53:15 schrieb Henrique de Moraes Holschuh:
> > Although I do wonder WTF we don't do that on our USB UHCI/EHCI kernel
> > drivers, that ACPI needs to step in to fix it. ?If it is not a wake
>
> EHCI/UHCI have no way to power down host controllers.
> It needs external means, namely ACPI.
I see. So we can't just set the pci devices to D0 to cut power to the USB
ports and USB controller while in STR? Must be the EC stuff I just noticed
in the ACPI AML, then.
> > device, why are we leaving it powered up? ?It is bad enough we don't
> > have any sort of proper USB power control, but to leave the entire USB
> > subsystem powered up without reason?!
>
> We don't. If you unplug all devices, USB will power down as much as is
> compatible with detecting new devices.
Thanks for the update.
> > Vista not only powers down the USB HCIs and ports on STR, it also
> > powers down ports when you "safely remove the USB device", and seems
> > to leave the port powered down until you insert a device (or remove
> > the device already in there, whatever).
>
> If you cut power to a port, you cannot detect a hotplugging. You can
> suspend it, which Linux does do.
Hmm, probably it is just suspend, then (outside of STR. On STR, it *is*
somehow powering down the ports when no wake devices are there, but that
might be done using extra stuff, and not just the PCI functions of the Intel
ICH6 EHCI/UHCI controllers).
I guess our userspace is not doing the suspend on "safely remove" then?
Because at least here, it is not behaving as Windows does when I tell KDE to
"safely eject" (which probably ends up being a HAL request of some sort).
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
Hi Witold!
On Wed, 10 Dec 2008, Henrique de Moraes Holschuh wrote:
> On Wed, 10 Dec 2008, Witold Szczeponik wrote:
> > Henrique de Moraes Holschuh wrote:
> >> On Mon, 08 Dec 2008, Witold Szczeponik wrote:
> >>> This patch sets the power of PnP ACPI devices to D0 when they
> >>> are activated and to D3 when they are disabled. The latter is
> >>
> >> This will likely fix my T43 to power down USB during STR. THANKS!!
> >> Now, I just need to check if the ACPI BIOS is smart enough to not do
> >> it when they are configured as wake devices [in the BIOS].
> >>
> >
> > Most likely it won't. :-( My patch takes only care of devices that are
> > properly registered as PnP by ACPI: attached USB devices don't fall
> > under that category.
>
> I do mean the UHCI and EHCI bridges, not USB devices hanging off them :)
>
> I will do an in-depth study of the AML in question, I just noticed it is
> coupled to some EC magic register, which might be doing stuff the USB
> subsystem just can't do normally.
Bah, never mind. Major issue between the keyboard and the chair, on my
end... Got PR# and PS# confused. The ThinkPad ACPI DSDT in question has
the standard crap for wake device control, and PR# methods, but no PS#
methods.
The PR0..PR2 nodes for the UHCI and EHCI USB ports on the ThinkPad T43
mention a power resource object, and that power resource CAN power on/off
the USB ports (and that freaks out the UHCI and EHCI kernel drivers causing
over-current reports, but that was to be expected). The AML code tells the
EC to do some magic that probably tells a MOSFET somewhere in the mainboard
to stop supplying power to the ports.
And Windows XP (this is not a Vista BIOS and I never tried running Vista in
it...) is noticing that power resource (even without PS3 or PR3 nodes) and
calling that power resource _OFF handler to power down USB ports when it
doesn't need these devices for wakeup reasons, before entering STR.
Anyone has any clue on what part of the ACPI spec could result on that
behaviour? Because otherwise I will just bloody special-case it into
thinkpad-acpi's suspend and resume stuff. It will piss the USB subsystem
off if I can't arrange to do it on early resume and late suspend, however.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Wed, 10 Dec 2008, Henrique de Moraes Holschuh wrote:
> Bah, never mind. Major issue between the keyboard and the chair, on my
> end... Got PR# and PS# confused. The ThinkPad ACPI DSDT in question has
> the standard crap for wake device control, and PR# methods, but no PS#
> methods.
>
> The PR0..PR2 nodes for the UHCI and EHCI USB ports on the ThinkPad T43
> mention a power resource object, and that power resource CAN power on/off
> the USB ports (and that freaks out the UHCI and EHCI kernel drivers causing
> over-current reports, but that was to be expected). The AML code tells the
> EC to do some magic that probably tells a MOSFET somewhere in the mainboard
> to stop supplying power to the ports.
>
> And Windows XP (this is not a Vista BIOS and I never tried running Vista in
> it...) is noticing that power resource (even without PS3 or PR3 nodes) and
> calling that power resource _OFF handler to power down USB ports when it
> doesn't need these devices for wakeup reasons, before entering STR.
>
> Anyone has any clue on what part of the ACPI spec could result on that
> behaviour? Because otherwise I will just bloody special-case it into
> thinkpad-acpi's suspend and resume stuff. It will piss the USB subsystem
> off if I can't arrange to do it on early resume and late suspend, however.
Found it. ACPI 3.0b, page 238, 7.2 Device Power Management Objects.
"For OSPM to put the device in the D3 state, the following must occur:
All Power Resources no longer referenced by any device in the system must
be in the OFF state"
If you want to put *any* particular ACPI device into D3, you turn off
*every* unused power resource, be them related or not to that device...
Windows XP seems to do it, and powers down the USB ports. None of them are
set for wake-from-S3, they all go to D3 and no references to the power
resources are left. We probably should be doing the same.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh