* [PATCH net-next] net: phy: improve pause mode reporting in phy_print_status @ 2019-05-05 17:03 Heiner Kallweit 2019-05-05 17:10 ` Florian Fainelli 2019-05-07 19:41 ` David Miller 0 siblings, 2 replies; 8+ messages in thread From: Heiner Kallweit @ 2019-05-05 17:03 UTC (permalink / raw) To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev 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. Signed-off-by: Heiner Kallweit <hkallweit1@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"); } -- 2.21.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] net: phy: improve pause mode reporting in phy_print_status 2019-05-05 17:03 [PATCH net-next] net: phy: improve pause mode reporting in phy_print_status Heiner Kallweit @ 2019-05-05 17:10 ` Florian Fainelli 2019-05-05 17:31 ` Heiner Kallweit 2019-05-07 19:41 ` David Miller 1 sibling, 1 reply; 8+ messages in thread From: Florian Fainelli @ 2019-05-05 17:10 UTC (permalink / raw) To: Heiner Kallweit, Andrew Lunn, David Miller; +Cc: netdev 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 Thanks! > > Signed-off-by: Heiner Kallweit <hkallweit1@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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] net: phy: improve pause mode reporting in phy_print_status 2019-05-05 17:10 ` Florian Fainelli @ 2019-05-05 17:31 ` Heiner Kallweit 2019-05-05 18:46 ` Florian Fainelli 0 siblings, 1 reply; 8+ messages in thread From: Heiner Kallweit @ 2019-05-05 17:31 UTC (permalink / raw) To: Florian Fainelli, Andrew Lunn, David Miller; +Cc: netdev 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. 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? 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. > > Thanks! > >> >> Signed-off-by: Heiner Kallweit <hkallweit1@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"); >> } >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] net: phy: improve pause mode reporting in phy_print_status 2019-05-05 17:31 ` Heiner Kallweit @ 2019-05-05 18:46 ` Florian Fainelli 2019-05-05 19:06 ` Heiner Kallweit 0 siblings, 1 reply; 8+ messages in thread From: Florian Fainelli @ 2019-05-05 18:46 UTC (permalink / raw) To: Heiner Kallweit, Andrew Lunn, David Miller; +Cc: netdev 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 <hkallweit1@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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] net: phy: improve pause mode reporting in phy_print_status 2019-05-05 18:46 ` Florian Fainelli @ 2019-05-05 19:06 ` Heiner Kallweit 2019-05-05 19:12 ` Florian Fainelli 0 siblings, 1 reply; 8+ messages in thread From: Heiner Kallweit @ 2019-05-05 19:06 UTC (permalink / raw) To: Florian Fainelli, Andrew Lunn, David Miller; +Cc: netdev 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 <hkallweit1@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"); >>>> } >>>> >>> >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] net: phy: improve pause mode reporting in phy_print_status 2019-05-05 19:06 ` Heiner Kallweit @ 2019-05-05 19:12 ` Florian Fainelli 2019-05-05 19:19 ` Heiner Kallweit 0 siblings, 1 reply; 8+ messages in thread From: Florian Fainelli @ 2019-05-05 19:12 UTC (permalink / raw) To: Heiner Kallweit, Andrew Lunn, David Miller; +Cc: netdev 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] net: phy: improve pause mode reporting in phy_print_status 2019-05-05 19:12 ` Florian Fainelli @ 2019-05-05 19:19 ` Heiner Kallweit 0 siblings, 0 replies; 8+ messages in thread From: Heiner Kallweit @ 2019-05-05 19:19 UTC (permalink / raw) To: Florian Fainelli, Andrew Lunn, David Miller; +Cc: netdev 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. > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] net: phy: improve pause mode reporting in phy_print_status 2019-05-05 17:03 [PATCH net-next] net: phy: improve pause mode reporting in phy_print_status Heiner Kallweit 2019-05-05 17:10 ` Florian Fainelli @ 2019-05-07 19:41 ` David Miller 1 sibling, 0 replies; 8+ messages in thread From: David Miller @ 2019-05-07 19:41 UTC (permalink / raw) To: hkallweit1; +Cc: andrew, f.fainelli, netdev From: Heiner Kallweit <hkallweit1@gmail.com> Date: Sun, 5 May 2019 19:03:51 +0200 > 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. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> It looks like Florian and Heiner are sort-of almost in agreement here so I'll apply this and we can make whatever minor tweaks are still necessary. Thanks. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-05-07 19:41 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-05 17:03 [PATCH net-next] net: phy: improve pause mode reporting in phy_print_status Heiner Kallweit 2019-05-05 17:10 ` Florian Fainelli 2019-05-05 17:31 ` Heiner Kallweit 2019-05-05 18:46 ` Florian Fainelli 2019-05-05 19:06 ` Heiner Kallweit 2019-05-05 19:12 ` Florian Fainelli 2019-05-05 19:19 ` Heiner Kallweit 2019-05-07 19:41 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).