Re: [PATCH v6 2/3] PCI: Add device-specific reset for Qualcomm devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Missing Device Initialization and CRS Polling Wait in `pci_dev_d3cold_d0_cycle`
- [Medium] Commit message contradiction regarding IOMMU preparation

Pre-existing issues:
- [High] IOMMU and ATS unconditionally disrupted during sysfs `reset_method` capabilities probe
--

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

PCI: Add device-specific reset for Qualcomm devices

This patch adds device-specific reset methods for some Qualcomm PCIe devices
(ath11k, ath12k, SDX62/SDX65) that lack working reset methods for VFIO
passthrough. It introduces a shared pci_dev_d3cold_d0_cycle helper function
that attempts D3cold with automatic fallback to D3hot on platforms without
ACPI _PR3 power resources.

> Extract a shared pci_dev_d3cold_d0_cycle() helper function to avoid code
> duplication between pci_d3cold_reset() (strict _PR3 requirement) and the
> new reset_d3cold_d3hot() device-specific reset (automaticfallback). The
> helper handles IOMMU preparation, performs the power cycle via
> pci_set_power_state(), and cleans up IOMMU state.

[Severity: Medium]
The commit message states "The helper handles IOMMU preparation", but the
helper code documentation states "Caller must handle IOMMU preparation/cleanup
if needed" and leaves IOMMU handling to the caller. Could the commit message
be updated to reflect the implemented code logic?

[ ... ]

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 096868f80cd4c..f7a7443287fdf 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4491,6 +4491,32 @@ static int pci_pm_reset(struct pci_dev *dev, bool probe)
>  	return ret;
>  }
>  
> +/**
> + * pci_dev_d3cold_d0_cycle - Perform D3cold->D0 power cycle
> + * @dev: Device to power cycle
> + *
> + * Common helper to perform D3cold->D0 power cycle for reset methods.
> + * Attempts D3cold transition with automatic fallback to D3hot on platforms
> + * without ACPI _PR3 power resources.
> + *
> + * Caller must handle IOMMU preparation/cleanup if needed.
> + *
> + * Returns 0 on success, negative error code on failure.
> + */
> +int pci_dev_d3cold_d0_cycle(struct pci_dev *dev)
> +{
> +	int ret;
> +
> +	if (dev->current_state != PCI_D0)
> +		return -EINVAL;
> +
> +	ret = pci_set_power_state(dev, PCI_D3cold);
> +	if (ret)
> +		return ret;
> +
> +	return pci_set_power_state(dev, PCI_D0);
> +}

[Severity: High]
Does this function need to wait for device readiness (CRS) before returning?

Unlike pci_pm_reset() which polls with pci_dev_wait(), this returns
immediately after the D3hot->D0 transition. Since this targets the endpoint
directly and bypasses bridge-level pci_bridge_wait_for_secondary_bus() delays,
if the device internally resets during the transition and returns CRS (0xFFFF),
will pci_set_full_power_state() incorrectly mask the state and return success
prematurely?

If so, could this cause subsequent configuration accesses by VFIO to fail while
the hardware is still initializing?

[ ... ]

> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index caaed1a01dc02..2380a29970736 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4237,6 +4237,22 @@ static int reset_hinic_vf_dev(struct pci_dev *pdev, bool probe)
>  	return 0;
>  }
>  
> +/*
> + * Device-specific reset method via D3cold/D3hot power cycle.
> + *
> + * Some devices lack working FLR, advertise NoSoftRst+ (blocking PM reset),
> + * and have broken bus reset. This function provides device-specific reset via
> + * power cycling, attempting D3cold with automatic fallback to D3hot on platforms
> + * without ACPI _PR3 power resources.
> + */
> +static int reset_d3cold_d3hot(struct pci_dev *dev, bool probe)
> +{
> +	if (probe)
> +		return 0;
> +
> +	return pci_dev_d3cold_d0_cycle(dev);
> +}

[Severity: High]
This is a pre-existing issue, but since the commit message suggests overriding
via sysfs, will querying these reset methods cause IOMMU faults for running
devices?

When userspace reads or writes to /sys/bus/pci/devices/.../reset_method, the
kernel calls reset methods with probe = true. In __pci_dev_specific_reset(),
it unconditionally executes pci_dev_reset_iommu_prepare(dev) before checking
the probe flag:

drivers/pci/quirks.c:__pci_dev_specific_reset() {
    ...
    ret = pci_dev_reset_iommu_prepare(dev);
    if (ret)
        return ret;
    // then calls i->reset(dev, probe);
    ...
}

Since pci_dev_reset_iommu_prepare() temporarily tears down PASID domains and
halts translations, will querying the reset methods for an actively running
device like ath11k disrupt ongoing DMA operations and cause system crashes?

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