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

[Sean Anderson]

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