| Message ID | 20241028-ftgmac-fixes-v1-2-b334a507be6c@codeconstruct.com.au (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: ftgmac100: fixes for ncsi/phy handling on device remove | expand |
Hi Jeremy,
I checked the phy_disconnect function again.
It will assign NULL pointer to netdev->phydev after calling itself.
In NC-SI mode, it does cause a crash when calling fixed_phy_unregister(netdev->phydev).
Thank you for your help in pointing out my mistack.
Thanks,
Jacky
Reviewed-by: Jacky Chou <jacky_chou@aspeedtech.com>
On 10/27/2024 9:54 PM, Jeremy Kerr wrote: > Commit e24a6c874601 ("net: ftgmac100: Get link speed and duplex for > NC-SI") introduced a fixed phydev attached to the ftgmac netdev for ncsi > configurations, cleaned up on remove as: > > phy_disconnect(netdev->phydev); > > /* ... */ > > if (priv->use_ncsi) > fixed_phy_unregister(netdev->phydev); > > However, phy_disconnect() will clear the netdev's ->phydev pointer, so > the fixed_phy_unregister() will always be invoked with a null pointer. > > Use a temporary for the phydev, rather than expecting the netdev->phydev > point to be valid over the phy_disconnect(). > > Fixes: e24a6c874601 ("net: ftgmac100: Get link speed and duplex for NC-SI") > Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au> > --- Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
On Mon, Oct 28, 2024 at 12:54:11PM +0800, Jeremy Kerr wrote: > Commit e24a6c874601 ("net: ftgmac100: Get link speed and duplex for > NC-SI") introduced a fixed phydev attached to the ftgmac netdev for ncsi > configurations, cleaned up on remove as: > > phy_disconnect(netdev->phydev); > > /* ... */ > > if (priv->use_ncsi) > fixed_phy_unregister(netdev->phydev); > > However, phy_disconnect() will clear the netdev's ->phydev pointer, so > the fixed_phy_unregister() will always be invoked with a null pointer. > > Use a temporary for the phydev, rather than expecting the netdev->phydev > point to be valid over the phy_disconnect(). > > Fixes: e24a6c874601 ("net: ftgmac100: Get link speed and duplex for NC-SI") > Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au> > --- > drivers/net/ethernet/faraday/ftgmac100.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c > index 9caee68468ff5f71d7ea63a0c8c9ec2be4a718bc..c6ed7ed0e2389a45a671b85ae60936df99458cd1 100644 > --- a/drivers/net/ethernet/faraday/ftgmac100.c > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > @@ -1730,16 +1730,17 @@ static int ftgmac100_setup_mdio(struct net_device *netdev) > static void ftgmac100_phy_disconnect(struct net_device *netdev) This all seems rather hacky. What is the mirror function to ftgmac100_phy_disconnect(). I don't see a ftgmac100_phy_connect(). Generally, if the teardown function does the same as the setup function, but reverse order, it just works.... Andrew
Hi Andrew, > This all seems rather hacky. What is the mirror function to > ftgmac100_phy_disconnect(). I don't see a > ftgmac100_phy_connect(). There are different paths in physical-phy vs ncsi, so they're implemented differently in ftgmac100_probe() based on those configurations. If you feel the driver needs a rework for the phy setup, that's fine, but I assume that's not something we want to add as a backported change in the net tree. Cheers, Jeremy
On Tue, Oct 29, 2024 at 12:36:44PM +0800, Jeremy Kerr wrote: > Hi Andrew, > > This all seems rather hacky. What is the mirror function to > > ftgmac100_phy_disconnect(). I don't see a > > ftgmac100_phy_connect(). > > There are different paths in physical-phy vs ncsi, so they're > implemented differently in ftgmac100_probe() based on those > configurations. > > If you feel the driver needs a rework for the phy setup, that's fine, > but I assume that's not something we want to add as a backported change > in the net tree. Lets see how big those changes are. If a fix needs a patcheset of lots of small obviously correct changes, GregKH will accept them for stable. Andrew
diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c index 9caee68468ff5f71d7ea63a0c8c9ec2be4a718bc..c6ed7ed0e2389a45a671b85ae60936df99458cd1 100644 --- a/drivers/net/ethernet/faraday/ftgmac100.c +++ b/drivers/net/ethernet/faraday/ftgmac100.c @@ -1730,16 +1730,17 @@ static int ftgmac100_setup_mdio(struct net_device *netdev) static void ftgmac100_phy_disconnect(struct net_device *netdev) { struct ftgmac100 *priv = netdev_priv(netdev); + struct phy_device *phydev = netdev->phydev; - if (!netdev->phydev) + if (!phydev) return; - phy_disconnect(netdev->phydev); + phy_disconnect(phydev); if (of_phy_is_fixed_link(priv->dev->of_node)) of_phy_deregister_fixed_link(priv->dev->of_node); if (priv->use_ncsi) - fixed_phy_unregister(netdev->phydev); + fixed_phy_unregister(phydev); } static void ftgmac100_destroy_mdio(struct net_device *netdev)
Commit e24a6c874601 ("net: ftgmac100: Get link speed and duplex for NC-SI") introduced a fixed phydev attached to the ftgmac netdev for ncsi configurations, cleaned up on remove as: phy_disconnect(netdev->phydev); /* ... */ if (priv->use_ncsi) fixed_phy_unregister(netdev->phydev); However, phy_disconnect() will clear the netdev's ->phydev pointer, so the fixed_phy_unregister() will always be invoked with a null pointer. Use a temporary for the phydev, rather than expecting the netdev->phydev point to be valid over the phy_disconnect(). Fixes: e24a6c874601 ("net: ftgmac100: Get link speed and duplex for NC-SI") Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au> --- drivers/net/ethernet/faraday/ftgmac100.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)