| Message ID | 20221203073441.3885317-1-zengheng4@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cb37617687f2bfa5b675df7779f869147c9002bd |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: mdio: fix unbalanced fwnode reference count in mdio_device_release() | expand |
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
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>
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>
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!
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)); > } >
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)); >> } >
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)); }
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(-)