Re: [PATCH net-next] net: phy: improve pause mode reporting in phy_print_status

[Heiner Kallweit <hkallweit1@gmail.com>]

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.
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?

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.

> 
> Thanks!
> 
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.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");
>>  	}
>>
>