| Message ID | m17hmhrl6v.fsf_-_@fess.ebiederm.org |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Jun 3, 2010 at 02:53, Eric W. Biederman <ebiederm@xmission.com> wrote: > > In the last painful restructuring of sysfs we created started > creating class directories under normal devices so we could place > devices such as network devices directly under their the hardware > that implements them instead of in their class directories like > /sys/class/net/. This creation of class directories avoids the > need to worry about namespace clonflicts if something is renamed. > > A special exception was made for devices that were still placed > directly in their class directory. Looking at how this interacts > with the wireless network devices it appears this special exception > is either completely unneeded or at least needs to be restricted to > a parent device with the same class as the child device. Certainly > in the case of unrelated classes we very much have the possibility > of namespace classes and we should be creating the subdirectory. The class-glue-directories are only created between a bus-parent and and a class device. Class devices usually don't have other class devices as parents, that's why it wasn't done that way. If people use class devices from other classes as parents, they should definitely convert the class that acts as a parent to a bus, to fit into the usual model. All that was really never meant to be used that way. The current behavior, to not to create the glue-directory is at least the intended one from the driver core's perspective. What kind of classes do this, where this change would help or would be needed? I don't mind trying if that change will work for people, I can't tell if there are any other users doing things like this which could break with such a change. Stuff like udev will be fine with directories inserted, but there are many things out there, that just access their parents attributes with ../../foo, which might no longer work when we insert directories. Thanks, 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
Kay Sievers <kay.sievers@vrfy.org> writes: > On Thu, Jun 3, 2010 at 02:53, Eric W. Biederman <ebiederm@xmission.com> wrote: >> >> In the last painful restructuring of sysfs we created started >> creating class directories under normal devices so we could place >> devices such as network devices directly under their the hardware >> that implements them instead of in their class directories like >> /sys/class/net/. This creation of class directories avoids the >> need to worry about namespace clonflicts if something is renamed. >> >> A special exception was made for devices that were still placed >> directly in their class directory. Looking at how this interacts >> with the wireless network devices it appears this special exception >> is either completely unneeded or at least needs to be restricted to >> a parent device with the same class as the child device. Certainly >> in the case of unrelated classes we very much have the possibility >> of namespace classes and we should be creating the subdirectory. > > The class-glue-directories are only created between a bus-parent and > and a class device. Class devices usually don't have other class > devices as parents, that's why it wasn't done that way. > If people use class devices from other classes as parents, they should > definitely convert the class that acts as a parent to a bus, to fit > into the usual model. All that was really never meant to be used that > way. The current behavior, to not to create the glue-directory is at > least the intended one from the driver core's perspective. > > What kind of classes do this, where this change would help or would be needed? > I don't mind trying if that change will work for people, I can't tell > if there are any other users doing things like this which could break > with such a change. Stuff like udev will be fine with directories > inserted, but there are many things out there, that just access their > parents attributes with ../../foo, which might no longer work when we > insert directories. To the best of my knowledge we are talking a very limited number of real world cases. The driver in particular that causes problems is mac80211_hwsim. It winds up placing network devices in a directory that isn't prepared to take network namespace tagged members, with the result that when the module is removed we don't delete the symlinks from /sys/class/net/. I see no reason to believe we are free of possible namespace conflicts either, which is why I suggested the patch. From my perspective not creating the directory in some weird corner case that appears to practically to never happen looks like an ugly nasty special case. If the solution winds up being converting mac80211_hwsim to using a bus instead of a class that seems reasonable to me as well. More code in one place to remove the chance of problems elsewhere. 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
On Wed, 2010-06-02 at 17:53 -0700, Eric W. Biederman wrote: > Johannes this should fix your issue with mac80211_hwsim, where > the device symlink were not destroyed when the driver was removed. It does, thank you. FWIW, I'm happy changing hwsim too, but I don't think I quite understand what you're proposing in your other email so I'll leave it up to you since you now know what is causing the problem :) 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
On Fri, Jun 4, 2010 at 08:54, Johannes Berg <johannes@sipsolutions.net> wrote: > On Wed, 2010-06-02 at 17:53 -0700, Eric W. Biederman wrote: > >> Johannes this should fix your issue with mac80211_hwsim, where >> the device symlink were not destroyed when the driver was removed. > > It does, thank you. > > FWIW, I'm happy changing hwsim too, but I don't think I quite understand > what you're proposing in your other email so I'll leave it up to you > since you now know what is causing the problem :) Assuming that hwsim is th parent of the network interface, it should us a "struct bus_type" not a "struct class" for the subsystem it assigns the devices to. Classes should not be used for anything completely simple, at best not be used at all, they are just too simple. We never know about future requirements, which usually all go wrong with the non-extendable class logic. The difference in the code to switch from class to bus should be minimal. Cheers, 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
On Fri, 2010-06-04 at 10:15 +0200, Kay Sievers wrote: > On Fri, Jun 4, 2010 at 08:54, Johannes Berg <johannes@sipsolutions.net> wrote: > > On Wed, 2010-06-02 at 17:53 -0700, Eric W. Biederman wrote: > > > >> Johannes this should fix your issue with mac80211_hwsim, where > >> the device symlink were not destroyed when the driver was removed. > > > > It does, thank you. > > > > FWIW, I'm happy changing hwsim too, but I don't think I quite understand > > what you're proposing in your other email so I'll leave it up to you > > since you now know what is causing the problem :) > > Assuming that hwsim is th parent of the network interface, it should > us a "struct bus_type" not a "struct class" for the subsystem it > assigns the devices to. It's all virtual, so yeah, I guess it is the parent? It currently creates a virtual struct device in the hwsim class and assigns that to the netdev parent indirectly via the wiphy or something like that. > Classes should not be used for anything completely simple, at best not > be used at all, they are just too simple. We never know about future > requirements, which usually all go wrong with the non-extendable class > logic. > > The difference in the code to switch from class to bus should be minimal. Does that mean cfg80211 (net/wireless/) should also not use a struct class? I'm not familiar with any of these details, mind helping me out? 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
On Fri, Jun 4, 2010 at 10:28, Johannes Berg <johannes@sipsolutions.net> wrote: > On Fri, 2010-06-04 at 10:15 +0200, Kay Sievers wrote: >> On Fri, Jun 4, 2010 at 08:54, Johannes Berg <johannes@sipsolutions.net> wrote: >> > On Wed, 2010-06-02 at 17:53 -0700, Eric W. Biederman wrote: >> > >> >> Johannes this should fix your issue with mac80211_hwsim, where >> >> the device symlink were not destroyed when the driver was removed. >> > >> > It does, thank you. >> > >> > FWIW, I'm happy changing hwsim too, but I don't think I quite understand >> > what you're proposing in your other email so I'll leave it up to you >> > since you now know what is causing the problem :) >> >> Assuming that hwsim is th parent of the network interface, it should >> us a "struct bus_type" not a "struct class" for the subsystem it >> assigns the devices to. > > It's all virtual, so yeah, I guess it is the parent? It currently > creates a virtual struct device in the hwsim class and assigns that to > the netdev parent indirectly via the wiphy or something like that. > >> Classes should not be used for anything completely simple, at best not >> be used at all, they are just too simple. We never know about future >> requirements, which usually all go wrong with the non-extendable class >> logic. >> >> The difference in the code to switch from class to bus should be minimal. > > Does that mean cfg80211 (net/wireless/) should also not use a struct > class? I'm not familiar with any of these details, mind helping me out? Everything that might ever have a child device, or might ever need a sysfs attribute gobal to the subsystem must convert to a bus. Actually there is not much reason to ever use "struct class" today. The layout for classes in /sys is not extendable like it is for buses which put all devices in a devices/ subdir and have the main subsystem directory to add custom things. 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
On Fri, 2010-06-04 at 10:34 +0200, Kay Sievers wrote: > >> Assuming that hwsim is th parent of the network interface, it should > >> us a "struct bus_type" not a "struct class" for the subsystem it > >> assigns the devices to. > > > > It's all virtual, so yeah, I guess it is the parent? It currently > > creates a virtual struct device in the hwsim class and assigns that to > > the netdev parent indirectly via the wiphy or something like that. > > > >> Classes should not be used for anything completely simple, at best not > >> be used at all, they are just too simple. We never know about future > >> requirements, which usually all go wrong with the non-extendable class > >> logic. > >> > >> The difference in the code to switch from class to bus should be minimal. > > > > Does that mean cfg80211 (net/wireless/) should also not use a struct > > class? I'm not familiar with any of these details, mind helping me out? > > Everything that might ever have a child device, or might ever need a > sysfs attribute gobal to the subsystem must convert to a bus. > > Actually there is not much reason to ever use "struct class" today. > The layout for classes in /sys is not extendable like it is for buses > which put all devices in a devices/ subdir and have the main subsystem > directory to add custom things. Ok, I don't get it. For hwsim, we create entirely virtual "struct device"s. I think we just need that so userspace like network-manager doesn't get completely confused. BUT: device_create() needs a struct class parameter as the first parameter. So should we have a virtual class _and_ a virtual bus?? Similarly for wireless itself (net/wireless/), we use device_initialize() and set up the dev.class and dev.platform_data (not sure why platform data?) since this is a virtual abstraction. Basically we want to show in sysfs that physical device +--- (virtual) wireless phy device 1 +--- netdev 1 +--- netdev 2 +--- netdev 3 +--- (virtual) wireless phy device 2 +--- netdev 4 Although the second wireless phy device is only done by ath9k and will be removed soon. Still we should show that those netdevs all belong to the given wireless phy device. So ... what should I do? How can I do device_create/device_initialize without a struct class? And why a bus instead? It's not like there are devices that need to be discovered and bound to it etc. 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
On Sun, Jun 6, 2010 at 15:08, Johannes Berg <johannes@sipsolutions.net> wrote: > On Fri, 2010-06-04 at 10:34 +0200, Kay Sievers wrote: >> Actually there is not much reason to ever use "struct class" today. >> The layout for classes in /sys is not extendable like it is for buses >> which put all devices in a devices/ subdir and have the main subsystem >> directory to add custom things. > > Ok, I don't get it. > > For hwsim, we create entirely virtual "struct device"s. I think we just > need that so userspace like network-manager doesn't get completely > confused. BUT: device_create() needs a struct class parameter as the > first parameter. So should we have a virtual class _and_ a virtual bus?? > > Similarly for wireless itself (net/wireless/), we use > device_initialize() and set up the dev.class and dev.platform_data (not > sure why platform data?) since this is a virtual abstraction. Basically > we want to show in sysfs that > > physical device > +--- (virtual) wireless phy device 1 > +--- netdev 1 > +--- netdev 2 > +--- netdev 3 > +--- (virtual) wireless phy device 2 > +--- netdev 4 > > Although the second wireless phy device is only done by ath9k and will > be removed soon. Still we should show that those netdevs all belong to > the given wireless phy device. > > So ... what should I do? How can I do device_create/device_initialize > without a struct class? There is no real difference between classes and buses. Actually we're working on merging them completely inside the kernel. Just declare a "struct bus_type" instead of a "struct class". > And why a bus instead? It's not like there are > devices that need to be discovered and bound to it etc. Because class devices are not meat to ever have children from other classes. This is just not what they should be used like. Also the flat directories in /sys/class/<name>/* are not extendable with other stuff, and they should not be used for new stuff. You can never put subsystem-wide properties to that directory without confusing userspace. That is not a problem at all with buses, as they have a dedicated devices/ subdir in the bus directory. If there are no legacy reason, or things which are already a class, and they should look similar, no new classes should be added to the kernel. 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
On Sun, 2010-06-06 at 19:17 +0200, Kay Sievers wrote: > There is no real difference between classes and buses. Actually we're > working on merging them completely inside the kernel. Just declare a > "struct bus_type" instead of a "struct class". Can you please tell me then how to device_create() without a class? I cannot seem to create devices without a class at all, even using manual allocation (yuck) and device_register crashes the kernel. 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
On Mon, Jun 7, 2010 at 11:42, Johannes Berg <johannes@sipsolutions.net> wrote: > On Sun, 2010-06-06 at 19:17 +0200, Kay Sievers wrote: > >> There is no real difference between classes and buses. Actually we're >> working on merging them completely inside the kernel. Just declare a >> "struct bus_type" instead of a "struct class". > > Can you please tell me then how to device_create() without a class? I > cannot seem to create devices without a class at all, even using manual > allocation (yuck) and device_register crashes the kernel. Right, this "convenience API" does not exist for buses. It's not doing much, just allocates a "struct device" and fills in the few values and calls device_register(). Does your device create a device node? If not, device_create() should not be used anyway, because the corresponding device_destroy() will not do anything. 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
On Mon, 2010-06-07 at 11:53 +0200, Kay Sievers wrote: > > Can you please tell me then how to device_create() without a class? I > > cannot seem to create devices without a class at all, even using manual > > allocation (yuck) and device_register crashes the kernel. > > Right, this "convenience API" does not exist for buses. It's not doing > much, just allocates a "struct device" and fills in the few values and > calls device_register(). > > Does your device create a device node? If not, device_create() should > not be used anyway, because the corresponding device_destroy() will > not do anything. No, it doesn't need a dev node. I tried this: + data->dev = kzalloc(sizeof(struct device), GFP_KERNEL); + if (!data->dev) { err = -ENOMEM; goto failed_drvdata; } + + dev_set_name(data->dev, "hwsim%d", i); + data->dev->bus = &hwsim_bus; data->dev->driver = &mac80211_hwsim_driver; + err = device_register(data->dev); + if (err) { + printk(KERN_DEBUG + "mac80211_hwsim: device_register failed (%d)\n", + err); + goto failed_drvdata; + } (ignore the pluses, snipped from a patch) but it ran into a null ptr deref? 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
On Mon, Jun 7, 2010 at 12:14, Johannes Berg <johannes@sipsolutions.net> wrote: > On Mon, 2010-06-07 at 11:53 +0200, Kay Sievers wrote: > >> > Can you please tell me then how to device_create() without a class? I >> > cannot seem to create devices without a class at all, even using manual >> > allocation (yuck) and device_register crashes the kernel. >> >> Right, this "convenience API" does not exist for buses. It's not doing >> much, just allocates a "struct device" and fills in the few values and >> calls device_register(). >> >> Does your device create a device node? If not, device_create() should >> not be used anyway, because the corresponding device_destroy() will >> not do anything. > > No, it doesn't need a dev node. I tried this: > > + data->dev = kzalloc(sizeof(struct device), GFP_KERNEL); > + if (!data->dev) { > err = -ENOMEM; > goto failed_drvdata; > } > + > + dev_set_name(data->dev, "hwsim%d", i); > + data->dev->bus = &hwsim_bus; > data->dev->driver = &mac80211_hwsim_driver; > > + err = device_register(data->dev); > + if (err) { > + printk(KERN_DEBUG > + "mac80211_hwsim: device_register failed (%d)\n", > + err); > + goto failed_drvdata; > + } > > (ignore the pluses, snipped from a patch) but it ran into a null ptr > deref? Oh, I see. It's probably something nobody ever did before. You try to register a bus device which has no parent. Seems that's something nobody ever expected to happen. :) Your driver/subsystem is completely virtual, does not depend on any hardware, right? If we create a virtual parent, like: parent = kzalloc(sizeof(struct device), GFP_KERNEL); dev_set_name(parent, "mac80211_hwsim"); device_register(parent); An in your code: data->dev->parent = parent; That should give you a /sys/devices/mac80211_hwsim/ directory where all the devices you create should show up. If that works as expected, we should probably add something like: struct device *device_virtual_parent(const char *name); which will allow you to create such a parent device in the /sys/device/virtual/ directory. Let me know if the above hack with the virtual parent works, then we can check what do add to the core. Thanks, 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
On Mon, 2010-06-07 at 13:05 +0200, Kay Sievers wrote: > > + data->dev = kzalloc(sizeof(struct device), GFP_KERNEL); > > + if (!data->dev) { > > err = -ENOMEM; > > goto failed_drvdata; > > } > > + > > + dev_set_name(data->dev, "hwsim%d", i); > > + data->dev->bus = &hwsim_bus; > > data->dev->driver = &mac80211_hwsim_driver; > > > > + err = device_register(data->dev); > > + if (err) { > > + printk(KERN_DEBUG > > + "mac80211_hwsim: device_register failed (%d)\n", > > + err); > > + goto failed_drvdata; > > + } > > > > (ignore the pluses, snipped from a patch) but it ran into a null ptr > > deref? > > Oh, I see. It's probably something nobody ever did before. You try to > register a bus device which has no parent. Seems that's something > nobody ever expected to happen. :) > > Your driver/subsystem is completely virtual, does not depend on any > hardware, right? If we create a virtual parent, like: > parent = kzalloc(sizeof(struct device), GFP_KERNEL); > dev_set_name(parent, "mac80211_hwsim"); > device_register(parent); > > An in your code: > data->dev->parent = parent; > > That should give you a /sys/devices/mac80211_hwsim/ directory where > all the devices you create should show up. So that seemed equivalent to my code except for the .bus/.driver assignments and the two-level hierarchy of course. (mind you, I think we probably need to have the bus/driver assignment, but I wanted to try out your suggestion first) So I removed bus/driver assignment from the above code just to try it out, and got Device 'hwsim0' does not have a release() function, it is broken and must be fixed. This has evolved far too much for me right now. Can we just apply the initial patch from Eric and be happier for a while? I can't justify spending this much time on it right now. Alternatively, you could look at hwsim too, since it's all virtual, nothing special is required, I do testing in a virtual machine ... current patch is at http://johannes.sipsolutions.net/patches/kernel/all/2010-06-07-11:41/hwsim-bus.patch 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
diff --git a/drivers/base/core.c b/drivers/base/core.c index 9630fbd..3725f81 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) return &parent->kobj; else parent_kobj = &parent->kobj;