netdev - Re: [PATCH net-next 1/3] net: dsa: qca8k: do not write port mask twice in bridge join/leave

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: <0ec0c543-8b5e-427f-856d-69a9623bb84b@intel.com>
Date: Fri, 21 Jun 2024 12:48:37 +0200
From: Wojciech Drewek <wojciech.drewek@...el.com>
To: Matthias Schiffer <mschiffer@...verse-factory.net>, Andrew Lunn
	<andrew@...n.ch>, Florian Fainelli <f.fainelli@...il.com>, Vladimir Oltean
	<olteanv@...il.com>
CC: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
	<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Christian Marangi <ansuelsmth@...il.com>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 1/3] net: dsa: qca8k: do not write port mask
 twice in bridge join/leave



On 20.06.2024 19:25, Matthias Schiffer wrote:
> qca8k_port_bridge_join() set QCA8K_PORT_LOOKUP_CTRL() for i == port twice,
> once in the loop handling all other port's masks, and finally at the end
> with the accumulated port_mask.
> 
> The first time it would incorrectly set the port's own bit in the mask,
> only to correct the mistake a moment later. qca8k_port_bridge_leave() had
> the same issue, but here the regmap_clear_bits() was a no-op rather than
> setting an unintended value.
> 
> Remove the duplicate assignment by skipping the whole loop iteration for
> i == port. The unintended bit setting doesn't seem to have any negative
> effects (even when not reverted right away), so the change is submitted
> as a simple cleanup rather than a fix.
> 
> Signed-off-by: Matthias Schiffer <mschiffer@...verse-factory.net>
> ---

Reviewed-by: Wojciech Drewek <wojciech.drewek@...el.com>

>  drivers/net/dsa/qca/qca8k-common.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c
> index 7f80035c5441..b33df84070d3 100644
> --- a/drivers/net/dsa/qca/qca8k-common.c
> +++ b/drivers/net/dsa/qca/qca8k-common.c
> @@ -653,6 +653,8 @@ int qca8k_port_bridge_join(struct dsa_switch *ds, int port,
>  	port_mask = BIT(cpu_port);
>  
>  	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> +		if (i == port)
> +			continue;
>  		if (dsa_is_cpu_port(ds, i))
>  			continue;
>  		if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))
> @@ -665,8 +667,7 @@ int qca8k_port_bridge_join(struct dsa_switch *ds, int port,
>  				      BIT(port));
>  		if (ret)
>  			return ret;
> -		if (i != port)
> -			port_mask |= BIT(i);
> +		port_mask |= BIT(i);
>  	}
>  
>  	/* Add all other ports to this ports portvlan mask */
> @@ -685,6 +686,8 @@ void qca8k_port_bridge_leave(struct dsa_switch *ds, int port,
>  	cpu_port = dsa_to_port(ds, port)->cpu_dp->index;
>  
>  	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> +		if (i == port)
> +			continue;
>  		if (dsa_is_cpu_port(ds, i))
>  			continue;
>  		if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))