Re: [PATCH 2/2] PCI: dwc: ep: Use align addr function for dw_pcie_ep_raise_{msi,msix}_irq()

[Niklas Cassel <cassel@kernel.org>]

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@kernel.org>
> >> ---
> >>  .../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