Re: [PATCH v4] PCI: Call _REG when transitioning D-states

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Jun 21, 2023 at 05:52:52PM -0500, Limonciello, Mario wrote:
> 
> On 6/21/2023 5:28 PM, Bjorn Helgaas wrote:
> > On Tue, Jun 20, 2023 at 09:04:51AM -0500, Mario Limonciello wrote:
> > > Section 6.5.4 of the ACPI 6.4 spec describes how AML is unable to access
> > > an OperationRegion unless `_REG` has been called.
> > > 
> > > "The OS runs _REG control methods to inform AML code of a change in the
> > > availability of an operation region. When an operation region handler
> > > is unavailable, AML cannot access data fields in that region.
> > > (Operation region writes will be ignored and reads will return
> > > indeterminate data.)"
> > > 
> > > The PCI core does not call `_REG` at anytime, leading to the undefined
> > > behavior mentioned in the spec.

> I double checked a BIOS debug log which shows ACPI calls
> to confirm and didn't see a single _REG call for any device
> before this patch across a boot/suspend/resume cycle.

OK, thanks, I didn't see one either, which surprised me.

Based on the weird exception in sec 6.5.4:

  Since the [PCI_Config operation region on a PCI root bus containing
  a _BBN object] is permanently available, no _REG methods are
  required, nor will OSPM evaluate any _REG methods that appear in the
  same scope as the operation region declaration(s) of this type.

it seems like when we add a PCI host bridge, we should evaluate any
_REG in the host bridge scope if it does not include _BBN.

The example:

  It should be noted that PCI Config Space Operation Regions are ready
  as soon the host controller or bridge controller has been programmed
  with a bus number. PCI1’s _REG method would not be run until the
  PCI-PCI bridge has been properly configured.

suggests that when we set a PCI-PCI bridge's secondary bus number, we
should evaluate any _REG in its scope.

  At the same time, the OS will also run ETH0’s _REG method since its
  PCI Config Space would be also available. The OS will again run
  ETH0’s _REG method when the ETH0 device is started.

So evidently we should evaluate ETH0._REG once after setting PCI1's
secondary bus number, and again when "ETH0 is started."  I have no
idea what "started" means.

> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> For the new patch.

Thanks for taking a look at it!

I think we're missing some other _REG stuff, but it seems like what
this patch adds does match the per-endpoint power management requires,
so I applied these to pci/pm for v6.5.

Bjorn