Message-ID: <20241213011405.ogogu5rvbjimuqji@skbuf>
Date: Fri, 13 Dec 2024 03:14:05 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Tim Harvey <tharvey@...eworks.com>
Cc: Arun Ramadoss <arun.ramadoss@...rochip.com>,
	Woojung Huh <woojung.huh@...rochip.com>,
	UNGLinuxDriver@...rochip.com, Andrew Lunn <andrew@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: dsa: microchip: ksz9477: fix multicast filtering

On Thu, Dec 12, 2024 at 04:32:28PM -0800, Tim Harvey wrote:
> > >  - forward to all but cpu port:
> >
> > Why would you not forward packets to the CPU port as a hardcoded configuration?
> > What if the KSZ ports are bridged together with a foreign interface
> > (different NIC, WLAN, tunnel etc), how should the packets reach that?
> 
> If that is the correct thing to do I can certainly do that. I was
> assuming that the default policy above must be somewhat standard. This
> patch leaves the policy that was created by the default table
> configuration and just updates the port configuration based on the dt
> definition of the user vs host ports.

I think you misunderstood my comment which you've quoted here, it was:
"why would you hardcode a configuration which can't be changed and which
doesn't include _at least_ the CPU port in the list of destination
ports?! isn't that needed for so many reasons?"

This particular paragraph did not contain any suggestion of the form
"the correct thing to do is ...", just an observation that the choice
you've made is most likely wrong.

> > >    group 4 (01-80-C2-00)-00-20 (GMRP)
> > >    group 5 (01-80-C2-00)-00-21 (GVRP)
> > >    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
> >
> > Don't you want to forgo the (odd) hardware defaults for the Reserved Multicast
> > table, and instead follow what the Linux bridge does in br_handle_frame()?
> > Which is to trap all is_link_local_ether_addr() addresses to the CPU, do
> > _not_ call dsa_default_offload_fwd_mark() for those packets (aka let the
> > bridge know that they haven't been forwarded in hardware, and if they
> > should reach other bridge ports, this must be done in software), and let the
> > user choose, via the bridge group_fwd_mask, if they should be forwarded
> > to other bridge ports or not?
> 
> Again, I really don't know what the 'right' thing to do is for
> multicast packets but the enabling of the reserved multicast table
> done in commit 331d64f752bb ("net: dsa: microchip: add the
> enable_stp_addr pointer in ksz_dev_ops") breaks forwarding of all
> multicast packets that are not sent to 01-80-C2-00-00-00

Yes, because prior to that commit, this table wasn't consulted by the
data path of the switch.

> > > Datasheets:
> > > [1] https://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9897S-Data-Sheet-DS00002394C.pdf
> > > [2] https://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9896C-Data-Sheet-DS00002390C.pdf
> > > [3] https://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9893R-Data-Sheet-DS00002420D.pdf
> > > [4] https://ww1.microchip.com/downloads/en/DeviceDoc/00002330B.pdf
> > > [5] https://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9563R-Data-Sheet-DS00002419D.pdf
> > > [6] https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ9567R-Data-Sheet-DS00002329.pdf
> > > [7] https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ9567R-Data-Sheet-DS00002329.pdf
> >
> > [6] and [7] are the same.
> >
> > Also, you'd better specify in the commit message what's with these datasheet
> > links, which to me and I suppose all other non-expert readers, are pasted here
> > out of the blue, with no context.
> >
> > Like for example: "KSZ9897, ..., have arbitrary CPU port assignments, as
> > can be seen in the driver's ksz_chip_data :: cpu_ports entries for these
> > families, and the CPU port selection on a certain board rarely coincides
> > with the default host port selection in the Reserved Multicast address
> > table".
> 
> I was just trying to be thorough. I took the time to look up the
> datasheets for all the switches that the ksz9447 driver supports to
> ensure they all had the same default configuration policy and same
> configuration method/registers so I thought I would include them in
> the message. I can drop the datasheet links if they add no value. I
> was also expecting perhaps the commit message was confusing so I
> wanted to show where the information came from.

They do add value, just guide the reader towards what they should be
looking at, rather than throwing 6 books at them. I gave my own
interpretation above of what I think should be the takeaway, after
spending more than 1 hour studying, and I still might have not seen the
same things as you. Just don't expect everybody to spend the same amount
of time.

> What you're suggesting above regarding trapping all
> is_link_local_ether_addr() addresses to the CPU and not calling
> dsa_default_offload_fwd_mark() is beyond my understanding.
> If the behavior of the reserved multicast address table is non-standard

The behavior of that table is customizable, in fact, and can be brought
into compliance with the Linux network stack's expectations.

Other network stacks might be different, but in Linux, the easiest way
to achieve configurability of the group forwarding would be to let
software do it. The bridge group_fwd_mask is a bit mask of reserved
multicast groups (group X in 01-80-C2-00-00-0X). For example, setting
bit 14 (0xe) (aka set group_fwd_mask to 0x4000) would mean "let group
01-80-C2-00-00-0E be forwarded on all bridge ports, and all other groups
be just trapped".

Conceivably, even in Linux there might be other ways to do it, like
reprogram the hardware tables according to the group_fwd_mask value
communicated through switchdev. But that infrastructure doesn't exist.

> then it should be disabled and the content of ksz9477_enable_stp_addr()
> removed.

I wouldn't jump the gun so soon.

> However based on Arun's commit message it seems that prior to
> that patch STP BPDU packets were not being forwarded to the CPU so
> it's unclear to me what the default behavior was for multicast without
> the reserved muticast address table being enabled. I know that if the
> table is disabled by removing the call to ksz9477_enable_stp_addr then
> LLDP packets are forwarded to the cpu port like they were before that
> patch.

Reading the commit message: "In order to transmit the STP BPDU packet to
the CPU port, the STP address (...) has to be added to static alu table",
I think the correct key in which it should be interpreted is:

"In order to transmit the STP BPDU packets [just] to the CPU port
[rather than flood them towards all ports], the STP address has to ...".

I will give it to you that it is quite a stretch to interpret it in this
way, and it is also frustrating to me to have to extract technically
valid information from loose formulations like these. Plus, unlike both
you and Arun, no access to this hardware. But at least, this is the only
interpretation with which I see no contradictions.

I will let Arun confirm that the commit message should be interpreted in
this way and not in another. But at the same time, you could also
confirm that when the Reserved Multicast address table lookup is disabled,
they are treated just like any other multicast traffic with no hit in
the address table: aka broadcast.

Powered by blists - more mailing lists