Subject: Re: Device driver implementation question
To: Ben Tober <tober@bbnplanet.com>
From: Chris G. Demetriou <cgd@CS.cmu.edu>
List: tech-kern
Date: 01/14/1997 15:38:58
>	I have noted that in -current, NetBSD is trying to move toward a more
>uniform and rational style for probing and attaching device drivers, and I
>think this is all well and good.

We are not trying to "move toward" it, we are trying to move _back_
toward it.  The match/attach function interfaces in Lite and Lite2
were the same as those in NetBSD when __BROKEN_INDIRECT_CONFIG is not
defined.  We pulled in the Lite interfaces, and then they were
_changed_ to have the 'match sometimes takes a softc' behaviour.
Since that's inconsistent and can often cause trouble (and certainly
lends itself to logical errors).


>I note, however, that, in the new regime
>(without __BROKEN_INDIRECT_CONFIG, which I take to be an interim hack), the
>match routine for the device has no access to a softc structure.

Right.  Eventually, unifdef -U__BROKEN_INDIRECT_CONFIG will be
applied.


>While I
>can understand this and feel that it is largely a good thing, it makes
>the probe process more difficult on busses on which probing is not necessarily
>straightforward (such as ISA).  A lot of ISA drivers will construct, at least
>partially, a softc structure and then proceed to twiddle the hardware some
>and see if it responds as expected.  Under the new regime, how is it
>anticipated that ISA match functions will operate?  Perhaps this hasn't been
>fully explored yet, as I do note that many of the ISA drivers still do things
>the old way and that i386/include/types.h has #define __BROKEN_INDIRECT_CONFIG.

So, there are several possible solutions here:
	(1) make the routines used by probe take a few more arguments,
	    to indicate what they should be touching,

	(2) allocate a softc (or some smaller structure containing
	    'important' data) on the stack in the match function and
	    pass it around, or

	(3) rewrite various match/attach/init functions to take
	    their current arguments (or maybe one or two more), along
	    with an argument that's a softc pointer to be filled in if
	    non-NULL.

Really, the only things that matter are:
	(1) there must be no permanent allocations 'passed' from
	    the match function to the attach function via a softc or
	    hidden variable.

	(2) The 'attach' function may not rely on any device
	    initialization to have been performed by the 'match'
	    function.

For indirect-config device (e.g. ISA devices), match routines may
modify the data pointed to by their 'aux' arguments to reflect ports,
etc., to be used by the device.  These are then printed by the bus
attachment code, and can be used by the attach routines.  However, as
noted, NO permanent allocations should be made at attach time.

Direct-config devices must NOT modify the data pointed to by their
'aux' arguments.


>Indeed, the only architecture with any ISA support that doesn't define
>__BROKEN_INDIRECT_CONFIG is the alpha, and its ISA support is currently very
>minimal.

There are two reasons for this:

i386: because the i386 uses lots of devices which still need
__BROKEN_INDIRECT_CONFIG.

other ports: because the other ports haven't been converted to work
without __BROKEN_INDIRECT_CONFIG yet.


For instance, in the latter set: the amiga can use ISA devices (i
dunno if the support is in the tree, though), and it supports
pretty-much the same set of ISA devices that the Alpha does.


> Would it be proper for the probe routine to allocate some static
>state information and then for the attach routine to use that information
>if necessary?

NO.  "see above."


> What about drivers that support multiple instances?  Is it
>even currently possible for an ISA driver (either with the old or new style
>match function) to support the use of the * syntax in the config file or
>is explicit mention of each device always needed for all ISA devices?

It's not currently possible*, but it does make sense to allow and
support it.

Right now, explicit mention of devices in the config file is necessary
for ISA devices.


> In the
>event that such a driver does support multiple instances, will the match
>routine be called repeatedly before the attach routine is called for the
>first time?

No.  For ISA (and other indirect-config busses) the match routine will
be called exactly once before each potential attach.  that's the way
that indirect config is supposed to work.

However:

> If it is, then it would seem improper for the match routine
>to use any static state which would be reused by the attach routine, and, yet,
>in the new style, I see no way to have the match routine dynamically allocate
>per-instance state.  I'm probably just missing something here.

In _any_ case, allocating static state is improper.

Even if the 'match' routine says "yes, i'm here!" the bus has no
obligation to actually _attach_ the device.


For instance, the current isa code does something like:

	for (each device) {
		rv = device's match result
		if (!rv)
			continue;
		attach device
	}

There's no reason that that couldn't be changed to do something like:

	for (each device) {
		rv = device's match result
		if (!rv)
			continue
		if (some other (perhaps failure?) condition)
			continue
		attach device
	}

Additionally, at some point it'd probably make sense to amend the
various interfaces to allow attachment failure (e.g. because kernel
has run out of memory), etc.


In other words (from an autoconfiguration standpoint):

No matter your bus type, even if 'match' succeeds there's no
guarantee that your 'attach' routine will be called.

No matter your bus type, it's wrong to take actions (such as
allocation of anything) in 'match' that cause persistent state changes
which persist even after the 'match' returns (i.e. are not undone
before match returns).

No matter your bus type, it's wrong to assume in 'attach' that any
device setup done in 'match' persists.  That is, if 'match' inits a
device's registers to be in a certain state, you cannot safely assume
that they are in the same state by the time 'attach' happens.

If you are the child of an indirect-configuration-style bus, your
match routine should modify the data pointed to by its 'aux' argument
to contain appropriate information.  If not, your match routine must
not modify the data pointed to by its 'aux' argument.


There are drivers in the tree (__BROKEN_INDIRECT_CONFIG or not) which
currently violate these rules.  (The specific examples that i'm
thinking of are the alpha keyboard and mouse drivers, which currently
support only one unit and _do_ use static global storage for some
state.  The corresponding i386 drivers are the same way, if i recall.)
However, those examples should not be taken as "correct," but rather
should be seen as things to be avoided.



chris