Re: [PATCH] PNPACPI: Enable Power Support

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

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 <Witold.Szczeponik@gmx.net>:
> > 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 <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,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;
> >  }