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

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

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

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

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

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.

quoted

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

Le 20/01/2016 13:20, Woojung.Huh@microchip.com a écrit :

quoted

quoted

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

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 :

quoted

quoted

quoted

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!

quoted

Tested and confirmed with LAN78xx USB-to-Ethernet driver except

following checkpatch.pl warnings.

quoted

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

quoted

WARNING: Comparisons should place the constant on the right side of the

test

quoted

#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

 > 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

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

 > I'm experiencing misbehavior after restart system.

quoted

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?

On January 20, 2016 6:58:09 PM PST, David Miller [off-list ref] wrote:

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

quoted

 > I'm experiencing misbehavior after restart system.

quoted

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

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 [off-list ref] wrote:

quoted

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

quoted

 > I'm experiencing misbehavior after restart system.

quoted

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.