In mvebu_mbus_dt_init(), of_find_matching_node_and_match() and
of_find_node_by_phandle() will both return node pointers with
refcount incremented. We should use of_node_put() in fail path
or when it is not used anymore.
Signed-off-by: Liang He <[email protected]>
---
changelog:
v2: (1) use real name (2) add of_node_put when not used anymore
v1: add of_node_put in fail path
drivers/bus/mvebu-mbus.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index db612045616f..e168c8de2ae8 100644
--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -1327,22 +1327,28 @@ int __init mvebu_mbus_dt_init(bool is_coherent)
prop = of_get_property(np, "controller", NULL);
if (!prop) {
+ of_node_put(np);
pr_err("required 'controller' property missing\n");
return -EINVAL;
}
controller = of_find_node_by_phandle(be32_to_cpup(prop));
if (!controller) {
+ of_node_put(np);
pr_err("could not find an 'mbus-controller' node\n");
return -ENODEV;
}
if (of_address_to_resource(controller, 0, &mbuswins_res)) {
+ of_node_put(np);
+ of_node_put(controller);
pr_err("cannot get MBUS register address\n");
return -EINVAL;
}
if (of_address_to_resource(controller, 1, &sdramwins_res)) {
+ of_node_put(np);
+ of_node_put(controller);
pr_err("cannot get SDRAM register address\n");
return -EINVAL;
}
@@ -1360,6 +1366,8 @@ int __init mvebu_mbus_dt_init(bool is_coherent)
pr_warn(FW_WARN "deprecated mbus-mvebu Device Tree, suspend/resume will not work\n");
}
+ of_node_put(controller);
+
mbus_state.hw_io_coherency = is_coherent;
/* Get optional pcie-{mem,io}-aperture properties */
@@ -1378,6 +1386,11 @@ int __init mvebu_mbus_dt_init(bool is_coherent)
return ret;
/* Setup statically declared windows in the DT */
- return mbus_dt_setup(&mbus_state, np);
+ ret = mbus_dt_setup(&mbus_state, np);
+
+ of_node_put(np);
+
+ return ret;
+
}
#endif
--
2.25.1
On Thursday 16 June 2022 10:01:35 Liang He wrote:
> In mvebu_mbus_dt_init(), of_find_matching_node_and_match() and
> of_find_node_by_phandle() will both return node pointers with
> refcount incremented. We should use of_node_put() in fail path
> or when it is not used anymore.
>
> Signed-off-by: Liang He <[email protected]>
Acked-by: Pali Rohár <[email protected]>
> ---
> changelog:
>
> v2: (1) use real name (2) add of_node_put when not used anymore
>
> v1: add of_node_put in fail path
>
>
> drivers/bus/mvebu-mbus.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
> index db612045616f..e168c8de2ae8 100644
> --- a/drivers/bus/mvebu-mbus.c
> +++ b/drivers/bus/mvebu-mbus.c
> @@ -1327,22 +1327,28 @@ int __init mvebu_mbus_dt_init(bool is_coherent)
>
> prop = of_get_property(np, "controller", NULL);
> if (!prop) {
> + of_node_put(np);
> pr_err("required 'controller' property missing\n");
> return -EINVAL;
> }
>
> controller = of_find_node_by_phandle(be32_to_cpup(prop));
> if (!controller) {
> + of_node_put(np);
> pr_err("could not find an 'mbus-controller' node\n");
> return -ENODEV;
> }
>
> if (of_address_to_resource(controller, 0, &mbuswins_res)) {
> + of_node_put(np);
> + of_node_put(controller);
> pr_err("cannot get MBUS register address\n");
> return -EINVAL;
> }
>
> if (of_address_to_resource(controller, 1, &sdramwins_res)) {
> + of_node_put(np);
> + of_node_put(controller);
> pr_err("cannot get SDRAM register address\n");
> return -EINVAL;
> }
> @@ -1360,6 +1366,8 @@ int __init mvebu_mbus_dt_init(bool is_coherent)
> pr_warn(FW_WARN "deprecated mbus-mvebu Device Tree, suspend/resume will not work\n");
> }
>
> + of_node_put(controller);
> +
> mbus_state.hw_io_coherency = is_coherent;
>
> /* Get optional pcie-{mem,io}-aperture properties */
> @@ -1378,6 +1386,11 @@ int __init mvebu_mbus_dt_init(bool is_coherent)
> return ret;
>
> /* Setup statically declared windows in the DT */
> - return mbus_dt_setup(&mbus_state, np);
> + ret = mbus_dt_setup(&mbus_state, np);
> +
> + of_node_put(np);
> +
> + return ret;
> +
> }
> #endif
> --
> 2.25.1
>
Le 16/06/2022 à 04:01, Liang He a écrit :
> In mvebu_mbus_dt_init(), of_find_matching_node_and_match() and
> of_find_node_by_phandle() will both return node pointers with
> refcount incremented. We should use of_node_put() in fail path
> or when it is not used anymore.
>
> Signed-off-by: Liang He <[email protected]>
> ---
> changelog:
>
> v2: (1) use real name (2) add of_node_put when not used anymore
>
> v1: add of_node_put in fail path
>
>
> drivers/bus/mvebu-mbus.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
> index db612045616f..e168c8de2ae8 100644
> --- a/drivers/bus/mvebu-mbus.c
> +++ b/drivers/bus/mvebu-mbus.c
> @@ -1327,22 +1327,28 @@ int __init mvebu_mbus_dt_init(bool is_coherent)
>
> prop = of_get_property(np, "controller", NULL);
> if (!prop) {
> + of_node_put(np);
> pr_err("required 'controller' property missing\n");
> return -EINVAL;
> }
>
> controller = of_find_node_by_phandle(be32_to_cpup(prop));
> if (!controller) {
> + of_node_put(np);
> pr_err("could not find an 'mbus-controller' node\n");
> return -ENODEV;
> }
>
> if (of_address_to_resource(controller, 0, &mbuswins_res)) {
> + of_node_put(np);
> + of_node_put(controller);
> pr_err("cannot get MBUS register address\n");
> return -EINVAL;
> }
>
> if (of_address_to_resource(controller, 1, &sdramwins_res)) {
> + of_node_put(np);
> + of_node_put(controller);
> pr_err("cannot get SDRAM register address\n");
> return -EINVAL;
> }
> @@ -1360,6 +1366,8 @@ int __init mvebu_mbus_dt_init(bool is_coherent)
> pr_warn(FW_WARN "deprecated mbus-mvebu Device Tree, suspend/resume will not work\n");
> }
>
> + of_node_put(controller);
> +
> mbus_state.hw_io_coherency = is_coherent;
>
> /* Get optional pcie-{mem,io}-aperture properties */
> @@ -1378,6 +1386,11 @@ int __init mvebu_mbus_dt_init(bool is_coherent)
> return ret;
I guess that a "of_node_put(np);" is missing before this return.
This really looks like an error handling path and it has not been
updated as all the other path, including the normal one below.
Moreover having an error handling path and some gotos is a more usual
solution. It avoids code duplication. (but it is also a matter of taste...)
CJ
>
> /* Setup statically declared windows in the DT */
> - return mbus_dt_setup(&mbus_state, np);
> + ret = mbus_dt_setup(&mbus_state, np);
> +
> + of_node_put(np);
> +
> + return ret;
> +
Nitpick : This extra new line is not needed.
> }
> #endif