| Message ID | 20221201092202.3394119-1-zengheng4@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: mdio: fix unbalanced fwnode reference count in mdio_device_release() | expand |
On Thu, Dec 01, 2022 at 05:22:02PM +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 after that. > 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. How does this interact with: https://lore.kernel.org/netdev/20221201033838.1938765-1-yangyingliang@huawei.com/T/ Andrew
On 2022/12/1 23:27, Andrew Lunn wrote: > On Thu, Dec 01, 2022 at 05:22:02PM +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 after that. >> 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. > How does this interact with: > > https://lore.kernel.org/netdev/20221201033838.1938765-1-yangyingliang@huawei.com/T/ > > Andrew No, they don't interact with each other, because they fix different issues respectively. The patch sent by me is about eliminating refcount warning in the normal and error handling route of mdio_device_register(), while the one sent by Yingliang (as you concern about) is fixing refcount warning in the error handle path of phy_device_register(). Yingliang and I work on cleaning the warning report and enhancing the quality of kernel. I am not sure, for your convenience, shall I need to send my patch to Yingliang and let him edit them into a set of patches? With best regards, Zeng Heng
> No, they don't interact with each other, because they fix different issues > respectively. I'm wanted to ensure you know about each others work and that the fixes don't clash. I could not see a Cc: between you, nor a reviewed-by: etc to indicate you are working together on this. The patches can go separately, but maybe you can review v3 of his patch? Ask him to review yours. Andrew
On 2022/12/2 20:56, Andrew Lunn wrote: >> No, they don't interact with each other, because they fix different issues >> respectively. > I'm wanted to ensure you know about each others work and that the > fixes don't clash. I could not see a Cc: between you, nor a > reviewed-by: etc to indicate you are working together on this. Yeah, we know each others work, he run test with both patches merged. > > The patches can go separately, but maybe you can review v3 of his > patch? Ask him to review yours. I can review his patch. Thanks, Yang > > Andrew > .
On 2022/12/1 17:22, 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 after that. > 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> > --- > drivers/net/mdio/of_mdio.c | 1 - > drivers/net/phy/mdio_device.c | 2 ++ > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c > index 796e9c7857d0..296317a6b333 100644 > --- a/drivers/net/mdio/of_mdio.c > +++ b/drivers/net/mdio/of_mdio.c > @@ -69,7 +69,6 @@ static int of_mdiobus_register_device(struct mii_bus *mdio, > rc = mdio_device_register(mdiodev); > if (rc) { > mdio_device_free(mdiodev); > - of_node_put(child); device_set_node() is called to set fwnode and of_node, for undoing this, calling device_set_node(NULL) is better, and then call the put function. Thanks, Yang > 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..296317a6b333 100644 --- a/drivers/net/mdio/of_mdio.c +++ b/drivers/net/mdio/of_mdio.c @@ -69,7 +69,6 @@ static int of_mdiobus_register_device(struct mii_bus *mdio, rc = mdio_device_register(mdiodev); if (rc) { 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 after that. 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> --- drivers/net/mdio/of_mdio.c | 1 - drivers/net/phy/mdio_device.c | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-)