| Message ID | 20130530132419.GA1368@fedora-17-guest.blr.amer.dell.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
<Narendra_K@Dell.com> writes: > From: Narendra K <narendra_k@dell.com> > > 'dev_id' sysfs attribute is initialized to zero by default. > It is also zero based. This creates ambiguity in differentiating > whether the driver set it to zero or it is the default value. > Initialize 'dev_id' to -1 to make the scenario unambiguous. I understand your concern, but I don't think you can do this. It changes the userspace API, and has some very visible side effects. Please take a look at net/ipv6/addrconf.c Bjørn -- 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 Thu, May 30, 2013 at 07:08:46PM +0530, Bjørn Mork wrote: > > <Narendra_K@Dell.com> writes: > > > From: Narendra K <narendra_k@dell.com> > > > > 'dev_id' sysfs attribute is initialized to zero by default. > > It is also zero based. This creates ambiguity in differentiating > > whether the driver set it to zero or it is the default value. > > Initialize 'dev_id' to -1 to make the scenario unambiguous. > > I understand your concern, but I don't think you can do this. It > changes the userspace API, and has some very visible side effects. > > Please take a look at net/ipv6/addrconf.c Ok, thank you for pointing it. I missed it while looking for its possible use scenarios. With regards, Narendra K Linux Engineering Dell Inc. -- 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/31/2013 5:17 AM, Narendra_K@Dell.com wrote: > On Thu, May 30, 2013 at 07:08:46PM +0530, Bjørn Mork wrote: >> >> <Narendra_K@Dell.com> writes: >> >>> From: Narendra K <narendra_k@dell.com> >>> >>> 'dev_id' sysfs attribute is initialized to zero by default. >>> It is also zero based. This creates ambiguity in differentiating >>> whether the driver set it to zero or it is the default value. >>> Initialize 'dev_id' to -1 to make the scenario unambiguous. >> >> I understand your concern, but I don't think you can do this. It >> changes the userspace API, and has some very visible side effects. >> >> Please take a look at net/ipv6/addrconf.c > > Ok, thank you for pointing it. I missed it while looking for its > possible use scenarios. Although I'm not sure how that check works with devices that are setting dev_id and also provide their own mac addresses. From inspection it looks like these devices end up with a local interface identifier unnecessarily. Maybe Ben knows one of the drivers is the siena solorflare controller apparently for the SFC9000 family? The other two 'grep' finds are an mlx and chelsio device. Interestingly I didn't find any devices setting dev_id that also didn't program unique mac addresses. Perhaps I'm missing something? Thanks, 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
On Fri, 2013-06-07 at 07:45 -0700, John Fastabend wrote: > On 5/31/2013 5:17 AM, Narendra_K@Dell.com wrote: > > On Thu, May 30, 2013 at 07:08:46PM +0530, Bjørn Mork wrote: > >> > >> <Narendra_K@Dell.com> writes: > >> > >>> From: Narendra K <narendra_k@dell.com> > >>> > >>> 'dev_id' sysfs attribute is initialized to zero by default. > >>> It is also zero based. This creates ambiguity in differentiating > >>> whether the driver set it to zero or it is the default value. > >>> Initialize 'dev_id' to -1 to make the scenario unambiguous. > >> > >> I understand your concern, but I don't think you can do this. It > >> changes the userspace API, and has some very visible side effects. > >> > >> Please take a look at net/ipv6/addrconf.c > > > > Ok, thank you for pointing it. I missed it while looking for its > > possible use scenarios. > > Although I'm not sure how that check works with devices that are > setting dev_id and also provide their own mac addresses. From > inspection it looks like these devices end up with a local interface > identifier unnecessarily. > > Maybe Ben knows one of the drivers is the siena solorflare controller > apparently for the SFC9000 family? The other two 'grep' finds are > an mlx and chelsio device. > > Interestingly I didn't find any devices setting dev_id that also > didn't program unique mac addresses. Perhaps I'm missing something? I set this for Siena so that userland can tell which PF and port is which even if the PCIe topology is hidden by virtualisation. In practice we haven't made use of that (and since virtualisation may also hide the VPD and serial number capabilities, it probably doesn't help much). Ben.
On Fri, 2013-06-07 at 07:45 -0700, John Fastabend wrote: > On 5/31/2013 5:17 AM, Narendra_K@Dell.com wrote: > > On Thu, May 30, 2013 at 07:08:46PM +0530, Bjørn Mork wrote: > >> > >> <Narendra_K@Dell.com> writes: > >> > >>> From: Narendra K <narendra_k@dell.com> > >>> > >>> 'dev_id' sysfs attribute is initialized to zero by default. > >>> It is also zero based. This creates ambiguity in differentiating > >>> whether the driver set it to zero or it is the default value. > >>> Initialize 'dev_id' to -1 to make the scenario unambiguous. > >> > >> I understand your concern, but I don't think you can do this. It > >> changes the userspace API, and has some very visible side effects. > >> > >> Please take a look at net/ipv6/addrconf.c > > > > Ok, thank you for pointing it. I missed it while looking for its > > possible use scenarios. > > Although I'm not sure how that check works with devices that are > setting dev_id and also provide their own mac addresses. From > inspection it looks like these devices end up with a local interface > identifier unnecessarily. > > Maybe Ben knows one of the drivers is the siena solorflare controller > apparently for the SFC9000 family? The other two 'grep' finds are > an mlx and chelsio device. The comment in addrconf.c refers to zSeries aka s390. See qeth_l3_setup_netdev() in drivers/s390/net/qeth_l3_main.c (why is this not in drivers/net?). It sounds like all the other drivers should be changed to stop setting dev_id, unless userland really uses it. Looking at /lib/udev/rules.d/75-persistent-net-generator.rules on a RHEL system, it appears to use dev_id only for devices without a globally-assigned MAC address. Ben. > Interestingly I didn't find any devices setting dev_id that also > didn't program unique mac addresses. Perhaps I'm missing something?
On 06/07/2013 08:23 AM, Ben Hutchings wrote: > On Fri, 2013-06-07 at 07:45 -0700, John Fastabend wrote: >> On 5/31/2013 5:17 AM, Narendra_K@Dell.com wrote: >>> On Thu, May 30, 2013 at 07:08:46PM +0530, Bjørn Mork wrote: >>>> >>>> <Narendra_K@Dell.com> writes: >>>> >>>>> From: Narendra K <narendra_k@dell.com> >>>>> >>>>> 'dev_id' sysfs attribute is initialized to zero by default. >>>>> It is also zero based. This creates ambiguity in differentiating >>>>> whether the driver set it to zero or it is the default value. >>>>> Initialize 'dev_id' to -1 to make the scenario unambiguous. >>>> >>>> I understand your concern, but I don't think you can do this. It >>>> changes the userspace API, and has some very visible side effects. >>>> >>>> Please take a look at net/ipv6/addrconf.c >>> >>> Ok, thank you for pointing it. I missed it while looking for its >>> possible use scenarios. >> >> Although I'm not sure how that check works with devices that are >> setting dev_id and also provide their own mac addresses. From >> inspection it looks like these devices end up with a local interface >> identifier unnecessarily. >> >> Maybe Ben knows one of the drivers is the siena solorflare controller >> apparently for the SFC9000 family? The other two 'grep' finds are >> an mlx and chelsio device. >> >> Interestingly I didn't find any devices setting dev_id that also >> didn't program unique mac addresses. Perhaps I'm missing something? > > I set this for Siena so that userland can tell which PF and port is > which even if the PCIe topology is hidden by virtualisation. In > practice we haven't made use of that (and since virtualisation may also > hide the VPD and serial number capabilities, it probably doesn't help > much). > > Ben. > I guess then to make dev_id work like Narendra wants you would need something in the management stack to come through and assign unique dev_id values to functions (net_devices) that share a MAC. Or have some unique 'unsigned short' value in the driver that it could be set to. I guess I miss why initializing it to -1 helps. Still best I can tell your ipv6 identifiers are not going to have the 'u' bit inverted creating a somewhat strange side effect from setting the dev_id. .John
[...] > Hi John, thank you for the inputs. It was set to -1 to differentiate between driver setting it to zero and the default value zero. > The point I was trying to make here is setting it to a single digit 0, 1, or -1 doesn't really help in a virtualized environment where you may not have access to the PCI topology, serial #s, etc. You need a system-wide unique id. Anyways looks like your on the right track. .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
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 8f967e3..986867e 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1142,7 +1142,7 @@ struct net_device { unsigned char addr_assign_type; /* hw address assignment type */ unsigned char addr_len; /* hardware address length */ unsigned char neigh_priv_len; - unsigned short dev_id; /* for shared network cards */ + short dev_id; /* physical port number */ spinlock_t addr_list_lock; struct netdev_hw_addr_list uc; /* Unicast mac addresses */ diff --git a/net/core/dev.c b/net/core/dev.c index d4d874a..aca6002 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -5727,6 +5727,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name, INIT_LIST_HEAD(&dev->upper_dev_list); dev->priv_flags = IFF_XMIT_DST_RELEASE; setup(dev); + dev->dev_id = -1; dev->num_tx_queues = txqs; dev->real_num_tx_queues = txqs;