Message-ID: <03e5d343-848c-02c7-2deb-917d1b93ce8c@amd.com>
Date:   Wed, 21 Jun 2023 17:52:52 -0500
From:   "Limonciello, Mario" <mario.limonciello@....com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     "Rafael J . Wysocki" <rafael@...nel.org>,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        Len Brown <lenb@...nel.org>, linux-acpi@...r.kernel.org
Subject: Re: [PATCH v4] PCI: Call _REG when transitioning D-states


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@....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@....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@...gle.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@...gle.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@....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@....com>
>      Signed-off-by: Bjorn Helgaas <bhelgaas@...gle.com>
>      Reviewed-by: Rafael J. Wysocki <rafael@...nel.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;
>   }
>   

Powered by blists - more mailing lists