| Message ID | 20190221175436.10767-1-sthemmin@microsoft.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | [RFC] rtnetlink: handle multiple vlan tags in set_vf_vlan | expand |
On Thu, 21 Feb 2019 09:54:36 -0800 Stephen Hemminger <stephen@networkplumber.org> wrote: > @@ -2224,13 +2223,15 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb) > > len++; > } > - if (len == 0) > - return -EINVAL; > > - err = ops->ndo_set_vf_vlan(dev, ivvl[0]->vf, ivvl[0]->vlan, > - ivvl[0]->qos, ivvl[0]->vlan_proto); > - if (err < 0) > - return err; > + err = -EINVAL; /* empty list error */ > + for (i = 0; i < len; i++) { > + err = ops->ndo_set_vf_vlan(dev, ivvl[i]->vf, > + ivvl[i]->vlan, ivvl[i]->qos, > + ivvl[i]->vlan_proto); > + if (err < 0) > + return err; > + } I think the: if (err < 0) return err; should be outside the loop (with a "break;" inside), otherwise you won't return anymore if len == 0.
On Thu, 21 Feb 2019 19:34:57 +0100 Stefano Brivio <sbrivio@redhat.com> wrote: > On Thu, 21 Feb 2019 09:54:36 -0800 > Stephen Hemminger <stephen@networkplumber.org> wrote: > > > @@ -2224,13 +2223,15 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb) > > > > len++; > > } > > - if (len == 0) > > - return -EINVAL; > > > > - err = ops->ndo_set_vf_vlan(dev, ivvl[0]->vf, ivvl[0]->vlan, > > - ivvl[0]->qos, ivvl[0]->vlan_proto); > > - if (err < 0) > > - return err; > > + err = -EINVAL; /* empty list error */ > > + for (i = 0; i < len; i++) { > > + err = ops->ndo_set_vf_vlan(dev, ivvl[i]->vf, > > + ivvl[i]->vlan, ivvl[i]->qos, > > + ivvl[i]->vlan_proto); > > + if (err < 0) > > + return err; > > + } > > I think the: > > if (err < 0) > return err; > > should be outside the loop (with a "break;" inside), otherwise you won't > return anymore if len == 0. > Your right with empty list it would fall through and look at other attributes which could overwrite err.
On 2/21/2019 7:54 PM, Stephen Hemminger wrote: > The netlink API for IFLA_VF_VLAN_LIST allows multiple VLAN tags to > be passed (and the message was validated) but only the first VLAN > tag was being passed to the device. Change to iterate over each tag received. > > Fixes: 79aab093a0b5 ("net: Update API for VF vlan protocol 802.1ad support") > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > --- > net/core/rtnetlink.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index a51cab95ba64..3a9ec988ae21 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -2207,11 +2207,10 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb) > if (tb[IFLA_VF_VLAN_LIST]) { > struct ifla_vf_vlan_info *ivvl[MAX_VLAN_LIST_LEN]; > struct nlattr *attr; > - int rem, len = 0; > + int i, rem, len = 0; > > - err = -EOPNOTSUPP; > if (!ops->ndo_set_vf_vlan) > - return err; > + return -EOPNOTSUPP; > > nla_for_each_nested(attr, tb[IFLA_VF_VLAN_LIST], rem) { > if (nla_type(attr) != IFLA_VF_VLAN_INFO || > @@ -2224,13 +2223,15 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb) > > len++; > } > - if (len == 0) > - return -EINVAL; > > - err = ops->ndo_set_vf_vlan(dev, ivvl[0]->vf, ivvl[0]->vlan, > - ivvl[0]->qos, ivvl[0]->vlan_proto); > - if (err < 0) > - return err; > + err = -EINVAL; /* empty list error */ > + for (i = 0; i < len; i++) { > + err = ops->ndo_set_vf_vlan(dev, ivvl[i]->vf, > + ivvl[i]->vlan, ivvl[i]->qos, > + ivvl[i]->vlan_proto); Doing that each vlan will just overwrite the vf vlan configuration set by its preceding one. Note #define MAX_VLAN_LIST_LEN 1 The point here was that I had to add the rtnl interface to set vf vlan with option to set vf vlan protocol. While doing that I was asked to add option to get a list of vlans from user to support QinQ, so once it will be needed there won't be a need to add another rtnl interface. The driver which will support setting double vlan or more per vf will change MAX_VLAN_LIST_LEN and change the ndo function. > + if (err < 0) > + return err; > + } > } > > if (tb[IFLA_VF_TX_RATE]) { >
On Fri, 22 Feb 2019 04:58:06 +0000 Moshe Shemesh <moshe@mellanox.com> wrote: > On 2/21/2019 7:54 PM, Stephen Hemminger wrote: > > The netlink API for IFLA_VF_VLAN_LIST allows multiple VLAN tags to > > be passed (and the message was validated) but only the first VLAN > > tag was being passed to the device. Change to iterate over each tag received. > > > > Fixes: 79aab093a0b5 ("net: Update API for VF vlan protocol 802.1ad support") > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > > --- > > net/core/rtnetlink.c | 19 ++++++++++--------- > > 1 file changed, 10 insertions(+), 9 deletions(-) > > > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > > index a51cab95ba64..3a9ec988ae21 100644 > > --- a/net/core/rtnetlink.c > > +++ b/net/core/rtnetlink.c > > @@ -2207,11 +2207,10 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb) > > if (tb[IFLA_VF_VLAN_LIST]) { > > struct ifla_vf_vlan_info *ivvl[MAX_VLAN_LIST_LEN]; > > struct nlattr *attr; > > - int rem, len = 0; > > + int i, rem, len = 0; > > > > - err = -EOPNOTSUPP; > > if (!ops->ndo_set_vf_vlan) > > - return err; > > + return -EOPNOTSUPP; > > > > nla_for_each_nested(attr, tb[IFLA_VF_VLAN_LIST], rem) { > > if (nla_type(attr) != IFLA_VF_VLAN_INFO || > > @@ -2224,13 +2223,15 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb) > > > > len++; > > } > > - if (len == 0) > > - return -EINVAL; > > > > - err = ops->ndo_set_vf_vlan(dev, ivvl[0]->vf, ivvl[0]->vlan, > > - ivvl[0]->qos, ivvl[0]->vlan_proto); > > - if (err < 0) > > - return err; > > + err = -EINVAL; /* empty list error */ > > + for (i = 0; i < len; i++) { > > + err = ops->ndo_set_vf_vlan(dev, ivvl[i]->vf, > > + ivvl[i]->vlan, ivvl[i]->qos, > > + ivvl[i]->vlan_proto); > > Doing that each vlan will just overwrite the vf vlan configuration set > by its preceding one. > Note #define MAX_VLAN_LIST_LEN 1 > The point here was that I had to add the rtnl interface to set vf vlan > with option to set vf vlan protocol. While doing that I was asked to add > option to get a list of vlans from user to support QinQ, so once it will > be needed there won't be a need to add another rtnl interface. > > The driver which will support setting double vlan or more per vf will > change MAX_VLAN_LIST_LEN and change the ndo function. > OK, then the original code kind of makes sense now. Maybe the whole loop could be eliminated. I expected that the vlan list was for allowing multiple vlan's to go to a single VF but it is really a future (unlikely to ever be implemented) support for QinQ.
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index a51cab95ba64..3a9ec988ae21 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -2207,11 +2207,10 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb) if (tb[IFLA_VF_VLAN_LIST]) { struct ifla_vf_vlan_info *ivvl[MAX_VLAN_LIST_LEN]; struct nlattr *attr; - int rem, len = 0; + int i, rem, len = 0; - err = -EOPNOTSUPP; if (!ops->ndo_set_vf_vlan) - return err; + return -EOPNOTSUPP; nla_for_each_nested(attr, tb[IFLA_VF_VLAN_LIST], rem) { if (nla_type(attr) != IFLA_VF_VLAN_INFO || @@ -2224,13 +2223,15 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb) len++; } - if (len == 0) - return -EINVAL; - err = ops->ndo_set_vf_vlan(dev, ivvl[0]->vf, ivvl[0]->vlan, - ivvl[0]->qos, ivvl[0]->vlan_proto); - if (err < 0) - return err; + err = -EINVAL; /* empty list error */ + for (i = 0; i < len; i++) { + err = ops->ndo_set_vf_vlan(dev, ivvl[i]->vf, + ivvl[i]->vlan, ivvl[i]->qos, + ivvl[i]->vlan_proto); + if (err < 0) + return err; + } } if (tb[IFLA_VF_TX_RATE]) {
The netlink API for IFLA_VF_VLAN_LIST allows multiple VLAN tags to be passed (and the message was validated) but only the first VLAN tag was being passed to the device. Change to iterate over each tag received. Fixes: 79aab093a0b5 ("net: Update API for VF vlan protocol 802.1ad support") Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- net/core/rtnetlink.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)