Re: [PATCH] dmaengine: ti: k3-udma-private: Fix refcount leak bug in of_xudma_dev_get()

["Péter Ujfalusi" <peter.ujfalusi@gmail.com>]

Hi,

On 16/07/2022 11:46, Liang He wrote:
> We should call of_node_put() for the reference returned by
> of_parse_phandle() in fail path or when it is not used anymore.
> Here we only need to move the of_node_put() before the check.

Thank you for the analysis, yes, this is a bug.

If you add the missing blank line, you can attach my:
Acked-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>

> Fixes: d70241913413 ("dmaengine: ti: k3-udma: Add glue layer for non DMAengine users")
> Signed-off-by: Liang He <windhl@126.com>
> ---
> 
> I cannot find the 'k3-udma-private.c' comiple item in drivers/dma/ti/Makefile, 
> I wonder if the author has forgotten the compile item and the
> k3-udma-private.c has not been compiled before.
> 
> I have tried to add k3-udma-private.o in the Makefile, but there are lots of
> compile errors.  
> Please check it carefully and if possbile please teach me how to compile it, thanks.

the k3-udma-private.c is included in to k3-udma.c (see end of file).
When the UDMA stack was introduced we needed to have a glue layer to
service the networking users due to the lack of infrastructure via
DMAengine.
The glue layer needs to tap in to the DMAengine driver, but I did not
wanted to expose low level interfaces, structs, ops in order to be able
to get rid of the glue layer when we have all the needed features in
DMAengine.

The k3-udma-private.c is part of the k3-udma.c but kept as separate file
to make it explicit that it is suppose to go away and to not mix it with
the DMAengine driver.

>  drivers/dma/ti/k3-udma-private.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma/ti/k3-udma-private.c b/drivers/dma/ti/k3-udma-private.c
> index d4f1e4e9603a..ec274ef7d5ea 100644
> --- a/drivers/dma/ti/k3-udma-private.c
> +++ b/drivers/dma/ti/k3-udma-private.c
> @@ -31,14 +31,13 @@ struct udma_dev *of_xudma_dev_get(struct device_node *np, const char *property)
>  	}
>  
>  	pdev = of_find_device_by_node(udma_node);
> +	if (np != udma_node)
> +		of_node_put(udma_node);

Can you add a blank line here for readability?

>  	if (!pdev) {
>  		pr_debug("UDMA device not found\n");
>  		return ERR_PTR(-EPROBE_DEFER);
>  	}
>  
> -	if (np != udma_node)
> -		of_node_put(udma_node);
> -
>  	ud = platform_get_drvdata(pdev);
>  	if (!ud) {
>  		pr_debug("UDMA has not been probed\n");

-- 
Péter