Re: [PATCH net-next 00/19] net: phy: add support for shared interrupts (part 1)

[Heiner Kallweit]

On 29.10.2020 11:07, Ioana Ciornei wrote:
> From: Ioana Ciornei <ioana.ciornei@xxxxxxx>
> 
> 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@xxxxxxxxxx>
> Cc: Andre Edich <andre.edich@xxxxxxxxxxxxx>
> Cc: Antoine Tenart <atenart@xxxxxxxxxx>
> Cc: Baruch Siach <baruch@xxxxxxxxxx>
> Cc: Christophe Leroy <christophe.leroy@xxxxxx>
> Cc: Dan Murphy <dmurphy@xxxxxx>
> Cc: Divya Koppera <Divya.Koppera@xxxxxxxxxxxxx>
> Cc: Florian Fainelli <f.fainelli@xxxxxxxxx>
> Cc: Hauke Mehrtens <hauke@xxxxxxxxxx>
> Cc: Heiner Kallweit <hkallweit1@xxxxxxxxx>
> Cc: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
> Cc: Kavya Sree Kotagiri <kavyasree.kotagiri@xxxxxxxxxxxxx>
> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Cc: Marco Felsch <m.felsch@xxxxxxxxxxxxxx>
> Cc: Marek Vasut <marex@xxxxxxx>
> Cc: Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
> Cc: Mathias Kresin <dev@xxxxxxxxx>
> Cc: Maxim Kochetkov <fido_max@xxxxxxxx>
> Cc: Michael Walle <michael@xxxxxxxx>
> Cc: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> Cc: Nisar Sayed <Nisar.Sayed@xxxxxxxxxxxxx>
> Cc: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> Cc: Philippe Schenker <philippe.schenker@xxxxxxxxxxx>
> Cc: Willy Liu <willy.liu@xxxxxxxxxxx>
> Cc: Yuiko Oshino <yuiko.oshino@xxxxxxxxxxxxx>
>