On 16.12.2018 18:55, Florian Fainelli wrote: > Le 12/16/18 à 9:16 AM, Heiner Kallweit a écrit : >> On 16.12.2018 18:02, Florian Fainelli wrote: >>> Le 12/16/18 à 7:52 AM, Heiner Kallweit a écrit : >>>> So far phy_error() silently stops the PHY state machine. If the network >>>> driver doesn't inform about a MDIO error then the user may wonder why >>>> his network is down. So let's inform the user. >>>> >>>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com> >>> >>> Reviewed-by: Florian Fainelli <f.faine...@gmail.com> >>> >>> Kind of similar to the netdev watchdog on a TX timeout, I wonder if we >>> should not just put a WARN() here to get a complete stack trace to help >>> debug those events? >>> >> AFAICS phy_error() is always (?) caused by a MDIO access error. >> Does it help us to know in which code path the MDIO error occurred? > > I would say yes because if you have things like nested MDIO bus accesses > (as can happen with Ethernet switches/DSA) or if runtime PM got in the > way, you would be able to know that. > Good, convinced. So I'll provide a v2.
>> >> And maybe the stack trace in case of a tx timeout isn't the best >> example, at least from my personal experience. I dealt with such cases >> and the stack trace never helped. The root cause always was in a totally >> different place (wrong chip tx configuration etc.) >> But maybe there are other cases where it's useful. > > That is entirely true, debugging TX timeouts is definitively no fun and > the watchdog does not help at all, other than making users scream and > shout :) > >> >>>> --- >>>> drivers/net/phy/phy.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c >>>> index 890ae1d73..a898fa411 100644 >>>> --- a/drivers/net/phy/phy.c >>>> +++ b/drivers/net/phy/phy.c >>>> @@ -739,6 +739,8 @@ static void phy_error(struct phy_device *phydev) >>>> phydev->state = PHY_HALTED; >>>> mutex_unlock(&phydev->lock); >>>> >>>> + phydev_err(phydev, "stopping PHY state machine due to error\n"); >>>> + >>>> phy_trigger_machine(phydev); >>>> } >>>> >>>> >>> >>> >> > >
