Issue with commit 33c133cc7598e60976a phy: IRQ cannot be shared

Issue with commit 33c133cc7598e60976a phy: IRQ cannot be shared

[leroy christophe]

Hello Segei, Florian and David,

I have an hardware with two ethernet interfaces, and with the two PHYs 
inside the same component INTEL LXT973 which has only one interrupt.
I also have another hardware with two ethernet interfaces and two 
independant PHYs. But the two PHYs are wired to the same interrupt.
This is working perfectly up to Linux 3.12.

But since your commit, introduced in Linux 3.13, my interfaces don't 
work anymore as the second PHYs can't register IRQ.

Reading the commit log, I can't really understand the reason for the change.
Is it really worth it, and therefore how shall my case be handled ?

Christophe

Re: Issue with commit 33c133cc7598e60976a phy: IRQ cannot be shared

[Sergei Shtylyov]

Hello.

On 8/14/2014 10:31 AM, leroy christophe wrote:

> I have an hardware with two ethernet interfaces, and with the two PHYs inside
> the same component INTEL LXT973 which has only one interrupt.
> I also have another hardware with two ethernet interfaces and two independant
> PHYs. But the two PHYs are wired to the same interrupt.
> This is working perfectly up to Linux 3.12.

    Hm, I'm surprised it works. Are you sure you're getting interrupts from 
both PHYs? Because if both Ethernet controllers are active simultaneously, 
only the first registered PHY IRQ handler should get all the interrupts.

> But since your commit, introduced in Linux 3.13, my interfaces don't work
> anymore as the second PHYs can't register IRQ.

    Strange too, the phylib should use polling in case request_irq() fails.

> Reading the commit log, I can't really understand the reason for the change.

    The shared IRQ handler should check for IRQ from its device and return 
IRQ_NONE if there's no IRQ active; phy_interrupt() doesn't do that (this is 
pushed to the workqueue).

> Is it really worth it, and therefore how shall my case be handled ?

    PHY IRQs are not necessary for the phylib state machine.

> Christophe

WBR, Sergei

Re: Issue with commit 33c133cc7598e60976a phy: IRQ cannot be shared

[christophe leroy]

Le 14/08/2014 13:03, Sergei Shtylyov a écrit :
> Hello.
>
> On 8/14/2014 10:31 AM, leroy christophe wrote:
>
>> I have an hardware with two ethernet interfaces, and with the two 
>> PHYs inside
>> the same component INTEL LXT973 which has only one interrupt.
>> I also have another hardware with two ethernet interfaces and two 
>> independant
>> PHYs. But the two PHYs are wired to the same interrupt.
>> This is working perfectly up to Linux 3.12.
>
>    Hm, I'm surprised it works. Are you sure you're getting interrupts 
> from both PHYs? Because if both Ethernet controllers are active 
> simultaneously, only the first registered PHY IRQ handler should get 
> all the interrupts.
Yes it works. Why should only the first one get the interrupts ? 
handle_irq_event_percpu() (from kernel/irq/handle.c, extract below) 
calls all handlers regardless of whether they answer IRQ_NONE or 
IRQ_HANDLED. The break applies to the switch(), not to the while(). So 
all handlers are called.

irqreturn_t
handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
{
     irqreturn_t retval = IRQ_NONE;
     unsigned int flags = 0, irq = desc->irq_data.irq;

     do {
[...]
         switch (res) {
         case IRQ_WAKE_THREAD:
[...]
         case IRQ_HANDLED:
             flags |= action->flags;
             break;

         default:
             break;
         }

         retval |= res;
         action = action->next;
     } while (action);

>
>> But since your commit, introduced in Linux 3.13, my interfaces don't 
>> work
>> anymore as the second PHYs can't register IRQ.
>
>    Strange too, the phylib should use polling in case request_irq() 
> fails.
Well, you are right, I didn't check closely enough, was assuming they 
didn't register due to the messages saying interrupt mismatch.
>
>> Reading the commit log, I can't really understand the reason for the 
>> change.
>
>    The shared IRQ handler should check for IRQ from its device and 
> return IRQ_NONE if there's no IRQ active; phy_interrupt() doesn't do 
> that (this is pushed to the workqueue).
Well, as seen above, this has no impact on whether other handlers are 
called or not.
>
>> Is it really worth it, and therefore how shall my case be handled ?
>
>    PHY IRQs are not necessary for the phylib state machine.
However, polling is less efficient than IRQs. It wastes CPU and link 
loss detection is slower.
>
>> Christophe
>
> WBR, Sergei
>
BR
Christophe

---
Ce courrier électronique ne contient aucun virus ou logiciel malveillant parce que la protection avast! Antivirus est active.
http://www.avast.com

Re: Issue with commit 33c133cc7598e60976a phy: IRQ cannot be shared

[Sergei Shtylyov]

Hello.

On 08/15/2014 01:10 PM, christophe leroy wrote:

>>> I have an hardware with two ethernet interfaces, and with the two PHYs inside
>>> the same component INTEL LXT973 which has only one interrupt.
>>> I also have another hardware with two ethernet interfaces and two independant
>>> PHYs. But the two PHYs are wired to the same interrupt.
>>> This is working perfectly up to Linux 3.12.

>>    Hm, I'm surprised it works. Are you sure you're getting interrupts from
>> both PHYs? Because if both Ethernet controllers are active simultaneously,
>> only the first registered PHY IRQ handler should get all the interrupts.

> Yes it works. Why should only the first one get the interrupts ?
> handle_irq_event_percpu() (from kernel/irq/handle.c, extract below) calls all
> handlers regardless of whether they answer IRQ_NONE or IRQ_HANDLED. The break
> applies to the switch(), not to the while(). So all handlers are called.

    Indeed, my reasoning seems obsolete now, if ever valid at all. :-/
    I couldn't yet remember other reasons that caused me to do that patch last 
December. Perhaps it was also connected to the "rude" behaviour of the 
phylib's IRQ handler, which calls disable_irq_nosync()...

[...]

>>> Reading the commit log, I can't really understand the reason for the change.

>>> Is it really worth it, and therefore how shall my case be handled ?

>>    PHY IRQs are not necessary for the phylib state machine.

> However, polling is less efficient than IRQs. It wastes CPU and link loss
> detection is slower.

    Yes, but you can't avoid it even with valid IRQ, the way phylib is 
written: the state workqueue is activated once a second even in the absence of 
interrupts.

    What can also be done is getting rid of the IRQ workqueue and using 
threaded IRQs,

> BR
> Christophe

WBR, Sergei