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:

quoted

This patch introduces 'dmadevice' which is a generic type of DMA
device.

[snip]

quoted

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

quoted

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

quoted

+
+#define RTE_DMADEV_LOG(level, fmt, args...) \

Do we need RTE_ prefix for internal API?

quoted

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

quoted

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

.