Patchwork [NET] several cleanups and bugfixes for fec.c: fix the .remove code

login
register
mail settings
Submitter Lothar Waßmann
Date Dec. 6, 2011, 10:27 a.m.
Message ID <04c11563fee748562d7b53d5e5b3c5cf37fef200.1323163127.git.LW@KARO-electronics.de>
Download mbox | patch
Permalink /patch/129606/
State New
Headers show

Comments

Lothar Waßmann - Dec. 6, 2011, 10:27 a.m.
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(-)
Lothar Waßmann - Dec. 7, 2011, 1:26 p.m.
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
Shawn Guo - Dec. 7, 2011, 1:28 p.m.
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
Shawn Guo - Dec. 7, 2011, 1:43 p.m.
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>

Patch

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);