Thu, Feb 09, 2017 at 06:20:14PM CET, ogerl...@mellanox.com wrote:
>On 2/9/2017 7:10 PM, Jiri Pirko wrote:
>> > @@ -1470,11 +1480,23 @@ static int devlink_nl_cmd_eswitch_set_doit(struct
>> > sk_buff *skb,
>> > const struct devlink_ops *ops = devlink->ops;
>> > u16 mode;
>> > u8 inline_mode;
>> > + bool encap;
>> > int err = 0;
>> >
>> > if (!ops)
>> > return -EOPNOTSUPP;
>> >
>> > + if (info->attrs[DEVLINK_ATTR_ESWITCH_ENCAP]) {
>> > + if (!ops->eswitch_encap_set)
>> > + return -EOPNOTSUPP;
>> > + if (!info->attrs[DEVLINK_ATTR_ESWITCH_MODE])
Why you need to check this? Should be possible to set the attrs
separatelly.
>> > + return -EINVAL;
>> > + encap = nla_get_u8(info->attrs[DEVLINK_ATTR_ESWITCH_ENCAP]);
>> > + err = ops->eswitch_encap_set(devlink, encap);
>> > + if (err)
>> > + return err;
>> > + }
>> Please maintain the same order as the attr enum and getters and put this
>> behind the inline_mode.
>
>The HW driver might want to use the attributes only when they are called to
>change the eswitch mode.
I don't get it. Should be a separate attribute - separate op
>
>I see that the patch is not too consistent with setting the inline mode which
>is done after setting the eswitch mode, mmm, guess we need to fix that.
>
>Or.
>
>
>
