Subject: Re: Device Properties: The Next Generation
To: None <cgd@sibyte.com, eeh@netbsd.org>
From: None <eeh@netbsd.org>
List: tech-kern
Date: 03/05/2001 21:30:28
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.
BUS_DMA_NOWAIT and BUS_DMA_COHERENT are passed together to
bus_dmamem_map().
Looked at another way, CACHEABLE, LINEAR, etc., don't neceesarily have
to be stored. STRING and CONST do (as far as I can tell).
Both PROP_STRING and PROP_CONST are used to control how a
property is created rather than stored in the property.
> 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...
Because the `prot' bits are used throughout the rest of the
kernel.
I suppose I could replace PROP_WAIT with a separate argument
that takes M_WAIT/M_NOWAIT. It would simplify the code.
> > 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...
O.K. Add detach/activate functions to the macro.
> >
> > 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-)
I disagree. If it has a valid parent pointer it should be in
the tree. If it's not in the tree then it shouldn't have a
valid parent pointer and we need to continue to pass in the
`parent' device pointer to both the match and attach functions.
Otherwise it's in a state of semi-insertion that really does
not make sense.
> 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.
It's in the tree in an unconfigured state, but if you want a
separate dev_destroy() that would be simple to add. But then
dev_detach() should not be destroying the device node if you
want a nice orthogonal interface.
> 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...
If you use indirect config, you are taking responsibility
for all the config details. So when things change the
drivers need to be chaged as appropriate.
That's the reason to send all this to tech-kern.
> 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()?
I thought that the submatch function was added to config_found()
specifically to support indirect-config through config_found().
Or are there problems with doing that?
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.
So where are the ISA and VME experts?
> 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...
cfprintt_t is caled from config_found() and config_attach(). Both
of those routines know what the parent bus is and what form of
form of arguments should be appropriate to the print function.
Calling a *print() routine from a child driver is an extremely
questionable practice.
> 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...
For the most part, comparing a locator to a property on the
device node could easily be delegated to MI code rather than
the sbmatch function. In fact, we self-identifying buses could
provide a specific device identifier to the MI code to allow
config_search() to determine the exact match without calling
submatch functions. But the increased search speed gained
by doing somethi like that is probably not worth the effort.
> 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.
Since the transitional version will require casting the functions
to stuff them in the cfattach structure, I don't think you will
get strong typechecking until the old inface is removed.
> 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.
O.K. No locators added automagically. Should they be compared automagically?
> 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_?
We're making `parent' go away? O.K. If we do that it should
be handled in the initial conversion rather than later where
it will cause more breakage. If `parent' will not be needed
in the future then it should not be needed right now.
> 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.
s/(parent, cf, child)/(child, cf)/
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.
The reason to attach the locators created config as properties
is to provide that information to some userland program that
wants to extract device properties to create a config file
without having to grovel through kernel memory. If they
are not useful for the generation of a config file then there
is no reason to add them as properties.
> [ 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.
Well, if the property returned breaks search order semantics, it
is, by definition, not "the correct property". The search order
semantics are well defined for properties that exist in the device
tree. dev_getmdprop should function a manner appropriate to those
semantics.
Eduardo