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: <f2c15d18-23c0-463e-775e-f99e42cd4a69@seco.com>
Date:   Wed, 7 Sep 2022 13:01:49 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
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 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