Re: [PATCH net-next] mdio: mdiobus: setup of_node for the MDIO device

[Ioana Ciornei <ciorneiioana@gmail.com>]

On Wed, Jun 16, 2021 at 10:40:12AM +0100, Russell King (Oracle) wrote:
> On Wed, Jun 16, 2021 at 11:20:52AM +0300, Ioana Ciornei wrote:
> > On Tue, Jun 15, 2021 at 11:31:47PM +0200, Andrew Lunn wrote:
> > > > The fwnode_operations declared in drivers/acpi/property.c also suggest
> > > > the ACPI fwnodes are not refcounted.
> > > 
> > > Is this because ACPI is not dynamic, unlike DT, where you can
> > > add/remove overlays at runtime?
> > > 
> > 
> > I am really not an expert here but the git history suggests so, yes.
> > 
> > Without the CONFIG_OF_DYNAMIC enabled, the fwnode_handle_get() call is
> > actually a no-op even in the OF case.
> 
> More accurately, of_node_get() is a no-op if CONFIG_OF_DYNAMIC is
> disabled, which in turn makes fwnode_handle_get() also a no-op.
> 
> I'm wondering whether we would need two helpers to assign these, or
> just a single helper that takes a fwnode and assigns both pointers.
> to_of_node() returns NULL if the fwnode is not a DT node, so would
> be safe to use even with ACPI.
> 

Yes, I think this approach was exactly what Andrew suggested initially.

> Then there's the cleanup side when the device is released. I haven't
> yet found where we release the reference to the fwnode/of_node when
> we release the phy_device. I would have expected it in
> phy_device_release() but that does nothing.

Looking at the fixed_phy.c use of the refcounts, I would expect that a
call to fwnode_handle_put/of_node_put should be right after a
phy_device_remove() call is made.

	void fixed_phy_unregister(struct phy_device *phy)
	{
		phy_device_remove(phy);
		of_node_put(phy->mdio.dev.of_node);
		fixed_phy_del(phy->mdio.addr);
	}


Now going back to the phy_device.c, the phy_device_remove() call is done
in phy_mdio_device_remove. This is the device_remove callback of any PHY
MDIO device, called when, for example, the MDIO bus is unregistered.

After a first pass through the code, I would expect the refcount to be
released in phy_mdio_device_remove().

> We could only add that
> when we are certain that all users who assign the firmware node to
> the phy device has properly refcounted it in the DT case.
> 

Agree. I think we need a proper mapping of the register/unregister code
paths before any of_node/fwnode_handle put is added.

Ioana