Messages in this thread | | | Date | Wed, 20 Jul 2022 07:43:59 +0100 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH v2 06/11] net: phylink: Support differing link/interface speed/duplex |
| |
On Tue, Jul 19, 2022 at 07:49:56PM -0400, Sean Anderson wrote: > This adds support for cases when the link speed or duplex differs from > the speed or duplex of the phy interface mode. Such cases can occur when > some kind of rate adaptation is occurring. > > The following terms are used within this and the following patches. I > do not believe the meaning of these terms are uncommon or surprising, > but for maximum clarity I would like to be explicit: > > - Phy interface mode: the protocol used to communicate between the MAC > or PCS (if used) and the phy. If no phy is in use, this is the same as > the link mode. Each phy interface mode supported by Linux is a member > of phy_interface_t. > - Link mode: the protocol used to communicate between the local phy (or > PCS) and the remote phy (or PCS) over the physical medium. Each link > mode supported by Linux is a member of ethtool_link_mode_bit_indices. > - Phy interface mode speed: the speed of unidirectional data transfer > over a phy interface mode, including encoding overhead, but excluding > protocol and flow-control overhead. The speed of a phy interface mode > may vary. For example, SGMII may have a speed of 10, 100, or 1000 > Mbit/s. > - Link mode speed: similarly, the speed of unidirectional data transfer > over a physical medium, including overhead, but excluding protocol and > flow-control overhead. The speed of a link mode is usually fixed, but > some exceptional link modes (such as 2BASE-TL) may vary their speed > depending on the medium characteristics. > > Before this patch, phylink assumed that the link mode speed was the same > as the phy interface mode speed. This is typically the case; however, > some phys have the ability to adapt between differing link mode and phy > interface mode speeds. To support these phys, this patch removes this > assumption, and adds a separate variable for link speed. Additionally, > to support rate adaptation, a MAC may need to have a certain duplex > (such as half or full). This may be different from the link's duplex. To > keep track of this distunction, this patch adds another variable to > track link duplex.
I thought we had decided that using the term "link" in these new members was a bad idea.
> @@ -925,12 +944,16 @@ 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; > - if (state->an_enabled) { > + if (state->an_enabled) { > + state->link_speed = SPEED_UNKNOWN; > + state->link_duplex = DUPLEX_UNKNOWN; > state->speed = SPEED_UNKNOWN; > state->duplex = DUPLEX_UNKNOWN; > state->pause = MLO_PAUSE_NONE; > } else { > - state->speed = pl->link_config.speed; > + state->link_speed = pl->link_config.link_speed; > + state->link_duplex = pl->link_config.link_duplex; > + state->speed = pl->link_config.speed; > state->duplex = pl->link_config.duplex; > state->pause = pl->link_config.pause; > } > @@ -944,6 +967,9 @@ static void phylink_mac_pcs_get_state(struct phylink *pl, > pl->mac_ops->mac_pcs_get_state(pl->config, state); > else > state->link = 0; > + > + state->link_speed = state->speed; > + state->link_duplex = state->duplex;
Why do you need to set link_speed and link_duple above if they're always copied over here?
> /* The fixed state is... fixed except for the link state, > @@ -953,10 +979,17 @@ static void phylink_get_fixed_state(struct phylink *pl, > struct phylink_link_state *state) > { > *state = pl->link_config; > - if (pl->config->get_fixed_state) > + if (pl->config->get_fixed_state) { > pl->config->get_fixed_state(pl->config, state); > - else if (pl->link_gpio) > + /* FIXME: these should not be updated, but > + * bcm_sf2_sw_fixed_state does it anyway > + */ > + state->link_speed = state->speed; > + state->link_duplex = state->duplex; > + phylink_state_fill_speed_duplex(state);
This looks weird. Why copy state->xxx to state->link_xxx and then copy them back to state->xxx in a helper function?
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
| |