[PATCH] Add device addition/removal notifier

[PATCH] Add device addition/removal notifier

[Benjamin Herrenschmidt]

This patch adds a notifier exposed by the device core to be used by
platform code to be notified of the addition and removal of devices
in the system.

It's intended as a replacement for the current platform_notify callbacks
which I'll remove as soon as we have converted the couple of users to
the new notifier.

In addition, that patch moves the remove callback & notification to after
bus_remove_device() where it belongs. The new notifier is also called
with a remove event in cases device addition fails after the platform
was notified of the addition to avoid leaks since I intend to use that
call to allocate auxilliary data structures.

The notifier approach is more interesting that just a pair of global
function pointers. For example, on powerpc, some of the new code I'm
working on involved separate subsystems requesting that to keep track
of various auxilliary informations attached to devices. PCI wants to
maintain some auxilliary data structure and this is the only good
place to actually dispose of it when a device is removed, and I have
at least one other case where the platform code wants to update the
dma operations for some types of platform devices. In addition, in
the future, I might use that to add firmware links to some other
generic bus types like USB etc... 

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

Note that we might even want to add more messages to it in the future.
For example, I already have a case where I could use a notification
when a driver binds to a device, when 2 inter-dependant device/drivers
couples need to find each other.

So I'd like that in 2.6.20 if possible and there is no objection.
It should be fairly trivial to update the 2 or so users of the old
callbacks to use that new notifier and remove the old style callbacks
too, I'm waiting for feedback from them though.

Index: linux-cell/drivers/base/core.c
===================================================================
--- linux-cell.orig/drivers/base/core.c	2006-10-19 17:44:02.000000000 +1000
+++ linux-cell/drivers/base/core.c	2006-10-19 17:45:07.000000000 +1000
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/kdev_t.h>
+#include <linux/notifier.h>
 
 #include <asm/semaphore.h>
 
@@ -25,6 +26,7 @@
 
 int (*platform_notify)(struct device * dev) = NULL;
 int (*platform_notify_remove)(struct device * dev) = NULL;
+static BLOCKING_NOTIFIER_HEAD(device_notifier);
 
 /*
  * sysfs bindings for devices.
@@ -427,6 +429,9 @@ int device_add(struct device *dev)
 	/* notify platform of device entry */
 	if (platform_notify)
 		platform_notify(dev);
+	/* notify clients of device entry (new way) */
+	blocking_notifier_call_chain(&device_notifier, DEVICE_NOTIFY_ADD_DEV,
+				     dev);
 
 	dev->uevent_attr.attr.name = "uevent";
 	dev->uevent_attr.attr.mode = S_IWUSR;
@@ -499,6 +504,8 @@ int device_add(struct device *dev)
  BusError:
 	device_pm_remove(dev);
  PMError:
+	blocking_notifier_call_chain(&device_notifier, DEVICE_NOTIFY_DEL_DEV,
+				     dev);
 	device_remove_groups(dev);
  GroupError:
  	device_remove_attrs(dev);
@@ -608,12 +615,14 @@ void device_del(struct device * dev)
 	device_remove_groups(dev);
 	device_remove_attrs(dev);
 
+	bus_remove_device(dev);
 	/* Notify the platform of the removal, in case they
 	 * need to do anything...
 	 */
 	if (platform_notify_remove)
 		platform_notify_remove(dev);
-	bus_remove_device(dev);
+	blocking_notifier_call_chain(&device_notifier, DEVICE_NOTIFY_DEL_DEV,
+				     dev);
 	device_pm_remove(dev);
 	kobject_uevent(&dev->kobj, KOBJ_REMOVE);
 	kobject_del(&dev->kobj);
@@ -836,3 +845,15 @@ int device_rename(struct device *dev, ch
 
 	return error;
 }
+
+int register_device_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&device_notifier, nb);
+}
+EXPORT_SYMBOL(register_device_notifier);
+
+int unregister_device_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&device_notifier, nb);
+}
+EXPORT_SYMBOL(unregister_device_notifier);
Index: linux-cell/include/linux/device.h
===================================================================
--- linux-cell.orig/include/linux/device.h	2006-10-19 17:43:58.000000000 +1000
+++ linux-cell/include/linux/device.h	2006-10-19 17:44:24.000000000 +1000
@@ -427,6 +427,22 @@ extern int (*platform_notify)(struct dev
 
 extern int (*platform_notify_remove)(struct device * dev);
 
+/**
+ * Device notifiers. Get notified of addition/removal of devices
+ * and possibly other events in the future. Replacement for the
+ * platform "fixup" functions. This is a low level hook provided
+ * for the platform to initialize private parts of struct device,
+ * like firmware related links. Add is called before the device is
+ * added to a bus (and thus the driver probed) and Remove is called
+ * afterward.
+ */
+struct notifier_block;
+
+extern int register_device_notifier(struct notifier_block *nb);
+extern int unregister_device_notifier(struct notifier_block *nb);
+
+#define DEVICE_NOTIFY_ADD_DEV	0x00000001	/* device added */
+#define DEVICE_NOTIFY_DEL_DEV	0x00000002	/* device removed */
 
 /**
  * get_device - atomically increment the reference count for the device.

Re: [PATCH] Add device addition/removal notifier

[Randy Dunlap]

On Thu, 19 Oct 2006 17:56:31 +1000 Benjamin Herrenschmidt wrote:

> Index: linux-cell/include/linux/device.h
> ===================================================================
> --- linux-cell.orig/include/linux/device.h	2006-10-19 17:43:58.000000000 +1000
> +++ linux-cell/include/linux/device.h	2006-10-19 17:44:24.000000000 +1000
> @@ -427,6 +427,22 @@ extern int (*platform_notify)(struct dev
>  
>  extern int (*platform_notify_remove)(struct device * dev);
>  
> +/**
> + * Device notifiers. Get notified of addition/removal of devices
> + * and possibly other events in the future. Replacement for the
> + * platform "fixup" functions. This is a low level hook provided
> + * for the platform to initialize private parts of struct device,
> + * like firmware related links. Add is called before the device is
> + * added to a bus (and thus the driver probed) and Remove is called
> + * afterward.
> + */

That's not kernel-doc, so please don't begin it with "/**".

---
~Randy

Re: [PATCH] Add device addition/removal notifier

[Benjamin Herrenschmidt]

> That's not kernel-doc, so please don't begin it with "/**".

Oops, sorry, new patch on the way

Ben.

[PATCH] Add device addition/removal notifier

[Benjamin Herrenschmidt]

This patch adds a notifier exposed by the device core to be used by
platform code to be notified of the addition and removal of devices
in the system.

It's intended as a replacement for the current platform_notify callbacks
which I'll remove as soon as we have converted the couple of users to
the new notifier.

In addition, that patch moves the remove callback & notification to after
bus_remove_device() where it belongs. The new notifier is also called
with a remove event in cases device addition fails after the platform
was notified of the addition to avoid leaks since I intend to use that
call to allocate auxilliary data structures.

The notifier approach is more interesting that just a pair of global
function pointers. For example, on powerpc, some of the new code I'm
working on involved separate subsystems requesting that to keep track
of various auxilliary informations attached to devices. PCI wants to
maintain some auxilliary data structure and this is the only good
place to actually dispose of it when a device is removed, and I have
at least one other case where the platform code wants to update the
dma operations for some types of platform devices. In addition, in
the future, I might use that to add firmware links to some other
generic bus types like USB etc... 

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>


Index: linux-cell/drivers/base/core.c
===================================================================
--- linux-cell.orig/drivers/base/core.c	2006-10-20 11:27:44.000000000 +1000
+++ linux-cell/drivers/base/core.c	2006-10-20 11:49:15.000000000 +1000
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/kdev_t.h>
+#include <linux/notifier.h>
 
 #include <asm/semaphore.h>
 
@@ -25,6 +26,7 @@
 
 int (*platform_notify)(struct device * dev) = NULL;
 int (*platform_notify_remove)(struct device * dev) = NULL;
+static BLOCKING_NOTIFIER_HEAD(device_notifier);
 
 /*
  * sysfs bindings for devices.
@@ -427,6 +429,9 @@ int device_add(struct device *dev)
 	/* notify platform of device entry */
 	if (platform_notify)
 		platform_notify(dev);
+	/* notify clients of device entry (new way) */
+	blocking_notifier_call_chain(&device_notifier, DEVICE_NOTIFY_ADD_DEV,
+				     dev);
 
 	dev->uevent_attr.attr.name = "uevent";
 	dev->uevent_attr.attr.mode = S_IWUSR;
@@ -499,6 +504,8 @@ int device_add(struct device *dev)
  BusError:
 	device_pm_remove(dev);
  PMError:
+	blocking_notifier_call_chain(&device_notifier, DEVICE_NOTIFY_DEL_DEV,
+				     dev);
 	device_remove_groups(dev);
  GroupError:
  	device_remove_attrs(dev);
@@ -608,12 +615,14 @@ void device_del(struct device * dev)
 	device_remove_groups(dev);
 	device_remove_attrs(dev);
 
+	bus_remove_device(dev);
 	/* Notify the platform of the removal, in case they
 	 * need to do anything...
 	 */
 	if (platform_notify_remove)
 		platform_notify_remove(dev);
-	bus_remove_device(dev);
+	blocking_notifier_call_chain(&device_notifier, DEVICE_NOTIFY_DEL_DEV,
+				     dev);
 	device_pm_remove(dev);
 	kobject_uevent(&dev->kobj, KOBJ_REMOVE);
 	kobject_del(&dev->kobj);
@@ -836,3 +845,15 @@ int device_rename(struct device *dev, ch
 
 	return error;
 }
+
+int register_device_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&device_notifier, nb);
+}
+EXPORT_SYMBOL(register_device_notifier);
+
+int unregister_device_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&device_notifier, nb);
+}
+EXPORT_SYMBOL(unregister_device_notifier);
Index: linux-cell/include/linux/device.h
===================================================================
--- linux-cell.orig/include/linux/device.h	2006-10-20 11:27:44.000000000 +1000
+++ linux-cell/include/linux/device.h	2006-10-20 11:49:33.000000000 +1000
@@ -427,6 +427,21 @@ extern int (*platform_notify)(struct dev
 
 extern int (*platform_notify_remove)(struct device * dev);
 
+/*  Device notifiers. Get notified of addition/removal of devices
+ * and possibly other events in the future. Replacement for the
+ * platform "fixup" functions. This is a low level hook provided
+ * for the platform to initialize private parts of struct device,
+ * like firmware related links. Add is called before the device is
+ * added to a bus (and thus the driver probed) and Remove is called
+ * afterward.
+ */
+struct notifier_block;
+
+extern int register_device_notifier(struct notifier_block *nb);
+extern int unregister_device_notifier(struct notifier_block *nb);
+
+#define DEVICE_NOTIFY_ADD_DEV	0x00000001	/* device added */
+#define DEVICE_NOTIFY_DEL_DEV	0x00000002	/* device removed */
 
 /**
  * get_device - atomically increment the reference count for the device.

Re: [PATCH] Add device addition/removal notifier

[Greg KH]

On Fri, Oct 20, 2006 at 11:55:50AM +1000, Benjamin Herrenschmidt wrote:
> @@ -608,12 +615,14 @@ void device_del(struct device * dev)
>  	device_remove_groups(dev);
>  	device_remove_attrs(dev);
>  
> +	bus_remove_device(dev);
>  	/* Notify the platform of the removal, in case they
>  	 * need to do anything...
>  	 */
>  	if (platform_notify_remove)
>  		platform_notify_remove(dev);
> -	bus_remove_device(dev);
> +	blocking_notifier_call_chain(&device_notifier, DEVICE_NOTIFY_DEL_DEV,
> +				     dev);

Why did you move the call to bus_remove_device() to be before the
platform_notify_remove() and notifier is called?

And I don't think this is really going to work well.  You have created a
notifier for all devices in the system, right?  How do you know what
type of struct device is being passed to your notifier callback?  At
least with the platform callback, you knew it was a platform device :)



>  	device_pm_remove(dev);
>  	kobject_uevent(&dev->kobj, KOBJ_REMOVE);
>  	kobject_del(&dev->kobj);
> @@ -836,3 +845,15 @@ int device_rename(struct device *dev, ch
>  
>  	return error;
>  }
> +
> +int register_device_notifier(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_register(&device_notifier, nb);
> +}
> +EXPORT_SYMBOL(register_device_notifier);
> +
> +int unregister_device_notifier(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_unregister(&device_notifier, nb);
> +}
> +EXPORT_SYMBOL(unregister_device_notifier);

All driver core exports should be _GPL() please.

thanks,

greg k-h

Re: [PATCH] Add device addition/removal notifier

[Benjamin Herrenschmidt]

On Thu, 2006-10-19 at 20:26 -0700, Greg KH wrote:
> On Fri, Oct 20, 2006 at 11:55:50AM +1000, Benjamin Herrenschmidt wrote:
> > @@ -608,12 +615,14 @@ void device_del(struct device * dev)
> >  	device_remove_groups(dev);
> >  	device_remove_attrs(dev);
> >  
> > +	bus_remove_device(dev);
> >  	/* Notify the platform of the removal, in case they
> >  	 * need to do anything...
> >  	 */
> >  	if (platform_notify_remove)
> >  		platform_notify_remove(dev);
> > -	bus_remove_device(dev);
> > +	blocking_notifier_call_chain(&device_notifier, DEVICE_NOTIFY_DEL_DEV,
> > +				     dev);
> 
> Why did you move the call to bus_remove_device() to be before the
> platform_notify_remove() and notifier is called?

I sent a mail about that a couple of days ago. The current behaviour is
bogus imho. (mail subjet was [PATCH/RFC] Call platform_notify_remove
later, and Len Brown who uses that hook in ACPI agreed).

Basically, we notify the platform of insertion before we bind devices to
busses & drivers (so the platform can do fixups, allocate auxilliary
data structures, whatever else...) and thus we should notify the
platform of removal -after- we unbind from the bus and driver where it
will then destroy those data structures). 

In my case, I need to hookup the DMA ops pointers. It would be very
bogus to destroy those before the driver remove() has been called.
 
> And I don't think this is really going to work well.  You have created a
> notifier for all devices in the system, right?  How do you know what
> type of struct device is being passed to your notifier callback?  At
> least with the platform callback, you knew it was a platform device :)

No I didn't. The platform_notify callback was called for any device as
is my notifier :)

All I did was to add a notifier chain in addition to the old function
pointer with the intend of deprecating the function pointer :)

You can know what type of device you are called for by comparing the
device->bus to the address of the bus type data structure. That's the
only way to do so but it works pretty fine. (There are actually some
severe issues with the device model and with PCI around that area that
I'm hitting trying to clean up some of the powerpc mess but let's handle
one problem at a time).

On PowerPC, for example, I'll use that notifier in at least 2 different
places: For PCI, I need to use the "delete" callback to destroy the
auxilliary data structure containing the DMA ops (it's currently created
by some PCI fixup code, but I might also migrate that to the "add"
callback. The other place is for platforms like Cell that are now
growing DMA capable non-PCI devices, that I'm catching in the cell
specific iommu code via that notifier, to hook them up to the right DMA
ops.

Ben.

> 
> 
> >  	device_pm_remove(dev);
> >  	kobject_uevent(&dev->kobj, KOBJ_REMOVE);
> >  	kobject_del(&dev->kobj);
> > @@ -836,3 +845,15 @@ int device_rename(struct device *dev, ch
> >  
> >  	return error;
> >  }
> > +
> > +int register_device_notifier(struct notifier_block *nb)
> > +{
> > +	return blocking_notifier_chain_register(&device_notifier, nb);
> > +}
> > +EXPORT_SYMBOL(register_device_notifier);
> > +
> > +int unregister_device_notifier(struct notifier_block *nb)
> > +{
> > +	return blocking_notifier_chain_unregister(&device_notifier, nb);
> > +}
> > +EXPORT_SYMBOL(unregister_device_notifier);
> 
> All driver core exports should be _GPL() please.
> 
> thanks,
> 
> greg k-h

Re: [PATCH] Add device addition/removal notifier

[Greg KH]

On Fri, Oct 20, 2006 at 02:29:24PM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2006-10-19 at 20:26 -0700, Greg KH wrote:
> > On Fri, Oct 20, 2006 at 11:55:50AM +1000, Benjamin Herrenschmidt wrote:
> > > @@ -608,12 +615,14 @@ void device_del(struct device * dev)
> > >  	device_remove_groups(dev);
> > >  	device_remove_attrs(dev);
> > >  
> > > +	bus_remove_device(dev);
> > >  	/* Notify the platform of the removal, in case they
> > >  	 * need to do anything...
> > >  	 */
> > >  	if (platform_notify_remove)
> > >  		platform_notify_remove(dev);
> > > -	bus_remove_device(dev);
> > > +	blocking_notifier_call_chain(&device_notifier, DEVICE_NOTIFY_DEL_DEV,
> > > +				     dev);
> > 
> > Why did you move the call to bus_remove_device() to be before the
> > platform_notify_remove() and notifier is called?
> 
> I sent a mail about that a couple of days ago. The current behaviour is
> bogus imho. (mail subjet was [PATCH/RFC] Call platform_notify_remove
> later, and Len Brown who uses that hook in ACPI agreed).
> 
> Basically, we notify the platform of insertion before we bind devices to
> busses & drivers (so the platform can do fixups, allocate auxilliary
> data structures, whatever else...) and thus we should notify the
> platform of removal -after- we unbind from the bus and driver where it
> will then destroy those data structures). 
> 
> In my case, I need to hookup the DMA ops pointers. It would be very
> bogus to destroy those before the driver remove() has been called.

Ok, as long as you all agree that this does change the behavior, it's
fine with me :)

> > And I don't think this is really going to work well.  You have created a
> > notifier for all devices in the system, right?  How do you know what
> > type of struct device is being passed to your notifier callback?  At
> > least with the platform callback, you knew it was a platform device :)
> 
> No I didn't. The platform_notify callback was called for any device as
> is my notifier :)
> 
> All I did was to add a notifier chain in addition to the old function
> pointer with the intend of deprecating the function pointer :)
> 
> You can know what type of device you are called for by comparing the
> device->bus to the address of the bus type data structure. That's the
> only way to do so but it works pretty fine. (There are actually some
> severe issues with the device model and with PCI around that area that
> I'm hitting trying to clean up some of the powerpc mess but let's handle
> one problem at a time).
> 
> On PowerPC, for example, I'll use that notifier in at least 2 different
> places: For PCI, I need to use the "delete" callback to destroy the
> auxilliary data structure containing the DMA ops (it's currently created
> by some PCI fixup code, but I might also migrate that to the "add"
> callback. The other place is for platforms like Cell that are now
> growing DMA capable non-PCI devices, that I'm catching in the cell
> specific iommu code via that notifier, to hook them up to the right DMA
> ops.

Ok, then perhaps you just want a bus specific callback for the devices
on that bus?  That would be much simpler and keep you from having to do
that mess with the different tests of bus type.

Actually, that's the only thing that really makes sense here, now that I
think about it, the platform_notify doesn't really make any sense...

thanks,

greg k-h

Re: [PATCH] Add device addition/removal notifier

[Benjamin Herrenschmidt]

> Ok, as long as you all agree that this does change the behavior, it's
> fine with me :)

I should probably split the patch in two: One that does that behaviour
change (I already have an Acked-by: Len Brown for that one even :) and
one adding that notifier.

> Ok, then perhaps you just want a bus specific callback for the devices
> on that bus?  That would be much simpler and keep you from having to do
> that mess with the different tests of bus type.
>
> Actually, that's the only thing that really makes sense here, now that I
> think about it, the platform_notify doesn't really make any sense...

Well... people already use it and go check the bus types :)

Having a notifier queue per bus type is a bit harder though because bus
types are generally allocated statically and thus we would need to find
them all in the kernel to add a proper static initialisation for the
notifier queue... bus_register() is not a good spot to do it because
platform code might want to register for bus types before those bus
types have been registered (it's not always easy to find a place to
"hook" between a bus is registered and things get added to it).

In fact, the whole bus type thing is a mess :) We can't easily register
for bus types that are in modules. 

For example, if I want to use the notifier to catch USB devices in order
to, for example, link them to firmware nodes, I'm lost if the USB
subsystem is modular ... unless I use a global notifier and strcmp the
bus type name in there.

So at this point, I'd rather stay on a global notifier.

Ben.

Re: [PATCH] Add device addition/removal notifier

[Greg KH]

On Fri, Oct 20, 2006 at 03:42:59PM +1000, Benjamin Herrenschmidt wrote:
> 
> > Ok, as long as you all agree that this does change the behavior, it's
> > fine with me :)
> 
> I should probably split the patch in two: One that does that behaviour
> change (I already have an Acked-by: Len Brown for that one even :) and
> one adding that notifier.

That would be a good idea.

> > Ok, then perhaps you just want a bus specific callback for the devices
> > on that bus?  That would be much simpler and keep you from having to do
> > that mess with the different tests of bus type.
> >
> > Actually, that's the only thing that really makes sense here, now that I
> > think about it, the platform_notify doesn't really make any sense...
> 
> Well... people already use it and go check the bus types :)

That's wrong to do.

> Having a notifier queue per bus type is a bit harder though because bus
> types are generally allocated statically and thus we would need to find
> them all in the kernel to add a proper static initialisation for the
> notifier queue... bus_register() is not a good spot to do it because
> platform code might want to register for bus types before those bus
> types have been registered (it's not always easy to find a place to
> "hook" between a bus is registered and things get added to it).

Why would it be any different from what we have today with the
usb_register_notify() function?  You could change that to be:
	bus_register_notify(struct bus_type *, struct notifier_block *);
and then just pass in the proper bus type that you care about.

And yes, you will have to export those bus_type pointers, but that's ok,
some of them already are there.

> In fact, the whole bus type thing is a mess :) We can't easily register
> for bus types that are in modules. 

Like everything except PCI?  :)

> For example, if I want to use the notifier to catch USB devices in order
> to, for example, link them to firmware nodes, I'm lost if the USB
> subsystem is modular ... unless I use a global notifier and strcmp the
> bus type name in there.

Ick, I'd like to say that this is a pretty bad thing to do.  If you need
that, then just statically link the bus into your code, or make your
code a module and it will depend on the usb core.  I don't know...

Remember, we didn't add a type identifier to the struct device for a
reason, comparing the string of the bus type is not a good idea (for
USB, it will get you in trouble, because two different types of devices
can be on that bus, who's to say other busses will not also have that
issue?)

I think you need to re-evaluate exactly what you are needing to do
here...

thanks,

greg k-h

Re: [PATCH] Add device addition/removal notifier

[Benjamin Herrenschmidt]

> > Well... people already use it and go check the bus types :)
> 
> That's wrong to do.

No, that's the only way to do anything.

> > Having a notifier queue per bus type is a bit harder though because bus
> > types are generally allocated statically and thus we would need to find
> > them all in the kernel to add a proper static initialisation for the
> > notifier queue... bus_register() is not a good spot to do it because
> > platform code might want to register for bus types before those bus
> > types have been registered (it's not always easy to find a place to
> > "hook" between a bus is registered and things get added to it).
> 
> Why would it be any different from what we have today with the
> usb_register_notify() function?  You could change that to be:
> 	bus_register_notify(struct bus_type *, struct notifier_block *);
> and then just pass in the proper bus type that you care about.
> 
> And yes, you will have to export those bus_type pointers, but that's ok,
> some of them already are there.

No, because some of them might be in modules.

> > In fact, the whole bus type thing is a mess :) We can't easily register
> > for bus types that are in modules. 
> 
> Like everything except PCI?  :)

Exactly.

> > For example, if I want to use the notifier to catch USB devices in order
> > to, for example, link them to firmware nodes, I'm lost if the USB
> > subsystem is modular ... unless I use a global notifier and strcmp the
> > bus type name in there.
> 
> Ick, I'd like to say that this is a pretty bad thing to do.  If you need
> that, then just statically link the bus into your code, or make your
> code a module and it will depend on the usb core.  I don't know...
> 
> Remember, we didn't add a type identifier to the struct device for a
> reason, comparing the string of the bus type is not a good idea (for
> USB, it will get you in trouble, because two different types of devices
> can be on that bus, who's to say other busses will not also have that
> issue?)
> 
> I think you need to re-evaluate exactly what you are needing to do
> here...

I want several things :)

 - For PCI, platform, of_platform, and maybe a couple others, I need the
DMA ops to go through a data structure attached to the device. For now,
I'm attaching it to firmware_data but I'll submit a way for archs to
actually add fields to the device instead for performances reason.

 - I want the ability to link devices in the system to their Open
Firmware node via the above same structure. I will have rules for
various bus types, to be able to do that. 

 - There are other users of that notifier, mostly embedded stuff using
platform or soc bus type for fixup-type tasks.

Now, it could be possible that the notifier that does the later for USB
would itself be a module that links on top of USB, in which case,
comparing the bus types remains possible.

Which means that in the end, your idea of doing 

bus_register_notify(struct bus_type *, struct notifier_block *);

Is workable, however, as I explained earlier, there is still a small
problem of how you initialize the notifier head in struct bus_type.
bus_register is not a good way because that would create a nasty
ordering requirement between code that attaches those notifiers and
whatever initcall registers the bus type. An option here might be to
have an "initialized" field that is statically set to 0 and have
bus_register_notify() test it and initialize the notifier head...

Ben.

Re: [PATCH] Add device addition/removal notifier

[Benjamin Herrenschmidt]

> Ick, I'd like to say that this is a pretty bad thing to do.  If you need
> that, then just statically link the bus into your code, or make your
> code a module and it will depend on the usb core.  I don't know...
> 
> Remember, we didn't add a type identifier to the struct device for a
> reason, comparing the string of the bus type is not a good idea (for
> USB, it will get you in trouble, because two different types of devices
> can be on that bus, who's to say other busses will not also have that
> issue?)
> 
> I think you need to re-evaluate exactly what you are needing to do
> here...

BTW. You basic saying that one should not test the bus type of a generic
struct device* makes it basically impossible to implement dma_map_* on
platforms that have various bus types with different DMA operations :)

Note that what I'm working on at the moment might make all of that
easier anyway, by having (on powerpc only for now but some of that could
be made generic once I'm finished) dma_ops having off the struct device
(or rather an extension of it).

Oh, also, right now, I re-use the firmware_data pointer there to point
to my struct device_ext, but I'd like to be better typed and avoid too
many pointer dereferences. I'm thus thinking instead of defining an
asm-*/device.h where archs can define their own struct device_ext and
have that flat in struct device (default being an empty struct). We
could even get rid of firmware_data on archs that don't use it by
putting it there :) (Among others).

Ben.

Re: [PATCH] Add device addition/removal notifier

[Paul Mackerras]

Greg KH writes:

> Remember, we didn't add a type identifier to the struct device for a
> reason, comparing the string of the bus type is not a good idea (for
> USB, it will get you in trouble, because two different types of devices
> can be on that bus, who's to say other busses will not also have that
> issue?)

But then we added the dma_map_* API, which only gets the struct device
pointer but needs to do something different depending on what type of
bus the device is on, and possibly even depending on which instance of
its bus type it's on...

If we are going to have APIs like this that work on a struct device,
then we need to have some good way to know what type of bus the device
is on, and what code we need to call to manipulate that bus type.

Paul.