| Message ID | 20241017015849.190271-7-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | Fix and improve the Rockchip endpoint driver | expand |
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@kernel.org> > --- > 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
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; }
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@kernel.org> --- drivers/pci/controller/pcie-rockchip-ep.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)