Re: [dpdk-dev] [PATCH v23 2/6] dmadev: add control plane function support

[Thomas Monjalon <thomas@monjalon.net>]

08/10/2021 09:55, fengchengwen:
> On 2021/10/6 18:46, Thomas Monjalon wrote:
> > 24/09/2021 12:53, Chengwen Feng:
> >> --- a/lib/dmadev/rte_dmadev.c
> >> +++ b/lib/dmadev/rte_dmadev.c
> >> @@ -218,6 +218,9 @@ rte_dma_pmd_release(const char *name)
> >>  	if (dev == NULL)
> >>  		return -EINVAL;
> >>  
> >> +	if (dev->state == RTE_DMA_DEV_READY)
> >> +		return rte_dma_close(dev->dev_id);
> > 
> > What is the logic here?
> > The only exposed function should be rte_dma_close()
> > and it should call the freeing function.
> > The API should use the dev_id. As you said somewhere else,
> > the name is only for debugging.
> > Please remove the function rte_dma_pmd_release(const char *name).
> 
> The rte_dma_pmd_release corresponding to pmd_allocate, so both use the 'const char *name' parameter.
> 
> The rte_dma_pmd_release is also used for error handling when PMD init. Therefore, a status variable
> is used here. If the device is not ready, only resources need to be released. Otherwise, the close
> interface of the driver is invoked.
> 
> For PMD, the rte_dma_pmd_release is only wrap for dev_close when remove device, it does not need to
> implement two callbacks.
> 
> If we replace rte_dma_pmd_release with rte_dma_close, then we should invoke rte_dma_close in error
> handling when PMD init, this can lead to conceptual inconsistencies because the initialization has
> not been successful.
> 
> So I think it's better keep rte_dma_pmd_release.

I will review again this logic in the next version.

> >> +/** DMA device support memory-to-memory transfer.
> >> + *
> >> + * @see struct rte_dma_info::dev_capa
> >> + */
> >> +#define RTE_DMA_CAPA_MEM_TO_MEM		RTE_BIT64(0)
> >> +/** DMA device support memory-to-device transfer.
> >> + *
> >> + * @see struct rte_dma_info::dev_capa
> >> + */
> >> +#define RTE_DMA_CAPA_MEM_TO_DEV		RTE_BIT64(1)
> > 
> > Same comment as in earlier version: please group the flags
> > in a doxygen group. Example of doxygen group:
> > https://patches.dpdk.org/project/dpdk/patch/20210830104232.598703-1-thomas@monjalon.net/
> 
> Tried, but found it didn't coexist well with multi-line comments.

What is not working? Example?
I think you didn't get what to do.
You must add a comment to give a title and group all these flags.

> >> --- a/lib/dmadev/rte_dmadev_core.h
> >> +++ b/lib/dmadev/rte_dmadev_core.h
> >> +/** @internal Used to get device information of a device. */
> >> +typedef int (*rte_dma_info_get_t)(const struct rte_dma_dev *dev,
> >> +				  struct rte_dma_info *dev_info,
> >> +				  uint32_t info_sz);
> > 
> > Please move all driver interfaces in a file dedicated to drivers.
> 
> There are three head file: rte_dmadev.h, rte_dmadev_core.h, rte_dmadev_pmd.h
> And we build the following dependency:
> 
>                 rte_dmadev.h   ---> rte_dmadev_core.h          // mainly because dataplane inline API.
>                     ^
>                     |
>            ---------------------
>            |                   |
>        Application       rte_dmadev_pmd.h
>                                ^
>                                |
>                              DMA PMD
> 
> 
> If move all driver interfaces to rte_dmadev_pmd.h from rte_dmadev_core.h, bidirectional
> dependency may exist, e.g.
> 
>                 rte_dmadev.h   ---> rte_dmadev_core.h  ---> rte_dmadev_pmd.h
>                     ^
>                     |
>            ---------------------
>            |                   |
>        Application       rte_dmadev_pmd.h
>                                ^
>                                |
>                              DMA PMD
> 
> So I think it's better keep it that way.

Please make sure only what is needed for inline is kept in the "core.h"
You should look at the current effort done by Konstanti in ethdev to hide everything.