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 <[email protected]>
---
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(-)
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)
--
2.43.0
Hello:
This patch was applied to netdev/net-next.git (main)
by David S. Miller <[email protected]>:
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
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 <[email protected]>
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> 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 <[email protected]>
>
> 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 <[email protected]>
> >
> > 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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
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()
*
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> 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