[PATCH] PCI: endpoint: Improve pci_epc_ops::align_addr() interface

[PATCH] PCI: endpoint: Improve pci_epc_ops::align_addr() interface

[Damien Le Moal]

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@kernel.org>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 6 +++---
 include/linux/pci-epc.h                         | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 0ada60487c25..df1460ed63d9 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -268,12 +268,12 @@ static int dw_pcie_find_index(struct dw_pcie_ep *ep, phys_addr_t addr,
 	return -EINVAL;
 }
 
-static phys_addr_t dw_pcie_ep_align_addr(struct pci_epc *epc,
-			phys_addr_t pci_addr, size_t *pci_size, size_t *offset)
+static u64 dw_pcie_ep_align_addr(struct pci_epc *epc, u64 pci_addr,
+				 size_t *pci_size, size_t *offset)
 {
 	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
-	phys_addr_t mask = pci->region_align - 1;
+	u64 mask = pci->region_align - 1;
 	size_t ofst = pci_addr & mask;
 
 	*pci_size = ALIGN(ofst + *pci_size, ep->page_size);
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

Re: [PATCH] PCI: endpoint: Improve pci_epc_ops::align_addr() interface

[Niklas Cassel]

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

Re: [PATCH] PCI: endpoint: Improve pci_epc_ops::align_addr() interface

[Niklas Cassel]

On Tue, Oct 15, 2024 at 09:24:01AM +0200, Niklas Cassel wrote:
> 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.

Since:
int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
                     phys_addr_t phys_addr, u64 pci_addr, size_t size)

Currently uses u64 for the pci address, I guess keeping this new API to
use u64 for the pci address is the most consistent thing after all...

Although ideally, sometime in the future someone should probably convert
both APIs to use pci_bus_addr_t or dma_addr_t instead.


Kind regards,
Niklas