| Message ID | 20201029100741.462818-1-ciorneiioana@gmail.com |
|---|---|
| Headers | show |
| Series | net: phy: add support for shared interrupts (part 1) | expand |
On 29.10.2020 11:07, Ioana Ciornei wrote: > From: Ioana Ciornei <ioana.ciornei@nxp.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. 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. > This patch set is part 1 and it addresses the changes needed in phylib > and 7 PHY drivers. The rest can be found on my Github branch here: > https://github.com/IoanaCiornei/linux/commits/phylib-shared-irq > > I do not have access to most of these PHY's, therefore I Cc-ed the > latest contributors to the individual PHY drivers in order to have > access, hopefully, to more regression testing. > > Ioana Ciornei (19): > net: phy: export phy_error and phy_trigger_machine > net: phy: add a shutdown procedure > net: phy: make .ack_interrupt() optional > net: phy: at803x: implement generic .handle_interrupt() callback > net: phy: at803x: remove the use of .ack_interrupt() > net: phy: mscc: use phy_trigger_machine() to notify link change > net: phy: mscc: implement generic .handle_interrupt() callback > net: phy: mscc: remove the use of .ack_interrupt() > net: phy: aquantia: implement generic .handle_interrupt() callback > net: phy: aquantia: remove the use of .ack_interrupt() > net: phy: broadcom: implement generic .handle_interrupt() callback > net: phy: broadcom: remove use of ack_interrupt() > net: phy: cicada: implement the generic .handle_interrupt() callback > net: phy: cicada: remove the use of .ack_interrupt() > net: phy: davicom: implement generic .handle_interrupt() calback > net: phy: davicom: remove the use of .ack_interrupt() > net: phy: add genphy_handle_interrupt_no_ack() > net: phy: realtek: implement generic .handle_interrupt() callback > net: phy: realtek: remove the use of .ack_interrupt() > > drivers/net/phy/aquantia_main.c | 57 ++++++++++---- > drivers/net/phy/at803x.c | 42 ++++++++-- > drivers/net/phy/bcm-cygnus.c | 2 +- > drivers/net/phy/bcm-phy-lib.c | 37 ++++++++- > drivers/net/phy/bcm-phy-lib.h | 1 + > drivers/net/phy/bcm54140.c | 39 +++++++--- > drivers/net/phy/bcm63xx.c | 20 +++-- > drivers/net/phy/bcm87xx.c | 50 ++++++------ > drivers/net/phy/broadcom.c | 70 ++++++++++++----- > drivers/net/phy/cicada.c | 35 ++++++++- > drivers/net/phy/davicom.c | 59 ++++++++++---- > drivers/net/phy/mscc/mscc_main.c | 70 +++++++++-------- > drivers/net/phy/phy.c | 6 +- > drivers/net/phy/phy_device.c | 23 +++++- > drivers/net/phy/realtek.c | 128 +++++++++++++++++++++++++++---- > include/linux/phy.h | 3 + > 16 files changed, 484 insertions(+), 158 deletions(-) > > Cc: Alexandru Ardelean <alexandru.ardelean@analog.com> > Cc: Andre Edich <andre.edich@microchip.com> > Cc: Antoine Tenart <atenart@kernel.org> > Cc: Baruch Siach <baruch@tkos.co.il> > Cc: Christophe Leroy <christophe.leroy@c-s.fr> > Cc: Dan Murphy <dmurphy@ti.com> > Cc: Divya Koppera <Divya.Koppera@microchip.com> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: Hauke Mehrtens <hauke@hauke-m.de> > Cc: Heiner Kallweit <hkallweit1@gmail.com> > Cc: Jerome Brunet <jbrunet@baylibre.com> > Cc: Kavya Sree Kotagiri <kavyasree.kotagiri@microchip.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Marco Felsch <m.felsch@pengutronix.de> > Cc: Marek Vasut <marex@denx.de> > Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Cc: Mathias Kresin <dev@kresin.me> > Cc: Maxim Kochetkov <fido_max@inbox.ru> > Cc: Michael Walle <michael@walle.cc> > Cc: Neil Armstrong <narmstrong@baylibre.com> > Cc: Nisar Sayed <Nisar.Sayed@microchip.com> > Cc: Oleksij Rempel <o.rempel@pengutronix.de> > Cc: Philippe Schenker <philippe.schenker@toradex.com> > Cc: Willy Liu <willy.liu@realtek.com> > Cc: Yuiko Oshino <yuiko.oshino@microchip.com> >
On Fri, Oct 30, 2020 at 10:56:24PM +0100, Heiner Kallweit wrote: > 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. You may be a little bit confused Heiner. This series adds support for exactly _that_ meaning of shared interrupts. Shared interrupts (aka wired-OR on the PCB) don't work today with the PHY library. I have a board that won't even boot to prompt when the interrupt lines of its 2 PHYs are enabled, that this series fixes. You might need to take another look through the commit messages I'm afraid.
On 30.10.2020 23:06, Vladimir Oltean wrote: > On Fri, Oct 30, 2020 at 10:56:24PM +0100, Heiner Kallweit wrote: >> 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. > > You may be a little bit confused Heiner. > This series adds support for exactly _that_ meaning of shared interrupts. > Shared interrupts (aka wired-OR on the PCB) don't work today with the > PHY library. I have a board that won't even boot to prompt when the > interrupt lines of its 2 PHYs are enabled, that this series fixes. > You might need to take another look through the commit messages I'm afraid. > Right, I was focusing on the PTP irq use case.
On 29.10.2020 11:07, Ioana Ciornei wrote: > From: Ioana Ciornei <ioana.ciornei@nxp.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) Here I have a concern, bits may be set even if the respective interrupt source isn't enabled. Therefore we may falsely blame a device to have triggered the interrupt. irq_status should be masked with the actually enabled irq source bits. > 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. > > This patch set is part 1 and it addresses the changes needed in phylib > and 7 PHY drivers. The rest can be found on my Github branch here: > https://github.com/IoanaCiornei/linux/commits/phylib-shared-irq > > I do not have access to most of these PHY's, therefore I Cc-ed the > latest contributors to the individual PHY drivers in order to have > access, hopefully, to more regression testing. > > Ioana Ciornei (19): > net: phy: export phy_error and phy_trigger_machine > net: phy: add a shutdown procedure > net: phy: make .ack_interrupt() optional > net: phy: at803x: implement generic .handle_interrupt() callback > net: phy: at803x: remove the use of .ack_interrupt() > net: phy: mscc: use phy_trigger_machine() to notify link change > net: phy: mscc: implement generic .handle_interrupt() callback > net: phy: mscc: remove the use of .ack_interrupt() > net: phy: aquantia: implement generic .handle_interrupt() callback > net: phy: aquantia: remove the use of .ack_interrupt() > net: phy: broadcom: implement generic .handle_interrupt() callback > net: phy: broadcom: remove use of ack_interrupt() > net: phy: cicada: implement the generic .handle_interrupt() callback > net: phy: cicada: remove the use of .ack_interrupt() > net: phy: davicom: implement generic .handle_interrupt() calback > net: phy: davicom: remove the use of .ack_interrupt() > net: phy: add genphy_handle_interrupt_no_ack() > net: phy: realtek: implement generic .handle_interrupt() callback > net: phy: realtek: remove the use of .ack_interrupt() > > drivers/net/phy/aquantia_main.c | 57 ++++++++++---- > drivers/net/phy/at803x.c | 42 ++++++++-- > drivers/net/phy/bcm-cygnus.c | 2 +- > drivers/net/phy/bcm-phy-lib.c | 37 ++++++++- > drivers/net/phy/bcm-phy-lib.h | 1 + > drivers/net/phy/bcm54140.c | 39 +++++++--- > drivers/net/phy/bcm63xx.c | 20 +++-- > drivers/net/phy/bcm87xx.c | 50 ++++++------ > drivers/net/phy/broadcom.c | 70 ++++++++++++----- > drivers/net/phy/cicada.c | 35 ++++++++- > drivers/net/phy/davicom.c | 59 ++++++++++---- > drivers/net/phy/mscc/mscc_main.c | 70 +++++++++-------- > drivers/net/phy/phy.c | 6 +- > drivers/net/phy/phy_device.c | 23 +++++- > drivers/net/phy/realtek.c | 128 +++++++++++++++++++++++++++---- > include/linux/phy.h | 3 + > 16 files changed, 484 insertions(+), 158 deletions(-) > > Cc: Alexandru Ardelean <alexandru.ardelean@analog.com> > Cc: Andre Edich <andre.edich@microchip.com> > Cc: Antoine Tenart <atenart@kernel.org> > Cc: Baruch Siach <baruch@tkos.co.il> > Cc: Christophe Leroy <christophe.leroy@c-s.fr> > Cc: Dan Murphy <dmurphy@ti.com> > Cc: Divya Koppera <Divya.Koppera@microchip.com> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: Hauke Mehrtens <hauke@hauke-m.de> > Cc: Heiner Kallweit <hkallweit1@gmail.com> > Cc: Jerome Brunet <jbrunet@baylibre.com> > Cc: Kavya Sree Kotagiri <kavyasree.kotagiri@microchip.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Marco Felsch <m.felsch@pengutronix.de> > Cc: Marek Vasut <marex@denx.de> > Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Cc: Mathias Kresin <dev@kresin.me> > Cc: Maxim Kochetkov <fido_max@inbox.ru> > Cc: Michael Walle <michael@walle.cc> > Cc: Neil Armstrong <narmstrong@baylibre.com> > Cc: Nisar Sayed <Nisar.Sayed@microchip.com> > Cc: Oleksij Rempel <o.rempel@pengutronix.de> > Cc: Philippe Schenker <philippe.schenker@toradex.com> > Cc: Willy Liu <willy.liu@realtek.com> > Cc: Yuiko Oshino <yuiko.oshino@microchip.com> >
On Sat, Oct 31, 2020 at 12:06:42AM +0200, Vladimir Oltean wrote: > On Fri, Oct 30, 2020 at 10:56:24PM +0100, Heiner Kallweit wrote: > > 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. > > You may be a little bit confused Heiner. > This series adds support for exactly _that_ meaning of shared interrupts. > Shared interrupts (aka wired-OR on the PCB) don't work today with the > PHY library. I have a board that won't even boot to prompt when the > interrupt lines of its 2 PHYs are enabled, that this series fixes. > You might need to take another look through the commit messages I'm afraid. Maybe this diagram will help you visualize better. time | | PHY 1 PHY 2 has pending IRQ | | (e.g. link up) | v | | phy_interrupt() | | called for PHY 1 | | | | | v | | returns IRQ_HANDLED via | | phy_clear_interrupt() | | | | | | | | v | | handling of shared IRQ | | ends here | | | | | | v | | PHY 2 still has pending IRQ | | because, you know, it wasn't actually | | serviced | v | | phy_interrupt() | | called for PHY 1 | | | | | v | | returns IRQ_HANDLED via | | phy_clear_interrupt() | | | | | | | | v | | handling of shared IRQ | | ends here | | | PHY 2: Hey! It's me! Over here! | | | | v | | phy_interrupt() | | called for PHY 1 | | | | | v | | returns IRQ_HANDLED via | | phy_clear_interrupt() | | | | | | | | v | | handling of shared IRQ | | ends here | | | PHY 2: Srsly? | | | | v | | phy_interrupt() | | called for PHY 1 | | | | | v | | ... ... | | 21 seconds later | | RCU stall v This happens because today, the way phy_interrupt() is written, you can only return IRQ_NONE and give the other driver a chance _if_ your driver implements .did_interrupt(). But the kernel documentation of .did_interrupt() only recommends to implement that function if you are a multi-PHY package driver (otherwise stated, the hardware chip has an embedded shared IRQ). But as things stand, _everybody_ should implement .did_interrupt() in order for any combination of PHY drivers to support shared IRQs. What Ioana is proposing, and this is something that I fully agree with, is that we just get rid of the layering where the PHY library tries to be helpful but instead invites everybody to write systematically bad code. Anyone knows how to write an IRQ handler with eyes closed, but the fact that .did_interrupt() is mandatory for proper shared IRQ support is not obvious to everybody, it seems. So let's just have a dedicated IRQ handling function per each PHY driver, so that we don't get confused in this sloppy mess of return values, and the code can actually be followed. Even _with_ Ioana's changes, there is one more textbook case of shared interrupts causing trouble, and that is actually the reason why nobody likes them except hardware engineers who don't get to deal with this. time | | PHY 1 probed | (module or built-in) | | PHY 2 has pending IRQ | | (it had link up from previous | v boot, or from bootloader, etc) | phy_interrupt() | | called for PHY 1 | | | | | v | | returns IRQ_NONE as | | it should v | | PHY 2 still has pending IRQ | | but its handler wasn't called | | because its driver has not been | | yet loaded | v | | phy_interrupt() | | called for PHY 1 | | | | | v | | returns IRQ_NONE as | | it should v | | PHY 2: Not again :( | | | | v | | phy_interrupt() | | called for PHY 1 | | | | | v | | returns IRQ_NONE as | | it should | | | | | ... ... | | | | | PHY 2 driver never gets probed | | either because it's a module or | | because the system is too busy | | checking PHY 1 over and over | | again for an interrupt that | | it did not trigger | | | | ... ... | | 21 seconds later | | RCU stall v The way that it's solved is that it's never 100% solved. This one you can just avoid, but never recover from it. To avoid it, you must ensure from your previous boot environments (bootloader, kexec) that the IRQ line is not pending. Because if you leave the shared IRQ line pending, the system is kaput if it happens to probe the drivers in the wrong order (aka don't probe first the driver that will clear that shared IRQ). It's like Minesweeper, only worse. That's why the shutdown hook is there, as a best-effort attempt for Linux to clean up after itself. But we're always at the mercy of the bootloader, or even at the mercy of chance. If the previous kernel panicked, there's no orderly cleanup to speak of. Hope it's clearer now.
> > - 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) > > Here I have a concern, bits may be set even if the respective interrupt > source isn't enabled. Therefore we may falsely blame a device to have > triggered the interrupt. irq_status should be masked with the actually > enabled irq source bits. Hi Heiner I would say that is a driver implementation detail, for each driver to handle how it needs to handle it. I've seen some hardware where the interrupt status is already masked with the interrupt enabled bits. I've soon other hardware where it is not. For example code, what is listed above is O.K. The real implementation in a driver need knowledge of the hardware. Andrew
On Sat, Oct 31, 2020 at 12:36:27AM +0100, Andrew Lunn wrote: > > > - 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) > > > > Here I have a concern, bits may be set even if the respective interrupt > > source isn't enabled. Therefore we may falsely blame a device to have > > triggered the interrupt. irq_status should be masked with the actually > > enabled irq source bits. > > Hi Heiner > > I would say that is a driver implementation detail, for each driver to > handle how it needs to handle it. I've seen some hardware where the > interrupt status is already masked with the interrupt enabled > bits. I've soon other hardware where it is not. > > For example code, what is listed above is O.K. The real implementation > in a driver need knowledge of the hardware. > Hi, As Andrew said, that is just an example code that could work for some devices but should be extended depending on how the actual PHY is working. For example, the VSC8584 will still be trigerring the link state machine just on the link change interrupt, I am not changing this: static irqreturn_t vsc8584_handle_interrupt(struct phy_device *phydev) { irqreturn_t ret; int irq_status; irq_status = phy_read(phydev, MII_VSC85XX_INT_STATUS); if (irq_status < 0) return IRQ_NONE; /* Timestamping IRQ does not set a bit in the global INT_STATUS, so * irq_status would be 0. */ ret = vsc8584_handle_ts_interrupt(phydev); if (!(irq_status & MII_VSC85XX_INT_MASK_MASK)) return ret; if (irq_status & MII_VSC85XX_INT_MASK_EXT) vsc8584_handle_macsec_interrupt(phydev); if (irq_status & MII_VSC85XX_INT_MASK_LINK_CHG) phy_trigger_machine(phydev); return IRQ_HANDLED; } Ioana
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@nxp.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
On 31.10.2020 00:36, Andrew Lunn wrote: >>> - 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) >> >> Here I have a concern, bits may be set even if the respective interrupt >> source isn't enabled. Therefore we may falsely blame a device to have >> triggered the interrupt. irq_status should be masked with the actually >> enabled irq source bits. > > Hi Heiner > Hi Andrew, > I would say that is a driver implementation detail, for each driver to > handle how it needs to handle it. I've seen some hardware where the > interrupt status is already masked with the interrupt enabled > bits. I've soon other hardware where it is not. > Sure, I just wanted to add the comment before others simply copy and paste this (pseudo) code. And in patch 9 (aquantia) and 18 (realtek) it is used as is. And IIRC at least the Aquantia PHY doesn't mask the interrupt status. > For example code, what is listed above is O.K. The real implementation > in a driver need knowledge of the hardware. > > Andrew > . > Heiner
On Sat, Oct 31, 2020 at 11:18:18AM +0100, Heiner Kallweit wrote: > On 31.10.2020 00:36, Andrew Lunn wrote: > >>> - 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) > >> > >> Here I have a concern, bits may be set even if the respective interrupt > >> source isn't enabled. Therefore we may falsely blame a device to have > >> triggered the interrupt. irq_status should be masked with the actually > >> enabled irq source bits. > > > > Hi Heiner > > > Hi Andrew, > > > I would say that is a driver implementation detail, for each driver to > > handle how it needs to handle it. I've seen some hardware where the > > interrupt status is already masked with the interrupt enabled > > bits. I've soon other hardware where it is not. > > > Sure, I just wanted to add the comment before others simply copy and > paste this (pseudo) code. And in patch 9 (aquantia) and 18 (realtek) > it is used as is. And IIRC at least the Aquantia PHY doesn't mask > the interrupt status. > Hi Heiner, If I understand correctly what you are suggesting, the .handle_interrupt() for the aquantia would look like this: static irqreturn_t aqr_handle_interrupt(struct phy_device *phydev) { int irq_status; irq_status = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_TX_VEND_INT_STATUS2); if (irq_status < 0) { phy_error(phydev); return IRQ_NONE; } if (!(irq_status & MDIO_AN_TX_VEND_INT_STATUS2_MASK)) return IRQ_NONE; phy_trigger_machine(phydev); return IRQ_HANDLED; } So only return IRQ_HANDLED when one of the bits from INT_STATUS corresponding with the enabled interrupts are set, not if any other link status bit is set. Ok, I'll send a v2 with these kind of changes. Ioana
> Sure, I just wanted to add the comment before others simply copy and > paste this (pseudo) code. And in patch 9 (aquantia) and 18 (realtek) > it is used as is. And IIRC at least the Aquantia PHY doesn't mask > the interrupt status. And that is were we are going to have issues with this patch set, and need review by individual PHY driver maintainers, or a good look at the datasheet. Andrew
On Sat, Oct 31, 2020 at 03:32:15PM +0100, Andrew Lunn wrote: > > Sure, I just wanted to add the comment before others simply copy and > > paste this (pseudo) code. And in patch 9 (aquantia) and 18 (realtek) > > it is used as is. And IIRC at least the Aquantia PHY doesn't mask > > the interrupt status. > > And that is were we are going to have issues with this patch set, and > need review by individual PHY driver maintainers, or a good look at > the datasheet. > Yep, I already started to comb through all the drivers and their datasheets so that I can only check for the interrupts that the driver enables. Ioana
From: Ioana Ciornei <ioana.ciornei@nxp.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.