On Fri, 27 Jun 2026 at 08:01, fengchengwen <[email protected]> wrote:
>
> > + Copyright(c) 2025 Advanced Micro Devices, Inc.
>
> 2025 -> 2026?
Fixed in v4 (ae4dma.rst and meson.build now both say 2026).
> > + return rte_memzone_reserve_aligned(queue_name, queue_size,
> > + socket_id, RTE_MEMZONE_IOVA_CONTIG, queue_size);
>
> No need to do such reuse, and this resource could setup in vchan_setup ops,
> but your dmadev has max 32 descriptors and only 1 vchan per-dmadev, so I think
> it's ok to setup in the probe.
Agreed. The memzone lookup/reuse branch is removed in v4; the ring is
reserved once at probe as you suggest.
> > + char hwq_dev_name[RTE_DEV_NAME_MAX_LEN];
>
> Please define local variables in a descending order, with longer ones
> placed at the front. It is recommended to modify the entire driver in
> this way.
Done. Local declarations are now ordered longest-first across the driver
(kept in init-dependency order where one local is computed from another).
> > + memset(hwq_dev_name, 0, sizeof(hwq_dev_name));
>
> why not char hwq_dev_name[RTE_DEV_NAME_MAX_LEN] = {0};
Changed to the initialiser form; the memset is gone.
> > + AE4DMA_PMD_ERR("failed");
>
> why not add more info, e.g. Probe failed!
Now logs "Failed to create dmadev %s" with the channel name.
> > +#include <rte_bus_pci.h>
> > +#include <rte_byteorder.h>
> > +#include <rte_io.h>
> > +#include <rte_pci.h>
> > +#include <rte_memzone.h>
>
> Some of the include file are not need for this head-file.
Trimmed: ae4dma_hw_defs.h now includes only <stdint.h> and
<rte_bitops.h> (for RTE_GENMASK64). The PCI/io/byteorder/memzone
includes moved to ae4dma_internal.h, which is what actually uses them.
> > +#define AE4DMA_QUEUE_SIZE(n) (AE4DMA_DESCRIPTORS_PER_CMDQ * (n))
> > +
> > +
> two blank lines
The double blank lines in ae4dma_internal.h are collapsed to one (all
three occurrences).
> > +# Copyright 2024 Advanced Micro Devices, Inc. All rights reserved.
> 2024 -> 2026
>
> Does this also support run BSD or Windows, if not please add following
> instruments:
> if not is_linux
> build = false
> reason = 'only supported on Linux'
> subdir_done()
> endif
Both done in v4: copyright is 2026 and the is_linux guard is added
before the x86 check.
Thanks,
Raghavendra
