Re: [PATCH v4 4/7] PCI: endpoint: Introduce pci_epc_mem_map()/unmap()

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Mon, Oct 07, 2024 at 01:03:16PM +0900, Damien Le Moal wrote:
> Introduce the function pci_epc_mem_map() to facilitate controller memory
> address allocation and mapping to a RC PCI address region in endpoint
> function drivers.
> 
> This function first uses pci_epc_map_align() to determine the controller
> memory address size (and offset into) depending on the controller
> address alignment constraints. The result of this function is used to
> allocate a controller physical memory region using
> pci_epc_mem_alloc_addr() and map that memory to the RC PCI address
> space with pci_epc_map_addr().
> 
> Since pci_epc_map_align() may indicate that the effective mapping
> of a PCI address region is smaller than the user requested size,
> pci_epc_mem_map() may only partially map the RC PCI address region
> specified. It is the responsibility of the caller (an endpoint function
> driver) to handle such smaller mapping.
> 
> The counterpart of pci_epc_mem_map() to unmap and free the controller
> memory address region is pci_epc_mem_unmap().
> 
> Both functions operate using a struct pci_epc_map data structure
> Endpoint function drivers can use struct pci_epc_map to access the
> mapped RC PCI address region using the ->virt_addr and ->pci_size
> fields.
> 
> Co-developed-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
> Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Looks good to me. Just one comment below.

> Reviewed-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/pci/endpoint/pci-epc-core.c | 78 +++++++++++++++++++++++++++++
>  include/linux/pci-epc.h             |  4 ++
>  2 files changed, 82 insertions(+)
> 
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 1adccf07c33e..d03c753d0a53 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -532,6 +532,84 @@ int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  }
>  EXPORT_SYMBOL_GPL(pci_epc_map_addr);
>  
> +/**
> + * pci_epc_mem_map() - allocate and map a PCI address to a CPU address
> + * @epc: the EPC device on which the CPU address is to be allocated and mapped
> + * @func_no: the physical endpoint function number in the EPC device
> + * @vfunc_no: the virtual endpoint function number in the physical function
> + * @pci_addr: PCI address to which the CPU address should be mapped
> + * @pci_size: the number of bytes to map starting from @pci_addr
> + * @map: where to return the mapping information
> + *
> + * Allocate a controller memory address region and map it to a RC PCI address
> + * region, taking into account the controller physical address mapping
> + * constraints using pci_epc_map_align().
> + * The effective size of the PCI address range mapped from @pci_addr is
> + * indicated by @map->pci_size. This size may be less than the requested
> + * @pci_size. The local virtual CPU address for the mapping is indicated by
> + * @map->virt_addr (@map->phys_addr indicates the physical address).
> + * The size and CPU address of the controller memory allocated and mapped are
> + * respectively indicated by @map->map_size and @map->virt_base (and
> + * @map->phys_base).
> + *
> + * Returns 0 on success and a negative error code in case of error.
> + */
> +int pci_epc_mem_map(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> +		    u64 pci_addr, size_t pci_size, struct pci_epc_map *map)
> +{
> +	int ret;
> +
> +	ret = pci_epc_map_align(epc, func_no, vfunc_no, pci_addr, pci_size, map);

I don't like the fact that one structure is passed to two functions and both
modify some members. If you get rid of the pci_epc_map_align() API and just use
the callback, then the arguments could be passed on their own without the 'map'
struct.

- Mani

-- 
மணிவண்ணன் சதாசிவம்