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@xxxxxxxxxx>
> ---
> 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
>
