Re: [PATCH net] net: ibm: emac: attach the fixed-link PHY with of_phy_connect()

[Andrew Lunn <andrew@lunn.ch>]

On Thu, Nov 12, 2026 at 03:53:21PM +0800, Linkui Xiao wrote:
> From: Linkui Xiao <xiaolinkui@kylinos.cn>
> 
> emac_init_phy() handles a "fixed-link" property by registering a fixed
> PHY for the MAC's own OF node and then assigning net_device->phydev to
> it by hand:
> 
> 	res = of_phy_register_fixed_link(np);
> 	ndev->phydev = of_phy_find_device(np);
> 	if (res || !ndev->phydev)
> 		return res ? res : -EINVAL;
> 	emac_adjust_link(dev->ndev);
> 	put_device(&ndev->phydev->mdio.dev);
> 
> net_device->phydev is meant to be set by phy_attach_direct(), which is
> what the of_phy_connect() helper reaches. Assigning it by hand skips
> everything attach does: phydev->attached_dev stays NULL, no link change
> handler is wired up, the PHY never enters the netdev's phy_link_topology,
> and the put_device() drops the reference the driver just took, so the
> phy_device is kept alive afterwards only by the fixed MDIO bus it is
> registered on.
> 
> The missing topology entry is what prompted this. ethnl_req_get_phydev()
> returns dev->phydev as-is when an ethtool netlink request carries no
> phy-index, so ETHTOOL_MSG_PHY_GET on an EMAC port with a fixed-link finds
> a phydev and then looks it up in a link_topo that was never allocated:
> 
> 	struct phy_link_topology *topo = reply_data->dev->link_topo;
> 	pdn = xa_load(&topo->phys, phydev->phyindex);
> 
> which dereferences NULL in phy_prepare_data().
> 
> Every other phylib driver with fixed-link support registers the fixed PHY
> and then passes the MAC's own OF node to of_phy_connect(): gianfar, fec,
> davinci_emac and nixge all do it, and so does emac_dt_phy_connect() for
> the phy-handle case. Do the same here. of_phy_connect() takes the attach
> reference and drops the of_phy_find_device() one, so the explicit
> put_device() goes away. emac_adjust_link() is still called once by hand
> afterwards and the PHY state machine is still not started, because a
> fixed link does not change. The rest of the driver already treats
> dev->phy.address < 0 as "no PHY to poll" and never reaches
> dev->phy.def->ops, so nothing else changes.
> 
> The hand assignment itself dates back to commit 08e39982ef64 ("net: emac:
> fix fixed-link setup for the RTL8363SB switch"), but it only started
> affecting the net_device when commit baab9de385a8 ("net: ibm: emac: use
> netdev's phydev directly") moved the pointer from the driver private
> dev->phy_dev into ndev->phydev, so that is the commit fixed here.
> 
> No in-tree DTS pairs "ibm,emac" with a "fixed-link" property, so this is
> latent in mainline; it is out-of-tree users such as the Netgear WNDAP620
> that hit it. There is no such hardware here, so the change is verified by
> cross compilation and inspection only.
> 
> emac_remove() still does not call phy_disconnect(), which the phy-handle
> path gets wrong today as well. That is left to a separate change.

This commit message is overly verbose and contains a lot of irrelevant
information. That is typical of LMM generated commit messages.

Please try writing the commit message by hand. Its O.K. to write it in
your first language and use a LLM to translate it to English.

The commit message is about "Why?". And it is there to answer
questions the reviewer might have. This last sentence about
phy_disconnect() was one of the things i thought about while looking
at the change, but because the commit messages was so long, i had
given up reading it....

> Fixes: baab9de385a8 ("net: ibm: emac: use netdev's phydev directly")
> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
> ---
>  drivers/net/ethernet/ibm/emac/core.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> index 640618c..6e8b711 100644
> --- a/drivers/net/ethernet/ibm/emac/core.c
> +++ b/drivers/net/ethernet/ibm/emac/core.c
> @@ -2674,11 +2674,14 @@ static int emac_init_phy(struct emac_instance *dev)
>  				return res;
>  
>  			res = of_phy_register_fixed_link(np);
> -			ndev->phydev = of_phy_find_device(np);
> -			if (res || !ndev->phydev)
> -				return res ? res : -EINVAL;
> -			emac_adjust_link(dev->ndev);
> -			put_device(&ndev->phydev->mdio.dev);
> +			if (res)
> +				return res;
> +
> +			if (!of_phy_connect(ndev, np, &emac_adjust_link, 0,
> +					    dev->phy_mode))
> +				return -EINVAL;
> +
> +			emac_adjust_link(ndev);

Why is emacs_adjust_link() still called?

    Andrew

---
pw-bot: cr