This patch series is to fix bugs for below APIs:
devm_phy_put() devm_of_phy_provider_unregister() devm_phy_destroy() phy_get() of_phy_get() devm_of_phy_get_by_index()
And simplify API of_phy_simple_xlate().
Signed-off-by: Zijun Hu quic_zijuhu@quicinc.com --- Zijun Hu (6): phy: core: Fix API devm_phy_put() can not release the phy phy: core: Fix API devm_of_phy_provider_unregister() can not unregister the phy provider phy: core: Fix API devm_phy_destroy() can not destroy the phy phy: core: Add missing of_node_put() for an error handling path of _of_phy_get() phy: core: Add missing of_node_put() in of_phy_provider_lookup() phy: core: Simplify API of_phy_simple_xlate() implementation
drivers/phy/phy-core.c | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) --- base-commit: d8f9d6d826fc15780451802796bb88ec52978f17 change-id: 20241020-phy_core_fix-e3ad65db98f7
Best regards,
From: Zijun Hu quic_zijuhu@quicinc.com
For devm_phy_put(), its comment says it needs to invoke phy_put() to release the phy, but it does not do that actually, so it can not fully undo what the API devm_phy_get() does, that is wrong, fixed by using devres_release() instead of devres_destroy() within the API.
BTW, the API is directly used by below source files in error handling path, so also fix relevant issue within these source files: drivers/pci/controller/cadence/pcie-cadence.c drivers/net/ethernet/ti/am65-cpsw-nuss.c
Fixes: ff764963479a ("drivers: phy: add generic PHY framework") Cc: stable@vger.kernel.org Cc: netdev@vger.kernel.org Cc: linux-pci@vger.kernel.org Signed-off-by: Zijun Hu quic_zijuhu@quicinc.com --- drivers/phy/phy-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index f053b525ccff..f190d7126613 100644 --- a/drivers/phy/phy-core.c +++ b/drivers/phy/phy-core.c @@ -737,7 +737,7 @@ void devm_phy_put(struct device *dev, struct phy *phy) if (!phy) return;
- r = devres_destroy(dev, devm_phy_release, devm_phy_match, phy); + r = devres_release(dev, devm_phy_release, devm_phy_match, phy); dev_WARN_ONCE(dev, r, "couldn't find PHY resource\n"); } EXPORT_SYMBOL_GPL(devm_phy_put);
From: Zijun Hu quic_zijuhu@quicinc.com
For devm_of_phy_provider_unregister(), its comment says it needs to invoke of_phy_provider_unregister() to unregister the phy provider but it does not do that actually, so it can not fully undo what the API devm_of_phy_provider_register() does, that is wrong, fixed by using devres_release() instead of devres_destroy() within the API.
Fixes: ff764963479a ("drivers: phy: add generic PHY framework") Cc: stable@vger.kernel.org Signed-off-by: Zijun Hu quic_zijuhu@quicinc.com --- drivers/phy/phy-core.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index f190d7126613..de07e1616b34 100644 --- a/drivers/phy/phy-core.c +++ b/drivers/phy/phy-core.c @@ -1259,12 +1259,12 @@ EXPORT_SYMBOL_GPL(of_phy_provider_unregister); * of_phy_provider_unregister to unregister the phy provider. */ void devm_of_phy_provider_unregister(struct device *dev, - struct phy_provider *phy_provider) + struct phy_provider *phy_provider) { int r;
- r = devres_destroy(dev, devm_phy_provider_release, devm_phy_match, - phy_provider); + r = devres_release(dev, devm_phy_provider_release, devm_phy_match, + phy_provider); dev_WARN_ONCE(dev, r, "couldn't find PHY provider device resource\n"); } EXPORT_SYMBOL_GPL(devm_of_phy_provider_unregister);
From: Zijun Hu quic_zijuhu@quicinc.com
For devm_phy_destroy(), its comment says it needs to invoke phy_destroy() to destroy the phy, but it does not do that actually, so it can not fully undo what the API devm_phy_create() does, that is wrong, fixed by using devres_release() instead of devres_destroy() within the API.
Fixes: ff764963479a ("drivers: phy: add generic PHY framework") Cc: stable@vger.kernel.org Signed-off-by: Zijun Hu quic_zijuhu@quicinc.com --- drivers/phy/phy-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index de07e1616b34..52ca590a58b9 100644 --- a/drivers/phy/phy-core.c +++ b/drivers/phy/phy-core.c @@ -1121,7 +1121,7 @@ void devm_phy_destroy(struct device *dev, struct phy *phy) { int r;
- r = devres_destroy(dev, devm_phy_consume, devm_phy_match, phy); + r = devres_release(dev, devm_phy_consume, devm_phy_match, phy); dev_WARN_ONCE(dev, r, "couldn't find PHY resource\n"); } EXPORT_SYMBOL_GPL(devm_phy_destroy);
From: Zijun Hu quic_zijuhu@quicinc.com
It is wrong for _of_phy_get() not to decrease the OF node refcount which was increased by previous of_parse_phandle_with_args() when returns due to of_device_is_compatible() error.
Fixed by adding of_node_put() before the error return.
Fixes: b7563e2796f8 ("phy: work around 'phys' references to usb-nop-xceiv devices") Cc: stable@vger.kernel.org Signed-off-by: Zijun Hu quic_zijuhu@quicinc.com --- drivers/phy/phy-core.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index 52ca590a58b9..967878b78797 100644 --- a/drivers/phy/phy-core.c +++ b/drivers/phy/phy-core.c @@ -629,8 +629,11 @@ static struct phy *_of_phy_get(struct device_node *np, int index) return ERR_PTR(-ENODEV);
/* This phy type handled by the usb-phy subsystem for now */ - if (of_device_is_compatible(args.np, "usb-nop-xceiv")) + if (of_device_is_compatible(args.np, "usb-nop-xceiv")) { + /* Put refcount above of_parse_phandle_with_args() got */ + of_node_put(args.np); return ERR_PTR(-ENODEV); + }
mutex_lock(&phy_provider_mutex); phy_provider = of_phy_provider_lookup(args.np);
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; + } }
return ERR_PTR(-EPROBE_DEFER);
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);
From: Zijun Hu quic_zijuhu@quicinc.com
Simplify of_phy_simple_xlate() implementation by API class_find_device_by_of_node() which is also safer since it subsys_get() @phy_class subsystem firstly then iterates devices.
Also comment its parameter @dev with unused in passing since the parameter provides no available input info but acts as an auto variable.
Signed-off-by: Zijun Hu quic_zijuhu@quicinc.com --- drivers/phy/phy-core.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index 24bd619a33dd..102fc6b6ff71 100644 --- a/drivers/phy/phy-core.c +++ b/drivers/phy/phy-core.c @@ -748,7 +748,7 @@ EXPORT_SYMBOL_GPL(devm_phy_put);
/** * of_phy_simple_xlate() - returns the phy instance from phy provider - * @dev: the PHY provider device + * @dev: the PHY provider device unused * @args: of_phandle_args (not used here) * * Intended to be used by phy provider for the common case where #phy-cells is @@ -759,20 +759,13 @@ EXPORT_SYMBOL_GPL(devm_phy_put); struct phy *of_phy_simple_xlate(struct device *dev, const struct of_phandle_args *args) { - struct phy *phy; - struct class_dev_iter iter; - - class_dev_iter_init(&iter, &phy_class, NULL, NULL); - while ((dev = class_dev_iter_next(&iter))) { - phy = to_phy(dev); - if (args->np != phy->dev.of_node) - continue; + struct device *target_dev;
- class_dev_iter_exit(&iter); - return phy; + target_dev = class_find_device_by_of_node(&phy_class, args->np); + if (target_dev) { + put_device(target_dev); + return to_phy(target_dev); } - - class_dev_iter_exit(&iter); return ERR_PTR(-ENODEV); } EXPORT_SYMBOL_GPL(of_phy_simple_xlate);
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree. Subject: [PATCH 6/6] phy: core: Simplify API of_phy_simple_xlate() implementation Link: https://lore.kernel.org/stable/20241020-phy_core_fix-v1-6-078062f7da71%40qui...
On 2024/10/20 13:27, Zijun Hu wrote:
From: Zijun Hu quic_zijuhu@quicinc.com
Simplify of_phy_simple_xlate() implementation by API class_find_device_by_of_node() which is also safer since it subsys_get() @phy_class subsystem firstly then iterates devices.
Also comment its parameter @dev with unused in passing since the parameter provides no available input info but acts as an auto variable.
i am not sure which parameter is unused. @dev or @args
comment says @args is not used here but code actually uses @args but does not use @dev
Signed-off-by: Zijun Hu quic_zijuhu@quicinc.com
drivers/phy/phy-core.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index 24bd619a33dd..102fc6b6ff71 100644 --- a/drivers/phy/phy-core.c +++ b/drivers/phy/phy-core.c @@ -748,7 +748,7 @@ EXPORT_SYMBOL_GPL(devm_phy_put); /**
- of_phy_simple_xlate() - returns the phy instance from phy provider
- @dev: the PHY provider device
- @dev: the PHY provider device unused
- @args: of_phandle_args (not used here)
- Intended to be used by phy provider for the common case where #phy-cells is
@@ -759,20 +759,13 @@ EXPORT_SYMBOL_GPL(devm_phy_put); struct phy *of_phy_simple_xlate(struct device *dev, const struct of_phandle_args *args) {
- struct phy *phy;
- struct class_dev_iter iter;
- class_dev_iter_init(&iter, &phy_class, NULL, NULL);
- while ((dev = class_dev_iter_next(&iter))) {
phy = to_phy(dev);if (args->np != phy->dev.of_node)continue;
- struct device *target_dev;
class_dev_iter_exit(&iter);return phy;
- target_dev = class_find_device_by_of_node(&phy_class, args->np);
- if (target_dev) {
put_device(target_dev);return to_phy(target_dev);
- class_dev_iter_exit(&iter); return ERR_PTR(-ENODEV);
} EXPORT_SYMBOL_GPL(of_phy_simple_xlate);
linux-stable-mirror@lists.linaro.org
-
Christophe JAILLET -
kernel test robot -
Zijun Hu