Messages in this thread | | | Date | Mon, 5 Aug 2019 15:58:10 -0700 | From | Bjorn Andersson <> | Subject | Re: [PATCH] soc: qcom: smp2p: Add of_node_put() at goto |
| |
On Sun 04 Aug 09:25 PDT 2019, Nishka Dasgupta wrote:
> Each iteration of for_each_available_child_of_node puts the previous > node, but in the case of a goto from the middle of the loop, there is no > put, thus causing a memory leak. Hence make the gotos within the loop > first go to a new label where an of_node_put() puts the last used node, > before falling through to the original label. > Issue found with Coccinelle. >
Good catch, thanks for the patch!
> Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com> > --- > drivers/soc/qcom/smp2p.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c > index c7300d54e444..d223e914487d 100644 > --- a/drivers/soc/qcom/smp2p.c > +++ b/drivers/soc/qcom/smp2p.c > @@ -501,7 +501,7 @@ static int qcom_smp2p_probe(struct platform_device *pdev) > entry = devm_kzalloc(&pdev->dev, sizeof(*entry), GFP_KERNEL); > if (!entry) { > ret = -ENOMEM; > - goto unwind_interfaces; > + goto release_child; > } > > entry->smp2p = smp2p; > @@ -509,18 +509,18 @@ static int qcom_smp2p_probe(struct platform_device *pdev) > > ret = of_property_read_string(node, "qcom,entry-name", &entry->name); > if (ret < 0) > - goto unwind_interfaces; > + goto release_child; > > if (of_property_read_bool(node, "interrupt-controller")) { > ret = qcom_smp2p_inbound_entry(smp2p, entry, node); > if (ret < 0) > - goto unwind_interfaces; > + goto release_child; > > list_add(&entry->node, &smp2p->inbound); > } else { > ret = qcom_smp2p_outbound_entry(smp2p, entry, node); > if (ret < 0) > - goto unwind_interfaces; > + goto release_child; > > list_add(&entry->node, &smp2p->outbound); > } > @@ -541,6 +541,8 @@ static int qcom_smp2p_probe(struct platform_device *pdev) > > return 0; > > +release_child: > + of_node_put(node);
Following the loop over the children we request the irq and if that falls we'll jump to unwind_interfaces. So while it would work fine to jump to release_child within the loop and then unwind_interfaces after the loop, it doesn't follow the idiomatic way of using the error path to "unroll" things that has been setup up until a particular point in the function.
So I would rather see that you of_node_put() in the loop and then jump to unwind_interfaces as is done today.
Regards, Bjorn
> unwind_interfaces: > list_for_each_entry(entry, &smp2p->inbound, node) > irq_domain_remove(entry->domain); > -- > 2.19.1 >
| |