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

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

On 10/12/24 16:56, Manivannan Sadhasivam wrote:
> On Fri, Oct 11, 2024 at 11:01:09AM +0900, Damien Le Moal wrote:
>> 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.
>>
> 
> Actually, there is no need to pass 'func, vfunc' as I don't think the controller
> can have different alignment requirements for each functions.
> 
> So I'm envisioning a callback like this:
> 
> 	u64 (*align_addr)(struct pci_epc *epc, u64 addr, size_t *offset, size_t *size);
> 
> And there is no need to check the error return also. Also you can avoid passing
> 'offset', as the caller can derive the offset using the mapped and unmapped
> addresses. This also avoids the extra local function and allows the callers to
> just use the callback directly.
> 
> NOTE: Please do not respin the patches without concluding the comments on
> previous revisions. I understand that you want to get the series merged asap and
> I do have the same adjective.

v5 that I posted yesterday addressed all your comment, except the one above.
The controller operation (renamed get_mem_map) still uses the pci_mem_map
structure as argument.

I need to respin a v6. Do you want me to change the controller op as you suggest
above ?

> 
> - Mani
> 


-- 
Damien Le Moal
Western Digital Research