[PATCH v2 1/3] dma/ae4dma: introduce AMD AE4DMA DMA PMD

[Raghavendra Ningoji]

On Mon, 22 Jun 2026 at 14:06, David Marchand <david.marchand at redhat.com> wrote:
>
> Here is a superficial review.
>
> Many places are fishy when it comes to integer/pointer casts: I only
> raised a few comments on this topic.

Thanks for the review. I went through the cast usage as well; the
low32_value()/high32_value() helpers (which took an unsigned long and
were therefore broken on LLP64) are gone in v3, replaced by
lower_32_bits()/upper_32_bits() on the rte_iova_t value, and the
redundant index casts are removed. Replies inline.

> > +       q_mz = ae4dma_queue_dma_zone_reserve(cmd_q->memz_name,
> > +                       cmd_q->qsize, rte_socket_id());
>
> I see no tracking of q_mz, so I suspect this memzone is leaked on
> device probing failure, and/or unplugging.

The memzone is now stored in cmd_q->mz at probe time and freed directly
in dev_close(). dev_close() is reached on the unplug path too
(remove() -> rte_dma_pmd_release() -> rte_dma_close()), so the ring is
no longer leaked.

> > +       cmd_q->next_write = (uint16_t)AE4DMA_READ_REG(...);
>
> Strange that you need to cast.

Removed; next_read/next_write/last_write are uint16_t and the registers
are read into them without an explicit cast in v3.

> > +/* Create a dmadev(dpdk DMA device) */
>
> This is a general comment for the patch: let's avoid Lapalissade /
> trivial comments that adds nothing.

Removed the trivial "what" comments across the series.

> > +       struct rte_dma_dev *dmadev = NULL;
> > +       struct ae4dma_dmadev *ae4dma = NULL;
>
> Those variables do not need any explicit setting to NULL [...]

Done.

> > +       if (!name) {
>
> [...] This is a static helper called internally from a single
> location, remove the check.

Removed.

> > +       ae4dma->dmadev = dmadev;
>
> [...] this field is never used in the series. Please remove.

Removed the field and the assignment.

> > +       ae4dma->pci = dev;
>
> [...] no user of this field in the series, please remove.

Removed. ae4dma_add_queue() now takes the rte_pci_device pointer as an
argument instead.

> > +init_error:
> > +       AE4DMA_PMD_ERR("driver %s(): failed", __func__);
>
> __func__ is already part of AE4DMA_PMD_LOG.

Dropped __func__ from the message.

> > +       dev->device.driver = &drv->driver;
>
> Setting the driver pointer in the device object is not the driver
> responsibility anymore [...]. EAL will set this field on probe()
> success.

Removed; the drv argument is now __rte_unused.

> > +#ifndef __AE4DMA_HW_DEFS_H__
>
> Is this header autosufficient ? I see references to uint32_t below,
> so this header probably depends on stdint.h.

Added #include <stdint.h>.

> > +#ifdef __cplusplus
> > +extern "C" {
>
> Do we really need C++ guards?

Removed (internal header).

> > +       AE4DMA_HWQUEUE_NOT_EMPTY = 4
>
> For consistency with other enums, add a comma.

Done.

> > +/**
>
> This is an internal header, we don't need doxygen style comments,
> simple comments are enough.

Converted the doxygen comments to plain comments.

Sent as v3.

Thanks,
Raghavendra