| Message ID | 04c11563fee748562d7b53d5e5b3c5cf37fef200.1323163127.git.LW@KARO-electronics.de |
|---|---|
| State | New |
| Headers | show |
Hi, Shawn Guo writes: > On Tue, Dec 06, 2011 at 11:27:15AM +0100, Lothar Waßmann wrote: > > The .remove code is broken in several ways. > > - mdiobus_unregister() is called twice for the same object in case of dual FEC > > - phy_disconnect() is being called when the PHY is already disconnected > > - the requested IRQ(s) are not freed > > - fec_stop() is being called with the inteface already stopped > > > > All of those lead to kernel crashes if the remove function is actually used. > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > --- > > drivers/net/ethernet/freescale/fec.c | 30 +++++++++++++++++++++--------- > > 1 files changed, 21 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c > > index ab0afb5..70ec7ec 100644 > > --- a/drivers/net/ethernet/freescale/fec.c > > +++ b/drivers/net/ethernet/freescale/fec.c > > @@ -235,6 +235,7 @@ struct fec_enet_private { > > > > /* Phylib and MDIO interface */ > > struct mii_bus *mii_bus; > > + int mii_cnt; > > struct phy_device *phy_dev; > > int mii_timeout; > > uint phy_speed; > > @@ -1040,8 +1041,12 @@ static int fec_enet_mii_init(struct platform_device *pdev) > > */ > > if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && pdev->id > 0) { > > /* fec1 uses fec0 mii_bus */ > > - fep->mii_bus = fec0_mii_bus; > > - return 0; > > + if (fep->mii_cnt && fec0_mii_bus) { > > This seems broken. The second fec has its own fep and fep->mii_cnt is > always 0 here. > I already noticed that myself. Lothar Waßmann
On Tue, Dec 06, 2011 at 11:27:15AM +0100, Lothar Waßmann wrote: > The .remove code is broken in several ways. > - mdiobus_unregister() is called twice for the same object in case of dual FEC > - phy_disconnect() is being called when the PHY is already disconnected > - the requested IRQ(s) are not freed > - fec_stop() is being called with the inteface already stopped > > All of those lead to kernel crashes if the remove function is actually used. > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > --- > drivers/net/ethernet/freescale/fec.c | 30 +++++++++++++++++++++--------- > 1 files changed, 21 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c > index ab0afb5..70ec7ec 100644 > --- a/drivers/net/ethernet/freescale/fec.c > +++ b/drivers/net/ethernet/freescale/fec.c > @@ -235,6 +235,7 @@ struct fec_enet_private { > > /* Phylib and MDIO interface */ > struct mii_bus *mii_bus; > + int mii_cnt; > struct phy_device *phy_dev; > int mii_timeout; > uint phy_speed; > @@ -1040,8 +1041,12 @@ static int fec_enet_mii_init(struct platform_device *pdev) > */ > if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && pdev->id > 0) { > /* fec1 uses fec0 mii_bus */ > - fep->mii_bus = fec0_mii_bus; > - return 0; > + if (fep->mii_cnt && fec0_mii_bus) { This seems broken. The second fec has its own fep and fep->mii_cnt is always 0 here. Regards, Shawn > + fep->mii_bus = fec0_mii_bus; > + fep->mii_cnt++; > + return 0; > + } > + return -ENOENT; > } > > fep->mii_timeout = 0; > @@ -1086,6 +1091,8 @@ static int fec_enet_mii_init(struct platform_device *pdev) > if (mdiobus_register(fep->mii_bus)) > goto err_out_free_mdio_irq; > > + fep->mii_cnt++; > + > /* save fec0 mii_bus */ > if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) > fec0_mii_bus = fep->mii_bus; > @@ -1102,11 +1109,11 @@ err_out: > > static void fec_enet_mii_remove(struct fec_enet_private *fep) > { > - if (fep->phy_dev) > - phy_disconnect(fep->phy_dev); > - mdiobus_unregister(fep->mii_bus); > - kfree(fep->mii_bus->irq); > - mdiobus_free(fep->mii_bus); > + if (--fep->mii_cnt == 0) { > + mdiobus_unregister(fep->mii_bus); > + kfree(fep->mii_bus->irq); > + mdiobus_free(fep->mii_bus); > + } > } > > static int fec_enet_get_settings(struct net_device *ndev, > @@ -1646,13 +1653,18 @@ fec_drv_remove(struct platform_device *pdev) > struct net_device *ndev = platform_get_drvdata(pdev); > struct fec_enet_private *fep = netdev_priv(ndev); > struct resource *r; > + int i; > > - fec_stop(ndev); > + unregister_netdev(ndev); > fec_enet_mii_remove(fep); > + for (i = 0; i < FEC_IRQ_NUM; i++) { > + int irq = platform_get_irq(pdev, i); > + if (irq > 0) > + free_irq(irq, ndev); > + } > clk_disable(fep->clk); > clk_put(fep->clk); > iounmap(fep->hwp); > - unregister_netdev(ndev); > free_netdev(ndev); > > r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > -- > 1.5.6.5
On Wed, Dec 07, 2011 at 02:26:34PM +0100, Lothar Waßmann wrote: > Hi, > > Shawn Guo writes: > > On Tue, Dec 06, 2011 at 11:27:15AM +0100, Lothar Waßmann wrote: > > > The .remove code is broken in several ways. > > > - mdiobus_unregister() is called twice for the same object in case of dual FEC > > > - phy_disconnect() is being called when the PHY is already disconnected > > > - the requested IRQ(s) are not freed > > > - fec_stop() is being called with the inteface already stopped > > > > > > All of those lead to kernel crashes if the remove function is actually used. > > > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > > --- > > > drivers/net/ethernet/freescale/fec.c | 30 +++++++++++++++++++++--------- > > > 1 files changed, 21 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c > > > index ab0afb5..70ec7ec 100644 > > > --- a/drivers/net/ethernet/freescale/fec.c > > > +++ b/drivers/net/ethernet/freescale/fec.c > > > @@ -235,6 +235,7 @@ struct fec_enet_private { > > > > > > /* Phylib and MDIO interface */ > > > struct mii_bus *mii_bus; > > > + int mii_cnt; > > > struct phy_device *phy_dev; > > > int mii_timeout; > > > uint phy_speed; > > > @@ -1040,8 +1041,12 @@ static int fec_enet_mii_init(struct platform_device *pdev) > > > */ > > > if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && pdev->id > 0) { > > > /* fec1 uses fec0 mii_bus */ > > > - fep->mii_bus = fec0_mii_bus; > > > - return 0; > > > + if (fep->mii_cnt && fec0_mii_bus) { > > > > This seems broken. The second fec has its own fep and fep->mii_cnt is > > always 0 here. > > > I already noticed that myself. > With a quick fix here, the last two patches: Tested-by: Shawn Guo <shawn.guo@linaro.org>
diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c index ab0afb5..70ec7ec 100644 --- a/drivers/net/ethernet/freescale/fec.c +++ b/drivers/net/ethernet/freescale/fec.c @@ -235,6 +235,7 @@ struct fec_enet_private { /* Phylib and MDIO interface */ struct mii_bus *mii_bus; + int mii_cnt; struct phy_device *phy_dev; int mii_timeout; uint phy_speed; @@ -1040,8 +1041,12 @@ static int fec_enet_mii_init(struct platform_device *pdev) */ if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && pdev->id > 0) { /* fec1 uses fec0 mii_bus */ - fep->mii_bus = fec0_mii_bus; - return 0; + if (fep->mii_cnt && fec0_mii_bus) { + fep->mii_bus = fec0_mii_bus; + fep->mii_cnt++; + return 0; + } + return -ENOENT; } fep->mii_timeout = 0; @@ -1086,6 +1091,8 @@ static int fec_enet_mii_init(struct platform_device *pdev) if (mdiobus_register(fep->mii_bus)) goto err_out_free_mdio_irq; + fep->mii_cnt++; + /* save fec0 mii_bus */ if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) fec0_mii_bus = fep->mii_bus; @@ -1102,11 +1109,11 @@ err_out: static void fec_enet_mii_remove(struct fec_enet_private *fep) { - if (fep->phy_dev) - phy_disconnect(fep->phy_dev); - mdiobus_unregister(fep->mii_bus); - kfree(fep->mii_bus->irq); - mdiobus_free(fep->mii_bus); + if (--fep->mii_cnt == 0) { + mdiobus_unregister(fep->mii_bus); + kfree(fep->mii_bus->irq); + mdiobus_free(fep->mii_bus); + } } static int fec_enet_get_settings(struct net_device *ndev, @@ -1646,13 +1653,18 @@ fec_drv_remove(struct platform_device *pdev) struct net_device *ndev = platform_get_drvdata(pdev); struct fec_enet_private *fep = netdev_priv(ndev); struct resource *r; + int i; - fec_stop(ndev); + unregister_netdev(ndev); fec_enet_mii_remove(fep); + for (i = 0; i < FEC_IRQ_NUM; i++) { + int irq = platform_get_irq(pdev, i); + if (irq > 0) + free_irq(irq, ndev); + } clk_disable(fep->clk); clk_put(fep->clk); iounmap(fep->hwp); - unregister_netdev(ndev); free_netdev(ndev); r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
The .remove code is broken in several ways. - mdiobus_unregister() is called twice for the same object in case of dual FEC - phy_disconnect() is being called when the PHY is already disconnected - the requested IRQ(s) are not freed - fec_stop() is being called with the inteface already stopped All of those lead to kernel crashes if the remove function is actually used. Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> --- drivers/net/ethernet/freescale/fec.c | 30 +++++++++++++++++++++--------- 1 files changed, 21 insertions(+), 9 deletions(-)