Re: [PATCH net-next v5 5/8] net: phylink: Adjust link settings based on rate adaptation

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

On 9/7/22 6:10 AM, Russell King (Oracle) wrote:
> On Tue, Sep 06, 2022 at 12:18:49PM -0400, Sean Anderson wrote:
>> @@ -1015,19 +1086,45 @@ static void phylink_link_up(struct phylink *pl,
>>   			    struct phylink_link_state link_state)
>>   {
>>   	struct net_device *ndev = pl->netdev;
>> +	int speed, duplex;
>> +	bool rx_pause;
>> +
>> +	speed = link_state.speed;
>> +	duplex = link_state.duplex;
>> +	rx_pause = !!(link_state.pause & MLO_PAUSE_RX);
>> +
>> +	switch (link_state.rate_adaptation) {
>> +	case RATE_ADAPT_PAUSE:
>> +		/* The PHY is doing rate adaption from the media rate (in
>> +		 * the link_state) to the interface speed, and will send
>> +		 * pause frames to the MAC to limit its transmission speed.
>> +		 */
>> +		speed = phylink_interface_max_speed(link_state.interface);
>> +		duplex = DUPLEX_FULL;
>> +		rx_pause = true;
>> +		break;
>> +
>> +	case RATE_ADAPT_CRS:
>> +		/* The PHY is doing rate adaption from the media rate (in
>> +		 * the link_state) to the interface speed, and will cause
>> +		 * collisions to the MAC to limit its transmission speed.
>> +		 */
>> +		speed = phylink_interface_max_speed(link_state.interface);
>> +		duplex = DUPLEX_HALF;
>> +		break;
>> +	}
>>   
>>   	pl->cur_interface = link_state.interface;
>> +	if (link_state.rate_adaptation == RATE_ADAPT_PAUSE)
>> +		link_state.pause |= MLO_PAUSE_RX;
> 
> I specifically omitted this from my patch because I don't think we
> should tell the user that "Link is Up - ... - flow control rx" if
> we have rate adaption, but the media link is not using flow control.
> 
> The "Link is Up" message tells the user what was negotiated on the
> media, not what is going on inside their network device, so the
> fact we're using rate adaption which has turned on RX pause on the
> MAC is neither here nor there.

OK, I will leave this out then.

>>   
>>   	if (pl->pcs && pl->pcs->ops->pcs_link_up)
>>   		pl->pcs->ops->pcs_link_up(pl->pcs, pl->cur_link_an_mode,
>> -					 pl->cur_interface,
>> -					 link_state.speed, link_state.duplex);
>> +					  pl->cur_interface, speed, duplex);
> 
> It seems you have one extra unnecessary space here - not sure how
> that occurred as it isn't in my original patch.
> 

This corrects a spacing issue in the existing code.

--Sean