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:
- [High] Missing Device Readiness Wait
- [High] Unprotected State Modifications (IOMMU ATS invalidation on suspended device)
- [Medium] Commit Message Discrepancy (IOMMU preparation)
--

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

PCI: Add device-specific reset for Qualcomm devices

This commit adds a device-specific reset method for certain Qualcomm devices 
(WCN6855, WCN7850, SDX62/SDX65) via D3cold/D3hot power cycling. It also
extracts a shared helper function for the power cycle transition.

> 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]
Is the description of the helper function accurate? The commit message states 
that the helper handles IOMMU preparation and cleanup, but the extracted 
pci_dev_d3cold_d0_cycle() function does not perform these operations and 
explicitly requires the caller to handle them. 

Could the commit message be updated to reflect the actual implementation?

> 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 sequence need to wait for the device to become ready? After
returning to D0 via pci_set_power_state(), the device undergoes a reset,
and the PCI spec requires polling Configuration Request Retry Status (CRS)
before it is ready.

Unlike pci_pm_reset(), which explicitly calls pci_dev_wait(), this helper
returns immediately. Since callers like pci_d3cold_reset() and
reset_d3cold_d3hot() rely on this, will this cause the PCI core to execute
pci_dev_restore() before the device is responsive, potentially leading to
dropped configuration writes?

> +}
> +
>  /**
>   * pci_d3cold_reset - Put device into D3cold and back to D0 for reset
>   * @dev: PCI device to reset
> @@ -4520,22 +4546,13 @@ static int pci_d3cold_reset(struct pci_dev *dev, bool probe)
>  	if (probe)
>  		return 0;
>  
> -	if (dev->current_state != PCI_D0)
> -		return -EINVAL;
> -
>  	ret = pci_dev_reset_iommu_prepare(dev);

[Severity: High]
Can this trigger ATS invalidations on suspended devices?

By moving the power state check inside the pci_dev_d3cold_d0_cycle()
helper, pci_dev_reset_iommu_prepare() is now executed before verifying that
the device is in D0. If the device is currently in D3, interacting with
the IOMMU to detach domains can trigger PCIe ATS invalidations, which may
result in Master Aborts or completion timeouts.

Should the state check remain before IOMMU preparation?

>  	if (ret) {
>  		pci_err(dev, "failed to stop IOMMU for a PCI reset: %d\n", ret);
>  		return ret;
>  	}

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