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

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

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