Re: [PATCH v6 1/3] PCI: Add d3cold as general reset method

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] pci_d3cold_reset() can falsely return success without actually power-cycling the device if ACPI power resources are shared, leaking device state across resets.
- [High] pci_d3cold_reset() calls pci_set_power_state(dev, PCI_D0) without waiting for the PCIe link to retrain and the device to become ready, causing immediate config space accesses to fail and breaking the reset process.
- [High] pci_d3cold_reset() does not prevent execution on bridges, allowing a single-function reset to unintentionally tear down an entire downstream hierarchy.
--

commit 7e36b6b44c0f04c412af5d637f17c6d81f5c01ff
Author: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>

PCI: Add d3cold as general reset method
    
Add D3cold power cycle as a general PCI reset method for single-function
devices on platforms with ACPI _PR3 power resources. This provides true
power cycle reset capability when the platform can physically cut power
to the device.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8f7cfcc000901..096868f80cd4c 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
[ ... ]
> @@ -4491,6 +4491,55 @@ static int pci_pm_reset(struct pci_dev *dev, bool probe)
>  	return ret;
>  }
>  
> +/**
> + * pci_d3cold_reset - Put device into D3cold and back to D0 for reset
[ ... ]
> + */
> +static int pci_d3cold_reset(struct pci_dev *dev, bool probe)
> +{
> +	int ret;
> +
> +	if (dev->multifunction)
> +		return -ENOTTY;

[Severity: High]
Does this code allow bridges to be power-cycled, tearing down downstream
links?

A single-function device can be a PCIe root port or switch downstream port.
If a reset is invoked on such a bridge and it has an ACPI _PR3 power
resource controlling slot power, transitioning the bridge to D3cold would
physically cut power to all downstream devices on its secondary bus.

Should we check pci_is_bridge(dev) to ensure function-level resets are scoped
to the targeted function and do not inflict collateral power loss on
downstream hierarchies?

> +
> +	if (!pci_pr3_present(dev))
> +		return -ENOTTY;
[ ... ]
> +	ret = pci_dev_reset_iommu_prepare(dev);
> +	if (ret) {
> +		pci_err(dev, "failed to stop IOMMU for a PCI reset: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = pci_set_power_state(dev, PCI_D3cold);
> +	if (ret)
> +		goto done;

[Severity: Critical]
Could this return false success without actually power-cycling the device if
ACPI power resources are shared?

ACPI power resources use reference counting, so if the power resource is
shared with another active device, the reference count is decremented but
power is not physically cut. The device merely transitions to D3hot.

If the device supports the NoSoftRst property, it will retain its complete
internal state across the cycle. Is there a way to verify physical power
was actually removed before returning success, to avoid leaking state when
scrubbing devices for assignments?

> +
> +	ret = pci_set_power_state(dev, PCI_D0);

[Severity: High]
Can this transition to D0 fail because the link hasn't been given time to
retrain?

A transition from D3cold to D0 is a Fundamental Reset. When
pci_set_power_state() calls pci_power_up(), it executes the platform power-on
method and immediately performs a configuration read on PCI_PM_CTRL without
delay or Configuration Request Retry Status (CRS) polling.

Because the link is not fully trained, the read hits CRS or returns 0xFFFF,
causing pci_power_up() to return -EIO and immediately abort the reset.

Unlike pci_pm_reset(), this function also lacks an explicit pci_dev_wait()
invocation, which would leave the device uninitialized if the PCI subsystem
attempts to restore configuration on an unresponsive link.

> +
> +done:
> +	pci_dev_reset_iommu_done(dev);
> +	return ret;
> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602160024.1171949-1-jtornosm@redhat.com?part=1