| Message ID | 20241007040319.157412-5-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve PCI memory mapping API | expand |
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. - Mani
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. I removed clearing map->map_size in the unmap function. I had added that to make that function a nop if it is called twice with for the same map. But given that pci_epc_unmap_addr() and pci_epc_mem_free_addr() will do nothing for memory that is not mapped/allocated, it is not super useful. Doing such double call would be a bug in the endpoint function anyway.
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. - Mani
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 >
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. - Mani
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).
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); + if (ret) + return ret; + + map->virt_base = pci_epc_mem_alloc_addr(epc, &map->phys_base, + map->map_size); + if (!map->virt_base) + return -ENOMEM; + + map->phys_addr = map->phys_base + map->map_ofst; + map->virt_addr = map->virt_base + map->map_ofst; + + ret = pci_epc_map_addr(epc, func_no, vfunc_no, map->phys_base, + map->map_pci_addr, map->map_size); + if (ret) { + pci_epc_mem_free_addr(epc, map->phys_base, map->virt_base, + map->map_size); + map->virt_base = 0; + return ret; + } + + return 0; +} +EXPORT_SYMBOL_GPL(pci_epc_mem_map); + +/** + * pci_epc_mem_unmap() - unmap and free a CPU address region + * @epc: the EPC device on which the CPU address is 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 + * @map: the mapping information + * + * Unmap and free a CPU address region that was allocated and mapped with + * pci_epc_mem_map(). + */ +void pci_epc_mem_unmap(struct pci_epc *epc, u8 func_no, u8 vfunc_no, + struct pci_epc_map *map) +{ + if (!pci_epc_function_is_valid(epc, func_no, vfunc_no)) + return; + + if (!map || !map->virt_base) + return; + + pci_epc_unmap_addr(epc, func_no, vfunc_no, map->phys_base); + pci_epc_mem_free_addr(epc, map->phys_base, map->virt_base, + map->map_size); + map->map_size = 0; +} +EXPORT_SYMBOL_GPL(pci_epc_mem_unmap); + /** * pci_epc_clear_bar() - reset the BAR * @epc: the EPC device for which the BAR has to be cleared diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h index 9df8a83e8d10..97d2fbb740fd 100644 --- a/include/linux/pci-epc.h +++ b/include/linux/pci-epc.h @@ -315,6 +315,10 @@ void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc, phys_addr_t *phys_addr, size_t size); void pci_epc_mem_free_addr(struct pci_epc *epc, phys_addr_t phys_addr, void __iomem *virt_addr, size_t size); +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); +void pci_epc_mem_unmap(struct pci_epc *epc, u8 func_no, u8 vfunc_no, + struct pci_epc_map *map); #else static inline void pci_epc_init_notify(struct pci_epc *epc)