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