Re: [PATCH v6 01/12] PCI/ACPI: Add D3cold Aux Power Limit_DSM method

Bjorn Helgaas
2026-01-14T20:24:23Z

On Tue, Jan 13, 2026 at 10:12:02PM +0530, Badal Nilawar wrote:
> From: Anshuman Gupta <anshuman.gupta@intel.com>
>
> Implement _DSM method 0Ah, as per PCI Firmware r3.3, sec 4.6.10,
> to request auxiliary power required by the device when in D3cold state.
>
> Implementation allows only a single device below the Downstream Port to
> request for Aux Power Limit under a given Root Port/Downstream Port
> because it does not track and aggregate requests from all child devices
> below the Downstream Port as required by PCI Firmware r3.3, sec 4.6.10.
>
> Co-developed-by: Badal Nilawar <badal.nilawar@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
> V4
> - Define enums for aux power request status (Rafael)
> - Add Co-developed-by and clean up Signed-off-by (Kappuswamy)
> (Bjorn)
> - Instead of root pci device pass the pci device of driver, traverse
> up the tree and discover _DSM
> - Allow only function 0 of device to request aux power
> - Allow retry_interval to be NULL
> - Refine commit message and function description
> V5(Rafael)
> - Remove function 0 check and allow first caller of the given
> downstream port (with _DSM) to requst aux power including
> different function
> - Squash Patch v5.02 to this patch
> - In the logic, to allow single device to req power, use linked list
> instead of adding extra variables to acpi device structure
> - return positive code for no main power removal to distinguish from
> aux power request granted
> ---
> drivers/pci/pci-acpi.c | 136 +++++++++++++++++++++++++++++++++++++++
> include/linux/pci-acpi.h | 9 +++
> 2 files changed, 145 insertions(+)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 9369377725fa..645d3005ba50 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1418,6 +1418,142 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
> ACPI_FREE(obj);
> }
>
> +static LIST_HEAD(acpi_aux_pwr_list);
> +static DEFINE_MUTEX(acpi_aux_pwr_lock);
> +
> +struct aux_pwr {
> + u32 aux_pwr_limit; /* aux power limit granted by platform firmware */
> + struct device *dev; /* device to which aux power is granted */

Shorten these to fit in 80 columns like the rest of the file.

> + struct acpi_device *adev; /* root port/downstream port */
> + struct list_head list;
> +};
> +
> +enum aux_pwr_req_status {
> + AUX_PWR_REQ_DENIED = 0x0,
> + AUX_PWR_REQ_GRANTED = 0x1,
> + AUX_PWR_REQ_NO_MAIN_PWR_REMOVAL = 0x2,
> + AUX_PWR_REQ_RETRY_INTERVAL_MIN = 0x11,
> + AUX_PWR_REQ_RETRY_INTERVAL_MAX = 0x1F

Use lower-case hex ("0x1f") like the rest of the file. Also below.

> +};
> +
> +/**
> + * pci_acpi_request_d3cold_aux_power - Request aux power while device is in D3cold

Shorten or wrap to fit in 80 columns.

> + * @dev: PCI device instance
> + * @requested_mw: Requested auxiliary power in milliwatts
> + * @retry_interval: Retry interval returned by platform to retry auxiliary
> + * power request
> + *
> + * Request auxilary power to platform firmware, via Root Port/Switch Downstream
> + * Port ACPI _DSM Function 0Ah, needed for the PCI device when it is in D3cold.
> + * Evaluate the _DSM and handle the response accordingly.

Drop this last sentence; I don't think it tells us anything new.

> + * For Multi-Function Devices, driver for Function 0 is required to report an
> + * aggregate power requirement covering all functions contained within the
> + * device.
> + *
> + * Return: 0 Aux power request granted
> + * 1 No main power removal
> + * errno on failure.
> + */
> +int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_mw,
> + u32 *retry_interval)
> +{
> + union acpi_object in_obj = {
> + .integer.type = ACPI_TYPE_INTEGER,
> + .integer.value = requested_mw,
> + };
> +
> + union acpi_object *out_obj;
> + int result;
> + struct pci_dev *bdev;
> + struct acpi_device *adev;
> + acpi_handle handle;
> + struct aux_pwr *apwr, *next;
> +
> + if (!dev)
> + return -EINVAL;

We talked about only allowing this for function 0:
https://lore.kernel.org/all/20250904183046.GA1267851@bhelgaas/

> + for (bdev = dev; bdev; bdev = pci_upstream_bridge(bdev)) {

I think bdev should start with pci_upstream_bridge(dev) as in the
other patch because this _DSM is only allowed in Downstream Ports.

> + handle = ACPI_HANDLE(&bdev->dev);
> + if (handle &&
> + acpi_check_dsm(handle, &pci_acpi_dsm_guid, 4,
> + 1 << DSM_PCI_D3COLD_AUX_POWER_LIMIT))
> + break;
> + }
> +
> + if (!bdev)
> + return -ENODEV;
> +
> + adev = ACPI_COMPANION(&bdev->dev);
> + if (!adev)
> + return -EINVAL;
> +
> + guard(mutex)(&acpi_aux_pwr_lock);
> + /* Check if aux power already granted to different device */
> + list_for_each_entry_safe(apwr, next, &acpi_aux_pwr_list, list) {
> + if (apwr->adev == adev && apwr->dev != &dev->dev) {
> + pci_info(to_pci_dev(apwr->dev),
> + "D3cold Aux Power request already granted: %u mW\n",
> + apwr->aux_pwr_limit);
> + return -EALREADY;
> + }
> + if (apwr->adev == adev && apwr->dev == &dev->dev) {
> + list_del(&apwr->list);
> + kfree(apwr);
> + break;
> + }
> + }
> +
> + out_obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&bdev->dev),
> + &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;
> + ACPI_FREE(out_obj);
> +
> + if (retry_interval)
> + *retry_interval = 0;
> +
> + switch (result) {
> + case AUX_PWR_REQ_DENIED:
> + pci_dbg(bdev, "D3cold Aux Power %u mW request denied\n",
> + requested_mw);
> + return -EINVAL;
> + case AUX_PWR_REQ_GRANTED:
> + pci_info(bdev, "D3cold Aux Power request granted: %u mW\n",
> + requested_mw);
> + apwr = kzalloc(sizeof(*apwr), GFP_KERNEL);
> + if (apwr) {
> + apwr->aux_pwr_limit = requested_mw;
> + apwr->dev = &dev->dev;
> + apwr->adev = adev;
> + INIT_LIST_HEAD(&apwr->list);
> + list_add(&acpi_aux_pwr_list,
> + &apwr->list);
> + }

I think we leak this allocation if the device is removed. I think the
list idea is more complicated than aggregating would be.

I think we could:

- add "aux_power_mw" in struct pci_dev

- walk the tree below bdev, accumulating aux_power_mw
(total_aux_power_mw += dev->aux_power_mw)

- pass "total_aux_power_mw + requested_mw" to the _DSM

- if successful, set dev->aux_power_mw = requested_mw

> + return 0;
> + case AUX_PWR_REQ_NO_MAIN_PWR_REMOVAL:
> + pci_info(bdev, "D3cold Aux Power: Main power won't be removed\n");
> + return 2;

Kernel-doc says we return 1 for this case.

> + case AUX_PWR_REQ_RETRY_INTERVAL_MIN ... AUX_PWR_REQ_RETRY_INTERVAL_MAX:
> + pci_info(bdev, "D3cold Aux Power request needs retry, interval: %u seconds\n",
> + result & 0xF);

Lower-case hex.

> + if (retry_interval) {
> + *retry_interval = result & 0xF;
> + return -EAGAIN;
> + }
> + return -EINVAL;

I think we should do:

case AUX_PWR_REQ_RETRY_INTERVAL_MIN ... AUX_PWR_REQ_RETRY_INTERVAL_MAX:
result &= 0xf;
pci_info(bdev, "... needs retry", result);
if (retry_interval)
*retry_interval = result;
return -EAGAIN;

I don't think it's useful to return different errors based on whether
the caller supplied a "retry_interval" pointer.

> + default:
> + pci_err(bdev, "D3cold Aux Power: Reserved or unsupported response: 0x%x\n",
> + result);
> + return -EINVAL;
> + }
> +}
> +EXPORT_SYMBOL_GPL(pci_acpi_request_d3cold_aux_power);