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

[Andrew Lunn <andrew@lunn.ch>]

> +	/* 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