Hello Damien,
On Tue, Oct 15, 2024 at 03:47:18PM +0900, Damien Le Moal wrote:
> The PCI endpoint controller operation interface for the align_addr()
> operation uses the phys_addr_t type for the PCI address argument and
> return a value using this type. This is not ideal as PCI addresses are
> bus addresses, not memory physical addresses. Replace the use of
> phys_addr_t for this operation with the generic u64 type. To be
> consistent with this change the Designware driver implementation of this
> operation (function dw_pcie_ep_align_addr()) is also changed.
>
> Fixes: e98c99e2ccad ("PCI: endpoint: Introduce pci_epc_mem_map()/unmap()")
> Fixes: cb6b7158fdf5 ("PCI: dwc: endpoint: Implement the pci_epc_ops::align_addr() operation")
> Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
> ---
> drivers/pci/controller/dwc/pcie-designware-ep.c | 6 +++---
> include/linux/pci-epc.h | 4 ++--
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
(snip)
> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> index f4b8dc37e447..ff208fd4526b 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -93,8 +93,8 @@ struct pci_epc_ops {
> struct pci_epf_bar *epf_bar);
> void (*clear_bar)(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> struct pci_epf_bar *epf_bar);
> - phys_addr_t (*align_addr)(struct pci_epc *epc, phys_addr_t pci_addr,
> - size_t *size, size_t *offset);
> + u64 (*align_addr)(struct pci_epc *epc, u64 pci_addr, size_t *size,
> + size_t *offset);
> int (*map_addr)(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> phys_addr_t addr, u64 pci_addr, size_t size);
> void (*unmap_addr)(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> --
> 2.47.0
>
1) You seem to have missed to update:
"phys_addr_t pci_addr" and "phys_addr_t map_pci_addr"
in struct pci_epc_map (in include/linux/pci-epc.h).
2) You seem to have missed my comment on v6:
> + * @align_addr: operation to get the mapping address, mapping size and offset
> + * into a controller memory window needed to map an RC PCI address
> + * region
I think this text should be more clear that it is about the PCI address.
Perhaps something like:
Operation to get the PCI address to map and the size of the mapping,
in order to satisfy address translation requirements of the controller.
3) The problem with using u64 is that it will be 64-bit even on 32-bit
systems.
Looking at:
https://github.com/torvalds/linux/blob/master/Documentation/core-api/dma-api-howto.rst#cpu-and-dma-addresses
and
https://github.com/torvalds/linux/blob/master/include/linux/pci.h#L820-L824
makes me think that dma_addr_t is a better choice than u64 in this case.
pci_bus_addr_t is probably an even better choice, but it doesn't seem
to be used outside drivers/pci/ core code, and it is simply defined to
have the same size as dma_addr_t (CONFIG_ARCH_DMA_ADDR_T_64BIT) anyway.
Kind regards,
Niklas
