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]

Date:   Mon, 04 Dec 2017 10:50:15 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     ard.biesheuvel@...aro.org
Cc:     hkallweit1@...il.com, f.fainelli@...il.com, andrew@...n.ch,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next resubmit 1/2] net: phy: core: remove now
 uneeded disabling of interrupts

From: Ard Biesheuvel <ard.biesheuvel@...aro.org>
Date: Mon, 4 Dec 2017 15:46:55 +0000

> 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:

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.