Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Ioana Ciornei <ciorneiioana@gmail.com>
Date:   Sat, 31 Oct 2020 07:27:08 +0200
To:     Heiner Kallweit <hkallweit1@gmail.com>
Cc:     Ioana Ciornei <ciorneiioana@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Russell King <linux@armlinux.org.uk>,
        Jakub Kicinski <kuba@kernel.org>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Ioana Ciornei <ioana.ciornei@nxp.com>,
        Alexandru Ardelean <alexandru.ardelean@analog.com>,
        Andre Edich <andre.edich@microchip.com>,
        Antoine Tenart <atenart@kernel.org>,
        Baruch Siach <baruch@tkos.co.il>,
        Christophe Leroy <christophe.leroy@c-s.fr>,
        Dan Murphy <dmurphy@ti.com>,
        Divya Koppera <Divya.Koppera@microchip.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Hauke Mehrtens <hauke@hauke-m.de>,
        Jerome Brunet <jbrunet@baylibre.com>,
        Kavya Sree Kotagiri <kavyasree.kotagiri@microchip.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        Marco Felsch <m.felsch@pengutronix.de>,
        Marek Vasut <marex@denx.de>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        Mathias Kresin <dev@kresin.me>,
        Maxim Kochetkov <fido_max@inbox.ru>,
        Michael Walle <michael@walle.cc>,
        Neil Armstrong <narmstrong@baylibre.com>,
        Nisar Sayed <Nisar.Sayed@microchip.com>,
        Oleksij Rempel <o.rempel@pengutronix.de>,
        Philippe Schenker <philippe.schenker@toradex.com>,
        Willy Liu <willy.liu@realtek.com>,
        Yuiko Oshino <yuiko.oshino@microchip.com>
Subject: Re: [PATCH net-next 00/19] net: phy: add support for shared
 interrupts (part 1)
Message-ID: <20201031052708.geep7ydfiotzdrvg@skbuf>
References: <20201029100741.462818-1-ciorneiioana@gmail.com>
 <43d672ae-c089-6621-5ab3-3a0f0303e51a@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <43d672ae-c089-6621-5ab3-3a0f0303e51a@gmail.com>
Precedence: bulk

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