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