Re: [PATCH net-next RFC 10/12] net: dsa: Move FDB dump implementation inside DSA

[Vivien Didelot]

Hi Arkadi,

Arkadi Sharshevsky <arka...@mellanox.com> writes:

> +struct dsa_slave_dump_ctx {
> +     struct net_device *dev;
> +     struct sk_buff *skb;
> +     struct netlink_callback *cb;
> +     int idx;
> +};
> +
>  struct dsa_switch_ops {
>       /*
>        * Legacy probing.
> @@ -392,9 +399,7 @@ struct dsa_switch_ops {
>       int     (*port_fdb_del)(struct dsa_switch *ds, int port,
>                               const unsigned char *addr, u16 vid);
>       int     (*port_fdb_dump)(struct dsa_switch *ds, int port,
> -                              struct switchdev_obj_port_fdb *fdb,
> -                               switchdev_obj_dump_cb_t *cb);
> -
> +                              struct dsa_slave_dump_ctx *dump);

I would prefer to pass a callback to the driver, so that we can call
port_fdb_dump for other interfaces like debugfs, and for non exposed
switch ports (CPU and DSA links) as well. Something like:

    typedef int dsa_fdb_dump_cb_t(const unsigned char *addr, u16 vid,
                                  bool is_static, void *data);

    int (*port_fdb_dump)(struct dsa_switch *ds, int port,
                         dsa_fdb_dump_cb_t *cb, void *data);

Then dsa_slave_dump_ctx and dsa_slave_port_fdb_do_dump can be static to
net/dsa/slave.c.

> +int dsa_slave_port_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
> +                         struct net_device *dev,
> +                         struct net_device *filter_dev, int *idx)
> +{
> +     struct dsa_slave_dump_ctx dump = {
> +             .dev = dev,
> +             .skb = skb,
> +             .cb = cb,
> +             .idx = *idx,
> +     };
> +     struct dsa_slave_priv *p = netdev_priv(dev);
> +     struct dsa_port *dp = p->dp;
> +     struct dsa_switch *ds = dp->ds;
> +     int err;
> +
> +     if (!ds->ops->port_fdb_dump) {
> +             err = -EOPNOTSUPP;
> +             goto out;
> +     }
> +
> +     err = ds->ops->port_fdb_dump(ds, dp->index, &dump);
> +out:
> +     *idx = dump.idx;
> +     return err;

There is no need to reassign *idx in case of error:

    if (!ds->ops->port_fdb_dump)
        return -EOPNOTSUPP;

    err = ds->ops->port_fdb_dump(ds, dp->index, &dump);
    *idx = dump.idx;

    return err;

> +     .ndo_fdb_dump           = dsa_slave_port_fdb_dump,

And s/dsa_slave_port_fdb_dump/dsa_slave_fdb_dump/ here will be even
better ;-)


Thanks,

        Vivien