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.
- Mani
--
மணிவண்ணன் சதாசிவம்
