netdev - Re: [PATCH net-next] mdio: mdiobus: setup of

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20210616110115.i3zykpc73qxu5odk@skbuf>
Date:   Wed, 16 Jun 2021 14:01:15 +0300
From:   Ioana Ciornei <ciorneiioana@...il.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Ioana Ciornei <ciorneiioana@...il.com>,
        Andrew Lunn <andrew@...n.ch>, davem@...emloft.net,
        kuba@...nel.org, netdev@...r.kernel.org,
        calvin.johnson@....nxp.com, hkallweit1@...il.com,
        Ioana Ciornei <ioana.ciornei@....com>
Subject: Re: [PATCH net-next] mdio: mdiobus: setup of_node for the MDIO device

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