* [PATCH] soc: qcom: smp2p: Add of_node_put() at goto
@ 2019-08-04 16:25 Nishka Dasgupta
2019-08-05 22:58 ` Bjorn Andersson
0 siblings, 1 reply; 2+ messages in thread
From: Nishka Dasgupta @ 2019-08-04 16:25 UTC (permalink / raw)
To: agross, linux-arm-msm, linux-kernel; +Cc: Nishka Dasgupta
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.
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);
unwind_interfaces:
list_for_each_entry(entry, &smp2p->inbound, node)
irq_domain_remove(entry->domain);
--
2.19.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] soc: qcom: smp2p: Add of_node_put() at goto
2019-08-04 16:25 [PATCH] soc: qcom: smp2p: Add of_node_put() at goto Nishka Dasgupta
@ 2019-08-05 22:58 ` Bjorn Andersson
0 siblings, 0 replies; 2+ messages in thread
From: Bjorn Andersson @ 2019-08-05 22:58 UTC (permalink / raw)
To: Nishka Dasgupta; +Cc: agross, linux-arm-msm, linux-kernel
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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-08-05 22:58 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-04 16:25 [PATCH] soc: qcom: smp2p: Add of_node_put() at goto Nishka Dasgupta
2019-08-05 22:58 ` Bjorn Andersson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).