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

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

[Brad Mouring]

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 <brad.mouring@ni.com>
---
 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

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

[Andrew Lunn]

On Wed, Mar 07, 2018 at 04:50:42PM -0600, 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.
> 
> Signed-off-by: Brad Mouring <brad.mouring@ni.com>
> ---
>  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?

> +		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
	Andrew

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

[Brad Mouring]

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:
> > 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 <brad.mouring@ni.com>
> > ---
> >  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...

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

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

[Sergei Shtylyov]

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?

> Signed-off-by: Brad Mouring <brad.mouring@ni.com>
> ---
>  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?

> +		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... 

> +			return IRQ_NONE;
> +	}
> +
>  	phy_change(phydev);
>  
>  	return IRQ_HANDLED;
[...]

MBR, Sergei

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

[Sergei Shtylyov]

On 03/08/2018 10:41 PM, Sergei Shtylyov 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?
> 
>> Signed-off-by: Brad Mouring <brad.mouring@ni.com>
>> ---
>>  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?
> 
>> +		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

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

[Brad Mouring]

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:
> 
> > 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.

> > Signed-off-by: Brad Mouring <brad.mouring@ni.com>
> > ---
> >  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.

> > +		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.

> > +			return IRQ_NONE;
> > +	}
> > +
> >  	phy_change(phydev);
> >  
> >  	return IRQ_HANDLED;
> [...]
> 
> MBR, Sergei

Thanks,
Brad

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

[Sergei Shtylyov]

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.
> 
> Signed-off-by: Brad Mouring <brad.mouring@ni.com>
> ---
>  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))

   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).

> +			return IRQ_NONE;
> +	}
> +
>  	phy_change(phydev);
>  
>  	return IRQ_HANDLED;
[...]

MBR, Sergei

[PATCH] net: phy: Tell caller result of phy_change()

[Brad Mouring]

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 <brad.mouring@ni.com>
---
 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

Re: [PATCH] net: phy: Tell caller result of phy_change()

[Andrew Lunn]

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

Hi Brad

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

       Thanks
              Andrew

[PATCH net v3] net: phy: Tell caller result of phy_change()

[Brad Mouring]

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 <brad.mouring@ni.com>
---
 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

Re: [PATCH net v3] net: phy: Tell caller result of phy_change()

[David Miller]

From: Brad Mouring <brad.mouring@ni.com>
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 <brad.mouring@ni.com>

Applied and queued up for -stable, thanks.