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@xxxxxxxxx>
> >>>> Signed-off-by: Rick Wertenbroek <rick.wertenbroek@xxxxxxxxx>
> >>>> Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
> >>>
> >>> Looks good to me. Just one comment below.
> >>>
> >>>> Reviewed-by: Niklas Cassel <cassel@xxxxxxxxxx>
> >>>> ---
> >>>> 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.
- Mani
--
மணிவண்ணன் சதாசிவம்
