On 15 November 2017 at 22:19, Heiner Kallweit <hkallwe...@gmail.com> wrote: > Am 15.11.2017 um 23:04 schrieb Florian Fainelli: >> On 11/12/2017 01:08 PM, Heiner Kallweit wrote: >>> 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. >> >> I am not currently in a position to test this, but this looks very >> similar, if not identical to what Ard submitted a few days earlier: >> > > Thanks for the hint. Indeed it's exactly the same patch, so the one > sent by me can be disregarded. >
Well, it does appear your patch is more complete. Another difference is that I actually need this change to fix an issue with a hierarchical irqchip stacked on top of the GIC. >> https://patchwork.kernel.org/patch/10048901/ >> >> Since net-next is closed at the moment, that should allow us to give >> this some good amount of testing. >> >> Thanks >> >>> >>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com> For the record Acked-by: Ard Biesheuvel <ard.biesheu...@linaro.org> Dear maintainers, Please take whichever of these patches looks more correct to you. Thanks, Ard. >>> --- >>> 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; >>> >>> >> >> >
