Messages in this thread | | | Subject | Re: [PATCH v2 07/11] net: phylink: Adjust link settings based on rate adaptation | From | Sean Anderson <> | Date | Thu, 21 Jul 2022 12:24:50 -0400 |
| |
On 7/20/22 2:50 AM, Russell King (Oracle) wrote: > On Tue, Jul 19, 2022 at 07:49:57PM -0400, Sean Anderson wrote: >> If the phy is configured to use pause-based rate adaptation, ensure that >> the link is full duplex with pause frame reception enabled. As >> suggested, if pause-based rate adaptation is enabled by the phy, then >> pause reception is unconditionally enabled. >> >> The interface duplex is determined based on the rate adaptation type. >> When rate adaptation is enabled, so is the speed. We assume the maximum >> interface speed is used. This is only relevant for MLO_AN_PHY. For >> MLO_AN_INBAND, the MAC/PCS's view of the interface speed will be used. >> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com> >> --- >> >> Changes in v2: >> - Use the phy's rate adaptation setting to determine whether to use its >> link speed/duplex or the MAC's speed/duplex with MLO_AN_INBAND. >> - Always use the rate adaptation setting to determine the interface >> speed/duplex (instead of sometimes using the interface mode). >> >> drivers/net/phy/phylink.c | 126 ++++++++++++++++++++++++++++++++++---- >> include/linux/phylink.h | 1 + >> 2 files changed, 114 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c >> index da0623d94a64..619ef553476f 100644 >> --- a/drivers/net/phy/phylink.c >> +++ b/drivers/net/phy/phylink.c >> @@ -160,16 +160,93 @@ static const char *phylink_an_mode_str(unsigned int mode) >> * @state: A link state >> * >> * Update the .speed and .duplex members of @state. We can determine them based >> - * on the .link_speed and .link_duplex. This function should be called whenever >> - * .link_speed and .link_duplex are updated. For example, userspace deals with >> - * link speed and duplex, and not the interface speed and duplex. Similarly, >> - * phys deal with link speed and duplex and only implicitly the interface speed >> - * and duplex. >> + * on the .link_speed, .link_duplex, .interface, and .rate_adaptation. This >> + * function should be called whenever .link_speed and .link_duplex are updated. >> + * For example, userspace deals with link speed and duplex, and not the >> + * interface speed and duplex. Similarly, phys deal with link speed and duplex >> + * and only implicitly the interface speed and duplex. >> */ >> static void phylink_state_fill_speed_duplex(struct phylink_link_state *state) >> { >> - state->speed = state->link_speed; >> - state->duplex = state->link_duplex; >> + switch (state->rate_adaptation) { >> + case RATE_ADAPT_NONE: >> + state->speed = state->link_speed; >> + state->duplex = state->link_duplex; >> + return; >> + case RATE_ADAPT_PAUSE: >> + state->duplex = DUPLEX_FULL; >> + break; >> + case RATE_ADAPT_CRS: >> + state->duplex = DUPLEX_HALF; >> + break; >> + case RATE_ADAPT_OPEN_LOOP: >> + state->duplex = state->link_duplex; >> + break; >> + } >> + >> + /* Use the max speed of the interface */ >> + switch (state->interface) { >> + case PHY_INTERFACE_MODE_100BASEX: >> + case PHY_INTERFACE_MODE_REVRMII: >> + case PHY_INTERFACE_MODE_RMII: >> + case PHY_INTERFACE_MODE_SMII: >> + case PHY_INTERFACE_MODE_REVMII: >> + case PHY_INTERFACE_MODE_MII: >> + state->speed = SPEED_100; >> + return; >> + >> + case PHY_INTERFACE_MODE_TBI: >> + case PHY_INTERFACE_MODE_MOCA: >> + case PHY_INTERFACE_MODE_RTBI: >> + case PHY_INTERFACE_MODE_1000BASEX: >> + case PHY_INTERFACE_MODE_1000BASEKX: >> + case PHY_INTERFACE_MODE_TRGMII: >> + case PHY_INTERFACE_MODE_RGMII_TXID: >> + case PHY_INTERFACE_MODE_RGMII_RXID: >> + case PHY_INTERFACE_MODE_RGMII_ID: >> + case PHY_INTERFACE_MODE_RGMII: >> + case PHY_INTERFACE_MODE_QSGMII: >> + case PHY_INTERFACE_MODE_SGMII: >> + case PHY_INTERFACE_MODE_GMII: >> + state->speed = SPEED_1000; >> + return; >> + >> + case PHY_INTERFACE_MODE_2500BASEX: >> + state->speed = SPEED_2500; >> + return; >> + >> + case PHY_INTERFACE_MODE_5GBASER: >> + state->speed = SPEED_5000; >> + return; >> + >> + case PHY_INTERFACE_MODE_XGMII: >> + case PHY_INTERFACE_MODE_RXAUI: >> + case PHY_INTERFACE_MODE_XAUI: >> + case PHY_INTERFACE_MODE_10GBASER: >> + case PHY_INTERFACE_MODE_10GKR: >> + case PHY_INTERFACE_MODE_USXGMII: >> + state->speed = SPEED_10000; >> + return; >> + >> + case PHY_INTERFACE_MODE_25GBASER: >> + state->speed = SPEED_25000; >> + return; >> + >> + case PHY_INTERFACE_MODE_XLGMII: >> + state->speed = SPEED_40000; >> + return; >> + >> + case PHY_INTERFACE_MODE_INTERNAL: >> + state->speed = state->link_speed; >> + return; >> + >> + case PHY_INTERFACE_MODE_NA: >> + case PHY_INTERFACE_MODE_MAX: >> + state->speed = SPEED_UNKNOWN; >> + return; >> + } >> + >> + WARN_ON(1); >> } >> >> /** >> @@ -803,11 +880,12 @@ static void phylink_mac_config(struct phylink *pl, >> const struct phylink_link_state *state) >> { >> phylink_dbg(pl, >> - "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n", >> + "%s: mode=%s/%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n", >> __func__, phylink_an_mode_str(pl->cur_link_an_mode), >> phy_modes(state->interface), >> phy_speed_to_str(state->speed), >> phy_duplex_to_str(state->duplex), >> + phy_rate_adaptation_to_str(state->rate_adaptation), >> __ETHTOOL_LINK_MODE_MASK_NBITS, state->advertising, >> state->pause, state->link, state->an_enabled); >> >> @@ -944,6 +1022,7 @@ static void phylink_mac_pcs_get_state(struct phylink *pl, >> linkmode_zero(state->lp_advertising); >> state->interface = pl->link_config.interface; >> state->an_enabled = pl->link_config.an_enabled; >> + state->rate_adaptation = pl->link_config.rate_adaptation; >> if (state->an_enabled) { >> state->link_speed = SPEED_UNKNOWN; >> state->link_duplex = DUPLEX_UNKNOWN; >> @@ -968,8 +1047,10 @@ static void phylink_mac_pcs_get_state(struct phylink *pl, >> else >> state->link = 0; >> >> - state->link_speed = state->speed; >> - state->link_duplex = state->duplex; >> + if (state->rate_adaptation == RATE_ADAPT_NONE) { >> + state->link_speed = state->speed; >> + state->link_duplex = state->duplex; >> + } > > So we need to have every PCS driver be udpated to fill in link_speed > and link_duplex if rate_adaption != none.
The PCS doesn't know what the link speed/duplex is. If rate adaptation is enabled, then the PCS only knows what the interface speed/duplex is.
> There's got to be a better way - maybe what I suggested in the last > round of only doing the rate adaption thing in the link_up() functions, > since that seems to be the only real difference. > > I'm not even sure we need to do that - in the "open loop" case, we > need to be passing the media speed to the MAC driver with the knowledge > that it should be increasing the IPG. > > So, I'm thinking we don't want any of these changes, what we instead > should be doing is passing the media speed/duplex and the interface > speed/duplex to the PCS and MAC.
> We can do that by storing the PHY rate adaption state, and processing > that in phylink_link_up().
This approach sounds better. You patch below looks good. I'll test it and use it for v3.
--Sean
| |