Re: [PATCH v5 01/18] PCI: Don't suspend IOMMU when probing reset capability

Jason Gunthorpe <[email protected]>
Newsgroups org.kernel.vger.linux-acpi,dev.linux.lists.iommu,org.infradead.lists.linux-arm-kernel,org.kernel.vger.linux-kernel,org.kernel.vger.linux-pci
Message-ID <178786269177.3808739.6471040051117543556.b4-review@b4>
> reset_method_store() in drivers/pci/pci-sysfs.c discovers supported reset
> methods by calling reset_fn(pdev, PCI_RESET_PROBE, ...) without holding a
> device_lock, since the probe path is expected to query the device's reset
> capability without changing device state.
> 
> However, pci_reset_bus_function() and __pci_dev_specific_reset() violate
> that contract after pci_dev_reset_iommu_prepare/done() were added, which
> moves the device into a blocking domain and abruptly aborts any in-flight
> DMA. Doing this for a probe -- a state-query call that does not even hold
> device_lock -- can cause driver timeouts and data loss on a DMAing device.
> 
> The peer reset helpers all handle this correctly: they short-circuit on a
> probe input before touching the IOMMU.
> 
> Skip pci_dev_reset_iommu_prepare()/_done() entirely when probe is set. The
> inner reset routines already implement their own probe semantics, and they
> perform the capability checks and return without changing device state.
> 
> Fixes: f5b16b802174 ("PCI: Suspend iommu function prior to resetting a device")
> Cc: [email protected]
> Assisted-by: Claude:claude-opus-4-7
> Signed-off-by: Nicolin Chen <[email protected]>
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 77b17b13ee6154..01cf3105405614 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4946,10 +4946,12 @@ static int pci_reset_bus_function(struct pci_dev *dev, bool probe)
>  	if (bridge && pcie_is_cxl(bridge) && cxl_sbr_masked(bridge))
>  		return -ENOTTY;
>  
> -	rc = pci_dev_reset_iommu_prepare(dev);
> -	if (rc) {
> -		pci_err(dev, "failed to stop IOMMU for a PCI reset: %d\n", rc);
> -		return rc;
> +	if (!probe) {

Given this is using this funny enum:

 #define PCI_RESET_PROBE		true
 #define PCI_RESET_DO_RESET	false

Was the intention to also use it to sample probe? Bit confusing

The actual change looks fine:

Reviewed-by: Jason Gunthorpe <[email protected]>

-- 
Jason
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.