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:
 <DM3PR11MB87369A57A48FA097DA7A5A74ECC5A@DM3PR11MB8736.namprd11.prod.outlook.com>
Date: Wed, 5 Nov 2025 01:52:57 +0000
From: <Tristram.Ha@...rochip.com>
To: <andrew@...n.ch>
CC: <Woojung.Huh@...rochip.com>, <Arun.Ramadoss@...rochip.com>,
	<olteanv@...il.com>, <linux@...pel-privat.de>, <lukma@...ladev.com>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<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

> Subject: Re: [PATCH net] net: dsa: microchip: Fix reserved multicast address table
> programming
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content
> is safe
> 
> > +     /* 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.
> 

There are 8 entries in the reserved multicast address table to support
multiple multicast addresses.  They look like this for 7-port switch:

0=0x40
1=0x00
2=0x40
3=0x7F
4=0x3F
5=0x3F
6=0x40
7=0x3F

For 3-port switch:

0=0x04
1=0x00
2=0x04
3=0x07
4=0x03
5=0x03
6=0x04
7=0x03

When the host port is not the expected last port like in KSZ9477 then
some entries in the table need to be updated like the ones with 0x40 and
0x3F port forwarding.

The design of LAN937X is a little bit different in that all host port
candidates are in the middle.  The first RGMII port starts at 5, probably
because there is a 5-port version.

0=0x10
1=0x00
2=0x10
3=0xFF
4=0xEF
5=0xEF
6=0x10
7=0xEF

So when the host port is port 6 or port 4 then the table needs to be
changed like this:

0=0x20
1=0x00
2=0x20
3=0xFF
4=0xDF
5=0xDF
6=0x20
7=0xDF

The code is to check each entry and only update it if it is necessary.

On the other hand software knows what the final mapping should be and
can just write the required entries.

> > +     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?

The first entry is used for STP, so the override bit needs to be set
regardless the host port is same or not as the host needs to receive
BPDU even when the port receive is turned off.

The six entry is responsible for multiple multicast addresses including
01:80:C2:00:00:0E, which is used for Layer 2 Pdelay PTP messages.  The
override bit also needs to be set as it is required to receive such PTP
messages even though the port is closed.  This change will be updated in
next patch.