diff mbox

Driver-core: Fix bluetooth network device rename regression

Message ID m1eievugon.fsf@fess.ebiederm.org
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Eric W. Biederman July 23, 2010, 1:31 a.m. UTC
Greg KH <gregkh@suse.de> writes:

> On Thu, Jul 22, 2010 at 08:36:41PM +0200, Johannes Berg wrote:
>> On Thu, 2010-07-22 at 11:28 -0700, Greg KH wrote:
>> 
>> > It worked only because no one realized that it was broken with the
>> > DEPRECATED option enabled.  When that is enabled, it is broken, right?
>> 
>> I'm pretty sure I always had that enabled, and never had issues. Can't
>> test right now since I don't have that option back yet in the tree I'm
>> using.
>> 
>> > Eric's changes to sysfs to add namespace support exposed this breakage.
>> > That's not a reason to paper over the problem, but it should be driving
>> > someone to fix it correctly, as has been pointed out a number of times
>> > already.
>> 
>> I'm just contesting that that someone should be me. I don't think you
>> get to blame driver developers for doing something that worked and
>> solved the problem they needed to solve. sysfs is largely opaque to most
>> of us already, and it now sure feels like Kay decided to change the
>> rules underneath the code in saying "this was wrong all along".
>
> Well, if it worked before, and it doesn't now, that's due to Eric's
> changes, nothing Kay and I did here :)

It mostly worked before, and it works with CONFIG_SYSFS_DEPRECATED=y
(After I my stupid bug is fixed).

With CONFIG_SYSFS_DEPRECATED=y we have in sysfs:
........./net:bnep0 -> /sys/class/net/bnep0
/sys/class/net/bnep0 is a directory.

With CONFIG_SYSFS_DEPRECATED=n we have in sysfs:
........./bnep0
/sys/class/net/bnep0 -> ......./bnep0

Where for a normal network device we have:
With CONFIG_SYSFS_DEPRECATED=y we have in sysfs:
........./net:eth0 -> /sys/class/net/eth0
/sys/class/net/bnep0 is a directory.

With CONFIG_SYSFS_DEPRECATED=n we have in sysfs:
........./net/eth0
/sys/class/net/eth0 -> ......./net/eth0

The new sysfs layout loses the net namespace separator for bluetooth
devices.

> But, in looking at it closer, it does seem that the code is doing things
> that was not expected to work at all previously, and It's amazing that
> it did.  I thought Kay offered to help fix it all up, and provided 2
> different ways to do it.  I know they aren't trivial, but then again,
> your usage of sysfs is not trivial either...

I don't expect much just the upper levels to work correctly.  The fact
this works on all but a handful of drivers is what I find dist

Does this version of the change look less bleh worthy?  The effect is
the same as my previous patch but the test is more abstract so the
effect is not strictly limited to /sys/class/net.



Eric
--
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

Comments

Greg KH July 26, 2010, 6:09 p.m. UTC | #1
On Thu, Jul 22, 2010 at 06:31:52PM -0700, Eric W. Biederman wrote:
> Greg KH <gregkh@suse.de> writes:
> 
> > On Thu, Jul 22, 2010 at 08:36:41PM +0200, Johannes Berg wrote:
> >> On Thu, 2010-07-22 at 11:28 -0700, Greg KH wrote:
> >> 
> >> > It worked only because no one realized that it was broken with the
> >> > DEPRECATED option enabled.  When that is enabled, it is broken, right?
> >> 
> >> I'm pretty sure I always had that enabled, and never had issues. Can't
> >> test right now since I don't have that option back yet in the tree I'm
> >> using.
> >> 
> >> > Eric's changes to sysfs to add namespace support exposed this breakage.
> >> > That's not a reason to paper over the problem, but it should be driving
> >> > someone to fix it correctly, as has been pointed out a number of times
> >> > already.
> >> 
> >> I'm just contesting that that someone should be me. I don't think you
> >> get to blame driver developers for doing something that worked and
> >> solved the problem they needed to solve. sysfs is largely opaque to most
> >> of us already, and it now sure feels like Kay decided to change the
> >> rules underneath the code in saying "this was wrong all along".
> >
> > Well, if it worked before, and it doesn't now, that's due to Eric's
> > changes, nothing Kay and I did here :)
> 
> It mostly worked before, and it works with CONFIG_SYSFS_DEPRECATED=y
> (After I my stupid bug is fixed).
> 
> With CONFIG_SYSFS_DEPRECATED=y we have in sysfs:
> ........./net:bnep0 -> /sys/class/net/bnep0
> /sys/class/net/bnep0 is a directory.
> 
> With CONFIG_SYSFS_DEPRECATED=n we have in sysfs:
> ........./bnep0
> /sys/class/net/bnep0 -> ......./bnep0
> 
> Where for a normal network device we have:
> With CONFIG_SYSFS_DEPRECATED=y we have in sysfs:
> ........./net:eth0 -> /sys/class/net/eth0
> /sys/class/net/bnep0 is a directory.
> 
> With CONFIG_SYSFS_DEPRECATED=n we have in sysfs:
> ........./net/eth0
> /sys/class/net/eth0 -> ......./net/eth0
> 
> The new sysfs layout loses the net namespace separator for bluetooth
> devices.
> 
> > But, in looking at it closer, it does seem that the code is doing things
> > that was not expected to work at all previously, and It's amazing that
> > it did.  I thought Kay offered to help fix it all up, and provided 2
> > different ways to do it.  I know they aren't trivial, but then again,
> > your usage of sysfs is not trivial either...
> 
> I don't expect much just the upper levels to work correctly.  The fact
> this works on all but a handful of drivers is what I find dist
> 
> Does this version of the change look less bleh worthy?  The effect is
> the same as my previous patch but the test is more abstract so the
> effect is not strictly limited to /sys/class/net.

What other class type has a namespace at this point in time?
Essentially this is the same exact thing, just in a different format
that obfuscates what you are really doing here.

As you have now sent this to Linus and he took it, well, it seems moot,
right?

kids these days...

greg k-h
--
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
Kay Sievers July 27, 2010, 9:10 a.m. UTC | #2
On Mon, Jul 26, 2010 at 20:09, Greg KH <greg@kroah.com> wrote:
>> Does this version of the change look less bleh worthy?  The effect is
>> the same as my previous patch but the test is more abstract so the
>> effect is not strictly limited to /sys/class/net.
>
> What other class type has a namespace at this point in time?
> Essentially this is the same exact thing, just in a different format
> that obfuscates what you are really doing here.

The patch still looks broken, and does not belong into the core the
way it is done. We denied hacks like this for good reason. But
out-of-the-blue it was a bluetooth naming regression to fix in the
driver core? Interesting!

If someone is going to add namespaces to 'block' or 'input', the sysfs
layout will break, and userspace will be unable to handle the
resulting changes.

Kay
--
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
Greg KH July 27, 2010, 1:49 p.m. UTC | #3
On Tue, Jul 27, 2010 at 11:10:26AM +0200, Kay Sievers wrote:
> On Mon, Jul 26, 2010 at 20:09, Greg KH <greg@kroah.com> wrote:
> >> Does this version of the change look less bleh worthy?  The effect is
> >> the same as my previous patch but the test is more abstract so the
> >> effect is not strictly limited to /sys/class/net.
> >
> > What other class type has a namespace at this point in time?
> > Essentially this is the same exact thing, just in a different format
> > that obfuscates what you are really doing here.
> 
> The patch still looks broken, and does not belong into the core the
> way it is done. We denied hacks like this for good reason. But
> out-of-the-blue it was a bluetooth naming regression to fix in the
> driver core? Interesting!
> 
> If someone is going to add namespaces to 'block' or 'input', the sysfs
> layout will break, and userspace will be unable to handle the
> resulting changes.

When that happens, I'm sure Eric will be willing to fix up any problems
that are found as he is the one that insisted on pushing it to Linus
around our objections.

Right Eric?

And Eric, where are you working on the "real" patches to solve the
problems of the bnet and wireless driver problems so we can remove this
check from the driver core as soon as possible?  Any idea when they will
be done?

thanks,

greg k-h
--
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
Johannes Berg July 27, 2010, 1:59 p.m. UTC | #4
On Tue, 2010-07-27 at 06:49 -0700, Greg KH wrote:

> And Eric, where are you working on the "real" patches to solve the
> problems of the bnet and wireless driver problems so we can remove this
> check from the driver core as soon as possible?  Any idea when they will
> be done?

They'll be done as soon as the driver core offers the required
functionality, which we discussed elsewhere in this thread.

johannes

--
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
Greg KH July 27, 2010, 3:09 p.m. UTC | #5
On Tue, Jul 27, 2010 at 03:59:50PM +0200, Johannes Berg wrote:
> On Tue, 2010-07-27 at 06:49 -0700, Greg KH wrote:
> 
> > And Eric, where are you working on the "real" patches to solve the
> > problems of the bnet and wireless driver problems so we can remove this
> > check from the driver core as soon as possible?  Any idea when they will
> > be done?
> 
> They'll be done as soon as the driver core offers the required
> functionality, which we discussed elsewhere in this thread.

{sigh}

Kay provided a solution for this a number of times already.  What, are
you really going to make him type it up _again_?  That's just completly
unfair to him.

And note, Eric, this was and is, completely due to your changes, so I am
holding you responsible for fixing it correctly.  Please do so.

greg k-h
--
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
Kay Sievers July 27, 2010, 3:32 p.m. UTC | #6
On Tue, Jul 27, 2010 at 17:09, Greg KH <greg@kroah.com> wrote:
> On Tue, Jul 27, 2010 at 03:59:50PM +0200, Johannes Berg wrote:
>> On Tue, 2010-07-27 at 06:49 -0700, Greg KH wrote:
>>
>> > And Eric, where are you working on the "real" patches to solve the
>> > problems of the bnet and wireless driver problems so we can remove this
>> > check from the driver core as soon as possible?  Any idea when they will
>> > be done?
>>
>> They'll be done as soon as the driver core offers the required
>> functionality, which we discussed elsewhere in this thread.
>
> {sigh}
>
> Kay provided a solution for this a number of times already.  What, are
> you really going to make him type it up _again_?  That's just completly
> unfair to him.

Sure, I still offer any needed help, if there are new requirements now
that need core changes. But hey, they will not land magically, if
nobody is working on it. And "as soon as the core offers" is not how
we make new features work on Linux. The people who add new features,
which uncover old bugs, need to fix the old bugs instead of papering
them over, like it happened here.

> And note, Eric, this was and is, completely due to your changes, so I am
> holding you responsible for fixing it correctly.  Please do so.

I guess faking a 'bluetooth renaming regression', which never existed,
which wasn't even known to the bluetooth maintainers, was just much
easier. :)

Kay
--
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
Eric W. Biederman July 27, 2010, 6:17 p.m. UTC | #7
Greg KH <greg@kroah.com> writes:

> On Tue, Jul 27, 2010 at 11:10:26AM +0200, Kay Sievers wrote:
>> On Mon, Jul 26, 2010 at 20:09, Greg KH <greg@kroah.com> wrote:
>> >> Does this version of the change look less bleh worthy?  The effect is
>> >> the same as my previous patch but the test is more abstract so the
>> >> effect is not strictly limited to /sys/class/net.
>> >
>> > What other class type has a namespace at this point in time?
>> > Essentially this is the same exact thing, just in a different format
>> > that obfuscates what you are really doing here.
>> 
>> The patch still looks broken, and does not belong into the core the
>> way it is done. We denied hacks like this for good reason. But
>> out-of-the-blue it was a bluetooth naming regression to fix in the
>> driver core? Interesting!

This is hardly out of the blue.  I reported a problem 3 years ago
which is one of the reasons we have the class directories in the
first place.

What is different is simply that it was noticed that there were
cases the existing heuristic in get_device_parent did not catch.

Any class that has sysfs namespace support requires the class directory.
Which means that the test !dev->class->ns_type is exactly the right
test to avoid the magic exception in get_device_parent.

>> If someone is going to add namespaces to 'block' or 'input', the sysfs
>> layout will break, and userspace will be unable to handle the
>> resulting changes.

Yes.  We don't even have a namespace that we can put those in, so worrying
about namespaces other than the network namespace or the user namespace
is looking much too far into the future.

What is in the near term is wireless phys getting network namespace support
in sysfs, and in practice the exact same issues apply.

> When that happens, I'm sure Eric will be willing to fix up any problems
> that are found as he is the one that insisted on pushing it to Linus
> around our objections.
>
> Right Eric?

Yes.  I certainly will.  The point of the namespace support in sysfs
is to keep these things invisible to userspace.  If something is visible
it is generally a bug.

> And Eric, where are you working on the "real" patches to solve the
> problems of the bnet and wireless driver problems so we can remove this
> check from the driver core as soon as possible?  Any idea when they will
> be done?

I looked at that, and it is arguable in the mac80211_hwsim case as that
is just a simulation for testing the infrastructure for wireless developers
as best as I understand it.  The more a simulation differs from reality
the more it matters.

For bluetooth drivers or for any other existing subsystem replacing a class
device with a bus device is totally inappropriate.

Let me say this again clearly.

class -> bus BROKEN

In the case of bluetooth it would require changing /sys/class/bluetooth
to /sys/bus/bluetooth.  Which is user visible in the worst possible
way and quick google search confirmed it will break user space scripts.

So as much as I sympathize with the position that buses have a better
layout than classes, after having examined the issue I completely oppose
changing one into the other with our current code base.

So for the medium term I think we are fine.

For the long term moving the driver core from a mandatory policy of
sysfs device node creation, to a default library for sysfs device node
creation that can be overridden in whole or in part on a subsystem by
subsystem basis looks like the right way to go.  Based on my
experience with sysfs this is likely to be a multi-year project, and 
one I'm uncertain how motivated I am to tackle.

Eric

--
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
Kay Sievers July 27, 2010, 6:24 p.m. UTC | #8
On Tue, Jul 27, 2010 at 20:17, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Greg KH <greg@kroah.com> writes:

> Let me say this again clearly.
>
> class -> bus BROKEN
>
> In the case of bluetooth it would require changing /sys/class/bluetooth
> to /sys/bus/bluetooth.  Which is user visible in the worst possible
> way and quick google search confirmed it will break user space scripts.

Clearly, we even have a compat API for that, and nothing would break.
If needed, a bus can create compat class links. Did you even check
with the bluetooth guys, last time I talked to them, they have been
totally fine with such a change.

Kay
--
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
Greg KH July 27, 2010, 6:36 p.m. UTC | #9
On Tue, Jul 27, 2010 at 08:24:31PM +0200, Kay Sievers wrote:
> On Tue, Jul 27, 2010 at 20:17, Eric W. Biederman <ebiederm@xmission.com> wrote:
> > Greg KH <greg@kroah.com> writes:
> 
> > Let me say this again clearly.
> >
> > class -> bus BROKEN
> >
> > In the case of bluetooth it would require changing /sys/class/bluetooth
> > to /sys/bus/bluetooth. ??Which is user visible in the worst possible
> > way and quick google search confirmed it will break user space scripts.
> 
> Clearly, we even have a compat API for that, and nothing would break.
> If needed, a bus can create compat class links. Did you even check
> with the bluetooth guys, last time I talked to them, they have been
> totally fine with such a change.

Yeah, I even had patches that almost did this work a few years ago.
Most of the bluetooth class stuff (if not all of it) can be safely moved
to debugfs, as it's just debug information.

I should dig them out and refresh them in my spare time :)

thanks,

greg k-h
--
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
Eric W. Biederman July 27, 2010, 6:44 p.m. UTC | #10
Greg KH <greg@kroah.com> writes:

> On Tue, Jul 27, 2010 at 08:24:31PM +0200, Kay Sievers wrote:
>> On Tue, Jul 27, 2010 at 20:17, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> > Greg KH <greg@kroah.com> writes:
>> 
>> > Let me say this again clearly.
>> >
>> > class -> bus BROKEN
>> >
>> > In the case of bluetooth it would require changing /sys/class/bluetooth
>> > to /sys/bus/bluetooth. ??Which is user visible in the worst possible
>> > way and quick google search confirmed it will break user space scripts.
>> 
>> Clearly, we even have a compat API for that, and nothing would break.
>> If needed, a bus can create compat class links. Did you even check
>> with the bluetooth guys, last time I talked to them, they have been
>> totally fine with such a change.

Hmm.  It appears I missed the compat API in my review of the bus code.
Could I get a pointer?

> Yeah, I even had patches that almost did this work a few years ago.
> Most of the bluetooth class stuff (if not all of it) can be safely moved
> to debugfs, as it's just debug information.

That change merged about March, so that is no longer an issue.

Eric
--
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
Kay Sievers July 27, 2010, 6:54 p.m. UTC | #11
On Tue, Jul 27, 2010 at 20:44, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Greg KH <greg@kroah.com> writes:
>> On Tue, Jul 27, 2010 at 08:24:31PM +0200, Kay Sievers wrote:
>>> On Tue, Jul 27, 2010 at 20:17, Eric W. Biederman <ebiederm@xmission.com> wrote:
>>> > Greg KH <greg@kroah.com> writes:
>>>
>>> > Let me say this again clearly.
>>> >
>>> > class -> bus BROKEN
>>> >
>>> > In the case of bluetooth it would require changing /sys/class/bluetooth
>>> > to /sys/bus/bluetooth. ??Which is user visible in the worst possible
>>> > way and quick google search confirmed it will break user space scripts.
>>>
>>> Clearly, we even have a compat API for that, and nothing would break.
>>> If needed, a bus can create compat class links. Did you even check
>>> with the bluetooth guys, last time I talked to them, they have been
>>> totally fine with such a change.
>
> Hmm.  It appears I missed the compat API in my review of the bus code.
> Could I get a pointer?

It's:
  class_compat_create_link()
  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/base/class.c;h=8e231d05b40058c6c3191b4a06a15ceff1714be3;hb=HEAD#l559

I think i2c uses this when the class was converted to a bus.

It should only be used if it's really needed for known used userspace
interfaces. A few others that got converted already did not need it.

Kay
--
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
Eric W. Biederman July 27, 2010, 8:53 p.m. UTC | #12
Kay Sievers <kay.sievers@vrfy.org> writes:

> On Tue, Jul 27, 2010 at 20:44, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> Greg KH <greg@kroah.com> writes:
>>> On Tue, Jul 27, 2010 at 08:24:31PM +0200, Kay Sievers wrote:
>>>> On Tue, Jul 27, 2010 at 20:17, Eric W. Biederman <ebiederm@xmission.com> wrote:
>>>> > Greg KH <greg@kroah.com> writes:
>>>>
>>>> > Let me say this again clearly.
>>>> >
>>>> > class -> bus BROKEN
>>>> >
>>>> > In the case of bluetooth it would require changing /sys/class/bluetooth
>>>> > to /sys/bus/bluetooth. ??Which is user visible in the worst possible
>>>> > way and quick google search confirmed it will break user space scripts.
>>>>
>>>> Clearly, we even have a compat API for that, and nothing would break.
>>>> If needed, a bus can create compat class links. Did you even check
>>>> with the bluetooth guys, last time I talked to them, they have been
>>>> totally fine with such a change.
>>
>> Hmm.  It appears I missed the compat API in my review of the bus code.
>> Could I get a pointer?
>
> It's:
>   class_compat_create_link()
>   http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/base/class.c;h=8e231d05b40058c6c3191b4a06a15ceff1714be3;hb=HEAD#l559
>
>
> I think i2c uses this when the class was converted to a bus.

> It should only be used if it's really needed for known used userspace
> interfaces. A few others that got converted already did not need it.

Interesting.  The symlink creation is slightly buggy in that it is
created after the uevent for device creation has been sent.  Which can
lead to some interesting races in userspace.

As for the rest the bus compat code is similar but not quite the same
as the class code, so I would be extremely reluctant to deploy it
except in extremely limited cases.  Backwards compatibility is
important, and we should strive our best to maintain backwards
compatibility it for the kernel<->userspace ABIs.

Eric
--
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
Kay Sievers July 28, 2010, 4:41 a.m. UTC | #13
On Tue, Jul 27, 2010 at 22:53, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Kay Sievers <kay.sievers@vrfy.org> writes:

>> It should only be used if it's really needed for known used userspace
>> interfaces. A few others that got converted already did not need it.
>
> Interesting.  The symlink creation is slightly buggy in that it is
> created after the uevent for device creation has been sent.  Which can
> lead to some interesting races in userspace.

At uevent time the 'subsystem' is specified by the 'sybsystem' link
and the SUBSYSTEM property in the event environment. The device should
not really rely on finding itself linked in class. The class and bus
link collections are only to find a group of devices of the same
subsystem, and this should not be a real world problem here.

Also there are plans to merge struct class and struct bus_type
completely and keep only a few flags around where stuff should show up
for compatibility. At that point all stuff will be created at the same
time.

> As for the rest the bus compat code is similar but not quite the same
> as the class code, so I would be extremely reluctant to deploy it
> except in extremely limited cases.  Backwards compatibility is
> important, and we should strive our best to maintain backwards
> compatibility it for the kernel<->userspace ABIs.

Today, the only real difference between class and bus devices are
these 'collection links', the devices otherwise look completely the
same. There should be no important difference.

Buses are very much preferred over classes today, no new stuff should
create any class. The bus directories are extendable and have a
reasonable layout with the devices/ subdirectory, unlike the flat
class directories where people got the silly idea to mix devices lists
with attributes to confuse everything.

Kay
--
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
Eric W. Biederman July 28, 2010, 5:12 a.m. UTC | #14
Kay Sievers <kay.sievers@vrfy.org> writes:

> On Tue, Jul 27, 2010 at 22:53, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> Kay Sievers <kay.sievers@vrfy.org> writes:
>
>>> It should only be used if it's really needed for known used userspace
>>> interfaces. A few others that got converted already did not need it.
>>
>> Interesting.  The symlink creation is slightly buggy in that it is
>> created after the uevent for device creation has been sent.  Which can
>> lead to some interesting races in userspace.
>
> At uevent time the 'subsystem' is specified by the 'sybsystem' link
> and the SUBSYSTEM property in the event environment. The device should
> not really rely on finding itself linked in class. The class and bus
> link collections are only to find a group of devices of the same
> subsystem, and this should not be a real world problem here.

I agree that there should be no real world problems.
The bottom line is that every sysfs attribute should be created
before we send the uevent or else we get horribly subtle
races or we get user space code that starts looping looking
for the interface.

> Also there are plans to merge struct class and struct bus_type
> completely and keep only a few flags around where stuff should show up
> for compatibility. At that point all stuff will be created at the same
> time.

That part seems reasonable.

>> As for the rest the bus compat code is similar but not quite the same
>> as the class code, so I would be extremely reluctant to deploy it
>> except in extremely limited cases.  Backwards compatibility is
>> important, and we should strive our best to maintain backwards
>> compatibility it for the kernel<->userspace ABIs.
>
> Today, the only real difference between class and bus devices are
> these 'collection links', the devices otherwise look completely the
> same. There should be no important difference.

I don't see the class subdirectories created for bus devices, and I
don't see any equivalent.  At least for the network devices this is a
huge difference, because the device namespace is controlled by
userspace and it is NOT ok to have namespace conflicts with arbitrary
sysfs attributes.

> Buses are very much preferred over classes today, no new stuff should
> create any class. The bus directories are extendable and have a
> reasonable layout with the devices/ subdirectory, unlike the flat
> class directories where people got the silly idea to mix devices lists
> with attributes to confuse everything.

Which is generally reasonable.  However busses appear to have the silly
idea that it is ok to mix child child device lists of different kinds
of children with attributes and confuse everything.

At the subsystem level bus devices look better.
At the individual device level bus devices stacked on bus devices 
appear to be a namespace disaster.

Eric

--
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
Kay Sievers July 28, 2010, 5:26 a.m. UTC | #15
On Wed, Jul 28, 2010 at 07:12, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Kay Sievers <kay.sievers@vrfy.org> writes:
>> On Tue, Jul 27, 2010 at 22:53, Eric W. Biederman <ebiederm@xmission.com> wrote:
>>> Kay Sievers <kay.sievers@vrfy.org> writes:
>>
>>>> It should only be used if it's really needed for known used userspace
>>>> interfaces. A few others that got converted already did not need it.
>>>
>>> Interesting.  The symlink creation is slightly buggy in that it is
>>> created after the uevent for device creation has been sent.  Which can
>>> lead to some interesting races in userspace.
>>
>> At uevent time the 'subsystem' is specified by the 'sybsystem' link
>> and the SUBSYSTEM property in the event environment. The device should
>> not really rely on finding itself linked in class. The class and bus
>> link collections are only to find a group of devices of the same
>> subsystem, and this should not be a real world problem here.
>
> I agree that there should be no real world problems.
> The bottom line is that every sysfs attribute should be created
> before we send the uevent or else we get horribly subtle
> races or we get user space code that starts looping looking
> for the interface.

Yeah, but most of these things we should have fixed over the last
years. There is no single WAIT_FOR instruction left in udev rules. :)

>> Also there are plans to merge struct class and struct bus_type
>> completely and keep only a few flags around where stuff should show up
>> for compatibility. At that point all stuff will be created at the same
>> time.
>
> That part seems reasonable.
>
>>> As for the rest the bus compat code is similar but not quite the same
>>> as the class code, so I would be extremely reluctant to deploy it
>>> except in extremely limited cases.  Backwards compatibility is
>>> important, and we should strive our best to maintain backwards
>>> compatibility it for the kernel<->userspace ABIs.
>>
>> Today, the only real difference between class and bus devices are
>> these 'collection links', the devices otherwise look completely the
>> same. There should be no important difference.
>
> I don't see the class subdirectories created for bus devices, and I
> don't see any equivalent.

Sure, I meant, if the class-compat stuff is used when a conversion is done.

> At least for the network devices this is a
> huge difference, because the device namespace is controlled by
> userspace and it is NOT ok to have namespace conflicts with arbitrary
> sysfs attributes.

Yeah, that's why the class-glue directories exist. But so far we did
not support stacking of classes of different types.

>> Buses are very much preferred over classes today, no new stuff should
>> create any class. The bus directories are extendable and have a
>> reasonable layout with the devices/ subdirectory, unlike the flat
>> class directories where people got the silly idea to mix devices lists
>> with attributes to confuse everything.
>
> Which is generally reasonable.  However buses appear to have the silly
> idea that it is ok to mix child child device lists of different kinds
> of children with attributes and confuse everything.

Classes do the same. They are usually distinguished by its name, like
mouseX, inputX, sdX, sdX3, they are all different, but still belong to
the same 'subsystem'.

> At the subsystem level bus devices look better.
> At the individual device level bus devices stacked on bus devices
> appear to be a namespace disaster.

They are usually created by the same code, in many cases by the same
drivers, and have not been a real problem so far. As you said, network
devices are special here, because of the ability to rename them from
userspace.

At some time in the future, when buses and classes are merged, I
expect stuff can just set a flag to have a 'glue dir' created or not.

For now 'glue dirs' are limited to be created between a bus and a
class device. It could possibly be extended to be created between
classes of different types to handle issues like this.

Kay
--
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
Eric W. Biederman July 28, 2010, 7:57 a.m. UTC | #16
Kay Sievers <kay.sievers@vrfy.org> writes:

> Yeah, but most of these things we should have fixed over the last
> years. There is no single WAIT_FOR instruction left in udev rules. :)

Last time I looked there were quite a few attributes that were still
getting created late.  I would not be surprised if the common case
works fine, but I know of a least one and I think a couple of weird
cases that still have to do unpleasant things.

Still that is a project for another time.


>> At the subsystem level bus devices look better.
>> At the individual device level bus devices stacked on bus devices
>> appear to be a namespace disaster.
>
> They are usually created by the same code, in many cases by the same
> drivers, and have not been a real problem so far. As you said, network
> devices are special here, because of the ability to rename them from
> userspace.
>
> At some time in the future, when buses and classes are merged, I
> expect stuff can just set a flag to have a 'glue dir' created or not.
>
> For now 'glue dirs' are limited to be created between a bus and a
> class device. It could possibly be extended to be created between
> classes of different types to handle issues like this.

Sounds like a plan.  And now I'm off on vacation.

Have a good one.

Eric
--
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 mbox

Patch

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 9630fbd..9b9d3bd 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -673,7 +673,7 @@  static struct kobject *get_device_parent(struct device *dev,
 		 */
 		if (parent == NULL)
 			parent_kobj = virtual_device_parent(dev);
-		else if (parent->class)
+		else if (parent->class && !dev->class->ns_type)
 			return &parent->kobj;
 		else
 			parent_kobj = &parent->kobj;