[BKPATCH] bus notifiers for the generic device model

[BKPATCH] bus notifiers for the generic device model

[James Bottomley]

There are certain bus types (notably MCA and parisc internal ones) that like 
to do generic houskeeping operations (claim slots, claim uniform resources 
etc.) when drivers are attached to devices.

I've added a fairly generic notifier callback to the bus_type so that these 
busses can intercept and process the events they're interested in.

At the moment, the only events are attachment and detachment of drivers but in 
principle, more could be added.

James

You can import this changeset into BK by piping this whole message to:
'| bk receive [path to repository]' or apply the patch as usual.

===================================================================


ChangeSet@1.928, 2002-12-04 10:21:28-06:00, jejb@raven.il.steeleye.com
  make bus notify events more general in generic device model

ChangeSet@1.927, 2002-12-03 14:16:53-06:00, jejb@raven.il.steeleye.com
  add device model bus notifier
  
  This adds a bus notifier (per bus type) which is called after a device
  driver successfully attaches to a device.  The idea behind this is
  that there are certain operations that need to be performed globally
  after driver attachment (bus resource claiming etc.).  This notifier
  is set by the bus so that it can do these.


 drivers/base/bus.c     |    4 ++++
 include/linux/device.h |    9 +++++++++
 2 files changed, 13 insertions(+)


diff -Nru a/drivers/base/bus.c b/drivers/base/bus.c
--- a/drivers/base/bus.c	Wed Dec  4 11:05:18 2002
+++ b/drivers/base/bus.c	Wed Dec  4 11:05:18 2002
@@ -145,6 +145,8 @@
 		 dev->bus_id,dev->driver->name);
 	list_add_tail(&dev->driver_list,&dev->driver->devices);
 	sysfs_create_link(&dev->driver->kobj,&dev->kobj,dev->kobj.name);
+	if(dev->bus && dev->bus->notify)
+		dev->bus->notify(dev, BUS_NOTIFIER_DRIVER_ATTACHED);
 }
 
 
@@ -260,6 +262,8 @@
 		if (drv->remove)
 			drv->remove(dev);
 		dev->driver = NULL;
+		if(dev->bus && dev->bus->notify)
+			dev->bus->notify(dev, BUS_NOTIFIER_DRIVER_DETACHED);
 	}
 }
 
diff -Nru a/include/linux/device.h b/include/linux/device.h
--- a/include/linux/device.h	Wed Dec  4 11:05:18 2002
+++ b/include/linux/device.h	Wed Dec  4 11:05:18 2002
@@ -58,6 +58,13 @@
 	DEVICE_GONE		= 3,
 };
 
+/* bus notifier events: events that the bus type may choose to be notified
+ * about */
+enum bus_notifier {
+	BUS_NOTIFIER_DRIVER_ATTACHED,
+	BUS_NOTIFIER_DRIVER_DETACHED,
+};
+
 struct device;
 struct device_driver;
 struct device_class;
@@ -75,6 +82,8 @@
 	struct device * (*add)	(struct device * parent, char * bus_id);
 	int		(*hotplug) (struct device *dev, char **envp, 
 				    int num_envp, char *buffer, int buffer_size);
+	/* notify the bus that something happened */
+	void		(*notify)(struct device *dev, enum bus_notifier event);
 };
 
 

===================================================================


This BitKeeper patch contains the following changesets:
1.927..1.928
## Wrapped with gzip_uu ##


begin 664 bkpatch486
M'XL(`,\U[CT``]57:T_C.!3]7/^**XTT`I8\[#S;$8A'F9V*U0YB8#^MA)S$
M;3*3)E7L,E-M]K_O==*4!3I=0"#8-F_;U\?WG'MMOX-+*:I![ZOX&I%W\*F4
M:M"K^+4HS"PWI1(B%PMAQN442\_+$DNMM)P*2S>PCDXMN9!C:41S:12ERL:9
MJ*3!3,_T;((MSKB*4[C&CX,>-9W5%[68B4'O_.37R]\.SPG9VX/CE!<3\44H
MV-LCJJRN>9[(`Z[2O"Q,5?%"3H7B&DB]JEHSVV;X]VC@V)Y?4]]V@SJF":7<
MI2*QF1OZ[HTU#7RC+4I9P)CC>&%MA[;#R!"HV6<!V,RBS+(=H.Z`^@//,6Q_
M8-N@O7"PUEWP"P/#)D?PO&,Y)C'P)(%$7&>Q@&F9B!S0^]!Y'\OQN$@SJ>OA
MY58I;,WPHK]H!K;A>YHA&U@WYGDN$N!CA>5\:1X-)56&[(&<Q[&0<CS/\P5P
MI7B<"K11KJJ:@'T*R!*!'8HT*Q)0&D,FT8A*N<*+J`1P/&-1*9X54"(6KK*R
MD&V-0B`"M!D)P))Q64WQ?9*7$6);Z'$WX):(6A!342C8TN.IA"SG%;HDSGDV
MS8H)"!6;VPVN[)9[\$VBS**%AM3X0I8M@$RA'PI(]*N0PB2G0&U*/7)V(T]B
M//)'B,UMLH]4H<_R@XD0!3=GR0^SE$EN%LA\.R)I15P*"_&8<2L&C[F,XNG5
MOL<"KW;BI.\Y'@_[@2?&`=V@OI_9I`Q5;5.?LIKY`0L1UTP'Y'IQ9D6<SQ-A
MY5DQ_V$MB4Y72F48)Q25&O1#5M.^'86<(X61-^8>WP1N@]U_`?3=P*>$;+!S
M)WB733VG;EEK@S?L@M=%,@>,#ECXFL$[Y=_$34@N0&#W2J(Z,#`FHL"(R`%C
MHWG,XEMQOI1C^"QR?'.TN]3'_.O6U'5<_PW%2X?+\?J>VTQ5]^OJ.>OY\78F
M&T-E-?DI2,HHF@MI6#.*I#3"9_[=28NRMSMIG4*;CCZ#47UO#E3JV1I//T'O
M(^H&P$@O&V\A`&-?=_S^/73/QGX;B=NDU[O[33?8_K`Q!STTT3:<!'=S$74?
MP(D#!GVM7(3,M,)_"6:&U.T#U03IVWKW[\+1Y9>KWS]?C#Z.3LZOAN>C/_!V
M>'%Q>/SI9(CDC)CO:GH?PN\C>AB>K'K0,;\^=^FX?Z%$>B_X-V5/G0"0;.;7
MGD^]5FR^]_]*`.UT?T=FZP?]E"00-#G`VNFTWJW]FH4?[U:5*9=KU[H)[%BD
M=UUF2:^WM;,4U)94U3Q6W:AV_CM9/&;ATW#H/REA],%@KY@PVAG\Q9CT;0@(
M$GEK7],B&72(NAW':JN#T!<0IV4IQ7*+L6R:$-@!'I5SI2D6Q7RJVURM#/]%
M>IL2T.[ZXBY[[)*_/Y`_R3`(47VCH+]!@Q(WI[AEPHU+RF<S=.9#-;<+]U$W
=;M!B[#;<J.'XFYQ/]Y#*,(HC1OX!&:=4)OH/````
`
end

Re: [BKPATCH] bus notifiers for the generic device model

[Greg KH]

On Wed, Dec 04, 2002 at 11:09:21AM -0600, James Bottomley wrote:
> There are certain bus types (notably MCA and parisc internal ones) that like 
> to do generic houskeeping operations (claim slots, claim uniform resources 
> etc.) when drivers are attached to devices.

But doesn't the bus specific core know when drivers are attached, as it
was told to register or unregister a specific driver?  So I don't see
why this is really needed.

thanks,

greg k-h

Re: [BKPATCH] bus notifiers for the generic device model

[James Bottomley]

greg@kroah.com said:
> But doesn't the bus specific core know when drivers are attached, as
> it was told to register or unregister a specific driver?  So I don't
> see why this is really needed. 

The problem is that the bus specific core registration no-longer knows if the 
probes succeeded or failed (and if they did, what devices were attached), 
since probing is controlled by the base core.

What the bus needs to know is when a driver attaches to a specific device (and 
what device it has attached to).

Unless you have a better way of getting the attachment information out of the 
bus after the base probes have executed, a notifier seemed to be the simplest 
thing.

James

Re: [BKPATCH] bus notifiers for the generic device model

[Patrick Mochel]

On Wed, 4 Dec 2002, James Bottomley wrote:

> greg@kroah.com said:
> > But doesn't the bus specific core know when drivers are attached, as
> > it was told to register or unregister a specific driver?  So I don't
> > see why this is really needed. 
> 
> The problem is that the bus specific core registration no-longer knows if the 
> probes succeeded or failed (and if they did, what devices were attached), 
> since probing is controlled by the base core.
> 
> What the bus needs to know is when a driver attaches to a specific device (and 
> what device it has attached to).
> 
> Unless you have a better way of getting the attachment information out of the 
> bus after the base probes have executed, a notifier seemed to be the simplest 
> thing.

I believe that the stuff you're allowing the bus to do once the driver has 
been attached, is traditionally triggered by the driver in their probe() 
method once they've found a valid device. Which, in most cases, is a 
perfectly valid place to do it -- drivers have always been considered 
masters of their own destiny, and the things that this patch allows the 
bus to do seems like they should be triggered by the driver, with the bus 
supplying helpers.

The fact that the probe() method is overloaded to setup the device as well 
as verify that it should attach to it is a potential opportunity for 
improvement. There's been talk in the past about splitting it up into 
probe() and start() (or perhaps something better). The first would only 
verify that the device was a valid one to attach to, and the latter would 
actually do the dirty work of setting it up. It would be called in 
drivers/base/bus.c::attach(), like this:

===== drivers/base/bus.c 1.26 vs edited =====
--- 1.26/drivers/base/bus.c	Sun Dec  1 23:22:04 2002
+++ edited/drivers/base/bus.c	Wed Dec  4 12:02:41 2002
@@ -228,6 +228,10 @@
 {
 	pr_debug("bound device '%s' to driver '%s'\n",
 		 dev->bus_id,dev->driver->name);
+
+	if (dev->driver->start)
+		dev->driver->start(dev);
+
 	list_add_tail(&dev->driver_list,&dev->driver->devices);
 	sysfs_create_link(&dev->driver->kobj,&dev->kobj,dev->kobj.name);
 }

I don't recall why the change was never done. Perhaps because of other 
distractions, or it seemed like it would be too much of a PITA to convert 
drivers to a two-step init sequence (though I think it could be done in a 
compatible manner).


	-pat

Re: [BKPATCH] bus notifiers for the generic device model

[Jeff Garzik]

Patrick Mochel wrote:
> ===== drivers/base/bus.c 1.26 vs edited =====
> --- 1.26/drivers/base/bus.c	Sun Dec  1 23:22:04 2002
> +++ edited/drivers/base/bus.c	Wed Dec  4 12:02:41 2002
> @@ -228,6 +228,10 @@
>  {
>  	pr_debug("bound device '%s' to driver '%s'\n",
>  		 dev->bus_id,dev->driver->name);
> +
> +	if (dev->driver->start)
> +		dev->driver->start(dev);
> +
>  	list_add_tail(&dev->driver_list,&dev->driver->devices);
>  	sysfs_create_link(&dev->driver->kobj,&dev->kobj,dev->kobj.name);
>  }
> 
> I don't recall why the change was never done. Perhaps because of other 
> distractions, or it seemed like it would be too much of a PITA to convert 
> drivers to a two-step init sequence (though I think it could be done in a 
> compatible manner).


Possibly because of the "do it in open(2)" rule?

Ignoring the device model entirely, if a driver does a lot of 
talking-to-the-hardware in its probe phase, I consider it buggy, in 2.4 
or 2.5.

The network driver and chardev ones typically follow this rule quite 
well... probe is simple, just registering interfaces with the kernel. 
dev->open is where the driver should (and usually does) power-up the 
hardware, [re-]initialize it, etc.

So each time you come upon a driver that wants dev->driver->start(), 
look closely at the code and wonder why it can't perform the 
dev->driver->start() code in its interface's dev->open member.

	Jeff

Re: [BKPATCH] bus notifiers for the generic device model

[Patrick Mochel]

> > I don't recall why the change was never done. Perhaps because of other 
> > distractions, or it seemed like it would be too much of a PITA to convert 
> > drivers to a two-step init sequence (though I think it could be done in a 
> > compatible manner).
> 
> 
> Possibly because of the "do it in open(2)" rule?
> 
> Ignoring the device model entirely, if a driver does a lot of 
> talking-to-the-hardware in its probe phase, I consider it buggy, in 2.4 
> or 2.5.
> 
> The network driver and chardev ones typically follow this rule quite 
> well... probe is simple, just registering interfaces with the kernel. 
> dev->open is where the driver should (and usually does) power-up the 
> hardware, [re-]initialize it, etc.
> 
> So each time you come upon a driver that wants dev->driver->start(), 
> look closely at the code and wonder why it can't perform the 
> dev->driver->start() code in its interface's dev->open member.


Oh, right. Sorry, I should have said 'init()' or 'setup()' rather than 
'start()'. 

In many drivers there are two discrete operations that happen in the 
probe() method: verifying the device can be claimed, and registering the 
interfaces. By breaking it into two calls, you can conceptually separate 
the actions. It would also give the bus a chance to intercept the call and 
perform whatever housekeeping it needed to do at that point, giving James 
what he wanted.

However, I don't see a great benefit of doing this, besides making it 
_blatantly_ obvious what each call is supposed to do. The driver can call 
the bus to perform housekeeping duties during the setup (probe()) period, 
and everything should be happy.

	-pat

Re: [BKPATCH] bus notifiers for the generic device model

[Greg KH]

On Wed, Dec 04, 2002 at 12:04:41PM -0600, James Bottomley wrote:
> greg@kroah.com said:
> > But doesn't the bus specific core know when drivers are attached, as
> > it was told to register or unregister a specific driver?  So I don't
> > see why this is really needed. 
> 
> The problem is that the bus specific core registration no-longer knows if the 
> probes succeeded or failed (and if they did, what devices were attached), 
> since probing is controlled by the base core.

Not quite.  Well, I guess you can modify all of your drivers to do this,
but see below for an easier way.

> What the bus needs to know is when a driver attaches to a specific device (and 
> what device it has attached to).

Why not have a call in the driver that notifies the bus specific core of
this?  Or just check the status of the return value of your "probe"
function that the bus provides.  See usb_device_probe() and
pci_device_probe() for examples of this.

thanks,

greg k-h

Re: [BKPATCH] bus notifiers for the generic device model

[James Bottomley]

greg@kroah.com said:
> Why not have a call in the driver that notifies the bus specific core
> of this?  Or just check the status of the return value of your "probe"
> function that the bus provides.  See usb_device_probe() and
> pci_device_probe() for examples of this. 

Well, I did do it this way for parisc.  However, I assumed from reading the 
driver model docs that we were transitioning to using the generic driver probe 
rather than doing probe interceptions like this.

Doing it like this just seems rather clumsy.  wouldn't it be better to 
deprecate bus specific registrations in favour of the generic one?

There is another advantage to notifiers: they can be chained.  Certain bus 
architectures need to do additional setup for things like pci devices.  They 
would be able to do this by attaching a notifier.

James

Re: [BKPATCH] bus notifiers for the generic device model

[Greg KH]

On Wed, Dec 04, 2002 at 01:35:00PM -0600, James Bottomley wrote:
> greg@kroah.com said:
> > Why not have a call in the driver that notifies the bus specific core
> > of this?  Or just check the status of the return value of your "probe"
> > function that the bus provides.  See usb_device_probe() and
> > pci_device_probe() for examples of this. 
> 
> Well, I did do it this way for parisc.  However, I assumed from reading the 
> driver model docs that we were transitioning to using the generic driver probe 
> rather than doing probe interceptions like this.
> 
> Doing it like this just seems rather clumsy.  wouldn't it be better to 
> deprecate bus specific registrations in favour of the generic one?

I don't know.  Then for every bus specific driver, they would have to do
the "dereference the structure" logic before they can start to determine
if they should bind to this specific device.  That's all the PCI and USB
code really does, strip out the proper structures that are relevant to
the specific bus type, and then call the driver.

So in the end you would probably have a lot of duplicated code that
would be better off being in one place, like it is today :)

> There is another advantage to notifiers: they can be chained.  Certain bus 
> architectures need to do additional setup for things like pci devices.  They 
> would be able to do this by attaching a notifier.

That seems a bit overkill.  The bus specific code can add those
architecture setup hooks right now if it's needed, right?

thanks,

greg k-h

Re: [BKPATCH] bus notifiers for the generic device model

[Patrick Mochel]

On Wed, 4 Dec 2002, Greg KH wrote:

> On Wed, Dec 04, 2002 at 01:35:00PM -0600, James Bottomley wrote:
> > greg@kroah.com said:
> > > Why not have a call in the driver that notifies the bus specific core
> > > of this?  Or just check the status of the return value of your "probe"
> > > function that the bus provides.  See usb_device_probe() and
> > > pci_device_probe() for examples of this. 
> > 
> > Well, I did do it this way for parisc.  However, I assumed from reading the 
> > driver model docs that we were transitioning to using the generic driver probe 
> > rather than doing probe interceptions like this.
> > 
> > Doing it like this just seems rather clumsy.  wouldn't it be better to 
> > deprecate bus specific registrations in favour of the generic one?
> 
> I don't know.  Then for every bus specific driver, they would have to do
> the "dereference the structure" logic before they can start to determine
> if they should bind to this specific device.  That's all the PCI and USB
> code really does, strip out the proper structures that are relevant to
> the specific bus type, and then call the driver.
> 
> So in the end you would probably have a lot of duplicated code that
> would be better off being in one place, like it is today :)

I tend to agree with Greg, though this exposes one of the great tradeoffs 
of the driver model. We can consolidate data structures and operations on 
them into common ones, but sooner or later, we have to call down to the 
bus (or class)-specific objects, necessitating a conversion to the 
relevant object. 

The current infrastructure was designed the way it is to provide seamless 
transition while features of the generic model evolved. This is precisely 
why the bus drivers provide intermediate calls for the generic struct 
device_driver. The thought was that one day we could hopefully convert 
everything to the generic way and everything would be fantastic. 

Along they way, we've found several things stand in the way of making this 
a smooth transition, so the plan is to stick with the bus intermediaries 
until the infrastructure matures enough to tackle those problems more 
easily. 

> > There is another advantage to notifiers: they can be chained.  Certain bus 
> > architectures need to do additional setup for things like pci devices.  They 
> > would be able to do this by attaching a notifier.

[ Replying to James... ]

I don't see how your patch adresses chaining; there is only one notifier 
per bus type.

On the other hand, I've considered implementing something like before 
(Most recently in the last couple of days). Essentially, one could have 
components register with a subsystem (e.g. bus type), that are called when 
a device is added or removed. 

This would make conditional features and extensions easy to call. For
example, drivers/pci/proc.c could register with the PCI bus to be called 
each time a device was added. This would eliminate uglies like:

int pci_proc_attach_device(struct pci_dev *dev);

#ifdef CONFIG_PROC_FS
        pci_proc_attach_device(dev);
#endif

Ditto for usbfs. 

It would also provide a means to eliminate the power management 
information in struct device, as PM could be an extension of the top-level 
device subsystem, only called if CONFIG_PM is set. 

However, there are a few details that are kinda nasty (like associating 
data between the extension and the device), and I've shelved the idea 
again..

	-pat

Re: [BKPATCH] bus notifiers for the generic device model

[James Bottomley]

mochel@osdl.org said:
> I don't see how your patch adresses chaining; there is only one
> notifier  per bus type. 

Well, you can do it by storing the pointer privately and hijacking it.  
However, I think we'd use the existing kernel notifier chain stuff for that if 
it is valuable.

> Along they way, we've found several things stand in the way of making
> this  a smooth transition, so the plan is to stick with the bus
> intermediaries  until the infrastructure matures enough to tackle
> those problems more  easily.  

I'm happy with keeping probe and remove in the bus specific driver template 
and having the <bustype>_add_driver install generic device probe and remove 
routines to handle these cases.  My point was that the docs implied I should 
use the generic driver probe and remove routines, which I can't without some 
type of functionality like this.

If you envisage us never eliminating the driver specific probe and remove 
routines, I'm happy.  I'm less happy if there will come a day when I have to 
revisit all the converted drivers to do the elimination.

James

Re: [BKPATCH] bus notifiers for the generic device model

[Jeff Garzik]

James Bottomley wrote:
> If you envisage us never eliminating the driver specific probe and remove 
> routines, I'm happy.  I'm less happy if there will come a day when I have to 
> revisit all the converted drivers to do the elimination.


Likewise...  I think with very minor changes, existing PCI-API-compliant 
drivers should stay pretty much the same as they are now.

Re: [BKPATCH] bus notifiers for the generic device model

[Patrick Mochel]

Just to follow up..

> I'm happy with keeping probe and remove in the bus specific driver template 
> and having the <bustype>_add_driver install generic device probe and remove 
> routines to handle these cases.  My point was that the docs implied I should 
> use the generic driver probe and remove routines, which I can't without some 
> type of functionality like this.
> 
> If you envisage us never eliminating the driver specific probe and remove 
> routines, I'm happy.  I'm less happy if there will come a day when I have to 
> revisit all the converted drivers to do the elimination.

I don't see us eliminating the bus-specific probe() and remove() methods. 
At least not for a very long time.

	-pat

Re: [BKPATCH] bus notifiers for the generic device model

[Mike Anderson]

Greg KH [greg@kroah.com] wrote:
> On Wed, Dec 04, 2002 at 11:09:21AM -0600, James Bottomley wrote:
> > There are certain bus types (notably MCA and parisc internal ones) that like 
> > to do generic houskeeping operations (claim slots, claim uniform resources 
> > etc.) when drivers are attached to devices.
> 
> But doesn't the bus specific core know when drivers are attached, as it
> was told to register or unregister a specific driver?  So I don't see
> why this is really needed.

The change is when a device is bound to a driver (i.e. when attach /
detach is called bus.c ).

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [BKPATCH] bus notifiers for the generic device model

[Greg KH]

On Wed, Dec 04, 2002 at 10:10:16AM -0800, Mike Anderson wrote:
> Greg KH [greg@kroah.com] wrote:
> > On Wed, Dec 04, 2002 at 11:09:21AM -0600, James Bottomley wrote:
> > > There are certain bus types (notably MCA and parisc internal ones) that like 
> > > to do generic houskeeping operations (claim slots, claim uniform resources 
> > > etc.) when drivers are attached to devices.
> > 
> > But doesn't the bus specific core know when drivers are attached, as it
> > was told to register or unregister a specific driver?  So I don't see
> > why this is really needed.
> 
> The change is when a device is bound to a driver (i.e. when attach /
> detach is called bus.c ).

Whis is called after probe / remove is called for the driver, which can
point to the bus specific functions, like PCI and USB cores do.

thanks,

greg k-h