On Fri, Nov 01, 2024 at 07:36:51AM +0900, Damien Le Moal wrote:
> On 11/1/24 05:28, Bjorn Helgaas wrote:
> > On Thu, Oct 17, 2024 at 03:20:55PM +0200, Niklas Cassel wrote:
> >> Use the dw_pcie_ep_align_addr() function to calculate the alignment in
> >> dw_pcie_ep_raise_{msi,msix}_irq() instead of open coding the same.
> >>
> >> Signed-off-by: Niklas Cassel <cassel@xxxxxxxxxx>
> >> ---
> >> .../pci/controller/dwc/pcie-designware-ep.c | 18 +++++++++---------
> >> 1 file changed, 9 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> >> index 20f67fd85e83..9bafa62bed1d 100644
> >> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> >> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> >> @@ -503,7 +503,8 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
> >> u32 msg_addr_lower, msg_addr_upper, reg;
> >> struct dw_pcie_ep_func *ep_func;
> >> struct pci_epc *epc = ep->epc;
> >> - unsigned int aligned_offset;
> >> + size_t msi_mem_size = epc->mem->window.page_size;
> >> + size_t offset;
> >> u16 msg_ctrl, msg_data;
> >> bool has_upper;
> >> u64 msg_addr;
> >> @@ -531,14 +532,13 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
> >> }
> >> msg_addr = ((u64)msg_addr_upper) << 32 | msg_addr_lower;
> >>
> >> - aligned_offset = msg_addr & (epc->mem->window.page_size - 1);
> >> - msg_addr = ALIGN_DOWN(msg_addr, epc->mem->window.page_size);
> >> + msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &msi_mem_size, &offset);
> >> ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr,
> >> - epc->mem->window.page_size);
> >> + msi_mem_size);
> >
> > I haven't worked through this; just double checking that this is
> > correct. Previously we did ALIGN_DOWN() here, but
> > dw_pcie_ep_align_addr() uses ALIGN() (not ALIGN_DOWN()). Similar
> > below in dw_pcie_ep_raise_msix_irq().
>
> The ALIGN() in dw_pcie_ep_align_addr() is for the mapping size. The address is
> aligned down manually:
>
> return pci_addr & ~mask;
>
> So it is the same. We could change dw_pcie_ep_align_addr() to do:
>
> return ALIGN_DOWN(pci_addr, epc->mem->window.page_size);
>
> But given that the offset calculation needs the alignment mask anyway, using
> the mask variable directly seems natural.
I agree.
>
> So this is functionnally identical for the PCI address being mapped, and it is
> even better for the mapping size since this was passing
> epc->mem->window.page_size before but if the PCI address range crosses over a
> page_size boundary, we actually need 2 x page_size as the mapping size...
>
> Which makes me realized that there is something still wrong: the memory being
> mapped (ep->msi_mem) is at most one page:
>
> ep->msi_mem = pci_epc_mem_alloc_addr(epc, &ep->msi_mem_phys,
> epc->mem->window.page_size);
> if (!ep->msi_mem) {
> ret = -ENOMEM;
> dev_err(dev, "Failed to reserve memory for MSI/MSI-X\n");
> goto err_exit_epc_mem;
> }
>
> But we may need up to 2 pages depending on the PCI address we get for the
> MSI/MSIX. So we need to fix that as well I think.
>
> Niklas, thoughts ?
Hmm.. the MSI data that we want to map is just two bytes.
It is just that the minimum mapping we can do is epc->mem->window.page_size.
On e.g. rk3588 the page_size is 64k.
I guess that we might be unlucky and get the MSI data at an offset that is
at the end of the page.
But considering that we just need 2 bytes (for MSI, 4 bytes for MSI-X),
and that the PCI word size is 4 bytes, I don't see a problem with the
current code.
While the allocation of the msi_mem should still be page_size:
ep->msi_mem = pci_epc_mem_alloc_addr(epc, &ep->msi_mem_phys,
epc->mem->window.page_size);
I guess that we could change dw_pcie_ep_raise_msi_irq() from:
size_t msi_mem_size = epc->mem->window.page_size;
to:
size_t msi_mem_size = 2;
and change dw_pcie_ep_raise_msix_irq() from:
size_t msi_mem_size = epc->mem->window.page_size;
to:
size_t msi_mem_size = 4;
To make the code strictly more correct for a reader, but I
don't think that there is a problem with the current code.
Kind regards,
Niklas
