[PATCH net 0/3] net: phy: Finally fix PHY_IGNORE_INTERRUPTS

[PATCH net 0/3] net: phy: Finally fix PHY_IGNORE_INTERRUPTS

[Florian Fainelli]

Hi all,

This patch series finally fixes how PHY_IGNORE_INTERRUPTS are treated by
avoiding to poll the PHY *and* getting notified from link state changes by the
Ethernet MAC interrupt service routine.

Tested with bcmgenet since this is the HW that I have access to.

Targetting the "net" tree since these are bugfixes, but I would like Woojun and
Andrew to take a look and test that on their respective HW setups as well.

Thank you!

Florian Fainelli (3):
  net: phy: Avoid polling PHY with PHY_IGNORE_INTERRUPTS
  net: phy: Fix phy_mac_interrupt()
  net: bcmgenet: Properly configure PHY to ignore interrupt

 drivers/net/ethernet/broadcom/genet/bcmmii.c |  2 +-
 drivers/net/phy/phy.c                        | 46 +++++++++++++++++-----------
 2 files changed, 29 insertions(+), 19 deletions(-)

-- 
2.1.0

[PATCH net 1/3] net: phy: Avoid polling PHY with PHY_IGNORE_INTERRUPTS

[Florian Fainelli]

Commit 2c7b49212a86 ("phy: fix the use of PHY_IGNORE_INTERRUPT") changed
a hunk in phy_state_machine() in the PHY_RUNNING case which was not
needed. The change essentially makes the PHY library treat PHY devices
with PHY_IGNORE_INTERRUPT to keep polling for the PHY device, even
though the intent is not to do it.

Fix this by reverting that specific hunk, which makes the PHY state
machine wait for state changes, and stay in the PHY_RUNNING state for as
long as needed.

Fixes: 2c7b49212a86 ("phy: fix the use of PHY_IGNORE_INTERRUPT")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 8763bb20988a..42b4c1eb7a90 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -905,10 +905,10 @@ void phy_state_machine(struct work_struct *work)
 		phydev->adjust_link(phydev->attached_dev);
 		break;
 	case PHY_RUNNING:
-		/* Only register a CHANGE if we are polling or ignoring
-		 * interrupts and link changed since latest checking.
+		/* Only register a CHANGE if we are polling and link changed
+		 * since latest checking.
 		 */
-		if (!phy_interrupt_is_valid(phydev)) {
+		if (phydev->irq == PHY_POLL) {
 			old_link = phydev->link;
 			err = phy_read_status(phydev);
 			if (err)
@@ -1000,8 +1000,13 @@ void phy_state_machine(struct work_struct *work)
 		   phy_state_to_str(old_state),
 		   phy_state_to_str(phydev->state));
 
-	queue_delayed_work(system_power_efficient_wq, &phydev->state_queue,
-			   PHY_STATE_TIME * HZ);
+	/* Only re-schedule a PHY state machine change if we are polling the
+	 * PHY, if PHY_IGNORE_INTERRUPT is set, then we will be moving
+	 * between states from phy_mac_interrupt()
+	 */
+	if (phydev->irq == PHY_POLL)
+		queue_delayed_work(system_power_efficient_wq, &phydev->state_queue,
+				   PHY_STATE_TIME * HZ);
 }
 
 void phy_mac_interrupt(struct phy_device *phydev, int new_link)
-- 
2.1.0

[PATCH net 2/3] net: phy: Fix phy_mac_interrupt()

[Florian Fainelli]

Commit 5ea94e7686a3 ("phy: add phy_mac_interrupt()") to use with
PHY_IGNORE_INTERRUPT added a cancel_work_sync() into phy_mac_interrupt()
which is allowed to sleep, whereas phy_mac_interrupt() is expected to be
callable from interrupt context.

Now that we have fixed how the PHY state machine treats
PHY_IGNORE_INTERRUPT with respect to state changes, we can just set the
new link state, and queue the PHY state machine for execution so it is
going to read the new link state.

For that to work properly, we need to update phy_change() not to try to
invoke any interrupt callbacks if we have configured the PHY device for
PHY_IGNORE_INTERRUPT, because that PHY device and its driver are not
required to implement those.

Fixes: 5ea94e7686a3 ("phy: add phy_mac_interrupt() to use with PHY_IGNORE_INTERRUPT")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 42b4c1eb7a90..5590b9c182c9 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -692,25 +692,29 @@ void phy_change(struct work_struct *work)
 	struct phy_device *phydev =
 		container_of(work, struct phy_device, phy_queue);
 
-	if (phydev->drv->did_interrupt &&
-	    !phydev->drv->did_interrupt(phydev))
-		goto ignore;
+	if (phy_interrupt_is_valid(phydev)) {
+		if (phydev->drv->did_interrupt &&
+		    !phydev->drv->did_interrupt(phydev))
+			goto ignore;
 
-	if (phy_disable_interrupts(phydev))
-		goto phy_err;
+		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);
 
-	atomic_dec(&phydev->irq_disable);
-	enable_irq(phydev->irq);
+	if (phy_interrupt_is_valid(phydev)) {
+		atomic_dec(&phydev->irq_disable);
+		enable_irq(phydev->irq);
 
-	/* Reenable interrupts */
-	if (PHY_HALTED != phydev->state &&
-	    phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED))
-		goto irq_enable_err;
+		/* Reenable interrupts */
+		if (PHY_HALTED != phydev->state &&
+		    phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED))
+			goto irq_enable_err;
+	}
 
 	/* reschedule state queue work to run as soon as possible */
 	cancel_delayed_work_sync(&phydev->state_queue);
@@ -1011,9 +1015,10 @@ void phy_state_machine(struct work_struct *work)
 
 void phy_mac_interrupt(struct phy_device *phydev, int new_link)
 {
-	cancel_work_sync(&phydev->phy_queue);
 	phydev->link = new_link;
-	schedule_work(&phydev->phy_queue);
+
+	/* Trigger a state machine change */
+	queue_work(system_power_efficient_wq, &phydev->phy_queue);
 }
 EXPORT_SYMBOL(phy_mac_interrupt);
 
-- 
2.1.0

[PATCH net 3/3] net: bcmgenet: Properly configure PHY to ignore interrupt

[Florian Fainelli]

By the time we execute bcmgenet_mii_probe(), the MDIO bus structure has
long been allocated and registered. Overirring the PHY interrupt using
the MDIO bus structure has no chance to work anymore, because
of_mdiobus_register() has call phy_device_create() for use, which copied
the MDIO bus address's irq for the PHY into the PHY device "irq" member.

Since we do have a proper reference to a PHY device in
bcmgenet_mii_probe(), just assign the desired IRQ value here.

Fixes: aa09677cba42 ("net: bcmgenet: add MDIO routines")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmmii.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 0d775964b060..457c3bc8cfff 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -401,7 +401,7 @@ int bcmgenet_mii_probe(struct net_device *dev)
 	 * Ethernet MAC ISRs
 	 */
 	if (priv->internal_phy)
-		priv->mii_bus->irq[phydev->mdio.addr] = PHY_IGNORE_INTERRUPT;
+		priv->phydev->irq = PHY_IGNORE_INTERRUPT;
 
 	return 0;
 }
-- 
2.1.0

Re: [PATCH net 0/3] net: phy: Finally fix PHY_IGNORE_INTERRUPTS

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Mon, 18 Jan 2016 19:33:05 -0800

> Targetting the "net" tree since these are bugfixes, but I would like
> Woojun and Andrew to take a look and test that on their respective
> HW setups as well.

Ok I'll wait for Woojun and Andrew to give feedback.

RE: [PATCH net 0/3] net: phy: Finally fix PHY_IGNORE_INTERRUPTS

[Woojung.Huh]

> > Targetting the "net" tree since these are bugfixes, but I would like
> > Woojun and Andrew to take a look and test that on their respective
> > HW setups as well.
> 
> Ok I'll wait for Woojun and Andrew to give feedback.

This patch fixes periodic phy read_status access when phy is configured as PHY_IGNORE_INTERRUPTS.
Tested and confirmed with LAN78xx USB-to-Ethernet driver except following checkpatch.pl warnings.

WARNING: line over 80 characters
#54: FILE: drivers/net/phy/phy.c:1008:
+		queue_delayed_work(system_power_efficient_wq, &phydev->state_queue,

WARNING: Comparisons should place the constant on the right side of the test
#68: FILE: drivers/net/phy/phy.c:714:
+		if (PHY_HALTED != phydev->state &&

Re: [PATCH net 0/3] net: phy: Finally fix PHY_IGNORE_INTERRUPTS

[Florian Fainelli]

Le 20/01/2016 13:20, Woojung.Huh@microchip.com a écrit :
>>> Targetting the "net" tree since these are bugfixes, but I would like
>>> Woojun and Andrew to take a look and test that on their respective
>>> HW setups as well.
>>
>> Ok I'll wait for Woojun and Andrew to give feedback.
> 
> This patch fixes periodic phy read_status access when phy is configured as PHY_IGNORE_INTERRUPTS.

Great, thanks for the feedback!

> Tested and confirmed with LAN78xx USB-to-Ethernet driver except following checkpatch.pl warnings.
> 
> WARNING: line over 80 characters
> #54: FILE: drivers/net/phy/phy.c:1008:
> +		queue_delayed_work(system_power_efficient_wq, &phydev->state_queue,

This one is definitively added by the patch

> 
> WARNING: Comparisons should place the constant on the right side of the test
> #68: FILE: drivers/net/phy/phy.c:714:
> +		if (PHY_HALTED != phydev->state &&
> 

This one is an existing warning.

David, let me know if you consider the over 80 columns issue worth a
resubmission or not, either way is fine with me.

Thanks!
-- 
Florian

RE: [PATCH net 0/3] net: phy: Finally fix PHY_IGNORE_INTERRUPTS

[Woojung.Huh]

Florian & David,

I'm experiencing misbehavior after restart system.
Can you wait applying the patch?

Sorry about it.
Woojung

> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Wednesday, January 20, 2016 4:30 PM
> To: Woojung Huh - C21699; davem@davemloft.net
> Cc: netdev@vger.kernel.org; andrew@lunn.ch;
> sergei.shtylyov@cogentembedded.com
> Subject: Re: [PATCH net 0/3] net: phy: Finally fix PHY_IGNORE_INTERRUPTS
> 
> Le 20/01/2016 13:20, Woojung.Huh@microchip.com a écrit :
> >>> Targetting the "net" tree since these are bugfixes, but I would like
> >>> Woojun and Andrew to take a look and test that on their respective
> >>> HW setups as well.
> >>
> >> Ok I'll wait for Woojun and Andrew to give feedback.
> >
> > This patch fixes periodic phy read_status access when phy is configured as
> PHY_IGNORE_INTERRUPTS.
> 
> Great, thanks for the feedback!
> 
> > Tested and confirmed with LAN78xx USB-to-Ethernet driver except
> following checkpatch.pl warnings.
> >
> > WARNING: line over 80 characters
> > #54: FILE: drivers/net/phy/phy.c:1008:
> > +		queue_delayed_work(system_power_efficient_wq,
> &phydev->state_queue,
> 
> This one is definitively added by the patch
> 
> >
> > WARNING: Comparisons should place the constant on the right side of the
> test
> > #68: FILE: drivers/net/phy/phy.c:714:
> > +		if (PHY_HALTED != phydev->state &&
> >
> 
> This one is an existing warning.
> 
> David, let me know if you consider the over 80 columns issue worth a
> resubmission or not, either way is fine with me.
> 
> Thanks!
> --
> Florian

RE: [PATCH net 0/3] net: phy: Finally fix PHY_IGNORE_INTERRUPTS

[Woojung.Huh]

 > I'm experiencing misbehavior after restart system.
> Can you wait applying the patch?
> 
> Sorry about it.
> Woojung

Sorry for spam. Version mismatch happened. :(
It works as expected on USB-to-Ethernet point of view.

Woojung

Re: [PATCH net 0/3] net: phy: Finally fix PHY_IGNORE_INTERRUPTS

[David Miller]

From: <Woojung.Huh@microchip.com>
Date: Thu, 21 Jan 2016 00:59:42 +0000

>  > I'm experiencing misbehavior after restart system.
>> Can you wait applying the patch?
>> 
>> Sorry about it.
>> Woojung
> 
> Sorry for spam. Version mismatch happened. :(
> It works as expected on USB-to-Ethernet point of view.

So Florian, can I apply this series now?

Re: [PATCH net 0/3] net: phy: Finally fix PHY_IGNORE_INTERRUPTS

[Florian Fainelli]

On January 20, 2016 6:58:09 PM PST, David Miller <davem@davemloft.net> wrote:
>From: <Woojung.Huh@microchip.com>
>Date: Thu, 21 Jan 2016 00:59:42 +0000
>
>>  > I'm experiencing misbehavior after restart system.
>>> Can you wait applying the patch?
>>> 
>>> Sorry about it.
>>> Woojung
>> 
>> Sorry for spam. Version mismatch happened. :(
>> It works as expected on USB-to-Ethernet point of view.
>
>So Florian, can I apply this series now?

Yes, sorry for the lag and thanks for your patience.


-- 
Florian

Re: [PATCH net 0/3] net: phy: Finally fix PHY_IGNORE_INTERRUPTS

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Wed, 20 Jan 2016 22:32:09 -0800

> On January 20, 2016 6:58:09 PM PST, David Miller <davem@davemloft.net> wrote:
>>From: <Woojung.Huh@microchip.com>
>>Date: Thu, 21 Jan 2016 00:59:42 +0000
>>
>>>  > I'm experiencing misbehavior after restart system.
>>>> Can you wait applying the patch?
>>>> 
>>>> Sorry about it.
>>>> Woojung
>>> 
>>> Sorry for spam. Version mismatch happened. :(
>>> It works as expected on USB-to-Ethernet point of view.
>>
>>So Florian, can I apply this series now?
> 
> Yes, sorry for the lag and thanks for your patience.

Great, series applied, thanks.