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

[Shawn Guo]

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@xxxxxxxxxxxxxxxxxxx>
> ---
>  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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/