several cleanups and bugfixes for fec.c

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 <[email protected]>

---
 drivers/net/ethernet/freescale/fec.c |   30 +++++++++++++++++++++---------
 1 files changed, 21 insertions(+), 9 deletions(-)

drivers/net/ethernet/freescale/fec.c +1 -0

index ab0afb5..70ec7ec 100644
@@ -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;

drivers/net/ethernet/freescale/fec.c +19 -7

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

Signature · 1 line

1.5.6.5

Quoted 19 lines · Lothar Wa?mann

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 <[email protected]>

---
 drivers/net/ethernet/freescale/fec.c |   30 +++++++++++++++++++++---------
 1 files changed, 21 insertions(+), 9 deletions(-)

drivers/net/ethernet/freescale/fec.c +0 -0

index ab0afb5..70ec7ec 100644
@@ -235,6 +235,7 @@ struct fec_enet_private {
 

drivers/net/ethernet/freescale/fec.c static int fec_enet_mii_init(struct platform_device *pdev) · line 1044

Quoted 13 lines

drivers/net/ethernet/freescale/fec.c +2 -2

 	/* 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.

Hi,

Quoted 20 lines · Shawn Guo

Quoted 19 lines · Lothar Waßmann

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 <[email protected]>

---
 drivers/net/ethernet/freescale/fec.c |   30 +++++++++++++++++++++---------
 1 files changed, 21 insertions(+), 9 deletions(-)

drivers/net/ethernet/freescale/fec.c +0 -0

index ab0afb5..70ec7ec 100644
@@ -235,6 +235,7 @@ struct fec_enet_private {
 

Quoted 17 lines

drivers/net/ethernet/freescale/fec.c static int fec_enet_mii_init(struct platform_device *pdev) · line 1044

Quoted 13 lines

drivers/net/ethernet/freescale/fec.c +2 -2

 	/* 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.

Additionally to setting the ETHER_EN bit in FEC_ECNTRL the MII/RMII setting in FEC_R_CNTRL needs to be preserved to keep the MII interface

functional.

Signed-off-by Lothar Waßmann <[email protected]>

---
 drivers/net/ethernet/freescale/fec.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

Quoted 2 lines · Lothar Wa?mann

Additionally to setting the ETHER_EN bit in FEC_ECNTRL the MII/RMII setting in FEC_R_CNTRL needs to be preserved to keep the MII interface

s/MII/RMII? From what I see from imx28 and imx6q RM, the reset state for this setting is MII mode.

Quoted 2 lines

functional.

Quoted 4 lines

Signed-off-by Lothar Wa?mann <[email protected]>

---
 drivers/net/ethernet/freescale/fec.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

I assume this is fixing a problem you are seeing on imx28 only. Do you see the problem on imx53/51?

drivers/net/ethernet/freescale/fec.c fec_stop(struct net_device *ndev) · line 518

Quoted 10 lines

drivers/net/ethernet/freescale/fec.c +1 -0

index 11534b9..ab0afb5 100644
@@ -515,6 +515,7 @@ fec_stop(struct net_device *ndev)
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	const struct platform_device_id *id_entry =
 				platform_get_device_id(fep->pdev);
+	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);

This bit is only available on ENET (imx28 and imx6q). Do we want to do the same thing for FEC (imx25/27/35/51/53)?

Quoted 5 lines

drivers/net/ethernet/freescale/fec.c +0 -0

 	/* We cannot expect a graceful transmit stop without link !!! */
 	if (fep->link) {

@@ -531,8 +532,10 @@ fec_stop(struct net_device *ndev)
 	writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
 

drivers/net/ethernet/freescale/fec.c fec_stop(struct net_device *ndev) · line 539

Quoted 7 lines

drivers/net/ethernet/freescale/fec.c +3 -1

 	/* We have to keep ENET enabled to have MII interrupt stay working */
-	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC)
+	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) {
 		writel(2, fep->hwp + FEC_ECNTRL);
+		writel(rmii_mode, fep->hwp + FEC_R_CNTRL);
+	}
 }

On imx6q, we have two bits, bit 6 and 8 of FEC_R_CNTRL, to select MII interface among MII, RMII and RGMII modes. I'm not sure if we will run into the same problem for RGMII mode. What's your test setup? I would try to reproduce it here.

Hi,

Quoted 9 lines · Shawn Guo

Quoted 2 lines · Lothar Waßmann

Additionally to setting the ETHER_EN bit in FEC_ECNTRL the MII/RMII setting in FEC_R_CNTRL needs to be preserved to keep the MII interface

s/MII/RMII? From what I see from imx28 and imx6q RM, the reset state for this setting is MII mode.

Quoted 2 lines

functional.

Quoted 8 lines

Quoted 4 lines

Signed-off-by Lothar Waßmann <[email protected]>

---
 drivers/net/ethernet/freescale/fec.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

I assume this is fixing a problem you are seeing on imx28 only. Do you see the problem on imx53/51?

No. i.MX53 uses the RMII gasket which is not affected by resetting the controller. And imMX51 does not support RMII at all.

Quoted 14 lines

drivers/net/ethernet/freescale/fec.c fec_stop(struct net_device *ndev) · line 518

Quoted 10 lines

drivers/net/ethernet/freescale/fec.c +1 -0

index 11534b9..ab0afb5 100644
@@ -515,6 +515,7 @@ fec_stop(struct net_device *ndev)
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	const struct platform_device_id *id_entry =
 				platform_get_device_id(fep->pdev);
+	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);

This bit is only available on ENET (imx28 and imx6q). Do we want to do the same thing for FEC (imx25/27/35/51/53)?

No. AFAICT that's not necessary there.

Quoted 37 lines · Lothar Wa?mann

Hi,

Quoted 9 lines · Shawn Guo

Quoted 2 lines · Lothar Wa?mann

Additionally to setting the ETHER_EN bit in FEC_ECNTRL the MII/RMII setting in FEC_R_CNTRL needs to be preserved to keep the MII interface

s/MII/RMII? From what I see from imx28 and imx6q RM, the reset state for this setting is MII mode.

Quoted 2 lines

functional.

Quoted 8 lines

Quoted 4 lines

Signed-off-by Lothar Wa?mann <[email protected]>

---
 drivers/net/ethernet/freescale/fec.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

I assume this is fixing a problem you are seeing on imx28 only. Do you see the problem on imx53/51?

No. i.MX53 uses the RMII gasket which is not affected by resetting the controller. And imMX51 does not support RMII at all.

Quoted 14 lines

drivers/net/ethernet/freescale/fec.c fec_stop(struct net_device *ndev) · line 518

Quoted 10 lines

drivers/net/ethernet/freescale/fec.c +1 -0

index 11534b9..ab0afb5 100644
@@ -515,6 +515,7 @@ fec_stop(struct net_device *ndev)
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	const struct platform_device_id *id_entry =
 				platform_get_device_id(fep->pdev);
+	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);

This bit is only available on ENET (imx28 and imx6q). Do we want to do the same thing for FEC (imx25/27/35/51/53)?

Quoted 2 lines

No. AFAICT that's not necessary there.

So you need to check it's actually running on ENET before accessing the bit.

Hi,

Quoted 38 lines · Shawn Guo

Quoted 37 lines · Lothar Waßmann

Hi,

Quoted 9 lines · Shawn Guo

Quoted 2 lines · Lothar Waßmann

Additionally to setting the ETHER_EN bit in FEC_ECNTRL the MII/RMII setting in FEC_R_CNTRL needs to be preserved to keep the MII interface

s/MII/RMII? From what I see from imx28 and imx6q RM, the reset state for this setting is MII mode.

Quoted 2 lines

functional.

Quoted 8 lines

Quoted 4 lines

Signed-off-by Lothar Waßmann <[email protected]>

---
 drivers/net/ethernet/freescale/fec.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

I assume this is fixing a problem you are seeing on imx28 only. Do you see the problem on imx53/51?

No. i.MX53 uses the RMII gasket which is not affected by resetting the controller. And imMX51 does not support RMII at all.

Quoted 14 lines

drivers/net/ethernet/freescale/fec.c fec_stop(struct net_device *ndev) · line 518

Quoted 10 lines

drivers/net/ethernet/freescale/fec.c +1 -0

index 11534b9..ab0afb5 100644
@@ -515,6 +515,7 @@ fec_stop(struct net_device *ndev)
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	const struct platform_device_id *id_entry =
 				platform_get_device_id(fep->pdev);
+	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);

This bit is only available on ENET (imx28 and imx6q). Do we want to do the same thing for FEC (imx25/27/35/51/53)?

Quoted 5 lines

Quoted 2 lines

No. AFAICT that's not necessary there.

So you need to check it's actually running on ENET before accessing the bit.

That's done in the place where the register is being written:

|	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) {
|		writel(2, fep->hwp + FEC_ECNTRL);
|		writel(rmii_mode, fep->hwp + FEC_R_CNTRL);
|	}

We could save one register read by doing the check also for the read, but that would further complicate the code.

I'm using a TX28 which has the Ethernet PHY connected in RMII mode. You should see the problem, if you unplug the ethernet cable on a configured link. Without this patch the kernel will not detect when the cable is plugged back in.

When the MAC address is supplied via platform_data it should be OK as it is and should not be modified in case of a dual FEC setup. Also copying the MAC from platform_data to the single 'macaddr' variable will overwrite the MAC for the first interface in case of a dual FEC setup.

Signed-off-by Lothar Waßmann <[email protected]>

---
 drivers/net/ethernet/freescale/fec.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/net/ethernet/freescale/fec.c +0 -0

index e2b5ce6..11534b9 100644

drivers/net/ethernet/freescale/fec.c +0 -0

 #endif
 	}
 

Hi Lothar,

Quoted 11 lines · Lothar Wa?mann

When the MAC address is supplied via platform_data it should be OK as it is and should not be modified in case of a dual FEC setup. Also copying the MAC from platform_data to the single 'macaddr' variable will overwrite the MAC for the first interface in case of a dual FEC setup.

Signed-off-by Lothar Wa?mann <[email protected]>

---
 drivers/net/ethernet/freescale/fec.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/net/ethernet/freescale/fec.c static void __inline__ fec_get_mac(struct net_device *ndev) · line 821

Quoted 10 lines

drivers/net/ethernet/freescale/fec.c +1 -1

index e2b5ce6..11534b9 100644
@@ -818,7 +818,7 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
 			iap = (unsigned char *)FEC_FLASHMAC;
 #else
 		if (pdata)
-			memcpy(iap, pdata->mac, ETH_ALEN);
+			iap = (unsigned char *)&pdata->mac;

Since pdata might point to __initdata struct, you must hold a copy of its content.

Hi,

Quoted 28 lines · Baruch Siach

Hi Lothar,

Quoted 16 lines · Lothar Waßmann

When the MAC address is supplied via platform_data it should be OK as it is and should not be modified in case of a dual FEC setup. Also copying the MAC from platform_data to the single 'macaddr' variable will overwrite the MAC for the first interface in case of a dual FEC setup.

Signed-off-by Lothar Waßmann <[email protected]>

---
 drivers/net/ethernet/freescale/fec.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/net/ethernet/freescale/fec.c +0 -0

index e2b5ce6..11534b9 100644

drivers/net/ethernet/freescale/fec.c static void __inline__ fec_get_mac(struct net_device *ndev) · line 821

Quoted 5 lines

drivers/net/ethernet/freescale/fec.c +1 -1

@@ -818,7 +818,7 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
 			iap = (unsigned char *)FEC_FLASHMAC;
 #else
 		if (pdata)
-			memcpy(iap, pdata->mac, ETH_ALEN);
+			iap = (unsigned char *)&pdata->mac;

Since pdata might point to __initdata struct, you must hold a copy of its content.

No. platform_data must be present during the life time of a driver using it and thus must never be __initdata!

Quoted 31 lines · Lothar Wa?mann

Hi,

Quoted 28 lines · Baruch Siach

Hi Lothar,

Quoted 16 lines · Lothar Wa?mann

When the MAC address is supplied via platform_data it should be OK as it is and should not be modified in case of a dual FEC setup. Also copying the MAC from platform_data to the single 'macaddr' variable will overwrite the MAC for the first interface in case of a dual FEC setup.

Signed-off-by Lothar Wa?mann <[email protected]>

---
 drivers/net/ethernet/freescale/fec.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/net/ethernet/freescale/fec.c +0 -0

index e2b5ce6..11534b9 100644

drivers/net/ethernet/freescale/fec.c static void __inline__ fec_get_mac(struct net_device *ndev) · line 821

Quoted 5 lines

drivers/net/ethernet/freescale/fec.c +1 -1

@@ -818,7 +818,7 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
 			iap = (unsigned char *)FEC_FLASHMAC;
 #else
 		if (pdata)
-			memcpy(iap, pdata->mac, ETH_ALEN);
+			iap = (unsigned char *)&pdata->mac;

Since pdata might point to __initdata struct, you must hold a copy of its content.

As iap will anyway be copied to ndev->dev_addr after that, it may still be fine to take this patch, for obviously for different reason.

Quoted 3 lines

No. platform_data must be present during the life time of a driver using it and thus must never be __initdata!

Then we need to fix a lot of imx/mxc platform codes, mach-mx28evk.c is the one for this case.

When the MAC address is supplied via platform_data it should be OK as it is and should not be modified in case of a dual FEC setup. Also copying the MAC from platform_data to the single 'macaddr' variable will overwrite the MAC for the first interface in case of a dual FEC setup.

Signed-off-by Lothar Waßmann <[email protected]>

---
 drivers/net/ethernet/freescale/fec.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/net/ethernet/freescale/fec.c +1 -1

index e2b5ce6..11534b9 100644
@@ -818,7 +818,7 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
 			iap = (unsigned char *)FEC_FLASHMAC;
 #else
 		if (pdata)
-			memcpy(iap, pdata->mac, ETH_ALEN);
+			iap = (unsigned char *)&pdata->mac;
 #endif
 	}
 

Signature · 1 line

1.5.6.5

Quoted 5 lines · Lothar Wa?mann

When the MAC address is supplied via platform_data it should be OK as it is and should not be modified in case of a dual FEC setup. Also copying the MAC from platform_data to the single 'macaddr' variable will overwrite the MAC for the first interface in case of a dual FEC setup.

Hmm, I do not follow that. If 'macaddr' holds a valid mac address, the code path has no chance to be hit at all. Otherwise, 'macaddr' is just a place holder for copying mac address from pdata, in which case the mac address will be fixed up at the end of the function for dual FEC setup.

Hi,

Quoted 7 lines · Shawn Guo

Quoted 5 lines · Lothar Waßmann

When the MAC address is supplied via platform_data it should be OK as it is and should not be modified in case of a dual FEC setup. Also copying the MAC from platform_data to the single 'macaddr' variable will overwrite the MAC for the first interface in case of a dual FEC setup.

Quoted 6 lines

Hmm, I do not follow that. If 'macaddr' holds a valid mac address, the code path has no chance to be hit at all. Otherwise, 'macaddr' is just a place holder for copying mac address from pdata, in which case the mac address will be fixed up at the end of the function for dual FEC setup.

In case of dual FEC and MAC address provided by platform_data the first mac address will be copied to the 'macaddr' variable with no chance for the second interface to get its mac address assigned from platform_data too.

prevent calling request_irq() with a known invalid IRQ number and preserve the return value of the platform_get_irq() function

Signed-off-by Lothar Waßmann <[email protected]>

The con_id is actually not needed for clk_get().

Signed-off-by Lothar Waßmann <[email protected]>

- remove some bogus whitespace
- remove line wraps from printk messages

Signed-off-by Lothar Waßmann <[email protected]>

The following set of patches provides some cleanup and bugfixes for drivers/net/ethernet/freescale/fec.c and makes the driver buildable as a module.

Lothar Wa??mann (8):
misc cleanups
set con_id in clk_get() call to NULL
prevent dobule restart of interface on FDX/HDX change don't request invalid IRQ
don't munge MAC address from platform data preserve MII/RMII setting in fec_stop()
fix the .remove code
make FEC driver buildable as module

  drivers/net/ethernet/freescale/Kconfig |    2 +-
  drivers/net/ethernet/freescale/fec.c   |   63 ++++++++++++++++++++++----------
  2 files changed, 44 insertions(+), 21 deletions(-)

Quoted 14 lines · Lothar Waßmann

The following set of patches provides some cleanup and bugfixes for drivers/net/ethernet/freescale/fec.c and makes the driver buildable as a module.

Lothar Waßmann (8):
misc cleanups
set con_id in clk_get() call to NULL
prevent dobule restart of interface on FDX/HDX change don't request invalid IRQ
don't munge MAC address from platform data preserve MII/RMII setting in fec_stop()
fix the .remove code
make FEC driver buildable as module

Quoted 3 lines

 drivers/net/ethernet/freescale/Kconfig |    2 +-
 drivers/net/ethernet/freescale/fec.c   |   63 ++++++++++++++++++++++----------
 2 files changed, 44 insertions(+), 21 deletions(-)

What's the status of this patch series?

If I remember correctly, most of them got an Ack or Tested-by from Shawn? But one or two need an update (not sure, though)? And the subject should be updated?