* Re: [PATCH v2] net: mdio: fix unbalanced fwnode reference count in mdio_device_release()
2022-12-03 7:34 [PATCH v2] net: mdio: fix unbalanced fwnode reference count in mdio_device_release() Zeng Heng
@ 2022-12-06 11:34 ` Paolo Abeni
2022-12-06 11:49 ` Zeng Heng
2022-12-06 11:42 ` Russell King (Oracle)
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Paolo Abeni @ 2022-12-06 11:34 UTC (permalink / raw)
To: Zeng Heng, hkallweit1, edumazet, kuba, davem, andrew, f.fainelli,
linux
Cc: liwei391, netdev
Hello,
On Sat, 2022-12-03 at 15:34 +0800, Zeng Heng wrote:
> There is warning report about of_node refcount leak
> while probing mdio device:
>
> OF: ERROR: memory leak, expected refcount 1 instead of 2,
> of_node_get()/of_node_put() unbalanced - destroy cset entry:
> attach overlay node /spi/soc@0/mdio@710700c0/ethernet@4
>
> In of_mdiobus_register_device(), we increase fwnode refcount
> by fwnode_handle_get() before associating the of_node with
> mdio device, but it has never been decreased in normal path.
> Since that, in mdio_device_release(), it needs to call
> fwnode_handle_put() in addition instead of calling kfree()
> directly.
>
> After above, just calling mdio_device_free() in the error handle
> path of of_mdiobus_register_device() is enough to keep the
> refcount balanced.
>
> Fixes: a9049e0c513c ("mdio: Add support for mdio drivers.")
> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
> Reviewed-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
> changes in v2:
> - Add operation about setting device node as NULL-pointer.
> There is no practical changes.
> - Add reviewed-by tag.
> ---
> drivers/net/mdio/of_mdio.c | 3 ++-
> drivers/net/phy/mdio_device.c | 2 ++
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
> index 796e9c7857d0..510822d6d0d9 100644
> --- a/drivers/net/mdio/of_mdio.c
> +++ b/drivers/net/mdio/of_mdio.c
> @@ -68,8 +68,9 @@ static int of_mdiobus_register_device(struct mii_bus *mdio,
> /* All data is now stored in the mdiodev struct; register it. */
> rc = mdio_device_register(mdiodev);
> if (rc) {
> + device_set_node(&mdiodev->dev, NULL);
> + fwnode_handle_put(fwnode);
> mdio_device_free(mdiodev);
> - of_node_put(child);
> return rc;
> }
>
> diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c
> index 250742ffdfd9..044828d081d2 100644
> --- a/drivers/net/phy/mdio_device.c
> +++ b/drivers/net/phy/mdio_device.c
> @@ -21,6 +21,7 @@
> #include <linux/slab.h>
> #include <linux/string.h>
> #include <linux/unistd.h>
> +#include <linux/property.h>
>
> void mdio_device_free(struct mdio_device *mdiodev)
> {
> @@ -30,6 +31,7 @@ EXPORT_SYMBOL(mdio_device_free);
>
> static void mdio_device_release(struct device *dev)
> {
> + fwnode_handle_put(dev->fwnode);
> kfree(to_mdio_device(dev));
> }
>
The patch LGTM, but I'll wait a bit more just in case Andrew want to
comment on it.
Cheers,
Paolo
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] net: mdio: fix unbalanced fwnode reference count in mdio_device_release()
2022-12-06 11:34 ` Paolo Abeni
@ 2022-12-06 11:49 ` Zeng Heng
0 siblings, 0 replies; 7+ messages in thread
From: Zeng Heng @ 2022-12-06 11:49 UTC (permalink / raw)
To: Paolo Abeni, hkallweit1, edumazet, kuba, davem, andrew,
f.fainelli, linux, Yang Yingliang
Cc: liwei391, netdev
On 2022/12/6 19:34, Paolo Abeni wrote:
> Hello,
> On Sat, 2022-12-03 at 15:34 +0800, Zeng Heng wrote:
>> There is warning report about of_node refcount leak
>> while probing mdio device:
>>
>> OF: ERROR: memory leak, expected refcount 1 instead of 2,
>> of_node_get()/of_node_put() unbalanced - destroy cset entry:
>> attach overlay node /spi/soc@0/mdio@710700c0/ethernet@4
>>
>> In of_mdiobus_register_device(), we increase fwnode refcount
>> by fwnode_handle_get() before associating the of_node with
>> mdio device, but it has never been decreased in normal path.
>> Since that, in mdio_device_release(), it needs to call
>> fwnode_handle_put() in addition instead of calling kfree()
>> directly.
>>
>> After above, just calling mdio_device_free() in the error handle
>> path of of_mdiobus_register_device() is enough to keep the
>> refcount balanced.
>>
>> Fixes: a9049e0c513c ("mdio: Add support for mdio drivers.")
>> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
>> Reviewed-by: Yang Yingliang <yangyingliang@huawei.com>
>> ---
>> changes in v2:
>> - Add operation about setting device node as NULL-pointer.
>> There is no practical changes.
>> - Add reviewed-by tag.
>> ---
>> drivers/net/mdio/of_mdio.c | 3 ++-
>> drivers/net/phy/mdio_device.c | 2 ++
>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
>> index 796e9c7857d0..510822d6d0d9 100644
>> --- a/drivers/net/mdio/of_mdio.c
>> +++ b/drivers/net/mdio/of_mdio.c
>> @@ -68,8 +68,9 @@ static int of_mdiobus_register_device(struct mii_bus *mdio,
>> /* All data is now stored in the mdiodev struct; register it. */
>> rc = mdio_device_register(mdiodev);
>> if (rc) {
>> + device_set_node(&mdiodev->dev, NULL);
>> + fwnode_handle_put(fwnode);
>> mdio_device_free(mdiodev);
>> - of_node_put(child);
>> return rc;
>> }
>>
>> diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c
>> index 250742ffdfd9..044828d081d2 100644
>> --- a/drivers/net/phy/mdio_device.c
>> +++ b/drivers/net/phy/mdio_device.c
>> @@ -21,6 +21,7 @@
>> #include <linux/slab.h>
>> #include <linux/string.h>
>> #include <linux/unistd.h>
>> +#include <linux/property.h>
>>
>> void mdio_device_free(struct mdio_device *mdiodev)
>> {
>> @@ -30,6 +31,7 @@ EXPORT_SYMBOL(mdio_device_free);
>>
>> static void mdio_device_release(struct device *dev)
>> {
>> + fwnode_handle_put(dev->fwnode);
>> kfree(to_mdio_device(dev));
>> }
>>
> The patch LGTM, but I'll wait a bit more just in case Andrew want to
> comment on it.
>
> Cheers,
>
> Paolo
+CC Yang Yingliang <yangyingliang@huawei.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] net: mdio: fix unbalanced fwnode reference count in mdio_device_release()
2022-12-03 7:34 [PATCH v2] net: mdio: fix unbalanced fwnode reference count in mdio_device_release() Zeng Heng
2022-12-06 11:34 ` Paolo Abeni
@ 2022-12-06 11:42 ` Russell King (Oracle)
2022-12-06 12:00 ` patchwork-bot+netdevbpf
2024-11-22 7:59 ` luoxuanqiang
3 siblings, 0 replies; 7+ messages in thread
From: Russell King (Oracle) @ 2022-12-06 11:42 UTC (permalink / raw)
To: Zeng Heng
Cc: hkallweit1, edumazet, pabeni, kuba, davem, andrew, f.fainelli,
liwei391, netdev
On Sat, Dec 03, 2022 at 03:34:41PM +0800, Zeng Heng wrote:
> There is warning report about of_node refcount leak
> while probing mdio device:
>
> OF: ERROR: memory leak, expected refcount 1 instead of 2,
> of_node_get()/of_node_put() unbalanced - destroy cset entry:
> attach overlay node /spi/soc@0/mdio@710700c0/ethernet@4
>
> In of_mdiobus_register_device(), we increase fwnode refcount
> by fwnode_handle_get() before associating the of_node with
> mdio device, but it has never been decreased in normal path.
> Since that, in mdio_device_release(), it needs to call
> fwnode_handle_put() in addition instead of calling kfree()
> directly.
>
> After above, just calling mdio_device_free() in the error handle
> path of of_mdiobus_register_device() is enough to keep the
> refcount balanced.
>
> Fixes: a9049e0c513c ("mdio: Add support for mdio drivers.")
> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
> Reviewed-by: Yang Yingliang <yangyingliang@huawei.com>
LGTM, thanks!
Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] net: mdio: fix unbalanced fwnode reference count in mdio_device_release()
2022-12-03 7:34 [PATCH v2] net: mdio: fix unbalanced fwnode reference count in mdio_device_release() Zeng Heng
2022-12-06 11:34 ` Paolo Abeni
2022-12-06 11:42 ` Russell King (Oracle)
@ 2022-12-06 12:00 ` patchwork-bot+netdevbpf
2024-11-22 7:59 ` luoxuanqiang
3 siblings, 0 replies; 7+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-12-06 12:00 UTC (permalink / raw)
To: Zeng Heng
Cc: hkallweit1, edumazet, pabeni, kuba, davem, andrew, f.fainelli,
linux, liwei391, netdev
Hello:
This patch was applied to netdev/net.git (master)
by Paolo Abeni <pabeni@redhat.com>:
On Sat, 3 Dec 2022 15:34:41 +0800 you wrote:
> There is warning report about of_node refcount leak
> while probing mdio device:
>
> OF: ERROR: memory leak, expected refcount 1 instead of 2,
> of_node_get()/of_node_put() unbalanced - destroy cset entry:
> attach overlay node /spi/soc@0/mdio@710700c0/ethernet@4
>
> [...]
Here is the summary with links:
- [v2] net: mdio: fix unbalanced fwnode reference count in mdio_device_release()
https://git.kernel.org/netdev/net/c/cb37617687f2
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] net: mdio: fix unbalanced fwnode reference count in mdio_device_release()
2022-12-03 7:34 [PATCH v2] net: mdio: fix unbalanced fwnode reference count in mdio_device_release() Zeng Heng
` (2 preceding siblings ...)
2022-12-06 12:00 ` patchwork-bot+netdevbpf
@ 2024-11-22 7:59 ` luoxuanqiang
2024-11-27 3:14 ` luoxuanqiang
3 siblings, 1 reply; 7+ messages in thread
From: luoxuanqiang @ 2024-11-22 7:59 UTC (permalink / raw)
To: Zeng Heng, hkallweit1, edumazet, pabeni, kuba, davem, andrew,
f.fainelli, linux
Cc: liwei391, netdev
Hi Heng,
在 2022/12/3 15:34, Zeng Heng 写道:
> There is warning report about of_node refcount leak
> while probing mdio device:
>
> OF: ERROR: memory leak, expected refcount 1 instead of 2,
> of_node_get()/of_node_put() unbalanced - destroy cset entry:
> attach overlay node /spi/soc@0/mdio@710700c0/ethernet@4
>
> In of_mdiobus_register_device(), we increase fwnode refcount
> by fwnode_handle_get() before associating the of_node with
> mdio device, but it has never been decreased in normal path.
> Since that, in mdio_device_release(), it needs to call
> fwnode_handle_put() in addition instead of calling kfree()
> directly.
>
> After above, just calling mdio_device_free() in the error handle
> path of of_mdiobus_register_device() is enough to keep the
> refcount balanced.
>
> Fixes: a9049e0c513c ("mdio: Add support for mdio drivers.")
> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
> Reviewed-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
> changes in v2:
> - Add operation about setting device node as NULL-pointer.
> There is no practical changes.
> - Add reviewed-by tag.
> ---
> drivers/net/mdio/of_mdio.c | 3 ++-
> drivers/net/phy/mdio_device.c | 2 ++
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
> index 796e9c7857d0..510822d6d0d9 100644
> --- a/drivers/net/mdio/of_mdio.c
> +++ b/drivers/net/mdio/of_mdio.c
> @@ -68,8 +68,9 @@ static int of_mdiobus_register_device(struct mii_bus *mdio,
> /* All data is now stored in the mdiodev struct; register it. */
> rc = mdio_device_register(mdiodev);
> if (rc) {
> + device_set_node(&mdiodev->dev, NULL);
> + fwnode_handle_put(fwnode);
According to my understanding, the process flow of mdio_device_free() is
as follows:
mdio_device_free()
-> put_device()
->kobject_put()
->kobject_release()
->device_release()
->mdio_device_release()
//Here, it will be called once fwnode_handle_put();
Why is it necessary to add fwnode_handle_put(fwnode) again here? In the
body of your email, you described that mdio_device_free() is sufficient
to keep refcount balanced, and it was not added in the v1.
Could you please explain the reason for this?
I am looking forward to your response :)
Thanks
Xuanqiang
> mdio_device_free(mdiodev);
> - of_node_put(child);
> return rc;
> }
>
> diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c
> index 250742ffdfd9..044828d081d2 100644
> --- a/drivers/net/phy/mdio_device.c
> +++ b/drivers/net/phy/mdio_device.c
> @@ -21,6 +21,7 @@
> #include <linux/slab.h>
> #include <linux/string.h>
> #include <linux/unistd.h>
> +#include <linux/property.h>
>
> void mdio_device_free(struct mdio_device *mdiodev)
> {
> @@ -30,6 +31,7 @@ EXPORT_SYMBOL(mdio_device_free);
>
> static void mdio_device_release(struct device *dev)
> {
> + fwnode_handle_put(dev->fwnode);
> kfree(to_mdio_device(dev));
> }
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] net: mdio: fix unbalanced fwnode reference count in mdio_device_release()
2024-11-22 7:59 ` luoxuanqiang
@ 2024-11-27 3:14 ` luoxuanqiang
0 siblings, 0 replies; 7+ messages in thread
From: luoxuanqiang @ 2024-11-27 3:14 UTC (permalink / raw)
To: Zeng Heng, hkallweit1, edumazet, pabeni, kuba, davem, andrew,
f.fainelli, linux
Cc: liwei391, netdev
Hi Heng,
在 2024/11/22 15:59, luoxuanqiang 写道:
> Hi Heng,
>
> 在 2022/12/3 15:34, Zeng Heng 写道:
>> There is warning report about of_node refcount leak
>> while probing mdio device:
>>
>> OF: ERROR: memory leak, expected refcount 1 instead of 2,
>> of_node_get()/of_node_put() unbalanced - destroy cset entry:
>> attach overlay node /spi/soc@0/mdio@710700c0/ethernet@4
>>
>> In of_mdiobus_register_device(), we increase fwnode refcount
>> by fwnode_handle_get() before associating the of_node with
>> mdio device, but it has never been decreased in normal path.
>> Since that, in mdio_device_release(), it needs to call
>> fwnode_handle_put() in addition instead of calling kfree()
>> directly.
>>
>> After above, just calling mdio_device_free() in the error handle
>> path of of_mdiobus_register_device() is enough to keep the
>> refcount balanced.
>>
>> Fixes: a9049e0c513c ("mdio: Add support for mdio drivers.")
>> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
>> Reviewed-by: Yang Yingliang <yangyingliang@huawei.com>
>> ---
>> changes in v2:
>> - Add operation about setting device node as NULL-pointer.
>> There is no practical changes.
>> - Add reviewed-by tag.
>> ---
>> drivers/net/mdio/of_mdio.c | 3 ++-
>> drivers/net/phy/mdio_device.c | 2 ++
>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
>> index 796e9c7857d0..510822d6d0d9 100644
>> --- a/drivers/net/mdio/of_mdio.c
>> +++ b/drivers/net/mdio/of_mdio.c
>> @@ -68,8 +68,9 @@ static int of_mdiobus_register_device(struct
>> mii_bus *mdio,
>> /* All data is now stored in the mdiodev struct; register it. */
>> rc = mdio_device_register(mdiodev);
>> if (rc) {
>> + device_set_node(&mdiodev->dev, NULL);
>> + fwnode_handle_put(fwnode);
> According to my understanding, the process flow of mdio_device_free() is
> as follows:
> mdio_device_free()
> -> put_device()
> ->kobject_put()
> ->kobject_release()
> ->device_release()
> ->mdio_device_release()
> //Here, it will be called once fwnode_handle_put();
>
> Why is it necessary to add fwnode_handle_put(fwnode) again here? In the
> body of your email, you described that mdio_device_free() is sufficient
> to keep refcount balanced, and it was not added in the v1.
> Could you please explain the reason for this?
> I am looking forward to your response :)
>
> Thanks
> Xuanqiang
I noticed that I had missed the fact that you had already disconnected
the relationship between dev and node by calling
device_set_node(&mdiodev->dev, NULL). Even though it would go through
fwnode_handle_put() again, it would not have any effect.
Thanks
>> mdio_device_free(mdiodev);
>> - of_node_put(child);
>> return rc;
>> }
>> diff --git a/drivers/net/phy/mdio_device.c
>> b/drivers/net/phy/mdio_device.c
>> index 250742ffdfd9..044828d081d2 100644
>> --- a/drivers/net/phy/mdio_device.c
>> +++ b/drivers/net/phy/mdio_device.c
>> @@ -21,6 +21,7 @@
>> #include <linux/slab.h>
>> #include <linux/string.h>
>> #include <linux/unistd.h>
>> +#include <linux/property.h>
>> void mdio_device_free(struct mdio_device *mdiodev)
>> {
>> @@ -30,6 +31,7 @@ EXPORT_SYMBOL(mdio_device_free);
>> static void mdio_device_release(struct device *dev)
>> {
>> + fwnode_handle_put(dev->fwnode);
>> kfree(to_mdio_device(dev));
>> }
>
^ permalink raw reply [flat|nested] 7+ messages in thread