Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 17 Jan 2023 14:41:31 -0600
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Shunsuke Mie <mie@igel.co.jp>
Cc:     Jingoo Han <jingoohan1@gmail.com>,
        Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        Rob Herring <robh@kernel.org>,
        Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Manivannan Sadhasivam <mani@kernel.org>,
        Kishon Vijay Abraham I <kishon@kernel.org>,
        Kunihiko Hayashi <hayashi.kunihiko@socionext.com>,
        Hou Zhiqiang <Zhiqiang.Hou@nxp.com>,
        Frank Li <Frank.Li@nxp.com>, Li Chen <lchen@ambarella.com>,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 1/3] PCI: endpoint: support an alignment aware
 map/unmaping
Message-ID: <20230117204131.GA145312@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230113090350.1103494-2-mie@igel.co.jp>
Precedence: bulk

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
>