Re: [PATCH net-next 1/2] net/devlink: Add E-Switch encapsulation control

Thu, Feb 09, 2017 at 06:20:14PM CET, ogerlitz@mellanox.com wrote:

On 2/9/2017 7:10 PM, Jiri Pirko wrote:

quoted

quoted

@@ -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.

quoted

quoted

+			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.