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

[Luiz Angelo Daros de Luca]

Hello,

I'm getting double calls to delete the same MDB entry:

[11725.824460] rtl8365mb-mdio mdio-bus:1d: rtl83xx_port_mdb_del:
port:2 addr:33:33:00:00:00:02 efid:1 vid:1 dbtype:2
[11725.836787] rtl8365mb-mdio mdio-bus:1d: l2_del_mc: found existing
addr=1376 member=0x4 igmpidx=0x0 static=1
[11725.848130] rtl8365mb-mdio mdio-bus:1d: rtl83xx_port_mdb_del:
port:2 addr:33:33:00:00:00:02 efid:1 vid:1 dbtype:2
[11725.859746] rtl8365mb-mdio mdio-bus:1d: mdb_del ERROR -ENOENT
[11725.865656] rtl8365mb-mdio mdio-bus:1d lan2: Failed to del Port
Multicast Database entry (object id=2) with error: -ENOENT (-2).
[11725.865656] Failure in updating the port's Multicast Database could lead to
[11725.865656] multicast forwarding issues.
[11725.865656]
[11725.890297] rtl8365mb-mdio mdio-bus:1d: rtl83xx_port_mdb_del:
port:2 addr:33:33:ff:cc:27:50 efid:1 vid:1 dbtype:2

(port 2(lan2) in this case is forming a bridge with port 3(lan1))

Is that normal?

I checked if 161ca59d39e9 from Vladimir Oltean was supposed to avoid
that but it explicilty ignores user ports (and focus on CPU/link
ports).

> +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)
> +               /* Any error, including -ENOENT is unexpected */
> +               return ret;

The easier solution would be to ignore -ENOENT here and just return 0
with a dbg message.
Some drivers I checked just do like that.

> +
> +       rtl8365mb_l2_data_to_mc(data, &mc);
> +       /* 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 is_static is everything else we wrote.
> +                * (and probably all that is needed by the HW)
> +                */
> +               mc.is_static = 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;
> +}

> @@ -2442,6 +2448,12 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops = {
...
> +       .port_mdb_del = rtl83xx_port_mdb_del,
...

> +/**
> + * rtl83xx_port_mdb_del() - delete a multicast database entry from a port database
> + * @ds: DSA switch instance
> + * @port: port index
> + * @mdb: multicast database entry to delete
> + * @db: database where the entry should be removed
> + *
> + * This function deletes a multicast database entry from the standalone port
> + * database or from a bridge database.
> + *
> + * Context: Can sleep.
> + * Return: 0 on success, negative value for failure.
> + */
> +int rtl83xx_port_mdb_del(struct dsa_switch *ds, int port,
> +                        const struct switchdev_obj_port_mdb *mdb,
> +                        struct dsa_db db)
> +{
> +       struct realtek_priv *priv = ds->priv;
> +       int ret;
> +
> +       if (!priv->ops->l2_del_mc)
> +               return -EOPNOTSUPP;
> +
> +       if (db.type != DSA_DB_PORT && db.type != DSA_DB_BRIDGE)
> +               return -EOPNOTSUPP;
> +
> +       dev_dbg(priv->dev, "mdb_del port %d addr %pM vid %d\n",
> +               port, mdb->addr, mdb->vid);
> +
> +       mutex_lock(&priv->l2_lock);
> +       ret = priv->ops->l2_del_mc(priv, port, mdb->addr, mdb->vid);
> +       mutex_unlock(&priv->l2_lock);
> +
> +       if (ret)
> +               dev_err(priv->dev, "mdb_del ERROR %pe\n", ERR_PTR(ret));
> +       return ret;
> +}

Regards,

Luiz