Re: [dpdk-dev] [PATCH v25 1/6] dmadev: introduce DMA device library

[Thomas Monjalon <thomas@monjalon.net>]

15/10/2021 11:59, fengchengwen:
> On 2021/10/15 16:29, Thomas Monjalon wrote:
> > 13/10/2021 09:41, Thomas Monjalon:
> >> 13/10/2021 02:21, fengchengwen:
> >>> On 2021/10/13 3:09, Thomas Monjalon wrote:
> >>>> 11/10/2021 09:33, Chengwen Feng:
> >>>>> +static void
> >>>>> +dma_release(struct rte_dma_dev *dev)
> >>>>> +{
> >>>>> +	rte_free(dev->dev_private);
> >>>>> +	memset(dev, 0, sizeof(struct rte_dma_dev));
> >>>>> +}
> >> [...]
> >>>>> +int
> >>>>> +rte_dma_pmd_release(const char *name)
> >>>>> +{
> >>>>> +	struct rte_dma_dev *dev;
> >>>>> +
> >>>>> +	if (dma_check_name(name) != 0)
> >>>>> +		return -EINVAL;
> >>>>> +
> >>>>> +	dev = dma_find_by_name(name);
> >>>>> +	if (dev == NULL)
> >>>>> +		return -EINVAL;
> >>>>> +
> >>>>> +	dma_release(dev);
> >>>>> +	return 0;
> >>>>> +}
> >>>>
> >>>> Trying to understand the logic of creation/destroy.
> >>>> skeldma_probe
> >>>> \-> skeldma_create
> >>>>     \-> rte_dma_pmd_allocate
> >>>>         \-> dma_allocate
> >>>>             \-> dma_data_prepare
> >>>>                 \-> dma_dev_data_prepare
> >>>> skeldma_remove
> >>>> \-> skeldma_destroy
> >>>>     \-> rte_dma_pmd_release
> >>>>         \-> dma_release
> >>>
> >>> This patch only provide device allocate function, the 2st patch provide extra logic:
> >>>
> >>> 	diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
> >>> 	index 42a4693bd9..a6a5680d2b 100644
> >>> 	--- a/lib/dmadev/rte_dmadev.c
> >>> 	+++ b/lib/dmadev/rte_dmadev.c
> >>> 	@@ -201,6 +201,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);
> >>> 	+
> >>>         	dma_release(dev);
> >>> 	        return 0;
> >>> 	 }
> >>>
> >>> So the skeldma remove will be:
> >>>
> >>>  skeldma_remove
> >>>  \-> skeldma_destroy
> >>>      \-> rte_dma_pmd_release
> >>>          \-> rte_dma_close
> >>>              \-> dma_release
> >>
> >> OK, in this case, no need to dma_release from rte_dma_pmd_release,
> >> because it is already called from rte_dma_close.
> > 
> > Ping for reply please.
> 
> Sorry, I think the previous reply was enough, Let me explain:

No, if previous answer was enough, I would not add a new comment.
Please read again:
"
no need to dma_release from rte_dma_pmd_release,
because it is already called from rte_dma_close
"

> The PMD use following logic create dmadev:
>   skeldma_probe
>     \-> skeldma_create
>       \-> rte_dma_pmd_allocate
>         \-> dma_allocate
>       \-> mark dmadev state to READY.
> 
> The PMD remove will be:
>  skeldma_remove
>   \-> skeldma_destroy
>       \-> rte_dma_pmd_release
>           \-> rte_dma_close
>               \-> dma_release
> 
> The application close dmadev:
>   rte_dma_close
>    \-> dma_release
> 
> in the above case, the PMD remove and application close both call rte_dma_close,
> I think that's what you expect.
> 
> 
> skeldma is simple, please let me give you a complicated example:
>   hisi_dma_probe
>     \-> hisi_dma_create
>       \-> rte_dma_pmd_allocate
>         \-> dma_allocate
>       \-> hisi_hw_init
>         \-> if init fail, call rte_dma_pmd_release.
>             \-> dma_release
>         \-> if init OK, mark dmadev state to READY.
> 
> as you can see, if hisi_hw_init fail, it call rte_dma_pmd_release to release dmadev,
> it will direct call dma_release.
> if hisi_hw_init success, it mean the hardware also OK, then mark dmadev state to
> READY. if PMD remove the dmadev it will call rte_dma_close because the dmadev's state
> is READY, and the application could also call rte_dma_close to destroy dmadev.
> 
> 
> The rte_dma_pmd_release take two function:
> 1. if the dmadev's hardware part init fail, the PMD could use this function release the
> dmadev.
> 2. if the dmadev's hardware part init success, the PMD could use this function destroy
> the dmadev.
> 
> 
> If we don't have the rte_dma_pmd_release function, we should export dma_release function
> which invoked when the hardware init fail.
> 
> And if we keep rte_dma_pmd_release, it correspond the rte_dma_pmd_allocate, the PMD just
> invoke rte_dma_pmd_release to handle above both cases (hardware part init fail when probe
> and remove phase).

You are justifying the existence of the functions, OK,
but I am just discussing one call of the function which is useless.

Anyway, now I am in the process of merging v26,
so I will send a fix.