From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
	by inbox.dpdk.org (Postfix) with ESMTP id 688F1A034F;
	Fri,  8 Oct 2021 09:55:20 +0200 (CEST)
Received: from [217.70.189.124] (localhost [127.0.0.1])
	by mails.dpdk.org (Postfix) with ESMTP id E8A3340140;
	Fri,  8 Oct 2021 09:55:19 +0200 (CEST)
Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188])
 by mails.dpdk.org (Postfix) with ESMTP id 1D60B40040
 for <dev@dpdk.org>; Fri,  8 Oct 2021 09:55:18 +0200 (CEST)
Received: from dggemv704-chm.china.huawei.com (unknown [172.30.72.54])
 by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4HQgNW6K3wzbcw6;
 Fri,  8 Oct 2021 15:50:51 +0800 (CST)
Received: from dggpeml500024.china.huawei.com (7.185.36.10) by
 dggemv704-chm.china.huawei.com (10.3.19.47) with Microsoft SMTP Server
 (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
 15.1.2308.8; Fri, 8 Oct 2021 15:55:16 +0800
Received: from [127.0.0.1] (10.67.100.224) by dggpeml500024.china.huawei.com
 (7.185.36.10) with Microsoft SMTP Server (version=TLS1_2,
 cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.8; Fri, 8 Oct 2021
 15:55:15 +0800
To: Thomas Monjalon <thomas@monjalon.net>
CC: <ferruh.yigit@intel.com>, <bruce.richardson@intel.com>,
 <jerinj@marvell.com>, <jerinjacobk@gmail.com>,
 <andrew.rybchenko@oktetlabs.ru>, <dev@dpdk.org>, <mb@smartsharesystems.com>,
 <nipun.gupta@nxp.com>, <hemant.agrawal@nxp.com>,
 <maxime.coquelin@redhat.com>, <honnappa.nagarahalli@arm.com>,
 <david.marchand@redhat.com>, <sburla@marvell.com>, <pkapoor@marvell.com>,
 <konstantin.ananyev@intel.com>, <conor.walsh@intel.com>,
 <kevin.laatz@intel.com>
References: <1625231891-2963-1-git-send-email-fengchengwen@huawei.com>
 <20210924105357.15386-1-fengchengwen@huawei.com>
 <20210924105357.15386-3-fengchengwen@huawei.com> <11425429.3DvE4F450j@thomas>
From: fengchengwen <fengchengwen@huawei.com>
Message-ID: <f1168d4d-c698-4d4b-d47e-2663214575df@huawei.com>
Date: Fri, 8 Oct 2021 15:55:15 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.11.0
MIME-Version: 1.0
In-Reply-To: <11425429.3DvE4F450j@thomas>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
X-Originating-IP: [10.67.100.224]
X-ClientProxiedBy: dggems702-chm.china.huawei.com (10.3.19.179) To
 dggpeml500024.china.huawei.com (7.185.36.10)
X-CFilter-Loop: Reflected
Subject: Re: [dpdk-dev] [PATCH v23 2/6] dmadev: add control plane function
 support
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

On 2021/10/6 18:46, Thomas Monjalon wrote:
> 24/09/2021 12:53, Chengwen Feng:
>> --- a/doc/guides/prog_guide/dmadev.rst
>> +++ b/doc/guides/prog_guide/dmadev.rst
>> @@ -62,3 +62,44 @@ identifiers:
>>  
>>  - A device name used to designate the DMA device in console messages, for
>>    administration or debugging purposes.

[snip]

> 
>> --- a/lib/dmadev/rte_dmadev.c
>> +++ b/lib/dmadev/rte_dmadev.c
>> @@ -218,6 +218,9 @@ rte_dma_pmd_release(const char *name)
>>  	if (dev == NULL)
>>  		return -EINVAL;
>>  
>> +	if (dev->state == RTE_DMA_DEV_READY)
>> +		return rte_dma_close(dev->dev_id);
> 
> What is the logic here?
> The only exposed function should be rte_dma_close()
> and it should call the freeing function.
> The API should use the dev_id. As you said somewhere else,
> the name is only for debugging.
> Please remove the function rte_dma_pmd_release(const char *name).

The rte_dma_pmd_release corresponding to pmd_allocate, so both use the 'const char *name' parameter.

The rte_dma_pmd_release is also used for error handling when PMD init. Therefore, a status variable
is used here. If the device is not ready, only resources need to be released. Otherwise, the close
interface of the driver is invoked.

For PMD, the rte_dma_pmd_release is only wrap for dev_close when remove device, it does not need to
implement two callbacks.

If we replace rte_dma_pmd_release with rte_dma_close, then we should invoke rte_dma_close in error
handling when PMD init, this can lead to conceptual inconsistencies because the initialization has
not been successful.

So I think it's better keep rte_dma_pmd_release.

> 
> [...]
>> --- a/lib/dmadev/rte_dmadev.h
>> +++ b/lib/dmadev/rte_dmadev.h
>> + * The functions exported by the dmadev API to setup a device designated by its
>> + * device identifier must be invoked in the following order:
>> + *     - rte_dma_configure()
>> + *     - rte_dma_vchan_setup()
>> + *     - rte_dma_start()
>> + *
>> + * If the application wants to change the configuration (i.e. invoke
>> + * rte_dma_configure() or rte_dma_vchan_setup()), it must invoke
>> + * rte_dma_stop() first to stop the device and then do the reconfiguration
>> + * before invoking rte_dma_start() again. The dataplane functions should not
>> + * be invoked when the device is stopped.
>> + *
>> + * Finally, an application can close a dmadev by invoking the rte_dma_close()
>> + * function.
> 
> Yes rte_dma_close, not rte_dma_pmd_release.
> 
>> + *
>> + * About MT-safe, all the functions of the dmadev API exported by a PMD are
> 
> API is not exported by a PMD, but implemented.
> 
>> + * lock-free functions which assume to not be invoked in parallel on different
>> + * logical cores to work on the same target dmadev object.
>> + * @note Different virtual DMA channels on the same dmadev *DO NOT* support
>> + * parallel invocation because these virtual DMA channels share the same
>> + * HW-DMA-channel.
>> + *
>>   */
> 
> No need of final blank line in a comment.
> 
>> +/** DMA device support memory-to-memory transfer.
>> + *
>> + * @see struct rte_dma_info::dev_capa
>> + */
>> +#define RTE_DMA_CAPA_MEM_TO_MEM		RTE_BIT64(0)
>> +/** DMA device support memory-to-device transfer.
>> + *
>> + * @see struct rte_dma_info::dev_capa
>> + */
>> +#define RTE_DMA_CAPA_MEM_TO_DEV		RTE_BIT64(1)
> 
> Same comment as in earlier version: please group the flags
> in a doxygen group. Example of doxygen group:
> https://patches.dpdk.org/project/dpdk/patch/20210830104232.598703-1-thomas@monjalon.net/

Tried, but found it didn't coexist well with multi-line comments.

> 
> [...]
> You are using uint64_t bitfields and anonymous union in below struct,
> it may not compile if not using __extension__ from RTE_STD_C11.
> 
>> +struct rte_dma_port_param {
>> +	/** The device access port type.
>> +	 *
>> +	 * @see enum rte_dma_port_type
>> +	 */
>> +	enum rte_dma_port_type port_type;
>> +	union {
> [...]
>> +		struct {
>> +			uint64_t coreid : 4; /**< PCIe core id used. */
>> +			uint64_t pfid : 8; /**< PF id used. */
>> +			uint64_t vfen : 1; /**< VF enable bit. */
>> +			uint64_t vfid : 16; /**< VF id used. */
>> +			/** The pasid filed in TLP packet. */
>> +			uint64_t pasid : 20;
>> +			/** The attributes filed in TLP packet. */
>> +			uint64_t attr : 3;
>> +			/** The processing hint filed in TLP packet. */
>> +			uint64_t ph : 2;
>> +			/** The steering tag filed in TLP packet. */
>> +			uint64_t st : 16;
>> +		} pcie;
>> +	};
>> +	uint64_t reserved[2]; /**< Reserved for future fields. */
>> +};
> 
>> --- a/lib/dmadev/rte_dmadev_core.h
>> +++ b/lib/dmadev/rte_dmadev_core.h
>> +/** @internal Used to get device information of a device. */
>> +typedef int (*rte_dma_info_get_t)(const struct rte_dma_dev *dev,
>> +				  struct rte_dma_info *dev_info,
>> +				  uint32_t info_sz);
> 
> Please move all driver interfaces in a file dedicated to drivers.

There are three head file: rte_dmadev.h, rte_dmadev_core.h, rte_dmadev_pmd.h
And we build the following dependency:

                rte_dmadev.h   ---> rte_dmadev_core.h          // mainly because dataplane inline API.
                    ^
                    |
           ---------------------
           |                   |
       Application       rte_dmadev_pmd.h
                               ^
                               |
                             DMA PMD


If move all driver interfaces to rte_dmadev_pmd.h from rte_dmadev_core.h, bidirectional
dependency may exist, e.g.

                rte_dmadev.h   ---> rte_dmadev_core.h  ---> rte_dmadev_pmd.h
                    ^
                    |
           ---------------------
           |                   |
       Application       rte_dmadev_pmd.h
                               ^
                               |
                             DMA PMD

So I think it's better keep it that way.

> 
> [...]
>> @@ -40,9 +96,13 @@ struct rte_dma_dev {
>>  	int16_t dev_id; /**< Device [external] identifier. */
>>  	int16_t numa_node; /**< Local NUMA memory ID. -1 if unknown. */
>>  	void *dev_private; /**< PMD-specific private data. */
>> +	/** Functions exported by PMD. */
> 
> s/exported/implemented/
> 
>> +	const struct rte_dma_dev_ops *dev_ops;
>> +	struct rte_dma_conf dev_conf; /**< DMA device configuration. */
>>  	/** Device info which supplied during device initialization. */
>>  	struct rte_device *device;
>>  	enum rte_dma_dev_state state; /**< Flag indicating the device state. */
>> +	uint8_t dev_started : 1; /**< Device state: STARTED(1)/STOPPED(0). */
>>  	uint64_t reserved[2]; /**< Reserved for future fields. */
>>  } __rte_cache_aligned;
> 
> 
> 
> 
> .
> 

Thanks