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] [day] [month] [year] [list]

Date:   Mon, 6 Jun 2022 14:19:39 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Lukas Wunner <lukas@...ner.de>
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 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