Re: [PATCH v3 1/2] PCI: Add D3cold as general reset method

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Silent reset failure and state leak via NoSoftRst+ bypass
- [High] Missing IOMMU coordination during reset sequence
- [High] Missing device readiness wait (CRS polling) after power transition
- [High] Catastrophic power loss for downstream hierarchy on single-function bridges
- [Medium] False advertisement of reset support in probe mode
--

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

PCI: Add D3cold as general reset method

This commit adds a D3cold power cycle as a general PCI reset method for
single-function devices. It implements pci_d3cold_reset() which attempts to
transition the device through D3cold and back to D0, falling back to D3hot if
ACPI _PR3 is unavailable.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8f7cfcc000901..6da8feff7ccc3 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c

[ ... ]

> @@ -4491,6 +4491,46 @@ static int pci_pm_reset(struct pci_dev *dev, bool probe)
>  	return ret;
>  }
>  
> +/**
> + * pci_d3cold_reset - Put device into D3cold (or D3hot) and back to D0 for reset
> + * @dev: PCI device to reset
> + * @probe: if true, check if D3cold reset is supported; if false, perform reset
> + *
> + * Attempt to reset the device by transitioning through D3cold and back to D0.
> + * On platforms with ACPI _PR3 power resources, this performs a true D3cold
> + * power cycle (actual power removal). On platforms without _PR3 support,
> + * pci_set_power_state() automatically falls back to D3hot, providing a
> + * D3hot->D0 reset transition.
> + *
> + * Only available for single-function devices to avoid affecting other
> + * functions in multi-function devices.
> + *
> + * Returns 0 if device can be/was reset this way, -ENOTTY if not supported,
> + * or other negative error code on failure.
> + */
> +static int pci_d3cold_reset(struct pci_dev *dev, bool probe)
> +{
> +	int ret;
> +
> +	if (dev->multifunction)
> +		return -ENOTTY;

Many PCIe bridges (like Root Ports and Downstream Switch Ports) are
single-function devices.

If pci_d3cold_reset() is triggered on a single-function bridge,
pci_set_power_state() will physically shut off power to the entire downstream
hierarchy. Because pci_reset_function() is intended to reset only the target
function, does this unexpectedly drop power to subordinate devices without
unbinding their drivers or saving their state?

Should there be a check for dev->subordinate to prevent execution on bridges?

> +
> +	if (probe) {
> +		if (!pci_pr3_present(dev))
> +			pci_dbg(dev, "d3cold reset: no _PR3 support, will use D3hot fallback\n");
> +		return 0;

When called with probe = true, this unconditionally returns 0 if the device
is single-function, even if it lacks both ACPI _PR3 and native PM
capabilities (dev->pm_cap is not checked).

Could this incorrectly register d3cold as a supported reset method for hardware
that physically cannot change power states?

> +	}
> +
> +	if (dev->current_state != PCI_D0)
> +		return -EINVAL;
> +
> +	ret = pci_set_power_state(dev, PCI_D3cold);

Other PCI reset methods in the kernel bracket their operations with IOMMU
coordination. For example:

static int pci_pm_reset(struct pci_dev *dev, bool probe)
{
        ...
        ret = pci_dev_reset_iommu_prepare(dev);
        ...
        pci_dev_reset_iommu_done(dev);
        return ret;
}

By omitting these calls, does pci_d3cold_reset() allow power to be removed
while the device might still have in-flight DMA or active IOMMU mappings,
potentially triggering spurious DMA faults or fatal IOMMU errors?

> +	if (ret)
> +		return ret;
> +
> +	return pci_set_power_state(dev, PCI_D0);

When true D3cold is unavailable, pci_set_power_state() transparently falls
back to putting the device into D3hot. If the device advertises the
PCI_PM_CTRL_NO_SOFT_RESET bit, a D3hot->D0 transition explicitly does not
reset the device's internal state.

Other reset methods like pci_pm_reset() explicitly check this:

static int pci_pm_reset(struct pci_dev *dev, bool probe)
{
        ...
        pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &csr);
        if (csr & PCI_PM_CTRL_NO_SOFT_RESET)
                return -ENOTTY;
        ...
}

Since pci_d3cold_reset() does not check this bit and returns 0 on success,
does this falsely report a successful reset to subsystems like VFIO that rely
on this to wipe guest state, potentially leaking state across VM boundaries?

Additionally, returning a device from D3cold or D3hot to D0 constitutes a
Conventional Reset, where devices may require up to 1 second to reinitialize
and return Configuration Request Retry Status (CRS).

Existing methods ensure readiness before returning:

static int pci_pm_reset(struct pci_dev *dev, bool probe)
{
        ...
        ret = pci_dev_wait(dev, "PM D3hot->D0", PCIE_RESET_READY_POLL_MS);
        ...
        return ret;
}

Does returning immediately from pci_set_power_state() mean the caller might
instantly invoke pci_dev_restore() while the hardware is still unresponsive?

> +}

[ ... ]

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