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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] pci_d3cold_reset() falsely claims a successful reset if the ACPI _PR3 power resource is shared, failing to perform a hardware reset.
- [High] Missing Device Readiness Wait after D3cold-to-D0 transition causes reset to fail systematically and triggers PCIe UR errors.
- [High] Improper error handling leaves the device in an unpowered state while the PCI core issues configuration restores, triggering fatal PCIe errors.
- [High] Function-level D3cold reset on a bridge device powers off and permanently corrupts the software and hardware state of all subordinate devices.
--

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

PCI: Add d3cold as general reset method

This commit adds D3cold power cycle as a general PCI reset method for
single-function devices with ACPI _PR3 power resources. It places the
D3cold reset at the end of the reset hierarchy, executing a transition
through D3cold and back to D0.

> 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
> + * @dev: PCI device to reset
> + * @probe: if true, check if D3cold reset is supported; if false, perform reset

[ ... ]

> +static int pci_d3cold_reset(struct pci_dev *dev, bool probe)
> +{
> +	int ret;
> +
> +	if (dev->multifunction)
> +		return -ENOTTY;

[Severity: High]
Does this also need to check if the device is a bridge?

If pci_d3cold_reset() is invoked on a bridge (e.g., via sysfs), transitioning
to D3cold cuts power to the entire downstream hierarchy. The software state of
all child devices is updated to PCI_D3cold.

When the bridge is restored to PCI_D0, only the bridge's software state is
updated. The subordinate devices' power is restored, but they remain marked
as PCI_D3cold without their drivers being notified. This exposes wiped hardware
state to active drivers and can lead to system crashes.

Could we add a check for pci_is_bridge(dev) or dev->subordinate here?

> +
> +	if (!pci_pr3_present(dev))
> +		return -ENOTTY;

[Severity: Critical]
Is there a way to verify that the _PR3 power resource is exclusively owned by
this device?

ACPI power resources are reference-counted. If the _PR3 resource is shared
among multiple devices, transitioning this single device to D3cold merely
decrements the OS reference count. Physical power is not actually cut unless
all other dependent devices are also in D3cold.

The pci_set_power_state() call will succeed logically, but if the device has
NoSoftRst=1, returning it to D0 will not actually reset the internal state.
This could falsely indicate a full reset was performed, which is a significant
risk in VFIO passthrough scenarios.

> +
> +	if (probe)
> +		return 0;
> +
> +	if (dev->current_state != PCI_D0)
> +		return -EINVAL;
> +
> +	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: High]
This isn't a bug introduced by this patch, but how does this error path
interact with the PCI reset framework?

If pci_set_power_state() fails here, the device might remain in an unpowered
or partially powered state (D3cold or D3hot). The PCI reset framework in
pci_reset_function() ignores this failure and unconditionally executes
pci_dev_restore(dev).

Executing pci_restore_state() on a device stuck in D3hot or D3cold generates
configuration space accesses that result in Unsupported Requests (UR), PCIe
Completion Timeouts, or Master Aborts.

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

[Severity: High]
Is a delay required here before returning to allow the link to retrain?

A transition from D3cold to D0 acts as a Conventional Reset. The PCIe
specification requires the host to wait for the link to retrain and poll for
Configuration Request Retry Status (CRS) completions.

Because this returns immediately, the underlying pci_power_up() function reads
the PCI_PM_CTRL register while the link is still down, typically receiving
~0 (0xFFFF). This causes pci_power_up() to conclude the device is
inaccessible, returning -EIO.

Should this sleep for dev->d3cold_delay and call pci_dev_wait() before
returning?

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

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