Message ID | 20220624103809.4167753-1-windhl@126.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Series | clk/sprd: Hold reference returned by of_get_parent() | expand |
Hi Liang, On Fri, Jun 24, 2022 at 6:39 PM Liang He <windhl@126.com> wrote: > > We should hold the reference returned by of_get_parent() and use it > to call of_node_out() for refcount balance. > > Fixes: f95e8c7923d1 ("clk: sprd: support to get regmap from parent node") > Signed-off-by: Liang He <windhl@126.com> > --- > these bugs are found in 5.19rc2 > these bugs are confirmed not fixed by lore.kernel.org > these bugs are compiled tested in 5.19rc2 with at91_dt_defconfig > > drivers/clk/sprd/common.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/drivers/clk/sprd/common.c b/drivers/clk/sprd/common.c > index d620bbbcdfc8..08c1d7a9ec8b 100644 > --- a/drivers/clk/sprd/common.c > +++ b/drivers/clk/sprd/common.c > @@ -41,35 +41,41 @@ int sprd_clk_regmap_init(struct platform_device *pdev, > { > void __iomem *base; > struct device *dev = &pdev->dev; > - struct device_node *node = dev->of_node; > + struct device_node *node = dev->of_node, *np; > struct regmap *regmap; > > + np = of_get_parent(dev->of_node); > if (of_find_property(node, "sprd,syscon", NULL)) { > regmap = syscon_regmap_lookup_by_phandle(node, "sprd,syscon"); Why not to call of_node_put() in this function? He returns an error and it is his responsibility to clean up everything he used, right? -Orson > if (IS_ERR(regmap)) { > pr_err("%s: failed to get syscon regmap\n", __func__); > + of_node_put(np); > return PTR_ERR(regmap); > } > - } else if (of_device_is_compatible(of_get_parent(dev->of_node), > - "syscon")) { > - regmap = device_node_to_regmap(of_get_parent(dev->of_node)); > + } else if (of_device_is_compatible(np, "syscon")) { > + regmap = device_node_to_regmap(np); > if (IS_ERR(regmap)) { > + of_node_put(np); > dev_err(dev, "failed to get regmap from its parent.\n"); > return PTR_ERR(regmap); > } > } else { > base = devm_platform_ioremap_resource(pdev, 0); > - if (IS_ERR(base)) > + if (IS_ERR(base)) { > + of_node_put(np); > return PTR_ERR(base); > + } > > regmap = devm_regmap_init_mmio(&pdev->dev, base, > &sprdclk_regmap_config); > if (IS_ERR(regmap)) { > + of_node_put(np); > pr_err("failed to init regmap\n"); > return PTR_ERR(regmap); > } > } > > + of_node_put(np); > sprd_clk_set_regmap(desc, regmap); > > return 0; > -- > 2.25.1 >
At 2022-06-25 16:36:04, "Orson Zhai" <orsonzhai@gmail.com> wrote: >Hi Liang, > >On Fri, Jun 24, 2022 at 6:39 PM Liang He <windhl@126.com> wrote: >> >> We should hold the reference returned by of_get_parent() and use it >> to call of_node_out() for refcount balance. >> >> Fixes: f95e8c7923d1 ("clk: sprd: support to get regmap from parent node") >> Signed-off-by: Liang He <windhl@126.com> >> --- >> these bugs are found in 5.19rc2 >> these bugs are confirmed not fixed by lore.kernel.org >> these bugs are compiled tested in 5.19rc2 with at91_dt_defconfig >> >> drivers/clk/sprd/common.c | 16 +++++++++++----- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/clk/sprd/common.c b/drivers/clk/sprd/common.c >> index d620bbbcdfc8..08c1d7a9ec8b 100644 >> --- a/drivers/clk/sprd/common.c >> +++ b/drivers/clk/sprd/common.c >> @@ -41,35 +41,41 @@ int sprd_clk_regmap_init(struct platform_device *pdev, >> { >> void __iomem *base; >> struct device *dev = &pdev->dev; >> - struct device_node *node = dev->of_node; >> + struct device_node *node = dev->of_node, *np; >> struct regmap *regmap; >> >> + np = of_get_parent(dev->of_node); >> if (of_find_property(node, "sprd,syscon", NULL)) { >> regmap = syscon_regmap_lookup_by_phandle(node, "sprd,syscon"); > >Why not to call of_node_put() in this function? >He returns an error and it is his responsibility to clean up >everything he used, right? > >-Orson > Hi, Orson, I only know we should use the pair of functions (of_get_parent(), of_node_put) to keep refcount balance. However, as I do not really know the semantic of other functions, so I cannot give other kinds of patch. By the way, if we chose to put of_node_put() in this function, I think it also need to put of_node_put() into 'device_node_to_regmap' as it also return error code in following path. Anyway, thanks very much for your reply and review on my patch code. Liang >> if (IS_ERR(regmap)) { >> pr_err("%s: failed to get syscon regmap\n", __func__); >> + of_node_put(np); >> return PTR_ERR(regmap); >> } >> - } else if (of_device_is_compatible(of_get_parent(dev->of_node), >> - "syscon")) { >> - regmap = device_node_to_regmap(of_get_parent(dev->of_node)); >> + } else if (of_device_is_compatible(np, "syscon")) { >> + regmap = device_node_to_regmap(np); >> if (IS_ERR(regmap)) { >> + of_node_put(np); >> dev_err(dev, "failed to get regmap from its parent.\n"); >> return PTR_ERR(regmap); >> } >> } else { >> base = devm_platform_ioremap_resource(pdev, 0); >> - if (IS_ERR(base)) >> + if (IS_ERR(base)) { >> + of_node_put(np); >> return PTR_ERR(base); >> + } >> >> regmap = devm_regmap_init_mmio(&pdev->dev, base, >> &sprdclk_regmap_config); >> if (IS_ERR(regmap)) { >> + of_node_put(np); >> pr_err("failed to init regmap\n"); >> return PTR_ERR(regmap); >> } >> } >> >> + of_node_put(np); >> sprd_clk_set_regmap(desc, regmap); >> >> return 0; >> -- >> 2.25.1 >>
Hi Liang, On Sat, Jun 25, 2022 at 11:34 PM Liang He <windhl@126.com> wrote: > > > At 2022-06-25 16:36:04, "Orson Zhai" <orsonzhai@gmail.com> wrote: > >Hi Liang, > > > >On Fri, Jun 24, 2022 at 6:39 PM Liang He <windhl@126.com> wrote: > >> > >> We should hold the reference returned by of_get_parent() and use it > >> to call of_node_out() for refcount balance. > >> > >> Fixes: f95e8c7923d1 ("clk: sprd: support to get regmap from parent node") > >> Signed-off-by: Liang He <windhl@126.com> > >> --- > >> these bugs are found in 5.19rc2 > >> these bugs are confirmed not fixed by lore.kernel.org > >> these bugs are compiled tested in 5.19rc2 with at91_dt_defconfig > >> > >> drivers/clk/sprd/common.c | 16 +++++++++++----- > >> 1 file changed, 11 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/clk/sprd/common.c b/drivers/clk/sprd/common.c > >> index d620bbbcdfc8..08c1d7a9ec8b 100644 > >> --- a/drivers/clk/sprd/common.c > >> +++ b/drivers/clk/sprd/common.c > >> @@ -41,35 +41,41 @@ int sprd_clk_regmap_init(struct platform_device *pdev, > >> { > >> void __iomem *base; > >> struct device *dev = &pdev->dev; > >> - struct device_node *node = dev->of_node; > >> + struct device_node *node = dev->of_node, *np; > >> struct regmap *regmap; > >> > >> + np = of_get_parent(dev->of_node); > >> if (of_find_property(node, "sprd,syscon", NULL)) { > >> regmap = syscon_regmap_lookup_by_phandle(node, "sprd,syscon"); > > > >Why not to call of_node_put() in this function? > >He returns an error and it is his responsibility to clean up > >everything he used, right? > > > >-Orson > > > Hi, Orson, > > I only know we should use the pair of functions (of_get_parent(), of_node_put) > to keep refcount balance. > > However, as I do not really know the semantic of other functions, so I cannot > give other kinds of patch. Sorry, I didn't catch your point before. You're right on this unbalance. But it has no direct relation to the function here. I think it is better to minimize the effective range of of_get_parent() Could you refer to Pantelis's answer to the last question at [1]. Thanks. -Orson [1] https://www.spinics.net/lists/devicetree/msg122555.html > > By the way, if we chose to put of_node_put() in this function, I think it also need > to put of_node_put() into 'device_node_to_regmap' as it also return error code in > following path. > > Anyway, thanks very much for your reply and review on my patch code. > > Liang > > > >> if (IS_ERR(regmap)) { > >> pr_err("%s: failed to get syscon regmap\n", __func__); > >> + of_node_put(np); > >> return PTR_ERR(regmap); > >> } > >> - } else if (of_device_is_compatible(of_get_parent(dev->of_node), > >> - "syscon")) { > >> - regmap = device_node_to_regmap(of_get_parent(dev->of_node)); > >> + } else if (of_device_is_compatible(np, "syscon")) { > >> + regmap = device_node_to_regmap(np); > >> if (IS_ERR(regmap)) { > >> + of_node_put(np); > >> dev_err(dev, "failed to get regmap from its parent.\n"); > >> return PTR_ERR(regmap); > >> } > >> } else { > >> base = devm_platform_ioremap_resource(pdev, 0); > >> - if (IS_ERR(base)) > >> + if (IS_ERR(base)) { > >> + of_node_put(np); > >> return PTR_ERR(base); > >> + } > >> > >> regmap = devm_regmap_init_mmio(&pdev->dev, base, > >> &sprdclk_regmap_config); > >> if (IS_ERR(regmap)) { > >> + of_node_put(np); > >> pr_err("failed to init regmap\n"); > >> return PTR_ERR(regmap); > >> } > >> } > >> > >> + of_node_put(np); > >> sprd_clk_set_regmap(desc, regmap); > >> > >> return 0; > >> -- > >> 2.25.1 > >>
At 2022-06-26 11:32:40, "Orson Zhai" <orsonzhai@gmail.com> wrote: >Hi Liang, > >On Sat, Jun 25, 2022 at 11:34 PM Liang He <windhl@126.com> wrote: >> >> >> At 2022-06-25 16:36:04, "Orson Zhai" <orsonzhai@gmail.com> wrote: >> >Hi Liang, >> > >> >On Fri, Jun 24, 2022 at 6:39 PM Liang He <windhl@126.com> wrote: >> >> >> >> We should hold the reference returned by of_get_parent() and use it >> >> to call of_node_out() for refcount balance. >> >> >> >> Fixes: f95e8c7923d1 ("clk: sprd: support to get regmap from parent node") >> >> Signed-off-by: Liang He <windhl@126.com> >> >> --- >> >> these bugs are found in 5.19rc2 >> >> these bugs are confirmed not fixed by lore.kernel.org >> >> these bugs are compiled tested in 5.19rc2 with at91_dt_defconfig >> >> >> >> drivers/clk/sprd/common.c | 16 +++++++++++----- >> >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> >> >> diff --git a/drivers/clk/sprd/common.c b/drivers/clk/sprd/common.c >> >> index d620bbbcdfc8..08c1d7a9ec8b 100644 >> >> --- a/drivers/clk/sprd/common.c >> >> +++ b/drivers/clk/sprd/common.c >> >> @@ -41,35 +41,41 @@ int sprd_clk_regmap_init(struct platform_device *pdev, >> >> { >> >> void __iomem *base; >> >> struct device *dev = &pdev->dev; >> >> - struct device_node *node = dev->of_node; >> >> + struct device_node *node = dev->of_node, *np; >> >> struct regmap *regmap; >> >> >> >> + np = of_get_parent(dev->of_node); >> >> if (of_find_property(node, "sprd,syscon", NULL)) { >> >> regmap = syscon_regmap_lookup_by_phandle(node, "sprd,syscon"); >> > >> >Why not to call of_node_put() in this function? >> >He returns an error and it is his responsibility to clean up >> >everything he used, right? >> > >> >-Orson >> > >> Hi, Orson, >> >> I only know we should use the pair of functions (of_get_parent(), of_node_put) >> to keep refcount balance. >> >> However, as I do not really know the semantic of other functions, so I cannot >> give other kinds of patch. > >Sorry, I didn't catch your point before. >You're right on this unbalance. But it has no direct relation to the >function here. Thanks for your understanding. And yes, it has no direct relation to the function here. >I think it is better to minimize the effective range of of_get_parent() > >Could you refer to Pantelis's answer to the last question at [1]. > >Thanks. >-Orson > >[1] https://www.spinics.net/lists/devicetree/msg122555.html > Thanks, Orson. Yes, I also wanted to make a better patch solution as the bug is only caused by the 'else if' branch. However, I cannot find a better solution to fix the bug in the middle of nested 'if-else if-else' brances unless we try to rewrite them, which is more difficult for me, now. But if you think it is (semantic) safe to rewrite these branches, I can try it. Thanks again for your relpy. Liang >> >> By the way, if we chose to put of_node_put() in this function, I think it also need >> to put of_node_put() into 'device_node_to_regmap' as it also return error code in >> following path. >> >> Anyway, thanks very much for your reply and review on my patch code. >> >> Liang >> >> >> >> if (IS_ERR(regmap)) { >> >> pr_err("%s: failed to get syscon regmap\n", __func__); >> >> + of_node_put(np); >> >> return PTR_ERR(regmap); >> >> } >> >> - } else if (of_device_is_compatible(of_get_parent(dev->of_node), >> >> - "syscon")) { >> >> - regmap = device_node_to_regmap(of_get_parent(dev->of_node)); >> >> + } else if (of_device_is_compatible(np, "syscon")) { >> >> + regmap = device_node_to_regmap(np); >> >> if (IS_ERR(regmap)) { >> >> + of_node_put(np); >> >> dev_err(dev, "failed to get regmap from its parent.\n"); >> >> return PTR_ERR(regmap); >> >> } >> >> } else { >> >> base = devm_platform_ioremap_resource(pdev, 0); >> >> - if (IS_ERR(base)) >> >> + if (IS_ERR(base)) { >> >> + of_node_put(np); >> >> return PTR_ERR(base); >> >> + } >> >> >> >> regmap = devm_regmap_init_mmio(&pdev->dev, base, >> >> &sprdclk_regmap_config); >> >> if (IS_ERR(regmap)) { >> >> + of_node_put(np); >> >> pr_err("failed to init regmap\n"); >> >> return PTR_ERR(regmap); >> >> } >> >> } >> >> >> >> + of_node_put(np); >> >> sprd_clk_set_regmap(desc, regmap); >> >> >> >> return 0; >> >> -- >> >> 2.25.1 >> >>
On Sun, Jun 26, 2022 at 12:03 PM Liang He <windhl@126.com> wrote: > > > > At 2022-06-26 11:32:40, "Orson Zhai" <orsonzhai@gmail.com> wrote: > >Hi Liang, > > > >On Sat, Jun 25, 2022 at 11:34 PM Liang He <windhl@126.com> wrote: > >> > >> > >> At 2022-06-25 16:36:04, "Orson Zhai" <orsonzhai@gmail.com> wrote: > >> >Hi Liang, > >> > > >> >On Fri, Jun 24, 2022 at 6:39 PM Liang He <windhl@126.com> wrote: > >> >> > >> >> We should hold the reference returned by of_get_parent() and use it > >> >> to call of_node_out() for refcount balance. > >> >> > >> >> Fixes: f95e8c7923d1 ("clk: sprd: support to get regmap from parent node") > >> >> Signed-off-by: Liang He <windhl@126.com> > >> >> --- > >> >> these bugs are found in 5.19rc2 > >> >> these bugs are confirmed not fixed by lore.kernel.org > >> >> these bugs are compiled tested in 5.19rc2 with at91_dt_defconfig > >> >> > >> >> drivers/clk/sprd/common.c | 16 +++++++++++----- > >> >> 1 file changed, 11 insertions(+), 5 deletions(-) > >> >> > >> >> diff --git a/drivers/clk/sprd/common.c b/drivers/clk/sprd/common.c > >> >> index d620bbbcdfc8..08c1d7a9ec8b 100644 > >> >> --- a/drivers/clk/sprd/common.c > >> >> +++ b/drivers/clk/sprd/common.c > >> >> @@ -41,35 +41,41 @@ int sprd_clk_regmap_init(struct platform_device *pdev, > >> >> { > >> >> void __iomem *base; > >> >> struct device *dev = &pdev->dev; > >> >> - struct device_node *node = dev->of_node; > >> >> + struct device_node *node = dev->of_node, *np; > >> >> struct regmap *regmap; > >> >> > >> >> + np = of_get_parent(dev->of_node); > >> >> if (of_find_property(node, "sprd,syscon", NULL)) { > >> >> regmap = syscon_regmap_lookup_by_phandle(node, "sprd,syscon"); > >> > > >> >Why not to call of_node_put() in this function? > >> >He returns an error and it is his responsibility to clean up > >> >everything he used, right? > >> > > >> >-Orson > >> > > >> Hi, Orson, > >> > >> I only know we should use the pair of functions (of_get_parent(), of_node_put) > >> to keep refcount balance. > >> > >> However, as I do not really know the semantic of other functions, so I cannot > >> give other kinds of patch. > > > >Sorry, I didn't catch your point before. > >You're right on this unbalance. But it has no direct relation to the > >function here. > > Thanks for your understanding. And yes, it has no direct relation > to the function here. > > >I think it is better to minimize the effective range of of_get_parent() > > > >Could you refer to Pantelis's answer to the last question at [1]. > > > >Thanks. > >-Orson > > > >[1] https://www.spinics.net/lists/devicetree/msg122555.html > > > > Thanks, Orson. > > Yes, I also wanted to make a better patch solution as the bug is only caused > by the 'else if' branch. > > However, I cannot find a better solution to fix the bug in the middle of nested > 'if-else if-else' brances unless we try to rewrite them, which is more difficult for me, now. > > But if you think it is (semantic) safe to rewrite these branches, I can try it. No problem. Please do it as long as not to change its original logic. Thanks. -Orson > > Thanks again for your relpy. > > Liang > > >> > >> By the way, if we chose to put of_node_put() in this function, I think it also need > >> to put of_node_put() into 'device_node_to_regmap' as it also return error code in > >> following path. > >> > >> Anyway, thanks very much for your reply and review on my patch code. > >> > >> Liang > >> > >> > >> >> if (IS_ERR(regmap)) { > >> >> pr_err("%s: failed to get syscon regmap\n", __func__); > >> >> + of_node_put(np); > >> >> return PTR_ERR(regmap); > >> >> } > >> >> - } else if (of_device_is_compatible(of_get_parent(dev->of_node), > >> >> - "syscon")) { > >> >> - regmap = device_node_to_regmap(of_get_parent(dev->of_node)); > >> >> + } else if (of_device_is_compatible(np, "syscon")) { > >> >> + regmap = device_node_to_regmap(np); > >> >> if (IS_ERR(regmap)) { > >> >> + of_node_put(np); > >> >> dev_err(dev, "failed to get regmap from its parent.\n"); > >> >> return PTR_ERR(regmap); > >> >> } > >> >> } else { > >> >> base = devm_platform_ioremap_resource(pdev, 0); > >> >> - if (IS_ERR(base)) > >> >> + if (IS_ERR(base)) { > >> >> + of_node_put(np); > >> >> return PTR_ERR(base); > >> >> + } > >> >> > >> >> regmap = devm_regmap_init_mmio(&pdev->dev, base, > >> >> &sprdclk_regmap_config); > >> >> if (IS_ERR(regmap)) { > >> >> + of_node_put(np); > >> >> pr_err("failed to init regmap\n"); > >> >> return PTR_ERR(regmap); > >> >> } > >> >> } > >> >> > >> >> + of_node_put(np); > >> >> sprd_clk_set_regmap(desc, regmap); > >> >> > >> >> return 0; > >> >> -- > >> >> 2.25.1 > >> >>
diff --git a/drivers/clk/sprd/common.c b/drivers/clk/sprd/common.c index d620bbbcdfc8..08c1d7a9ec8b 100644 --- a/drivers/clk/sprd/common.c +++ b/drivers/clk/sprd/common.c @@ -41,35 +41,41 @@ int sprd_clk_regmap_init(struct platform_device *pdev, { void __iomem *base; struct device *dev = &pdev->dev; - struct device_node *node = dev->of_node; + struct device_node *node = dev->of_node, *np; struct regmap *regmap; + np = of_get_parent(dev->of_node); if (of_find_property(node, "sprd,syscon", NULL)) { regmap = syscon_regmap_lookup_by_phandle(node, "sprd,syscon"); if (IS_ERR(regmap)) { pr_err("%s: failed to get syscon regmap\n", __func__); + of_node_put(np); return PTR_ERR(regmap); } - } else if (of_device_is_compatible(of_get_parent(dev->of_node), - "syscon")) { - regmap = device_node_to_regmap(of_get_parent(dev->of_node)); + } else if (of_device_is_compatible(np, "syscon")) { + regmap = device_node_to_regmap(np); if (IS_ERR(regmap)) { + of_node_put(np); dev_err(dev, "failed to get regmap from its parent.\n"); return PTR_ERR(regmap); } } else { base = devm_platform_ioremap_resource(pdev, 0); - if (IS_ERR(base)) + if (IS_ERR(base)) { + of_node_put(np); return PTR_ERR(base); + } regmap = devm_regmap_init_mmio(&pdev->dev, base, &sprdclk_regmap_config); if (IS_ERR(regmap)) { + of_node_put(np); pr_err("failed to init regmap\n"); return PTR_ERR(regmap); } } + of_node_put(np); sprd_clk_set_regmap(desc, regmap); return 0;
We should hold the reference returned by of_get_parent() and use it to call of_node_out() for refcount balance. Fixes: f95e8c7923d1 ("clk: sprd: support to get regmap from parent node") Signed-off-by: Liang He <windhl@126.com> --- these bugs are found in 5.19rc2 these bugs are confirmed not fixed by lore.kernel.org these bugs are compiled tested in 5.19rc2 with at91_dt_defconfig drivers/clk/sprd/common.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)