Re: [dpdk-dev] [PATCH v19 6/7] dma/skeleton: introduce skeleton dmadev driver

On 2021/9/3 23:14, Kevin Laatz wrote:

On 02/09/2021 14:13, Chengwen Feng wrote:

quoted

Skeleton dmadevice driver, on the lines of rawdev skeleton, is for
showcasing of the dmadev library.

Design of skeleton involves a virtual device which is plugged into VDEV
bus on initialization.

Also, enable compilation of dmadev skeleton drivers.

Signed-off-by: Chengwen Feng <redacted>
---
  MAINTAINERS                            |   1 +
  drivers/dma/meson.build                |  11 +
  drivers/dma/skeleton/meson.build       |   7 +
  drivers/dma/skeleton/skeleton_dmadev.c | 601 +++++++++++++++++++++++++++++++++
  drivers/dma/skeleton/skeleton_dmadev.h |  59 ++++
  drivers/dma/skeleton/version.map       |   3 +
  drivers/meson.build                    |   1 +
  7 files changed, 683 insertions(+)
  create mode 100644 drivers/dma/meson.build
  create mode 100644 drivers/dma/skeleton/meson.build
  create mode 100644 drivers/dma/skeleton/skeleton_dmadev.c
  create mode 100644 drivers/dma/skeleton/skeleton_dmadev.h
  create mode 100644 drivers/dma/skeleton/version.map

snip

quoted

+
+static int
+vchan_setup(struct skeldma_hw *hw, uint16_t nb_desc)
+{
+    struct skeldma_desc *desc;
+    struct rte_ring *empty;
+    struct rte_ring *pending;
+    struct rte_ring *running;
+    struct rte_ring *completed;
+    uint16_t i;
+
+    desc = rte_zmalloc_socket("dma_skelteon_desc",
+                  nb_desc * sizeof(struct skeldma_desc),
+                  RTE_CACHE_LINE_SIZE, hw->socket_id);
+    if (desc == NULL) {
+        SKELDMA_LOG(ERR, "Malloc dma skeleton desc fail!");
+        return -ENOMEM;
+    }
+
+    empty = rte_ring_create("dma_skeleton_desc_empty", nb_desc,
+                hw->socket_id, RING_F_SP_ENQ | RING_F_SC_DEQ);
+    pending = rte_ring_create("dma_skeleton_desc_pending", nb_desc,
+                  hw->socket_id, RING_F_SP_ENQ | RING_F_SC_DEQ);
+    running = rte_ring_create("dma_skeleton_desc_running", nb_desc,
+                  hw->socket_id, RING_F_SP_ENQ | RING_F_SC_DEQ);
+    completed = rte_ring_create("dma_skeleton_desc_completed", nb_desc,
+                  hw->socket_id, RING_F_SP_ENQ | RING_F_SC_DEQ);
+    if (empty == NULL || pending == NULL || running == NULL ||
+        completed == NULL) {
+        SKELDMA_LOG(ERR, "Create dma skeleton desc ring fail!");
+        rte_ring_free(empty);
+        rte_ring_free(pending);
+        rte_ring_free(running);
+        rte_ring_free(completed);
+        rte_free(desc);

These pointers should be set to NULL after free'ing, similar to what you have in "vchan_release()".

These pointers are local variables, Need to clean them up?

The set to NULL operation in 'vhcan_release', is because those
pointers are held by dmadev with a longer life cycle.

Thanks

quoted

+        return -ENOMEM;
+    }
+
+    /* The real usable ring size is *count-1* instead of *count* to
+     * differentiate a free ring from an empty ring.
+     * @see rte_ring_create
+     */
+    for (i = 0; i < nb_desc - 1; i++)
+        (void)rte_ring_enqueue(empty, (void *)(desc + i));
+
+    hw->desc_mem = desc;
+    hw->desc_empty = empty;
+    hw->desc_pending = pending;
+    hw->desc_running = running;
+    hw->desc_completed = completed;
+
+    return 0;
+}
+
+static void
+vchan_release(struct skeldma_hw *hw)
+{
+    if (hw->desc_mem == NULL)
+        return;
+
+    rte_free(hw->desc_mem);
+    hw->desc_mem = NULL;
+    rte_ring_free(hw->desc_empty);
+    hw->desc_empty = NULL;
+    rte_ring_free(hw->desc_pending);
+    hw->desc_pending = NULL;
+    rte_ring_free(hw->desc_running);
+    hw->desc_running = NULL;
+    rte_ring_free(hw->desc_completed);
+    hw->desc_completed = NULL;
+}
+

<snip>

With the minor comments above addressed,

Reviewed-by: Kevin Laatz <redacted>



.