| Message ID | 20220701024606.223438-1-windhl@126.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v4] clk: sprd: Hold reference returned by of_get_parent() | expand |
Hi Liang, On Fri, Jul 1, 2022 at 10:46 AM Liang He <windhl@126.com> wrote: > > We should hold the reference returned by of_get_parent() and use it > to call of_node_put() for refcount balance. > > Fixes: f95e8c7923d1 ("clk: sprd: support to get regmap from parent node") > Signed-off-by: Liang He <windhl@126.com> You are using the wrong patch number here. Please re-send this patch with subject that begins with [PATCH V4] ... and add Reviewed-by: Orson Zhai <orsonzhai@gmail.com> Thanks. -Orson > --- > changelog: > > v4: fix another bug in the same place, missing in v3 > v3: (1) keep original 'if-else if-else' coding style adviesd by Orson > (2) fix typo in commit-log: of_node_out --> of_node_put > v2: minimize the effective range of of_get_parent() advised by Orson > v1: hold reference returned by of_get_parent() > > > drivers/clk/sprd/common.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/clk/sprd/common.c b/drivers/clk/sprd/common.c > index d620bbbcdfc8..1b9c2aa0836f 100644 > --- a/drivers/clk/sprd/common.c > +++ b/drivers/clk/sprd/common.c > @@ -41,7 +41,7 @@ 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; > > if (of_find_property(node, "sprd,syscon", NULL)) { > @@ -50,9 +50,10 @@ int sprd_clk_regmap_init(struct platform_device *pdev, > pr_err("%s: failed to get syscon regmap\n", __func__); > 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 = of_get_parent(node), "syscon") > + || (of_node_put(np), 0)) { > + regmap = device_node_to_regmap(np = of_get_parent(node)); > + of_node_put(np); > if (IS_ERR(regmap)) { > dev_err(dev, "failed to get regmap from its parent.\n"); > return PTR_ERR(regmap); > -- > 2.25.1 >
On Fri, Jul 1, 2022 at 12:57 PM Orson Zhai <orsonzhai@gmail.com> wrote: > > Hi Liang, > > On Fri, Jul 1, 2022 at 10:46 AM Liang He <windhl@126.com> wrote: > > > > We should hold the reference returned by of_get_parent() and use it > > to call of_node_put() for refcount balance. > > > > Fixes: f95e8c7923d1 ("clk: sprd: support to get regmap from parent node") > > Signed-off-by: Liang He <windhl@126.com> > > You are using the wrong patch number here. > Please re-send this patch with subject that begins with [PATCH V4] ... Sorry, the patch number is correct but it's collapsed with patch v2 in my mailbox. Maybe you are using the same message id for all these patches? Anyway, the code change looks good to me. Reviewed-by: Orson Zhai <orsonzhai@gmail.com> Thanks. -Orson > and add Reviewed-by: Orson Zhai <orsonzhai@gmail.com> > > Thanks. > > -Orson > > > --- > > changelog: > > > > v4: fix another bug in the same place, missing in v3 > > v3: (1) keep original 'if-else if-else' coding style adviesd by Orson > > (2) fix typo in commit-log: of_node_out --> of_node_put > > v2: minimize the effective range of of_get_parent() advised by Orson > > v1: hold reference returned by of_get_parent() > > > > > > drivers/clk/sprd/common.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/clk/sprd/common.c b/drivers/clk/sprd/common.c > > index d620bbbcdfc8..1b9c2aa0836f 100644 > > --- a/drivers/clk/sprd/common.c > > +++ b/drivers/clk/sprd/common.c > > @@ -41,7 +41,7 @@ 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; > > > > if (of_find_property(node, "sprd,syscon", NULL)) { > > @@ -50,9 +50,10 @@ int sprd_clk_regmap_init(struct platform_device *pdev, > > pr_err("%s: failed to get syscon regmap\n", __func__); > > 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 = of_get_parent(node), "syscon") > > + || (of_node_put(np), 0)) { > > + regmap = device_node_to_regmap(np = of_get_parent(node)); > > + of_node_put(np); > > if (IS_ERR(regmap)) { > > dev_err(dev, "failed to get regmap from its parent.\n"); > > return PTR_ERR(regmap); > > -- > > 2.25.1 > >
On Fri, 1 Jul 2022 at 10:46, Liang He <windhl@126.com> wrote: > > We should hold the reference returned by of_get_parent() and use it > to call of_node_put() for refcount balance. > > Fixes: f95e8c7923d1 ("clk: sprd: support to get regmap from parent node") > Signed-off-by: Liang He <windhl@126.com> > --- > changelog: > > v4: fix another bug in the same place, missing in v3 > v3: (1) keep original 'if-else if-else' coding style adviesd by Orson > (2) fix typo in commit-log: of_node_out --> of_node_put > v2: minimize the effective range of of_get_parent() advised by Orson > v1: hold reference returned by of_get_parent() > > > drivers/clk/sprd/common.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/clk/sprd/common.c b/drivers/clk/sprd/common.c > index d620bbbcdfc8..1b9c2aa0836f 100644 > --- a/drivers/clk/sprd/common.c > +++ b/drivers/clk/sprd/common.c > @@ -41,7 +41,7 @@ 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; > > if (of_find_property(node, "sprd,syscon", NULL)) { > @@ -50,9 +50,10 @@ int sprd_clk_regmap_init(struct platform_device *pdev, > pr_err("%s: failed to get syscon regmap\n", __func__); > 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 = of_get_parent(node), "syscon") > + || (of_node_put(np), 0)) { > + regmap = device_node_to_regmap(np = of_get_parent(node)); of_get_parent() one more time would cause another unbalance, why not use 'np' directly here? I would also suggest to cc LKML (linux-kernel@vger.kernel.org) Thanks, Chunyan > + of_node_put(np); > if (IS_ERR(regmap)) { > dev_err(dev, "failed to get regmap from its parent.\n"); > return PTR_ERR(regmap); > -- > 2.25.1 >
At 2022-07-01 14:39:53, "Chunyan Zhang" <zhang.lyra@gmail.com> wrote: >On Fri, 1 Jul 2022 at 10:46, Liang He <windhl@126.com> wrote: >> >> We should hold the reference returned by of_get_parent() and use it >> to call of_node_put() for refcount balance. >> >> Fixes: f95e8c7923d1 ("clk: sprd: support to get regmap from parent node") >> Signed-off-by: Liang He <windhl@126.com> >> --- >> changelog: >> >> v4: fix another bug in the same place, missing in v3 >> v3: (1) keep original 'if-else if-else' coding style adviesd by Orson >> (2) fix typo in commit-log: of_node_out --> of_node_put >> v2: minimize the effective range of of_get_parent() advised by Orson >> v1: hold reference returned by of_get_parent() >> >> >> drivers/clk/sprd/common.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/clk/sprd/common.c b/drivers/clk/sprd/common.c >> index d620bbbcdfc8..1b9c2aa0836f 100644 >> --- a/drivers/clk/sprd/common.c >> +++ b/drivers/clk/sprd/common.c >> @@ -41,7 +41,7 @@ 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; >> >> if (of_find_property(node, "sprd,syscon", NULL)) { >> @@ -50,9 +50,10 @@ int sprd_clk_regmap_init(struct platform_device *pdev, >> pr_err("%s: failed to get syscon regmap\n", __func__); >> 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 = of_get_parent(node), "syscon") >> + || (of_node_put(np), 0)) { >> + regmap = device_node_to_regmap(np = of_get_parent(node)); > >of_get_parent() one more time would cause another unbalance, why not >use 'np' directly here? > >I would also suggest to cc LKML (linux-kernel@vger.kernel.org) > >Thanks, >Chunyan > Hi, Chunyan, Thanks for reviewing this patch code. In fact, the 'np' has already been |PUT| in the 'else if ( ...|| of_node_put(np)..)'. Based on the original code, there are two of_get_parent(), so we need the second one with the following second |PUT|. Thanks, Liang >> + of_node_put(np); >> if (IS_ERR(regmap)) { >> dev_err(dev, "failed to get regmap from its parent.\n"); >> return PTR_ERR(regmap); >> -- >> 2.25.1 >>
Liang, On Fri, Jul 1, 2022 at 2:50 PM Liang He <windhl@126.com> wrote: > > > > At 2022-07-01 14:39:53, "Chunyan Zhang" <zhang.lyra@gmail.com> wrote: > >On Fri, 1 Jul 2022 at 10:46, Liang He <windhl@126.com> wrote: > >> > >> We should hold the reference returned by of_get_parent() and use it > >> to call of_node_put() for refcount balance. > >> > >> Fixes: f95e8c7923d1 ("clk: sprd: support to get regmap from parent node") > >> Signed-off-by: Liang He <windhl@126.com> > >> --- > >> changelog: > >> > >> v4: fix another bug in the same place, missing in v3 > >> v3: (1) keep original 'if-else if-else' coding style adviesd by Orson > >> (2) fix typo in commit-log: of_node_out --> of_node_put > >> v2: minimize the effective range of of_get_parent() advised by Orson > >> v1: hold reference returned by of_get_parent() > >> > >> > >> drivers/clk/sprd/common.c | 9 +++++---- > >> 1 file changed, 5 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/clk/sprd/common.c b/drivers/clk/sprd/common.c > >> index d620bbbcdfc8..1b9c2aa0836f 100644 > >> --- a/drivers/clk/sprd/common.c > >> +++ b/drivers/clk/sprd/common.c > >> @@ -41,7 +41,7 @@ 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; > >> > >> if (of_find_property(node, "sprd,syscon", NULL)) { > >> @@ -50,9 +50,10 @@ int sprd_clk_regmap_init(struct platform_device *pdev, > >> pr_err("%s: failed to get syscon regmap\n", __func__); > >> 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 = of_get_parent(node), "syscon") > >> + || (of_node_put(np), 0)) { > >> + regmap = device_node_to_regmap(np = of_get_parent(node)); > > > >of_get_parent() one more time would cause another unbalance, why not > >use 'np' directly here? > > > >I would also suggest to cc LKML (linux-kernel@vger.kernel.org) > > > >Thanks, > >Chunyan > > > > Hi, Chunyan, > > Thanks for reviewing this patch code. > > In fact, the 'np' has already been |PUT| in the 'else if ( ...|| of_node_put(np)..)'. > > Based on the original code, there are two of_get_parent(), so we need the second > one with the following second |PUT|. Sorry, I missed it. I think what Chunyan said is right. np is declared at the beginning, so it's ok to be referred to in all places in this function after being assigned. the first of_node_put() will not be called if of_device_is_compatible() returns success. So we can refer to np directly here. -Orson > > Thanks, > > Liang > > >> + of_node_put(np); > >> if (IS_ERR(regmap)) { > >> dev_err(dev, "failed to get regmap from its parent.\n"); > >> return PTR_ERR(regmap); > >> -- > >> 2.25.1 > >>
At 2022-07-02 01:12:10, "Orson Zhai" <orsonzhai@gmail.com> wrote: >Liang, > >On Fri, Jul 1, 2022 at 2:50 PM Liang He <windhl@126.com> wrote: >> >> >> >> At 2022-07-01 14:39:53, "Chunyan Zhang" <zhang.lyra@gmail.com> wrote: >> >On Fri, 1 Jul 2022 at 10:46, Liang He <windhl@126.com> wrote: >> >> >> >> We should hold the reference returned by of_get_parent() and use it >> >> to call of_node_put() for refcount balance. >> >> >> >> Fixes: f95e8c7923d1 ("clk: sprd: support to get regmap from parent node") >> >> Signed-off-by: Liang He <windhl@126.com> >> >> --- >> >> changelog: >> >> >> >> v4: fix another bug in the same place, missing in v3 >> >> v3: (1) keep original 'if-else if-else' coding style adviesd by Orson >> >> (2) fix typo in commit-log: of_node_out --> of_node_put >> >> v2: minimize the effective range of of_get_parent() advised by Orson >> >> v1: hold reference returned by of_get_parent() >> >> >> >> >> >> drivers/clk/sprd/common.c | 9 +++++---- >> >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/drivers/clk/sprd/common.c b/drivers/clk/sprd/common.c >> >> index d620bbbcdfc8..1b9c2aa0836f 100644 >> >> --- a/drivers/clk/sprd/common.c >> >> +++ b/drivers/clk/sprd/common.c >> >> @@ -41,7 +41,7 @@ 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; >> >> >> >> if (of_find_property(node, "sprd,syscon", NULL)) { >> >> @@ -50,9 +50,10 @@ int sprd_clk_regmap_init(struct platform_device *pdev, >> >> pr_err("%s: failed to get syscon regmap\n", __func__); >> >> 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 = of_get_parent(node), "syscon") >> >> + || (of_node_put(np), 0)) { >> >> + regmap = device_node_to_regmap(np = of_get_parent(node)); >> > >> >of_get_parent() one more time would cause another unbalance, why not >> >use 'np' directly here? >> > >> >I would also suggest to cc LKML (linux-kernel@vger.kernel.org) >> > >> >Thanks, >> >Chunyan >> > >> >> Hi, Chunyan, >> >> Thanks for reviewing this patch code. >> >> In fact, the 'np' has already been |PUT| in the 'else if ( ...|| of_node_put(np)..)'. >> >> Based on the original code, there are two of_get_parent(), so we need the second >> one with the following second |PUT|. > >Sorry, I missed it. I think what Chunyan said is right. > >np is declared at the beginning, so it's ok to be referred to in all >places in this function >after being assigned. >the first of_node_put() will not be called if >of_device_is_compatible() returns success. >So we can refer to np directly here. > >-Orson > Thanks, Orson and Chunyan. I have learned a great lesson. This is what I learned: if of_device_is_compatible() returns success, the first of_node_put as the '||' will not be called, then we directly use 'np' in the branch and also use it to call the second of_node_put(). If of_device_is_compatible() returns false, the branch will not be executed, but the of_node_put() in the 'else if' condition will be executued. If this is the truth, I will begin to prepare my new version patch. Thanks again for both of you. Liang >> >> Thanks, >> >> Liang >> >> >> + of_node_put(np); >> >> if (IS_ERR(regmap)) { >> >> dev_err(dev, "failed to get regmap from its parent.\n"); >> >> return PTR_ERR(regmap); >> >> -- >> >> 2.25.1 >> >>
At 2022-07-02 01:12:10, "Orson Zhai" <orsonzhai@gmail.com> wrote: >Liang, > >On Fri, Jul 1, 2022 at 2:50 PM Liang He <windhl@126.com> wrote: >> >> >> >> At 2022-07-01 14:39:53, "Chunyan Zhang" <zhang.lyra@gmail.com> wrote: >> >On Fri, 1 Jul 2022 at 10:46, Liang He <windhl@126.com> wrote: >> >> >> >> We should hold the reference returned by of_get_parent() and use it >> >> to call of_node_put() for refcount balance. >> >> >> >> Fixes: f95e8c7923d1 ("clk: sprd: support to get regmap from parent node") >> >> Signed-off-by: Liang He <windhl@126.com> >> >> --- >> >> changelog: >> >> >> >> v4: fix another bug in the same place, missing in v3 >> >> v3: (1) keep original 'if-else if-else' coding style adviesd by Orson >> >> (2) fix typo in commit-log: of_node_out --> of_node_put >> >> v2: minimize the effective range of of_get_parent() advised by Orson >> >> v1: hold reference returned by of_get_parent() >> >> >> >> >> >> drivers/clk/sprd/common.c | 9 +++++---- >> >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/drivers/clk/sprd/common.c b/drivers/clk/sprd/common.c >> >> index d620bbbcdfc8..1b9c2aa0836f 100644 >> >> --- a/drivers/clk/sprd/common.c >> >> +++ b/drivers/clk/sprd/common.c >> >> @@ -41,7 +41,7 @@ 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; >> >> >> >> if (of_find_property(node, "sprd,syscon", NULL)) { >> >> @@ -50,9 +50,10 @@ int sprd_clk_regmap_init(struct platform_device *pdev, >> >> pr_err("%s: failed to get syscon regmap\n", __func__); >> >> 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 = of_get_parent(node), "syscon") >> >> + || (of_node_put(np), 0)) { >> >> + regmap = device_node_to_regmap(np = of_get_parent(node)); >> > >> >of_get_parent() one more time would cause another unbalance, why not >> >use 'np' directly here? >> > >> >I would also suggest to cc LKML (linux-kernel@vger.kernel.org) >> > >> >Thanks, >> >Chunyan >> > >> >> Hi, Chunyan, >> >> Thanks for reviewing this patch code. >> >> In fact, the 'np' has already been |PUT| in the 'else if ( ...|| of_node_put(np)..)'. >> >> Based on the original code, there are two of_get_parent(), so we need the second >> one with the following second |PUT|. > >Sorry, I missed it. I think what Chunyan said is right. > >np is declared at the beginning, so it's ok to be referred to in all >places in this function >after being assigned. >the first of_node_put() will not be called if >of_device_is_compatible() returns success. Hi, Orson, Now, I think this totally depends on the implementation of compilers. So, we should introduce this possible risk or use more conservative way I use before? do not put |PUT| in condition and move it to the 'else' branch? Thanks, Liang >So we can refer to np directly here. > >-Orson > >> >> Thanks, >> >> Liang >> >> >> + of_node_put(np); >> >> if (IS_ERR(regmap)) { >> >> dev_err(dev, "failed to get regmap from its parent.\n"); >> >> return PTR_ERR(regmap); >> >> -- >> >> 2.25.1 >> >>
Liang, On Sat, Jul 2, 2022 at 9:29 AM Liang He <windhl@126.com> wrote: > > > > > > At 2022-07-02 01:12:10, "Orson Zhai" <orsonzhai@gmail.com> wrote: > >Liang, > > > >On Fri, Jul 1, 2022 at 2:50 PM Liang He <windhl@126.com> wrote: > >> > >> > >> > >> At 2022-07-01 14:39:53, "Chunyan Zhang" <zhang.lyra@gmail.com> wrote: > >> >On Fri, 1 Jul 2022 at 10:46, Liang He <windhl@126.com> wrote: > >> >> > >> >> We should hold the reference returned by of_get_parent() and use it > >> >> to call of_node_put() for refcount balance. > >> >> > >> >> Fixes: f95e8c7923d1 ("clk: sprd: support to get regmap from parent node") > >> >> Signed-off-by: Liang He <windhl@126.com> > >> >> --- > >> >> changelog: > >> >> > >> >> v4: fix another bug in the same place, missing in v3 > >> >> v3: (1) keep original 'if-else if-else' coding style adviesd by Orson > >> >> (2) fix typo in commit-log: of_node_out --> of_node_put > >> >> v2: minimize the effective range of of_get_parent() advised by Orson > >> >> v1: hold reference returned by of_get_parent() > >> >> > >> >> > >> >> drivers/clk/sprd/common.c | 9 +++++---- > >> >> 1 file changed, 5 insertions(+), 4 deletions(-) > >> >> > >> >> diff --git a/drivers/clk/sprd/common.c b/drivers/clk/sprd/common.c > >> >> index d620bbbcdfc8..1b9c2aa0836f 100644 > >> >> --- a/drivers/clk/sprd/common.c > >> >> +++ b/drivers/clk/sprd/common.c > >> >> @@ -41,7 +41,7 @@ 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; > >> >> > >> >> if (of_find_property(node, "sprd,syscon", NULL)) { > >> >> @@ -50,9 +50,10 @@ int sprd_clk_regmap_init(struct platform_device *pdev, > >> >> pr_err("%s: failed to get syscon regmap\n", __func__); > >> >> 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 = of_get_parent(node), "syscon") > >> >> + || (of_node_put(np), 0)) { > >> >> + regmap = device_node_to_regmap(np = of_get_parent(node)); > >> > > >> >of_get_parent() one more time would cause another unbalance, why not > >> >use 'np' directly here? > >> > > >> >I would also suggest to cc LKML (linux-kernel@vger.kernel.org) > >> > > >> >Thanks, > >> >Chunyan > >> > > >> > >> Hi, Chunyan, > >> > >> Thanks for reviewing this patch code. > >> > >> In fact, the 'np' has already been |PUT| in the 'else if ( ...|| of_node_put(np)..)'. > >> > >> Based on the original code, there are two of_get_parent(), so we need the second > >> one with the following second |PUT|. > > > >Sorry, I missed it. I think what Chunyan said is right. > > > >np is declared at the beginning, so it's ok to be referred to in all > >places in this function > >after being assigned. > >the first of_node_put() will not be called if > >of_device_is_compatible() returns success. > > Hi, Orson, > > Now, I think this totally depends on the implementation of compilers. > Don't worry. Logical OR (||) is guaranteed in C standard. Check [1] for more information, please. -Orson [1] https://www.geeksforgeeks.org/sequence-points-in-c-set-1/ > So, we should introduce this possible risk or use more conservative way I use before? > > do not put |PUT| in condition and move it to the 'else' branch? > > Thanks, > > Liang > > > >So we can refer to np directly here. > > > >-Orson > > > >> > >> Thanks, > >> > >> Liang > >> > >> >> + of_node_put(np); > >> >> if (IS_ERR(regmap)) { > >> >> dev_err(dev, "failed to get regmap from its parent.\n"); > >> >> return PTR_ERR(regmap); > >> >> -- > >> >> 2.25.1 > >> >>
diff --git a/drivers/clk/sprd/common.c b/drivers/clk/sprd/common.c index d620bbbcdfc8..1b9c2aa0836f 100644 --- a/drivers/clk/sprd/common.c +++ b/drivers/clk/sprd/common.c @@ -41,7 +41,7 @@ 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; if (of_find_property(node, "sprd,syscon", NULL)) { @@ -50,9 +50,10 @@ int sprd_clk_regmap_init(struct platform_device *pdev, pr_err("%s: failed to get syscon regmap\n", __func__); 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 = of_get_parent(node), "syscon") + || (of_node_put(np), 0)) { + regmap = device_node_to_regmap(np = of_get_parent(node)); + of_node_put(np); if (IS_ERR(regmap)) { dev_err(dev, "failed to get regmap from its parent.\n"); return PTR_ERR(regmap);
We should hold the reference returned by of_get_parent() and use it to call of_node_put() for refcount balance. Fixes: f95e8c7923d1 ("clk: sprd: support to get regmap from parent node") Signed-off-by: Liang He <windhl@126.com> --- changelog: v4: fix another bug in the same place, missing in v3 v3: (1) keep original 'if-else if-else' coding style adviesd by Orson (2) fix typo in commit-log: of_node_out --> of_node_put v2: minimize the effective range of of_get_parent() advised by Orson v1: hold reference returned by of_get_parent() drivers/clk/sprd/common.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)