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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 17.12.2019 13:53, Russell King wrote:
> 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>
> ---
> There is another related problem in this area. If an error is detected
> while the PHY is running, phy_error() moves to PHY_HALTED state. If we
> try to take the network device down, then:
> 
> void phy_stop(struct phy_device *phydev)
> {
>         if (!phy_is_started(phydev)) {
>                 WARN(1, "called from state %s\n",
>                      phy_state_to_str(phydev->state));
>                 return;
>         }
> 
> triggers, and we never do any of the phy_stop() cleanup. I'm not sure
> what the best way to solve this is - introducing a PHY_ERROR state may
> be a solution, but I think we want some phy_is_started() sites to
> return true for it and others to return false.
> 
> Heiner - you introduced the above warning, could you look at improving
> this case so we don't print a warning and taint the kernel when taking
> a network device down after phy_error() please?
> 
I think we need both types of information:
- the affected PHY device
- the stack trace to see where the issue was triggered

In general it's not fully clear yet what's the appropriate reaction
after a PHY error. Few reasons for PHY errors I see:
- MDIO bus may not be accessible, e.g. because parent device is in
  power-down mode
- Frequently polling is used to determine end of a MDIO operation.
  If timeout for polling is too low, then we may end up with an
  -ETIMEDOUT.

In case of singular timeouts they may be acceptable or not.
- If we miss a single PHY status poll, then this may be acceptable.
- But if e.g. a relevant setting in config_init fails, then this
  may not be acceptable.

The current behavior has been existing for the last 15 years,
and I'm just aware of one issue with PHY errors. The case I've
seen was triggered by timeouts, and adjusting the timeouts
fixed it: c3b084c24c8a (net: fec: Adjust ENET MDIO timeouts)

> Thanks.
> 
>  drivers/net/phy/phy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 49300fb59757..06fbca959383 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -663,7 +663,7 @@ void phy_stop_machine(struct phy_device *phydev)
>   */
>  static void phy_error(struct phy_device *phydev)
>  {
> -	WARN_ON(1);
> +	phydev_err(phydev, "Error detected, halting PHY\n");
>  
>  	mutex_lock(&phydev->lock);
>  	phydev->state = PHY_HALTED;
>