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

[fengchengwen]

On 2021/9/3 23:14, Kevin Laatz wrote:
> On 02/09/2021 14:13, Chengwen Feng wrote:
>> 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 <fengchengwen at huawei.com>
>> ---
>>   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

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

> 
>> +        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 <kevin.laatz at intel.com>
> 
> 
> 
> .