Re: [PATCH v4 0/7] Add Devres managed IRQ vectors allocation

[Philipp Stanner <phasta@mailbox.org>]

Hi Shawn,

On Fri, 2026-04-24 at 15:57 +0800, Shawn Lin wrote:
> 
> There is a long-standing design ambiguity in the PCI/MSI subsystem regarding
> the management of IRQ vectors. Currently, the management operates in a "hybrid"
> mode. Sometimes it's handled by devres but sometimes not, creating potential
> for resource management bugs.
> 
> Historically, pcim_enable_device() implicitly triggers automatic IRQ vector
> management when calling pci_alloc_irq_vectors[_affinity], as pcim_enable_device()
> sets the is_managed flag, thus pcim_msi_release() will register a cleanup action.
> However, using pci_enable_device() will not make pci_alloc_irq_vectors[_affinity]
> register a cleanup action.
> 
> This creates an ambiguous ownership model. Many drivers follow a pattern of:
> 1. Calling pci_alloc_irq_vectors() to allocate interrupts.
> 2. Also calling pci_free_irq_vectors() in their error paths or remove routines.
> 
> When such a driver also uses pcim_enable_device(), the devres framework may
> attempt to free the IRQ vectors a second time upon device release, leading to
> a double-free. Analysis of the tree shows this hazardous pattern exists widely,
> while 37 other drivers correctly rely solely on the implicit cleanup.
> 
> To identify the scope of this issue, I used the following shell commands to
> search for drivers using pcim_enable_device and allocation functions but missing
> the explicit free call:
> 
> grep -rl "pcim_enable_device" drivers/ \
>   | xargs grep -l -E "pci_alloc_irq_vectors|pci_alloc_irq_vectors_affinity" 2>/dev/null \
>   | xargs grep -L "pci_free_irq_vectors" 2>/dev/null
> 
> drivers/dma/hsu/pci.c
> drivers/dma/hisi_dma.c
> drivers/hid/intel-thc-hidintel-quicki2c/pci-quicki2c.
> drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> drivers/hid/intel-ish-hid/ipc/pci-ish.c
> drivers/accel/ivpu/ivpu_drv.c
> drivers/accel/qaic/qaic_drv.c
> drivers/accel/amdxdna/aie2_pci.c
> drivers/platform/x86/intel/ehl_pse_io.c
> drivers/media/pci/intel/ipu6/ipu6.c
> drivers/mfd/intel-lpss-pci.c
> drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c
> drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c
> drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c
> drivers/crypto/inside-secure/safexcel.c
> drivers/thunderbolt/nhi.c
> drivers/i2c/busses/i2c-mchp-pci1xxxx.c
> drivers/i2c/busses/i2c-amd-mp2-pci.c
> drivers/i2c/busses/i2c-thunderx-pcidrv.c
> drivers/i2c/busses/i2c-designware-pcidrv.c
> drivers/tty/serial/8250/8250_exar.c
> drivers/tty/serial/8250/8250_pci.c
> drivers/tty/serial/8250/8250_mid.c
> drivers/gpio/gpio-merrifield.c
> drivers/iommu/riscv/iommu-pci.c
> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> drivers/pci/switch/switchtec.c
> drivers/pci/controller/vmd.c
> drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> drivers/net/ethernet/hisilicon/hibmcge/hbg_irq.c
> drivers/net/ethernet/cavium/thunder/thunder_bgx.c
> drivers/net/ethernet/realtek/r8169_main.c
> drivers/mmc/host/cavium-thunderx.c
> drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
> drivers/cxl/pci.c
> drivers/spi/spi-pxa2xx-pci.c
> drivers/spi/spi-pci1xxxx.c
> 
> To count how many drivers, including using lagacy APIs, have the double-free bugs, the
> following shell commands find out 60 drivers.
> grep -rl "pcim_enable_device" drivers/ --include="*.c" \
>   | xargs grep -l -E "pci_alloc_irq_vectors|pci_alloc_irq_vectors_affinity" 2>/dev/null \
>   | xargs grep -l "pci_free_irq_vectors" 2>/dev/null \
>   | tee /dev/tty \
>   | wc -l
> 
> drivers/dma/dw-edma/dw-edma-pcie.c
> drivers/dma/plx_dma.c
> drivers/dma/amd/ae4dma/ae4dma-pci.c
> drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
> drivers/perf/hisilicon/hisi_pcie_pmu.c
> drivers/perf/hisilicon/hns3_pmu.c
> drivers/platform/x86/intel_ips.c
> ....
> drivers/hwtracing/intel_th/pci.c
> drivers/bus/mhi/host/pci_generic.c
> drivers/fpga/dfl-pci.c
> 51
> 
> grep -rl "pcim_enable_device" drivers/ --include="*.c" \
>   | xargs grep -l -E "pci_enable_msi|pci_enable_msix" 2>/dev/null \
>   | xargs grep -l "pci_disable_msi" 2>/dev/null \
>   | tee /dev/tty \
>   | wc -l
> 
> drivers/dma/ioat/init.c
> drivers/dma/amd/ptdma/ptdma-pci.c
> drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci_legacy.c
> drivers/crypto/ccp/sp-pci.c
> drivers/i2c/busses/i2c-ismt.c
> drivers/pci/msi/api.c
> drivers/misc/mei/pci-me.c
> drivers/misc/mei/pci-txe.c
> drivers/block/mtip32xx/mtip32xx.c
> 9
> 
> This series introduces new managed APIs: pcim_alloc_irq_vectors() and
> pcim_alloc_irq_vectors_affinity(). Drivers that wish to have devres-managed IRQ
> vectors should use these functions to ensure proper automatic cleanup without
> ambiguity.
> 
> Regarding the removal strategy discussed in v3 [1], two approaches were considered:
> a) Introducing another hybrid function temporarily.
> b) Duplicating code temporarily.
> 
> Following the discussion with Philipp, I agree that adding another hybrid function
> does not guarantee the completion of the final cleanup work. Therefore, starting
> from v4, I have chosen to duplicate the necessary code temporarily. Helper functions
> have been factored out to minimize code duplication.
> 
> In the short term, the series converts two drivers within the PCI subsystem to
> use the new APIs. The long-term goal is to convert all other drivers which wish to
> use these managed functions. For the legacy APIs, like pci_enable_msix_range_*() and
> pci_enable_msi(), we won't consider to provide devres-managed version for them but to
> fix the only 9 double-free drivers by using the new APIs provided by this series. And
> once ready, we could remove the problematic hybrid management pattern from
> pcim_setup_msi_release() entirely.
> 
> [1]: https://lore.kernel.org/linux-pci/9dc66010d61670dc5182062ef5f1a932f7374323.camel@mailbox.org/T/#t
> 
> 
> Changes in v4:
> - Rework to create dedicated devres-managed function (Philipp)

Thanks for the rework.

I skimmed through it and it very much looks spot-on, I don't see major
concerns.

I'm a bit loaded right now. Can do a more detailed review earliest next
week, probably later.


Regards
P.

> 
> Changes in v3:
> - Rework the commit message and function doc (Philipp)
> - Remove setting is_msi_managed flag from new APIs (Philipp)
> 
> Changes in v2:
> - Rebase
> - Introduce patches only for PCI subsystem to convert the API
> 
> Shawn Lin (7):
>   PCI/MSI: Split __pci_enable_msi_range() for reuse
>   PCI/MSI: Split __pci_enable_msix_range() for reuse
>   PCI/MSI: Introduce __pcim_enable_msi_range()
>   PCI/MSI: Introduce __pcim_enable_msix_range()
>   PCI/MSI: Add Devres managed IRQ vectors allocation
>   PCI: switchtec: Replace pci_alloc_irq_vectors() with
>     pcim_alloc_irq_vectors()
>   PCI: vmd: Replace pci_alloc_irq_vectors() with
>     pcim_alloc_irq_vectors()
> 
>  drivers/pci/controller/vmd.c   |   4 +-
>  drivers/pci/msi/api.c          |  84 +++++++++++++++++++++++++++++++
>  drivers/pci/msi/msi.c          | 110 +++++++++++++++++++++++++++++++++++------
>  drivers/pci/msi/msi.h          |   3 ++
>  drivers/pci/switch/switchtec.c |   6 +--
>  include/linux/pci.h            |  22 +++++++++
>  6 files changed, 210 insertions(+), 19 deletions(-)
>