eeh@netbsd.org writes:
> Another version with many enhancements:

s/enhancements/changes/

I thought we were getting closer to closure on this, I think this new
proposal introduced a few 'random' things hadn't been mentioned at all
previously, but also took some in just the wrong direction.

I think maybe there are more 'wrong' changes in this update than there
are right ones, unfortunately.


> The device structure will be modified as follows:

No, that's the new device structure.  It would have been better to
call out your modifications.


> 	CIRCLEQ_HEAD(dv_children, device) dv_child;	/* circular list of children */
> 	CIRCLEQ_ENTRY(device) dv_sibling;	/* circular list sibling device */

These haven't been brought up before, and I'm not at all convinced
they're either a good idea or necessary.


> 	SLIST_HEAD(devpropslist, devprop) dv_props;
> 	void	*dv_private;
> };
> 
> 
> 2.1	Flags
> 
> PROP_WAIT		-- Specify that the call can sleep.
> PROP_INHERIT		-- Inherit properties from parent.
> PROP_STRING		-- Property is a string.
> PROP_CONST		-- Property is constant do not allocate storage
> 			   for the data.

These are logically two different sets of flags.  They should be
separated.

I think if you have a solution which provide sane limitation of
inheritance (which I think you need), there's no need for
PROP_INHERIT.


> 2.2	Macros
> 
> DEVICE_PRIVATE(dev)	-- Access the private softc data of dev.
> 
> 
> 2.3	Functions
> 
>    struct device *config_create(struct device *parent, int flags);
>    struct device *config_found_smd(struct device *dev, void *aux, 
> 			cfprint_t print, cfmatch_t submatch, 
> 			struct device *child);
>    struct device *dev_config_found(struct device *parent, 
> 			struct device *child, cfprint_t print, 
> 			cfmatch_t submatch);
>    struct cfdata *dev_config_search(cfmatch_t fn, struct device *parent, 
> 			void *aux, struct device *child);
>    struct device *dev_config_attach(struct device *parent, struct cfdata *cf, 
> 			void *aux, cfprint_t print, struct device *child);

No.  The new-style name functions should _not_ take 'aux' at all.  If
you want to provide functions which take both, to help
backward-compatibility, they should be provided with names similar to
the old functions (e.g. _d at the end).  You get this right for
config_found_smd, but other than that, it's wrong.

The goal here is to completely remove 'aux' right?  Well, if you do
that, why should we have to go through and edit large numbers of
drivers in the tree when it's time to do that?


> 
>    int dev_setprop(struct device *dev, const char *name, void *val, 
> 			size_t len, int flags);
>    int dev_copyprops(struct device *source, struct device *dest, 
> 			int flags);
>    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 md_getprop(struct device *dev, const char *name, void *val, 
> 			size_t len, int flags);

These seem OK modulo the need to limit inheritance.  Some mechanism to
limit inheritance at the source of the data should be provided.


> 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, md_getprop() that is called if
> attempting to retrieve a property fails.  md_getprop() 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.
> 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.
> 
> The second phase will separate the `struct device' from the device's softc.
> 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 seems OK enough.

> If devices are detached, the `struct device' will remain
> with all its properties but the softc will be destroyed.  This change will
> cause breakage in practically all existing device drivers, so it will be
> relegated to some future date.

If devices are detached, the 'struct device' will remain attached to
the device tree?  _NO_.  If a device is detached, it is _detached_.
Removed.  Completely.  'struct device' gets completely destroyed.

> 
> 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 circular list of child devices will be added so device tree traversal can be
> done both up-tree and down-tree.  The head of the list is the `dv_child'
> field, and the `dv_sibling' field will be used for nodes on the list.

This is entirely new.  It's not clear that you need it, especially not
for any of the functionality described in here.

I don't think it should be added.  unless there's a compelling reason
for it.  The fact that it's only popped up at this late date kind-of
makes me think that, as far as this discussion goes, there's not.


> 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'.
> 
> 
> 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 Specify a negative size in the `ca_devsize' field of
> 	  the `cfattach' structure they provide.
> 
> By doing that, the `aux' parameter will point to a `struct device *', with all
> all locators attached as properties in addition to any properties the parent
> driver has attached. In addition, any `aux' structure provided by the parent
> can be accessible from the device pointer through the use of the
> `DEVICE_PRIVATE(dev)' macro.

No.

This sohuld pretty much all be done with macros, and not negative
sizes.

There should be a macro, to be used to define the entire struct
'cfattach,' used by drivers which have been converted.  Similarly,
there should be macros to define the match, attach, and detach
functions.

In the first phase, cfattach will always contain complete size, softc
+ struct device.  In the second, cfattach will only contain size of
softc.

In the first phase, definitions of converted match/attach fns will
take an extra arg, and will be cast appropriately.  'aux' will be
accessable through an argument, but should not be used.  In the second
phase, 'aux' will be removed entirely.


The notions here being:

(1) in the end, you want _no_ trace of the old framework left over.
    No mentions or use of 'aux', no negative numbers, all match
    routines taking a prototype device rather than a void *.

(2) Based on (1), converted drivers should _never_ use the 'aux' data
    for anything.

(3) The scheme described above will get you there modifying drivers
    exactly once, to convert them.  Also, if you decide that you want
    to tweak the match/attach prototypes in the future, it's slightly
    easier to do so.


> config_rootsearch() will be modified to provide devices with locators as 
> properties to new style devices as dev_config_search() below.

Unless i'm mistaken, root busses are currently machine-dependent.
Therefore, i'd say, when 'root' is converted to use properties, all of
a given port's children should be converted.

Therefore, all you have to do is:

* leave config_rootsearch() and config_rootfound() alone,

* don't provide _d variants of then, and

* provide dev_config_rootsearch() and dev_config_rootfound() which
take property holders as their non-root counterparts do.



> 
> 3.2	New Functionality
> 
> DEV_PRIVATE(dev)
> 
> Retrieves a pointer to the device's private data.
> 
> 
> struct device *config_create(struct device *parent, int flags);
> 
> 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 config_create(9) will not sleep to resolve resource
> shortages.  Returns a pointer to a `struct device' on success, and NULL on
> error.

It should _never_ attach anything to the device tree.  I should keep a
reference to parent, for inheritance, but until actually attached it
should _not_ be linked into the device tree.


> struct device *config_found_smd(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_smd(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).

This seems right enough, except you need to have some mechanism by
which properties are made read-only, and that should be used by
config_found_smd() and dev_config_found().


> 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.  

Same as above.


> struct cfdata *dev_config_search(cfmatch_t fn, struct device *parent, void *aux, 
> 			struct device *child);
> 
> dev_config_search(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.  

No, no aux.  If you want a variant that does backward compatibility
(and you should 8-), then you want a config_search_d() or similar.


> If the device being probed specifies a negative value in the `ca_devsize'
> field of the `cfattach' structure, a `child' device is created if it does not
> exist.

I don't see any reason to do this.  If the parent didn't supply a
device to hold properties, and the child requires one, then that's
simply an error.

the property-getting/setting code should probably recognize 'NULL'
devices and panic.

> Locator data are attached to the `child' as properties, and the
> `aux' parameter is placed in its device private data field.  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.  If it needs access to the aux data, use of `DEVICE_PRIVATE()'
> on it will return the `aux' provided by the parent.

No.  New-style drivers should never be able to get to aux.


> If the device provides a positive value in `ca_devsize' field of the 
> `cfattach' structure, 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 from the *_probe() routine.
> 
> struct device *dev_config_attach(struct device *parent, struct cfdata *cf, 
> 			void *aux, cfprint_t print, struct device *child)
> 
> If a non-NULL `child' is passed in to dev_config_attach(9), and the device
> being probed specifies a negative value in the `ca_devsize' field of the
> `cfattach' structure, a softc of that size will be allocated and a pointer
> to it placed in the device private field of the `child' device.
> 
> If a non-NULL `child' is passed in to dev_config_attach(9), and the device
> being probed specifies a positive value in the `ca_devsize' field of the
> `cfattach' structure, 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.

pretty much same comments as above.

NULL child is not allowed 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 an ASCIIZ string.  Returns 0 on success or an error value.

s/ASCIIZ/NUL-terminated ASCII/


> 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
> md_getprop().  If PROP_WAIT is not specified then dev_copyprops(9) will not
> sleep for resource shortage.  Returns 0 on success or an error value.

What will the state of 'dest' be on error?


> 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 md_getprop(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'.

see other discussions around inheritance.




cgd