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

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Jan 20, 2026 at 09:29:16PM +0530, Nilawar, Badal wrote:
> 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.

> > > +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us)
> > > +{
> ...

> > > +	if (!dev)
> > > +		return -EINVAL;

Calling this with "dev == NULL" is a bug in the caller, so I don't
think we should check for this.  That way we will take the NULL
pointer dereference, cause an oops, and can fix the bug.  If we check
for NULL, the bug is likely to be unnoticed.

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

I think lists will be a maintenance headache.

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

It's set for *all* devices below bdev, so I think the message should
give a clue about that.  If we only include "dev", there is no
indication that other devices are affected.  Maybe we could include
both dev and bdev?

  pci_info(bdev, "PERST# assertion delay %u microseconds set as requested by %s\n",
           delay_us, pci_name(bdev));