| Message ID | 20231220045228.27079-2-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cff9c565e65f3622e8dc1dcc21c1520a083dff35 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: mdio: get/put device node during (un)registration | expand |
Hello: This patch was applied to netdev/net-next.git (main) by David S. Miller <davem@davemloft.net>: On Wed, 20 Dec 2023 01:52:29 -0300 you wrote: > The __of_mdiobus_register() function was storing the device node in > dev.of_node without increasing its reference count. It implicitly relied > on the caller to maintain the allocated node until the mdiobus was > unregistered. > > Now, __of_mdiobus_register() will acquire the node before assigning it, > and of_mdiobus_unregister_callback() will be called at the end of > mdio_unregister(). > > [...] Here is the summary with links: - [net-next] net: mdio: get/put device node during (un)registration https://git.kernel.org/netdev/net-next/c/cff9c565e65f You are awesome, thank you!
On Wed, Dec 20, 2023 at 01:52:29AM -0300, Luiz Angelo Daros de Luca wrote: > The __of_mdiobus_register() function was storing the device node in > dev.of_node without increasing its reference count. It implicitly relied > on the caller to maintain the allocated node until the mdiobus was > unregistered. > > Now, __of_mdiobus_register() will acquire the node before assigning it, > and of_mdiobus_unregister_callback() will be called at the end of > mdio_unregister(). > > Drivers can now release the node immediately after MDIO registration. > Some of them are already doing that even before this patch. > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> I don't like this, certainly not the use of a method prefixed by a double-underscore, and neither the conditional nature of "putting" this. That alone seems to point to there being more issues. I also notice that netdev have applied this without *any* review from phylib maintainers. Grr. Indeed there are more issues with the refcounting here. If one looks at drivers/net/phy/mdio_bus.c::of_mdiobus_link_mdiodev(), we find this: if (addr == mdiodev->addr) { device_set_node(dev, of_fwnode_handle(child)); /* The refcount on "child" is passed to the mdio * device. Do _not_ use of_node_put(child) here. */ return; but there is nowhere that this refcount is dropped. Really, the patch should be addressing the problem rather than putting a sticky-plaster over just one instance of it.
> On Wed, Dec 20, 2023 at 01:52:29AM -0300, Luiz Angelo Daros de Luca wrote: > > The __of_mdiobus_register() function was storing the device node in > > dev.of_node without increasing its reference count. It implicitly relied > > on the caller to maintain the allocated node until the mdiobus was > > unregistered. > > > > Now, __of_mdiobus_register() will acquire the node before assigning it, > > and of_mdiobus_unregister_callback() will be called at the end of > > mdio_unregister(). > > > > Drivers can now release the node immediately after MDIO registration. > > Some of them are already doing that even before this patch. > > > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > > I don't like this, certainly not the use of a method prefixed by a > double-underscore, and neither the conditional nature of "putting" > this. That alone seems to point to there being more issues. Thanks Russel. At least one driver (bcm_sf2_mdio_register) is writing directly to the mii_bus->dev.of_node and not using of_mdiobus_register(). We should not put a node in the MDIO bus if the bus didn't get it before. That's the reason for the conditional putting the node. I wasn't sure about the names. What would be an appropriate name? The same without the prefix? In order to put the node only when the bus was registered by __of_mdiobus_register, I opted for a callback but it might be a better approach. > I also notice that netdev have applied this without *any* review from > phylib maintainers. Grr. Some reviews are required. Should we revert it? > Indeed there are more issues with the refcounting here. If one looks at > drivers/net/phy/mdio_bus.c::of_mdiobus_link_mdiodev(), we find this: > > if (addr == mdiodev->addr) { > device_set_node(dev, of_fwnode_handle(child)); > /* The refcount on "child" is passed to the mdio > * device. Do _not_ use of_node_put(child) here. > */ > return; > > but there is nowhere that this refcount is dropped. The same file where we have the get should also contain the put, ideally in a reverse function like register/unregister. It is too easy to miss a put that should happen in a different context. fixed_phy_unregister seems to be one case where it put that node after phy_device_remove() but I didn't investigate it further if that was related to a different of_node_get. mdiobus_unregister_device might be a nice place to fit that put but I'm not an expert in MDIO API. > Really, the patch should be addressing the problem rather than putting > a sticky-plaster over just one instance of it. I'm trying to address an issue I ran into while modifying a DSA driver. We have drivers putting the node passed to of_mdiobus_register just after it returns. In my option, it feels more natural and this patch fixes that scenario. Other drivers keep that reference until the driver is removed, which might still be too soon without this patch. I guess putting the node should happen between mdiobus_unregister and mdiobus_free. If the driver uses devm variants, it does not control the code between those two methods and it should just hope that it is enough to put the node as its last step. I issue that the child node you pointed to should also be addressed. However, I think they are two different but related issues. Any place we see a device_set_node(), we should see a of_node_get before and a of_node_put when the device is gone. Regards, Luiz
On Tue, 2 Jan 2024 18:57:35 -0300 Luiz Angelo Daros de Luca wrote: > > I also notice that netdev have applied this without *any* review from > > phylib maintainers. Grr. > > Some reviews are required. Should we revert it? Reverted.
On Tue, Jan 02, 2024 at 06:57:35PM -0300, Luiz Angelo Daros de Luca wrote: > > On Wed, Dec 20, 2023 at 01:52:29AM -0300, Luiz Angelo Daros de Luca wrote: > > > The __of_mdiobus_register() function was storing the device node in > > > dev.of_node without increasing its reference count. It implicitly relied > > > on the caller to maintain the allocated node until the mdiobus was > > > unregistered. > > > > > > Now, __of_mdiobus_register() will acquire the node before assigning it, > > > and of_mdiobus_unregister_callback() will be called at the end of > > > mdio_unregister(). > > > > > > Drivers can now release the node immediately after MDIO registration. > > > Some of them are already doing that even before this patch. > > > > > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > > > > I don't like this, certainly not the use of a method prefixed by a > > double-underscore, and neither the conditional nature of "putting" > > this. That alone seems to point to there being more issues. > > Thanks Russel. Hi Lewis, > At least one driver (bcm_sf2_mdio_register) is writing directly to the > mii_bus->dev.of_node and not using of_mdiobus_register(). We should > not put a node in the MDIO bus if the bus didn't get it before. That's > the reason for the conditional putting the node. I agree with the idea that a node placed in a bus needs to have it's reference count increased before hand, _unless_ the reference is being passed from the code registering. What I don't agree with is the conditional putting of the node. What I think should have happened is a review of all the code, and either a justification needed to be put forward (and considered *before* this patch was merged) about why to do this conditionally, _or_ all the places where the refcounting is not correct get fixed at the same time. Adding this conditional mechanism adds more complexity which makes the situation more difficult to analyse and fix later. > I wasn't sure about the names. What would be an appropriate name? The > same without the prefix? In order to put the node only when the bus > was registered by __of_mdiobus_register, I opted for a callback but it > might be a better approach. Normally, the callback is just named "release". > > I also notice that netdev have applied this without *any* review from > > phylib maintainers. Grr. > > Some reviews are required. Should we revert it? Clearly reviews are needed, even more so as there is indeed an issue with this patch. Looking at __of_mdiobus_register(), let's assume __mdiobus_register() succeeds. While scanning the PHYs, we hit an error that calls us to head to the unregister label. This calls mdiobus_unregister(), which calls your bus->__unregister_callback function, which puts the node. When that returns, we continue past the "put_node" label, which does *another* of_node_put() on the same node. So, this patch has traded a lack-of-get for a double-put bug. Given that it wasn't reviewed before being applied, and I think we can do much better, I am definitely in the mindset that it should be reverted. > > Indeed there are more issues with the refcounting here. If one looks at > > drivers/net/phy/mdio_bus.c::of_mdiobus_link_mdiodev(), we find this: > > > > if (addr == mdiodev->addr) { > > device_set_node(dev, of_fwnode_handle(child)); > > /* The refcount on "child" is passed to the mdio > > * device. Do _not_ use of_node_put(child) here. > > */ > > return; > > > > but there is nowhere that this refcount is dropped. > > The same file where we have the get should also contain the put, > ideally in a reverse function like register/unregister. Not necessarily true. There are cases where we need the node to hang around until the device is actually released, so putting the node in the release callback for the device tends to be the best place. The rule for all devices of that class then becomes that the node must be "got" before assigning them to the device which then becomes easy to audit. > I'm trying to address an issue I ran into while modifying a DSA > driver. We have drivers putting the node passed to of_mdiobus_register > just after it returns. In my option, it feels more natural and this > patch fixes that scenario. I agree with that approach, but as you rightly point out, we need MDIO to behave correctly, and I don't think that patching just one bit of MDIO to fix this mess is the right approach. Jakub: please revert, if that's still possible.
On Wed, Jan 03, 2024 at 10:22:00AM +0000, Russell King (Oracle) wrote: > I agree with that approach, but as you rightly point out, we need MDIO > to behave correctly, and I don't think that patching just one bit of > MDIO to fix this mess is the right approach. This is probably a safer approach to ensuring that the firmware data reference count isn't dropped while the bus exists byensuring that we always take a reference at register time. It also likely fixes similar issues with ACPI and swnode based users as well. It doesn't deal with the excess-refcount problem, as with this approach the two issues are entirely independent of each other. Please test to check that this addresses your issue. Thanks. diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c index 6cf73c15635b..afbad1ad8683 100644 --- a/drivers/net/phy/mdio_bus.c +++ b/drivers/net/phy/mdio_bus.c @@ -193,6 +193,10 @@ static void mdiobus_release(struct device *d) bus->state != MDIOBUS_ALLOCATED, "%s: not in RELEASED or ALLOCATED state\n", bus->id); + + if (bus->state == MDIOBUS_RELEASED) + fwnode_handle_put(dev_fwnode(d)); + kfree(bus); } @@ -684,6 +688,15 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner) bus->dev.groups = NULL; dev_set_name(&bus->dev, "%s", bus->id); + /* If the bus state is allocated, we're registering a fresh bus + * that may have a fwnode associated with it. Grab a reference + * to the fwnode. This will be dropped when the bus is released. + * If the bus was set to unregistered, it means that the bus was + * previously registered, and we've already grabbed a reference. + */ + if (bus->state == MDIOBUS_ALLOCATED) + fwnode_handle_get(dev_fwnode(&bus->dev)); + /* We need to set state to MDIOBUS_UNREGISTERED to correctly release * the device in mdiobus_free() *
> Please test to check that this addresses your issue. Thanks. > > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c > index 6cf73c15635b..afbad1ad8683 100644 > --- a/drivers/net/phy/mdio_bus.c > +++ b/drivers/net/phy/mdio_bus.c > @@ -193,6 +193,10 @@ static void mdiobus_release(struct device *d) > bus->state != MDIOBUS_ALLOCATED, > "%s: not in RELEASED or ALLOCATED state\n", > bus->id); > + > + if (bus->state == MDIOBUS_RELEASED) > + fwnode_handle_put(dev_fwnode(d)); > + > kfree(bus); > } > > @@ -684,6 +688,15 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner) > bus->dev.groups = NULL; > dev_set_name(&bus->dev, "%s", bus->id); > > + /* If the bus state is allocated, we're registering a fresh bus > + * that may have a fwnode associated with it. Grab a reference > + * to the fwnode. This will be dropped when the bus is released. > + * If the bus was set to unregistered, it means that the bus was > + * previously registered, and we've already grabbed a reference. > + */ > + if (bus->state == MDIOBUS_ALLOCATED) > + fwnode_handle_get(dev_fwnode(&bus->dev)); > + > /* We need to set state to MDIOBUS_UNREGISTERED to correctly release > * the device in mdiobus_free() > * > -- Thanks Russel. It is much better than my approach. You simply get/put during registration/unregistration when a node is defined, no matter who defined it (of_mdiobus_register or anything else). Clean and simple. Regards, Luiz
diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c index 64ebcb6d235c..9b6cab6154e0 100644 --- a/drivers/net/mdio/of_mdio.c +++ b/drivers/net/mdio/of_mdio.c @@ -139,6 +139,11 @@ bool of_mdiobus_child_is_phy(struct device_node *child) } EXPORT_SYMBOL(of_mdiobus_child_is_phy); +static void __of_mdiobus_unregister_callback(struct mii_bus *mdio) +{ + of_node_put(mdio->dev.of_node); +} + /** * __of_mdiobus_register - Register mii_bus and create PHYs from the device tree * @mdio: pointer to mii_bus structure @@ -166,6 +171,8 @@ int __of_mdiobus_register(struct mii_bus *mdio, struct device_node *np, * the device tree are populated after the bus has been registered */ mdio->phy_mask = ~0; + mdio->__unregister_callback = __of_mdiobus_unregister_callback; + of_node_get(np); device_set_node(&mdio->dev, of_fwnode_handle(np)); /* Get bus level PHY reset GPIO details */ @@ -177,7 +184,7 @@ int __of_mdiobus_register(struct mii_bus *mdio, struct device_node *np, /* Register the MDIO bus */ rc = __mdiobus_register(mdio, owner); if (rc) - return rc; + goto put_node; /* Loop over the child nodes and register a phy_device for each phy */ for_each_available_child_of_node(np, child) { @@ -237,6 +244,9 @@ int __of_mdiobus_register(struct mii_bus *mdio, struct device_node *np, unregister: of_node_put(child); mdiobus_unregister(mdio); + +put_node: + of_node_put(np); return rc; } EXPORT_SYMBOL(__of_mdiobus_register); diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c index 25dcaa49ab8b..1229b8e4c53b 100644 --- a/drivers/net/phy/mdio_bus.c +++ b/drivers/net/phy/mdio_bus.c @@ -787,6 +787,9 @@ void mdiobus_unregister(struct mii_bus *bus) gpiod_set_value_cansleep(bus->reset_gpiod, 1); device_del(&bus->dev); + + if (bus->__unregister_callback) + bus->__unregister_callback(bus); } EXPORT_SYMBOL(mdiobus_unregister); diff --git a/include/linux/phy.h b/include/linux/phy.h index e5f1f41e399c..2b383da4d825 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -433,6 +433,9 @@ struct mii_bus { /** @shared: shared state across different PHYs */ struct phy_package_shared *shared[PHY_MAX_ADDR]; + + /** @__unregister_callback: called at the last step of unregistration */ + void (*__unregister_callback)(struct mii_bus *bus); }; #define to_mii_bus(d) container_of(d, struct mii_bus, dev)
The __of_mdiobus_register() function was storing the device node in dev.of_node without increasing its reference count. It implicitly relied on the caller to maintain the allocated node until the mdiobus was unregistered. Now, __of_mdiobus_register() will acquire the node before assigning it, and of_mdiobus_unregister_callback() will be called at the end of mdio_unregister(). Drivers can now release the node immediately after MDIO registration. Some of them are already doing that even before this patch. Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> --- drivers/net/mdio/of_mdio.c | 12 +++++++++++- drivers/net/phy/mdio_bus.c | 3 +++ include/linux/phy.h | 3 +++ 3 files changed, 17 insertions(+), 1 deletion(-)