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
| ||
|
Message-ID: <20201031052708.geep7ydfiotzdrvg@skbuf> Date: Sat, 31 Oct 2020 07:27:08 +0200 From: Ioana Ciornei <ciorneiioana@...il.com> To: Heiner Kallweit <hkallweit1@...il.com> Cc: Ioana Ciornei <ciorneiioana@...il.com>, Andrew Lunn <andrew@...n.ch>, Russell King <linux@...linux.org.uk>, Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, Ioana Ciornei <ioana.ciornei@....com>, Alexandru Ardelean <alexandru.ardelean@...log.com>, Andre Edich <andre.edich@...rochip.com>, Antoine Tenart <atenart@...nel.org>, Baruch Siach <baruch@...s.co.il>, Christophe Leroy <christophe.leroy@....fr>, Dan Murphy <dmurphy@...com>, Divya Koppera <Divya.Koppera@...rochip.com>, Florian Fainelli <f.fainelli@...il.com>, Hauke Mehrtens <hauke@...ke-m.de>, Jerome Brunet <jbrunet@...libre.com>, Kavya Sree Kotagiri <kavyasree.kotagiri@...rochip.com>, Linus Walleij <linus.walleij@...aro.org>, Marco Felsch <m.felsch@...gutronix.de>, Marek Vasut <marex@...x.de>, Martin Blumenstingl <martin.blumenstingl@...glemail.com>, Mathias Kresin <dev@...sin.me>, Maxim Kochetkov <fido_max@...ox.ru>, Michael Walle <michael@...le.cc>, Neil Armstrong <narmstrong@...libre.com>, Nisar Sayed <Nisar.Sayed@...rochip.com>, Oleksij Rempel <o.rempel@...gutronix.de>, Philippe Schenker <philippe.schenker@...adex.com>, Willy Liu <willy.liu@...ltek.com>, Yuiko Oshino <yuiko.oshino@...rochip.com> Subject: Re: [PATCH net-next 00/19] net: phy: add support for shared interrupts (part 1) On Fri, Oct 30, 2020 at 10:56:24PM +0100, Heiner Kallweit wrote: > On 29.10.2020 11:07, Ioana Ciornei wrote: > > From: Ioana Ciornei <ioana.ciornei@....com> > > > > This patch set aims to actually add support for shared interrupts in > > phylib and not only for multi-PHY devices. While we are at it, > > streamline the interrupt handling in phylib. > > > > For a bit of context, at the moment, there are multiple phy_driver ops > > that deal with this subject: > > > > - .config_intr() - Enable/disable the interrupt line. > > > > - .ack_interrupt() - Should quiesce any interrupts that may have been > > fired. It's also used by phylib in conjunction with .config_intr() to > > clear any pending interrupts after the line was disabled, and before > > it is going to be enabled. > > > > - .did_interrupt() - Intended for multi-PHY devices with a shared IRQ > > line and used by phylib to discern which PHY from the package was the > > one that actually fired the interrupt. > > > > - .handle_interrupt() - Completely overrides the default interrupt > > handling logic from phylib. The PHY driver is responsible for checking > > if any interrupt was fired by the respective PHY and choose > > accordingly if it's the one that should trigger the link state machine. > > > >>From my point of view, the interrupt handling in phylib has become > > somewhat confusing with all these callbacks that actually read the same > > PHY register - the interrupt status. A more streamlined approach would > > be to just move the responsibility to write an interrupt handler to the > > driver (as any other device driver does) and make .handle_interrupt() > > the only way to deal with interrupts. > > > > Another advantage with this approach would be that phylib would gain > > support for shared IRQs between different PHY (not just multi-PHY > > devices), something which at the moment would require extending every > > PHY driver anyway in order to implement their .did_interrupt() callback > > and duplicate the same logic as in .ack_interrupt(). The disadvantage > > of making .did_interrupt() mandatory would be that we are slightly > > changing the semantics of the phylib API and that would increase > > confusion instead of reducing it. > > > > What I am proposing is the following: > > > > - As a first step, make the .ack_interrupt() callback optional so that > > we do not break any PHY driver amid the transition. > > > > - Every PHY driver gains a .handle_interrupt() implementation that, for > > the most part, would look like below: > > > > irq_status = phy_read(phydev, INTR_STATUS); > > if (irq_status < 0) { > > phy_error(phydev); > > return IRQ_NONE; > > } > > > > if (irq_status == 0) > > return IRQ_NONE; > > > > phy_trigger_machine(phydev); > > > > return IRQ_HANDLED; > > > > - Remove each PHY driver's implementation of the .ack_interrupt() by > > actually taking care of quiescing any pending interrupts before > > enabling/after disabling the interrupt line. > > > > - Finally, after all drivers have been ported, remove the > > .ack_interrupt() and .did_interrupt() callbacks from phy_driver. > > > > Looks good to me. The current interrupt support in phylib basically > just covers the link change interrupt and we need more flexibility. > > And even in the current limited use case we face smaller issues. > One reason is that INTR_STATUS typically is self-clearing on read. > phylib has to deal with the case that did_interrupt may or may not > have read INTR_STATUS already. > > I'd just like to avoid the term "shared interrupt", because it has > a well-defined meaning. Our major concern isn't shared interrupts > but support for multiple interrupt sources (in addition to > link change) in a PHY. > I am not going to address this part, Vladimir did a good job in the following emails describing exactly the problem that I am trying to fix - shared interrupts even between PHYs which are not in the same package or even the same type of device. > WRT implementing a shutdown hook another use case was mentioned > recently: https://lkml.org/lkml/2020/9/30/451 > But that's not really relevant here and just fyi. > I missed this thread. Thanks for the link! Ioana
Powered by blists - more mailing lists