Hi Patrick, I know you've passed maintainership of the driver model code
on, but I was hoping I could jog your memory.
I've found a bug in drivers/base/bus.c, where the bus_type.devices.list
is treated as a list of device structs. bus_type.devices is a kset
though, so devices.list should contain kobjects rather than devices.
Here is the diff I've come up with:
===== drivers/base/bus.c 1.52 vs edited =====
--- 1.52/drivers/base/bus.c Tue Sep 30 10:59:35 2003
+++ edited/drivers/base/bus.c Thu Jan 22 11:22:15 2004
@@ -18,7 +18,7 @@
#include "base.h"
#include "power/power.h"
-#define to_dev(node) container_of(node,struct device,bus_list)
+#define to_dev(node) container_of(node,struct device,kobj.entry)
#define to_drv(node) container_of(node,struct device_driver,kobj.entry)
#define to_bus_attr(_attr) container_of(_attr,struct bus_attribute,attr)
@@ -164,7 +164,7 @@
if (!(bus = get_bus(bus)))
return -EINVAL;
- head = start ? &start->bus_list : &bus->devices.list;
+ head = start ? &start->kobj.entry : &bus->devices.list;
down_read(&bus->subsys.rwsem);
list_for_each(entry,head) {
@@ -337,7 +337,7 @@
return;
list_for_each(entry,&bus->devices.list) {
- struct device * dev = container_of(entry,struct
device,bus_list);
+ struct device * dev = to_dev(entry);
if (!dev->driver) {
error = bus_match(dev,drv);
if (error && (error != -ENODEV))
@@ -405,7 +405,7 @@
if (bus) {
down_write(&dev->bus->subsys.rwsem);
pr_debug("bus %s: add device %s\n",bus->name,dev->bus_id);
- list_add_tail(&dev->bus_list,&dev->bus->devices.list);
+ list_add_tail(&dev->kobj.entry,&dev->bus->devices.list);
device_attach(dev);
up_write(&dev->bus->subsys.rwsem);
sysfs_create_link(&bus->devices.kobj,&dev->kobj,dev->bus_id);
The first hunk you can see is symmetrical with to_drv just below. The
next hunk, to bus_for_each_dev, makes it match up with bus_for_each_drv
(I take this as evidence that I'm right). The next hunk, in
driver_attach, can be compared with device_attach and again you'll see
the symmetry (to_drv/to_dev).
The last hunk in bus_add_device is where we were inserting the
device.bus_list rather than devices.kobject.entry into the kset.
These are the only 3 users of the devices kset that I could find.
Unfortunately when I make these changes I get a panic like so:
bus_match +0x28
driver_attach +0xa0
bus_add_driver +0xdc
driver_register +0x38
pci_register_driver +0x6c
serial8250_pci_init +0x28
I'm having a bit of trouble figuring out what went wrong. Are there
other users of bus_type.devices that I've missed and need updating? Thanks!
--
Hollis Blanchard
IBM Linux Technology Center
Hollis Blanchard wrote:
> Hi Patrick, I know you've passed maintainership of the driver model code
> on, but I was hoping I could jog your memory.
Sorry, found the Mozilla "do not wrap" option now.
===== drivers/base/bus.c 1.52 vs edited =====
--- 1.52/drivers/base/bus.c Tue Sep 30 10:59:35 2003
+++ edited/drivers/base/bus.c Thu Jan 22 11:22:15 2004
@@ -8,7 +8,7 @@
*
*/
-#undef DEBUG
+#define DEBUG
#include <linux/device.h>
#include <linux/module.h>
@@ -18,7 +18,7 @@
#include "base.h"
#include "power/power.h"
-#define to_dev(node) container_of(node,struct device,bus_list)
+#define to_dev(node) container_of(node,struct device,kobj.entry)
#define to_drv(node) container_of(node,struct device_driver,kobj.entry)
#define to_bus_attr(_attr) container_of(_attr,struct bus_attribute,attr)
@@ -164,7 +164,7 @@
if (!(bus = get_bus(bus)))
return -EINVAL;
- head = start ? &start->bus_list : &bus->devices.list;
+ head = start ? &start->kobj.entry : &bus->devices.list;
down_read(&bus->subsys.rwsem);
list_for_each(entry,head) {
@@ -337,7 +337,7 @@
return;
list_for_each(entry,&bus->devices.list) {
- struct device * dev = container_of(entry,struct device,bus_list);
+ struct device * dev = to_dev(entry);
if (!dev->driver) {
error = bus_match(dev,drv);
if (error && (error != -ENODEV))
@@ -405,7 +405,7 @@
if (bus) {
down_write(&dev->bus->subsys.rwsem);
pr_debug("bus %s: add device %s\n",bus->name,dev->bus_id);
- list_add_tail(&dev->bus_list,&dev->bus->devices.list);
+ list_add_tail(&dev->kobj.entry,&dev->bus->devices.list);
device_attach(dev);
up_write(&dev->bus->subsys.rwsem);
sysfs_create_link(&bus->devices.kobj,&dev->kobj,dev->bus_id);
--
Hollis Blanchard
IBM Linux Technology Center
On Jan 22, 2004, at 1:38 PM, Hollis Blanchard wrote:
>
> I've found a bug in drivers/base/bus.c, where the
> bus_type.devices.list is treated as a list of device structs.
> bus_type.devices is a kset though, so devices.list should contain
> kobjects rather than devices. Here is the diff I've come up with:
>
[big snip]
> @@ -405,7 +405,7 @@
> if (bus) {
> down_write(&dev->bus->subsys.rwsem);
> pr_debug("bus %s: add device
> %s\n",bus->name,dev->bus_id);
> - list_add_tail(&dev->bus_list,&dev->bus->devices.list);
> +
> list_add_tail(&dev->kobj.entry,&dev->bus->devices.list);
> device_attach(dev);
> up_write(&dev->bus->subsys.rwsem);
Here's the problem: dev->kobj is already in use; it's part of the
global devices_subsys kset.
devices_subsys looks like it's only used for two things: global hotplug
policy and suspend. Of the 3 hotplug functions it provides
(dev_hotplug_filter, dev_hotplug_name, and dev_hotplug), 2 of them
refer to bus data or code anyways.
I'm very surprised to see it's used by device_shutdown(). I thought one
of the points of the device tree was to do depth-first-suspend, so e.g
we don't try to suspend a PCI bridge and *then* try to suspend children
of that bridge. Instead we're walking a global list in the reverse
order they were registered. I guess this works because busses are
discovered from the root down, so going backwards will give you the
deepest first.
I see three options, and I like the last best:
- add another kobject to struct device. This will allow a device to be
registered with the global devices_subsys as well as a bus.devices kset
simultaneously.
- change the kset "bus_type.devices" to a normal "list_head*" (which is
how it's being used today, incorrectly). This will preclude some of the
nice kobject/kset functionality however (e.g. see last paragraph
below).
- remove devices_subsys. The hotplug policy is already entirely
bus-specific anyways. The suspend code can be made to use bus
structures as well instead of a global device list (can it?).
The point of all of this is I want to be able to call
device_find("mydevice", &my_bus_type)
device_find() uses kset_find_obj() on the bus_type.devices kset, and
that doesn't work because bus_type.devices isn't a real kset, and it's
not a real kset because you can't register device kobjects in it, and
you can't because those kobjects have already been registered with
devices_subsys. I could call
device_find("mydevice", &devices_subsys.kset)
instead, but I already know what bus my device is on; no need to search
them all...
--
Hollis Blanchard
IBM Linux Technology Center
On Jan 27, 2004, at 5:31 PM, Hollis Blanchard wrote:
>
> devices_subsys looks like it's only used for two things: global
> hotplug policy and suspend. Of the 3 hotplug functions it provides
> (dev_hotplug_filter, dev_hotplug_name, and dev_hotplug), 2 of them
> refer to bus data or code anyways.
>
> I'm very surprised to see it's used by device_shutdown(). I thought
> one of the points of the device tree was to do depth-first-suspend, so
> e.g we don't try to suspend a PCI bridge and *then* try to suspend
> children of that bridge. Instead we're walking a global list in the
> reverse order they were registered. I guess this works because busses
> are discovered from the root down, so going backwards will give you
> the deepest first.
To reply to myself again (starting to get the hint...), I wonder how
long the global devices_subsys list will work for power-suspend once we
start hotplugging devices and busses? Seems to me that a cascading bus
power-down message is what has to happen...
--
Hollis Blanchard
IBM Linux Technology Center