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

From: Marc Zyngier <marc.zyngier@arm.com>
To: Jisheng Zhang <Jisheng.Zhang@synaptics.com>,
	"RaghuramChary.Jallipalli@microchip.com" 
	<RaghuramChary.Jallipalli@microchip.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>
Cc: "Woojung.Huh@microchip.com" <Woojung.Huh@microchip.com>,
	"UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] net: lan78xx: fix "enabled interrupts" warninig
Date: Wed, 10 Apr 2019 11:27:57 +0100	[thread overview]
Message-ID: <cc4814ef-56fc-c59e-7331-b95d791a0951@arm.com> (raw)
In-Reply-To: <20190410174538.5ebe1787@xhacker.debian>

On 10/04/2019 10:53, Jisheng Zhang wrote:
> 
> 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.

Most definitely. This code is completely busted (handling pseudo
interrupts out of a tasklet is a BUG), and things will crash or deadlock
under the right conditions.

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

Please also see this[1] thread, which has a potential solution for this.

Thanks,

	M.

[1] https://www.spinics.net/lists/netdev/msg542290.html
-- 
Jazz is not dead. It just smells funny...