Message-ID: <CAJ+vNU2WQ2n588vOcofJ5Ga76hsyff51EW-e6T8PbYFY_4xu0A@mail.gmail.com>
Date: Thu, 12 Dec 2024 16:32:28 -0800
From: Tim Harvey <tharvey@...eworks.com>
To: Vladimir Oltean <olteanv@...il.com>, Arun Ramadoss <arun.ramadoss@...rochip.com>
Cc: 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 4:00 PM Vladimir Oltean <olteanv@...il.com> wrote:
>
> On Thu, Dec 12, 2024 at 01:51:32PM -0800, Tim Harvey wrote:
> > commit 331d64f752bb ("net: dsa: microchip: add the enable_stp_addr
> > pointer in ksz_dev_ops") introduced enabling of the reserved multicast
> > address table function to filter packets based on multicast MAC address
> > but only configured one MAC address group, group 0 for
> > (01-80-C2-00)-00-00 for bridge group data.
> >
> > This causes other multicast groups to fail to be received such as LLDP
> > which uses a MAC address of 01-80-c2-00-00-0e (group 6).
> >
> > Enabling the reserved multicast address table requires configuring the
> > port forward mask for all eight address groups as the mask depends on
> > the port configuration.
>

Hi Vladimir,

> Personal experience reading your commit message: it took me a long while
> to realize that the reason why the 8 pre-configured Reserved Multicast
> table entries don't work is written here: "the mask depends on the port
> configuration." It is absolutely understated IMO.
>

Yes, if you are going to enable the reserved multicast address table
it should be configured fully as the default configuration makes an
assumption on what user/host ports are valid.

> > The table determines the forwarding ports for
> > 48 specific multicast addresses and is addressed by the least
> > significant 6 bits of the multicast address. Changing a forwarding
> > port mask for one address also makes the same change for all other
> > addresses in the same group.
> >
> > Add configuration of the groups as such:
> >  - leave these as default:
> >    group 1 (01-80-C2-00)-00-01 (MAC Control Frame) (drop)
> >    group 3 (01-80-C2-00)-00-10) (Bridge Management) (all ports)
> >  - forward to cpu port:
> >    group 0 (01-80-C2-00)-00-00 (Bridge Group Data)
> >    group 2 (01-80-C2-00)-00-03 (802.1X access control)
> >    group 6 (01-80-C2-00)-00-02, (01-80-C2-00)-00-04 – (01-80-C2-00)-00-0F
> >  - 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.

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

> >
> > 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.

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 then
it should be disabled and the content of ksz9477_enable_stp_addr()
removed. 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.

All of your coding style comments below make complete sense to me so
as soon as we figure out what the proper fix is for commit
331d64f752bb ("net: dsa: microchip: add the enable_stp_addr pointer in
ksz_dev_ops") breaking multicast I can resubmit with those resolved.

Best Regards,

Tim

>
> >
> > Fixes: 331d64f752bb ("net: dsa: microchip: add the enable_stp_addr pointer in ksz_dev_ops")
> > Signed-off-by: Tim Harvey <tharvey@...eworks.com>
> > ---
> >  drivers/net/dsa/microchip/ksz9477.c | 84 +++++++++++++++++++++++++----
> >  1 file changed, 75 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> > index d16817e0476f..d8fe809dd461 100644
> > --- a/drivers/net/dsa/microchip/ksz9477.c
> > +++ b/drivers/net/dsa/microchip/ksz9477.c
> > @@ -1138,25 +1138,24 @@ void ksz9477_config_cpu_port(struct dsa_switch *ds)
> >       }
> >  }
> >
> > -int ksz9477_enable_stp_addr(struct ksz_device *dev)
> > +static int ksz9477_reserved_muticast_group(struct ksz_device *dev, int index, int mask)
> >  {
> > +     const u8 *shifts;
> >       const u32 *masks;
> >       u32 data;
> >       int ret;
> >
> > +     shifts = dev->info->shifts;
> >       masks = dev->info->masks;
> >
> > -     /* Enable Reserved multicast table */
> > -     ksz_cfg(dev, REG_SW_LUE_CTRL_0, SW_RESV_MCAST_ENABLE, true);
> > -
> > -     /* Set the Override bit for forwarding BPDU packet to CPU */
> > -     ret = ksz_write32(dev, REG_SW_ALU_VAL_B,
> > -                       ALU_V_OVERRIDE | BIT(dev->cpu_port));
> > +     /* write the PORT_FORWARD value to the Reserved Multicast Address Table Entry 2 Register */
>
> In netdev the coding style limits the line length to 80 characters where
> that is easy, like here.
>
> > +     ret = ksz_write32(dev, REG_SW_ALU_VAL_B, mask);
> >       if (ret < 0)
> >               return ret;
> >
> > -     data = ALU_STAT_START | ALU_RESV_MCAST_ADDR | masks[ALU_STAT_WRITE];
> > -
> > +     /* write to the Static Address and Reserved Multicast Table Control Register */
> > +     data = (index << shifts[ALU_STAT_INDEX]) |
> > +             ALU_STAT_START | ALU_RESV_MCAST_ADDR | masks[ALU_STAT_WRITE];
> >       ret = ksz_write32(dev, REG_SW_ALU_STAT_CTRL__4, data);
> >       if (ret < 0)
> >               return ret;
> > @@ -1167,8 +1166,75 @@ int ksz9477_enable_stp_addr(struct ksz_device *dev)
> >               dev_err(dev->dev, "Failed to update Reserved Multicast table\n");
> >               return ret;
> >       }
> > +     return ksz9477_wait_alu_sta_ready(dev);
> > +}
> > +
> > +int ksz9477_enable_stp_addr(struct ksz_device *dev)
> > +{
> > +     int ret;
> > +     int cpu_mask = dsa_cpu_ports(dev->ds);
> > +     int user_mask = dsa_user_ports(dev->ds);
>
> Also, in netdev, the coding style is to sort lines with variable
> declarations in the reverse order of their length (so-called reverse
> Christmas tree).
>
> > +     /* array of indexes into table:
> > +      * The table is indexed by the low 6 bits of the MAC address.
> > +      * Changing the PORT_FORWARD value for any single address affects
> > +      * all others in group
> > +      */
> > +     u16 addr_groups[8] = {
>
> Array can be static const. Also, since all elements are initialized,
> specifying its size explicitly is not necessary ("[8]" can be "[]").
>
> > +             /* group 0: (01-80-C2-00)-00-00 (Bridge Group Data) */
> > +             0x000,
> > +             /* group 1: (01-80-C2-00)-00-01 (MAC Control Frame) */
> > +             0x001,
> > +             /* group 2: (01-80-C2-00)-00-03 (802.1X access control) */
> > +             0x003,
> > +             /* group 3: (01-80-C2-00)-00-10) (Bridge Management) */
> > +             0x010,
> > +             /* group 4: (01-80-C2-00)-00-20 (GMRP) */
> > +             0x020,
> > +             /* group 5: (01-80-C2-00)-00-21 (GVRP) */
> > +             0x021,
> > +             /* group 6: (01-80-C2-00)-00-02, (01-80-C2-00)-00-04 – (01-80-C2-00)-00-0F */
> > +             0x002,
> > +             /* 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
> > +              */
> > +             0x011,
> > +     };
> > +
> > +     /* Enable Reserved multicast table */
> > +     ksz_cfg(dev, REG_SW_LUE_CTRL_0, SW_RESV_MCAST_ENABLE, true);
> > +
> > +     /* update reserved multicast address table:
> > +      * leave as default:
> > +      *  - group 1 (01-80-C2-00)-00-01 (MAC Control Frame) (drop)
> > +      *  - group 3 (01-80-C2-00)-00-10) (Bridge Management) (all ports)
> > +      * forward to cpu port:
> > +      *  - group 0 (01-80-C2-00)-00-00 (Bridge Group Data)
> > +      *  - group 2 (01-80-C2-00)-00-03 (802.1X access control)
> > +      *  - group 6 (01-80-C2-00)-00-02, (01-80-C2-00)-00-04 – (01-80-C2-00)-00-0F
> > +      * forward to all but cpu port:
> > +      *  - 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
> > +      */
> > +     if (ksz9477_reserved_muticast_group(dev, addr_groups[0], cpu_mask))
> > +             goto exit;
>
> err = (function return code), and print it with %pe, ERR_PTR(err) please.
> We want to distinguish between -ETIMEDOUT in ksz9477_wait_alu_sta_ready()
> vs whatever ksz_write32() may return.
>
> > +     if (ksz9477_reserved_muticast_group(dev, addr_groups[2], cpu_mask))
> > +             goto exit;
> > +     if (ksz9477_reserved_muticast_group(dev, addr_groups[6], cpu_mask))
> > +             goto exit;
> > +     if (ksz9477_reserved_muticast_group(dev, addr_groups[4], user_mask))
> > +             goto exit;
> > +     if (ksz9477_reserved_muticast_group(dev, addr_groups[5], user_mask))
> > +             goto exit;
> > +     if (ksz9477_reserved_muticast_group(dev, addr_groups[7], user_mask))
> > +             goto exit;
> >
> >       return 0;
> > +
> > +exit:
> > +     dev_err(dev->dev, "Failed to update Reserved Multicast table\n");
> > +     return ret;
> >  }
> >
> >  int ksz9477_setup(struct dsa_switch *ds)
> > --
> > 2.34.1
> >
>

Powered by blists - more mailing lists