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

[Damien Le Moal <dlemoal@kernel.org>]

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.

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 ?


-- 
Damien Le Moal
Western Digital Research