| Message ID | 20100514013526.1816.45104.stgit@savbu-pc100.cisco.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Scott Feldman wrote: > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -653,6 +653,26 @@ static inline int rtnl_vfinfo_size(const struct net_device *dev) > return 0; > } > > +static size_t rtnl_vf_port_size(const struct net_device *dev) > +{ > + size_t vf_port_size = nla_total_size(sizeof(struct nlattr)) > + /* VF_PORT_VF */ > + + nla_total_size(VF_PORT_PROFILE_MAX)/* VF_PORT_PROFILE */ > + + nla_total_size(sizeof(struct ifla_vf_port_vsi)) > + /* VF_PORT_VSI_TYPE */ > + + nla_total_size(VF_PORT_UUID_MAX) /* VF_PORT_VSI_INSTANCE */ > + + nla_total_size(VF_PORT_UUID_MAX) /* VF_PORT_HOST_UUID */ > + + nla_total_size(1) /* VF_PROT_VDP_REQUEST */ Do messages generated by the kernel really contain a request? > + + nla_total_size(2); /* VF_PORT_VDP_RESPONSE */ > + > + if (!dev->netdev_ops->ndo_get_vf_port || !dev->dev.parent) > + return 0; > + if (dev_num_vf(dev->dev.parent)) > + return vf_port_size * dev_num_vf(dev->dev.parent); > + else > + return vf_port_size; > +} > + > +static int rtnl_vf_port_fill_nest(struct sk_buff *skb, struct net_device *dev, > + int vf) > +{ > + struct nlattr *data; > + int err; > + > + data = nla_nest_start(skb, IFLA_VF_PORT); We usually use a top-level attribute to encapsulate lists of identical attributes. The other iflink attributes may only occur once and are usually parsed using nla_parse_nested(), which will parse all IFLA_VF_PORT attributes, but only return the last one. Something like: iflink message: ... [IFLA_VF_PORTS] [IFLA_VF_PORT] [IFLA_VF_PORT_*], ... [IFLA_VF_PORT] [IFLA_VF_PORT_*], ... ... > + if (!data) > + return -EMSGSIZE; > + > + if (vf != VF_PORT_VF_NOT_USED) > + nla_put_u32(skb, IFLA_VF_PORT_VF, vf); This should be checking for errors or use NLA_PUT_U32. > + > + err = dev->netdev_ops->ndo_get_vf_port(dev, vf, skb); > + if (err) { > + nla_nest_cancel(skb, data); > + return err; > + } > + > + nla_nest_end(skb, data); > + > + return 0; > +} > + > static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev, > int type, u32 pid, u32 seq, u32 change, > unsigned int flags) > @@ -747,17 +819,23 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev, > goto nla_put_failure; > copy_rtnl_link_stats64(nla_data(attr), stats); > > + if (dev->dev.parent) > + NLA_PUT_U32(skb, IFLA_NUM_VF, dev_num_vf(dev->dev.parent)); Just wondering, is the only case where dev.parent is non-NULL really when virtual ports are present? > + > if (dev->netdev_ops->ndo_get_vf_config && dev->dev.parent) { > int i; > struct ifla_vf_info ivi; > > - NLA_PUT_U32(skb, IFLA_NUM_VF, dev_num_vf(dev->dev.parent)); > for (i = 0; i < dev_num_vf(dev->dev.parent); i++) { > if (dev->netdev_ops->ndo_get_vf_config(dev, i, &ivi)) > break; > NLA_PUT(skb, IFLA_VFINFO, sizeof(ivi), &ivi); > } > } > + > + if (rtnl_vf_port_fill(skb, dev)) > + goto nla_put_failure; > + > if (dev->rtnl_link_ops) { > if (rtnl_link_fill(skb, dev) < 0) > goto nla_put_failure; > @@ -824,6 +902,7 @@ const struct nla_policy ifla_policy[IFLA_MAX+1] = { > .len = sizeof(struct ifla_vf_vlan) }, > [IFLA_VF_TX_RATE] = { .type = NLA_BINARY, > .len = sizeof(struct ifla_vf_tx_rate) }, > + [IFLA_VF_PORT] = { .type = NLA_NESTED }, > }; > EXPORT_SYMBOL(ifla_policy); > > @@ -832,6 +911,20 @@ static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = { > [IFLA_INFO_DATA] = { .type = NLA_NESTED }, > }; > > +static const struct nla_policy ifla_vf_port_policy[IFLA_VF_PORT_MAX+1] = { > + [IFLA_VF_PORT_VF] = { .type = NLA_U32 }, > + [IFLA_VF_PORT_PROFILE] = { .type = NLA_STRING, > + .len = VF_PORT_PROFILE_MAX }, > + [IFLA_VF_PORT_VSI_TYPE] = { .type = NLA_BINARY, > + .len = sizeof(struct ifla_vf_port_vsi)}, > + [IFLA_VF_PORT_INSTANCE_UUID]= { .type = NLA_BINARY, > + .len = VF_PORT_UUID_MAX }, > + [IFLA_VF_PORT_HOST_UUID] = { .type = NLA_STRING, > + .len = VF_PORT_UUID_MAX }, > + [IFLA_VF_PORT_REQUEST] = { .type = NLA_U8, }, > + [IFLA_VF_PORT_RESPONSE] = { .type = NLA_U16, }, > +}; > + > struct net *rtnl_link_get_net(struct net *src_net, struct nlattr *tb[]) > { > struct net *net; > @@ -1028,6 +1121,27 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm, > } > err = 0; > > + if (tb[IFLA_VF_PORT]) { > + struct nlattr *vf_port[IFLA_VF_PORT_MAX+1]; > + int vf = VF_PORT_VF_NOT_USED; > + > + err = nla_parse_nested(vf_port, IFLA_VF_PORT_MAX, > + tb[IFLA_VF_PORT], ifla_vf_port_policy); > + if (err < 0) > + goto errout; > + > + if (vf_port[IFLA_VF_PORT_VF]) > + vf = nla_get_u32(vf_port[IFLA_VF_PORT_VF]); > + > + err = -EOPNOTSUPP; > + if (ops->ndo_set_vf_port) > + err = ops->ndo_set_vf_port(dev, vf, vf_port); > + if (err < 0) > + goto errout; > + modified = 1; > + } > + err = 0; > + > errout: > if (err < 0 && modified && net_ratelimit()) > printk(KERN_WARNING "A link change request failed with " > -- 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 Friday 14 May 2010, Patrick McHardy wrote: > Scott Feldman wrote: > > --- a/net/core/rtnetlink.c > > +++ b/net/core/rtnetlink.c > > @@ -653,6 +653,26 @@ static inline int rtnl_vfinfo_size(const struct net_device *dev) > > return 0; > > } > > > > +static size_t rtnl_vf_port_size(const struct net_device *dev) > > +{ > > + size_t vf_port_size = nla_total_size(sizeof(struct nlattr)) > > + /* VF_PORT_VF */ > > + + nla_total_size(VF_PORT_PROFILE_MAX)/* VF_PORT_PROFILE */ > > + + nla_total_size(sizeof(struct ifla_vf_port_vsi)) > > + /* VF_PORT_VSI_TYPE */ > > + + nla_total_size(VF_PORT_UUID_MAX) /* VF_PORT_VSI_INSTANCE */ > > + + nla_total_size(VF_PORT_UUID_MAX) /* VF_PORT_HOST_UUID */ > > + + nla_total_size(1) /* VF_PROT_VDP_REQUEST */ > > Do messages generated by the kernel really contain a request? Yes, the request field of the VDP message shows the status (e.g. associated or disassociated). > > +static int rtnl_vf_port_fill_nest(struct sk_buff *skb, struct net_device *dev, > > + int vf) > > +{ > > + struct nlattr *data; > > + int err; > > + > > + data = nla_nest_start(skb, IFLA_VF_PORT); > > We usually use a top-level attribute to encapsulate lists of identical > attributes. The other iflink attributes may only occur once and are > usually parsed using nla_parse_nested(), which will parse all > IFLA_VF_PORT attributes, but only return the last one. > > Something like: > > iflink message: > ... > [IFLA_VF_PORTS] > [IFLA_VF_PORT] > [IFLA_VF_PORT_*], ... > [IFLA_VF_PORT] > [IFLA_VF_PORT_*], ... > ... Ah, I was wondering about this already. Does this mean that IFLA_VFINFO does this incorrectly as well? > > static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev, > > int type, u32 pid, u32 seq, u32 change, > > unsigned int flags) > > @@ -747,17 +819,23 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev, > > goto nla_put_failure; > > copy_rtnl_link_stats64(nla_data(attr), stats); > > > > + if (dev->dev.parent) > > + NLA_PUT_U32(skb, IFLA_NUM_VF, dev_num_vf(dev->dev.parent)); > > Just wondering, is the only case where dev.parent is non-NULL > really when virtual ports are present? No, but if parent is NULL, we must not call dev_num_vf(). The way that enic needs the attributes, they can be either for the VF of dev->dev.parent (the PCI PF), or for the PF itself, even if it does not have VFs, in which case it would be interesting to have IFLA_NUM_VF = 0 in the output. Maybe a better structure would be to separate the two cases, also allowing a port profile to be associated with both the PF and with each of its VFs? Something like this: [IFLA_NUM_VF] [IFLA_VF_PORTS] [IFLA_VF_PORT] [IFLA_VF_PORT_*], ... [IFLA_VF_PORT] [IFLA_VF_PORT_*], ... [IFLA_PORT_SELF] [IFLA_VF_PORT_*], ... Arnd -- 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
Arnd Bergmann wrote: > On Friday 14 May 2010, Patrick McHardy wrote: >>> +static int rtnl_vf_port_fill_nest(struct sk_buff *skb, struct net_device *dev, >>> + int vf) >>> +{ >>> + struct nlattr *data; >>> + int err; >>> + >>> + data = nla_nest_start(skb, IFLA_VF_PORT); >> We usually use a top-level attribute to encapsulate lists of identical >> attributes. The other iflink attributes may only occur once and are >> usually parsed using nla_parse_nested(), which will parse all >> IFLA_VF_PORT attributes, but only return the last one. >> >> Something like: >> >> iflink message: >> ... >> [IFLA_VF_PORTS] >> [IFLA_VF_PORT] >> [IFLA_VF_PORT_*], ... >> [IFLA_VF_PORT] >> [IFLA_VF_PORT_*], ... >> ... > > Ah, I was wondering about this already. Does this mean that IFLA_VFINFO > does this incorrectly as well? Yes. >>> static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev, >>> int type, u32 pid, u32 seq, u32 change, >>> unsigned int flags) >>> @@ -747,17 +819,23 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev, >>> goto nla_put_failure; >>> copy_rtnl_link_stats64(nla_data(attr), stats); >>> >>> + if (dev->dev.parent) >>> + NLA_PUT_U32(skb, IFLA_NUM_VF, dev_num_vf(dev->dev.parent)); >> Just wondering, is the only case where dev.parent is non-NULL >> really when virtual ports are present? > > No, but if parent is NULL, we must not call dev_num_vf(). The way that enic > needs the attributes, they can be either for the VF of dev->dev.parent (the > PCI PF), or for the PF itself, even if it does not have VFs, in which case > it would be interesting to have IFLA_NUM_VF = 0 in the output. I see. I was mainly wondering about completely different types of devices. > Maybe a better structure would be to separate the two cases, also allowing > a port profile to be associated with both the PF and with each of its VFs? > > Something like this: > > [IFLA_NUM_VF] > [IFLA_VF_PORTS] > [IFLA_VF_PORT] > [IFLA_VF_PORT_*], ... > [IFLA_VF_PORT] > [IFLA_VF_PORT_*], ... > [IFLA_PORT_SELF] > [IFLA_VF_PORT_*], ... That would also be fine. -- 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/14/10 9:42 AM, "Patrick McHardy" <kaber@trash.net> wrote: > Arnd Bergmann wrote: >> Maybe a better structure would be to separate the two cases, also allowing >> a port profile to be associated with both the PF and with each of its VFs? >> >> Something like this: >> >> [IFLA_NUM_VF] >> [IFLA_VF_PORTS] >> [IFLA_VF_PORT] >> [IFLA_VF_PORT_*], ... >> [IFLA_VF_PORT] >> [IFLA_VF_PORT_*], ... >> [IFLA_PORT_SELF] >> [IFLA_VF_PORT_*], ... > > That would also be fine. I want to make sure I've got this right before starting on ver8 of patch: - we'll use the layout listed above - RTM_SETLINK msg includes the full nested layout - contains IFLA_VF_PORTs for all VFs of a PF - OR, contains IFLA_PORT_SELF if PF is it's own VF - it's up to the receiver to compare for changes for each VF - RTM_GETLINK msg includes the full nested layout - same rules as RTM_SETLINK above I think we should redo the other IFLA_VF_xxx msgs in the same style. I'm not going to tackle that for IFLA_VF_PORTS patch, but it would be a good followup patch. Do we have a plan? -scott -- 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
Scott Feldman wrote: > On 5/14/10 9:42 AM, "Patrick McHardy" <kaber@trash.net> wrote: > >> Arnd Bergmann wrote: >>> Maybe a better structure would be to separate the two cases, also allowing >>> a port profile to be associated with both the PF and with each of its VFs? >>> >>> Something like this: >>> >>> [IFLA_NUM_VF] >>> [IFLA_VF_PORTS] >>> [IFLA_VF_PORT] >>> [IFLA_VF_PORT_*], ... >>> [IFLA_VF_PORT] >>> [IFLA_VF_PORT_*], ... >>> [IFLA_PORT_SELF] >>> [IFLA_VF_PORT_*], ... >> That would also be fine. > > I want to make sure I've got this right before starting on ver8 of patch: > > - we'll use the layout listed above > > - RTM_SETLINK msg includes the full nested layout > > - contains IFLA_VF_PORTs for all VFs of a PF > - OR, contains IFLA_PORT_SELF if PF is it's own VF > > - it's up to the receiver to compare for changes for each VF > > - RTM_GETLINK msg includes the full nested layout > > - same rules as RTM_SETLINK above > > I think we should redo the other IFLA_VF_xxx msgs in the same style. I'm > not going to tackle that for IFLA_VF_PORTS patch, but it would be a good > followup patch. Agreed. > Do we have a plan? That sounds good to me. If you have any netlink related questions, just let me know. -- 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 Friday 14 May 2010 19:19:00 Scott Feldman wrote: > I want to make sure I've got this right before starting on ver8 of patch: > > - we'll use the layout listed above > > - RTM_SETLINK msg includes the full nested layout > > - contains IFLA_VF_PORTs for all VFs of a PF > - OR, contains IFLA_PORT_SELF if PF is it's own VF > > - it's up to the receiver to compare for changes for each VF > > - RTM_GETLINK msg includes the full nested layout > > - same rules as RTM_SETLINK above I was thinking that a device could have both IFLA_VF_PORTS and IFLA_PORT_SELF, but you know more about the IOV specifics. If an adapter having multiple VFs always gets configured as VF 0 itself, that would be fine as well, otherwise we could have an extra argument to the two device driver callbacks to differentiate VF/SELF. As long as this does not impact the user ABI, we could do either. > I think we should redo the other IFLA_VF_xxx msgs in the same style. I'm > not going to tackle that for IFLA_VF_PORTS patch, but it would be a good > followup patch. I fear it's too late for that now. While we have not yet released 2.6.34 and 2.6.33 does not contain the broken message, it's extremely late in the stabilization phase of v2.6.34, so I doubt that there is still a chance for that at this point. Arnd -- 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
* Scott Feldman (scofeldm@cisco.com) wrote: > On 5/14/10 9:42 AM, "Patrick McHardy" <kaber@trash.net> wrote: > > > Arnd Bergmann wrote: > >> Maybe a better structure would be to separate the two cases, also allowing > >> a port profile to be associated with both the PF and with each of its VFs? > >> > >> Something like this: > >> > >> [IFLA_NUM_VF] > >> [IFLA_VF_PORTS] > >> [IFLA_VF_PORT] > >> [IFLA_VF_PORT_*], ... > >> [IFLA_VF_PORT] > >> [IFLA_VF_PORT_*], ... > >> [IFLA_PORT_SELF] > >> [IFLA_VF_PORT_*], ... > > > > That would also be fine. > > I want to make sure I've got this right before starting on ver8 of patch: > > - we'll use the layout listed above > > - RTM_SETLINK msg includes the full nested layout > > - contains IFLA_VF_PORTs for all VFs of a PF > - OR, contains IFLA_PORT_SELF if PF is it's own VF > > - it's up to the receiver to compare for changes for each VF > > - RTM_GETLINK msg includes the full nested layout > > - same rules as RTM_SETLINK above > > I think we should redo the other IFLA_VF_xxx msgs in the same style. I'm > not going to tackle that for IFLA_VF_PORTS patch, but it would be a good > followup patch. Patrick layed out some nice details before. Here's the link: http://thread.gmane.org/gmane.linux.network/151605/focus=151738 thanks, -chris -- 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/14/10 10:29 AM, "Arnd Bergmann" <arnd@arndb.de> wrote: > On Friday 14 May 2010 19:19:00 Scott Feldman wrote: >> I want to make sure I've got this right before starting on ver8 of patch: >> >> - we'll use the layout listed above >> >> - RTM_SETLINK msg includes the full nested layout >> >> - contains IFLA_VF_PORTs for all VFs of a PF >> - OR, contains IFLA_PORT_SELF if PF is it's own VF >> >> - it's up to the receiver to compare for changes for each VF >> >> - RTM_GETLINK msg includes the full nested layout >> >> - same rules as RTM_SETLINK above > > I was thinking that a device could have both IFLA_VF_PORTS and IFLA_PORT_SELF, > but you know more about the IOV specifics. If an adapter having multiple > VFs always gets configured as VF 0 itself, that would be fine as well, > otherwise > we could have an extra argument to the two device driver callbacks to > differentiate VF/SELF. As long as this does not impact the user ABI, we > could do either. I think you're right. I should have said AND/OR. I would rather not have an extra argument to the driver callbacks. >> I think we should redo the other IFLA_VF_xxx msgs in the same style. I'm >> not going to tackle that for IFLA_VF_PORTS patch, but it would be a good >> followup patch. > > I fear it's too late for that now. While we have not yet released 2.6.34 > and 2.6.33 does not contain the broken message, it's extremely late in the > stabilization phase of v2.6.34, so I doubt that there is still a chance for > that at this point. That's too bad. I wish Patrick's objections were honored and then we wouldn't have followed that broken model! Can the broken msgs be disabled somehow for 2.6.34? Keep the definitions in if_link.h but fail the SET/GET actions in rtnetlink.c? -scott -- 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/14/10 10:35 AM, "Chris Wright" <chrisw@redhat.com> wrote: > Patrick layed out some nice details before. Here's the link: > > http://thread.gmane.org/gmane.linux.network/151605/focus=151738 Double drats, it looks like that one was caught too late. So we're collectively agreeing to let a known bad netlink msg in? I guess it can be fixed up later with a IFLA_VF_INFOS nest, and move away from the broken msgs. -scott -- 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
Scott Feldman wrote: > On 5/14/10 10:29 AM, "Arnd Bergmann" <arnd@arndb.de> wrote: > >> I was thinking that a device could have both IFLA_VF_PORTS and IFLA_PORT_SELF, >> but you know more about the IOV specifics. If an adapter having multiple >> VFs always gets configured as VF 0 itself, that would be fine as well, >> otherwise >> we could have an extra argument to the two device driver callbacks to >> differentiate VF/SELF. As long as this does not impact the user ABI, we >> could do either. > > I think you're right. I should have said AND/OR. I would rather not have > an extra argument to the driver callbacks. > >>> I think we should redo the other IFLA_VF_xxx msgs in the same style. I'm >>> not going to tackle that for IFLA_VF_PORTS patch, but it would be a good >>> followup patch. >> I fear it's too late for that now. While we have not yet released 2.6.34 >> and 2.6.33 does not contain the broken message, it's extremely late in the >> stabilization phase of v2.6.34, so I doubt that there is still a chance for >> that at this point. > > That's too bad. I wish Patrick's objections were honored and then we > wouldn't have followed that broken model! Can the broken msgs be disabled > somehow for 2.6.34? Keep the definitions in if_link.h but fail the SET/GET > actions in rtnetlink.c? That would be a possibility. Unfortunately I don't think we can fix this in a backwards compatible way. -- 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
* Patrick McHardy (kaber@trash.net) wrote: > Scott Feldman wrote: > > On 5/14/10 10:29 AM, "Arnd Bergmann" <arnd@arndb.de> wrote: > > > >> I was thinking that a device could have both IFLA_VF_PORTS and IFLA_PORT_SELF, > >> but you know more about the IOV specifics. If an adapter having multiple > >> VFs always gets configured as VF 0 itself, that would be fine as well, > >> otherwise > >> we could have an extra argument to the two device driver callbacks to > >> differentiate VF/SELF. As long as this does not impact the user ABI, we > >> could do either. > > > > I think you're right. I should have said AND/OR. I would rather not have > > an extra argument to the driver callbacks. > > > >>> I think we should redo the other IFLA_VF_xxx msgs in the same style. I'm > >>> not going to tackle that for IFLA_VF_PORTS patch, but it would be a good > >>> followup patch. > >> I fear it's too late for that now. While we have not yet released 2.6.34 > >> and 2.6.33 does not contain the broken message, it's extremely late in the > >> stabilization phase of v2.6.34, so I doubt that there is still a chance for > >> that at this point. > > > > That's too bad. I wish Patrick's objections were honored and then we > > wouldn't have followed that broken model! Can the broken msgs be disabled > > somehow for 2.6.34? Keep the definitions in if_link.h but fail the SET/GET > > actions in rtnetlink.c? > > That would be a possibility. Unfortunately I don't think we can fix > this in a backwards compatible way. $ git describe --contains ebc08a6f47ee76ecad8e9f26c26e6ec9b46ca659 v2.6.34-rc1~233^2~336 It's not released yet? -- 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
Chris Wright wrote: > * Patrick McHardy (kaber@trash.net) wrote: >> Scott Feldman wrote: >>> On 5/14/10 10:29 AM, "Arnd Bergmann" <arnd@arndb.de> wrote: >>> >>>>> I think we should redo the other IFLA_VF_xxx msgs in the same style. I'm >>>>> not going to tackle that for IFLA_VF_PORTS patch, but it would be a good >>>>> followup patch. >>>> I fear it's too late for that now. While we have not yet released 2.6.34 >>>> and 2.6.33 does not contain the broken message, it's extremely late in the >>>> stabilization phase of v2.6.34, so I doubt that there is still a chance for >>>> that at this point. >>> That's too bad. I wish Patrick's objections were honored and then we >>> wouldn't have followed that broken model! Can the broken msgs be disabled >>> somehow for 2.6.34? Keep the definitions in if_link.h but fail the SET/GET >>> actions in rtnetlink.c? >> That would be a possibility. Unfortunately I don't think we can fix >> this in a backwards compatible way. > > $ git describe --contains ebc08a6f47ee76ecad8e9f26c26e6ec9b46ca659 > v2.6.34-rc1~233^2~336 > > It's not released yet? Correct, it was added in 2.6.34-rc. -- 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
* Patrick McHardy (kaber@trash.net) wrote: > Chris Wright wrote: > > * Patrick McHardy (kaber@trash.net) wrote: > >> Scott Feldman wrote: > >>> On 5/14/10 10:29 AM, "Arnd Bergmann" <arnd@arndb.de> wrote: > >>> > >>>>> I think we should redo the other IFLA_VF_xxx msgs in the same style. I'm > >>>>> not going to tackle that for IFLA_VF_PORTS patch, but it would be a good > >>>>> followup patch. > >>>> I fear it's too late for that now. While we have not yet released 2.6.34 > >>>> and 2.6.33 does not contain the broken message, it's extremely late in the > >>>> stabilization phase of v2.6.34, so I doubt that there is still a chance for > >>>> that at this point. > >>> That's too bad. I wish Patrick's objections were honored and then we > >>> wouldn't have followed that broken model! Can the broken msgs be disabled > >>> somehow for 2.6.34? Keep the definitions in if_link.h but fail the SET/GET > >>> actions in rtnetlink.c? > >> That would be a possibility. Unfortunately I don't think we can fix > >> this in a backwards compatible way. > > > > $ git describe --contains ebc08a6f47ee76ecad8e9f26c26e6ec9b46ca659 > > v2.6.34-rc1~233^2~336 > > > > It's not released yet? > > Correct, it was added in 2.6.34-rc. AFAICT iproute2 hasn't been released either w/ that support. So, I'll prepare patches to fix it (or disable as Scott mentioned). What do you think? thanks, -chris -- 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
Chris Wright wrote: > * Patrick McHardy (kaber@trash.net) wrote: >> Chris Wright wrote: >>> * Patrick McHardy (kaber@trash.net) wrote: >>>> Scott Feldman wrote: >>>>> On 5/14/10 10:29 AM, "Arnd Bergmann" <arnd@arndb.de> wrote: >>>>> >>>>>>> I think we should redo the other IFLA_VF_xxx msgs in the same style. I'm >>>>>>> not going to tackle that for IFLA_VF_PORTS patch, but it would be a good >>>>>>> followup patch. >>>>>> I fear it's too late for that now. While we have not yet released 2.6.34 >>>>>> and 2.6.33 does not contain the broken message, it's extremely late in the >>>>>> stabilization phase of v2.6.34, so I doubt that there is still a chance for >>>>>> that at this point. >>>>> That's too bad. I wish Patrick's objections were honored and then we >>>>> wouldn't have followed that broken model! Can the broken msgs be disabled >>>>> somehow for 2.6.34? Keep the definitions in if_link.h but fail the SET/GET >>>>> actions in rtnetlink.c? >>>> That would be a possibility. Unfortunately I don't think we can fix >>>> this in a backwards compatible way. >>> $ git describe --contains ebc08a6f47ee76ecad8e9f26c26e6ec9b46ca659 >>> v2.6.34-rc1~233^2~336 >>> >>> It's not released yet? >> Correct, it was added in 2.6.34-rc. > > AFAICT iproute2 hasn't been released either w/ that support. > So, I'll prepare patches to fix it (or disable as Scott mentioned). > What do you think? That would be great, otherwise we'll probably have to support it forever. -- 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
>-----Original Message----- >From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] >On Behalf Of Patrick McHardy >Sent: Friday, May 14, 2010 11:50 AM >To: Chris Wright >Cc: Scott Feldman; Arnd Bergmann; davem@davemloft.net; >shemminger@vyatta.com; netdev@vger.kernel.org >Subject: Re: [net-next-2.6 V7 PATCH 1/2] Add netlink support for virtual >port management (was iovnl) > >Chris Wright wrote: >> * Patrick McHardy (kaber@trash.net) wrote: >>> Chris Wright wrote: >>>> * Patrick McHardy (kaber@trash.net) wrote: >>>>> Scott Feldman wrote: >>>>>> On 5/14/10 10:29 AM, "Arnd Bergmann" <arnd@arndb.de> wrote: >>>>>> >>>>>>>> I think we should redo the other IFLA_VF_xxx msgs in the same >style. I'm >>>>>>>> not going to tackle that for IFLA_VF_PORTS patch, but it would be >a good >>>>>>>> followup patch. >>>>>>> I fear it's too late for that now. While we have not yet released >2.6.34 >>>>>>> and 2.6.33 does not contain the broken message, it's extremely >late in the >>>>>>> stabilization phase of v2.6.34, so I doubt that there is still a >chance for >>>>>>> that at this point. >>>>>> That's too bad. I wish Patrick's objections were honored and then >we >>>>>> wouldn't have followed that broken model! Can the broken msgs be >disabled >>>>>> somehow for 2.6.34? Keep the definitions in if_link.h but fail the >SET/GET >>>>>> actions in rtnetlink.c? >>>>> That would be a possibility. Unfortunately I don't think we can fix >>>>> this in a backwards compatible way. >>>> $ git describe --contains ebc08a6f47ee76ecad8e9f26c26e6ec9b46ca659 >>>> v2.6.34-rc1~233^2~336 >>>> >>>> It's not released yet? >>> Correct, it was added in 2.6.34-rc. >> >> AFAICT iproute2 hasn't been released either w/ that support. >> So, I'll prepare patches to fix it (or disable as Scott mentioned). >> What do you think? > >That would be great, otherwise we'll probably have to support it >forever. I'd really like to find a way to fix this, instead of having the functionality disabled. I know Patrick had objections to how the data structures were set up, but I figured that was something we'd tackle later, especially since the series got merged after Patrick's email, and with no further objection. At that point, things got quiet, I got shuffled onto another project, and we all thought things were OK. At this point, Scott's work depends on what I did, so it's probably more work to unwind my stuff and respin his, than to just fix mine. Can we quickly respin the kernel side to take care of Patrick's objections, and work the userspace side over the next week or so? The userspace side is where the real fiddly bits will be. I'll drop what I'm working on and get this taken care of as quickly as I can. Sorry for the confusion - I thought we were OK, until yesterday. -Mitch >-- >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
Hi Mitch,
* Williams, Mitch A (mitch.a.williams@intel.com) wrote:
> I'd really like to find a way to fix this, instead of having the functionality disabled.
I've got some patches that are close to complete. I'll post them
shortly. I'm able to query link from ip at this point.
thanks,
-chris
--
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 cfd420b..5522055 100644 --- a/include/linux/if_link.h +++ b/include/linux/if_link.h @@ -116,6 +116,7 @@ enum { IFLA_VF_TX_RATE, /* TX Bandwidth Allocation */ IFLA_VFINFO, IFLA_STATS64, + IFLA_VF_PORT, __IFLA_MAX }; @@ -259,4 +260,56 @@ struct ifla_vf_info { __u32 qos; __u32 tx_rate; }; + +/* VF Port management section */ + +enum { + IFLA_VF_PORT_UNSPEC, + IFLA_VF_PORT_VF, /* __u32 */ + IFLA_VF_PORT_PROFILE, /* string */ + IFLA_VF_PORT_VSI_TYPE, /* 802.1Qbg (pre-)standard VDP */ + IFLA_VF_PORT_INSTANCE_UUID, /* binary UUID */ + IFLA_VF_PORT_HOST_UUID, /* binary UUID */ + IFLA_VF_PORT_REQUEST, /* __u8 */ + IFLA_VF_PORT_RESPONSE, /* __u16, output only */ + __IFLA_VF_PORT_MAX, +}; + +#define IFLA_VF_PORT_MAX (__IFLA_VF_PORT_MAX - 1) + +#define VF_PORT_PROFILE_MAX 40 +#define VF_PORT_UUID_MAX 16 +#define VF_PORT_VF_NOT_USED -1 + +enum { + VF_PORT_REQUEST_PREASSOCIATE = 0, + VF_PORT_REQUEST_PREASSOCIATE_RR, + VF_PORT_REQUEST_ASSOCIATE, + VF_PORT_REQUEST_DISASSOCIATE, +}; + +enum { + VF_PORT_VDP_RESPONSE_SUCCESS = 0, + VF_PORT_VDP_RESPONSE_INVALID_FORMAT, + VF_PORT_VDP_RESPONSE_INSUFFICIENT_RESOURCES, + VF_PORT_VDP_RESPONSE_UNUSED_VTID, + VF_PORT_VDP_RESPONSE_VTID_VIOLATION, + VF_PORT_VDP_RESPONSE_VTID_VERSION_VIOALTION, + VF_PORT_VDP_RESPONSE_OUT_OF_SYNC, + /* 0x08-0xFF reserved for future VDP use */ + VF_PORT_PROFILE_RESPONSE_SUCCESS = 0x100, + VF_PORT_PROFILE_RESPONSE_INPROGRESS, + VF_PORT_PROFILE_RESPONSE_INVALID, + VF_PORT_PROFILE_RESPONSE_BADSTATE, + VF_PORT_PROFILE_RESPONSE_INSUFFICIENT_RESOURCES, + VF_PORT_PROFILE_RESPONSE_ERROR, +}; + +struct ifla_vf_port_vsi { + __u8 vsi_mgr_id; + __u8 vsi_type_id[3]; + __u8 vsi_type_version; + __u8 pad[3]; +}; + #endif /* _LINUX_IF_LINK_H */ diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 69022d4..c2ba8d4 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -686,6 +686,10 @@ struct netdev_rx_queue { * int (*ndo_set_vf_tx_rate)(struct net_device *dev, int vf, int rate); * int (*ndo_get_vf_config)(struct net_device *dev, * int vf, struct ifla_vf_info *ivf); + * int (*ndo_set_vf_port)(struct net_device *dev, int vf, + * struct nlattr *vf_port[]); + * int (*ndo_get_vf_port)(struct net_device *dev, int vf, + * struct sk_buff *skb); */ #define HAVE_NET_DEVICE_OPS struct net_device_ops { @@ -735,6 +739,12 @@ struct net_device_ops { int (*ndo_get_vf_config)(struct net_device *dev, int vf, struct ifla_vf_info *ivf); + int (*ndo_set_vf_port)(struct net_device *dev, + int vf, + struct nlattr *vf_port[]); + int (*ndo_get_vf_port)(struct net_device *dev, + int vf, + struct sk_buff *skb); #if defined(CONFIG_FCOE) || defined(CONFIG_FCOE_MODULE) int (*ndo_fcoe_enable)(struct net_device *dev); int (*ndo_fcoe_disable)(struct net_device *dev); diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 23a71cb..1e9a896 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -653,6 +653,26 @@ static inline int rtnl_vfinfo_size(const struct net_device *dev) return 0; } +static size_t rtnl_vf_port_size(const struct net_device *dev) +{ + size_t vf_port_size = nla_total_size(sizeof(struct nlattr)) + /* VF_PORT_VF */ + + nla_total_size(VF_PORT_PROFILE_MAX)/* VF_PORT_PROFILE */ + + nla_total_size(sizeof(struct ifla_vf_port_vsi)) + /* VF_PORT_VSI_TYPE */ + + nla_total_size(VF_PORT_UUID_MAX) /* VF_PORT_VSI_INSTANCE */ + + nla_total_size(VF_PORT_UUID_MAX) /* VF_PORT_HOST_UUID */ + + nla_total_size(1) /* VF_PROT_VDP_REQUEST */ + + nla_total_size(2); /* VF_PORT_VDP_RESPONSE */ + + if (!dev->netdev_ops->ndo_get_vf_port || !dev->dev.parent) + return 0; + if (dev_num_vf(dev->dev.parent)) + return vf_port_size * dev_num_vf(dev->dev.parent); + else + return vf_port_size; +} + static inline size_t if_nlmsg_size(const struct net_device *dev) { return NLMSG_ALIGN(sizeof(struct ifinfomsg)) @@ -673,9 +693,61 @@ static inline size_t if_nlmsg_size(const struct net_device *dev) + nla_total_size(1) /* IFLA_LINKMODE */ + nla_total_size(4) /* IFLA_NUM_VF */ + nla_total_size(rtnl_vfinfo_size(dev)) /* IFLA_VFINFO */ + + rtnl_vf_port_size(dev) /* IFLA_VF_PORT */ + rtnl_link_get_size(dev); /* IFLA_LINKINFO */ } +static int rtnl_vf_port_fill_nest(struct sk_buff *skb, struct net_device *dev, + int vf) +{ + struct nlattr *data; + int err; + + data = nla_nest_start(skb, IFLA_VF_PORT); + if (!data) + return -EMSGSIZE; + + if (vf != VF_PORT_VF_NOT_USED) + nla_put_u32(skb, IFLA_VF_PORT_VF, vf); + + err = dev->netdev_ops->ndo_get_vf_port(dev, vf, skb); + if (err) { + nla_nest_cancel(skb, data); + return err; + } + + nla_nest_end(skb, data); + + return 0; +} + +static int rtnl_vf_port_fill(struct sk_buff *skb, struct net_device *dev) +{ + int num_vf; + int err; + + if (!dev->netdev_ops->ndo_get_vf_port || !dev->dev.parent) + return 0; + + num_vf = dev_num_vf(dev->dev.parent); + + if (num_vf) { + int i; + + for (i = 0; i < num_vf; i++) { + err = rtnl_vf_port_fill_nest(skb, dev, i); + if (err) + return err; + } + } else { + err = rtnl_vf_port_fill_nest(skb, dev, VF_PORT_VF_NOT_USED); + if (err) + return err; + } + + return 0; +} + static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev, int type, u32 pid, u32 seq, u32 change, unsigned int flags) @@ -747,17 +819,23 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev, goto nla_put_failure; copy_rtnl_link_stats64(nla_data(attr), stats); + if (dev->dev.parent) + NLA_PUT_U32(skb, IFLA_NUM_VF, dev_num_vf(dev->dev.parent)); + if (dev->netdev_ops->ndo_get_vf_config && dev->dev.parent) { int i; struct ifla_vf_info ivi; - NLA_PUT_U32(skb, IFLA_NUM_VF, dev_num_vf(dev->dev.parent)); for (i = 0; i < dev_num_vf(dev->dev.parent); i++) { if (dev->netdev_ops->ndo_get_vf_config(dev, i, &ivi)) break; NLA_PUT(skb, IFLA_VFINFO, sizeof(ivi), &ivi); } } + + if (rtnl_vf_port_fill(skb, dev)) + goto nla_put_failure; + if (dev->rtnl_link_ops) { if (rtnl_link_fill(skb, dev) < 0) goto nla_put_failure; @@ -824,6 +902,7 @@ const struct nla_policy ifla_policy[IFLA_MAX+1] = { .len = sizeof(struct ifla_vf_vlan) }, [IFLA_VF_TX_RATE] = { .type = NLA_BINARY, .len = sizeof(struct ifla_vf_tx_rate) }, + [IFLA_VF_PORT] = { .type = NLA_NESTED }, }; EXPORT_SYMBOL(ifla_policy); @@ -832,6 +911,20 @@ static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = { [IFLA_INFO_DATA] = { .type = NLA_NESTED }, }; +static const struct nla_policy ifla_vf_port_policy[IFLA_VF_PORT_MAX+1] = { + [IFLA_VF_PORT_VF] = { .type = NLA_U32 }, + [IFLA_VF_PORT_PROFILE] = { .type = NLA_STRING, + .len = VF_PORT_PROFILE_MAX }, + [IFLA_VF_PORT_VSI_TYPE] = { .type = NLA_BINARY, + .len = sizeof(struct ifla_vf_port_vsi)}, + [IFLA_VF_PORT_INSTANCE_UUID]= { .type = NLA_BINARY, + .len = VF_PORT_UUID_MAX }, + [IFLA_VF_PORT_HOST_UUID] = { .type = NLA_STRING, + .len = VF_PORT_UUID_MAX }, + [IFLA_VF_PORT_REQUEST] = { .type = NLA_U8, }, + [IFLA_VF_PORT_RESPONSE] = { .type = NLA_U16, }, +}; + struct net *rtnl_link_get_net(struct net *src_net, struct nlattr *tb[]) { struct net *net; @@ -1028,6 +1121,27 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm, } err = 0; + if (tb[IFLA_VF_PORT]) { + struct nlattr *vf_port[IFLA_VF_PORT_MAX+1]; + int vf = VF_PORT_VF_NOT_USED; + + err = nla_parse_nested(vf_port, IFLA_VF_PORT_MAX, + tb[IFLA_VF_PORT], ifla_vf_port_policy); + if (err < 0) + goto errout; + + if (vf_port[IFLA_VF_PORT_VF]) + vf = nla_get_u32(vf_port[IFLA_VF_PORT_VF]); + + err = -EOPNOTSUPP; + if (ops->ndo_set_vf_port) + err = ops->ndo_set_vf_port(dev, vf, vf_port); + if (err < 0) + goto errout; + modified = 1; + } + err = 0; + errout: if (err < 0 && modified && net_ratelimit()) printk(KERN_WARNING "A link change request failed with "