On Thu, Oct 17, 2024 at 10:58:41AM +0900, Damien Le Moal wrote:
> The call to rockchip_pcie_prog_ep_ob_atu() used to map the PCI address
> of MSI data to the memory window allocated on probe for IRQs is done
> in rockchip_pcie_ep_send_msi_irq() assuming a fixed alignment to a
> 256B boundary of the PCI address. This is not correct as the alignment
> constraint for the RK3399 PCI mapping depends on the number of bits of
> address changing in the mapped region. This leads to an unstable system
> which sometimes work and sometimes does not (crashing on paging faults
> when memcpy_toio() or memcpy_fromio() are used).
>
> Similar to regular data mapping, the MSI data mapping must thus be
> handled according to the information provided by
> rockchip_pcie_ep_align_addr(). Modify rockchip_pcie_ep_send_msi_irq()
> to use rockchip_pcie_ep_align_addr() to correctly program entry 0 of
> the ATU for sending MSI IRQs.
>
> Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
> ---
> drivers/pci/controller/pcie-rockchip-ep.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> index f6959f9b94b7..dcd1b5415602 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -379,9 +379,10 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn,
> {
> struct rockchip_pcie *rockchip = &ep->rockchip;
> u32 flags, mme, data, data_mask;
> + size_t irq_pci_size, offset;
> + u64 irq_pci_addr;
> u8 msi_count;
> u64 pci_addr;
> - u32 r;
>
> /* Check MSI enable bit */
> flags = rockchip_pcie_read(&ep->rockchip,
> @@ -417,18 +418,21 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn,
> PCI_MSI_ADDRESS_LO);
>
> /* Set the outbound region if needed. */
> - if (unlikely(ep->irq_pci_addr != (pci_addr & PCIE_ADDR_MASK) ||
> + irq_pci_size = ~PCIE_ADDR_MASK + 1;
> + irq_pci_addr = rockchip_pcie_ep_align_addr(ep->epc,
> + pci_addr & PCIE_ADDR_MASK,
> + &irq_pci_size, &offset);
> + if (unlikely(ep->irq_pci_addr != irq_pci_addr ||
> ep->irq_pci_fn != fn)) {
> - r = rockchip_ob_region(ep->irq_phys_addr);
> - rockchip_pcie_prog_ep_ob_atu(rockchip, fn, r,
> - ep->irq_phys_addr,
> - pci_addr & PCIE_ADDR_MASK,
> - ~PCIE_ADDR_MASK + 1);
> - ep->irq_pci_addr = (pci_addr & PCIE_ADDR_MASK);
> + rockchip_pcie_prog_ep_ob_atu(rockchip, fn,
> + rockchip_ob_region(ep->irq_phys_addr),
> + ep->irq_phys_addr,
> + irq_pci_addr, irq_pci_size);
> + ep->irq_pci_addr = irq_pci_addr;
> ep->irq_pci_fn = fn;
> }
>
> - writew(data, ep->irq_cpu_addr + (pci_addr & ~PCIE_ADDR_MASK));
> + writew(data, ep->irq_cpu_addr + offset + (pci_addr & ~PCIE_ADDR_MASK));
> return 0;
> }
>
> --
> 2.47.0
>
Nice catch.
For DWC, in dw_pcie_ep_raise_msi_irq()
https://github.com/torvalds/linux/blob/v6.12-rc3/drivers/pci/controller/dwc/pcie-designware-ep.c#L519-L522
and in dw_pcie_ep_raise_msix_irq()
https://github.com/torvalds/linux/blob/v6.12-rc3/drivers/pci/controller/dwc/pcie-designware-ep.c#L603-L606
We also make sure that the address that we map is aligned,
and then write to the correct offset within that mapping:
ep->msi_mem + aligned_offset;
in order to write to the actual MSI address.
To me, it looks like doing a similar change as this patch does,
to dw_pcie_ep_raise_msi_irq() and dw_pcie_ep_raise_msix_irq(),
would make the PCI endpoint code more consistent overall.
Thoughts?
Kind regards,
Niklas
