[PATCH] net: phy: Move interrupt check from phy_check to phy_interrupt

If multiple phys share the same interrupt (e.g. a multi-phy chip),
the first device registered is the only one checked as phy_interrupt
will always return IRQ_HANDLED if the first phydev is not halted.
Move the interrupt check into phy_interrupt and, if it was not this
phydev, return IRQ_NONE to allow other devices on this irq a chance
to check if it was their interrupt.

Signed-off-by: Brad Mouring <redacted>
---
 drivers/net/phy/phy.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e3e29c2b028b..ff1aa815568f 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c

@@ -632,6 +632,12 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 	if (PHY_HALTED == phydev->state)
 		return IRQ_NONE;		/* It can't be ours.  */
 
+	if (phy_interrupt_is_valid(phydev)) {
+		if (phydev->drv->did_interrupt &&
+			!phydev->drv->did_interrupt(phydev))
+			return IRQ_NONE;
+	}
+
 	phy_change(phydev);
 
 	return IRQ_HANDLED;

@@ -725,16 +731,6 @@ EXPORT_SYMBOL(phy_stop_interrupts);
  */
 void phy_change(struct phy_device *phydev)
 {
-	if (phy_interrupt_is_valid(phydev)) {
-		if (phydev->drv->did_interrupt &&
-		    !phydev->drv->did_interrupt(phydev))
-			return;
-
-		if (phydev->state == PHY_HALTED)
-			if (phy_disable_interrupts(phydev))
-				goto phy_err;
-	}
-
 	mutex_lock(&phydev->lock);
 	if ((PHY_RUNNING == phydev->state) || (PHY_NOLINK == phydev->state))
 		phydev->state = PHY_CHANGELINK;

-- 
2.16.2

On Wed, Mar 07, 2018 at 04:50:42PM -0600, Brad Mouring wrote:

Hi Brad

Is this check needed? Can phy_interrupt() be called for a PHY which
does not have an interrupt?

phy_change() can also be called via phy_mac_interrupt(). I wonder if
this change is going to break anything? Did you think about that?

Thanks
	Andrew

On Thu, Mar 08, 2018 at 05:29:05PM +0100, Andrew Lunn wrote:

On Wed, Mar 07, 2018 at 04:50:42PM -0600, Brad Mouring wrote:

quoted

If multiple phys share the same interrupt (e.g. a multi-phy chip),
the first device registered is the only one checked as phy_interrupt
will always return IRQ_HANDLED if the first phydev is not halted.
Move the interrupt check into phy_interrupt and, if it was not this
phydev, return IRQ_NONE to allow other devices on this irq a chance
to check if it was their interrupt.

Signed-off-by: Brad Mouring <redacted>
---
 drivers/net/phy/phy.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e3e29c2b028b..ff1aa815568f 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c

@@ -632,6 +632,12 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 	if (PHY_HALTED == phydev->state)
 		return IRQ_NONE;		/* It can't be ours.  */
 
+	if (phy_interrupt_is_valid(phydev)) {

Hi Brad

Is this check needed? Can phy_interrupt() be called for a PHY which
does not have an interrupt?

Ah, fair point (and I guess a result of copy-pasting without
thinking), to address this and the next point...

quoted

+		if (phydev->drv->did_interrupt &&
+			!phydev->drv->did_interrupt(phydev))
+			return IRQ_NONE;
+	}
+
 	phy_change(phydev);
 
 	return IRQ_HANDLED;

@@ -725,16 +731,6 @@ EXPORT_SYMBOL(phy_stop_interrupts);
  */
 void phy_change(struct phy_device *phydev)
 {
-	if (phy_interrupt_is_valid(phydev)) {
-		if (phydev->drv->did_interrupt &&
-		    !phydev->drv->did_interrupt(phydev))
-			return;
-
-		if (phydev->state == PHY_HALTED)
-			if (phy_disable_interrupts(phydev))
-				goto phy_err;
-	}
-

phy_change() can also be called via phy_mac_interrupt(). I wonder if
this change is going to break anything? Did you think about that?

Thanks for pointing this out. I'll post a v2 that (hopefully) address
both of these valid points.

Thanks,
Brad

Hello!

On 03/08/2018 01:50 AM, Brad Mouring wrote:

If multiple phys share the same interrupt (e.g. a multi-phy chip),
the first device registered is the only one checked as phy_interrupt
will always return IRQ_HANDLED if the first phydev is not halted.
Move the interrupt check into phy_interrupt and, if it was not this
phydev, return IRQ_NONE to allow other devices on this irq a chance
to check if it was their interrupt.

   Hm, looking at kernel/irq/handle.c, all registered IRQ handlers are always
called regardless of their results. Care to explain?

  Always true in this context, no?

+		if (phydev->drv->did_interrupt &&
+			!phydev->drv->did_interrupt(phydev))

   I don't think we can do this in the interrupt context as this function *will*
read from MDIO... I think that was the reason why IRQ handling is done in the
thread context... 

[...]

MBR, Sergei

On 03/08/2018 10:41 PM, Sergei Shtylyov wrote:

quoted

If multiple phys share the same interrupt (e.g. a multi-phy chip),
the first device registered is the only one checked as phy_interrupt
will always return IRQ_HANDLED if the first phydev is not halted.
Move the interrupt check into phy_interrupt and, if it was not this
phydev, return IRQ_NONE to allow other devices on this irq a chance
to check if it was their interrupt.

   Hm, looking at kernel/irq/handle.c, all registered IRQ handlers are always
called regardless of their results. Care to explain?

quoted

Signed-off-by: Brad Mouring <redacted>
---
 drivers/net/phy/phy.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e3e29c2b028b..ff1aa815568f 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c

@@ -632,6 +632,12 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 	if (PHY_HALTED == phydev->state)
 		return IRQ_NONE;		/* It can't be ours.  */
 
+	if (phy_interrupt_is_valid(phydev)) {

  Always true in this context, no?

quoted

+		if (phydev->drv->did_interrupt &&
+			!phydev->drv->did_interrupt(phydev))

   I don't think we can do this in the interrupt context as this function *will*
read from MDIO... I think that was the reason why IRQ handling is done in the
thread context...

   Ah, we're already in a thread context here! Forgot about it...
 

[...]

MBR, Sergei

Thanks for the feedback, Sergei.

On Thu, Mar 08, 2018 at 10:41:04PM +0300, Sergei Shtylyov wrote:

Hello!

On 03/08/2018 01:50 AM, Brad Mouring wrote:

quoted

If multiple phys share the same interrupt (e.g. a multi-phy chip),
the first device registered is the only one checked as phy_interrupt
will always return IRQ_HANDLED if the first phydev is not halted.
Move the interrupt check into phy_interrupt and, if it was not this
phydev, return IRQ_NONE to allow other devices on this irq a chance
to check if it was their interrupt.

   Hm, looking at kernel/irq/handle.c, all registered IRQ handlers are always
called regardless of their results. Care to explain?

In the phy interrupt handler case, the phy_interrupt function is
registered as the threaded secondary, and irq_default_primary_handler
is being used as the primary (which will turn around and wake the
threaded handler). It seems that we wake the thread_fns in order, and
the first to report back HANDLED stops us from waking the next.

quoted

Signed-off-by: Brad Mouring <redacted>
---
 drivers/net/phy/phy.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e3e29c2b028b..ff1aa815568f 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c

@@ -632,6 +632,12 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 	if (PHY_HALTED == phydev->state)
 		return IRQ_NONE;		/* It can't be ours.  */
 
+	if (phy_interrupt_is_valid(phydev)) {

  Always true in this context, no?

Yes, already noted.

quoted

+		if (phydev->drv->did_interrupt &&
+			!phydev->drv->did_interrupt(phydev))

   I don't think we can do this in the interrupt context as this function *will*
read from MDIO... I think that was the reason why IRQ handling is done in the
thread context... 

phy_interrupt is the thread_fn here. We're not in interrupt context.

quoted

+			return IRQ_NONE;
+	}
+
 	phy_change(phydev);
 
 	return IRQ_HANDLED;

[...]

MBR, Sergei

Thanks,
Brad

On 03/08/2018 01:50 AM, Brad Mouring wrote:

   Forgot to mention: DaveM prefers that continuation lines start under the 1st
column after ( on the broken up line. The way you did it this line blends with
the next one (they shouldn't).

[...]

MBR, Sergei

In 664fcf123a30e (net: phy: Threaded interrupts allow some simplification)
the phy_interrupt system was changed to use a traditional threaded
interrupt scheme instead of a workqueue approach.

With this change, the phy status check moved into phy_change, which
did not report back to the caller whether or not the interrupt was
handled. This means that, in the case of a shared phy interrupt,
only the first phydev's interrupt registers are checked (since
phy_interrupt() would always return IRQ_HANDLED). This leads to
interrupt storms when it is a secondary device that's actually the
interrupt source.

Signed-off-by: Brad Mouring <redacted>
---
 drivers/net/phy/phy.c | 13 +++++++------
 include/linux/phy.h   |  1 -
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e3e29c2b028b..ebce9f2b2742 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c

@@ -617,6 +617,8 @@ static void phy_error(struct phy_device *phydev)
 	phy_trigger_machine(phydev, false);
 }
 
+static irqreturn_t phy_change(struct phy_device *phydev);
+
 /**
  * phy_interrupt - PHY interrupt handler
  * @irq: interrupt line

@@ -632,9 +634,7 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 	if (PHY_HALTED == phydev->state)
 		return IRQ_NONE;		/* It can't be ours.  */
 
-	phy_change(phydev);
-
-	return IRQ_HANDLED;
+	return phy_change(phydev);
 }
 
 /**

@@ -723,12 +723,12 @@ EXPORT_SYMBOL(phy_stop_interrupts);
  * phy_change - Called by the phy_interrupt to handle PHY changes
  * @phydev: phy_device struct that interrupted
  */
-void phy_change(struct phy_device *phydev)
+static irqreturn_t phy_change(struct phy_device *phydev)
 {
 	if (phy_interrupt_is_valid(phydev)) {
 		if (phydev->drv->did_interrupt &&
 		    !phydev->drv->did_interrupt(phydev))
-			return;
+			return IRQ_NONE;
 
 		if (phydev->state == PHY_HALTED)
 			if (phy_disable_interrupts(phydev))

@@ -745,10 +745,11 @@ void phy_change(struct phy_device *phydev)
 
 	if (phy_interrupt_is_valid(phydev) && phy_clear_interrupt(phydev))
 		goto phy_err;
-	return;
+	return IRQ_HANDLED;
 
 phy_err:
 	phy_error(phydev);
+	return IRQ_NONE;
 }
 
 /**

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5a0c3e53e7c2..2e55838655b9 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -1011,7 +1011,6 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner);
 int phy_drivers_register(struct phy_driver *new_driver, int n,
 			 struct module *owner);
 void phy_state_machine(struct work_struct *work);
-void phy_change(struct phy_device *phydev);
 void phy_change_work(struct work_struct *work);
 void phy_mac_interrupt(struct phy_device *phydev);
 void phy_start_machine(struct phy_device *phydev);

-- 
2.16.2

Hi Brad

Rather than add a forward declaration, please move phy_change().

       Thanks
              Andrew

In 664fcf123a30e (net: phy: Threaded interrupts allow some simplification)
the phy_interrupt system was changed to use a traditional threaded
interrupt scheme instead of a workqueue approach.

With this change, the phy status check moved into phy_change, which
did not report back to the caller whether or not the interrupt was
handled. This means that, in the case of a shared phy interrupt,
only the first phydev's interrupt registers are checked (since
phy_interrupt() would always return IRQ_HANDLED). This leads to
interrupt storms when it is a secondary device that's actually the
interrupt source.

Signed-off-by: Brad Mouring <redacted>
---
 drivers/net/phy/phy.c | 145 +++++++++++++++++++++++++-------------------------
 include/linux/phy.h   |   1 -
 2 files changed, 72 insertions(+), 74 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e3e29c2b028b..dfff2cff7da9 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c

@@ -617,6 +617,77 @@ static void phy_error(struct phy_device *phydev)
 	phy_trigger_machine(phydev, false);
 }
 
+/**
+ * phy_disable_interrupts - Disable the PHY interrupts from the PHY side
+ * @phydev: target phy_device struct
+ */
+static int phy_disable_interrupts(struct phy_device *phydev)
+{
+	int err;
+
+	/* Disable PHY interrupts */
+	err = phy_config_interrupt(phydev, PHY_INTERRUPT_DISABLED);
+	if (err)
+		goto phy_err;
+
+	/* Clear the interrupt */
+	err = phy_clear_interrupt(phydev);
+	if (err)
+		goto phy_err;
+
+	return 0;
+
+phy_err:
+	phy_error(phydev);
+
+	return err;
+}
+
+/**
+ * phy_change - Called by the phy_interrupt to handle PHY changes
+ * @phydev: phy_device struct that interrupted
+ */
+static irqreturn_t phy_change(struct phy_device *phydev)
+{
+	if (phy_interrupt_is_valid(phydev)) {
+		if (phydev->drv->did_interrupt &&
+		    !phydev->drv->did_interrupt(phydev))
+			return IRQ_NONE;
+
+		if (phydev->state == PHY_HALTED)
+			if (phy_disable_interrupts(phydev))
+				goto phy_err;
+	}
+
+	mutex_lock(&phydev->lock);
+	if ((PHY_RUNNING == phydev->state) || (PHY_NOLINK == phydev->state))
+		phydev->state = PHY_CHANGELINK;
+	mutex_unlock(&phydev->lock);
+
+	/* reschedule state queue work to run as soon as possible */
+	phy_trigger_machine(phydev, true);
+
+	if (phy_interrupt_is_valid(phydev) && phy_clear_interrupt(phydev))
+		goto phy_err;
+	return IRQ_HANDLED;
+
+phy_err:
+	phy_error(phydev);
+	return IRQ_NONE;
+}
+
+/**
+ * phy_change_work - Scheduled by the phy_mac_interrupt to handle PHY changes
+ * @work: work_struct that describes the work to be done
+ */
+void phy_change_work(struct work_struct *work)
+{
+	struct phy_device *phydev =
+		container_of(work, struct phy_device, phy_queue);
+
+	phy_change(phydev);
+}
+
 /**
  * phy_interrupt - PHY interrupt handler
  * @irq: interrupt line

@@ -632,9 +703,7 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 	if (PHY_HALTED == phydev->state)
 		return IRQ_NONE;		/* It can't be ours.  */
 
-	phy_change(phydev);
-
-	return IRQ_HANDLED;
+	return phy_change(phydev);
 }
 
 /**

@@ -651,32 +720,6 @@ static int phy_enable_interrupts(struct phy_device *phydev)
 	return phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED);
 }
 
-/**
- * phy_disable_interrupts - Disable the PHY interrupts from the PHY side
- * @phydev: target phy_device struct
- */
-static int phy_disable_interrupts(struct phy_device *phydev)
-{
-	int err;
-
-	/* Disable PHY interrupts */
-	err = phy_config_interrupt(phydev, PHY_INTERRUPT_DISABLED);
-	if (err)
-		goto phy_err;
-
-	/* Clear the interrupt */
-	err = phy_clear_interrupt(phydev);
-	if (err)
-		goto phy_err;
-
-	return 0;
-
-phy_err:
-	phy_error(phydev);
-
-	return err;
-}
-
 /**
  * phy_start_interrupts - request and enable interrupts for a PHY device
  * @phydev: target phy_device struct

@@ -719,50 +762,6 @@ int phy_stop_interrupts(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_stop_interrupts);
 
-/**
- * phy_change - Called by the phy_interrupt to handle PHY changes
- * @phydev: phy_device struct that interrupted
- */
-void phy_change(struct phy_device *phydev)
-{
-	if (phy_interrupt_is_valid(phydev)) {
-		if (phydev->drv->did_interrupt &&
-		    !phydev->drv->did_interrupt(phydev))
-			return;
-
-		if (phydev->state == PHY_HALTED)
-			if (phy_disable_interrupts(phydev))
-				goto phy_err;
-	}
-
-	mutex_lock(&phydev->lock);
-	if ((PHY_RUNNING == phydev->state) || (PHY_NOLINK == phydev->state))
-		phydev->state = PHY_CHANGELINK;
-	mutex_unlock(&phydev->lock);
-
-	/* reschedule state queue work to run as soon as possible */
-	phy_trigger_machine(phydev, true);
-
-	if (phy_interrupt_is_valid(phydev) && phy_clear_interrupt(phydev))
-		goto phy_err;
-	return;
-
-phy_err:
-	phy_error(phydev);
-}
-
-/**
- * phy_change_work - Scheduled by the phy_mac_interrupt to handle PHY changes
- * @work: work_struct that describes the work to be done
- */
-void phy_change_work(struct work_struct *work)
-{
-	struct phy_device *phydev =
-		container_of(work, struct phy_device, phy_queue);
-
-	phy_change(phydev);
-}
-
 /**
  * phy_stop - Bring down the PHY link, and stop checking the status
  * @phydev: target phy_device struct

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5a0c3e53e7c2..2e55838655b9 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -1011,7 +1011,6 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner);
 int phy_drivers_register(struct phy_driver *new_driver, int n,
 			 struct module *owner);
 void phy_state_machine(struct work_struct *work);
-void phy_change(struct phy_device *phydev);
 void phy_change_work(struct work_struct *work);
 void phy_mac_interrupt(struct phy_device *phydev);
 void phy_start_machine(struct phy_device *phydev);

-- 
2.16.2

From: Brad Mouring <redacted>
Date: Thu, 8 Mar 2018 16:23:03 -0600

In 664fcf123a30e (net: phy: Threaded interrupts allow some simplification)
the phy_interrupt system was changed to use a traditional threaded
interrupt scheme instead of a workqueue approach.

With this change, the phy status check moved into phy_change, which
did not report back to the caller whether or not the interrupt was
handled. This means that, in the case of a shared phy interrupt,
only the first phydev's interrupt registers are checked (since
phy_interrupt() would always return IRQ_HANDLED). This leads to
interrupt storms when it is a secondary device that's actually the
interrupt source.

Signed-off-by: Brad Mouring <redacted>

Applied and queued up for -stable, thanks.