Re: [net-next PATCH v4 7/8] net: dsa: realtek: rtl8365mb: add FDB support

[Luiz Angelo Daros de Luca]

> +int rtl8365mb_l2_add_mc(struct realtek_priv *priv, int port,
> +                       const unsigned char mac_addr[static ETH_ALEN],
> +                       u16 vid)
> +{
> +       u16 data[RTL8365MB_L2_ENTRY_SIZE] = { 0 };
> +       struct rtl8365mb_l2_mc mc = { 0 };
> +       u16 addr;
> +       int ret;
> +
> +       memcpy(mc.key.mac_addr, mac_addr, ETH_ALEN);
> +       mc.key.vid = vid;
> +       mc.key.ivl = true;
> +       /* Already set the port and is_static, although not used in OP_READ,
> +        * data will be ready for OP_WRITE if it is a new entry.
> +        */
> +       mc.member |= BIT(port);
> +       mc.is_static = 1;
> +       rtl8365mb_l2_mc_to_data(&mc, data);
> +
> +       /* First look for an existing entry (to get existing port members) */
> +       ret = rtl8365mb_table_query(priv, RTL8365MB_TABLE_L2,
> +                                   RTL8365MB_TABLE_OP_READ, &addr,
> +                                   RTL8365MB_TABLE_L2_METHOD_MAC, 0,
> +                                   data, RTL8365MB_L2_ENTRY_SIZE);
> +       if (!ret) {
> +               /* There is already an entry... */
> +               rtl8365mb_l2_data_to_mc(data, &mc);
> +               dev_dbg(priv->dev, "l2_add_mc: found existing addr=%d member=0x%x igmpidx=0x%x static=%d\n",
> +                       addr, mc.member, mc.igmpidx, mc.is_static);
> +               /* the port must be added as a member */
> +               mc.member |= BIT(port);
> +               rtl8365mb_l2_mc_to_data(&mc, data);

Sashiko asked me to force is_static = 1 on any existing multicast
group. However, I am not sure if the hardware can even create dynamic
multicast groups when operating under a DSA driver. Shouldn't all
multicast control be handled directly by the kernel?

If a dynamic multicast group does appear in the hardware, what would
be the lesser of two evils? I could do as Sashiko suggested and
convert dynamic groups into static ones, but that carries a major risk
of leaking them in the table forever. Alternatively, I could keep them
dynamic and accept losing them once they expire. Personally, I would
prefer to keep the current code as-is.

> +       } else if (ret == -ENOENT) {
> +               /* New entry, no need to update data again as it already
> +                * includes the member.
> +                *
> +                * Multicast hardware entries do not support EFID (bridge
> +                * isolation). However, traffic isolation is still maintained
> +                * because the hardware applies the port isolation masks
> +                * (pmasks) configured in bridge_join after the L2 lookup.
> +                * Entries from different bridges will collide on the same
> +                * MAC+VID slot with an OR'ed member mask, but packets will
> +                * only exit through ports allowed by the source port's pmask.
> +                */
> +       } else {
> +               return ret;
> +       }
> +
> +       /* add the new entry or update an existing one */
> +       ret = rtl8365mb_table_query(priv, RTL8365MB_TABLE_L2,
> +                                   RTL8365MB_TABLE_OP_WRITE, &addr,
> +                                   0, 0,
> +                                   data, RTL8365MB_L2_ENTRY_SIZE);
> +       /* addr will hold the table index, but it is not used here */
> +       if (ret == -ENOENT) {
> +               /* -ENOENT means the just added entry was not found (and @addr
> +                * does not hold the table index. Although any error will be
> +                * treated equally by the caller, assume that the missing entry
> +                * means the table is full (tested in real HW).
> +                */
> +               return -ENOSPC;
> +       }
> +
> +       return ret;
> +}

> +
> +int rtl8365mb_l2_del_mc(struct realtek_priv *priv, int port,
> +                       const unsigned char mac_addr[static ETH_ALEN],
> +                       u16 vid)
> +{
> +       u16 data[RTL8365MB_L2_ENTRY_SIZE] = { 0 };
> +       struct rtl8365mb_l2_mc mc = { 0 };
> +       u16 addr;
> +       int ret;
> +
> +       memcpy(mc.key.mac_addr, mac_addr, ETH_ALEN);
> +       mc.key.vid = vid;
> +       mc.key.ivl = true;
> +       rtl8365mb_l2_mc_to_data(&mc, data);
> +
> +       /* First look for an existing entry (to get existing port members) */
> +       ret = rtl8365mb_table_query(priv, RTL8365MB_TABLE_L2,
> +                                   RTL8365MB_TABLE_OP_READ, &addr,
> +                                   RTL8365MB_TABLE_L2_METHOD_MAC, 0,
> +                                   data, RTL8365MB_L2_ENTRY_SIZE);
> +       if (ret == -ENOENT) {
> +               dev_dbg(priv->dev, "l2_del_mc: addr=%d vid=%d missing\n",
> +                       addr, vid);

I will drop addr from the debug message since it will be uninitialized.

> +               /* Silently return success */
> +               return 0;
> +       }
> +
> +       if (ret)
> +               /* Return on any other error */
> +               return ret;
> +
> +       rtl8365mb_l2_data_to_mc(data, &mc);
> +       dev_dbg(priv->dev, "l2_del_mc: found existing addr=%d member=0x%x igmpidx=0x%x static=%d\n",
> +               addr, mc.member, mc.igmpidx, mc.is_static);
> +       /* the port must be removed as a member */
> +       mc.member &= ~BIT(port);
> +       if (!mc.member) {
> +               /* With no members, zero all non-key fields to delete the
> +                * entry. However, setting is_static = 0 might be enough to
> +                * signal the HW to release the slot.
> +                */
> +               mc.is_static = 0;
> +               mc.igmpidx = 0;
> +               mc.priority = 0;
> +               mc.fwd_pri = 0;
> +               mc.igmp_asic = 0;
> +       }
> +       rtl8365mb_l2_mc_to_data(&mc, data);
> +
> +       /* update the existing entry. */
> +       ret = rtl8365mb_table_query(priv, RTL8365MB_TABLE_L2,
> +                                   RTL8365MB_TABLE_OP_WRITE, &addr,
> +                                   0, 0,
> +                                   data, RTL8365MB_L2_ENTRY_SIZE);
> +       return ret;
> +}

[...]

> @@ -2511,6 +2529,7 @@ const struct realtek_variant rtl8365mb_variant = {
>         .clk_delay = 10,
>         .cmd_read = 0xb9,
>         .cmd_write = 0xb8,
> +       .l2_table_size = RTL8365MB_LEARN_LIMIT_MAX,

Sashiko asked me to reconsider using the learning limit macro to
define .l2_table_size. The vendor's reference code explicitly derives
one definition from the other:

#define RTK_MAX_LUT_ADDR_ID         (RTK_MAX_LUT_ADDRESS - 1)
#define RTK_MAX_LUT_ADDRESS         (RTK_MAX_NUM_OF_LEARN_LIMIT)

To make this relationship explicit and ensure we use each concept
properly, I'll add a new macro:

+/* The LUT table size matches the maximum learning limit */
+#define RTL8365MB_L2_TABLE_SIZE        RTL8365MB_LEARN_LIMIT_MAX

Regards,

Luiz