Subject: Re: Device Properties: The Next Generation
To: None <eeh@netbsd.org>
From: Chris G. Demetriou <cgd@sibyte.com>
List: tech-kern
Date: 02/15/2001 16:37:09
eeh@netbsd.org writes:
> 	To this end, I suggest you make the following changes to existing
> 	device stuff (in addition to adding the properties list).
> 
> 		* add a new devclass, DV_PROPERTIES (for non-devices that are
> 		  solely property containers.)
> 
> 		* add a new flag, DV_PROP_IMMUTABLE, which indiates that
> 		  a device's properties are immutable.
> 
> Actually, I spent some time thinking about this issue. 
> If you don't trust your child nodes not to mess up your
> properties, why should you trust your child nodes not to
> remove the immutable bit?

To large degree, it's not a matter of trust, it's a matter of
accident.  Code using the interface to its fullest should be able to
protect itself from accidental problems like this.

An example of this is the whole "indirect config front-end sloppily
converted to be direct config front-ends" issue, where drivers
(accidentally) modify the 'aux' values passed into them.

One answer is, of course, 'write code better.'  But i'm personally in
favor of providing tools that allow sanity checking.  (maybe even, if
immutable and something tries to write, have an option to panic
instead of just returning an error...  8-)


> Hence I added dev_copyprops(9),
> so you can keep a private property container that nobody 
> else has access to.
> 
> Also, dev_destroy(9) checks to see if the perperty container
> is unused before deleting it.  That way you can do:

I didn't get anything from your latest message that said "checks to
see if it's unused" or similar...

(how do you propose to do this?)


> 	create properties container
> 	config_search(... properties container ...)
> 	destroy properties container
> 
> or:
> 
> 	create static properties container
> 	set up properties that will never change
> 	loop {
> 		create dynamic properties container
> 		copy properties from static properties container
> 			to dynamic properties container
> 		set up properties that change from dev to dev on
> 			dynamic properties container

BTW, this example makes me ask whether your fake static properties
containers need to to specify inheritance.  I'd think they do.

does copyprops copy the inheritance (parent) pointer?  8-)


> [ ordering of getprop ]
> 
> No, the order is slightly different:
> 
> 	(1) look at current device
> 	(2) call md_getprop()
> 	(3) if (PROP_INHERIT) look at parents for interhited devices
> 
> The reason for this is that md_getprop() is specific for that particular
> device, and when dev_getprop() is called on the parent, it will also
> call md_getprop() on that device before going on to it's parent, etc.

Ahh, that makes sense I guess.


> 	I don't think it's right for there to be a mechanism by which a device
> 	can modify the properties of its parents.  (Under what circumstances
> 	do you think this is a valid thing to do?)
> 
> Frankly, I don't thinka device should ever delete properties
> from parent nodes.  I would be quite happy to remove both
> PROP_INHERIT and PROP_ALL functionality.

"nuke 'em!"  8-)


> 	There definitely needs to be a way to 'reset' the inheritance, though,
> 	i.e. terminate the inheritance chain.  Probably DVF_PROP_NOINHERIT,
> 	which says that this node doesn't inherit from its parents.
> 
> 	That way, to clear a device's slate, you might do:
> 
> 		(1) set NOINHERIT,
> 		(2) delete all.
> 
> 	Actually makes me wonder if you want to have the equivalent of a
> 	'whiteout'.  (it might be useful to see how often that'd be used in
> 	practice, and leave the door open to implementing it later...)  If
> 	that type of thing isn't supported, it might be useful to say
> 	something like:
> 
> 		(1) copy all inherited values down from parent,
> 		(2) set NOINHERIT.
> 
> Since you specifically select whether you want to PROP_INHERIT
> or not when you do the query, none of that is really needed.
> To clear a device's node all you need is:
> 
> 	dev_delprop(dev, NULL, 0);

Right, so, that raises some issues...

you've gotta be very careful about whether or not you use
PROP_INHERIT.  (I'd expect that, in general, you'd always say "yes, do
inherit."  What are you expecting the default usage to be?  Or do you
think it'll really depend on which attribute you're asking about?)

It also means that if you want your _child_ to see none (or some
subset) of the parent's properties, if that child has to use
inheritance, you're kinda out of luck.  e.g.:

	pchb
	pci at pchb
	dev at pci

I think you'd actually _want_ to have the 'dev' get its
pci_chipset_tag_t via inheritance from the PCHB.  otherwise, the pci
would just be doing property additions left and right for no reason.
(In that case, what's the point of inheritance?)

	pchb
	pci at pchb
	pcib at pci
	isa at pcib
	dev at pci

well, all of a sudden now, if you're looking for a PCHB by
inheritance, you're going to find the one inherited from pchb...

Not necessarily desirable.  Interrupt routing information might even
be more confusing.

One answer to this is keeping bus-dependent match/attach routines
(like I said before, I think you've gotta do that anyway 8-), and be
_very_ careful about what properties they look at...

I dunno, it's just an obvious feature that I expected to see, but
didn't.  I expect that when you start trying to combine attachments,
you'll run into problems here (esp. for ISA-ish devices that are
effectively living behind other ISA-ish devices, e.g. multiport cards,
sound cards, etc.)


> Hopefully, config_found_smd() would eventually be phased
> out.  (Yeah, sure.)

If you think in terms of years, sure.  8-)


> Actually, we could get rid of this `aux' vs. `propdev' distinction in
> the function signitures by basing it on a property associated with the
> parent (bus) node.  If the property "device-properties" is set it casts
> `aux' to a `struct device *' and uses it to pass propreties.  If that
> property is not set it uses it at an opaque `aux' structure the way
> it works now.
>
> But that would preclude mixing old and new attachment schemes.  So
> macros are probably a better way to go.  Unless there were some way
> to provide this information in the cfattach strucure....

you might be able to stuff a special value at the end of cfattach --
if non-zero, then it'd be new style.

but that makes it harder to flip the switch after everything's been
converted, and I dunno how you'd tell before hand if everything's been
converted...


> And add a new macro, DEV_PRIVATE(type, dev) that will initially
> cast the `struct device' pointer to the softc, then later dereference
> the pointer to the softc in `struct device' once the two are separate.

That's what I was saying in the chunk:

> (1) add a new fn/macro dev_private_date(struct device *), which
> returns a pointer to the private data.

cast of just the given struct device ptr doesn't work, you need ((void
*)&dev[1]) to get at the private data.  8-)

And, type is "void *".  eventually, that all becomes:

	dev->dv_private

(indeed, you could do that up front, just making dv_private always
point to after the 'struct device'  8-)


chris