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

["Liang He" <windhl@126.com>]

At 2022-07-20 14:39:56, "Péter Ujfalusi" <peter.ujfalusi@gmail.com> wrote:
>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>
>

Thanks very much for your effort to review my patch.

I will do that in new version soon.

>> 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.
>

Thanks for you explaination, this is a great lesson!

>>  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?
>

Thanks, I will do it.

>>  	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