[RFC PATCH] Initialize dev_id sysfs attribute to -1 by default

[RFC PATCH] Initialize dev_id sysfs attribute to -1 by default

[Narendra_K]

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.

Signed-off-by: Narendra K <narendra_k@dell.com>
---
The patch applies to net-next tree.

 include/linux/netdevice.h | 2 +-
 net/core/dev.c            | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

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;
-- 
1.8.0.1

-- 
With regards,
Narendra K
Linux Engineering
Dell Inc.

Re: [RFC PATCH] Initialize dev_id sysfs attribute to -1 by default

[Bjørn Mork]

<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

Re: [RFC PATCH] Initialize dev_id sysfs attribute to -1 by default

[Narendra_K]

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.

Re: [RFC PATCH] Initialize dev_id sysfs attribute to -1 by default

[John Fastabend]

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

Re: [RFC PATCH] Initialize dev_id sysfs attribute to -1 by default

[Ben Hutchings]

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.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC PATCH] Initialize dev_id sysfs attribute to -1 by default

[Ben Hutchings]

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?

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC PATCH] Initialize dev_id sysfs attribute to -1 by default

[John Fastabend]

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



-- 
John Fastabend         Intel Corporation

RE: [RFC PATCH] Initialize dev_id sysfs attribute to -1 by default

[Narendra_K]

> -----Original Message-----
> From: John Fastabend [mailto:john.fastabend@gmail.com]
> Sent: Friday, June 07, 2013 10:45 PM
> To: Ben Hutchings
> Cc: John Fastabend; K, Narendra; bjorn@mork.no; netdev@vger.kernel.org;
> yoshfuji@linux-ipv6.org
> Subject: Re: [RFC PATCH] Initialize dev_id sysfs attribute to -1 by default
> 

[...]
> >>
> >> 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.
> 

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.

With regards,
Narendra K
Linux Engineering
Dell Inc.

Re: [RFC PATCH] Initialize dev_id sysfs attribute to -1 by default

[John Fastabend]

[...]

> 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