i2c: Warn when adapters have no parent

We don't need to give adapters a parent if they don't have one. The driver core will put them in the virtual device directory and all will be fine.

Signed-off-by Jean Delvare <[email protected]>

---
Much better than my first attempt. Not sure what will happen if we ever turn i2c-adapters into bus devices. I see that the driver core puts class devices without a parent in virtual, but what about _bus_ devices

Quoted 9 lines · Jean Delvare

We don't need to give adapters a parent if they don't have one. The driver core will put them in the virtual device directory and all will be fine.

Signed-off-by Jean Delvare <[email protected]>

---
Much better than my first attempt. Not sure what will happen if we ever turn i2c-adapters into bus devices. I see that the driver core puts class devices without a parent in virtual, but what about _bus_ devices

I'm sure, that "virtual" logic could be done for bus devices too, if it's needed. I don't see any reason why that wouldn't be fine.

(The less difference between classes and buses the better. It is wrong to have two types of subsystems, doing almost the same thing. One could argue that it could be useful inside the kernel, which it isn't, I think, but exporting them to userspace was definitely the wrong thing.)

Quoted 5 lines · Kay Sievers

(The less difference between classes and buses the better. It is wrong to have two types of subsystems, doing almost the same thing. One could argue that it could be useful inside the kernel, which it isn't, I think, but exporting them to userspace was definitely the wrong thing.)

I finally took a stab at this. The resulting patch is below. I have used device_type to differentiate between I2C clients and I2C adapters. Is this what you had in mind?

It seems to work reasonably well, with the following issues remaining:

* The change breaks at least sensors-detect and libsensors. I can easily modify them so that they work again, but we still have a compatibility issue. Is it possible to have a compatibility option that would add symbolic links from class/i2c-adapter/i2c-* to bus/i2c/devices/i2c-* for a couple years?

* Now that i2c-core makes use of device_type, I tried to move the power management handling callbacks there from bus_type, to save a test in each function, however I found that the callback set is different between bus_type and device_type.pm. Why is it so? Is there a document explaining the difference? Is the whole world (including bus_type) eventually moving to dev_pm_ops?

* When i2c-adapters were class devices, virtual ones (for example i2c-stub) appeared in sysfs as devices/virtual/i2c-adapter/i2c-*, which made sense and seemed safe. Now that I have turned them into bus devices, virtual ones appear in sysfs as devices/i2c-* directly, which looks dirty and could result in collisions someday. What should be done about this? I wanted to use virtual_device_parent() but it is internal to the driver core at the moment, and doesn't even exist if CONFIG_SYSFS_DEPRECATED=y.

I would be grateful if you can advise on any of the above points.

Quoted 11 lines · Jean Delvare

Quoted 5 lines · Kay Sievers

(The less difference between classes and buses the better. It is wrong to have two types of subsystems, doing almost the same thing. One could argue that it could be useful inside the kernel, which it isn't, I think, but exporting them to userspace was definitely the wrong thing.)

I finally took a stab at this. The resulting patch is below. I have used device_type to differentiate between I2C clients and I2C adapters. Is this what you had in mind?

Looks fine, by just looking at the patch.

Quoted 7 lines

It seems to work reasonably well, with the following issues remaining:

* The change breaks at least sensors-detect and libsensors. I can easily modify them so that they work again, but we still have a compatibility issue. Is it possible to have a compatibility option that would add symbolic links from class/i2c-adapter/i2c-* to bus/i2c/devices/i2c-* for a couple years?

Yeah, we can add that. I guess others will need that too, if we convert things from class to bus. How would that look like? Like a device_add_class_compat_link(*dev, *class)?

Quoted 6 lines

* Now that i2c-core makes use of device_type, I tried to move the power management handling callbacks there from bus_type, to save a test in each function, however I found that the callback set is different between bus_type and device_type.pm. Why is it so? Is there a document explaining the difference? Is the whole world (including bus_type) eventually moving to dev_pm_ops?

I think this is already removed in the current git tree, and all should use dev_pm_ops, yes.

Quoted 8 lines

* When i2c-adapters were class devices, virtual ones (for example i2c-stub) appeared in sysfs as devices/virtual/i2c-adapter/i2c-*, which made sense and seemed safe. Now that I have turned them into bus devices, virtual ones appear in sysfs as devices/i2c-* directly, which looks dirty and could result in collisions someday. What should be done about this? I wanted to use virtual_device_parent() but it is internal to the driver core at the moment, and doesn't even exist if CONFIG_SYSFS_DEPRECATED=y.

Yeah, we just need to apply the /sys/devices/virtual logic to bus devices too, it's currently limited to class devices, because there was no bus device so far who needed this, but should be an easy change.

Quoted 1 line

I would be grateful if you can advise on any of the above points.

If you decide to do it that way, you would need the driver core to be able:
- to create a link from an otherwise empty "struct class" to an
existing bus-device
- put bus devices without a parent into the /sys/devices/virtual logic
right? Let me know, I can look into that, if you need that.

Quoted 7 lines · Kay Sievers

Quoted 6 lines

* Now that i2c-core makes use of device_type, I tried to move the power management handling callbacks there from bus_type, to save a test in each function, however I found that the callback set is different between bus_type and device_type.pm. Why is it so? Is there a document explaining the difference? Is the whole world (including bus_type) eventually moving to dev_pm_ops?

Quoted 2 lines

I think this is already removed in the current git tree, and all should use dev_pm_ops, yes.

In which git tree? In Linus' tree, struct bus_type definitely doesn't use dev_pm_ops yet.

Quoted 9 lines

Quoted 8 lines

* When i2c-adapters were class devices, virtual ones (for example i2c-stub) appeared in sysfs as devices/virtual/i2c-adapter/i2c-*, which made sense and seemed safe. Now that I have turned them into bus devices, virtual ones appear in sysfs as devices/i2c-* directly, which looks dirty and could result in collisions someday. What should be done about this? I wanted to use virtual_device_parent() but it is internal to the driver core at the moment, and doesn't even exist if CONFIG_SYSFS_DEPRECATED=y.

Quoted 4 lines

Yeah, we just need to apply the /sys/devices/virtual logic to bus devices too, it's currently limited to class devices, because there was no bus device so far who needed this, but should be an easy change.

It will probably have do be a little different, as it is valid to register a device without a parent, to have it appear at the root (actually 1st level) of the device tree. So we'll need a way to differentiate between this case and the virtual device case.

I admit I am a little surprised that I am the first person asking for virtual bus devices, especially given how you like to repeat that i2c was doing things differently from the rest of the world so far and I am merely changing i2c to fit in the common model.

Quoted 7 lines

Quoted 1 line

I would be grateful if you can advise on any of the above points.

If you decide to do it that way, you would need the driver core to be able:
  - to create a link from an otherwise empty "struct class" to an existing bus-device
  - put bus devices without a parent into the /sys/devices/virtual logic right? Let me know, I can look into that, if you need that.

Yes, this is a good summary of my needs. With some room for discussion on both points:
* Do we need an actually struct class for each fake class, or just a class name?

* Do we want to put virtual devices in devices/virtual directly, or do we want separate namespaces?
But these are details which can be solved on the way, and I have no strong opinion about them anyway.

Quoted 6 lines · Jean Delvare

Quoted 2 lines · Kay Sievers

I think this is already removed in the current git tree, and all should use dev_pm_ops, yes.

In which git tree? In Linus' tree, struct bus_type definitely doesn't use dev_pm_ops yet.

But it removed all the other stuff:
commit 00725787511e20dbd1fdc1fb233606120ae5c8cf
Author: Magnus Damm <damm-AlSX/UN32fvPDbFq/[email protected]>
Date: Thu Jun 4 22:13:25 2009 +0200

PM: Remove device_type suspend()/resume()

Quoted 10 lines

Quoted 4 lines

Yeah, we just need to apply the /sys/devices/virtual logic to bus devices too, it's currently limited to class devices, because there was no bus device so far who needed this, but should be an easy change.

It will probably have do be a little different, as it is valid to register a device without a parent, to have it appear at the root (actually 1st level) of the device tree. So we'll need a way to differentiate between this case and the virtual device case.

Quoted 4 lines

I admit I am a little surprised that I am the first person asking for virtual bus devices, especially given how you like to repeat that i2c was doing things differently from the rest of the world so far and I am merely changing i2c to fit in the common model.

Usually bus devices are a child of root device like pci or some platform stuff. If i2c does not have any parents like this, and is by itself a root for other stuff, maybe we get /sys/devices/i2c, like we have for pci, acpi, ccw (IBM s390), and all these device roots.

Quoted 10 lines

Quoted 5 lines

If you decide to do it that way, you would need the driver core to be able:
  - to create a link from an otherwise empty "struct class" to an existing bus-device
  - put bus devices without a parent into the /sys/devices/virtual logic right? Let me know, I can look into that, if you need that.

Yes, this is a good summary of my needs. With some room for discussion on both points:
* Do we need an actually struct class for each fake class, or just a class name?

We will need to create a kobject for the compat class directory, we will not need a "struct class" for it and can just use a simple pointer to the registered kobject. If we use a string, we would need to find the registered kobject with every call to create a link there, not necessarily bad, but an explicitely registered object might be easier.

Quoted 4 lines

* Do we want to put virtual devices in devices/virtual directly, or do we want separate namespaces?
But these are details which can be solved on the way, and I have no strong opinion about them anyway.

It's basically the question if these objects usually represent real hardware or not. If the root i2c device just happens to have no other existing bus as a parent, we might just want: /sys/devices/i2c/.

Quoted 9 lines

Quoted 2 lines

* Do we need an actually struct class for each fake class, or just a class name?

We will need to create a kobject for the compat class directory, we will not need a "struct class" for it and can just use a simple pointer to the registered kobject. If we use a string, we would need to find the registered kobject with every call to create a link there, not necessarily bad, but an explicitely registered object might be easier.

Any progress on this? I have just committed the patches to sensors-detect and libsensors, and the kernel patch is ready to go, but without the compatibility links it doesn't make any sense to push it upstream

Quoted 4 lines · Jean Delvare

Any progress on this? I have just committed the patches to sensors-detect and libsensors, and the kernel patch is ready to go, but without the compatibility links it doesn't make any sense to push it upstream

Something like this? Please change it as you need. I did only a very quick test.

The only important part is that the kobject of the class directly is not exposed, so nobody else can do weird things with it.
(...)

drivers/base/class.c +26 -0

@@ -488,6 +488,45 @@ void class_interface_unregister(struct c
 	class_put(parent);
 }
 
+struct class_compat {
+	struct kobject *kobj;
+};
+
+struct class_compat *class_compat_register(const char *name)
+{
+	struct class_compat *cls;
+
+	cls = kmalloc(sizeof(struct class_compat), GFP_KERNEL);
+	if (!cls)
+		return NULL;
+	cls->kobj = kobject_create_and_add(name, &class_kset->kobj);
+	if (!cls->kobj) {
+		kfree(cls);
+		return NULL;
+	}
+	return cls;
+}
+EXPORT_SYMBOL_GPL(class_compat_register);
+
+void class_compat_unregister(struct class_compat *cls)
+{
+	kobject_put(cls->kobj);
+	kfree(cls);
+}
+EXPORT_SYMBOL_GPL(class_compat_unregister);

drivers/base/class.c +3 -0

+}
+EXPORT_SYMBOL_GPL(class_compat_remove_link);
+
 int __init classes_init(void)
 {
 	class_kset = kset_create_and_add("class", NULL, NULL);

include/linux/device.h +0 -0

@@ -223,6 +223,12 @@ extern void class_unregister(struct clas
 	__class_register(class, &__key);	\
 })

Quoted 1 line · Kay Sievers

+     return sysfs_remove_link(cls->kobj, dev_name(dev));

I don't think you need the "return" here, as sysfs_remove_link returns void.

Other than that (and in practice even with that) your patch works just fine for me. Thanks! Unfortunately it doesn't provide perfect compatibility, [...] to add this device link (pointing to "..") temporarily or would that be too confusing?

Another thing we have to discuss is the compatibility option. For now I've made it i2c-specific and enabled by default:

config I2C_COMPAT
       boolean "Enable compatibility bits for old user-space" default y
       help
         Say Y here if you intend to run lm-sensors 3.1.1 or older.

But this means a lot of ifdefs in my code (6). With a system-wide option, we could provide empty stubs I could get rid of them. OTOH, It is easier to control the lifetime, and change the default value, of a subsystem specific option. So I'm not too sure what do to.

Maybe it
depends on how many subsystems will need the compatibility layer... Do you have an opinion?

Quoted 2 lines · Jean Delvare

I don't think you need the "return" here, as sysfs_remove_link returns void.

Fixed. Thanks.

Quoted 4 lines

Other than that (and in practice even with that) your patch works just fine for me. Thanks! Unfortunately it doesn't provide perfect compatibility, [...] to add this device link (pointing to "..") temporarily or would that be too confusing?

I think that's ok, if it solves a real problem. The entire idea of _a_ "device" link is pretty flawed, and the reason we ripped all the "struct class_device" devices out.

Quoted 9 lines

Another thing we have to discuss is the compatibility option. For now I've made it i2c-specific and enabled by default:

config I2C_COMPAT
       boolean "Enable compatibility bits for old user-space" default y
       help
         Say Y here if you intend to run lm-sensors 3.1.1 or older.

Quoted 4 lines

But this means a lot of ifdefs in my code (6). With a system-wide option, we could provide empty stubs I could get rid of them. OTOH, It is easier to control the lifetime, and change the default value, of a subsystem specific option. So I'm not too sure what do to.

I'm not sure too. I think it's fine to have it per-subsytem, as only the subsystem knows for how long it is needed, and it can probably be dropped some day.

What's the merge timeline for this patch?

Quoted 8 lines

Quoted 4 lines

Other than that (and in practice even with that) your patch works just fine for me. Thanks! Unfortunately it doesn't provide perfect compatibility, [...] to add this device link (pointing to "..") temporarily or would that be too confusing?

I think that's ok, if it solves a real problem. The entire idea of _a_ "device" link is pretty flawed, and the reason we ripped all the "struct class_device" devices out.

OK, I've added the "device" link and now compatibility works perfectly. I'll post the updated patch series soon, if you want to take a look.

Would it make sense to move the "device" link creation into class_compat_create_link()? I suspect other users of a compatibility class may need it as well.

Quoted 2 lines · Jean Delvare

What's the merge timeline for this patch?

I just did it in the moment you asked, there are no other users so far. Just submit it with your changes that need it, if Greg is ok with that?

Quoted 11 lines

Quoted 8 lines

Quoted 4 lines

Other than that (and in practice even with that) your patch works just fine for me. Thanks! Unfortunately it doesn't provide perfect compatibility, [...] to add this device link (pointing to "..") temporarily or would that be too confusing?

I think that's ok, if it solves a real problem. The entire idea of _a_ "device" link is pretty flawed, and the reason we ripped all the "struct class_device" devices out.

OK, I've added the "device" link and now compatibility works perfectly. I'll post the updated patch series soon, if you want to take a look.

Sounds great.

Quoted 3 lines

Would it make sense to move the "device" link creation into class_compat_create_link()? I suspect other users of a compatibility class may need it as well.

Might make sense, as long as it's not the built-in default.

Sometimes we need to insert devices into the devpath, and then the "device" link does not point to the direct parent, but the next one. So it would need to take another device parameter, if we do that, and also accept NULL, if no such link is really needed.

The "device" link itself is a pretty broken concept, and should be avoided wherever possible, so it should be as optional as we can do it. :)

I have come up with the following
implementation, would that be OK with you? If it is, please add your Signed-off-by.