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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKv+Gu-q7m_2uhE-Ghk0zmGjqVDwpBte2SwV8Mx-kPYA+uLwkA@mail.gmail.com>
Date:   Mon, 4 Dec 2017 15:46:55 +0000
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     David Miller <davem@...emloft.net>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        "<netdev@...r.kernel.org>" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next resubmit 1/2] net: phy: core: remove now uneeded
 disabling of interrupts

On 4 December 2017 at 15:24, David Miller <davem@...emloft.net> wrote:
> From: Heiner Kallweit <hkallweit1@...il.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@...il.com>
>> Acked-by: Ard Biesheuvel <ard.biesheuvel@...aro.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