Subject: Re: Device Properties: The Next Generation
To: None <eeh@netbsd.org>
From: Chris G. Demetriou <cgd@sibyte.com>
List: tech-kern
Date: 03/04/2001 23:08:55
eeh@netbsd.org writes:
> 	eeh@netbsd.org writes:
> 	> [ ... property flags ... ]
> 
> 	still not keen on grouping the "operation" flags with the "property
> 	meaning" flags.
> 
> bus_dma* and bus_space* group flags like this.

huh?

i didn't look at bus_dma, but bus_space seems to only have 'operation'
flags.  CACHEABLE, LINEAR, etc., affect the result of the mapping
operation.  As a side effect, they may affect the representation of
the data... but e.g. PROP_STRING or PROP_CONST really exist _only_ to
tweak the representation of the data.

Looked at another way, CACHEABLE, LINEAR, etc., don't neceesarily have
to be stored.  STRING and CONST do (as far as I can tell).


> 	you don't see e.g. mmap() mixing 'prot' and 'flags' even though it
> 	could.  This isn't different than that.
> 
> This isn't like mmap where there needs to be a separate
> set of protections and flags.

why did there 'need' to be such a thing for mmap()?  I'd argue it was
good interface design...  other 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.
> 
> Deriving the match and attach names from the `name' won't work
> when different buses share the same match and attach functions.
> This is done in many sparc device drivers.

#define if you want to do something funky.  (including defining entries to
NULL or other names entries.)

If we're going down this road, at minimum you need:

	detach,
	activate

as args to the fn, to cover entry points that are already part of
cfattach...


> 	> 
> 	> 2.4	New Functions
> 	> 
> 	>    struct device *dev_config_create(struct device *parent, int flags);
> 
> 	more discussion of this later.
> 
> 	dev_create, no parent, opflags.
> 
> Needs to have WAIT/NOWAIT passed in, and needs parent so
> it's put on the tree in the right place and depth searches
> work.

yeah, i was thinking about this:

(1) you do need parent, for the reason you specify.  I think i was
just knee-jerking against the notion of linking it in to alldevs (or
as a child) until it was attached.  Sorry about that.

(2) you still shouldn't 'put it into the tree' until it's attached.  8-)


> 	also, it occurs to me that we need a dev_destroy...
> 
> dev_detach() does that.

i don't think that's sufficient.

e.g.:

	dev_create() to get a new device you're going to try to attach
	attempt to copy or set properties
	that fails.

uh oh, how do you nuke the device?  You pretty much have to -- if you
were doing a copy, the state is now ... not well defined.

dev_detach() isn't the right thing -- it's not an attached device yet.


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

I think another note has been posted about this...


> 	(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.
> 
> Don't ask me.  I just provided the interface because some buses
> use this directly.

The problem is, not all of the existing uses will map to it...  As
part of a proposal like this, you really need to think about how
problems like those get solved with the new system.

It's actually kinda disturbing that not only did you not seem to
notice this stuff before, but you sound like you want to punt on it
now...


> I think calling config_search() directly is
> a Bad Thing.  I would be happy to make config_search() an internal
> interface only.

uh... how would you implement indirect-config busses, except via
config_search()?

This has to be figured out; a lot of busses rely on it.  Unless you're
going to introduce a new mechanism that does the same type of thing we
would lose support for, oh, ISA, VME, who knows what else -- those are
the two that jump out at me.


> 	Also, this just occurred to me: how do cfmatch_t, cfprint_t, et
> 	al. change? 8-)
> 
> Hm.  They may need changes to take a struct device* rather than
> an `aux'.

or, in transition, both, as previously noted.  print doesn't have
enough info to tell what it's been passed, otherwise...


> 	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?
> 
> No, I don't think so.  I can't see them ever being used. 
> And it violates the definition of a property: (dev, name, data).
> If any of the three is different, it's a different property. 

That makes it harder to do submatch fns...


> Yeah, the locators are an issue.  We could have the MI code
> do the locator comparison, taking in to account wildcards.
> The only question is how you present wildcarded locators 
> to a program trying to extract a config file from a running
> kernel.  
>
> This is a protocol issue that should be discussed separately.

I could buy that.  8-)

Is the goal to generate a hardwired config file from the running
kernel, or the 'original' config file?

The latter you could do looking at the raw data.  the former you
should do using only the properties.


> 	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.
> 
> No, the child node is simply passed instead of the `aux'.  No
> change of function signatures needed.  Ever.

You lose some type checking this way.  Also, you still have access to
the parent data that isn't really needed/wanted anymore...

I actually think the type checking thing is somewhat
important... because e.g. you're changing the interfaces pretty
heavily, you don't necessarily want unmodified old interfaces to drop
in & compile when completely converted.



> 	s/_config// i think.
> 
> 	this _should not_ take a parent, and should not attach into the device
> 	tree.
> 
> I thought you wanted it in the device tree so you could query 
> recursively in the *probe() routine.

Yeah; see my comments above.  This was a thinko.  Chalk it up to to
little time to pay attention with.  8-)

It should take a parent and point to it, but not be otherwise attached
into the tree until attach.



> 	> 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.
> 
> O.K.  So if you use this function you need to provide a non-NULL
> child?

No, that just doesn't make sense.

The parent bus is going to create a child, and fill in its properties
(either via a submatch fn called through config search, or before calling
config_found).

If the parent can't fill in properties and have them passed down, what's
the point at all.

locators sholdn't be automatically added to children.


> I don't like the idea of having macros for all the 
> match/attach/detach functions.  If we want to change
> the function signature, I would handle it all inside
> the DEV_CFA_DECL() macro: 
> 
> 	1) Declare the match/attach functions with the
> 	new parameters.
> 
> 	2) Cast them inside the the parameter list to
> 	the appropriate type.
> 
> That way when you use DEV_CFA_DECL() you will be forced
> to change the calling sequence or have a conflicting
> function declaration.

so, if you do that, how do you handle the case where, at the end
of the conversion, e.g. 'parent' should go away entirely, without
modifying every single driver _again_?


> No.  No hidden argument.  You either get a (parent, cf, aux) or
> a (parent, cf, child).  No point in keeping vestiges of the old
> interface around.

Including, when the final conversion is done, parent.  8-)


> I was thinking that the submatch could compare the locators
> attached by the parent to the device structure against the
> locators provided by config.  But this is open to revision.

But, other than this comparison, what's the purpose of attaching
the locators provided by 'config' at all?

You could certainly do this... but it basically means:

(1) every device has two complete sets of locators, one of which
will never be used after it's attached,

(2) starting the time that this is implemented, every bus's locators
are set in stone as parts of that bus's 'protocol'.

I think (1) is silly and wasteful, and (2) is ill-advised.


> [ md getprop ]
> 	It _may not_ traverse parent device nodes; if it does, that'll totally
> 	screw up search order, right?
> 
> Well, I don't really care what it does as long as it returns the correct
> property.

Right, but, what happened to the whole discussion of search order
semantics?

the search semantics should be well-defined, and (i thought) were.
That's the only way you can guarantee what "the correct property" is,
in fact.


cgd