From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arnd Bergmann Subject: Re: [net-next-2.6 V7 PATCH 1/2] Add netlink support for virtual port management (was iovnl) Date: Fri, 14 May 2010 14:12:01 +0200 Message-ID: <201005141412.01578.arnd@arndb.de> References: <20100514013144.1816.31191.stgit@savbu-pc100.cisco.com> <20100514013526.1816.45104.stgit@savbu-pc100.cisco.com> <4BED2CD8.4020209@trash.net> Mime-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Cc: Scott Feldman , davem@davemloft.net, netdev@vger.kernel.org, chrisw@redhat.com To: Patrick McHardy Return-path: Received: from moutng.kundenserver.de ([212.227.17.9]:49579 "EHLO moutng.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754238Ab0ENMMI (ORCPT ); Fri, 14 May 2010 08:12:08 -0400 In-Reply-To: <4BED2CD8.4020209@trash.net> Sender: netdev-owner@vger.kernel.org List-ID: 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