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