| Message ID | 20230113090350.1103494-2-mie@igel.co.jp (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | Deal with alignment restriction on EP side | expand |
On Fri, Jan 13, 2023 at 06:03:48PM +0900, Shunsuke Mie wrote: > Add an align_mem operation to the EPC ops, which function is used to > pci_epc_map/unmap_addr(). These change to enable mapping for any alignment > restriction of EPC. The map function maps an aligned memory to include a > requested memory region. I think this does two things: 1) add the .align_mem() function pointer, and 2) move the pci_epc_mem_alloc_addr() call into pci_epc_map_addr(). For 2), I would expect to see pci_epc_mem_alloc_addr() being *removed* from somewhere else. Anyway, both are significant and should be mentioned in the commit log. Possibly they could even be separate commits: move the alloc/free first, then add .align_mem(). Another question below. > Signed-off-by: Shunsuke Mie <mie@igel.co.jp> > --- > drivers/pci/endpoint/pci-epc-core.c | 57 ++++++++++++++++++++++++----- > include/linux/pci-epc.h | 10 +++-- > 2 files changed, 53 insertions(+), 14 deletions(-) > > diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c > index 2542196e8c3d..60d586e05e7d 100644 > --- a/drivers/pci/endpoint/pci-epc-core.c > +++ b/drivers/pci/endpoint/pci-epc-core.c > @@ -430,8 +430,12 @@ EXPORT_SYMBOL_GPL(pci_epc_set_msix); > * Invoke to unmap the CPU address from PCI address. > */ > void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, > - phys_addr_t phys_addr) > + phys_addr_t phys_addr, void __iomem *virt_addr, size_t size) > { > + u64 aligned_phys; > + void __iomem *aligned_virt; > + size_t offset; > + > if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions) > return; > > @@ -441,9 +445,22 @@ void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, > if (!epc->ops->unmap_addr) > return; > > + if (epc->ops->align_mem) { > + mutex_lock(&epc->lock); > + aligned_phys = epc->ops->align_mem(epc, phys_addr, &size); > + mutex_unlock(&epc->lock); > + } else { > + aligned_phys = phys_addr; > + } > + > + offset = phys_addr - aligned_phys; > + aligned_virt = virt_addr - offset; > + > mutex_lock(&epc->lock); > - epc->ops->unmap_addr(epc, func_no, vfunc_no, phys_addr); > + epc->ops->unmap_addr(epc, func_no, vfunc_no, aligned_phys); > mutex_unlock(&epc->lock); > + > + pci_epc_mem_free_addr(epc, aligned_phys, aligned_virt, size); > } > EXPORT_SYMBOL_GPL(pci_epc_unmap_addr); > > @@ -458,26 +475,46 @@ EXPORT_SYMBOL_GPL(pci_epc_unmap_addr); > * > * Invoke to map CPU address with PCI address. > */ > -int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, > - phys_addr_t phys_addr, u64 pci_addr, size_t size) > +void __iomem *pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, > + u64 pci_addr, phys_addr_t *phys_addr, size_t size) > { > int ret; > + u64 aligned_addr; > + size_t offset; > + void __iomem *virt_addr; > > if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions) > - return -EINVAL; > + return ERR_PTR(-EINVAL); > > if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no])) > - return -EINVAL; > + return ERR_PTR(-EINVAL); > > if (!epc->ops->map_addr) > - return 0; > + return ERR_PTR(-ENOPTSUPP); > + > + if (epc->ops->align_mem) { > + mutex_lock(&epc->lock); > + aligned_addr = epc->ops->align_mem(epc, pci_addr, &size); > + mutex_unlock(&epc->lock); > + } else { > + aligned_addr = pci_addr; > + } > + > + offset = pci_addr - aligned_addr; > + > + virt_addr = pci_epc_mem_alloc_addr(epc, phys_addr, size); > + if (!virt_addr) > + return ERR_PTR(-ENOMEM); > > mutex_lock(&epc->lock); > - ret = epc->ops->map_addr(epc, func_no, vfunc_no, phys_addr, pci_addr, > - size); > + ret = epc->ops->map_addr(epc, func_no, vfunc_no, *phys_addr, aligned_addr, size); > mutex_unlock(&epc->lock); > + if (ret) > + return ERR_PTR(ret); > > - return ret; > + *phys_addr += offset; > + > + return virt_addr + offset; > } > EXPORT_SYMBOL_GPL(pci_epc_map_addr); > > diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h > index a48778e1a4ee..8f29161bce80 100644 > --- a/include/linux/pci-epc.h > +++ b/include/linux/pci-epc.h > @@ -84,6 +84,7 @@ struct pci_epc_ops { > phys_addr_t phys_addr, u8 interrupt_num, > u32 entry_size, u32 *msi_data, > u32 *msi_addr_offset); > + u64 (*align_mem)(struct pci_epc *epc, u64 addr, size_t *size); Is there a requirement for multiple implementations of .align_mem()? There's only one implementation in this series (dw_pcie_ep_align_mem()), and it only needs pci->region_align. That *value* might be DWC-specific, but the concept really isn't, so maybe there could be a generic function that uses the device-specific value. > int (*start)(struct pci_epc *epc); > void (*stop)(struct pci_epc *epc); > const struct pci_epc_features* (*get_features)(struct pci_epc *epc, > @@ -218,11 +219,12 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, > struct pci_epf_bar *epf_bar); > void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, > struct pci_epf_bar *epf_bar); > -int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, > - phys_addr_t phys_addr, > - u64 pci_addr, size_t size); > +void __iomem *pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, > + u64 pci_addr, phys_addr_t *phys_addr, > + size_t size); > void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, > - phys_addr_t phys_addr); > + phys_addr_t phys_addr, void __iomem *virt_addr, > + size_t size); > int pci_epc_set_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no, > u8 interrupts); > int pci_epc_get_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no); > -- > 2.25.1 >
On 2023/01/18 5:41, Bjorn Helgaas wrote: > On Fri, Jan 13, 2023 at 06:03:48PM +0900, Shunsuke Mie wrote: >> Add an align_mem operation to the EPC ops, which function is used to >> pci_epc_map/unmap_addr(). These change to enable mapping for any alignment >> restriction of EPC. The map function maps an aligned memory to include a >> requested memory region. > I think this does two things: 1) add the .align_mem() function > pointer, and 2) move the pci_epc_mem_alloc_addr() call into > pci_epc_map_addr(). For 2), I would expect to see > pci_epc_mem_alloc_addr() being *removed* from somewhere else. > > Anyway, both are significant and should be mentioned in the commit > log. Possibly they could even be separate commits: move the > alloc/free first, then add .align_mem(). I understood. I attempt to arrange commits as your mention. > > Another question below. > >> Signed-off-by: Shunsuke Mie <mie@igel.co.jp> >> --- >> drivers/pci/endpoint/pci-epc-core.c | 57 ++++++++++++++++++++++++----- >> include/linux/pci-epc.h | 10 +++-- >> 2 files changed, 53 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c >> index 2542196e8c3d..60d586e05e7d 100644 >> --- a/drivers/pci/endpoint/pci-epc-core.c >> +++ b/drivers/pci/endpoint/pci-epc-core.c >> @@ -430,8 +430,12 @@ EXPORT_SYMBOL_GPL(pci_epc_set_msix); >> * Invoke to unmap the CPU address from PCI address. >> */ >> void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, >> - phys_addr_t phys_addr) >> + phys_addr_t phys_addr, void __iomem *virt_addr, size_t size) >> { >> + u64 aligned_phys; >> + void __iomem *aligned_virt; >> + size_t offset; >> + >> if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions) >> return; >> >> @@ -441,9 +445,22 @@ void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, >> if (!epc->ops->unmap_addr) >> return; >> >> + if (epc->ops->align_mem) { >> + mutex_lock(&epc->lock); >> + aligned_phys = epc->ops->align_mem(epc, phys_addr, &size); >> + mutex_unlock(&epc->lock); >> + } else { >> + aligned_phys = phys_addr; >> + } >> + >> + offset = phys_addr - aligned_phys; >> + aligned_virt = virt_addr - offset; >> + >> mutex_lock(&epc->lock); >> - epc->ops->unmap_addr(epc, func_no, vfunc_no, phys_addr); >> + epc->ops->unmap_addr(epc, func_no, vfunc_no, aligned_phys); >> mutex_unlock(&epc->lock); >> + >> + pci_epc_mem_free_addr(epc, aligned_phys, aligned_virt, size); >> } >> EXPORT_SYMBOL_GPL(pci_epc_unmap_addr); >> >> @@ -458,26 +475,46 @@ EXPORT_SYMBOL_GPL(pci_epc_unmap_addr); >> * >> * Invoke to map CPU address with PCI address. >> */ >> -int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, >> - phys_addr_t phys_addr, u64 pci_addr, size_t size) >> +void __iomem *pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, >> + u64 pci_addr, phys_addr_t *phys_addr, size_t size) >> { >> int ret; >> + u64 aligned_addr; >> + size_t offset; >> + void __iomem *virt_addr; >> >> if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions) >> - return -EINVAL; >> + return ERR_PTR(-EINVAL); >> >> if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no])) >> - return -EINVAL; >> + return ERR_PTR(-EINVAL); >> >> if (!epc->ops->map_addr) >> - return 0; >> + return ERR_PTR(-ENOPTSUPP); >> + >> + if (epc->ops->align_mem) { >> + mutex_lock(&epc->lock); >> + aligned_addr = epc->ops->align_mem(epc, pci_addr, &size); >> + mutex_unlock(&epc->lock); >> + } else { >> + aligned_addr = pci_addr; >> + } >> + >> + offset = pci_addr - aligned_addr; >> + >> + virt_addr = pci_epc_mem_alloc_addr(epc, phys_addr, size); >> + if (!virt_addr) >> + return ERR_PTR(-ENOMEM); >> >> mutex_lock(&epc->lock); >> - ret = epc->ops->map_addr(epc, func_no, vfunc_no, phys_addr, pci_addr, >> - size); >> + ret = epc->ops->map_addr(epc, func_no, vfunc_no, *phys_addr, aligned_addr, size); >> mutex_unlock(&epc->lock); >> + if (ret) >> + return ERR_PTR(ret); >> >> - return ret; >> + *phys_addr += offset; >> + >> + return virt_addr + offset; >> } >> EXPORT_SYMBOL_GPL(pci_epc_map_addr); >> >> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h >> index a48778e1a4ee..8f29161bce80 100644 >> --- a/include/linux/pci-epc.h >> +++ b/include/linux/pci-epc.h >> @@ -84,6 +84,7 @@ struct pci_epc_ops { >> phys_addr_t phys_addr, u8 interrupt_num, >> u32 entry_size, u32 *msi_data, >> u32 *msi_addr_offset); >> + u64 (*align_mem)(struct pci_epc *epc, u64 addr, size_t *size); > Is there a requirement for multiple implementations of .align_mem()? > There's only one implementation in this series > (dw_pcie_ep_align_mem()), and it only needs pci->region_align. That > *value* might be DWC-specific, but the concept really isn't, so maybe > there could be a generic function that uses the device-specific value. That is the correct way, but some handlers require different implementation. Sorry, this patch could have been misleading. it is my fault. I'll add the other handlers to a next version. > >> int (*start)(struct pci_epc *epc); >> void (*stop)(struct pci_epc *epc); >> const struct pci_epc_features* (*get_features)(struct pci_epc *epc, >> @@ -218,11 +219,12 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, >> struct pci_epf_bar *epf_bar); >> void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, >> struct pci_epf_bar *epf_bar); >> -int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, >> - phys_addr_t phys_addr, >> - u64 pci_addr, size_t size); >> +void __iomem *pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, >> + u64 pci_addr, phys_addr_t *phys_addr, >> + size_t size); >> void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, >> - phys_addr_t phys_addr); >> + phys_addr_t phys_addr, void __iomem *virt_addr, >> + size_t size); >> int pci_epc_set_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no, >> u8 interrupts); >> int pci_epc_get_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no); >> -- >> 2.25.1 >> Best, Shunsuke
diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c index 2542196e8c3d..60d586e05e7d 100644 --- a/drivers/pci/endpoint/pci-epc-core.c +++ b/drivers/pci/endpoint/pci-epc-core.c @@ -430,8 +430,12 @@ EXPORT_SYMBOL_GPL(pci_epc_set_msix); * Invoke to unmap the CPU address from PCI address. */ void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, - phys_addr_t phys_addr) + phys_addr_t phys_addr, void __iomem *virt_addr, size_t size) { + u64 aligned_phys; + void __iomem *aligned_virt; + size_t offset; + if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions) return; @@ -441,9 +445,22 @@ void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, if (!epc->ops->unmap_addr) return; + if (epc->ops->align_mem) { + mutex_lock(&epc->lock); + aligned_phys = epc->ops->align_mem(epc, phys_addr, &size); + mutex_unlock(&epc->lock); + } else { + aligned_phys = phys_addr; + } + + offset = phys_addr - aligned_phys; + aligned_virt = virt_addr - offset; + mutex_lock(&epc->lock); - epc->ops->unmap_addr(epc, func_no, vfunc_no, phys_addr); + epc->ops->unmap_addr(epc, func_no, vfunc_no, aligned_phys); mutex_unlock(&epc->lock); + + pci_epc_mem_free_addr(epc, aligned_phys, aligned_virt, size); } EXPORT_SYMBOL_GPL(pci_epc_unmap_addr); @@ -458,26 +475,46 @@ EXPORT_SYMBOL_GPL(pci_epc_unmap_addr); * * Invoke to map CPU address with PCI address. */ -int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, - phys_addr_t phys_addr, u64 pci_addr, size_t size) +void __iomem *pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, + u64 pci_addr, phys_addr_t *phys_addr, size_t size) { int ret; + u64 aligned_addr; + size_t offset; + void __iomem *virt_addr; if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions) - return -EINVAL; + return ERR_PTR(-EINVAL); if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no])) - return -EINVAL; + return ERR_PTR(-EINVAL); if (!epc->ops->map_addr) - return 0; + return ERR_PTR(-ENOPTSUPP); + + if (epc->ops->align_mem) { + mutex_lock(&epc->lock); + aligned_addr = epc->ops->align_mem(epc, pci_addr, &size); + mutex_unlock(&epc->lock); + } else { + aligned_addr = pci_addr; + } + + offset = pci_addr - aligned_addr; + + virt_addr = pci_epc_mem_alloc_addr(epc, phys_addr, size); + if (!virt_addr) + return ERR_PTR(-ENOMEM); mutex_lock(&epc->lock); - ret = epc->ops->map_addr(epc, func_no, vfunc_no, phys_addr, pci_addr, - size); + ret = epc->ops->map_addr(epc, func_no, vfunc_no, *phys_addr, aligned_addr, size); mutex_unlock(&epc->lock); + if (ret) + return ERR_PTR(ret); - return ret; + *phys_addr += offset; + + return virt_addr + offset; } EXPORT_SYMBOL_GPL(pci_epc_map_addr); diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h index a48778e1a4ee..8f29161bce80 100644 --- a/include/linux/pci-epc.h +++ b/include/linux/pci-epc.h @@ -84,6 +84,7 @@ struct pci_epc_ops { phys_addr_t phys_addr, u8 interrupt_num, u32 entry_size, u32 *msi_data, u32 *msi_addr_offset); + u64 (*align_mem)(struct pci_epc *epc, u64 addr, size_t *size); int (*start)(struct pci_epc *epc); void (*stop)(struct pci_epc *epc); const struct pci_epc_features* (*get_features)(struct pci_epc *epc, @@ -218,11 +219,12 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, struct pci_epf_bar *epf_bar); void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, struct pci_epf_bar *epf_bar); -int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, - phys_addr_t phys_addr, - u64 pci_addr, size_t size); +void __iomem *pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, + u64 pci_addr, phys_addr_t *phys_addr, + size_t size); void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, - phys_addr_t phys_addr); + phys_addr_t phys_addr, void __iomem *virt_addr, + size_t size); int pci_epc_set_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no, u8 interrupts); int pci_epc_get_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no);
Add an align_mem operation to the EPC ops, which function is used to pci_epc_map/unmap_addr(). These change to enable mapping for any alignment restriction of EPC. The map function maps an aligned memory to include a requested memory region. Signed-off-by: Shunsuke Mie <mie@igel.co.jp> --- drivers/pci/endpoint/pci-epc-core.c | 57 ++++++++++++++++++++++++----- include/linux/pci-epc.h | 10 +++-- 2 files changed, 53 insertions(+), 14 deletions(-)