Re: [PATCH net] net: phy: make phy_error() report which PHY has failed

[Andrew Lunn <andrew@lunn.ch>]

On Thu, Dec 19, 2019 at 09:45:37PM +0000, Russell King - ARM Linux admin wrote:
> On Thu, Dec 19, 2019 at 12:50:10PM -0800, David Miller wrote:
> > From: Russell King <rmk+kernel@armlinux.org.uk>
> > Date: Tue, 17 Dec 2019 12:53:05 +0000
> > 
> > > phy_error() is called from phy_interrupt() or phy_state_machine(), and
> > > uses WARN_ON() to print a backtrace. The backtrace is not useful when
> > > reporting a PHY error.
> > > 
> > > However, a system may contain multiple ethernet PHYs, and phy_error()
> > > gives no clue which one caused the problem.
> > > 
> > > Replace WARN_ON() with a call to phydev_err() so that we can see which
> > > PHY had an error, and also inform the user that we are halting the PHY.
> > > 
> > > Fixes: fa7b28c11bbf ("net: phy: print stack trace in phy_error")
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > 
> > I think I agree with Heiner that it is valuable to know whether the
> > error occurred from the interrupt handler or the state machine (and
> > if the state machine, where that got called from).
> 
> Would you accept, then, passing a string to indicate where phy_error()
> was called from, which would do the same job without tainting the
> kernel for something that becomes a _normal_ event when removing a
> SFP?

I'm actually wondering what purpose phy_error has, and if the SFP is
using it correctly.

It is currently used when the phy driver interrupts handling returns
an error, or phy_start_aneg returns an error.

In general, with a traditional MDIO bus, you cannot tell when the
device has disappeared. Writes just disappear into the bit bucket, and
reads return 0xffff. If an error is returned by the MDIO bus driver,
it is because the mechanisms surrounding the bus have returned an
error. A needed clock has been turned off, the system is too busy to
service the interrupt in a timely manor etc. An error indicates a bug
of some sort, bad power management, or timers too short.

SFPs, with their MDIO bus tunnelled over i2c are however a little
different. i2c does allow you to know the device has gone, and return
-EIO.

We might want to consider the question, should the MDIO over i2c code
actually be tossing this EIO error, and returning 0xffff on read, no
error on write? It makes the emulation more true? Only return an error
for power management or timeouts, similar to traditional MDIO? Maybe
print a rate limited warning when dropping an EIO error?

Phylink is going to notice the PHY has disappeared soon anyway, so is
there any value in returning an error when the module has been
unplugged?

      Andrew