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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pci_d3cold_reset() fails to wait for device readiness after transitioning from D3cold to D0, leading to immediate config read failures and state corruption.
- [High] pci_d3cold_reset() does not prevent execution on single-function bridges, leading to permanent state corruption and physical data loss of all subordinate devices.
--

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

PCI: Add d3cold as general reset method

This patch adds D3cold power cycling as a reset method for single-function
devices via ACPI _PR3, placing it at the end of the reset hierarchy.

> 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

[ ... ]

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

[Severity: High]
Does this allow execution on single-function bridges, such as PCIe Root
Ports?

If pci_set_power_state(dev, PCI_D3cold) is called on a bridge, it recursively
updates all subordinate devices' tracking state to D3cold via
__pci_bus_set_current_state() and physically removes power from them.

The subsequent transition back to D0 only updates the bridge itself. This
leaves all downstream devices tracked as PCI_D3cold indefinitely, and they
lose all their configuration and BARs since the reset framework only saves
and restores the bridge's configuration state. 

> +	if (!pci_pr3_present(dev))
> +		return -ENOTTY;
> +
> +	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;
> +
> +	ret = pci_set_power_state(dev, PCI_D0);

[Severity: High]
Does this sequence fail to account for the mandatory wait time before the
device is responsive to config space accesses?

A D3cold to D0 transition acts as a fundamental reset. pci_set_power_state()
internally invokes pci_power_up(), which applies power and immediately
attempts to read PCI_PM_CTRL without waiting for link training or handling
Configuration Request Retry Status (CRS).

Because the device is unready, this read can return 0xFFFF (Unsupported
Request) and cause pci_power_up() to spuriously fail with -EIO. Should this
include a polling delay, similar to pci_dev_wait(), to prevent the reset
framework from blindly failing or restoring state to an uninitialized device?

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

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