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

[Florian Fainelli <f.fainelli@gmail.com>]

On 5/5/2019 12:06 PM, Heiner Kallweit wrote:
> 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.

My point was really that I would rather see the resolved pause status,
which takes into account the link partner advertised, locally advertised
pause settings and local policy (enabled/disabled/auto-negotiated),
rather than the current/locally advertised pause settings which are just
one view of the link. Your patch is fine in that it properly decouples
the symetric/assymetric nature of the settings though.

> 
>>> 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().

Given that pause can be changed from ethtool -A, would not that just be
a partial view of pause at the time the MAC and PHY get bound together?

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

Your mileage may vary of course, but if the remote side either does not
support pause or has receive pause frame support disabled, then these
frames should be ignored.
-- 
Florian