| Message ID | 1368020717-22194-4-git-send-email-ogerlitz@mellanox.com |
|---|---|
| State | RFC, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
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
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
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
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
> 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
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();