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

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

On 10/11/24 01:43, Manivannan Sadhasivam wrote:
> 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.

That would be far too many arguments. The pci_epc functions already have many
(minimum of 3 for epc, func and vfunc). So I prefer trying to minimize that.

I removed clearing map->map_size in the unmap function. I had added that to make
that function a nop if it is called twice with for the same map. But given that
pci_epc_unmap_addr() and pci_epc_mem_free_addr() will do nothing for memory that
is not mapped/allocated, it is not super useful. Doing such double call would be
a bug in the endpoint function anyway.

-- 
Damien Le Moal
Western Digital Research