Re: [dpdk-dev] [PATCH] dmadev: introduce DMA device library

[fengchengwen <fengchengwen@huawei.com>]

Many thanks, mostly OK, a few comment inline

On 2021/7/4 22:57, Andrew Rybchenko wrote:
> On 7/2/21 4:18 PM, Chengwen Feng wrote:
>> This patch introduces 'dmadevice' which is a generic type of DMA
>> device.

[snip]

>> +#ifndef _RTE_DMADEV_CORE_H_
>> +#define _RTE_DMADEV_CORE_H_
>> +
>> +/**
>> + * @file
>> + *
>> + * RTE DMA Device internal header.
>> + *
>> + * This header contains internal data types. But they are still part of the
>> + * public API because they are used by inline public functions.
> 
> Do we really want it? Anyway rte_dmadev must not be here.
> Some sub-structure could be, but not entire rte_dmadev.
> 

struct rte_dmadev should expose to public for device probe and etc.
and because the public dataplane function use static inline to embellish,
should put the rte_dmadevices to public file too.

PS: it widely used in eth/regexdev...

>> +
>> +extern struct rte_dmadev rte_dmadevices[];
>> +
>> +#endif /* _RTE_DMADEV_CORE_H_ */
>> diff --git a/lib/dmadev/rte_dmadev_pmd.h b/lib/dmadev/rte_dmadev_pmd.h
> 
> Let's remove rte_ prefix from DPDK internal headers.

as above explained, it's public header file.

>> +
>> +#define RTE_DMADEV_LOG(level, fmt, args...) \
> 
> Do we need RTE_ prefix for internal API?
> 
>> +	rte_log(RTE_LOG_ ## level, libdmadev_logtype, "%s(): " fmt "\n", \
>> +		__func__, ##args)
>> +
>> +/* Macros to check for valid device */
>> +#define RTE_DMADEV_VALID_DEVID_OR_ERR_RET(dev_id, retval) do { \
>> +	if (!rte_dmadev_pmd_is_valid_dev((dev_id))) { \
>> +		RTE_DMADEV_LOG(ERR, "Invalid dev_id=%d", dev_id); \
>> +		return retval; \
>> +	} \
>> +} while (0)
>> +
>> +#define RTE_DMADEV_VALID_DEVID_OR_RET(dev_id) do { \
>> +	if (!rte_dmadev_pmd_is_valid_dev((dev_id))) { \
>> +		RTE_DMADEV_LOG(ERR, "Invalid dev_id=%d", dev_id); \
>> +		return; \
>> +	} \
>> +} while (0)
>> +
>> +#define RTE_DMADEV_DETACHED  0
>> +#define RTE_DMADEV_ATTACHED  1
> 
> Do we really need RTE_ prefix for interlal defines?

with RTE_ prefix will reduce namespace conflicts.

it's same as it lib/eth or regexdev...

>> +typedef int (*dmadev_xstats_reset_t)(struct rte_dmadev *dev,
>> +				     const uint32_t ids[], uint32_t nb_ids);
>> +/**< @internal Function used to reset extended stats. */
> 
> Do we really need both stats and xstats from the very
> beginning? I think it is better to start from just
> generic stats and add xstats when it is really required.

OK, but I think we should add one dump ops, which could be useful to
find the problem.

> .
>