Re: [PATCH 2/4] PCI: Query platform firmware for device power state

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thursday, September 15, 2016 12:58:57 AM Lukas Wunner wrote:
> On Wed, Sep 14, 2016 at 06:47:40PM +0200, Rafael J. Wysocki wrote:
> > 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.
> 
> The patch only trusts the platform firmware to report D3cold correctly.
> If it reports anything else the patch reads the PMCSR.
> 
> Thus if _PSC returns a bogus D0 status, worst that can happen is that we
> incorrectly assume D3hot while the device is actually in D3cold.  Which
> is still kind of a bummer.
> 
> We've got this handy pci_device_is_present() function which reads the
> vendor ID and returns false if it's "all ones" (which is what we'd get
> in D3cold).
> 
> What if we ask the platform firmware first.  If it says D3cold, assume
> that's true.  In all other cases, call pci_device_is_present().  If
> that returns false, assume D3cold.  Otherwise read the PMCSR.

Sounds reasonable.  We already use pci_device_is_present() for a similar
purpose in at least one place IIRC.

> That would also work for devices that are suspended to D3cold in a
> nonstandard way, such as Thunderbolt.  Having a function that updates
> the current_state in a robust way and correctly discerns D3cold and
> D3hot would be really useful I think.
> 
> Thanks a lot for sharing these ACPI intricacies, it wasn't clear to me
> in how far we can trust the platform firmware.

No problem.  Unfortunately, it often takes me some time to recall those
things.

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



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux