| Message ID | ba11e1ae-dc21-e036-f918-1ed44c92a424@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,resubmit,1/2] net: phy: core: remove now uneeded disabling of interrupts | expand |
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 <ard.biesheuvel@linaro.org> Ok, applied. But if this causes regressions I'm reverting.
On 4 December 2017 at 15:24, David Miller <davem@davemloft.net> wrote: > 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 <ard.biesheuvel@linaro.org> > > 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 <ard.biesheuvel@linaro.org> Date: Mon, 4 Dec 2017 15:46:55 +0000 > On 4 December 2017 at 15:24, David Miller <davem@davemloft.net> wrote: >> 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 <ard.biesheuvel@linaro.org> >> >> 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 <davem@davemloft.net> wrote: > From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Date: Mon, 4 Dec 2017 15:46:55 +0000 > >> On 4 December 2017 at 15:24, David Miller <davem@davemloft.net> wrote: >>> 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 <ard.biesheuvel@linaro.org> >>> >>> 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.
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;