netdev - Re: [PATCH net] net: dsa: microchip: Fix reserved multicast address table programming

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] [day] [month] [year] [list]

Message-ID: <9f8e7666-d78b-418d-b660-82af4d79983e@lunn.ch>
Date: Sun, 2 Nov 2025 16:30:32 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Tristram.Ha@...rochip.com
Cc: Woojung Huh <woojung.huh@...rochip.com>,
	Arun Ramadoss <arun.ramadoss@...rochip.com>,
	Vladimir Oltean <olteanv@...il.com>,
	Oleksij Rempel <linux@...pel-privat.de>,
	Łukasz Majewski <lukma@...ladev.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	UNGLinuxDriver@...rochip.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: dsa: microchip: Fix reserved multicast address
 table programming

> +	/* The reserved multicast address table has 8 entries.  Each entry has
> +	 * a default value of which port to forward.  It is assumed the host
> +	 * port is the last port in most of the switches, but that is not the
> +	 * case for KSZ9477 or maybe KSZ9897.  For LAN937X family the default
> +	 * port is port 5, the first RGMII port.  It is okay for LAN9370, a
> +	 * 5-port switch, but may not be correct for the other 8-port
> +	 * versions.  It is necessary to update the whole table to forward to
> +	 * the right ports.
> +	 * Furthermore PTP messages can use a reserved multicast address and
> +	 * the host will not receive them if this table is not correct.
> +	 */
> +	def_port = BIT(dev->info->port_cnt - 1);
> +	if (is_lan937x(dev))
> +		def_port = BIT(4);

Why not just def_port = dsa_cpu_ports(ds)?

The aim here is to send frames to the CPU. You then don't need the
comment about different switch versions.

> +	for (i = 0; i < 8; i++) {

Please replace the 8 with a #define.

> +		if (ports == def_port) {
> +			/* Change the host port. */
> +			update = BIT(dev->cpu_port);
> +
> +			/* The host port is correct so no need to update the
> +			 * the whole table but the first entry still needs to
> +			 * set the Override bit for STP.
> +			 */
> +			if (update == def_port && i == 0)
> +				ports = 0;
> +		} else if (ports == 0) {
> +			/* No change to entry. */
> +			update = 0;
> +		} else if (ports == (all_ports & ~def_port)) {
> +			/* This entry does not forward to host port.  But if
> +			 * the host needs to process protocols like MVRP and
> +			 * MMRP the host port needs to be set.
> +			 */
> +			update = ports & ~BIT(dev->cpu_port);
> +			update |= def_port;
> +		} else {
> +			/* No change to entry. */
> +			update = ports;
> +		}
> +		if (update != ports) {
> +			data &= ~dev->port_mask;
> +			data |= update;
> +			/* Set Override bit for STP in the first entry. */
> +			if (i == 0)
> +				data |= ALU_V_OVERRIDE;

You have already made this comparison once before. Maybe

	update |= ALU_V_OVERRIDE

higher up?

	Andrew