Date:   Sun, 5 May 2019 21:19:00 +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 21:12, Florian Fainelli wrote:
> 
> 
> 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.
> 
Not that we misunderstand each other:
phydev->pause and phydev->asym_pause represent the link partner
capabilities, see phy_resolve_aneg_linkmode(). Therefore my patch should
exactly do what you describe: resolve the pause status based on what
local and remote side advertise.

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

Powered by blists - more mailing lists