On Wednesday, September 14, 2016 03:01:33 PM Rafael J. Wysocki wrote:
> On Wednesday, September 14, 2016 12:50:24 PM Lukas Wunner wrote:
> > On Wed, Sep 14, 2016 at 02:21:30AM +0200, Rafael J. Wysocki wrote:
> > > On Wednesday, August 31, 2016 08:15:18 AM Lukas Wunner wrote:
> > > > Usually the most accurate way to determine a PCI device's power state is
> > > > to read its PM Control & Status Register. There are two cases however
> > > > when this is not an option: If the device doesn't have the PM
> > > > capability at all, or if it is in D3cold.
> > > >
> > > > In D3cold, reading from config space typically results in a fabricated
> > > > "all ones" response. But in D3hot, the two bits representing the power
> > > > state in the PMCSR are *also* set to 1. Thus D3hot and D3cold are not
> > > > discernible by just reading the PMCSR.
> > > >
> > > > A (supposedly) reliable way to detect D3cold is to query the platform
> > > > firmware for its opinion on the device's power state. To this end,
> > > > add a ->get_power callback to struct pci_platform_pm_ops, and an
> > > > implementation to acpi_pci_platform_pm. (The only pci_platform_pm_ops
> > > > existing so far).
> > > >
> > > > Amend pci_update_current_state() to query the platform firmware before
> > > > reading the PMCSR.
> > > >
> > > > Cc: Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>
> > > > Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> > > > ---
> > > > drivers/pci/pci-acpi.c | 23 +++++++++++++++++++++++
> > > > drivers/pci/pci.c | 21 ++++++++++++++++-----
> > > > drivers/pci/pci.h | 3 +++
> > > > 3 files changed, 42 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > > > index 9a033e8..89f2707 100644
> > > > --- a/drivers/pci/pci-acpi.c
> > > > +++ b/drivers/pci/pci-acpi.c
> > > > @@ -452,6 +452,28 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> > > > return error;
> > > > }
> > > >
> > > > +static pci_power_t acpi_pci_get_power_state(struct pci_dev *dev)
> > > > +{
> > > > + struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
> > > > + static const pci_power_t state_conv[] = {
> > > > + [ACPI_STATE_D0] = PCI_D0,
> > > > + [ACPI_STATE_D1] = PCI_D1,
> > > > + [ACPI_STATE_D2] = PCI_D2,
> > > > + [ACPI_STATE_D3_HOT] = PCI_D3hot,
> > > > + [ACPI_STATE_D3_COLD] = PCI_D3cold,
> > > > + };
> > > > + int state;
> > >
> > > ACPI_STATE_D3_HOT and ACPI_STATE_D3_COLD were introduced in ACPI 4.0. For
> > > systems predating that, ACPI_STATE_D3_HOT is the deepest state returned by
> > > acpi_device_get_power().
> >
> > Would it be possible to detect the ACPI spec version the platform
> > firmware conforms to, and amend acpi_device_get_power() to return
> > ACPI_STATE_D3_COLD if the device is in D3?
> >
> > Then we could avoid the unnecessary runtime resume after direct_complete
> > also for these older machines.
>
> Well, please see below.
>
> > Can the revision in the FADT (offset 8) be used as a proxy?
> > => E.g. the old Clevo B7130 has revision 3 in the FADT and uses
> > a _DSM and _PS3 to put the discrete GPU in D3cold:
> > https://github.com/Lekensteyn/acpi-stuff/tree/master/dsl/Clevo_B7130
> > => Whereas the newer Clevo P651RA has revision 5 in the FADT and uses
> > _PR3 to put the discrete GPU in D3cold:
> > https://github.com/Lekensteyn/acpi-stuff/tree/master/dsl/Clevo_P651RA
> >
> > However the FADT revision was already 4 in the ACPI 3.0 spec,
> > so we can only use it to discern ACPI 2.0 vs 3.0, not 3.0 vs 4.0,
> > which is what we'd actually want. And there's a comment in
> > acpica/tbfadt.c that "The FADT revision value is unreliable."
> >
> > Do you know of a better way to discern ACPI 3.0 vs 4.0?
>
> I'm not sure if there is a reliable way to be honest.
>
> Also even if the FADT etc tells you something, there's no guarantee that AML
> actually follows that.
>
> In fact, whether or not acpi_device_get_power() will ever return D3cold
> for a device depends on whether or not _PR3 is there. If it's not there,
> the deepest state returned will be D3hot in any case.
>
> So I'm not sure how useful that is in the context of D3cold detection?
There is more to this.
In fact, we can't really trust _PSC too, because in some ACPI tables it is
implemented to always return 0 (seriously).
For this reason, I think the way to go would be to use something like
acpi_device_update_power() to ensure that the device really is in the
state we think it is in.
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
