[PATCH net-next resubmit 1/2] net: phy: core: remove now uneeded disabling of interrupts

After commits c974bdbc3e "net: phy: Use threaded IRQ, to allow IRQ from
sleeping devices" and 664fcf123a30 "net: phy: Threaded interrupts allow
some simplification" all relevant code pieces run in process context
anyway and I don't think we need the disabling of interrupts any longer.

Interestingly enough, latter commit already removed the comment
explaining why interrupts need to be temporarily disabled.

On my system phy interrupt mode works fine with this patch.
However I may miss something, especially in the context of shared phy
interrupts, therefore I'd appreciate if more people could test this.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Acked-by: Ard Biesheuvel <redacted>
---
 drivers/net/phy/phy.c | 26 ++------------------------
 include/linux/phy.h   |  1 -
 2 files changed, 2 insertions(+), 25 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 2b1e67bc1..b3784c9a2 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c

@@ -629,9 +629,6 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 	if (PHY_HALTED == phydev->state)
 		return IRQ_NONE;		/* It can't be ours.  */
 
-	disable_irq_nosync(irq);
-	atomic_inc(&phydev->irq_disable);
-
 	phy_change(phydev);
 
 	return IRQ_HANDLED;

@@ -689,7 +686,6 @@ static int phy_disable_interrupts(struct phy_device *phydev)
  */
 int phy_start_interrupts(struct phy_device *phydev)
 {
-	atomic_set(&phydev->irq_disable, 0);
 	if (request_threaded_irq(phydev->irq, NULL, phy_interrupt,
 				 IRQF_ONESHOT | IRQF_SHARED,
 				 phydev_name(phydev), phydev) < 0) {

@@ -716,13 +712,6 @@ int phy_stop_interrupts(struct phy_device *phydev)
 
 	free_irq(phydev->irq, phydev);
 
-	/* If work indeed has been cancelled, disable_irq() will have
-	 * been left unbalanced from phy_interrupt() and enable_irq()
-	 * has to be called so that other devices on the line work.
-	 */
-	while (atomic_dec_return(&phydev->irq_disable) >= 0)
-		enable_irq(phydev->irq);
-
 	return err;
 }
 EXPORT_SYMBOL(phy_stop_interrupts);

@@ -736,7 +725,7 @@ void phy_change(struct phy_device *phydev)
 	if (phy_interrupt_is_valid(phydev)) {
 		if (phydev->drv->did_interrupt &&
 		    !phydev->drv->did_interrupt(phydev))
-			goto ignore;
+			return;
 
 		if (phy_disable_interrupts(phydev))
 			goto phy_err;

@@ -748,27 +737,16 @@ void phy_change(struct phy_device *phydev)
 	mutex_unlock(&phydev->lock);
 
 	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;
+			goto phy_err;
 	}
 
 	/* reschedule state queue work to run as soon as possible */
 	phy_trigger_machine(phydev, true);
 	return;
 
-ignore:
-	atomic_dec(&phydev->irq_disable);
-	enable_irq(phydev->irq);
-	return;
-
-irq_enable_err:
-	disable_irq(phydev->irq);
-	atomic_inc(&phydev->irq_disable);
 phy_err:
 	phy_error(phydev);
 }

diff --git a/include/linux/phy.h b/include/linux/phy.h
index dc82a07cb..8a87e441f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -468,7 +468,6 @@ struct phy_device {
 	/* Interrupt and Polling infrastructure */
 	struct work_struct phy_queue;
 	struct delayed_work state_queue;
-	atomic_t irq_disable;
 
 	struct mutex lock;
 

-- 
2.15.0

If state is not PHY_HALTED I see no need to temporarily disable
interrupts on the device. As long as the current interrupt isn't acked
on the device no new interrupt can happen anyway.

In addition remove a unneeded enabling of interrupts in the state
machine when handling state PHY_CHANGELINK.

Tested on a Odroid-C2 with RTL8211F phy in interrupt mode.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index b3784c9a2..4a11de8cb 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c

@@ -727,8 +727,9 @@ void phy_change(struct phy_device *phydev)
 		    !phydev->drv->did_interrupt(phydev))
 			return;
 
-		if (phy_disable_interrupts(phydev))
-			goto phy_err;
+		if (phydev->state == PHY_HALTED)
+			if (phy_disable_interrupts(phydev))
+				goto phy_err;
 	}
 
 	mutex_lock(&phydev->lock);

@@ -736,15 +737,11 @@ void phy_change(struct phy_device *phydev)
 		phydev->state = PHY_CHANGELINK;
 	mutex_unlock(&phydev->lock);
 
-	if (phy_interrupt_is_valid(phydev)) {
-		/* Reenable interrupts */
-		if (PHY_HALTED != phydev->state &&
-		    phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED))
-			goto phy_err;
-	}
-
 	/* 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:

@@ -984,10 +981,6 @@ void phy_state_machine(struct work_struct *work)
 			phydev->state = PHY_NOLINK;
 			phy_link_down(phydev, true);
 		}
-
-		if (phy_interrupt_is_valid(phydev))
-			err = phy_config_interrupt(phydev,
-						   PHY_INTERRUPT_ENABLED);
 		break;
 	case PHY_HALTED:
 		if (phydev->link) {

-- 
2.15.0

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Thu, 30 Nov 2017 23:55:15 +0100

After commits c974bdbc3e "net: phy: Use threaded IRQ, to allow IRQ from
sleeping devices" and 664fcf123a30 "net: phy: Threaded interrupts allow
some simplification" all relevant code pieces run in process context
anyway and I don't think we need the disabling of interrupts any longer.

Interestingly enough, latter commit already removed the comment
explaining why interrupts need to be temporarily disabled.

On my system phy interrupt mode works fine with this patch.
However I may miss something, especially in the context of shared phy
interrupts, therefore I'd appreciate if more people could test this.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Acked-by: Ard Biesheuvel <redacted>

Ok, applied.

But if this causes regressions I'm reverting.

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Thu, 30 Nov 2017 23:57:00 +0100

If state is not PHY_HALTED I see no need to temporarily disable
interrupts on the device. As long as the current interrupt isn't acked
on the device no new interrupt can happen anyway.

In addition remove a unneeded enabling of interrupts in the state
machine when handling state PHY_CHANGELINK.

Tested on a Odroid-C2 with RTL8211F phy in interrupt mode.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Also applied, and again if this regresses things I'm reverting.

On 4 December 2017 at 15:24, David Miller [off-list ref] wrote:

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Thu, 30 Nov 2017 23:55:15 +0100

quoted

After commits c974bdbc3e "net: phy: Use threaded IRQ, to allow IRQ from
sleeping devices" and 664fcf123a30 "net: phy: Threaded interrupts allow
some simplification" all relevant code pieces run in process context
anyway and I don't think we need the disabling of interrupts any longer.

Interestingly enough, latter commit already removed the comment
explaining why interrupts need to be temporarily disabled.

On my system phy interrupt mode works fine with this patch.
However I may miss something, especially in the context of shared phy
interrupts, therefore I'd appreciate if more people could test this.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Acked-by: Ard Biesheuvel <redacted>

Ok, applied.

But if this causes regressions I'm reverting.

Thanks. But please note that the code in question does seem to use the
interrupt API incorrectly, and tbh, I was expecting some more
discussion first. For reference, here's the commit log for the mostly
equivalent patch [0] I sent out almost at the same time:

"""
The PHY interrupt handling code registers its interrupt as a oneshot
threaded interrupt, which guarantees that the interrupt will be masked
throughout both the primary and secondary handling stages.

However, the handling code still disables the interrupt by calling
disable_irq(), and re-enables it by calling enable_irq() after having
acked the interrupt in the PHY hardware.

This causes problems with hierarchical irqchip implementations built
on top of the GIC, because the core threaded interrupt code will only
EOI the interrupt if it is still masked after the secondary handler
completes. If this is not the case, the EOI is not emitted, and the
interrupt remains active, blocking further interrupts from the same
source. Disabling and enabling the interrupt will result in the secondary
handler completing with the interrupt unmasked, resulting in the above
behavior.

So remove the disable_irq/enable_irq, and rely on the fact that the
interrupt remains masked already.
"""

[0] https://marc.info/?l=linux-netdev&m=151016412011661&w=2

From: Ard Biesheuvel <redacted>
Date: Mon, 4 Dec 2017 15:46:55 +0000

On 4 December 2017 at 15:24, David Miller [off-list ref] wrote:

quoted

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Thu, 30 Nov 2017 23:55:15 +0100

quoted

After commits c974bdbc3e "net: phy: Use threaded IRQ, to allow IRQ from
sleeping devices" and 664fcf123a30 "net: phy: Threaded interrupts allow
some simplification" all relevant code pieces run in process context
anyway and I don't think we need the disabling of interrupts any longer.

Interestingly enough, latter commit already removed the comment
explaining why interrupts need to be temporarily disabled.

On my system phy interrupt mode works fine with this patch.
However I may miss something, especially in the context of shared phy
interrupts, therefore I'd appreciate if more people could test this.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Acked-by: Ard Biesheuvel <redacted>

Ok, applied.

But if this causes regressions I'm reverting.

Thanks. But please note that the code in question does seem to use the
interrupt API incorrectly, and tbh, I was expecting some more
discussion first. For reference, here's the commit log for the mostly
equivalent patch [0] I sent out almost at the same time:

Yes, it seemed to me that when the code was converted to threaded IRQS
this {enable,disable}_irq() stuff was not considered.

Again, I read these patches over and I'm willing to own up to the
changes for now.  And if they cause regressions or someone screams
loudly enough we can revert and talk about it some more.

On 4 December 2017 at 15:50, David Miller [off-list ref] wrote:

From: Ard Biesheuvel <redacted>
Date: Mon, 4 Dec 2017 15:46:55 +0000

quoted

On 4 December 2017 at 15:24, David Miller [off-list ref] wrote:

quoted

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Thu, 30 Nov 2017 23:55:15 +0100

quoted

After commits c974bdbc3e "net: phy: Use threaded IRQ, to allow IRQ from
sleeping devices" and 664fcf123a30 "net: phy: Threaded interrupts allow
some simplification" all relevant code pieces run in process context
anyway and I don't think we need the disabling of interrupts any longer.

Interestingly enough, latter commit already removed the comment
explaining why interrupts need to be temporarily disabled.

On my system phy interrupt mode works fine with this patch.
However I may miss something, especially in the context of shared phy
interrupts, therefore I'd appreciate if more people could test this.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Acked-by: Ard Biesheuvel <redacted>

Ok, applied.

But if this causes regressions I'm reverting.

Thanks. But please note that the code in question does seem to use the
interrupt API incorrectly, and tbh, I was expecting some more
discussion first. For reference, here's the commit log for the mostly
equivalent patch [0] I sent out almost at the same time:

Yes, it seemed to me that when the code was converted to threaded IRQS
this {enable,disable}_irq() stuff was not considered.

Again, I read these patches over and I'm willing to own up to the
changes for now.  And if they cause regressions or someone screams
loudly enough we can revert and talk about it some more.

Excellent, thanks.