Message-ID: <CAJ+vNU2RT2MmyO_YgoQmkb0UdWWKS42_fb1jqYLPmLJf5XNO=A@mail.gmail.com>
Date: Fri, 17 Jan 2025 13:02:31 -0800
From: Tim Harvey <tharvey@...eworks.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Tristram.Ha@...rochip.com, Arun.Ramadoss@...rochip.com, andrew@...n.ch, 
	davem@...emloft.net, Woojung.Huh@...rochip.com, linux-kernel@...r.kernel.org, 
	pabeni@...hat.com, netdev@...r.kernel.org, edumazet@...gle.com, 
	UNGLinuxDriver@...rochip.com, kuba@...nel.org
Subject: Re: [PATCH net] net: dsa: microchip: ksz9477: fix multicast filtering

On Fri, Jan 17, 2025 at 8:13 AM Vladimir Oltean <olteanv@...il.com> wrote:
>
> On Thu, Jan 16, 2025 at 05:25:49PM -0800, Tim Harvey wrote:
> > On Thu, Jan 16, 2025 at 5:09 PM <Tristram.Ha@...rochip.com> wrote:
> > >
> > > > Hi Tim,
> > > >
> > > > > Hi Arun,
> > > > >
> > > > > Ok, that makes sense to me and falls in line with what my patch here
> > > > > was trying to do. When you enable the reserved multicast table it
> > > > > makes sense to update the entire table right? You are only updating
> > > > > one address/group. Can you please review and comment on my patch
> > > > > here?
> > > >
> > > >
> > > > During my testing of STP protocol, I found that Group 0 of reserved
> > > > multicast table needs to be updated. Since I have not worked on other
> > > > groups in the multicast table, I didn't update it.
> > > >
> > > > I could not find the original patch to review, it shows "not found" in
> > > > lore.kernel.org.
> > > >
> > > > Below are my comments,
> > > >
> > > > - Why override bit is not set in REG_SW_ALU_VAL_B register.
> > > > - ksz9477_enable_stp_addr() can be renamed since it updates all the
> > > > table entries.
> > >
> > > The reserved multicast table has only 8 entries that apply to 48
> > > multicast addresses, so some addresses share one entry.
> > >
> > > Some entries that are supposed to forward only to the host port or skip
> > > should be updated when that host port is not the default one.
> > >
> > > The override bit should be set for the STP address as that is required
> > > for receiving when the port is closed.
> > >
> > > Some entries for MVRP/MSRP should forward to the host port when the host
> > > can process those messages and broadcast to all ports when the host does
> > > not process those messages, but that is not controllable by the switch
> > > driver so I do not know how to handle in this situation.
> > >
> >
> > Hi Tristram,
> >
> > Thanks for your feedback.
> >
> > What is the behavior when the reserved multicast table is not enabled
> > (does it forward to all ports, drop all mcast, something else?)
> >
> > > The default reserved multicast table forwards to host port on entries 0,
> > > 2, and 6; skips host port on entries 4, 5, and 7; forwards to all ports
> > > on entry 3; and drops on entry 1.
> > >
> >
> > Is this behavior the desired behavior as far as the Linux DSA folks would want?
> >
> > commit 331d64f752bb ("net: dsa: microchip: add the enable_stp_addr
> > pointer in ksz_dev_ops") enables the reserved multicast table and
> > adjusts the cpu port for entry 0 leaving the rest the same (and wrong
> > if the cpu port is not the highest port in the switch).
> >
> > My patch adjusts the entries but keeps the rules the same and the
> > question that is posed is that the right thing to do with respect to
> > Linux DSA?
> >
> > Best Regard,
> >
> > Tim
>
> Not sure if that's a question for the DSA maintainers or for Tristram,
> because I've expressed already earlier in this thread what is the
> current way in which the Linux bridge expects this mechanism to work.
> This is not the Microchip KSZ SDK, and thus, following the network stack
> rules is not optional, and inventing local conventions when there
> already exist global ones is a no-go. If you don't like the conventions
> you are of course free to challenge them, but this is not the right
> audience to do that.

Vladimir,

The question to Tristram was what is the behavior if the multicast
address table is disabled which was the case prior to commit
331d64f752bb ("net: dsa: microchip: add the enable_stp_addr
pointer in ksz_dev_ops"). I have tested and been able to determine
that the behavior without the multicast address table enabled is that
multicast packets are forwarded to all ports.

What commit 331d64f752bb ("net: dsa: microchip: add the
enable_stp_addr pointer in ksz_dev_ops") does is to effectively
'disable' forwarding of packets to 01-80-C2-00-00-00 (group 0) to
downstream ports. I misread or confused the commit log by thinking the
patch was 'enabling' forwarding... it's the opposite.

The flaw with that patch is that enabling the multicast address table
invokes other default rules in the table that need to be re-configured
for the cpu port but the patch only configures group 0
(01-80-C2-00-00-00). It fails to configure group 6 (01-80-C2-00-00-08)
which is also used for STP so I would argue that it doesn't even do
what the commit log says it does. It also has the side effect of
disabling forwarding of other groups that were previously forwarded:
- group 1 01-80-C2-00-00-01 (MAC Control Frame) (previously were
forwarded, now are dropped)
- group 2 01-80-C2-00-00-03 (802.1X access control) (previously were
forwarded, now are forwarded to the highest port which may not be the
cpu port)
- group 4 01-80-C2-00-00-20 (GMRP) (previously were forwarded, now
forwarded to all except the highest port number which may not be the
cpu port)
- group 5 01-80-C2-00-00-21 (GVRP) (previously were forwarded, now
forwarded to all except the highest port number which may not be the
cpu port)
- group 6 01-80-C2-00-00-02, 01-80-C2-00-00-04 - 01-80-C2-00-00-0F
(previously were forwarded, now are forwarded to the highest port
which may not be the cpu port)
- group 7 01-80-C2-00-00-11 - 01-80-C2-00-00-1F, 01-80-C2-00-00-22 -
01-80-C2-00-00-2F (previously were forwarded, now forwarded to all
except the highest port number which may not be the cpu port)

To fix this, I propose adding a function to configure each of the
above groups (which are hardware filtering functions of the switch)
with proper port masks but I need to know from the DSA experts what is
desired for the port mask of those groups. The multicast address table
can only invoke rules based on those groups of addresses so if that is
not flexible enough then the multicast address table should instead be
disabled.

Obviously Alun felt that forwarding STP packets to only the cpu port
instead of all ports was desired and there were no complaints about
that. If that is wrong then I can revert the enabling of the multicast
address table for ksz9477 and restore the default forwarding of
multicast packets to all ports.

I am not that familiar with Linux DSA so I don't know if it's normal
to forward multicast packets to upstream cpu port only, all ports, or
no ports. If there is some software table where the decision making is
done then the multicast address table should not have been enabled at
all in order to allow the software to determine the course of action.

My goal was to restore multicast packets flowing through to the CPU
port which in my board's case is not the highest number port so the
above default rules are just wrong and would need to be updated if the
multicast address table is to be used.

How would you like me to proceed?

Best Regards,

Tim

Powered by blists - more mailing lists