Re: [PATCH v4 01/11] PCI/ACPI: Add D3cold Aux Power Limit_DSM method

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, May 29, 2025 at 04:46:44PM +0530, Badal Nilawar wrote:
> From: Anshuman Gupta <anshuman.gupta@intel.com>
> 
> Implement _DSM method 0Ah according to PCI firmware specifications,
> section 4.6.10 Rev 3.3., to request auxilary power needed for the
> device when in D3Cold.

Cite as "PCI Firmware r3.3, sec 4.6.10"
> 
> Note that this implementation assumes only a single device below the
> Downstream Port will request for Aux Power Limit under a given
> Root Port because it does not track and aggregate requests
> from all child devices below the Downstream Port as required
> by Section 4.6.10 Rev 3.3.

Wrap to fill 75 columns.  Update citation as above.

> One possible mitigation would be only allowing only first PCIe
> Non-Bridge Endpoint Function 0 driver to call_DSM method 0Ah.

I don't quite understand how this mitigation would address the lack of
aggregation or how the mitigation could be enforced.  Maybe just drop
this?

s/call_DSM/call _DSM/

> Signed-off-by: Varun Gupta <varun.gupta@intel.com>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
> V2(Bjorn/Rafael):
>   - Call acpi_dsm_check() to find method 0Ah supported
>   - Return retry interval to caller
> V3(Kuppuswamy)
>   - Add NULL check for retry interval
> ---
>  drivers/pci/pci-acpi.c   | 87 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-acpi.h |  8 ++++
>  2 files changed, 95 insertions(+)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index af370628e583..87f30910a5f1 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1421,6 +1421,93 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
>  	ACPI_FREE(obj);
>  }
>  
> +/**
> + * pci_acpi_request_d3cold_aux_power - Request aux power while device is in D3Cold
> + * @dev: PCI device instance
> + * @requested_power: Requested auxiliary power in milliwatts
> + * @retry_interval: Retry interval returned by platform to retry auxiliary
> + *                  power request
> + *
> + * This function sends a request to the host BIOS via root port ACPI _DSM Function 0Ah
> + * for the auxiliary power needed by the PCI device when it is in D3Cold.
> + * It checks and evaluates the _DSM (Device Specific Method) to request the auxiliary
> + * power and handles the response accordingly.

s/This functions sends a request/Request auxiliary power .../ (imperative mood)
s/the host BIOS/platform firmware/ (non-x86 firmware may not be BIOS)
s/D3Cold/D3cold/ (twice) to match other usage
s/root port/Root Port/ to match spec usage
s/It checks and evaluates the/Evaluate the/
s/and handles/and handle/

It's not quite clear from this description, but the code assumes this
is called for a Root Port.  I don't think that's quite the right
approach because the spec only says this _DSM function is implemented
in the scope of a Downstream Port.  That *could* be a Root Port, but
it doesn't have to be; it could be a Switch Downstream Port.

The caller shouldn't have to traverse up the tree, checking whether
this _DSM function is implemented at each level.  So I think the
driver should call this with *its* device, and
pci_acpi_request_d3cold_aux_power() should walk up the tree looking
for this _DSM function.

> + * This function shall be only called by 1st non-bridge Endpoint driver
> + * on Function 0. For a Multi-Function Device, the driver for Function 0 is
> + * required to report an aggregate power requirement covering all
> + * functions contained within the device.

This last paragraph covers two separate issues:

  1) Sec 4.6.10 requires driver for function 0 to aggregate power
     requirements for all functions in a multi-function device.  We
     can't enforce that the driver, e.g., xe, does this, but it seems
     like this is a hint that we should only evaluate this _DSM for
     function 0 of a multi-function device.  So it seems like we
     should return an error if called for a function other than 0.

  2) The "Location" part of sec 4.6.10 has a separate restriction
     that system software, i.e., the PCI core, should aggregate
     requests from child devices, which we don't do so far.

     If drivers call pci_acpi_request_d3cold_aux_power() for their
     devices (not the Root Port), there might be a way to do this
     aggregation by tracking the sum of all requests at the Downstream
     Port where we find this _DSM function implemented.  But I don't
     know whether it's worth trying to implement this now because it
     seems complicated.

I don't know what "1st non-bridge Endpoint driver" means.  How does a
driver writer know whether the driver qualifies?  Maybe just keep the
part about this only being supported for function 0 and note that we
don't support the aggregation across multiple devices?

Wrap this all to fit in 80 columns like the rest of the file.

> + * Return: Returns 0 on success and errno on failure.
> + */
> +int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
> +				      u32 *retry_interval)

Name the parameter "requested_power_mw" or even just "requested_mw" so
we know the units.  Also in header file.

> +{
> +	union acpi_object in_obj = {
> +		.integer.type = ACPI_TYPE_INTEGER,
> +		.integer.value = requested_power,
> +	};
> +
> +	union acpi_object *out_obj;
> +	acpi_handle handle;
> +	int result, ret = -EINVAL;
> +
> +	if (!dev || !retry_interval)
> +		return -EINVAL;

I think it's reasonable to allow retry_interval to be NULL if the
caller doesn't want to bother with retries.

> +	handle = ACPI_HANDLE(&dev->dev);
> +	if (!handle)
> +		return -EINVAL;
> +
> +	if (!acpi_check_dsm(handle, &pci_acpi_dsm_guid, 4, 1 << DSM_PCI_D3COLD_AUX_POWER_LIMIT)) {
> +		pci_dbg(dev, "ACPI _DSM 0%Xh not supported\n", DSM_PCI_D3COLD_AUX_POWER_LIMIT);
> +		return -ENODEV;
> +	}
> +
> +	out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
> +					  DSM_PCI_D3COLD_AUX_POWER_LIMIT,
> +					  &in_obj, ACPI_TYPE_INTEGER);
> +	if (!out_obj)
> +		return -EINVAL;
> +
> +	result = out_obj->integer.value;
> +	if (retry_interval)
> +		*retry_interval = 0;
> +
> +	switch (result) {
> +	case 0x0:
> +		pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
> +			requested_power);
> +		break;
> +	case 0x1:
> +		pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
> +			 requested_power);
> +		ret = 0;
> +		break;
> +	case 0x2:
> +		pci_info(dev, "D3cold Aux Power: Main power won't be removed\n");
> +		ret = -EBUSY;
> +		break;
> +	default:
> +		if (result >= 0x11 && result <= 0x1F) {
> +			if (retry_interval) {
> +				*retry_interval = result & 0xF;
> +				pci_warn(dev, "D3cold Aux Power request needs retry interval: %u seconds\n",
> +					 *retry_interval);

Seems like pci_info() to me; the user can't do anything about this,
nothing is really wrong, and the message shouldn't prompt a bug
report.

> +				ret = -EAGAIN;
> +			}
> +		} else {
> +			pci_err(dev, "D3cold Aux Power: Reserved or unsupported response: 0x%x\n",
> +				result);
> +		}
> +		break;
> +	}
> +
> +	ACPI_FREE(out_obj);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pci_acpi_request_d3cold_aux_power);
> +
>  static void pci_acpi_set_external_facing(struct pci_dev *dev)
>  {
>  	u8 val;
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 078225b514d4..6079306ad754 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -121,6 +121,7 @@ extern const guid_t pci_acpi_dsm_guid;
>  #define DSM_PCI_DEVICE_NAME			0x07
>  #define DSM_PCI_POWER_ON_RESET_DELAY		0x08
>  #define DSM_PCI_DEVICE_READINESS_DURATIONS	0x09
> +#define DSM_PCI_D3COLD_AUX_POWER_LIMIT		0x0A
>  
>  #ifdef CONFIG_PCIE_EDR
>  void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
> @@ -132,10 +133,17 @@ static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
>  
>  int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_dev *));
>  void pci_acpi_clear_companion_lookup_hook(void);
> +int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
> +				      u32 *retry_interval);
>  
>  #else	/* CONFIG_ACPI */
>  static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> +static inline int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
> +						    u32 *retry_interval)

Wrap this to fit in 80 columns like the rest of the file.

> +{
> +	return -EOPNOTSUPP;
> +}
>  #endif	/* CONFIG_ACPI */
>  
>  #endif	/* _PCI_ACPI_H_ */
> -- 
> 2.34.1
>