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

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

On 10/12/24 18:41, Manivannan Sadhasivam wrote:
> On Sat, Oct 12, 2024 at 05:33:34PM +0900, Damien Le Moal wrote:
>> 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 ?
>>
> 
> Please do so. I will apply it right away as everything else looks good.

Done. Under test now but everything is looking good. Note that I kept the offset
argument as otherwise pci_epc_mem_map() needs to re-calculate it but the
controller ->align_addr() may already have calculated it (or can calculate it
more efficiently than with a substraction).

So keeping offset as an argument is cleaner I think.

I also remove the mutex lock/unlock around ->align_addr() call as I really do
not think it is necessary at all (and if needed a controller implementation of
align_addr can always take that mutex).


-- 
Damien Le Moal
Western Digital Research