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

[PATCH v2] 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 regular 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()) as well as the type of PCI
address fields of struct pci_epc_map are 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>
---
Changes from v1:
 - Also updated the type of the pci_addr and map_pci_addr fields of
   struct pci_epc_map.

 drivers/pci/controller/dwc/pcie-designware-ep.c | 6 +++---
 include/linux/pci-epc.h                         | 8 ++++----
 2 files changed, 7 insertions(+), 7 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..de8cc3658220 100644
--- a/include/linux/pci-epc.h
+++ b/include/linux/pci-epc.h
@@ -49,10 +49,10 @@ pci_epc_interface_string(enum pci_epc_interface_type type)
  * @virt_addr: virtual address at which @pci_addr is mapped
  */
 struct pci_epc_map {
-	phys_addr_t	pci_addr;
+	u64		pci_addr;
 	size_t		pci_size;
 
-	phys_addr_t	map_pci_addr;
+	u64		map_pci_addr;
 	size_t		map_size;
 
 	phys_addr_t	phys_base;
@@ -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 v2] PCI: endpoint: Improve pci_epc_ops::align_addr() interface

[Niklas Cassel]

On Tue, Oct 15, 2024 at 06:07:12PM +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 regular 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()) as well as the type of PCI
> address fields of struct pci_epc_map are 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>
> ---
> Changes from v1:
>  - Also updated the type of the pci_addr and map_pci_addr fields of
>    struct pci_epc_map.
> 
>  drivers/pci/controller/dwc/pcie-designware-ep.c | 6 +++---
>  include/linux/pci-epc.h                         | 8 ++++----
>  2 files changed, 7 insertions(+), 7 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..de8cc3658220 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -49,10 +49,10 @@ pci_epc_interface_string(enum pci_epc_interface_type type)
>   * @virt_addr: virtual address at which @pci_addr is mapped
>   */
>  struct pci_epc_map {
> -	phys_addr_t	pci_addr;
> +	u64		pci_addr;
>  	size_t		pci_size;
>  
> -	phys_addr_t	map_pci_addr;
> +	u64		map_pci_addr;
>  	size_t		map_size;
>  
>  	phys_addr_t	phys_base;
> @@ -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
> 

Reviewed-by: Niklas Cassel <cassel@kernel.org>

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

[Manivannan Sadhasivam]

On Tue, Oct 15, 2024 at 06:07:12PM +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 regular 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()) as well as the type of PCI
> address fields of struct pci_epc_map are 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>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
> Changes from v1:
>  - Also updated the type of the pci_addr and map_pci_addr fields of
>    struct pci_epc_map.
> 
>  drivers/pci/controller/dwc/pcie-designware-ep.c | 6 +++---
>  include/linux/pci-epc.h                         | 8 ++++----
>  2 files changed, 7 insertions(+), 7 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..de8cc3658220 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -49,10 +49,10 @@ pci_epc_interface_string(enum pci_epc_interface_type type)
>   * @virt_addr: virtual address at which @pci_addr is mapped
>   */
>  struct pci_epc_map {
> -	phys_addr_t	pci_addr;
> +	u64		pci_addr;
>  	size_t		pci_size;
>  
> -	phys_addr_t	map_pci_addr;
> +	u64		map_pci_addr;
>  	size_t		map_size;
>  
>  	phys_addr_t	phys_base;
> @@ -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 v2] PCI: endpoint: Improve pci_epc_ops::align_addr() interface

[Manivannan Sadhasivam]

On Tue, Oct 15, 2024 at 06:07:12PM +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 regular 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()) as well as the type of PCI
> address fields of struct pci_epc_map are 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>

I thought of applying it, but then decided to squash it with the offending
patches.

- Mani

> ---
> Changes from v1:
>  - Also updated the type of the pci_addr and map_pci_addr fields of
>    struct pci_epc_map.
> 
>  drivers/pci/controller/dwc/pcie-designware-ep.c | 6 +++---
>  include/linux/pci-epc.h                         | 8 ++++----
>  2 files changed, 7 insertions(+), 7 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..de8cc3658220 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -49,10 +49,10 @@ pci_epc_interface_string(enum pci_epc_interface_type type)
>   * @virt_addr: virtual address at which @pci_addr is mapped
>   */
>  struct pci_epc_map {
> -	phys_addr_t	pci_addr;
> +	u64		pci_addr;
>  	size_t		pci_size;
>  
> -	phys_addr_t	map_pci_addr;
> +	u64		map_pci_addr;
>  	size_t		map_size;
>  
>  	phys_addr_t	phys_base;
> @@ -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 v2] PCI: endpoint: Improve pci_epc_ops::align_addr() interface

[Damien Le Moal]

On 10/17/24 01:59, Manivannan Sadhasivam wrote:
> On Tue, Oct 15, 2024 at 06:07:12PM +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 regular 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()) as well as the type of PCI
>> address fields of struct pci_epc_map are 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>
> 
> I thought of applying it, but then decided to squash it with the offending
> patches.

Fine with me. Thanks !


-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 10/17/24 09:37, Damien Le Moal wrote:
> On 10/17/24 01:59, Manivannan Sadhasivam wrote:
>> On Tue, Oct 15, 2024 at 06:07:12PM +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 regular 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()) as well as the type of PCI
>>> address fields of struct pci_epc_map are 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>
>>
>> I thought of applying it, but then decided to squash it with the offending
>> patches.
> 
> Fine with me. Thanks !

Just checked the pci/endpoint branch but I do not see this patch squashed there.
Did you push the change ?

-- 
Damien Le Moal
Western Digital Research

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

[Krzysztof Wilczyński]

Hello,

[...]
> >> I thought of applying it, but then decided to squash it with the offending
> >> patches.
> > 
> > Fine with me. Thanks !
> 
> Just checked the pci/endpoint branch but I do not see this patch squashed there.

If you so happen to check using the Git web interface kernel.org, then
there is some sort of a caching going on that can return old results for
a while before the cache expires.

> Did you push the change ?

Looks like changes are there.

	Krzysztof