[PATCH net-next] net: phy: print error message in phy_error

[PATCH net-next] net: phy: print error message in phy_error

[Heiner Kallweit]

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 <hkallweit1@gmail.com>
---
 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);
 }
 
-- 
2.20.0

Re: [PATCH net-next] net: phy: print error message in phy_error

[Andrew Lunn]

On Sun, Dec 16, 2018 at 04:52:21PM +0100, Heiner Kallweit wrote:
> 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 <hkallweit1@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next] net: phy: print error message in phy_error

[Florian Fainelli]

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 <hkallweit1@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@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?

> ---
>  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);
>  }
>  
> 


-- 
Florian

Re: [PATCH net-next] net: phy: print error message in phy_error

[Heiner Kallweit]

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 <hkallweit1@gmail.com>
> 
> Reviewed-by: Florian Fainelli <f.fainelli@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?

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.

>> ---
>>  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);
>>  }
>>  
>>
> 
> 

Re: [PATCH net-next] net: phy: print error message in phy_error

[Florian Fainelli]

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 <hkallweit1@gmail.com>
>>
>> Reviewed-by: Florian Fainelli <f.fainelli@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.

> 
> 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);
>>>  }
>>>  
>>>
>>
>>
> 


-- 
Florian

Re: [PATCH net-next] net: phy: print error message in phy_error

[Heiner Kallweit]

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 <hkallweit1@gmail.com>
>>>
>>> Reviewed-by: Florian Fainelli <f.fainelli@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);
>>>>  }
>>>>  
>>>>
>>>
>>>
>>
> 
> 

Re: [PATCH net-next] net: phy: print error message in phy_error

[Heiner Kallweit]

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 <hkallweit1@gmail.com>
> 
> Reviewed-by: Florian Fainelli <f.fainelli@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?
> 
But it's good that you mention the WARN(). This is the better option in
the other patch I submitted today. I'll prepare an update to this patch.

>> ---
>>  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);
>>  }
>>  
>>
> 
>