netdev - Re: [PATCH] net: lan78xx: fix "enabled interrupts" warninig

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: <20190410174538.5ebe1787@xhacker.debian>
Date:   Wed, 10 Apr 2019 09:53:40 +0000
From:   Jisheng Zhang <Jisheng.Zhang@...aptics.com>
To:     "RaghuramChary.Jallipalli@...rochip.com" 
        <RaghuramChary.Jallipalli@...rochip.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>
CC:     "marc.zyngier@....com" <marc.zyngier@....com>,
        "Woojung.Huh@...rochip.com" <Woojung.Huh@...rochip.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: lan78xx: fix "enabled interrupts" warninig


On Wed, 10 Apr 2019 09:20:38 +0000 wrote:

> 
> > > >
> > > > The warning comes from calling generic_handle_irq() in usb tasklet  
> > context.  
> > > > This is not correct.
> > > >
> > > > Per my understanding, if there's chained irq, we could introduce
> > > > extra irqdomain. E.g
> > > >
> > > > GIC <--> another irqchip controller <--> HW device
> > > >  
> > > Correct, IRQ domain is generally used in chained irq controllers.
> > > Yes, We need to check why irq domain is used in the current driver.
> > >  
> >
> > It's introduced in the commit cc89c323a30e
> >  
> Hi Jisheng,

Hi,

> I had spent some time to look into the history of commit. The reason for having irq domain in driver is because of the unavailability of real hardware interrupt, here we have USB interrupt pipe which is not actual interrupt.
> So changes were proposed to have pseudo phy interrupt and handlers in phy lib. But later it was suggested to implement linux interrupt controller in driver itself.

+ Andrew, Florian

USB net is different with Andrew's case.

there's no irq at all in usb net

but in Andrew's case, interrupt is connected to a GPIO line. As is known
GPIO can behave as irq chip.

> You can see the archive here https://patchwork.ozlabs.org/patch/564511/

Per my understanding, the purpose is to avoid phy_state_machine() poll
phy_read_status() to monitor phy link. But as pointed out by Florian,
phy_mac_interrupt() is intended for that purpose. And the poll should be
fixed in phy. Since the email was sent in 2016, maybe the bug has been fixed.

> I want to understand if there is any functionality impact with this warning? Because I'm afraid if the current changes are removed we might hit some other issues (or older ones). We have to go through rigorous testing before going ahead.

Warning indicates there's something wrong in the code.

IMHO phy_mac_interrupt() and PHY_IGNORE_INTERRUPT is the correct
solution. If the phy_mac_interrupt() poll is fixed, I think maybe
old issue which commit cc89c323a30e want to fix won't exist.

Thanks