lkml.org 
[lkml]   [2022]   [Jul]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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!

\
 
 \ /
  Last update: 2022-07-20 08:51    [W:0.147 / U:0.700 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site