[PATCH 1/4] net: phy: balance disable/enable irq on change

[PATCH 1/4] net: phy: balance disable/enable irq on change

[Jean-Michel Hautbois]

When phy interface changes its status, it calls phy_change() function.
This function calls the interrupt disabling functions for the driver
registered, but if this driver doesn't implement it, there is no IRQ
disabling. After doing the work, we call enable_irq and not the
respective driver function. This fixes it, as it could lead to an
unbalanced IRQ. Error code changed to EOPNOTSUPP.

Signed-off-by: Jean-Michel Hautbois <jhautbois@gmail.com>
---
 drivers/net/phy/phy.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7670aac..5f23e8e 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -89,7 +89,8 @@ static int phy_config_interrupt(struct phy_device *phydev, u32 interrupts)
 	phydev->interrupts = interrupts;
 	if (phydev->drv->config_intr)
 		err = phydev->drv->config_intr(phydev);
-
+	else
+		err = -EOPNOTSUPP;
 	return err;
 }
 
@@ -541,6 +542,10 @@ static int phy_enable_interrupts(struct phy_device *phydev)
 		return err;
 
 	err = phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED);
+	if (err == -EOPNOTSUPP) {
+		err = 0;
+		enable_irq(phydev->irq);
+	}
 
 	return err;
 }
@@ -556,7 +561,10 @@ static int phy_disable_interrupts(struct phy_device *phydev)
 	/* Disable PHY interrupts */
 	err = phy_config_interrupt(phydev, PHY_INTERRUPT_DISABLED);
 
-	if (err)
+	if (err == -EOPNOTSUPP) {
+		err = 0;
+		disable_irq(phydev->irq);
+	} else if (err != 0)
 		goto phy_err;
 
 	/* Clear the interrupt */
-- 
1.7.0.4

[PATCH 2/4] net: phy: printk issues fixed

[Jean-Michel Hautbois]

When calling net link change, pr_info and printk where interleaved.
Changed the way to print messages.

Signed-off-by: Jean-Michel Hautbois <jhautbois@gmail.com>
---
 drivers/net/phy/phy.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 5f23e8e..4ab48d7 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -44,14 +44,14 @@
  */
 void phy_print_status(struct phy_device *phydev)
 {
-	pr_info("PHY: %s - Link is %s", dev_name(&phydev->dev),
-			phydev->link ? "Up" : "Down");
 	if (phydev->link)
-		printk(" - %d/%s", phydev->speed,
-				DUPLEX_FULL == phydev->duplex ?
-				"Full" : "Half");
-
-	printk("\n");
+		pr_info("PHY: %s - Link is up - %d/%s\n",
+				dev_name(&phydev->dev),
+				phydev->speed,
+				phydev->duplex == DUPLEX_FULL ? "Full" : "Half");
+	else
+		pr_info("PHY: %s - Link is down\n",
+				dev_name(&phydev->dev));
 }
 EXPORT_SYMBOL(phy_print_status);
 
-- 
1.7.0.4

[PATCH 3/4] net: phy: White space correction and switch/case braces

[Jean-Michel Hautbois]

Little corrections in the file :
- use linux and not asm for some include files
- Trailing whitespaces removed
- no lines over 80 characters
- switch and case on the same column

Signed-off-by: Jean-Michel Hautbois <jhautbois@gmail.com>
---
 drivers/net/phy/phy.c |  304 ++++++++++++++++++++++++------------------------
 1 files changed, 152 insertions(+), 152 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 4ab48d7..87a64c0 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -33,10 +33,10 @@
 #include <linux/timer.h>
 #include <linux/workqueue.h>
 
-#include <asm/atomic.h>
-#include <asm/io.h>
+#include <linux/atomic.h>
+#include <linux/io.h>
 #include <asm/irq.h>
-#include <asm/uaccess.h>
+#include <linux/uaccess.h>
 
 /**
  * phy_print_status - Convenience function to print out the current phy status
@@ -48,7 +48,8 @@ void phy_print_status(struct phy_device *phydev)
 		pr_info("PHY: %s - Link is up - %d/%s\n",
 				dev_name(&phydev->dev),
 				phydev->speed,
-				phydev->duplex == DUPLEX_FULL ? "Full" : "Half");
+				phydev->duplex == DUPLEX_FULL ?
+				"Full" : "Half");
 	else
 		pr_info("PHY: %s - Link is down\n",
 				dev_name(&phydev->dev));
@@ -325,7 +326,7 @@ int phy_mii_ioctl(struct phy_device *phydev,
 
 	case SIOCSMIIREG:
 		if (mii_data->phy_id == phydev->addr) {
-			switch(mii_data->reg_num) {
+			switch (mii_data->reg_num) {
 			case MII_BMCR:
 				if ((val & (BMCR_RESET|BMCR_ANENABLE)) == 0)
 					phydev->autoneg = AUTONEG_DISABLE;
@@ -352,7 +353,6 @@ int phy_mii_ioctl(struct phy_device *phydev,
 		}
 
 		phy_write(phydev, mii_data->reg_num, val);
-		
 		if (mii_data->reg_num == MII_BMCR &&
 		    val & BMCR_RESET &&
 		    phydev->drv->config_init) {
@@ -471,7 +471,7 @@ static void phy_force_reduction(struct phy_device *phydev)
 	int idx;
 
 	idx = phy_find_setting(phydev->speed, phydev->duplex);
-	
+
 	idx++;
 
 	idx = phy_find_valid(idx, phydev->supported);
@@ -749,16 +749,16 @@ void phy_start(struct phy_device *phydev)
 	mutex_lock(&phydev->lock);
 
 	switch (phydev->state) {
-		case PHY_STARTING:
-			phydev->state = PHY_PENDING;
-			break;
-		case PHY_READY:
-			phydev->state = PHY_UP;
-			break;
-		case PHY_HALTED:
-			phydev->state = PHY_RESUMING;
-		default:
-			break;
+	case PHY_STARTING:
+		phydev->state = PHY_PENDING;
+		break;
+	case PHY_READY:
+		phydev->state = PHY_UP;
+		break;
+	case PHY_HALTED:
+		phydev->state = PHY_RESUMING;
+	default:
+		break;
 	}
 	mutex_unlock(&phydev->lock);
 }
@@ -782,172 +782,157 @@ void phy_state_machine(struct work_struct *work)
 	if (phydev->adjust_state)
 		phydev->adjust_state(phydev->attached_dev);
 
-	switch(phydev->state) {
-		case PHY_DOWN:
-		case PHY_STARTING:
-		case PHY_READY:
-		case PHY_PENDING:
-			break;
-		case PHY_UP:
-			needs_aneg = 1;
+	switch (phydev->state) {
+	case PHY_DOWN:
+	case PHY_STARTING:
+	case PHY_READY:
+	case PHY_PENDING:
+		break;
+	case PHY_UP:
+		needs_aneg = 1;
 
-			phydev->link_timeout = PHY_AN_TIMEOUT;
+		phydev->link_timeout = PHY_AN_TIMEOUT;
+
+		break;
+	case PHY_AN:
+		err = phy_read_status(phydev);
 
+		if (err < 0)
 			break;
-		case PHY_AN:
-			err = phy_read_status(phydev);
 
-			if (err < 0)
-				break;
+		/* If the link is down, give up on
+		 * negotiation for now */
+		if (!phydev->link) {
+			phydev->state = PHY_NOLINK;
+			netif_carrier_off(phydev->attached_dev);
+			phydev->adjust_link(phydev->attached_dev);
+			break;
+		}
 
-			/* If the link is down, give up on
-			 * negotiation for now */
-			if (!phydev->link) {
-				phydev->state = PHY_NOLINK;
-				netif_carrier_off(phydev->attached_dev);
-				phydev->adjust_link(phydev->attached_dev);
-				break;
-			}
+		/* Check if negotiation is done.  Break
+		 * if there's an error */
+		err = phy_aneg_done(phydev);
+		if (err < 0)
+			break;
 
-			/* Check if negotiation is done.  Break
-			 * if there's an error */
-			err = phy_aneg_done(phydev);
-			if (err < 0)
-				break;
+		/* If AN is done, we're running */
+		if (err > 0) {
+			phydev->state = PHY_RUNNING;
+			netif_carrier_on(phydev->attached_dev);
+			phydev->adjust_link(phydev->attached_dev);
 
-			/* If AN is done, we're running */
-			if (err > 0) {
-				phydev->state = PHY_RUNNING;
-				netif_carrier_on(phydev->attached_dev);
-				phydev->adjust_link(phydev->attached_dev);
+		} else if (0 == phydev->link_timeout--) {
+			int idx;
 
-			} else if (0 == phydev->link_timeout--) {
-				int idx;
+			needs_aneg = 1;
+			/* If we have the magic_aneg bit,
+			 * we try again */
+			if (phydev->drv->flags & PHY_HAS_MAGICANEG)
+				break;
 
-				needs_aneg = 1;
-				/* If we have the magic_aneg bit,
-				 * we try again */
-				if (phydev->drv->flags & PHY_HAS_MAGICANEG)
-					break;
+			/* The timer expired, and we still
+			 * don't have a setting, so we try
+			 * forcing it until we find one that
+			 * works, starting from the fastest speed,
+			 * and working our way down */
+			idx = phy_find_valid(0, phydev->supported);
 
-				/* The timer expired, and we still
-				 * don't have a setting, so we try
-				 * forcing it until we find one that
-				 * works, starting from the fastest speed,
-				 * and working our way down */
-				idx = phy_find_valid(0, phydev->supported);
+			phydev->speed = settings[idx].speed;
+			phydev->duplex = settings[idx].duplex;
 
-				phydev->speed = settings[idx].speed;
-				phydev->duplex = settings[idx].duplex;
+			phydev->autoneg = AUTONEG_DISABLE;
 
-				phydev->autoneg = AUTONEG_DISABLE;
+			pr_info("Trying %d/%s\n", phydev->speed,
+					DUPLEX_FULL ==
+					phydev->duplex ?
+					"FULL" : "HALF");
+		}
+		break;
+	case PHY_NOLINK:
+		err = phy_read_status(phydev);
 
-				pr_info("Trying %d/%s\n", phydev->speed,
-						DUPLEX_FULL ==
-						phydev->duplex ?
-						"FULL" : "HALF");
-			}
+		if (err)
 			break;
-		case PHY_NOLINK:
-			err = phy_read_status(phydev);
 
-			if (err)
-				break;
+		if (phydev->link) {
+			phydev->state = PHY_RUNNING;
+			netif_carrier_on(phydev->attached_dev);
+			phydev->adjust_link(phydev->attached_dev);
+		}
+		break;
+	case PHY_FORCING:
+		err = genphy_update_link(phydev);
 
-			if (phydev->link) {
-				phydev->state = PHY_RUNNING;
-				netif_carrier_on(phydev->attached_dev);
-				phydev->adjust_link(phydev->attached_dev);
-			}
+		if (err)
 			break;
-		case PHY_FORCING:
-			err = genphy_update_link(phydev);
-
-			if (err)
-				break;
 
-			if (phydev->link) {
-				phydev->state = PHY_RUNNING;
-				netif_carrier_on(phydev->attached_dev);
-			} else {
-				if (0 == phydev->link_timeout--) {
-					phy_force_reduction(phydev);
-					needs_aneg = 1;
-				}
+		if (phydev->link) {
+			phydev->state = PHY_RUNNING;
+			netif_carrier_on(phydev->attached_dev);
+		} else {
+			if (0 == phydev->link_timeout--) {
+				phy_force_reduction(phydev);
+				needs_aneg = 1;
 			}
+		}
 
-			phydev->adjust_link(phydev->attached_dev);
-			break;
-		case PHY_RUNNING:
-			/* Only register a CHANGE if we are
-			 * polling */
-			if (PHY_POLL == phydev->irq)
-				phydev->state = PHY_CHANGELINK;
+		phydev->adjust_link(phydev->attached_dev);
+		break;
+	case PHY_RUNNING:
+		/* Only register a CHANGE if we are
+		 * polling */
+		if (PHY_POLL == phydev->irq)
+			phydev->state = PHY_CHANGELINK;
+		break;
+	case PHY_CHANGELINK:
+		err = phy_read_status(phydev);
+
+		if (err)
 			break;
-		case PHY_CHANGELINK:
-			err = phy_read_status(phydev);
 
-			if (err)
-				break;
+		if (phydev->link) {
+			phydev->state = PHY_RUNNING;
+			netif_carrier_on(phydev->attached_dev);
+		} else {
+			phydev->state = PHY_NOLINK;
+			netif_carrier_off(phydev->attached_dev);
+		}
 
-			if (phydev->link) {
-				phydev->state = PHY_RUNNING;
-				netif_carrier_on(phydev->attached_dev);
-			} else {
-				phydev->state = PHY_NOLINK;
-				netif_carrier_off(phydev->attached_dev);
-			}
+		phydev->adjust_link(phydev->attached_dev);
 
+		if (PHY_POLL != phydev->irq)
+			err = phy_config_interrupt(phydev,
+					PHY_INTERRUPT_ENABLED);
+		break;
+	case PHY_HALTED:
+		if (phydev->link) {
+			phydev->link = 0;
+			netif_carrier_off(phydev->attached_dev);
 			phydev->adjust_link(phydev->attached_dev);
+		}
+		break;
+	case PHY_RESUMING:
 
-			if (PHY_POLL != phydev->irq)
-				err = phy_config_interrupt(phydev,
-						PHY_INTERRUPT_ENABLED);
-			break;
-		case PHY_HALTED:
-			if (phydev->link) {
-				phydev->link = 0;
-				netif_carrier_off(phydev->attached_dev);
-				phydev->adjust_link(phydev->attached_dev);
-			}
-			break;
-		case PHY_RESUMING:
+		err = phy_clear_interrupt(phydev);
 
-			err = phy_clear_interrupt(phydev);
+		if (err)
+			break;
 
-			if (err)
-				break;
+		err = phy_config_interrupt(phydev,
+				PHY_INTERRUPT_ENABLED);
 
-			err = phy_config_interrupt(phydev,
-					PHY_INTERRUPT_ENABLED);
+		if (err)
+			break;
 
-			if (err)
+		if (AUTONEG_ENABLE == phydev->autoneg) {
+			err = phy_aneg_done(phydev);
+			if (err < 0)
 				break;
 
-			if (AUTONEG_ENABLE == phydev->autoneg) {
-				err = phy_aneg_done(phydev);
-				if (err < 0)
-					break;
-
-				/* err > 0 if AN is done.
-				 * Otherwise, it's 0, and we're
-				 * still waiting for AN */
-				if (err > 0) {
-					err = phy_read_status(phydev);
-					if (err)
-						break;
-
-					if (phydev->link) {
-						phydev->state = PHY_RUNNING;
-						netif_carrier_on(phydev->attached_dev);
-					} else
-						phydev->state = PHY_NOLINK;
-					phydev->adjust_link(phydev->attached_dev);
-				} else {
-					phydev->state = PHY_AN;
-					phydev->link_timeout = PHY_AN_TIMEOUT;
-				}
-			} else {
+			/* err > 0 if AN is done.
+			 * Otherwise, it's 0, and we're
+			 * still waiting for AN */
+			if (err > 0) {
 				err = phy_read_status(phydev);
 				if (err)
 					break;
@@ -958,8 +943,23 @@ void phy_state_machine(struct work_struct *work)
 				} else
 					phydev->state = PHY_NOLINK;
 				phydev->adjust_link(phydev->attached_dev);
+			} else {
+				phydev->state = PHY_AN;
+				phydev->link_timeout = PHY_AN_TIMEOUT;
 			}
-			break;
+		} else {
+			err = phy_read_status(phydev);
+			if (err)
+				break;
+
+			if (phydev->link) {
+				phydev->state = PHY_RUNNING;
+				netif_carrier_on(phydev->attached_dev);
+			} else
+				phydev->state = PHY_NOLINK;
+			phydev->adjust_link(phydev->attached_dev);
+		}
+		break;
 	}
 
 	mutex_unlock(&phydev->lock);
-- 
1.7.0.4

[PATCH 4/4] net: phy: EXPORT_SYMBOL not following the function

[Jean-Michel Hautbois]

The symbol phy_stop was not following the related function.

Signed-off-by: Jean-Michel Hautbois <jhautbois@gmail.com>
---
 drivers/net/phy/phy.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 87a64c0..ed503cd 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -732,6 +732,7 @@ out_unlock:
 	 * will not reenable interrupts.
 	 */
 }
+EXPORT_SYMBOL(phy_stop);
 
 
 /**
@@ -762,7 +763,6 @@ void phy_start(struct phy_device *phydev)
 	}
 	mutex_unlock(&phydev->lock);
 }
-EXPORT_SYMBOL(phy_stop);
 EXPORT_SYMBOL(phy_start);
 
 /**
-- 
1.7.0.4

Re: [PATCH 1/4] net: phy: balance disable/enable irq on change

[David Miller]

From: Jean-Michel Hautbois <jhautbois@gmail.com>
Date: Sun, 19 Dec 2010 11:58:48 +0100

> When phy interface changes its status, it calls phy_change() function.
> This function calls the interrupt disabling functions for the driver
> registered, but if this driver doesn't implement it, there is no IRQ
> disabling. After doing the work, we call enable_irq and not the
> respective driver function. This fixes it, as it could lead to an
> unbalanced IRQ. Error code changed to EOPNOTSUPP.
> 
> Signed-off-by: Jean-Michel Hautbois <jhautbois@gmail.com>

This is completely bogus.

First of all, there are 5 call sites for phy_change_interrupt() but
you've only implemented the new semantics for two of those.

Therefore, if we even wanted this, we should implement the behavior in
phy_change_interrupt() itself instead of duplicating the logic at
each and every call site.

But we don't want this.

It's not appropriate at all.  If a device lacks a way to turn
interrupt off and on, using disable_irq() and enable_irq() is not
necessarily correct.

If the interrupt line is shared, for example, this will break
everything.

Re: [PATCH 1/4] net: phy: balance disable/enable irq on change

[Jean-Michel Hautbois]

2010/12/23 David Miller <davem@davemloft.net>:
> From: Jean-Michel Hautbois <jhautbois@gmail.com>
> Date: Sun, 19 Dec 2010 11:58:48 +0100
>
>> When phy interface changes its status, it calls phy_change() function.
>> This function calls the interrupt disabling functions for the driver
>> registered, but if this driver doesn't implement it, there is no IRQ
>> disabling. After doing the work, we call enable_irq and not the
>> respective driver function. This fixes it, as it could lead to an
>> unbalanced IRQ. Error code changed to EOPNOTSUPP.
>>
>> Signed-off-by: Jean-Michel Hautbois <jhautbois@gmail.com>
>
> This is completely bogus.
>
> First of all, there are 5 call sites for phy_change_interrupt() but
> you've only implemented the new semantics for two of those.
>
> Therefore, if we even wanted this, we should implement the behavior in
> phy_change_interrupt() itself instead of duplicating the logic at
> each and every call site.
>
> But we don't want this.

OK, I understand that point.

> It's not appropriate at all.  If a device lacks a way to turn
> interrupt off and on, using disable_irq() and enable_irq() is not
> necessarily correct.
>
> If the interrupt line is shared, for example, this will break
> everything.
>

OK, well, maybe is there at least one thing we could do : in
phy_change, instead of calling phy_disable_interrupts(), balanced by
enable_irq, we probably should use phy_enable_interrupts().