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

Nilawar, Badal
2026-01-20T14:03:34Z

On 15-01-2026 01:54, Bjorn Helgaas wrote:
> 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.
Sure.
>
>> + 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.
Ok.
>
>> +};
>> +
>> +/**
>> + * pci_acpi_request_d3cold_aux_power - Request aux power while device is in D3cold
> Shorten or wrap to fit in 80 columns.
Ok
>
>> + * @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.
Ok
>
>> + * 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/

In rev5 there was suggestion from Rafael to remove function 0 check as
synchronization among drivers will be
tricky.
https://patchwork.freedesktop.org/patch/681119/?series=145342&rev=5#comment_1255325

>
>> + 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.
Ok
>
>> + 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

Introduced list based handling as in rev5 there was suggestion to avoid
adding variables in acpi/acpi_power structures.
To handle allocation leak in function acpi_device_release() I will add
code to scan list and delete the entry.

>> + 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.
Will fix it.
>
>> + 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.
Ok
>
>> + 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.

This is to indicate caller to retry after retry_interval returned by
ACPI method.
It's up to caller whether to retry or just proceed.

Thanks,
Badal

>
>> + 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);