Re: [PATCH v9 1/5] PCI: dwc: ep: Clear MSI iATU mapping in dw_pcie_ep_cleanup()

[sashiko-bot@kernel.org]

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

New issues:
- [High] dw_pcie_ep_cleanup() modifies ep->msi_iatu_mapped and unmaps iATU without holding epc->lock, introducing a race condition.

Pre-existing issues:
- [Critical] Repeated invocations of dwc_pcie_debugfs_deinit() lead to a double-free of pci->ptm_debugfs.
- [High] Persistent MSI iATU mapping conflicts with dynamic MSI-X iATU mapping, causing misdirected unmaps and permanent iATU window leaks.
- [High] Multi-Function (MF) MSI Requester ID is corrupted because the iATU mapping cache does not track func_no.
- [Medium] Debugfs entries are permanently lost after the first link reset.
--

--- Patch [1]: [PATCH v9 1/5] PCI: dwc: ep: Clear MSI iATU mapping in dw_pcie_ep_cleanup() ---
commit c81bd7f3f798231612f6baba5357a5d4dcc7b736
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

PCI: dwc: ep: Clear MSI iATU mapping in dw_pcie_ep_cleanup()

This commit unmaps the MSI iATU region and clears the msi_iatu_mapped
flag in dw_pcie_ep_cleanup() during a PERST# reset sequence. This ensures
that dw_pcie_ep_raise_msi_irq() performs a fresh iATU mapping on its next
invocation.

> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 7d2794945704e..31402ae218c79 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -1153,6 +1153,11 @@ void dw_pcie_ep_cleanup(struct dw_pcie_ep *ep)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>  
> +	if (ep->msi_iatu_mapped) {
> +		dw_pcie_ep_unmap_addr(ep->epc, 0, 0, ep->msi_mem_phys);
> +		ep->msi_iatu_mapped = false;
> +	}

[Severity: High]
Does this introduce a regression due to a race condition with epc->lock?

If a user-space ioctl or workqueue in an EPF driver calls pci_epc_raise_irq(),
it takes epc->lock and modifies ep->msi_iatu_mapped and iATU registers.
Concurrently, if a host-initiated PERST# reset triggers dw_pcie_ep_cleanup()
in the controller driver's IRQ handler, it modifies the exact same state here
without taking epc->lock.

Could this lead to data corruption of iATU registers or concurrent unaligned
accesses to the outbound window bitmap?

[Severity: High]
This is a pre-existing issue, but does the persistent MSI iATU mapping
conflict with dynamic MSI-X iATU mapping?

Both MSI and MSI-X use the same physical address (ep->msi_mem_phys). If
dw_pcie_ep_raise_msi_irq() caches the ATU mapping, and then
dw_pcie_ep_raise_msix_irq() is called, it maps the same address to a new
window:

drivers/pci/controller/dwc/pcie-designware-ep.c:dw_pcie_ep_raise_msix_irq() {
    ...
    ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr,
                              map_size);
    ...
}

When dw_pcie_ep_raise_msix_irq() later calls dw_pcie_ep_unmap_addr(), the
lookup function returns the first matching address:

drivers/pci/controller/dwc/pcie-designware-ep.c:dw_pcie_find_index() {
    ...
    if (ep->outbound_addr[index] != addr)
        continue;
    *atu_index = index;
    return 0;
}

Could this unmap the cached MSI window instead of the newly allocated MSI-X
window, permanently leaking the MSI-X window?

[Severity: High]
This is a pre-existing issue, but is the Multi-Function (MF) MSI Requester ID
corrupted because the iATU mapping cache does not track func_no?

When multiple endpoint functions are assigned the same MSI address by the host,
dw_pcie_ep_raise_msi_irq() for function 1 hits the cache because msg_addr and
map_size match the values cached by function 0:

drivers/pci/controller/dwc/pcie-designware-ep.c:dw_pcie_ep_raise_msi_irq() {
    ...
    if (ep->msi_iatu_mapped && (ep->msi_msg_addr != msg_addr ||
                                ep->msi_map_size != map_size)) {
    ...
}

Will it reuse the iATU programmed with PCIE_ATU_FUNC_NUM(0), sending function
1's MSI with the Requester ID of function 0, causing host IOMMU faults?

> +
>  	dwc_pcie_debugfs_deinit(pci);

[Severity: Critical]
This is a pre-existing issue, but do repeated invocations of
dwc_pcie_debugfs_deinit() lead to a double-free of pci->ptm_debugfs?

dw_pcie_ep_cleanup() is called by host drivers during a PERST# reset sequence.
It invokes dwc_pcie_debugfs_deinit(), which calls pcie_ptm_destroy_debugfs()
and frees the structure:

drivers/pci/pcie/ptm.c:pcie_ptm_destroy_debugfs() {
    ...
    kfree(ptm_debugfs);
}

However, pci->ptm_debugfs is never set to NULL. If the host resets the link a
second time, will the same pointer be passed to kfree() resulting in a
double-free?

[Severity: Medium]
This is a pre-existing issue, but are debugfs entries permanently lost after
the first link reset?

dw_pcie_ep_cleanup() unconditionally removes the debugfs directory during a
PERST# reset. However, dwc_pcie_debugfs_init() is only called during the
initial driver probe.

Does this mean the debugfs structure is destroyed and never recreated on
subsequent link-ups?

>  	dw_pcie_edma_remove(pci);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-pci-port-reset-v9-0-53570b92064d@oss.qualcomm.com?part=1