netdev - Re: [PATCH net] net: phy: Don't trigger state machine while in suspend

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, 6 Jun 2022 07:53:20 +0200
From:   Lukas Wunner <lukas@...ner.de>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        netdev@...r.kernel.org,
        Steve Glendinning <steve.glendinning@...well.net>,
        UNGLinuxDriver@...rochip.com, Oliver Neukum <oneukum@...e.com>,
        Andre Edich <andre.edich@...rochip.com>,
        Oleksij Rempel <linux@...pel-privat.de>,
        Martyn Welch <martyn.welch@...labora.com>,
        Gabriel Hojda <ghojda@...urs.ro>,
        Christoph Fritz <chf.fritz@...glemail.com>,
        Lino Sanfilippo <LinoSanfilippo@....de>,
        Philipp Rosenberger <p.rosenberger@...bus.com>,
        Ferry Toth <fntoth@...il.com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH net] net: phy: Don't trigger state machine while in
 suspend

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.

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.

> You need to first call the handler, and then re-enable the interrupt.

I guess I could call phy_interrupt() before the enable_irq(),
in lieu of irq_wake_thread().

However, it would mean that I'd invoke the IRQ handler behind the back
of the generic irq code.  That doesn't feel quite right.
Calling irq_wake_thread() is the correct way if one wants to be
compliant with the generic irq code's expectations.

If you feel strongly about it I can make that change but I would
advise against it.  Let me know what you think.

Thanks,

Lukas