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