Re: [PATCH net-next 2/2] net: sparx5: Add mdb handlers

[Casper Andersson <casper.casan@gmail.com>]

On 2022-03-22 15:51, Steen Hegelund wrote:
> Hi Casper
> 
> On Tue, 2022-03-22 at 10:59 +0100, Casper Andersson wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > So this was already merged, but I have some comments on the feedback for
> > the follow up patch.
> > 
> > > > +static int sparx5_handle_port_mdb_add(struct net_device *dev,
> > > > +                                     struct notifier_block *nb,
> > > > +                                     const struct switchdev_obj_port_mdb *v)
> > > > +{
> > > > +       struct sparx5_port *port = netdev_priv(dev);
> > > > +       struct sparx5 *spx5 = port->sparx5;
> > > > +       u16 pgid_idx, vid;
> > > > +       u32 mact_entry;
> > > > +       int res, err;
> > > > +
> > > > +       /* When VLAN unaware the vlan value is not parsed and we receive vid 0.
> > > > +        * Fall back to bridge vid 1.
> > > > +        */
> > > > +       if (!br_vlan_enabled(spx5->hw_bridge_dev))
> > > > +               vid = 1;
> > > > +       else
> > > > +               vid = v->vid;
> > > > +
> > > > +       res = sparx5_mact_find(spx5, v->addr, vid, &mact_entry);
> > > > +
> > > > +       if (res) {
> > > > +               pgid_idx = LRN_MAC_ACCESS_CFG_2_MAC_ENTRY_ADDR_GET(mact_entry);
> > > > +
> > > > +               /* MC_IDX has an offset of 65 in the PGID table. */
> > > > +               pgid_idx += PGID_MCAST_START;
> > > 
> > > This will overlap some of the first ports with the flood masks according to:
> > > 
> > > https://microchip-ung.github.io/sparx-5_reginfo/reginfo_sparx-5.html?select=ana_ac,pgid
> > > 
> > > You should use the custom area (PGID_BASE + 8 and onwards) for this new feature.
> > 
> > I'm aware of the overlap, hence why the PGID table has those fields
> > marked as reserved. But your datasheet says that the multicast index
> > has an offset of 65 (ie. MC_IDX = 0 is at PGID = 65). This is already
> > taken into account in the mact_learn function. I could set the
> > allocation to start at PGID_BASE + 8, but the offset still needs to
> > be 65, right?
> 
> As I understand the PGID table functionality, you will need to start your custom table at PGID_BASE
> + 8 as the bitmasks at offset 65 to 70 are used as flood masks, so their purpose are already
> defined.

It is fine to start the allocation of multicast entries at PGID_BASE + 8,
but the multicast index (MC_IDX) in the mactable (that points at the
PGID table, to get the port mask) assumes that MC_IDX = 0 is at PGID = 65.
Not sure if it's appropriate to reference the datasheet here on Netdev,
but on figure 4-48 (PGID Layout) this is shown.

I tried setting the offset to base + 8 but it does not seem to find the 
right mask. Because if I say I put the mask on MC_IDX 0, and then place
it on base + 8 (73), then it will not find that mask because the hardware
will look for the mask at 65.

Even though the offset is 65, the PGID allocation will make sure
that it never allocates anything below 73. Meaning that the lowest
possible MC_IDX it can allocate will be 8.

I'm also a bit confused as to why the multicast offset is 65, but there
are a lot of overlapping areas in the PGID table and, e.g., GLAG has 
another offset where its index 0 is at PGID = 833 (as can be seen in
figure 4-48).

BR
Casper

> BR
> Steen
> 
> > 
> > BR
> > Casper
>