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

["Rafael J. Wysocki" <rafael@kernel.org>]

On Wed, Oct 15, 2025 at 10:02 AM Badal Nilawar <badal.nilawar@intel.com> 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.
>
> Note that this implementation assumes only a single device below the
> Downstream Port will 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
> ---
>  drivers/pci/pci-acpi.c   | 114 +++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-acpi.h |   9 ++++
>  2 files changed, 123 insertions(+)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 9369377725fa..c7eab1e75dd5 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1418,6 +1418,120 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
>         ACPI_FREE(obj);
>  }
>
> +static struct pci_dev *pci_acpi_check_dsm(struct pci_dev *dev, u64 rev, u64 funcs)

The name of the function should be more specific than this, at least
to indicate that it is about the downstream port _DSM.

> +{
> +       struct pci_dev *bdev;
> +       acpi_handle handle;
> +
> +       if (!dev)
> +               return ERR_PTR(-EINVAL);

dev has been checked against NULL already by the caller.

> +
> +       bdev =  dev;
> +       while (bdev) {

And there would be fewer code lines if a for () loop were used here.

> +               handle = ACPI_HANDLE(&bdev->dev);
> +               if (handle && acpi_check_dsm(handle, &pci_acpi_dsm_guid, rev, 1 << funcs))
> +                       return bdev;
> +
> +               bdev = pci_upstream_bridge(bdev);
> +       }
> +
> +       return ERR_PTR(-ENODEV);

Personally, I don't see much value in putting this code into a
separate function.

> +}
> +
> +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
> +};
> +
> +/**
> + * pci_acpi_request_d3cold_aux_power - Request aux power while device is in D3cold
> + * @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.
> + *
> + * For Multi-Function Devices, driver for Function 0 is required to report an
> + * aggregate power requirement covering all functions contained within the
> + * device.
> + *
> + * Note: Aggregation across multiple child devices beneath the Root/Switch Downstream
> + * Port is not supported.

"Note: " isn't particularly useful here IMV.

> + *
> + * Return: Returns 0 on success and 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, ret = -EINVAL;
> +       struct pci_dev *bdev;
> +
> +       if (!dev || PCI_FUNC(dev->devfn) != 0)

I'm not sure about the PCI_FUNC(dev->devfn) check.

Synchronization between drivers in this respect can be somewhat
tricky.  It would be more straightforward to allow the first caller of
this for a given port with the _DSM under it to "win" in all cases,
including different functions within the same device (this happens in
the other cases anyway IIUC).

> +               return -EINVAL;
> +
> +       bdev = pci_acpi_check_dsm(dev, 4, 1 << DSM_PCI_D3COLD_AUX_POWER_LIMIT);
> +
> +       if (IS_ERR(bdev))
> +               return PTR_ERR(bdev);
> +
> +       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;

out_obj can be freed here and the code below can be made to return an
error code right away in multiple places.

> +       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);
> +               break;
> +       case AUX_PWR_REQ_GRANTED:
> +               pci_info(bdev, "D3cold Aux Power request granted: %u mW\n",
> +                        requested_mw);
> +               ret = 0;
> +               break;
> +       case AUX_PWR_REQ_NO_MAIN_PWR_REMOVAL:
> +               pci_info(bdev, "D3cold Aux Power: Main power won't be removed\n");
> +               ret = -EBUSY;
> +               break;
> +       default:
> +               if (result >= AUX_PWR_REQ_RETRY_INTERVAL_MIN &&
> +                   result <= AUX_PWR_REQ_RETRY_INTERVAL_MAX) {
> +                       pci_info(bdev, "D3cold Aux Power request needs retry interval: %u seconds\n",
> +                                result & 0xF);
> +                       if (retry_interval) {
> +                               *retry_interval = result & 0xF;
> +                               ret = -EAGAIN;
> +                       }
> +               } else {
> +                       pci_err(bdev, "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);
> +