| Message ID | E1tXGei-000EtL-Fn@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | net: phylink: fix PCS without autoneg | expand |
On Mon, Jan 13, 2025 at 09:22:44AM +0000, Russell King (Oracle) wrote: > When decoding clause 22 state, if in-band is disabled and using either > 1000base-X or 2500base-X, rather than reporting link-down, we know the > speed, and we only support full duplex. Pause modes taken from XPCS. > > This fixes a problem reported by Eric Woudstra. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > drivers/net/phy/phylink.c | 29 +++++++++++++++++++---------- > 1 file changed, 19 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index b79f975bc164..ff0efb52189f 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -3882,27 +3882,36 @@ void phylink_mii_c22_pcs_decode_state(struct phylink_link_state *state, > if (!state->link) > return; > > - /* If in-band is disabled, then the advertisement data is not > - * meaningful. > - */ > - if (neg_mode != PHYLINK_PCS_NEG_INBAND_ENABLED) > - return; > - > switch (state->interface) { > case PHY_INTERFACE_MODE_1000BASEX: > - phylink_decode_c37_word(state, lpa, SPEED_1000); > + if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) { > + phylink_decode_c37_word(state, lpa, SPEED_1000); > + } else { > + state->speed = SPEED_1000; > + state->duplex = DUPLEX_FULL; > + state->pause |= MLO_PAUSE_TX | MLO_PAUSE_RX; > + } > break; > > case PHY_INTERFACE_MODE_2500BASEX: > - phylink_decode_c37_word(state, lpa, SPEED_2500); > + if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) { > + phylink_decode_c37_word(state, lpa, SPEED_2500); > + } else { > + state->speed = SPEED_2500; > + state->duplex = DUPLEX_FULL; > + state->pause |= MLO_PAUSE_TX | MLO_PAUSE_RX; > + } > break; Are the "else" branches necessary, given the "else" branch from phylink_mac_pcs_get_state? static void phylink_mac_pcs_get_state(struct phylink *pl, struct phylink_link_state *state) { struct phylink_pcs *pcs; bool autoneg; linkmode_copy(state->advertising, pl->link_config.advertising); linkmode_zero(state->lp_advertising); state->interface = pl->link_config.interface; state->rate_matching = pl->link_config.rate_matching; state->an_complete = 0; state->link = 1; pcs = pl->pcs; if (!pcs || pcs->neg_mode) autoneg = pl->pcs_neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED; else autoneg = linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, state->advertising); if (autoneg) { state->speed = SPEED_UNKNOWN; state->duplex = DUPLEX_UNKNOWN; state->pause = MLO_PAUSE_NONE; } else { | state->speed = pl->link_config.speed; | state->duplex = pl->link_config.duplex; | this state->pause = pl->link_config.pause; | } | if (pcs) pcs->ops->pcs_get_state(pcs, pl->pcs_neg_mode, state); else state->link = 0; }
On Tue, Jan 14, 2025 at 02:57:39PM +0200, Vladimir Oltean wrote: > On Mon, Jan 13, 2025 at 09:22:44AM +0000, Russell King (Oracle) wrote: > > When decoding clause 22 state, if in-band is disabled and using either > > 1000base-X or 2500base-X, rather than reporting link-down, we know the > > speed, and we only support full duplex. Pause modes taken from XPCS. > > > > This fixes a problem reported by Eric Woudstra. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > --- > > drivers/net/phy/phylink.c | 29 +++++++++++++++++++---------- > > 1 file changed, 19 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > > index b79f975bc164..ff0efb52189f 100644 > > --- a/drivers/net/phy/phylink.c > > +++ b/drivers/net/phy/phylink.c > > @@ -3882,27 +3882,36 @@ void phylink_mii_c22_pcs_decode_state(struct phylink_link_state *state, > > if (!state->link) > > return; > > > > - /* If in-band is disabled, then the advertisement data is not > > - * meaningful. > > - */ > > - if (neg_mode != PHYLINK_PCS_NEG_INBAND_ENABLED) > > - return; > > - > > switch (state->interface) { > > case PHY_INTERFACE_MODE_1000BASEX: > > - phylink_decode_c37_word(state, lpa, SPEED_1000); > > + if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) { > > + phylink_decode_c37_word(state, lpa, SPEED_1000); > > + } else { > > + state->speed = SPEED_1000; > > + state->duplex = DUPLEX_FULL; > > + state->pause |= MLO_PAUSE_TX | MLO_PAUSE_RX; > > + } > > break; > > > > case PHY_INTERFACE_MODE_2500BASEX: > > - phylink_decode_c37_word(state, lpa, SPEED_2500); > > + if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) { > > + phylink_decode_c37_word(state, lpa, SPEED_2500); > > + } else { > > + state->speed = SPEED_2500; > > + state->duplex = DUPLEX_FULL; > > + state->pause |= MLO_PAUSE_TX | MLO_PAUSE_RX; > > + } > > break; > > Are the "else" branches necessary, given the "else" branch from > phylink_mac_pcs_get_state? > > static void phylink_mac_pcs_get_state(struct phylink *pl, > struct phylink_link_state *state) > { > struct phylink_pcs *pcs; > bool autoneg; > > linkmode_copy(state->advertising, pl->link_config.advertising); > linkmode_zero(state->lp_advertising); > state->interface = pl->link_config.interface; > state->rate_matching = pl->link_config.rate_matching; > state->an_complete = 0; > state->link = 1; > > pcs = pl->pcs; > if (!pcs || pcs->neg_mode) > autoneg = pl->pcs_neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED; > else > autoneg = linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, > state->advertising); > > if (autoneg) { > state->speed = SPEED_UNKNOWN; > state->duplex = DUPLEX_UNKNOWN; > state->pause = MLO_PAUSE_NONE; > } else { | > state->speed = pl->link_config.speed; | > state->duplex = pl->link_config.duplex; | this > state->pause = pl->link_config.pause; | > } | This is fine for cases where ethtool is used to turn autoneg off, but not if we're in in-band mode and the PCS/PHY have decided to have autoneg off - in that case, nothing sets pl->link_config.* to anything sensible.
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index b79f975bc164..ff0efb52189f 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -3882,27 +3882,36 @@ void phylink_mii_c22_pcs_decode_state(struct phylink_link_state *state, if (!state->link) return; - /* If in-band is disabled, then the advertisement data is not - * meaningful. - */ - if (neg_mode != PHYLINK_PCS_NEG_INBAND_ENABLED) - return; - switch (state->interface) { case PHY_INTERFACE_MODE_1000BASEX: - phylink_decode_c37_word(state, lpa, SPEED_1000); + if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) { + phylink_decode_c37_word(state, lpa, SPEED_1000); + } else { + state->speed = SPEED_1000; + state->duplex = DUPLEX_FULL; + state->pause |= MLO_PAUSE_TX | MLO_PAUSE_RX; + } break; case PHY_INTERFACE_MODE_2500BASEX: - phylink_decode_c37_word(state, lpa, SPEED_2500); + if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) { + phylink_decode_c37_word(state, lpa, SPEED_2500); + } else { + state->speed = SPEED_2500; + state->duplex = DUPLEX_FULL; + state->pause |= MLO_PAUSE_TX | MLO_PAUSE_RX; + } break; case PHY_INTERFACE_MODE_SGMII: case PHY_INTERFACE_MODE_QSGMII: - phylink_decode_sgmii_word(state, lpa); + if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) + phylink_decode_sgmii_word(state, lpa); break; + case PHY_INTERFACE_MODE_QUSGMII: - phylink_decode_usgmii_word(state, lpa); + if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) + phylink_decode_usgmii_word(state, lpa); break; default:
When decoding clause 22 state, if in-band is disabled and using either 1000base-X or 2500base-X, rather than reporting link-down, we know the speed, and we only support full duplex. Pause modes taken from XPCS. This fixes a problem reported by Eric Woudstra. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/phy/phylink.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-)