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