On 07/10/2017 10:36 PM, Vivien Didelot wrote:
> 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.
>
Originally thought about it. But it makes sense to pass a callback
when different functions can be supplied.
Here is only one option which is the fdb_do_dump(). So I thought
exporting it makes more sense..
>> +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;
>
Will fix, thanks.
>> + .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
>
Will fix, thanks.
