[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
<SEYPR06MB513455F4FBCB9AF4F6E4CB199D97A@SEYPR06MB5134.apcprd06.prod.outlook.com>
Date: Thu, 22 Jan 2026 07:31:58 +0000
From: Jacky Chou <jacky_chou@...eedtech.com>
To: Andrew Lunn <andrew@...n.ch>, Simon Horman <horms@...nel.org>
CC: "davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com"
<edumazet@...gle.com>, "andrew+netdev@...n.ch" <andrew+netdev@...n.ch>,
"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: [net-next,v2,09/15] net: ftgmac100: Always register the MDIO bus when
it exists
Hi Andrew,
> On Tue, Jan 20, 2026 at 12:14:07PM +0000, Simon Horman wrote:
> > This is an AI-generated review of your patch. The human sending this
> > email has considered the AI review valid, or at least plausible.
> > Full review at: https://netdev-ai.bots.linux.dev/ai-review.html
> > ---
> > net: ftgmac100: Always register the MDIO bus when it exists
> >
> > This patch moves MDIO bus registration earlier in the probe function
> > for FARADAY, AST2400, and AST2500 MACs, simplifying the code by
> > registering it unconditionally when the hardware has an MDIO controller.
> >
> > Note: These issues appear to be fixed later in the series by commits
> > 8961daa5f2c0 ("Simplify error handling for ftgmac100_setup_mdio") and
> > 046e51399f0f ("Move DT probe into a helper").
> >
> > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
> > > b/drivers/net/ethernet/faraday/ftgmac100.c
> > > index f1cb5dc37919..931fdf3d07d1 100644
> > > --- a/drivers/net/ethernet/faraday/ftgmac100.c
> > > +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> > > @@ -1957,6 +1957,14 @@ static int ftgmac100_probe(struct
> platform_device *pdev)
> > > priv->txdes0_edotr_mask = BIT(15);
> > > }
> > >
> > > + if (priv->mac_id == FTGMAC100_FARADAY ||
> > > + priv->mac_id == FTGMAC100_AST2400 ||
> > > + priv->mac_id == FTGMAC100_AST2500) {
> > > + err = ftgmac100_setup_mdio(netdev);
> > > + if (err)
> > > + goto err_phy_connect;
> > > + }
> >
> > If ftgmac100_setup_mdio() fails after of_mdiobus_register() returns an
> > error, it calls mdiobus_free(priv->mii_bus) but does not set
> > priv->mii_bus to NULL. The goto err_phy_connect then leads to
> > ftgmac100_destroy_mdio() which checks if (!priv->mii_bus) but finds a
> > dangling pointer, resulting in mdiobus_unregister() on freed memory
> followed by a double free.
>
> The call to register the MDIO bus has been moved earlier. That means the call
> to ftgmac100_destroy_mdio() the bus also needs to move earlier on the error
> cleanup path, before err_phy_connect:
>
I've been thinking about this part. One possible approach is to switch to
devm_mdiobus_alloc() and devm_of_mdiobus_register() instead of
mdiobus_alloc() and of_mdiobus_register().
With devm-managed MDIO resources, the explicit cleanup via
ftgmac100_destroy_mdio() would no longer be necessary, and the error paths
would become simpler and safer.
This should also avoid ordering issues in the error cleanup paths when MDIO
registration is moved earlier in the probe sequence.
Thanks,
Jacky
Powered by blists - more mailing lists