| Message ID | 20241020-phy_core_fix-v1-5-078062f7da71@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | phy: core: Fix bugs for several APIs and simplify an API | expand |
Le 20/10/2024 à 07:27, Zijun Hu a écrit : > From: Zijun Hu <quic_zijuhu@quicinc.com> > > The for_each_child_of_node() macro automatically decrements the child > refcount at the end of every iteration. On early exits, of_node_put() > must be used to manually decrement the refcount and avoid memory leaks. > > The macro called by of_phy_provider_lookup() has such early exit, but > it does not call of_node_put() before early exit. > > Fixed by adding missing of_node_put() in of_phy_provider_lookup(). > > Fixes: 2a4c37016ca9 ("phy: core: Fix of_phy_provider_lookup to return PHY provider for sub node") > Cc: stable@vger.kernel.org > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > > --- > The impact of this change is wide since of_phy_provider_lookup() > is indirectly called by APIs phy_get(), of_phy_get(), and > devm_of_phy_get_by_index(). > > The following kernel mainline commit has similar fix: > Commit: b337cc3ce475 ("backlight: lm3509_bl: Fix early returns in for_each_child_of_node()") > --- > drivers/phy/phy-core.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c > index 967878b78797..24bd619a33dd 100644 > --- a/drivers/phy/phy-core.c > +++ b/drivers/phy/phy-core.c > @@ -143,10 +143,11 @@ static struct phy_provider *of_phy_provider_lookup(struct device_node *node) > list_for_each_entry(phy_provider, &phy_provider_list, list) { > if (phy_provider->dev->of_node == node) > return phy_provider; > - > for_each_child_of_node(phy_provider->children, child) > - if (child == node) > + if (child == node) { > + of_node_put(child); > return phy_provider; > + } Hi, Maybe for_each_child_of_node_scoped() to slightly simplify things at the same time? > } > > return ERR_PTR(-EPROBE_DEFER); >
On 2024/10/20 15:23, Christophe JAILLET wrote: > Le 20/10/2024 à 07:27, Zijun Hu a écrit : >> From: Zijun Hu <quic_zijuhu@quicinc.com> [snip] >> --- >> drivers/phy/phy-core.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c >> index 967878b78797..24bd619a33dd 100644 >> --- a/drivers/phy/phy-core.c >> +++ b/drivers/phy/phy-core.c >> @@ -143,10 +143,11 @@ static struct phy_provider >> *of_phy_provider_lookup(struct device_node *node) >> list_for_each_entry(phy_provider, &phy_provider_list, list) { >> if (phy_provider->dev->of_node == node) >> return phy_provider; >> - >> for_each_child_of_node(phy_provider->children, child) >> - if (child == node) >> + if (child == node) { >> + of_node_put(child); >> return phy_provider; >> + } > > Hi, > > Maybe for_each_child_of_node_scoped() to slightly simplify things at the > same time? > thank you for code review. it does not use _scoped() since for_each_child_of_node() usage here is very simple and only has one early exit, _scoped() normally is for complex case. i maybe use _scoped() in next revision if one more reviewer also prefers it. thank you. >> } >> return ERR_PTR(-EPROBE_DEFER); >> >
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index 967878b78797..24bd619a33dd 100644 --- a/drivers/phy/phy-core.c +++ b/drivers/phy/phy-core.c @@ -143,10 +143,11 @@ static struct phy_provider *of_phy_provider_lookup(struct device_node *node) list_for_each_entry(phy_provider, &phy_provider_list, list) { if (phy_provider->dev->of_node == node) return phy_provider; - for_each_child_of_node(phy_provider->children, child) - if (child == node) + if (child == node) { + of_node_put(child); return phy_provider; + } } return ERR_PTR(-EPROBE_DEFER);