Re: [PATCH net-next v2 2/3] net: phy: add callback for custom interrupt handler to struct phy_driver

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Tue, Nov 19, 2019 at 11:33:47AM +0100, Michael Walle wrote:
> 
> Hi,
> 
> this is an old thread and I know its already applied. But I'd like to hear
> your opinion on the following problem below.
> 
> > The phylib interrupt handler handles link change events only currently.
> > However PHY drivers may want to use other interrupt sources too,
> > e.g. to report temperature monitoring events. Therefore add a callback
> > to struct phy_driver allowing PHY drivers to implement a custom
> > interrupt handler.
> > 
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > Suggested-by: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> > Acked-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/phy/phy.c | 9 +++++++--
> >  include/linux/phy.h   | 3 +++
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > index d90d9863e..068f0a126 100644
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -772,8 +772,13 @@ static irqreturn_t phy_interrupt(int irq, void
> > *phy_dat)
> >  	if (phydev->drv->did_interrupt && !phydev->drv->did_interrupt(phydev))
> >  		return IRQ_NONE;
> > 
> > -	/* reschedule state queue work to run as soon as possible */
> > -	phy_trigger_machine(phydev);
> > +	if (phydev->drv->handle_interrupt) {
> > +		if (phydev->drv->handle_interrupt(phydev))
> > +			goto phy_err;
> 
> There are PHYs which clears the interrupt already by reading the interrupt
> status register. To do something useful in handle_interrupt() I have to read
> the interrupt status register, thus clearing the pending interrupts.
> 
> 
> > +	} else {
> > +		/* reschedule state queue work to run as soon as possible */
> > +		phy_trigger_machine(phydev);
> > +	}
> > 
> >  	if (phy_clear_interrupt(phydev))
> >  		goto phy_err;
> 
> But here the interrupts are cleared again, which means we might loose
> interrupt causes in between.
> 
> I could think of two different fixes:
>  (1) handle_interrupt() has to take care to clear the interrupts and skip
> the phy_clear_interrupt() above.
>  (2) handle_interrupt() might return a special return code which skips the
> phy_clear_interrupt
> 
> TBH, I'd prefer (1) but I don't know if it is allowed to change semantics
> afterwards. (Also, I've found no driver where handle_interrupt() is actually
> used for now?)

I made the argument at the time that phylib should stop being a middle-
layer, but instead let PHY drivers take care of interrupt handling
themselves, just like we do elsewhere in the kernel.  I think your
case just shows that trying to keep the interrupt handling structured
inside phylib and trying to make all PHYs fit is just going to be
painful.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up