Patchwork [RFC,iproute2] Add VF link state control

login
register
mail settings
Submitter Or Gerlitz
Date May 8, 2013, 1:45 p.m.
Message ID <1368020717-22194-4-git-send-email-ogerlitz@mellanox.com>
Download mbox | patch
Permalink /patch/242580/
State RFC
Delegated to: stephen hemminger
Headers show

Comments

Or Gerlitz - May 8, 2013, 1:45 p.m.
From: Rony Efraim <ronye@mellanox.com>

Add link state per VF command

Signed-off-by: Rony Efraim <ronye@mellanox.com>
---
 include/linux/if_link.h |   13 +++++++++++++
 ip/iplink.c             |   14 ++++++++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)
stephen hemminger - May 8, 2013, 4:17 p.m.
On Wed,  8 May 2013 16:45:17 +0300
Or Gerlitz <ogerlitz@mellanox.com> wrote:

> From: Rony Efraim <ronye@mellanox.com>
> 
> Add link state per VF command
> 
> Signed-off-by: Rony Efraim <ronye@mellanox.com>

Isn't this redundant with OPERSTATE and LOWER_DOWN?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams - May 8, 2013, 4:24 p.m.
On Wed, 2013-05-08 at 09:17 -0700, Stephen Hemminger wrote:
> On Wed,  8 May 2013 16:45:17 +0300
> Or Gerlitz <ogerlitz@mellanox.com> wrote:
> 
> > From: Rony Efraim <ronye@mellanox.com>
> > 
> > Add link state per VF command
> > 
> > Signed-off-by: Rony Efraim <ronye@mellanox.com>
> 
> Isn't this redundant with OPERSTATE and LOWER_DOWN?

I was going to say it was mostly redundant with the "set carrier from
userspace" patches from jpirko last December, but since a VF doesn't
appear to always have a netdev, it seems that functionality has to be
special-cased for VFs instead of being generic :(

Dan

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
John Fastabend - May 8, 2013, 4:44 p.m.
On 5/8/2013 9:24 AM, Dan Williams wrote:
> On Wed, 2013-05-08 at 09:17 -0700, Stephen Hemminger wrote:
>> On Wed,  8 May 2013 16:45:17 +0300
>> Or Gerlitz <ogerlitz@mellanox.com> wrote:
>>
>>> From: Rony Efraim <ronye@mellanox.com>
>>>
>>> Add link state per VF command
>>>
>>> Signed-off-by: Rony Efraim <ronye@mellanox.com>
>>
>> Isn't this redundant with OPERSTATE and LOWER_DOWN?
>
> I was going to say it was mostly redundant with the "set carrier from
> userspace" patches from jpirko last December, but since a VF doesn't
> appear to always have a netdev, it seems that functionality has to be
> special-cased for VFs instead of being generic :(
>
> Dan
>

Or the netdev is direct assigned to some VM/namespace or otherwise out
of scope.

It does seem unfortunate though that every time we want a feature that
already exists to be applicable for a VF we have to go through this
exercise of adding an ndo op and adding lookup code in each and
every driver to find the VF and pass messages back and forth.

.John

> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Or Gerlitz - May 8, 2013, 5:31 p.m.
On Wed, May 8, 2013 at 7:44 PM, John Fastabend
<john.r.fastabend@intel.com> wrote:
> It does seem unfortunate though that every time we want a feature that
> already exists to be applicable for a VF we have to go through this
> exercise of adding an ndo op and adding lookup code in each and
> every driver to find the VF and pass messages back and forth.

agree, how about introducing ndo_set_vf_config which we can extend to
include new attrbiutes each time we want to add them...  actually do
we have robust way to extend
the existing ifla_vf_info? I guess so

int (*ndo_get_vf_config)(struct net_device *dev, int vf, struct
ifla_vf_info *ivf);

these two calls are also there but only the enic driver uses them

int (*ndo_set_vf_port)(struct net_device *dev, int vf, struct nlattr *port[]);
int (*ndo_get_vf_port)(struct net_device *dev, int vf, struct sk_buff *skb);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Or Gerlitz - May 9, 2013, 8:34 a.m.
> John Fastabend <john.r.fastabend@intel.com> wrote:
> > It does seem unfortunate though that every time we want a feature that
> > already exists to be applicable for a VF we have to go through this
> > exercise of adding an ndo op and adding lookup code in each and
> > every driver to find the VF and pass messages back and forth.
>
> agree, how about introducing ndo_set_vf_config which we can extend to
> include new attrbiutes each time we want to add them...  actually do
> we have robust way to extend the existing ifla_vf_info? I guess so
>
> int (*ndo_get_vf_config)(struct net_device *dev, int vf, struct ifla_vf_info *ivf);

Do we lean towards introducing new call/s to set/get VF config and can
be extended to new features as they come  - e.g
control VF link state, read VF packet/byte counters, set vlan egress
map for the VF when its in VGT mode, etc, or
stick to the current method of one ndo per need?


>
> these two calls are also there but only the enic driver uses them
>
> int (*ndo_set_vf_port)(struct net_device *dev, int vf, struct nlattr *port[]);
> int (*ndo_get_vf_port)(struct net_device *dev, int vf, struct sk_buff *skb);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index 965dc9f..f01f691 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -334,6 +334,7 @@  enum {
 	IFLA_VF_VLAN,
 	IFLA_VF_TX_RATE,	/* TX Bandwidth Allocation */
 	IFLA_VF_SPOOFCHK,	/* Spoof Checking on/off switch */
+	IFLA_VF_LINK_STATE,     /* link state on/off switch */
 	__IFLA_VF_MAX,
 };
 
@@ -360,6 +361,18 @@  struct ifla_vf_spoofchk {
 	__u32 setting;
 };
 
+enum {
+	IFLA_VF_LINK_STATE_AUTO,	/* link state of the uplink */
+	IFLA_VF_LINK_STATE_ENABLE,	/* link always up */
+	IFLA_VF_LINK_STATE_DISABLE,	/* link always down */
+	__IFLA_VF_LINK_STATE_MAX,
+};
+
+struct ifla_vf_link_state {
+	__u32 vf;
+	__u32 link_state;
+};
+
 /* VF ports management section
  *
  *	Nested layout of set/get msg is:
diff --git a/ip/iplink.c b/ip/iplink.c
index dc98019..ada9d42 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -77,6 +77,7 @@  void iplink_usage(void)
 	fprintf(stderr, "				   [ rate TXRATE ] ] \n");
 
 	fprintf(stderr, "				   [ spoofchk { on | off} ] ] \n");
+	fprintf(stderr, "				   [ state { auto | enable | disable} ] ]\n");
 	fprintf(stderr, "			  [ master DEVICE ]\n");
 	fprintf(stderr, "			  [ nomaster ]\n");
 	fprintf(stderr, "       ip link show [ DEVICE | group GROUP ] [up]\n");
@@ -255,6 +256,19 @@  static int iplink_parse_vf(int vf, int *argcp, char ***argvp,
 			ivs.vf = vf;
 			addattr_l(&req->n, sizeof(*req), IFLA_VF_SPOOFCHK, &ivs, sizeof(ivs));
 
+		} else if (matches(*argv, "state") == 0) {
+			struct ifla_vf_link_state ivl;
+			NEXT_ARG();
+			if (matches(*argv, "auto") == 0)
+				ivl.link_state = IFLA_VF_LINK_STATE_AUTO;
+			else if (matches(*argv, "enable") == 0)
+				ivl.link_state = IFLA_VF_LINK_STATE_ENABLE;
+			else if (matches(*argv, "disable") == 0)
+				ivl.link_state = IFLA_VF_LINK_STATE_DISABLE;
+			else
+				invarg("Invalid \"state\" value\n", *argv);
+			ivl.vf = vf;
+			addattr_l(&req->n, sizeof(*req), IFLA_VF_LINK_STATE, &ivl, sizeof(ivl));
 		} else {
 			/* rewind arg */
 			PREV_ARG();