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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191220091814.GA24174@lunn.ch>
Date:   Fri, 20 Dec 2019 10:18:14 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     David Miller <davem@...emloft.net>, f.fainelli@...il.com,
        hkallweit1@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH net] net: phy: make phy_error() report which PHY has
 failed

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@...linux.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@...linux.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