Re: [PATCH v2] PCI: Call _REG when saving/restoring PCI state

[Rafael J. Wysocki]

On Tue, Jun 6, 2023 at 10:26 PM Limonciello, Mario
<mario.limonciello@xxxxxxx> wrote:
>
>
> On 6/6/2023 2:58 PM, Bjorn Helgaas wrote:
> > On Tue, Jun 06, 2023 at 02:40:45PM -0500, Limonciello, Mario wrote:
> >> On 6/6/2023 2:23 PM, Bjorn Helgaas wrote:
> >>> On Tue, Jun 06, 2023 at 11:23:21AM -0500, Mario Limonciello wrote:
> >>>> ASMedia PCIe GPIO controllers fail functional tests after returning from
> >>>> suspend (S3 or s2idle). This is because the BIOS checks whether the
> >>>> OSPM has called the `_REG` method to determine whether it can interact with
> >>>> the OperationRegion assigned to the device.
> >>>>
> >>>> As described in 6.5.4 in the APCI spec, `_REG` is used to inform the AML
> >>>> code on the availability of an operation region.
> >>>>
> >>>> To fix this issue, call acpi_evaluate_reg() when saving and restoring the
> >>>> state of PCI devices.
> >>>>
> >>>> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#reg-region
> >>>> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> >>>> ---
> >>>> v1->v2:
> >>>>    * Handle case of no CONFIG_ACPI
> >>>>    * Rename function
> >>>>    * Update commit message
> >>>>    * Move ACPI calling code into pci-acpi.c instead
> >>>>    * Cite the ACPI spec
> >>> Thanks for the spec reference (s/APCI/ACPI/ and add the revision if
> >>> you rev this (r6.5 is the latest, AFAIK) if you rev this).
> >>>
> >>> I don't see text in that section that connects S3 with _REG.  If it's
> >>> there, you might have to quote the relevant sentence or two in the
> >>> commit log.
> >> I don't think there is anything the spec connecting this
> >> with S3.  At least from my perspective S3 is the reason
> >> this was exposed but there is a deficiency that exists
> >> that _REG is not being called by Linux.
> >>
> >> I intend to re-word the commit message something to the
> >> effect of explaining what _REG does and why _REG should be
> >> called, along with citations.
> >>
> >> Then in another paragraph "Fixing this resolves an issue ...".
> >>
> >>> You mentioned _REG being sort of a mutex to synchronize OSPM vs
> >>> platform access; if there's spec language to that effect, let's cite
> >>> it.
> >> That sentence I included was cited from the spec.
> > If it's necessary to justify the commit, include the citation in the
> > commit log.
> >
> >>> Ideally we should have been able to read the PCI and ACPI specs and
> >>> implement this without tripping over problem on this particular
> >>> hardware.  I'm looking for the text that enables that "clean-room"
> >>> implementation.  If the spec doesn't have that text, it's either a
> >>> hole in the spec or a BIOS defect that depends on something the spec
> >>> doesn't require.
> >> IMO both the spec and BIOS are correct, it's a Linux
> >> issue that _REG wasn't used.
> > What tells Linux that _REG needs to be used here?  If there's nothing
> > that tells Linux to use _REG here, I claim it's a BIOS defect.  I'm
> > happy to be convinced otherwise; the way to convince me is to point to
> > the spec.
>  From the spec it says "control methods must assume
> all operation regions are inaccessible until the
> _REG(RegionSpace, 1) method is executed"
>
> It also points out the opposite: "Conversely,
> control methods must not access fields in
> operation regions when _REG method execution
> has not indicated that the operation region
> handler is ready."
>
> The ACPI spec doesn't refer to D3 in this context, but
> it does make an allusion to power off in an example case.
>
> "Also, when the host controller or bridge controller
> is turned off or disabled, PCI Config Space Operation
> Regions for child devices are no longer available.
> As such, ETH0’s _REG method will be run when it is
> turned off and will again be run when PCI1 is
> turned off."
>
> >
> > If it's a BIOS defect, it's fine to work around it, but we need to
> > understand that, own up to it, and make the exact requirements very
> > clear.  Otherwise we're likely to break this in the future because
> > future developers and maintainers will rely on the specs.
>  From my discussions with BIOS developers, this is entirely
> intended behavior based on the _REG section in the spec.
> >>> Doing this in pci_save_state() still seems wrong to me.  For example,
> >>> e1000_probe() calls pci_save_state(), but this is not part of suspend.
> >>> IIUC, this patch will disconnect the opregion when we probe an e1000
> >>> NIC.  Is that what you intend?
> >> Thanks for pointing this one out.  I was narrowly focused
> >> on callers in PCI core.  This was a caller I wasn't
> >> aware of; I agree it doesn't make sense.
> >>
> >> I think pci_set_power_state() might be another good
> >> candidate to use.  What do you think of this?
> > I can't suggest a call site because (1) I'm not a power management
> > person, and (2) I don't think we have a clear statement of when it is
> > required.  This must be expressed in terms of PCI power state
> > transitions, or at least something discoverable from a pci_dev, not
> > "s2idle" or even "S3" because those are meaningless in the PCI
> > context.
> >
> > Bjorn
> Right; I'm with you on not putting it with a suspend
> transition.
>
> The spec indicates that control methods can't access
> the regions until _REG is called, so
> my leaning is to keep the call at init time, and
> then add another call for the D3 and D0 transitions
> which is why I think pci_set_power_state() is probably
> best.

Except that it is not used in all transitions; see the callers oif
pci_power_up() for example.

Technically, the config space should become accessible right after
acpi_pci_set_power_state() has transitioned the device into D0 and it
should be accessible still right before acpi_pci_set_power_state()
attempts to transition the device into a low-power state, so it looks
like _REG could be evaluated from there.