[PATCH] PNPACPI: Enable Power Management

[PATCH] PNPACPI: Enable Power Management

[Witold Szczeponik]

Subject: Enable PNPACI Power Management

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

The patch fixes the 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.

No regressions were observed on hardware that does not require
this patch.

The patch is applied against 2.6.27.7 (vanilla).


Signed-off-by: Witold Szczeponik <Witold.Szczeponik@gmx.net>


Index: linux/drivers/pnp/pnpacpi/core.c
===================================================================
--- linux.orig/drivers/pnp/pnpacpi/core.c
+++ linux/drivers/pnp/pnpacpi/core.c
@@ -98,18 +98,24 @@ static int pnpacpi_set_resources(struct
  	status = acpi_set_current_resources(handle, &buffer);
  	if (ACPI_FAILURE(status))
  		ret = -EINVAL;
+	else if (acpi_bus_power_manageable(handle))
+		ret = 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;
+	int ret = 0;
  	acpi_status status;

-	/* acpi_unregister_gsi(pnp_irq(dev, 0)); */
-	status = acpi_evaluate_object((acpi_handle) dev->data,
-				      "_DIS", NULL, NULL);
-	return ACPI_FAILURE(status) ? -ENODEV : 0;
+	if (acpi_bus_power_manageable(handle))
+		ret = acpi_bus_set_power(handle, ACPI_STATE_D3);
+	status = acpi_evaluate_object(handle, "_DIS", NULL, NULL);
+	if (ACPI_FAILURE(status))
+		ret = -ENODEV;
+	return ret;
  }

  #ifdef CONFIG_ACPI_SLEEP

Re: [PATCH] PNPACPI: Enable Power Management

[Bjorn Helgaas]

On Sunday 23 November 2008 02:09:03 pm Witold Szczeponik wrote:
> Subject: Enable PNPACI Power Management

Hi Witold,

Thanks for your patch.  I'm glad somebody is paying attention to
PNP and power.  I CC'd Adam and Rafael because they care about this
area, too, but might not read everything on linux-acpi.

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

Do you know of anything that specifies the order of the _CRS/_PS0
and the _PS3/_DIS evaluation?  I don't know much about power
management, and I couldn't find anything obvious in the spec.
It seems plausible that we should run _CRS before turning on
the power, but I really don't know.

> The patch fixes the 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.

Is pnpacpi_set_resources() the only place that needs this change?
For active devices, we normally don't call pnpacpi_set_resources()
at all.  So I suppose on these ThinkPads, we exercise this path
because the serial ports are initially disabled?

> No regressions were observed on hardware that does not require
> this patch.
> 
> The patch is applied against 2.6.27.7 (vanilla).
> 
> 
> Signed-off-by: Witold Szczeponik <Witold.Szczeponik@gmx.net>
> 
> 
> Index: linux/drivers/pnp/pnpacpi/core.c
> ===================================================================
> --- linux.orig/drivers/pnp/pnpacpi/core.c
> +++ linux/drivers/pnp/pnpacpi/core.c
> @@ -98,18 +98,24 @@ static int pnpacpi_set_resources(struct
>   	status = acpi_set_current_resources(handle, &buffer);
>   	if (ACPI_FAILURE(status))
>   		ret = -EINVAL;
> +	else if (acpi_bus_power_manageable(handle))
> +		ret = acpi_bus_set_power(handle, ACPI_STATE_D0);

I don't really like testing acpi_bus_power_manageable() first.
I think we should just call acpi_bus_set_power() and let *it*
bail out if the device doesn't support it.

>   	kfree(buffer.pointer);
>   	return ret;
>   }
> 
>   static int pnpacpi_disable_resources(struct pnp_dev *dev)
>   {
> +	acpi_handle handle = dev->data;
> +	int ret = 0;
>   	acpi_status status;
> 
> -	/* acpi_unregister_gsi(pnp_irq(dev, 0)); */

Can you leave the "unregister_gsi" comment there, since it's not
related to your patch?  It's a reminder that we need to think about
how to handle interrupts when enabling/disabling devices.

> -	status = acpi_evaluate_object((acpi_handle) dev->data,
> -				      "_DIS", NULL, NULL);
> -	return ACPI_FAILURE(status) ? -ENODEV : 0;
> +	if (acpi_bus_power_manageable(handle))
> +		ret = acpi_bus_set_power(handle, ACPI_STATE_D3);
> +	status = acpi_evaluate_object(handle, "_DIS", NULL, NULL);
> +	if (ACPI_FAILURE(status))
> +		ret = -ENODEV;
> +	return ret;
>   }
> 
>   #ifdef CONFIG_ACPI_SLEEP

Re: [PATCH] PNPACPI: Enable Power Management

[Witold Szczeponik]

Bjorn Helgaas wrote:

> On Sunday 23 November 2008 02:09:03 pm Witold Szczeponik wrote:
>> Subject: Enable PNPACI Power Management
> 
> Hi Witold,

Hi Bjorn,

> 
> Thanks for your patch.  I'm glad somebody is paying attention to
> PNP and power.  I CC'd Adam and Rafael because they care about this
> area, too, but might not read everything on linux-acpi.

thanks, since this was my first patch submission, I wasn't sure
who to send the patch to...

> 

[patch description removed...]

> 
> Do you know of anything that specifies the order of the _CRS/_PS0
> and the _PS3/_DIS evaluation?  I don't know much about power
> management, and I couldn't find anything obvious in the spec.
> It seems plausible that we should run _CRS before turning on
> the power, but I really don't know.
> 

There's some info the ACPI Specification that says a device needs
to be put to D3 before _DIS is called (section 6.2.2) but there
is no clear statement as to when to put a device to D0... :-(

I think it should be _SRS/_PS0 and _PS3/_DIS.

[patch description removed...]

> 
> Is pnpacpi_set_resources() the only place that needs this change?
> For active devices, we normally don't call pnpacpi_set_resources()
> at all.  So I suppose on these ThinkPads, we exercise this path
> because the serial ports are initially disabled?
> 

I'm not sure.  I would guess that we need to put any device that is
enabled (either via _SRS or by default) into D0.  My patch does that
for the _SRS case but the generic ACPI code does not.  On my 600E,
the serial port has power when the machine is booted but has no power
once GRUB kicks in.  It remains in this state until the 8250-pnp module
gets loaded, where my patch enables it.

The ACPI Specification says for _SRS: the device is enabled after the 
method has returned.  There is no statement as to when to call _PS0...

>> No regressions were observed on hardware that does not require
>> this patch.
>>
>> The patch is applied against 2.6.27.7 (vanilla).
>>
>>
>> Signed-off-by: Witold Szczeponik <Witold.Szczeponik@gmx.net>
>>
>>
>> Index: linux/drivers/pnp/pnpacpi/core.c
>> ===================================================================
>> --- linux.orig/drivers/pnp/pnpacpi/core.c
>> +++ linux/drivers/pnp/pnpacpi/core.c
>> @@ -98,18 +98,24 @@ static int pnpacpi_set_resources(struct
>>   	status = acpi_set_current_resources(handle, &buffer);
>>   	if (ACPI_FAILURE(status))
>>   		ret = -EINVAL;
>> +	else if (acpi_bus_power_manageable(handle))
>> +		ret = acpi_bus_set_power(handle, ACPI_STATE_D0);
> 
> I don't really like testing acpi_bus_power_manageable() first.
> I think we should just call acpi_bus_set_power() and let *it*
> bail out if the device doesn't support it.

Fair enough. :-) Will remove the test.

> 
>>   	kfree(buffer.pointer);
>>   	return ret;
>>   }
>>
>>   static int pnpacpi_disable_resources(struct pnp_dev *dev)
>>   {
>> +	acpi_handle handle = dev->data;
>> +	int ret = 0;
>>   	acpi_status status;
>>
>> -	/* acpi_unregister_gsi(pnp_irq(dev, 0)); */
> 
> Can you leave the "unregister_gsi" comment there, since it's not
> related to your patch?  It's a reminder that we need to think about
> how to handle interrupts when enabling/disabling devices.

I'd rather remove the comment as it is misleading, IMHO.  This call
should be made by a driver.  After all, the PNPACPI core does not
register any IRQs, either.  Otherwise, we need to think about
"pnp_irq(dev, 1)", too.  Either way, with my patch there should be
no IRQ handling possible.

> 
>> -	status = acpi_evaluate_object((acpi_handle) dev->data,
>> -				      "_DIS", NULL, NULL);
>> -	return ACPI_FAILURE(status) ? -ENODEV : 0;
>> +	if (acpi_bus_power_manageable(handle))
>> +		ret = acpi_bus_set_power(handle, ACPI_STATE_D3);
>> +	status = acpi_evaluate_object(handle, "_DIS", NULL, NULL);
>> +	if (ACPI_FAILURE(status))
>> +		ret = -ENODEV;
>> +	return ret;
>>   }

Will remove the test, too.

>>
>>   #ifdef CONFIG_ACPI_SLEEP
> 

Re: [PATCH] PNPACPI: Enable Power Management

[Bjorn Helgaas]

On Sunday 07 December 2008 5:41:43 am Witold Szczeponik wrote:
> Bjorn Helgaas wrote:
> > Do you know of anything that specifies the order of the _CRS/_PS0
> > and the _PS3/_DIS evaluation?  I don't know much about power
> > management, and I couldn't find anything obvious in the spec.
> > It seems plausible that we should run _CRS before turning on
> > the power, but I really don't know.
>
> There's some info the ACPI Specification that says a device needs
> to be put to D3 before _DIS is called (section 6.2.2) but there
> is no clear statement as to when to put a device to D0... :-(
>
> I think it should be _SRS/_PS0 and _PS3/_DIS.

Thanks for the pointer.  That makes sense, and your proposed order
makes the enable/disable paths symmetric, so I think you're right.
Can you put that spec pointer in your changelog?

> > Is pnpacpi_set_resources() the only place that needs this change?
> > For active devices, we normally don't call pnpacpi_set_resources()
> > at all.  So I suppose on these ThinkPads, we exercise this path
> > because the serial ports are initially disabled?
>
> I'm not sure.  I would guess that we need to put any device that is
> enabled (either via _SRS or by default) into D0.  My patch does that
> for the _SRS case but the generic ACPI code does not.  On my 600E,
> the serial port has power when the machine is booted but has no power
> once GRUB kicks in.  It remains in this state until the 8250-pnp module
> gets loaded, where my patch enables it.

Interesting.  I'd be surprised if GRUB does anything with ACPI to
disable the port.  I don't know how you determine when the port has
power.  Maybe the power-up default state is powered, and the BIOS
turns it off before launching GRUB?

I wonder if _STA is influenced by the power state.  I would think
if _STA said a device was "enabled and decoding resources," that
would only make sense if the device were already in D0.  But let's
say we start with an active device, then run _PS3 and _DIS.  What
would _STA say in the interval between _PS3 and _DIS?

This is just idle curiousity on my part, I guess.  I just don't
know much about ACPI power states, and I'm afraid of getting in
trouble if we change too much.  The _SRS path is a little less
scary because it won't be exercised much (most devices are already
enabled, and we don't change their settings).

> The ACPI Specification says for _SRS: the device is enabled after the
> method has returned.  There is no statement as to when to call _PS0...

> >>   static int pnpacpi_disable_resources(struct pnp_dev *dev)
> >>   {
> >> +	acpi_handle handle = dev->data;
> >> +	int ret = 0;
> >>   	acpi_status status;
> >>
> >> -	/* acpi_unregister_gsi(pnp_irq(dev, 0)); */
> >
> > Can you leave the "unregister_gsi" comment there, since it's not
> > related to your patch?  It's a reminder that we need to think about
> > how to handle interrupts when enabling/disabling devices.
>
> I'd rather remove the comment as it is misleading, IMHO.  This call
> should be made by a driver.  After all, the PNPACPI core does not
> register any IRQs, either.  Otherwise, we need to think about
> "pnp_irq(dev, 1)", too.  Either way, with my patch there should be
> no IRQ handling possible.

The comment is logically unrelated to the rest of your patch, so I
would at least split it into a separate patch just on that grounds.

The PNP core *does* register IRQs: we call acpi_register_gsi()
in pnpacpi_parse_allocated_irqresource(), but there's no
corresponding unregister.  I think this asymmetry is a bug,
but I haven't looked at how to handle it yet.  PNP currently
does the registration "eagerly," when the device is discovered.
I think we should instead do the registration/unregistration
"lazily," when a driver claims the device, as PCI does.

I haven't changed this yet because there are some issues related to
pcibios_penalize_isa_irq() -- we don't know the IRQ to penalize until
we register the GSI.

Anyway, that's a long-winded explanation of why that stupid-looking
comment still has some value to me :-)

Bjorn

Re: [PATCH] PNPACPI: Enable Power Management

[Witold Szczeponik]

Bjorn Helgaas wrote:

[old stuff removed]

>> I think it should be _SRS/_PS0 and _PS3/_DIS.
> 
> Thanks for the pointer.  That makes sense, and your proposed order
> makes the enable/disable paths symmetric, so I think you're right.
> Can you put that spec pointer in your changelog?
> 

Will do.  I'll send out a new version of the patch in a separate note.

>>> Is pnpacpi_set_resources() the only place that needs this change?
>>> For active devices, we normally don't call pnpacpi_set_resources()
>>> at all.  So I suppose on these ThinkPads, we exercise this path
>>> because the serial ports are initially disabled?
>> I'm not sure.  I would guess that we need to put any device that is
>> enabled (either via _SRS or by default) into D0.  My patch does that
>> for the _SRS case but the generic ACPI code does not.  On my 600E,
>> the serial port has power when the machine is booted but has no power
>> once GRUB kicks in.  It remains in this state until the 8250-pnp module
>> gets loaded, where my patch enables it.
> 
> Interesting.  I'd be surprised if GRUB does anything with ACPI to
> disable the port.  I don't know how you determine when the port has
> power.  Maybe the power-up default state is powered, and the BIOS
> turns it off before launching GRUB?

Most likely you are correct assuming that the BIOS turns the devices
off before starting GRUB.  Need to investigate...

> 
> I wonder if _STA is influenced by the power state.  I would think
> if _STA said a device was "enabled and decoding resources," that
> would only make sense if the device were already in D0.  But let's
> say we start with an active device, then run _PS3 and _DIS.  What
> would _STA say in the interval between _PS3 and _DIS?

I did not check this, but a quick look at the 600E's DSDT revealed
that powering up a device and its _STA status are not correlated,
at least on this machine.

> 
> This is just idle curiousity on my part, I guess.  I just don't
> know much about ACPI power states, and I'm afraid of getting in
> trouble if we change too much.  The _SRS path is a little less
> scary because it won't be exercised much (most devices are already
> enabled, and we don't change their settings).

ACK.

> 

[parts of the patch removed]

>>>> -	/* acpi_unregister_gsi(pnp_irq(dev, 0)); */
>>> Can you leave the "unregister_gsi" comment there, since it's not
>>> related to your patch?  It's a reminder that we need to think about
>>> how to handle interrupts when enabling/disabling devices.
>> I'd rather remove the comment as it is misleading, IMHO.  This call
>> should be made by a driver.  After all, the PNPACPI core does not
>> register any IRQs, either.  Otherwise, we need to think about
>> "pnp_irq(dev, 1)", too.  Either way, with my patch there should be
>> no IRQ handling possible.
> 
> The comment is logically unrelated to the rest of your patch, so I
> would at least split it into a separate patch just on that grounds.

ACK.  But will not tackle this any time soon.

> 
> The PNP core *does* register IRQs: we call acpi_register_gsi()
> in pnpacpi_parse_allocated_irqresource(), but there's no
> corresponding unregister.  I think this asymmetry is a bug,
> but I haven't looked at how to handle it yet.  PNP currently
> does the registration "eagerly," when the device is discovered.
> I think we should instead do the registration/unregistration
> "lazily," when a driver claims the device, as PCI does.

I find this asymmetry weird, too.  Maybe we could register when
the device is enabled and unregister when it is disabled?

> 
> I haven't changed this yet because there are some issues related to
> pcibios_penalize_isa_irq() -- we don't know the IRQ to penalize until
> we register the GSI.
> 
> Anyway, that's a long-winded explanation of why that stupid-looking
> comment still has some value to me :-)

Now I understand.  I'm giving in! :-D

> 
> Bjorn
> 

--- Witold