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

["Nilawar, Badal" <badal.nilawar@intel.com>]

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