| Message ID | 20180306150006.31512-1-jeremy.linton@arm.com |
|---|---|
| State | Accepted |
| Commit | e06513d78d54e6c7026c9043a39e2c01ee25bdbe |
| Headers | show |
| Series | net: smsc911x: Fix unload crash when link is up | expand |
> This is caused by the mdiobus being unregistered/free'd > and the code in phy_detach() attempting to manipulate mdio > related structures from unregister_netdev() calling close() > > To fix this, we delay the mdiobus teardown until after > the netdev is deregistered. > > Reported-by: Matt Sealey <matt.sealey@arm.com> > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > drivers/net/ethernet/smsc/smsc911x.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c > index 012fb66eed8d..f0afb88d7bc2 100644 > --- a/drivers/net/ethernet/smsc/smsc911x.c > +++ b/drivers/net/ethernet/smsc/smsc911x.c > @@ -2335,14 +2335,14 @@ static int smsc911x_drv_remove(struct platform_device *pdev) > pdata = netdev_priv(dev); > BUG_ON(!pdata); > BUG_ON(!pdata->ioaddr); > - WARN_ON(dev->phydev); Hi Jeremy I assume this WARN_ON() also fired? It would be good to comment about why you removed it, that the code now handles that case. Apart from that Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
Hi, On 03/06/2018 09:23 AM, Andrew Lunn wrote: >> This is caused by the mdiobus being unregistered/free'd >> and the code in phy_detach() attempting to manipulate mdio >> related structures from unregister_netdev() calling close() >> >> To fix this, we delay the mdiobus teardown until after >> the netdev is deregistered. >> >> Reported-by: Matt Sealey <matt.sealey@arm.com> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >> --- >> drivers/net/ethernet/smsc/smsc911x.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c >> index 012fb66eed8d..f0afb88d7bc2 100644 >> --- a/drivers/net/ethernet/smsc/smsc911x.c >> +++ b/drivers/net/ethernet/smsc/smsc911x.c >> @@ -2335,14 +2335,14 @@ static int smsc911x_drv_remove(struct platform_device *pdev) >> pdata = netdev_priv(dev); >> BUG_ON(!pdata); >> BUG_ON(!pdata->ioaddr); >> - WARN_ON(dev->phydev); > > Hi Jeremy > > I assume this WARN_ON() also fired? It would be good to comment about > why you removed it, that the code now handles that case. Yes, the phydev is started and assigned in the netdev _open and stopped/set to null in the _stop. Since the module remove is not blocked by having the netdev active, and unregister_netdev closes out active connections, the WARN_ON would needlessly trigger if the netdev was still open. > > Apart from that > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> Thanks for looking at this.
From: Jeremy Linton <jeremy.linton@arm.com> Date: Tue, 6 Mar 2018 09:00:06 -0600 > The smsc911x driver will crash if it is rmmod'ed while the netdev > is up like: > > Call trace: ... > This is caused by the mdiobus being unregistered/free'd > and the code in phy_detach() attempting to manipulate mdio > related structures from unregister_netdev() calling close() > > To fix this, we delay the mdiobus teardown until after > the netdev is deregistered. > > Reported-by: Matt Sealey <matt.sealey@arm.com> > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> Applied, thanks.
diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c index 012fb66eed8d..f0afb88d7bc2 100644 --- a/drivers/net/ethernet/smsc/smsc911x.c +++ b/drivers/net/ethernet/smsc/smsc911x.c @@ -2335,14 +2335,14 @@ static int smsc911x_drv_remove(struct platform_device *pdev) pdata = netdev_priv(dev); BUG_ON(!pdata); BUG_ON(!pdata->ioaddr); - WARN_ON(dev->phydev); SMSC_TRACE(pdata, ifdown, "Stopping driver"); + unregister_netdev(dev); + mdiobus_unregister(pdata->mii_bus); mdiobus_free(pdata->mii_bus); - unregister_netdev(dev); res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "smsc911x-memory"); if (!res)
The smsc911x driver will crash if it is rmmod'ed while the netdev is up like: Call trace: phy_detach+0x94/0x150 phy_disconnect+0x40/0x50 smsc911x_stop+0x104/0x128 [smsc911x] __dev_close_many+0xb4/0x138 dev_close_many+0xbc/0x190 rollback_registered_many+0x140/0x460 rollback_registered+0x68/0xb0 unregister_netdevice_queue+0x100/0x118 unregister_netdev+0x28/0x38 smsc911x_drv_remove+0x58/0x130 [smsc911x] platform_drv_remove+0x30/0x50 device_release_driver_internal+0x15c/0x1f8 driver_detach+0x54/0x98 bus_remove_driver+0x64/0xe8 driver_unregister+0x34/0x60 platform_driver_unregister+0x20/0x30 smsc911x_cleanup_module+0x14/0xbca8 [smsc911x] SyS_delete_module+0x1e8/0x238 __sys_trace_return+0x0/0x4 This is caused by the mdiobus being unregistered/free'd and the code in phy_detach() attempting to manipulate mdio related structures from unregister_netdev() calling close() To fix this, we delay the mdiobus teardown until after the netdev is deregistered. Reported-by: Matt Sealey <matt.sealey@arm.com> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- drivers/net/ethernet/smsc/smsc911x.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.13.6