From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiner Kallweit Subject: Re: [PATCH RfC 1/2] net: phy: core: remove now uneeded disabling of interrupts Date: Wed, 15 Nov 2017 23:19:24 +0100 Message-ID: References: <0f24f12a-2e1d-dd79-b425-77bd16db21e8@gmail.com> <272363be-cf94-e83d-d685-6d33a3900ad3@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, Ard Biesheuvel To: Florian Fainelli , Andrew Lunn , David Miller Return-path: Received: from mail-wm0-f67.google.com ([74.125.82.67]:45706 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756730AbdKOWTd (ORCPT ); Wed, 15 Nov 2017 17:19:33 -0500 Received: by mail-wm0-f67.google.com with SMTP id 9so5909943wme.4 for ; Wed, 15 Nov 2017 14:19:32 -0800 (PST) In-Reply-To: <272363be-cf94-e83d-d685-6d33a3900ad3@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: 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. > 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 >> --- >> 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; >> >> > >