Re: [PATCH] PNPACPI: Enable Power Management

[Bjorn Helgaas <bjorn.helgaas@hp.com>]

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