Date:   Sun, 5 May 2019 21:06:47 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        David Miller <davem@...emloft.net>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] net: phy: improve pause mode reporting in
 phy_print_status

On 05.05.2019 20:46, Florian Fainelli wrote:
> 
> 
> On 5/5/2019 10:31 AM, Heiner Kallweit wrote:
>> On 05.05.2019 19:10, Florian Fainelli wrote:
>>>
>>>
>>> On 5/5/2019 10:03 AM, Heiner Kallweit wrote:
>>>> So far we report symmetric pause only, and we don't consider the local
>>>> pause capabilities. Let's properly consider local and remote
>>>> capabilities, and report also asymmetric pause.
>>>
>>> I would go one step further which is to print what is the link state of
>>> RX/TX pause, so something like:
>>>
>>> - local RX/TX pause advertisement
>>> - link partnr RX/TX pause advertisement
>>> - RX/TX being enabled for the link (auto-negotiated or manual)
>>>
>>> this sort of duplicates what ethtool offers already but arguably so does
>>> printing the link state so this would not be that big of a stretch.
>>>
>>> I would make the print be something like:
>>>
>>> Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
>>> pause: auto-negotiated
>>> Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
>>> pause: forced off
>>> Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
>>> pause: forced on
>>>
>> For speed and duplex we don't print the capabilities of both sides
>> but the negotiation result. Therefore I think it's more plausible
>> to do the same for pause.
> 
> Pause is different though, if the link speed does not match, there is no
> link, if the duplex do not match you may establish a link but there will
> be a duplex mismatch which will cause all sorts of issues. Pause is not
> an essential link parameter and is more of an optimization.
> 
Right, still I think this is too much and only partially relevant
information for the user. And if e.g. the remote side doesn't support
pause, then it's irrelevant what we support. I think the user is
(if at all) interested in the information which pause mode is effectively
used.

>> IMO the intention of phy_print_status() is to print what is
>> effectively used. If a user is interested in the detailed capabilities
>> of both sides he can use ethtool, as mentioned by you.
>>
>> In fixed mode we currently report pause "off" always.
>>
>> Maybe, before we go further, one question for my understanding:
>> If the link partner doesn't support pause, who tells the MAC how that
>> it must not send pause frames? Is the network driver supposed to
>> do this in the adjust_link callback?
> 
> If the link partner does not support pause, they are not advertised by
> the link partner and you can read that from the LPA and the resolution
> of the local pause and link partner pause settings should come back as
> "not possible" (there may be caveats with symmetric vs. asymmetric pause
> support).
> 
> PHYLINK is a good example of how pause should be reported towards the MAC.
> 
Thanks. So I think the usual MAC driver would have to check pause support
in the handler passed as argument to phy_connect_direct().

>>
>> In the Realtek network chip datasheet I found a vague comment that
>> the MAC checks the aneg result of the internal PHY to decide
>> whether send pause frames or not.
> 
> That would mean that the MAC behaves in a mode where it defaults to
> pause frame being auto-negotiated, which is something that some Ethernet
> MAC drivers default to as well. As long as you can disable pause when
> the user requests it, that should be fine.
> 
At least for the Realtek chips there is no documented way to disable pause.
If the remote side doesn't support pause, what happens if a pause frame is
sent? Is it just ignored or can we expect some sort of issue?

>>
>>>
>>> Thanks!
>>>
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
>>>> ---
>>>>  drivers/net/phy/phy.c | 28 +++++++++++++++++++++++++++-
>>>>  1 file changed, 27 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>>>> index 1a146c5c5..e88854292 100644
>>>> --- a/drivers/net/phy/phy.c
>>>> +++ b/drivers/net/phy/phy.c
>>>> @@ -60,6 +60,32 @@ static void phy_link_down(struct phy_device *phydev, bool do_carrier)
>>>>  	phy_led_trigger_change_speed(phydev);
>>>>  }
>>>>  
>>>> +static const char *phy_pause_str(struct phy_device *phydev)
>>>> +{
>>>> +	bool local_pause, local_asym_pause;
>>>> +
>>>> +	if (phydev->autoneg == AUTONEG_DISABLE)
>>>> +		goto no_pause;
>>>> +
>>>> +	local_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
>>>> +					phydev->advertising);
>>>> +	local_asym_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
>>>> +					     phydev->advertising);
>>>> +
>>>> +	if (local_pause && phydev->pause)
>>>> +		return "rx/tx";
>>>> +
>>>> +	if (local_asym_pause && phydev->asym_pause) {
>>>> +		if (local_pause)
>>>> +			return "rx";
>>>> +		if (phydev->pause)
>>>> +			return "tx";
>>>> +	}
>>>> +
>>>> +no_pause:
>>>> +	return "off";
>>>> +}
>>>> +
>>>>  /**
>>>>   * phy_print_status - Convenience function to print out the current phy status
>>>>   * @phydev: the phy_device struct
>>>> @@ -71,7 +97,7 @@ void phy_print_status(struct phy_device *phydev)
>>>>  			"Link is Up - %s/%s - flow control %s\n",
>>>>  			phy_speed_to_str(phydev->speed),
>>>>  			phy_duplex_to_str(phydev->duplex),
>>>> -			phydev->pause ? "rx/tx" : "off");
>>>> +			phy_pause_str(phydev));
>>>>  	} else	{
>>>>  		netdev_info(phydev->attached_dev, "Link is Down\n");
>>>>  	}
>>>>
>>>
>>
> 

Powered by blists - more mailing lists