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

Limonciello, Mario
2023-06-21T22:53:10Z

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 got lost in the maze of users of ACPI_ADR_SPACE_PCI_CONFIG, but is
> it really true that we never call _REG for PCI config space at all
> today?
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.
>
> If so, I guess AML that uses ACPI_ADR_SPACE_PCI_CONFIG won't work
> until after we set the relevant device to D0?
The particular problem that that exposed this issue doesn't
happen until suspend/resume time, but yes I think this should
be called when setting the device to D0.
>
> Do we explicitly set devices to D0 during enumeration, e.g., somewhere
> in the pci_scan_device() path? If not, should we?
AFAICT it's happening for PCIe ports as part of:
pcie_portdrv_probe
->pcie_port_device_register
->->pci_enable_device
->->->pci_enable_device_flags
->->->->do_pci_enable_device
->->->->->pci_set_power_state(pci_dev, PCI_D0)
>
> If we don't set things to D0 during enumeration, it seems like this
> AML won't work until we suspend and resume the device.
>
> Separately, I propose a minor restructuring to avoid the need for
> mentioning PCI_POWER_ERROR and PCI_UNKNOWN. Checking for those means
> we need to look at the definitions to be sure we cover all cases, and
> it also doesn't solve the problem that a caller can pass undefined
> pci_power_t values that would index outside the state_conv[].
>
> Possible rework attached below. I also like the fact that it makes
> the _REG patch very simple and specific to _REG.

I like your rework as well.

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

>
>> 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@amd.com>
>> 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)
>
> commit 79d4fdf58711 ("PCI/PM: Validate acpi_pci_set_power_state() parameter")
> Author: Bjorn Helgaas <bhelgaas@google.com>
> Date: Wed Jun 21 16:36:12 2023 -0500
>
> PCI/PM: Validate acpi_pci_set_power_state() parameter
>
> Previously acpi_pci_set_power_state() assumed the requested power state was
> valid (PCI_D0 ... PCI_D3cold). If a caller supplied something else, we
> could index outside the state_conv[] array and pass junk to
> acpi_device_set_power().
>
> Validate the pci_power_t parameter and return -EINVAL if it's invalid.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 052a611081ec..bf545f719182 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1053,32 +1053,37 @@ 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 error;
>
> /* If the ACPI device has _EJ0, ignore the device */
> if (!adev || acpi_has_method(adev->handle, "_EJ0"))
> return -ENODEV;
>
> switch (state) {
> - 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]);
> + case PCI_D3cold:
> + break;
> + default:
> + return -EINVAL;
> }
>
> - if (!error)
> - pci_dbg(dev, "power state changed by ACPI to %s\n",
> - acpi_power_state_string(adev->power.state));
> + if (state == PCI_D3cold) {
> + if (dev_pm_qos_flags(&dev->dev, PM_QOS_FLAG_NO_POWER_OFF) ==
> + PM_QOS_FLAGS_ALL)
> + return -EBUSY;
> + }
>
> - return error;
> + error = acpi_device_set_power(adev, state_conv[state]);
> + if (error)
> + return error;
> +
> + pci_dbg(dev, "power state changed by ACPI to %s\n",
> + acpi_power_state_string(adev->power.state));
> +
> + return 0;
> }
>
> pci_power_t acpi_pci_get_power_state(struct pci_dev *dev)
> commit 746652bd0376 ("PCI/PM: Call _REG when transitioning D-states")
> Author: Mario Limonciello <mario.limonciello@amd.com>
> Date: Tue Jun 20 09:04:51 2023 -0500
>
> PCI/PM: Call _REG when transitioning D-states
>
> ACPI r6.5, sec 6.5.4, describes how AML is unable to access an
> OperationRegion unless _REG has been called to connect a handler:
>
> 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 any time, 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
> Link: https://lore.kernel.org/r/20230620140451.21007-1-mario.limonciello@amd.com
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>
>
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index bf545f719182..a05350a4e49c 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);
> @@ -1074,6 +1084,9 @@ int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> if (dev_pm_qos_flags(&dev->dev, PM_QOS_FLAG_NO_POWER_OFF) ==
> PM_QOS_FLAGS_ALL)
> return -EBUSY;
> +
> + /* Notify AML lack of PCI config space availability */
> + acpi_pci_config_space_access(dev, false);
> }
>
> error = acpi_device_set_power(adev, state_conv[state]);
> @@ -1083,6 +1096,15 @@ int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> pci_dbg(dev, "power state changed by ACPI to %s\n",
> acpi_power_state_string(adev->power.state));
>
> + /*
> + * Notify AML of PCI config space availability. Config space is
> + * accessible in all states except D3cold; the only transitions
> + * that change availability are transitions to D3cold and from
> + * D3cold to D0.
> + */
> + if (state == PCI_D0)
> + acpi_pci_config_space_access(dev, true);
> +
> return 0;
> }
>