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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YxhuMjZsBb7wCBFy@shell.armlinux.org.uk>
Date:   Wed, 7 Sep 2022 11:10:58 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Sean Anderson <sean.anderson@...o.com>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Alexandru Marginean <alexandru.marginean@....com>,
        linux-kernel@...r.kernel.org,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net-next v5 5/8] net: phylink: Adjust link settings based
 on rate adaptation

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.

>  
>  	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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!