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.
> .
>
