On Tue, Jun 20, 2023 at 4:23 PM Mario Limonciello <mario.limonciello@xxxxxxx> 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. > > The spec explains that _REG should be executed to indicate whether a > given region can be accessed. > > "Once _REG has been executed for a particular operation region, indicating > that the operation region handler is ready, a control method can > access fields in the operation region. Conversely, control methods > must not access fields in operation regions when _REG method execution > has not indicated that the operation region handler is ready." > > An example included in the spec demonstrates calling _REG when devices are > turned off: "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.". > > It is reported that ASMedia PCIe GPIO controllers fail functional tests > after the system has 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 part of the other AML called for the device. > > To fix this issue, call acpi_evaluate_reg() when devices are > transitioning to D3cold or D0. > > 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> > --- > v3->v4: > * Only enable config space access for D0 state instead of D1/D2 states. > * Only disallow config space access for D3cold state. I would ACPI to the patch subject, like "PCI: ACPI: Call ...", but that can be done when applying the patch, and it is the only comment I have to this version, so Reviewed-by: Rafael J. Wysocki <rafael@xxxxxxxxxx> > v2->v3: > * Move call site > * Rename static function to better describe behavior > * call _REG with disconnect /before/ going into D3 > * call _REG with connect /after/ going into D0/D1/D2 > * Update commit message > 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 > --- > drivers/pci/pci-acpi.c | 44 ++++++++++++++++++++++++++++-------------- > 1 file changed, 29 insertions(+), 15 deletions(-) > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index 052a611081ec..182cac535250 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -1043,6 +1043,16 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev) > return false; > } > > +static void acpi_pci_config_space_access(struct pci_dev *dev, bool enable) > +{ > + int val = enable ? ACPI_REG_CONNECT : ACPI_REG_DISCONNECT; > + int ret = acpi_evaluate_reg(ACPI_HANDLE(&dev->dev), > + ACPI_ADR_SPACE_PCI_CONFIG, val); > + if (ret) > + pci_dbg(dev, "ACPI _REG %s evaluation failed (%d)\n", > + enable ? "connect" : "disconnect", ret); > +} > + > int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) > { > struct acpi_device *adev = ACPI_COMPANION(&dev->dev); > @@ -1053,32 +1063,36 @@ int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) > [PCI_D3hot] = ACPI_STATE_D3_HOT, > [PCI_D3cold] = ACPI_STATE_D3_COLD, > }; > - int error = -EINVAL; > + int ret; > > /* If the ACPI device has _EJ0, ignore the device */ > if (!adev || acpi_has_method(adev->handle, "_EJ0")) > return -ENODEV; > > switch (state) { > + case PCI_POWER_ERROR: > + case PCI_UNKNOWN: > + return -EINVAL; > case PCI_D3cold: > if (dev_pm_qos_flags(&dev->dev, PM_QOS_FLAG_NO_POWER_OFF) == > - PM_QOS_FLAGS_ALL) { > - error = -EBUSY; > - break; > - } > - fallthrough; > - case PCI_D0: > - case PCI_D1: > - case PCI_D2: > - case PCI_D3hot: > - error = acpi_device_set_power(adev, state_conv[state]); > + PM_QOS_FLAGS_ALL) > + return -EBUSY; > + /* Notify AML lack of PCI config space availability */ > + acpi_pci_config_space_access(dev, false); > + break; > } > > - if (!error) > - pci_dbg(dev, "power state changed by ACPI to %s\n", > - acpi_power_state_string(adev->power.state)); > + ret = acpi_device_set_power(adev, state_conv[state]); > + if (ret) > + return ret; > + pci_dbg(dev, "power state changed by ACPI to %s\n", > + acpi_power_state_string(adev->power.state)); > > - return error; > + /* Notify AML of PCI config space availability */ > + if (state == PCI_D0) > + acpi_pci_config_space_access(dev, true); > + > + return 0; > } > > pci_power_t acpi_pci_get_power_state(struct pci_dev *dev) > -- > 2.34.1 >
