On Mon, Jun 06, 2022 at 07:53:20AM +0200, Lukas Wunner wrote:
> On Mon, Jun 06, 2022 at 03:40:49AM +0200, Andrew Lunn wrote:
> > > + if (phy_interrupt_is_valid(phydev)) {
> > > + phydev->irq_suspended = 0;
> > > + synchronize_irq(phydev->irq);
> > > +
> > > + /* Rerun interrupts which were postponed by phy_interrupt()
> > > + * because they occurred during the system sleep transition.
> > > + */
> > > + if (phydev->irq_rerun) {
> > > + phydev->irq_rerun = 0;
> > > + enable_irq(phydev->irq);
> > > + irq_wake_thread(phydev->irq, phydev);
> > > + }
> > > + }
> >
> > As i said in a previous thread, PHY interrupts are generally level,
> > not edge. So when you call enable_irq(phydev->irq), doesn't it
> > immediately fire?
>
> Yes, if the interrupt is indeed level and the PHY is capable of
> remembering that an interrupt occurred while the system was suspended
> or was about to be suspended.
It should remember, in the WoL case. It keeps it power etc.
> The irq_wake_thread() ensures that the IRQ handler is called,
> should one of those conditions *not* be met.
>
> Recall that phylib uses irq_default_primary_handler() as hardirq
> handler. That handler does nothing else but wake the IRQ thread,
> which runs phy->handle_interrupt() in task context.
>
> The irq_wake_thread() above likewise wakes the IRQ thread,
> i.e. it tells the scheduler to put it on the run queue.
>
> If, as you say, the interrupt is level and fires upon enable_irq(),
> the result is that the scheduler is told twice to put the IRQ thread
> on the run queue. Usually this will happen faster than the IRQ thread
> actually gets scheduled, so it will only run once.
>
> In the unlikely event that the IRQ thread gets scheduled before the
> call to irq_wake_thread(), the IRQ thread will run twice.
> However, that's harmless. IRQ handlers can cope with that.
I'm just slightly worried about the IRQ handler returning there was
nothing to do. The IRQ core counts such interrupts, and will disable
the interrupt if nobody says it is actually handling the
interrupts. But it needs to be a few interrupts before this kicks in,
so it should be safe.
One other thought is we should probably get the IRQ Maintainers to
look this patch over. Please could you repost and Cc: them.
Thanks
Andrew
