Re: [PATCH v6 02/12] PCI/ACPI: Add PERST# Assertion Delay _DSM method

Nilawar, Badal
2026-01-20T15:59:33Z

On 15-01-2026 01:25, Bjorn Helgaas wrote:
> On Tue, Jan 13, 2026 at 10:12:03PM +0530, Badal Nilawar wrote:
>> From: Anshuman Gupta <anshuman.gupta@intel.com>
>>
>> Implement _DSM Method 0Bh as per PCI Firmware r3.3, sec 4.6.10, to request
>> fixed delay in timing between the time the PME_TO_Ack message is received
>> at the PCI Express Downstream Port that originated the PME_Turn_Off
>> message, and the time the platform asserts PERST# to the slot during the
>> corresponding Endpoint’s or PCI Express Upstream Port’s transition to
>> D3cold while the system is in an ACPI operational state.
>> Host platform supporting this feature ensures that device is observing
>> this delay in every applicable D3Cold transition.
> Thanks for this work!
>
> Add blank lines between paragraphs.
>
> s/D3Cold/D3cold/ to match other uses in drivers/pci/
Ok.
>
>> 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>
>> ---
>> For VRSR feature with PERST# Assertion delay device will get enough time
>> to transition to auxiliary power before main power removal.
>> ---
>> drivers/pci/pci-acpi.c | 60 ++++++++++++++++++++++++++++++++++++++++
>> include/linux/pci-acpi.h | 9 +++++-
>> 2 files changed, 68 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index 645d3005ba50..73eaee20a270 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -1554,6 +1554,66 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_mw,
>> }
>> EXPORT_SYMBOL_GPL(pci_acpi_request_d3cold_aux_power);
>>
>> +/**
>> + * pci_acpi_add_perst_assertion_delay - Request PERST# Delay via ACPI DSM
>> + * @dev: PCI device instance
>> + * @delay_us: Requested delay_us
>> + *
>> + * Request PERST# Assertion Delay to platform firmware, via Root Port/
>> + * Switch Downstream Port ACPI _DSM Function 0Bh, for the specified
>> + * PCI device.
>> + * Evaluate the _DSM and handle the response accordingly.
> Add blank line between paragraphs. Actually, you can just omit the
> last sentence because it doesn't tell us anything useful.
Sure.
>
>> + * Return: returns 0 on success and errno on failure.
> s/Return: returns 0/Return: 0/
Ok
>
>> + */
>> +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us)
>> +{
>> + union acpi_object in_obj = {
>> + .integer.type = ACPI_TYPE_INTEGER,
>> + .integer.value = delay_us,
>> + };
>> +
> Spurious blank line.
Ok
>
>> + union acpi_object *out_obj;
>> + int result, ret = -EINVAL;
> "ret" is unnecessary; see below.
Will take care of this.
>
>> + struct pci_dev *bdev;
>> + acpi_handle handle;
>> +
>> + if (!dev)
>> + return -EINVAL;
>> +
>> + for (bdev = dev; bdev; bdev = pci_upstream_bridge(bdev)) {
> I think bdev should start with pci_upstream_bridge(dev).
Yes.
>
> IIUC you intend that "dev" is the Endpoint or Upstream Port for which
> a driver wants PERST# assertion to be delayed. Per sec 4.6.11, the
> _DSM must be in a Downstream Port, not the device itself.

Yes. _DSM is in Downstream port but not device itself. For VRSR use case
_DSM
is in root port. From the description, my understanding is Downstream
port (bridge/root port),
which has sent PME_Turn_Off message, will introduce delay after
PME_TO_Ack and then initiate PERST.

>
> Sec 4.6.11 also says we should track this per Downstream Port and
> request the maximum of delays requested by any child. So I think we
> need to:
>
> - add a perst_delay in struct pci_dev
>
> - when we find this _DSM, set
> bdev.perst_delay = max(bdev.perst_delay, delay_us)
>
> - pass bdev.perst_delay to the _DSM instead of delay_us

For vrsr use case delay requested is 10ms, which is maximum allowed by
this _DSM.
I think we should take care of above suggestion when we implement aux
power aggregation.
For now, I can create another list let's call it acpi_perst_delay_list,
similar to acpi_aux_power_list, and
allow a only one device to request a PERST delay.

Thanks,
Badal

>> + handle = ACPI_HANDLE(&bdev->dev);
>> + if (handle &&
>> + acpi_check_dsm(handle, &pci_acpi_dsm_guid, 4,
>> + 1 << DSM_PCI_PERST_ASSERTION_DELAY))
>> + break;
>> + }
>> +
>> + if (!bdev)
>> + return -ENODEV;
>> +
>> + out_obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&bdev->dev),
>> + &pci_acpi_dsm_guid, 4,
>> + DSM_PCI_PERST_ASSERTION_DELAY,
>> + &in_obj, ACPI_TYPE_INTEGER);
>> + if (!out_obj)
>> + return -EINVAL;
>> +
>> + result = out_obj->integer.value;
>> + ACPI_FREE(out_obj);
>> +
>> + if (result == delay_us) {
>> + pci_info(dev, "PERST# Assertion Delay set to %u microseconds\n", delay_us);
> I think this message should use "bdev" instead of "dev". I know the
> request is for "dev", but the delay connected to bdev and applies to
> all functions below the Downstream Port.
Since it is set for the device which as requested I kept dev.
>
> Wrap such that the string itself isn't broken across lines, but the
> "delay_us" is one the next line (as in the "failed" message below).
>
>> + ret = 0;
>> + } else {
>> + pci_info(dev, "PERST# Assertion Delay request failed, using %u microseconds\n",
>> + result);
>> + }
>> +
>> + return ret;
> No need for the "else"; we can just return error early and unindent
> the normal path:
Sure.
>
> if (result != delay_us) {
> pci_info(bdev, "... failed");
> return -EINVAL;
> }
>
> pci_info(bdev, "... set");
> return 0;
>
>> +}