Re: [PATCH v2 06/11] net: phylink: Support differing link/interface speed/duplex

[Sean Anderson <sean.anderson@seco.com>]

On 7/20/22 2:43 AM, Russell King (Oracle) wrote:
> 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.

I saw that you and Andrew were not in favor, but I did not get a response to
my defense of this terminology. That said, this is not a terribly large
change to make.

>> @@ -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?

This will be conditional on the rate adaptation in the next patch. I should
have been more clear in the commit message, but this patch is not really useful
on its own, and primarily serves to break up the changes to make things easier
to review.

>>  /* 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?
> 

Because in the next patch the speed/rate could be different if rate adaptation
is enabled. This is not really necessary for now, since fixed state links cannot
specify rate adaptation, but I have tried to be complete.

--Sean