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

Shawn Guo
2011-12-07T13:28:29Z

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