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;
Message-ID: <5a87a5b1-270c-5756-f3f1-ecbd01c4091b@igel.co.jp>
Date:   Wed, 18 Jan 2023 19:33:51 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: [RFC PATCH 1/3] PCI: endpoint: support an alignment aware
 map/unmaping
Content-Language: en-US
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Jingoo Han <jingoohan1@gmail.com>,
        Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        Rob Herring <robh@kernel.org>,
        =?UTF-8?Q?Krzysztof_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
References: <20230117204131.GA145312@bhelgaas>
From:   Shunsuke Mie <mie@igel.co.jp>
In-Reply-To: <20230117204131.GA145312@bhelgaas>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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