| Message ID | 20241218011520.2579828-1-joe@pf.is.s.u-tokyo.ac.jp (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] dmaengine: ti: edma: fix OF node reference leaks in edma_driver | expand |
On Wed, Dec 18, 2024 at 10:15:20AM +0900, Joe Hattori wrote: > drivers/dma/ti/edma.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma/ti/edma.c b/drivers/dma/ti/edma.c > index 343e986e66e7..4ece125b2ae7 100644 > --- a/drivers/dma/ti/edma.c > +++ b/drivers/dma/ti/edma.c > @@ -208,7 +208,6 @@ struct edma_desc { > struct edma_cc; > > struct edma_tc { > - struct device_node *node; > u16 id; > }; > > @@ -2460,19 +2459,19 @@ static int edma_probe(struct platform_device *pdev) > goto err_reg1; > } > > - for (i = 0;; i++) { > + for (i = 0; i < ecc->num_tc; i++) { > ret = of_parse_phandle_with_fixed_args(node, "ti,tptcs", > 1, i, &tc_args); > - if (ret || i == ecc->num_tc) I feel bad for not saying this earlier, but probably this "i < ecc->num_tc" change should be done as patch 1/2? It's sort of related because if we didn't do this then we'd have to do this we'd have to re-write it to for the i == ecc->num_tc to add another of_node_put(tc_args.np). But really it needs to be reviewed separately. It's such a weird thing, that I have to think that it was done deliberately for some reason although I can't figure out why. The rest of the patch is nice. So much simpler than v1. regards, dan carpenter
Thank you for your review. On 12/18/24 18:09, Dan Carpenter wrote: > On Wed, Dec 18, 2024 at 10:15:20AM +0900, Joe Hattori wrote: >> drivers/dma/ti/edma.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/dma/ti/edma.c b/drivers/dma/ti/edma.c >> index 343e986e66e7..4ece125b2ae7 100644 >> --- a/drivers/dma/ti/edma.c >> +++ b/drivers/dma/ti/edma.c >> @@ -208,7 +208,6 @@ struct edma_desc { >> struct edma_cc; >> >> struct edma_tc { >> - struct device_node *node; >> u16 id; >> }; >> >> @@ -2460,19 +2459,19 @@ static int edma_probe(struct platform_device *pdev) >> goto err_reg1; >> } >> >> - for (i = 0;; i++) { >> + for (i = 0; i < ecc->num_tc; i++) { >> ret = of_parse_phandle_with_fixed_args(node, "ti,tptcs", >> 1, i, &tc_args); >> - if (ret || i == ecc->num_tc) > > I feel bad for not saying this earlier, but probably this > "i < ecc->num_tc" change should be done as patch 1/2? It's sort of > related because if we didn't do this then we'd have to do this we'd > have to re-write it to for the i == ecc->num_tc to add another > of_node_put(tc_args.np). But really it needs to be reviewed > separately. It's such a weird thing, that I have to think that it > was done deliberately for some reason although I can't figure out why. Totally. Separated the loop condition change as 1/2 and the rest as 2/2 in the v3 patch. > > The rest of the patch is nice. So much simpler than v1. Thanks :) > > regards, > dan carpenter > Best, Joe
diff --git a/drivers/dma/ti/edma.c b/drivers/dma/ti/edma.c index 343e986e66e7..4ece125b2ae7 100644 --- a/drivers/dma/ti/edma.c +++ b/drivers/dma/ti/edma.c @@ -208,7 +208,6 @@ struct edma_desc { struct edma_cc; struct edma_tc { - struct device_node *node; u16 id; }; @@ -2460,19 +2459,19 @@ static int edma_probe(struct platform_device *pdev) goto err_reg1; } - for (i = 0;; i++) { + for (i = 0; i < ecc->num_tc; i++) { ret = of_parse_phandle_with_fixed_args(node, "ti,tptcs", 1, i, &tc_args); - if (ret || i == ecc->num_tc) + if (ret) break; - ecc->tc_list[i].node = tc_args.np; ecc->tc_list[i].id = i; queue_priority_mapping[i][1] = tc_args.args[0]; if (queue_priority_mapping[i][1] > lowest_priority) { lowest_priority = queue_priority_mapping[i][1]; info->default_queue = i; } + of_node_put(tc_args.np); } /* See if we have optional dma-channel-mask array */
The .probe() of edma_driver calls of_parse_phandle_with_fixed_args() but does not release the obtained OF nodes. Thus add a of_node_put() call. This bug was found by an experimental verification tool that I am developing. Fixes: 1be5336bc7ba ("dmaengine: edma: New device tree binding") Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> --- Changes in v2: - Get rid of the .node field in struct edma_tc and put the node in .probe(). --- drivers/dma/ti/edma.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)