Re: udev can't name PS3's network devices correctly

Re: udev can't name PS3's network devices correctly

[David Woodhouse]

On Mon, 2008-04-07 at 16:38 +0200, Marco d'Itri wrote:
> On Apr 07, Harald Hoyer <harald@redhat.com> wrote:
> 
> > So Kazunori Asayama attached a patch to the bugzilla, which modifies 
> > write_net_rules to take the KERNEL attribute also in account.
> >
> > What do you think?
>
> If this were accepted by Kay I would definitely remove it from the
> Debian package. It's an ugly hack for a broken device which should not
> be generalized. 

Actually it's a generic problem. It also occurs on OLPC, where the
libertas wireless device presents two interfaces with the same MAC
address -- the 'eth%d' interface for normal 802.11 wireless, and the
'msh%d' interface for mesh. This approach fixes that too. Our previous
fix for that was 'TEST=="anycast_mask"' and 'TEST=="lbs_rtap"', which
are horridly device-specific hacks.

It isn't _broken_ to have more than one device on the same host with the
same MAC address; it's just unusual.

One thing I don't understand: Don't we already emit a KERNEL== criterion
in the case where there is already a udev rule 'reserving' the name that
the kernel used for the current device? Why is that one OK, and why only
in that case? This patch just makes it consistent.

> Unconditionally adding a KERNEL key means that persistent names will
> break if a driver changes the base name (e.g. wlan* -> eth*).

Drivers shouldn't do that. It'll change the name of the device and may
confuse userspace :)


One alternative approach would be to use dev->dev_id (is that exported
in sysfs?). That's what IPv6 addrconf uses in addition to the MAC
address to provide a unique address when MAC addresses are shared.

We could modify the libertas and gelic (and any other affected) drivers
to provide a dev_id, make sure it's exported in sysfs, and then use that
in the udev rules. Would that make you happy?

It has implications w.r.t. the autoconfigured IPv6 addresses of those
devices, but I think we can cope with those. I'm probably the only
person in the world with an IPv6 address for a PS3 listed in DNS anyway.

> This workaround should be confined to the PS3 install media, there is
> no reason to add it to general-purpose distributions.

Our general-purpose distribution installs and runs on PS3. :)

-- 
dwmw2

Re: udev can't name PS3's network devices correctly

[David Woodhouse]

On Mon, 2008-04-14 at 11:08 +0100, David Woodhouse wrote:
> One alternative approach would be to use dev->dev_id (is that exported
> in sysfs?). That's what IPv6 addrconf uses in addition to the MAC
> address to provide a unique address when MAC addresses are shared.
> 
> We could modify the libertas and gelic (and any other affected) drivers
> to provide a dev_id, make sure it's exported in sysfs, and then use that
> in the udev rules. Would that make you happy?

That would look something like this (in fact, I think it would let us
get rid of the special case for S390 too)...

--- ./udev-120/extras/rule_generator/75-persistent-net-generator.rules~	2008-04-03 20:12:53.000000000 +0100
+++ ./udev-120/extras/rule_generator/75-persistent-net-generator.rules	2008-04-14 11:52:46.000000000 +0100
@@ -18,7 +18,7 @@ SUBSYSTEM!="net", GOTO="persistent_net_g
 NAME=="?*", GOTO="persistent_net_generator_end"
 
 # device name whitelist
-KERNEL!="eth*|ath*|wlan*[0-9]|ra*|sta*|ctc*|lcs*|hsi*", GOTO="persistent_net_generator_end"
+KERNEL!="eth*|ath*|wlan*[0-9]|ra*|sta*|ctc*|lcs*|hsi*|msh*", GOTO="persistent_net_generator_end"
 
 # ignore Xen virtual interfaces
 SUBSYSTEMS=="xen", GOTO="persistent_net_generator_end"
@@ -29,6 +29,9 @@ ENV{MATCHADDR}="$attr{address}"
 # match interface type
 ENV{MATCHIFTYPE}="$attr{type}"
 
+# match dev_id
+ENV{MATCHDEVID}="$attr{dev_id}"
+
 # do not use "locally administered" MAC address
 ENV{MATCHADDR}=="?[2367abef]:*", ENV{MATCHADDR}=""
 
--- ./udev-120/extras/rule_generator/write_net_rules~	2008-04-03 20:12:53.000000000 +0100
+++ ./udev-120/extras/rule_generator/write_net_rules	2008-04-14 11:47:27.000000000 +0100
@@ -15,6 +15,7 @@
 # variables used to communicate:
 #   MATCHADDR             MAC address used for the match
 #   MATCHID               bus_id used for the match
+#   MATCHDEVID            dev_id used for the match
 #   MATCHDRV              driver name used for the match
 #   MATCHIFTYPE           interface type match
 #   COMMENT               comment to add to the generated rule
@@ -78,6 +79,10 @@ if [ "$MATCHDRV" ]; then
 	match="$match, DRIVERS==\"$MATCHDRV\""
 fi
 
+if [ "$MATCHDEVID" ]; then
+	match="$match, ATTR{dev_id}==\"$MATCHDEVID\""
+fi
+
 if [ "$MATCHID" ]; then
 	match="$match, KERNELS==\"$MATCHID\""
 fi


... and this...

--- ./linux-2.6.24.ppc/drivers/net/ps3_gelic_wireless.c~	2008-04-13 13:38:15.000000000 +0100
+++ ./linux-2.6.24.ppc/drivers/net/ps3_gelic_wireless.c	2008-04-14 11:15:10.000000000 +0100
@@ -2699,6 +2699,7 @@ int gelic_wl_driver_probe(struct gelic_c
 	gelic_wl_setup_netdev_ops(netdev);
 
 	/* setup some of net_device and register it */
+	netdev->dev_id = GELIC_PORT_WIRELESS;
 	ret = gelic_net_setup_netdev(netdev, card);
 	if (ret)
 		goto fail_setup;
--- ./linux-2.6.24.ppc/drivers/net/ps3_gelic_net.c~	2008-04-13 13:38:15.000000000 +0100
+++ ./linux-2.6.24.ppc/drivers/net/ps3_gelic_net.c	2008-04-14 11:15:19.000000000 +0100
@@ -1635,6 +1635,7 @@ static int ps3_gelic_driver_probe(struct
 	netdev->irq = card->irq;
 	SET_NETDEV_DEV(netdev, &card->dev->core);
 	gelic_ether_setup_netdev_ops(netdev, &card->napi);
+	netdev->dev_id = GELIC_PORT_ETHERNET;
 	result = gelic_net_setup_netdev(netdev, card);
 	if (result) {
 		dev_dbg(&dev->core, "%s: setup_netdev failed %d",
--- ./linux-2.6.24.ppc/drivers/net/wireless/libertas/main.c~	2008-04-13 13:38:16.000000000 +0100
+++ ./linux-2.6.24.ppc/drivers/net/wireless/libertas/main.c	2008-04-14 10:49:52.000000000 +0100
@@ -1205,6 +1205,8 @@ int lbs_start_card(struct lbs_private *p
 	/* init 802.11d */
 	lbs_init_11d(priv);
 
+	dev->dev_id = 0;
+
 	if (register_netdev(dev)) {
 		lbs_pr_err("cannot register ethX device\n");
 		goto done;
@@ -1327,6 +1329,7 @@ static int lbs_add_mesh(struct lbs_priva
 	mesh_dev->wireless_handlers = (struct iw_handler_def *)&mesh_handler_def;
 #endif
 	/* Register virtual mesh interface */
+	mesh_dev->dev_id = 1;
 	ret = register_netdev(mesh_dev);
 	if (ret) {
 		lbs_pr_err("cannot register mshX virtual interface\n");
@@ -1542,6 +1545,7 @@ static int lbs_add_rtap(struct lbs_priva
 	rtap_dev->set_multicast_list = lbs_set_multicast_list;
 	rtap_dev->priv = priv;
 
+	rtap_dev->dev_id = 2;
 	ret = register_netdev(rtap_dev);
 	if (ret) {
 		free_netdev(rtap_dev);
--- ./linux-2.6.24.ppc/net/core/net-sysfs.c~	2008-04-13 13:38:24.000000000 +0100
+++ ./linux-2.6.24.ppc/net/core/net-sysfs.c	2008-04-14 10:58:32.000000000 +0100
@@ -87,6 +87,7 @@ static ssize_t netdev_store(struct devic
 	return ret;
 }
 
+NETDEVICE_SHOW(dev_id, fmt_hex);
 NETDEVICE_SHOW(addr_len, fmt_dec);
 NETDEVICE_SHOW(iflink, fmt_dec);
 NETDEVICE_SHOW(ifindex, fmt_dec);
@@ -210,6 +211,7 @@ static ssize_t store_tx_queue_len(struct
 
 static struct device_attribute net_class_attributes[] = {
 	__ATTR(addr_len, S_IRUGO, show_addr_len, NULL),
+	__ATTR(dev_id, S_IRUGO, show_dev_id, NULL),
 	__ATTR(iflink, S_IRUGO, show_iflink, NULL),
 	__ATTR(ifindex, S_IRUGO, show_ifindex, NULL),
 	__ATTR(features, S_IRUGO, show_features, NULL),

-- 
dwmw2

Re: udev can't name PS3's network devices correctly

[Kay Sievers]

On Mon, Apr 14, 2008 at 1:11 PM, David Woodhouse <dwmw2@infradead.org> wrote:
> On Mon, 2008-04-14 at 11:08 +0100, David Woodhouse wrote:
>  > One alternative approach would be to use dev->dev_id (is that exported
>  > in sysfs?). That's what IPv6 addrconf uses in addition to the MAC
>  > address to provide a unique address when MAC addresses are shared.
>  >
>  > We could modify the libertas and gelic (and any other affected) drivers
>  > to provide a dev_id, make sure it's exported in sysfs, and then use that
>  > in the udev rules. Would that make you happy?
>
>  That would look something like this (in fact, I think it would let us
>  get rid of the special case for S390 too)...
>
>  --- ./udev-120/extras/rule_generator/75-persistent-net-generator.rules~ 2008-04-03 20:12:53.000000000 +0100
>  +++ ./udev-120/extras/rule_generator/75-persistent-net-generator.rules  2008-04-14 11:52:46.000000000 +0100
>  @@ -18,7 +18,7 @@ SUBSYSTEM!="net", GOTO="persistent_net_g
>   NAME=="?*", GOTO="persistent_net_generator_end"
>
>   # device name whitelist
>  -KERNEL!="eth*|ath*|wlan*[0-9]|ra*|sta*|ctc*|lcs*|hsi*", GOTO="persistent_net_generator_end"
>  +KERNEL!="eth*|ath*|wlan*[0-9]|ra*|sta*|ctc*|lcs*|hsi*|msh*", GOTO="persistent_net_generator_end"
>
>   # ignore Xen virtual interfaces
>   SUBSYSTEMS=="xen", GOTO="persistent_net_generator_end"
>  @@ -29,6 +29,9 @@ ENV{MATCHADDR}="$attr{address}"
>   # match interface type
>   ENV{MATCHIFTYPE}="$attr{type}"
>
>  +# match dev_id
>  +ENV{MATCHDEVID}="$attr{dev_id}"
>  +

Sure, looks good. Let me know when the kernel part gets merged, and I
will apply that.

Thanks,
Kay

[PATCH] Expose netdevice dev_id through sysfs

[David Woodhouse]

Expose dev_id to userspace, because it helps to disambiguate between
interfaces where the MAC address is unique.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>

--- ./linux-2.6.24.ppc/net/core/net-sysfs.c~	2008-04-13 13:38:24.000000000 +0100
+++ ./linux-2.6.24.ppc/net/core/net-sysfs.c	2008-04-14 10:58:32.000000000 +0100
@@ -87,6 +87,7 @@ static ssize_t netdev_store(struct devic
 	return ret;
 }
 
+NETDEVICE_SHOW(dev_id, fmt_hex);
 NETDEVICE_SHOW(addr_len, fmt_dec);
 NETDEVICE_SHOW(iflink, fmt_dec);
 NETDEVICE_SHOW(ifindex, fmt_dec);
@@ -210,6 +211,7 @@ static ssize_t store_tx_queue_len(struct
 
 static struct device_attribute net_class_attributes[] = {
 	__ATTR(addr_len, S_IRUGO, show_addr_len, NULL),
+	__ATTR(dev_id, S_IRUGO, show_dev_id, NULL),
 	__ATTR(iflink, S_IRUGO, show_iflink, NULL),
 	__ATTR(ifindex, S_IRUGO, show_ifindex, NULL),
 	__ATTR(features, S_IRUGO, show_features, NULL),


-- 
dwmw2

Re: [PATCH] Expose netdevice dev_id through sysfs

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Mon, 14 Apr 2008 13:32:02 +0100

> Expose dev_id to userspace, because it helps to disambiguate between
> interfaces where the MAC address is unique.
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>

As far as I can tell there is only one driver, s390's qeth, even
setting netdev->dev_id.

And this driver is providing netdev->dev_id uniqueness amongst qeth
device instances.

But that's not the problem we're trying to solve.

We're trying to provide uniqueness amongst all devices in the system
that are using the same MAC address.

On a Sparc system, for example, ethernet chips driven by several
different drivers can end up with the same MAC address, as the
system IDPROM specified ethernet MAC is what will be used by
default.

So we need some global scheme.  And this dev_id value would need to be
persistent.  As best as I can tell, such things aren't available.
Sure we could do something silly like use the device I/O physical
address, but that would defeat the whole purpose of making device
identification agnostic to I/O bus geography.  I could move the
card to a different slot and it would have a different dev_id.

We therefore need a more concrete and workable plan to resolve
this issue.

Re: [PATCH] Expose netdevice dev_id through sysfs

[Andrey Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 1418 bytes --]

On Sunday 20 April 2008, David Miller wrote:
> We're trying to provide uniqueness amongst all devices in the system
> that are using the same MAC address.
> 
> On a Sparc system, for example, ethernet chips driven by several
> different drivers can end up with the same MAC address, as the
> system IDPROM specified ethernet MAC is what will be used by
> default.
> 

On Sparc system we also have global device tree which provides unique
and persistent reference to every device. Solaris has no problems with
having same MAC for all interfaces.

> So we need some global scheme.  And this dev_id value would need to be
> persistent.  As best as I can tell, such things aren't available.

What is exactly wrong with using device topology path? This should exist
on any system, it is unique and it is persistent.

> Sure we could do something silly like use the device I/O physical
> address, but that would defeat the whole purpose of making device
> identification agnostic to I/O bus geography.  I could move the
> card to a different slot and it would have a different dev_id.
> 

Sure; a card in different slot *is* a different device. And when broken
card is replaced in the same slot for all practical purposes it *is* the
same device even if MAC has changed.

Nobody makes cable labels like "card with MAC xxx"; every cable label has
something like "shelf 2; PCI slot 3; port 1".

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Expose netdevice dev_id through sysfs

[David Miller]

From: Andrey Borzenkov <arvidjaar@mail.ru>
Date: Sun, 20 Apr 2008 09:21:03 +0400

> On Sparc system we also have global device tree which provides unique
> and persistent reference to every device. Solaris has no problems with
> having same MAC for all interfaces.

SunOS/Solaris names devices geographically on the I/O bus.  So if you
move the card from one slot to another, it's a different device and
you have to change your configuration, which is pretty bogus.

At least under Linux the identity follows the MAC address, which I
definitely see as a superior behavior.

The only problem is handling the multiple devices with same MAC case.

> What is exactly wrong with using device topology path? This should
> exist on any system, it is unique and it is persistent.

Because if you move the device to a different PCI slot it should
still show up as the same device, using the MAC address as the key.
The Solaris behavior isn't so intelligent.

Re: [PATCH] Expose netdevice dev_id through sysfs

[David Woodhouse]

On Sat, 2008-04-19 at 18:33 -0700, David Miller wrote:
> From: David Woodhouse <dwmw2@infradead.org>
> Date: Mon, 14 Apr 2008 13:32:02 +0100
> 
> > Expose dev_id to userspace, because it helps to disambiguate between
> > interfaces where the MAC address is unique.
> > 
> > Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> 
> As far as I can tell there is only one driver, s390's qeth, even
> setting netdev->dev_id.

There was talk of adding it for libertas and ps3_gelic too, since they
also provide devices with the same MAC address.

> And this driver is providing netdev->dev_id uniqueness amongst qeth
> device instances.
> But that's not the problem we're trying to solve.
> 
> We're trying to provide uniqueness amongst all devices in the system
> that are using the same MAC address.
>
> On a Sparc system, for example, ethernet chips driven by several
> different drivers can end up with the same MAC address, as the
> system IDPROM specified ethernet MAC is what will be used by
> default.
> 
> So we need some global scheme. 

Well, kind of. The kernel doesn't need to provide a single, globally
unique device identifier and hand it to udev on a plate.

The kernel just needs to provide a sufficient _set_ of attributes such
that udev can tell the difference between devices. On an S390 system,
that set of attributes should probably include dev_id, which would let
us remove the special-case hack in udev's persistent naming script. 

We _could_ also use dev_id for libertas and ps3_gelic, but in fact udev
is almost capable of using another criterion for matching -- the
basename of the interface (eth%d vs. msh%0 or wlan%d) -- which is
already sufficient to distinguish between those particular logical
devices. We just need to fix udev to use it _consistently_ rather than
spuriously dropping it from the generated rule in certain cases.

But if that gets fixed in udev and addresses my original issues
(libertas, ps3), that still doesn't mean that dev_id shouldn't be
exposed and used as _one_ of udev's criteria for matching devices.

> And this dev_id value would need to be
> persistent.  As best as I can tell, such things aren't available.
> Sure we could do something silly like use the device I/O physical
> address, but that would defeat the whole purpose of making device
> identification agnostic to I/O bus geography.  I could move the
> card to a different slot and it would have a different dev_id.
>
> We therefore need a more concrete and workable plan to resolve
> this issue.

Certainly, it's possible that we'll need _more_ criteria for udev to
match on; that doesn't necessarily mean that dev_id shouldn't be one of
them, does it?

-- 
dwmw2

Re: [PATCH] Expose netdevice dev_id through sysfs

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Sun, 20 Apr 2008 11:50:44 +0100

> Certainly, it's possible that we'll need _more_ criteria for udev to
> match on; that doesn't necessarily mean that dev_id shouldn't be one of
> them, does it?

I've read what you have to say, and your point is logical.

However you've just also stated that dev_id isn't even needed to solve
the particular problems you're interested in.

I'm not against exporting the value.  But let's do it because it does
solve a problem, not because it's fun to export every object instance
variable via sysfs :-)

Re: [PATCH] Expose netdevice dev_id through sysfs

[David Woodhouse]

On Sun, 2008-04-20 at 03:55 -0700, David Miller wrote:
> From: David Woodhouse <dwmw2@infradead.org>
> Date: Sun, 20 Apr 2008 11:50:44 +0100
> 
> > Certainly, it's possible that we'll need _more_ criteria for udev to
> > match on; that doesn't necessarily mean that dev_id shouldn't be one of
> > them, does it?
> 
> I've read what you have to say, and your point is logical.
> 
> However you've just also stated that dev_id isn't even needed to solve
> the particular problems you're interested in.

This is true (assuming udev gets fixed). 

> I'm not against exporting the value.  But let's do it because it does
> solve a problem, not because it's fun to export every object instance
> variable via sysfs :-)

The reason I still submitted it, after realising that Marco's objections
to letting udev rely on the 'KERNEL==' match were bogus, is because I
think it does still help on S390 -- it lets us remove the special case
hack there.

I don't think it's actually _broken_ on S390 at the moment; just a bit
icky. That was sufficient motivation to still submit the patch :)

-- 
dwmw2

Re: [PATCH] Expose netdevice dev_id through sysfs

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Sun, 20 Apr 2008 12:12:46 +0100

> The reason I still submitted it, after realising that Marco's objections
> to letting udev rely on the 'KERNEL==' match were bogus, is because I
> think it does still help on S390 -- it lets us remove the special case
> hack there.
> 
> I don't think it's actually _broken_ on S390 at the moment; just a bit
> icky. That was sufficient motivation to still submit the patch :)

Fair enough.

Why don't you do the following?  Respin the patch with some
added verbiage in the changelog entry based upon what we've
discussed here and I'll apply the patch.

Thanks.

[PATCH] Expose netdevice dev_id through sysfs

[David Woodhouse]

Expose dev_id to userspace, because it helps to disambiguate between
interfaces where the MAC address is unique.

This should allow us to simplify the handling of persistent naming for
S390 network devices in udev -- because it can depend on a simple
attribute of the device like the other match criteria, rather than 
having a special case for SUBSYSTEMS=="ccwgroup".

Signed-off-by: David Woodhouse <dwmw2@infradead.org>

--- ./linux-2.6.24.ppc/net/core/net-sysfs.c~	2008-04-13 13:38:24.000000000 +0100
+++ ./linux-2.6.24.ppc/net/core/net-sysfs.c	2008-04-14 10:58:32.000000000 +0100
@@ -87,6 +87,7 @@ static ssize_t netdev_store(struct devic
 	return ret;
 }
 
+NETDEVICE_SHOW(dev_id, fmt_hex);
 NETDEVICE_SHOW(addr_len, fmt_dec);
 NETDEVICE_SHOW(iflink, fmt_dec);
 NETDEVICE_SHOW(ifindex, fmt_dec);
@@ -210,6 +211,7 @@ static ssize_t store_tx_queue_len(struct
 
 static struct device_attribute net_class_attributes[] = {
 	__ATTR(addr_len, S_IRUGO, show_addr_len, NULL),
+	__ATTR(dev_id, S_IRUGO, show_dev_id, NULL),
 	__ATTR(iflink, S_IRUGO, show_iflink, NULL),
 	__ATTR(ifindex, S_IRUGO, show_ifindex, NULL),
 	__ATTR(features, S_IRUGO, show_features, NULL),


-- 
dwmw2

Re: udev can't name PS3's network devices correctly

[David Woodhouse]

On Mon, 2008-04-14 at 13:55 +0200, Kay Sievers wrote:
> 
> >  +# match dev_id
> >  +ENV{MATCHDEVID}="$attr{dev_id}"
> >  +
> 
> Sure, looks good. Let me know when the kernel part gets merged, and I
> will apply that.

http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=9d29672c64505f2d7f707701b829715705308a69

-- 
dwmw2

Re: udev can't name PS3's network devices correctly

[Kay Sievers]

On Sun, 2008-04-27 at 18:37 +0100, David Woodhouse wrote:
> On Mon, 2008-04-14 at 13:55 +0200, Kay Sievers wrote:
> > 
> > >  +# match dev_id
> > >  +ENV{MATCHDEVID}="$attr{dev_id}"
> > >  +
> > 
> > Sure, looks good. Let me know when the kernel part gets merged, and I
> > will apply that.
> 
> http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=9d29672c64505f2d7f707701b829715705308a69

http://git.kernel.org/?p=linux/hotplug/udev.git;a=commitdiff;h=8b6e9f287d4ba774a12c023c2ef8fc78def29df8

I have a link for you too. :)

Thanks,
Kay

Re: udev can't name PS3's network devices correctly

[Kay Sievers]

On Mon, Apr 14, 2008 at 12:08 PM, David Woodhouse <dwmw2@infradead.org> wrote:
> On Mon, 2008-04-07 at 16:38 +0200, Marco d'Itri wrote:
>  > On Apr 07, Harald Hoyer <harald@redhat.com> wrote:
>  >
>  > > So Kazunori Asayama attached a patch to the bugzilla, which modifies
>  > > write_net_rules to take the KERNEL attribute also in account.
>  > >
>  > > What do you think?
>  >
>  > If this were accepted by Kay I would definitely remove it from the
>  > Debian package. It's an ugly hack for a broken device which should not
>  > be generalized.
>
>  Actually it's a generic problem.

Yeah, unfortunately it is.

> It also occurs on OLPC, where the
>  libertas wireless device presents two interfaces with the same MAC
>  address -- the 'eth%d' interface for normal 802.11 wireless, and the
>  'msh%d' interface for mesh. This approach fixes that too. Our previous
>  fix for that was 'TEST=="anycast_mask"' and 'TEST=="lbs_rtap"', which
>  are horridly device-specific hacks.
>
>  It isn't _broken_ to have more than one device on the same host with the
>  same MAC address; it's just unusual.

Yeah, PS3 even had both interfaces with the same MAC named eth*. :)
That got changed recently to make it easier for usersapce.

>  One thing I don't understand: Don't we already emit a KERNEL== criterion
>  in the case where there is already a udev rule 'reserving' the name that
>  the kernel used for the current device? Why is that one OK, and why only
>  in that case? This patch just makes it consistent.

Yes, we do that in the recent udev versions. We only make sure we keep
the enumeration across the same basename, not across different device
names.

>  > Unconditionally adding a KERNEL key means that persistent names will
>  > break if a driver changes the base name (e.g. wlan* -> eth*).
>
>  Drivers shouldn't do that. It'll change the name of the device and may
>  confuse userspace :)

It happens sometimes, that a new driver has a new name, but that is
not neccesarily something we need to cover with the persistent net
names. The main goal is to provide stable names regardless of
module-loading/bus-probe/device-enumeration order, not the use of new
drivers which look different to userspace.

>  One alternative approach would be to use dev->dev_id (is that exported
>  in sysfs?). That's what IPv6 addrconf uses in addition to the MAC
>  address to provide a unique address when MAC addresses are shared.
>
>  We could modify the libertas and gelic (and any other affected) drivers
>  to provide a dev_id, make sure it's exported in sysfs, and then use that
>  in the udev rules. Would that make you happy?
>
>  It has implications w.r.t. the autoconfigured IPv6 addresses of those
>  devices, but I think we can cope with those. I'm probably the only
>  person in the world with an IPv6 address for a PS3 listed in DNS anyway.

Sounds good.

Thanks,
Kay

Re: udev can't name PS3's network devices correctly

[David Woodhouse]

On Mon, 2008-04-14 at 14:03 +0200, Kay Sievers wrote:
> 
> >  One thing I don't understand: Don't we already emit a KERNEL== criterion
> >  in the case where there is already a udev rule 'reserving' the name that
> >  the kernel used for the current device? Why is that one OK, and why only
> >  in that case? This patch just makes it consistent.
> 
> Yes, we do that in the recent udev versions. We only make sure we keep
> the enumeration across the same basename, not across different device
> names.

Right, but you _only_ do that when you're asked to create a rule for a
device where the kernel's name matches a pre-existing rule. You don't do
it consistently -- and Marco was objecting to a patch which just made it
happen consistently.

That patch on its own would be sufficient to fix the PS3 and Libertas
problems, since the kernel uses a different basename for the logical
devices we want to disambiguate. (It would also fix the clash between
wlan0 and wmaster0 on mac80211 interfaces).

We probably also want to do the dev_id thing, but maybe not for PS3 and
Libertas (since it affects their IPv6 addressing too).

-- 
dwmw2

Re: udev can't name PS3's network devices correctly

[Kay Sievers]

On Mon, 2008-04-14 at 13:19 +0100, David Woodhouse wrote:
> On Mon, 2008-04-14 at 14:03 +0200, Kay Sievers wrote:
> > 
> > >  One thing I don't understand: Don't we already emit a KERNEL== criterion
> > >  in the case where there is already a udev rule 'reserving' the name that
> > >  the kernel used for the current device? Why is that one OK, and why only
> > >  in that case? This patch just makes it consistent.
> > 
> > Yes, we do that in the recent udev versions. We only make sure we keep
> > the enumeration across the same basename, not across different device
> > names.
> 
> Right, but you _only_ do that when you're asked to create a rule for a
> device where the kernel's name matches a pre-existing rule.

We do it for all new rules, regardless of pre-existing rules with that
name. It just does not add KERNEL, if some external tool tried to
overwrite the kernel name, we write a rule for.

Not sure why Fedora rules are mangling INTERFACE_NAME here:
  https://bugzilla.redhat.com/show_bug.cgi?id=440568#c4

> You don't do it consistently --

Right, we will do that, if there are no other issues coming up with the
general approach of adding KERNEL matches.

> and Marco was objecting to a patch which just made it
> happen consistently.

Right, I'm just waiting for Marco to comment on this.

> That patch on its own would be sufficient to fix the PS3 and Libertas
> problems, since the kernel uses a different basename for the logical
> devices we want to disambiguate. (It would also fix the clash between
> wlan0 and wmaster0 on mac80211 interfaces).

Sure, that's why we have it in every default rule.

> We probably also want to do the dev_id thing, but maybe not for PS3 and
> Libertas (since it affects their IPv6 addressing too).

It can just be in all rules, like the ATTR{type} match, right?

Thanks,
Kay

Re: udev can't name PS3's network devices correctly

[David Woodhouse]

On Mon, 2008-04-14 at 14:52 +0200, Kay Sievers wrote:
> On Mon, 2008-04-14 at 13:19 +0100, David Woodhouse wrote:
> > On Mon, 2008-04-14 at 14:03 +0200, Kay Sievers wrote:
> > > 
> > > >  One thing I don't understand: Don't we already emit a KERNEL== criterion
> > > >  in the case where there is already a udev rule 'reserving' the name that
> > > >  the kernel used for the current device? Why is that one OK, and why only
> > > >  in that case? This patch just makes it consistent.
> > > 
> > > Yes, we do that in the recent udev versions. We only make sure we keep
> > > the enumeration across the same basename, not across different device
> > > names.
> > 
> > Right, but you _only_ do that when you're asked to create a rule for a
> > device where the kernel's name matches a pre-existing rule.
> 
> We do it for all new rules, regardless of pre-existing rules with that
> name. It just does not add KERNEL, if some external tool tried to
> overwrite the kernel name, we write a rule for.

Ah, yes -- that's an improvement on the version in udev-120, which would
only write it if $INTERFACE_NAME wasn't set _AND_ the current interface
name is already reserved by an existing rule.

> Not sure why Fedora rules are mangling INTERFACE_NAME here:
>   https://bugzilla.redhat.com/show_bug.cgi?id=440568#c4

It's for compatibility with older configuration. Before udev, the Fedora
network configuration used to allow you to set a MAC address for each
configured interface, and would rename interfaces accordingly. This
preserves that functionality, automatically converting it to udev rules.

If we were to include the KERNEL== criterion even when $INTERFACE_NAME
is set, that would solve the problem for now.

> > You don't do it consistently --
> 
> Right, we will do that, if there are no other issues coming up with the
> general approach of adding KERNEL matches.

That would be good. :)

> > We probably also want to do the dev_id thing, but maybe not for PS3 and
> > Libertas (since it affects their IPv6 addressing too).
> 
> It can just be in all rules, like the ATTR{type} match, right?

I think so, yes. The only problem I can foresee is if you write rules
with a new kernel (with the patch I just posted), and then boot into an
older kernel. Since the dev_id attribute will no longer exist, your rule
won't match... and you'll get a new rule without the dev_id.

I'm not sure how much we care about that case -- perhaps the latter rule
should have a 'TEST!="dev_id"' criterion? And/or maybe when we generate
rules where dev_id==0x0, we should make it a special case which accepts
either 0x0, or no dev_id attribute at all?

-- 
dwmw2

Re: udev can't name PS3's network devices correctly

[Marco d'Itri]

[-- Attachment #1: Type: text/plain, Size: 1127 bytes --]

On Apr 14, David Woodhouse <dwmw2@infradead.org> wrote:

> Actually it's a generic problem. It also occurs on OLPC, where the
> libertas wireless device presents two interfaces with the same MAC
And on some Sun systems as well.

> > Unconditionally adding a KERNEL key means that persistent names will
> > break if a driver changes the base name (e.g. wlan* -> eth*).
> Drivers shouldn't do that. It'll change the name of the device and may
But they do...

> We could modify the libertas and gelic (and any other affected) drivers
> to provide a dev_id, make sure it's exported in sysfs, and then use that
> in the udev rules. Would that make you happy?
Yes, I like not fixing just the symptoms. :-)

> It has implications w.r.t. the autoconfigured IPv6 addresses of those
> devices,
How so?

> > This workaround should be confined to the PS3 install media, there is
> > no reason to add it to general-purpose distributions.
> Our general-purpose distribution installs and runs on PS3. :)
So if PS3 needs special hacks they should be special-cased to only by
used on PS3 hardware.

-- 
ciao,
Marco

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: udev can't name PS3's network devices correctly

[David Woodhouse]

On Mon, 2008-04-14 at 14:51 +0200, Marco d'Itri wrote: 
> On Apr 14, David Woodhouse <dwmw2@infradead.org> wrote:
> 
> > Actually it's a generic problem. It also occurs on OLPC, where the
> > libertas wireless device presents two interfaces with the same MAC
> And on some Sun systems as well.
> 
> > > Unconditionally adding a KERNEL key means that persistent names will
> > > break if a driver changes the base name (e.g. wlan* -> eth*).
> > Drivers shouldn't do that. It'll change the name of the device and may
> But they do...

And when they do, they change the name that userspace sees. Film at 11 :)

> > We could modify the libertas and gelic (and any other affected) drivers
> > to provide a dev_id, make sure it's exported in sysfs, and then use that
> > in the udev rules. Would that make you happy?
> Yes, I like not fixing just the symptoms. :-)

I wouldn't put it like that. We are looking for some way to tell the
difference between the two interfaces. If the kernel uses a different
basename (wlan%d vs. eth%d, or eth%d vs. msh%d), that's a fairly strong
hint about which is which -- and is fairly reliable to boot.

The dev_id provides _another_ strong hint which helps us tell which
device is which, but that doesn't mean that using the basename is wrong.

> > It has implications w.r.t. the autoconfigured IPv6 addresses of those
> > devices,
> How so?

The low 64 bits of the autoconfigured IPv6 address are derived from the
MAC address, so for example a MAC address of 00:50:43:28:0A:FC would
give you an IPv6 link-local address of fe80::250:43ff:fe28:afc -- and a
corresponding global address like 2001:8b0:10b:1:250:43ff:fe28:afc.

When you set dev_id to anything other than zero, that 'ff:fe' in the
middle changes. So with my patches, the mesh device on the same libertas
hardware becomes fe80::50:4300:128:afc -- using the newly-provided
dev_id (0x0001) instead of 0xFFFE.

See net/ipv6/addrconf.c for more details. But basically, it means we are
changing the public IPv6 addresses of the affected machines, just to
help udev tell the difference between the interfaces -- when in fact, it
already _could_ tell the difference, due to the kernel's choice of name.

We should do the dev_id thing in sysfs and udev anyway, because there
are cases where it's the right thing to do (like S390, where I think
it'll let us drop the existing hack we have, and probably also the Sun
hardware you mention).

But I don't think we should add dev_id to the gelic and libertas
drivers. We should just make write_net_rules consistently include the
KERNEL criterion -- even when $INTERFACE_NAME is set. It does it in all
other cases anyway, now.

> > > This workaround should be confined to the PS3 install media, there is
> > > no reason to add it to general-purpose distributions.
> > Our general-purpose distribution installs and runs on PS3. :)
> So if PS3 needs special hacks they should be special-cased to only by
> used on PS3 hardware.

It's not a problem which is limited to PS3.

-- 
dwmw2