netdev - Re: [RFC PATCH net-next 08/16] net: mscc: ocelot: avoid unneeded "lp" variable in LAG join

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]

Date:   Tue, 15 Dec 2020 17:47:56 +0100
From:   Alexandre Belloni <alexandre.belloni@...tlin.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     Tobias Waldekranz <tobias@...dekranz.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        UNGLinuxDriver@...rochip.com, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Claudiu Manoil <claudiu.manoil@....com>
Subject: Re: [RFC PATCH net-next 08/16] net: mscc: ocelot: avoid unneeded
 "lp" variable in LAG join

On 08/12/2020 14:07:54+0200, Vladimir Oltean wrote:
> The index of the LAG is equal to the logical port ID that all the
> physical port members have, which is further equal to the index of the
> first physical port that is a member of the LAG.
> 
> The code gets a bit carried away with logic like this:
> 
> 	if (a == b)
> 		c = a;
> 	else
> 		c = b;
> 
> which can be simplified, of course, into:
> 
> 	c = b;
> 
> (with a being port, b being lp, c being lag)
> 
> This further makes the "lp" variable redundant, since we can use "lag"
> everywhere where "lp" (logical port) was used. So instead of a "c = b"
> assignment, we can do a complete deletion of b. Only one comment here:
> 
> 		if (bond_mask) {
> 			lp = __ffs(bond_mask);
> 			ocelot->lags[lp] = 0;
> 		}
> 
> lp was clobbered before, because it was used as a temporary variable to
> hold the new smallest port ID from the bond. Now that we don't have "lp"
> any longer, we'll just avoid the temporary variable and zeroize the
> bonding mask directly.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@...tlin.com>

> ---
>  drivers/net/ethernet/mscc/ocelot.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 30dee1f957d1..080fd4ce37ea 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -1291,28 +1291,24 @@ int ocelot_port_lag_join(struct ocelot *ocelot, int port,
>  			 struct net_device *bond)
>  {
>  	u32 bond_mask = 0;
> -	int lag, lp;
> +	int lag;
>  
>  	ocelot->ports[port]->bond = bond;
>  
>  	bond_mask = ocelot_get_bond_mask(ocelot, bond);
>  
> -	lp = __ffs(bond_mask);
> +	lag = __ffs(bond_mask);
>  
>  	/* If the new port is the lowest one, use it as the logical port from
>  	 * now on
>  	 */
> -	if (port == lp) {
> -		lag = port;
> +	if (port == lag) {
>  		ocelot->lags[port] = bond_mask;
>  		bond_mask &= ~BIT(port);
> -		if (bond_mask) {
> -			lp = __ffs(bond_mask);
> -			ocelot->lags[lp] = 0;
> -		}
> +		if (bond_mask)
> +			ocelot->lags[__ffs(bond_mask)] = 0;
>  	} else {
> -		lag = lp;
> -		ocelot->lags[lp] |= BIT(port);
> +		ocelot->lags[lag] |= BIT(port);
>  	}
>  
>  	ocelot_setup_lag(ocelot, lag);
> -- 
> 2.25.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com