eeh@netbsd.org writes:
> 	It's not clear that you can actually get away with this...  That
> 	means, for instance, that the properties values as defined in NetBSD
> 	must match exactly those defined in OFW... Which may not be desirable
> 	at all in many cases.
> 
> Not necessarily.  They just shouldn't conflict.

No, they _may not_ conflict.  If they do conflict, the whole system
loses its value.


> 	So, could you re-post a copy of your latest proposal, with all the
> 	feedback that you're planning to integrate pulled in?
> 
> O.K:

So, as I mentioned in private mail previously, I'd been very busy
which is why it'd taken me so long to respond.  I'm still very busy.
But this needs responding to before it gets further as it currently
is.

"*sigh*" You seem to have gone off into left field again in at least
one place, and you keep ignoring the changes I ask you to make.  I
noticed this when i started to read the diffs, and then went back to
look at this...  I don't think I'm alone in thinking these changes are
the Right Thing.

Also, thinking about some of the issues and looking at the source code
of current users of the config code has made me think of a few
additional possible issues.


> The device structure will be rearranged to look like this:
> 
> struct device {
> 	enum	devclass dv_class;	/* this device's classification */
> 	TAILQ_ENTRY(device) dv_list;	/* entry on list of all devices */
> 	struct	cfdata *dv_cfdata;	/* config data that found us */
> 	int	dv_unit;		/* device unit number */
> 	int	dv_flags;		/* misc. flags; see below */
> 	char	dv_xname[16];		/* external name (name + unit) */
> 	struct	device *dv_parent;	/* pointer to parent device */
> 	SLIST_HEAD(devpropslist, devprop) dv_props;
> 	void	*dv_private;
> };

Up to here looks fine (assuming that dv_flags aren't the flags you
mention below, which I don't think they are 8-).


> [ ... property flags ... ]

still not keen on grouping the "operation" flags with the "property
meaning" flags.

you don't see e.g. mmap() mixing 'prot' and 'flags' even though it
could.  This isn't different than that.


> DEV_CFA_DECL(name, size, match, attach)	-- Declare cfattach structure

You should derive match and attach function names from 'name'.
Drivers also need to support detach, and perhaps more entry points at
some time in the future.


> DEV_PROTECT(dev)			-- Make properties on the device immutable.
> DEV_UNPROTECT(dev)			-- Remove immutability from a device.

maybe DEV_PROP_*()


> 
> 2.3	Transitional Functions
> 
>    struct device *config_found_sad(struct device *dev, void *aux, 
> 			cfprint_t print, cfmatch_t submatch, 
> 			struct device *child);
>    struct cfdata *config_search_ad(cfmatch_t fn, struct device *parent, 
> 			void *aux, struct device *child);
>    struct device *config_attach_ad(struct device *parent, struct cfdata *cf, 
> 			void *aux, cfprint_t print, struct device *child);

look good, I think.

> 
> 2.4	New Functions
> 
>    struct device *dev_config_create(struct device *parent, int flags);

more discussion of this later.

dev_create, no parent, opflags.

also, it occurs to me that we need a dev_destroy...


>    struct device *dev_config_found(struct device *parent, struct device *child, 
> 			cfmatch_t submatch, cfprint_t print);
>    struct cfdata *dev_config_search(struct device *parent, struct device *child,
> 			cfmatch_t fn);

Here's a question: is config_search() as is ever called with a
non-NULL aux ptr?  Given the way it's used in the places i'm aware of
(e.g. ISA), that's not really necessary/appropriate.

The vast majority of uses in this case are with NULL.  There are a few
counterexamples:

* config_found().

* weird MD early-probe code.  This is done by amiga and atari, and to
be honest I don't understand it and don't have time right now to
understand.  Perhaps somebody with clue could tell us what exactly
that is about.

* cobalt and sgimips mainbus config.  I'd guess off-hand that this
could/should probably be done via direct config.

* a few really weird uses in hpcmips where the parent _print_ routine
is passed down in the aux.

* a few possibly correct uses (hpib, pnpbios -- but i'd have thought
this would be direct config!!!, some ports obio, vme & similar).

I think this needs a bit more study.

In particular:

(1) the MD early-probe code needs to be understood, to see if that's
ameable to conversion.

(2) the weird cases need to be shot in the head.

(3) the semantics for the seemingly-normal case of "things that don't
want to pass any aux info/properties" (which typically do all the work
in the cfmatch_t they provide to config_search()) needs to be figured
out.


Also, this just occurred to me: how do cfmatch_t, cfprint_t, et
al. change? 8-)



>    struct device *dev_config_attach(struct device *parent, struct device *child, 
> 			struct cfdata *cf, cfprint_t print);

These look OK.


>    int dev_setprop(struct device *dev, const char *name, void *val, 
> 			size_t len, int flags);

This should take 'propflags' and 'opflags'.

>    int dev_copyprops(struct device *source, struct device *dest, 
> 			int flags);

This is _only_ 'opflags'.

>    size_t dev_getprop(struct device *dev, const char *name, void *val, 
> 			size_t len, int flags);
>    int dev_delprop(struct device *dev, const char *name, int flags);
>
>    size_t dev_mdgetprop(struct device *dev, const char *name, void *val, 
> 			size_t len, int flags);

I believe these are all only 'opflags' as well, right?


So, perhaps you'll hate me for this, but we have:

DEV_PROP_PROTECT (assuming you change as suggested above),
dev_config_*, etc....

I would think that:

	dev_prop_set
	dev_prop_copyall
	dev_prop_get
	dev_prop_delete
	dev_prop_get_md

would be better names.  Sorry, I know, you've changed (some of?) these
once before at my request...  but with the rest of the dev_* functions
it seems obvious and neither of us should have overlooked it.  trivial
to change, too.

It also occurs to me that config_detach needs at least a name
remapping.


does it make sense to have dev_prop_cmp to compare two named
properties?  also, possibly dev_prop_cmpdata to compare a property
agains known data?

The thought being, if you are going to go into attaching locators
directoly, you need to be able to do something like:

	get property value passed by parent
	get property value from locator
	if (locator is not a wildcard &&
	    parent value != locator value)
		reject

That's best accomplished via something like:

	int x = wildcard;

	if (dev_prop_cmpdata("locatorval", PROP_SEARCH or 0, x, sizeof x) &&
	    dev_prop_cmp("locatorval", PROP_SEARCH or 0,
			 "parentval", PROP_SEARCH or 0))
		reject;


> 3	Description
> 
> A property is a tuple that consists of a pointer to a device node (struct
> device *), string, and an arbitrary amount of data.  This tuple is established
> by dev_setprop(), retrieved by dev_getprop(), and destroyed by dev_delprop().
> In addition, there is a machine dependent hook, dev_mdgetprop() that is called if
> attempting to retrieve a property fails.  dev_mdgetprop() can then use any
> arbitrary method to generate property data if it so desires, or cause a
> failure to be reported.
> 
> This functionality will be implemented in two phases.  The first phase will
> add these functions, but retain the existing behavior for compatilbility.
> dev_config_create(9) will generate a dummy device node that the parent can use
> to attach properties to.  config_attach*(9) functions will consume the dummy
> device node then create the real device node and migrate properties to it.

not just config_attach*, right?  I'd suspect config_found(),

> The second phase will separate the `struct device' from the device's softc.
> dev_config_create(9) will create the true device node, which config_found*(9)
> will attach to the device tree.  If the device probes successfully,
> config_attach(9) will allocate a separate device softc structure and link it
> to `struct device'.  This change will cause breakage in practically all
> existing device drivers, so it will be relegated to some future date.

s/it.*/the interfaces described here will allow smooth transition/  8-)



> 3.0	Changed Data Structures
> 
> Several fields will be added to `struct device' and some will be relocated to
> compact the size of the structure on 64-bit platforms.
> 
> A singly linked list of properties will hang from the `dv_props' field.
> 
> A pointer to the device private data will be contained in `dv_private'.

cool.


> 3.1	Changed Functionality
> 
> A new device type `DV_NONE' will be added to signify that a device node is as
> yet un-probed.
> 
> Drivers that expect properties during probe should:
> 
> 	o Define a softc that does not contain a `struct device *'
> 
> 	o Use CFA_DECL() to declare the softc.

s/softc/cfattach structure/

> By doing that, the `aux' parameter passed in to the device's probe function
> will point to a `struct device *', with all all locators attached as
> properties, in addition to any properties the parent driver has attached. 

You also need to use macros to define & declare the match, attach,
detach, etc., functions.

The point here is to define them something like:

int foomatch(__parent_dontuse, cfdata, __aux_dontuse, childdevnode)

in such a way that the 'dontuse' params can magically be made to
disappear later.


> DEV_CFA_DECL(name, size, match, attach)
> 
> Declare a `struct cfattach' for a new-style device driver.  The `size' is the
> size of the softc, and the `match' and `attach' parameters should be pointers
> to the match and attach functions for that device.  The `name' parameter should
> match the name referenced by `ioconf.c'.

additional comments above on this.


> struct device *dev_config_create(struct device *parent, int flags);
> 
> dev_config_create() will trigger the creation of an un-configured device node
> and attach it to the tree under `parent'.  This allows the parent driver to
> attach properties to the device node before calling config_found*() so they
> are available to the child device during the probe and attach steps.  If
> PROP_WAIT is not specified then dev_config_create(9) will not sleep to resolve
> resource shortages.  Returns a pointer to a `struct device' on success, and
> NULL on error.

s/_config// i think.

this _should not_ take a parent, and should not attach into the device
tree.

These are opflags, and should probably be named along the form
DEV_WAIT or DEV_OP_WAIT.

They shold not be PROP_ flags!!!


> struct device *config_found_sad(struct device *parent, void *aux, cfprint_t print, 
> 			cfmatch_t submatch, struct device *child);
> 
> In addition to config_found(9), and config_found_sm(9), config_found_sad(9)
> can be used to supply device properties to child devices during probe and
> attach.  The `child' parameter is passed down to config_search(9) and
> config_attach(9).

s/sad/sd/, s/sad/smd/ or similar, I think.  This follows from the
previous convention of adding; you're replacing letters...  'aux' has
always been there.

config_found() and config_found_sm() are wrappers around this
function, right (which pass null submatch and child, and null child,
respectively).


> struct cfdata *config_search_ad(cfmatch_t fn, struct device *parent, 
> 			void *aux, struct device *child);
> 
> config_search_ad(9) takes an additional parameter `child' which is a pointer
> to a device which provides a set of properties for devices to use during
> probe.

config_search_d.

> If the device being probed used DEV_CFA_DECL() to declare it's `cfattach'
> structure, a `child' device is created if it does not exist.  Locator data are
> attached to the `child' as properties.

It's not clear to me that this is actually desirable.  I think you
want the bus-provided function that does the matchign to do it, just
like right now that's the fn that fills in the aux.


> It is then passed to the probe routine
> as the `aux' parameter.  The child driver should cast the `aux' parameter to a
> `struct device *' and use it to query for properties.

_no_.  the parameters should be as strongly typed as possible, and
'aux' should be nuked entirely.

That means no aux, no casting.  The declare the match/attach/detach
functions with their macros, and then use the 'child' device that's
passed in.

That's really the right way to do it.


> If the device did not use DEV_CFA_DECL(), the probe routine is called with the
> `aux' passed in by the parent in the `aux' field and the `child' device with
> locator properties is not available to the *_probe() routine.

no.  The calling convention is the same either way.  Just old-style
drivers don't notice that they've got a hidden extra argument.


> struct device *config_attach_ad(struct device *parent, struct cfdata *cf, 
> 			void *aux, cfprint_t print, struct device *child)

config_attach_d or similar.  oh, hell, i'm not going to argue over
this trivial a name issue, these functions are going to die eventually
anyway.


> If a non-NULL `child' is passed in to config_attach_ad(9), a softc of the size
> specified by DEV_CFA_DECL() will be allocated and a pointer to it placed in
> the device private field of the `child' device.  This structure can be
> accessed through `DEV_PRIVATE(dev)'.
> 
> If a non-NULL `child' is passed in to config_attach_ad(9), a new device node
> is allocated of the appropriate size, all properties attached to it are moved
> to the newly created device node for the device being attached.
> 
> If a NULL value is passed in as the `child', a device structure is allocated
> of the appropriate size and locator data are associated with it.  If the
> device used DEV_CFA_DECL(), then the softc will be accessible through
> `DEV_PRIVATE(dev)'.

No.  If NULL is passed, this is an invocation via a wrapper of
config_attach or similar.

the parent fills in the properties for the locators, if appropriate.

In cases of direct config, the parent should also be doing
'submatch'ing to ensure sanity.

I think that this could probably be done different ways ... but it
merits some more discussion up front since it begins to form the basis
of the 'protocols' for property passing.

_Exactly_ what do you have in mind here, if it's going to be done
automatically?


> struct device *dev_config_found(struct device *dev, struct device *child,
> 		 	cfprint_t print, cfmatch_t submatch);
> 
> dev_config_found(9) is intended as an interface for bus drivers that no longer
> need to supply an `aux'.  The `child' parameter will initally be a dummy
> container but later will be the real child device node.

not a dummy, it contains useful properties.


> struct cfdata *dev_config_search(cfmatch_t fn, struct device *parent, void *aux, 
> 			struct device *child);
> 
> dev_config_search(9) is intended as an interface for bus drivers that no
> longer need to supply an `aux'.  The `child' parameter will initally be a
> dummy container but later will be the real child device node.

same here.


> 
> struct device *dev_config_attach(struct device *parent, struct device *child, 
> 			struct cfdata *cf, cfprint_t print);
> 
> dev_config_attach(9) is intended as an interface for bus drivers that no
> longer need to supply an `aux'.  The `child' parameter will initally be a
> dummy container but later will be the real child device node.

same here.


> 
> int dev_setprop(struct device *dev, const char *name, void *val, size_t len, 
> 			int flags);
> 
> Create a property `name' and attach it to `dev' with a `len' byte value copied
> from location `val'.  If PROP_WAIT is not specified then dev_setprop(9) will
> not sleep for resource shortage.  If PROP_CONST is specified, no storage is
> allocated for the value, and when the property is queried it will copy `len'
> bytes from the location specified by `val', so that data cannot be freed or
> the kernel may panic.  If PROP_STRING is specified then the property is marked
> as being a NUL terminated ASCII string.

does size include NUL in this case?

> Returns 0 on success or an error
> value.  This will fail without modifying the device node if it is protected.

This is fine, modulo the name and the issues about flags.

> 
> int dev_copyprops(struct device *source, struct device *dest, int flags);
> 
> Copy all properties associated with `source' device to `dest' device
> structure.  It does not traverse the device tree or make any use of
> dev_mdgetprop().  If PROP_WAIT is not specified then dev_copyprops(9) will not
> sleep for resource shortage.  Returns 0 on success or an error value.  The
> state of properties on the destination device is undefined if the operation
> fails.  This will fail without modifying the destination device node if it is
> protected.

this seems fine.


> size_t dev_getprop(struct device *dev, const char *name, void *val, 
> 			size_t len, int flags);
> 
> Retrieve a property called `name' associated with `dev'.  If the property is
> not found dev_getprop(9) will call dev_mdgetprop(9).  If the flag PROP_INHERIT is
> set, and there is no property with that name associated with this device node,
> it will try to retrieve the property from any parent device nodes.  Returns -1
> if the property cannot be found, otherwise it returns the length of the value
> data and if `val' is not NULL it copies up to `len' bytes of the property data
> to the location pointed to by `val'.
> 
> 
> int dev_delprop(struct device *dev, const char *name, int flags);
> 
> Remove a property from a device node.  If a NULL is supplied for the name,
> dev_delprop(9) will remove all properties from a device node.  It returns 0 on
> success or an error value.  This will fail without modifying the device node
> if it is protected.

these seem fine modulo name issues.


> size_t dev_mdgetprop(struct device *dev, const char *name, void *val, 
> 			size_t len, int flags);
> 
> If defined an a machine dependent header file, this function is called when
> ever an attempt is made to retrieve a property from a device node but the
> property is not found.  It allows machine dependent code to look up properties
> from other locations.  It should be implemented to behave the same way as
> dev_getprop(9) does.  It does not need to traverse parent device nodes.

How do you tell if a function is defined in a MD header file?  Are you
saying it's a macro?

It _may not_ traverse parent device nodes; if it does, that'll totally
screw up search order, right?


> 4	Converting Drivers
> 
> There are essentially two types of drivers, bus drivers and device drivers.
> Bus drivers are distinguished because they can attach other drivers to
> themselves.  The conversion process consists of four steps.  First a protocol
> must be established for the use of properties during probe and attach.  Then
> the bus driver needs to be converted to use the transitional interfaces that
> provides both an `aux' and properties.  Next the device drivers attaching to
> it need to be converted to handle the new interface to use properties rather
> than `aux'.  Finally, the bus driver needs to be converted to use the new
> interface and only use properties instead of providing an `aux'.

yes.


> A protocol should be carefully defined to provide what ever information a
> driver needs from a bus so it can probe and attach.  A detailed discussion of
> the issues involved is well beyond the scope of this document, but the
> protocol should be able to handle layering of bus drivers.

yes.  The one concern I have here is that you've started to define the
protocol, by specifying that locators will be passed automatically,
without actually going into more details on that...


> The bus driver is then changed to manage its child device node explicitly.
> The driver calls call `dev_config_create()' to allocate a driver and attach
> properties to it.  It also creates an `aux'.  It then calls
> `config_found_sad()' with both the `aux' and the device it created with
> `dev_config_create()'.
> 
> Child drivers can now be modified to the new interface.  The `struct device'
> is removed from the softc, and `DEV_CFA_DECL()' is used to declare the
> device's `cfattach' structure.  The device's `*_probe()' and `*_attach()'
> routines are modified to cast the `aux' parameter to a `struct device *', and
> `dev_getprop()' is used to retrieve properties from it.  After the probe,
> `DEV_PRIVATE(dev)' is used to access the softc, rather than directly casting
> the `struct device *'.

Comments per the above.


> Once all the child drivers for a particular bus are converted, the bus driver
> can be modified not to create an `aux' at all, and the call to
> `config_found_sad()' is changed to a call to `dev_config_found()'.
> 
> 
> Appendix 1
> 
> Providing dev_mdsetprop() and dev_mddelprop() would be possible but make the
> framework much more complicated.  If permanent storage of properties is
> desired it should use some machine dependent method since non-volatile storage
> is not necessarily available on all architectures.
> 
> 
> Appendix 2
> 
> The config(8) application will eventually be extended to take typed values for
> locators and for properties that can be specified in config files.
> 
> 
> Appendix 3
> 
> A set of protocols need to be developed for the use of properties for
> attaching device and bus nodes.  Required global properties should be defined,
> as well as bus-specific properties.
> 
> 
> Eduardo