Re: General device properties API

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

> Date: Sun, 22 Aug 2021 06:42:41 -0700
> From: Jason Thorpe <thorpej%me.com@localhost>
> 
> > On Aug 21, 2021, at 2:09 AM, Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
> > 
> > I read the man page, and...I'm more confused now than I was before.
> 
> This isn't exactly a difficult concept to grasp.  I'm honestly
> baffled at your confusion.

Currently, device_properties is strictly a NetBSD-internal interface
between drivers and MD platform code.  So the naming authority is
NetBSD -- you can grep through src/sys to find how it's defined and
used.

OF/fdt_getprop and acpi_eval_* are an OS<->firmware interfaces.  So
the naming authority is the firmware vendor.

At first I thought device_getprop was supposed to be essentially the
latter, an OS<->firmware interface, but with a single API name to
cover OF/fdt_getprop and acpi_eval_* instead of writing out the type
of firmware interface each time.  Maybe this reduces code duplication
if the same name, with the same meaning, is sometimes available via
both ACPI and FDT.

But now the man page seems to indicate it's a combination of all of
these, which is what I meant by `I'm more confused now': `I have an
even less definite idea of what properties are supposed to mean and
who is responsible for defining them.'

(I also got a less definite idea of where we could improve type- and
typo-safety -- for NetBSD device_properties in principle we could have
made type/typo-safety mandatory requiring a .h file between .c files
setting and reading each property we use; for OF/fdt_getprop,
acpi_eval_*, that obviously can't be mandatory because we don't
control who sets them.)


> > 1. Who defines properties?  Who is the naming authority that a driver
> >   author or auditor consults to determine what the meaning of a
> >   property is?  Is it NetBSD, or is it vendor-supplied (and, perhaps,
> >   -documented) firmware?  If you set a property, how do you avoid
> >   colliding with existing meaning for the name?  How do you audit a
> >   driver to determine whether it's making sensible use of the
> >   properties?
> 
> Right now in NetBSD it's the wild f***ing west.  It's my personal
> opinion that we should just adopt the Device Tree specification's
> bindings / conventions for everything.
> 
> Note that that spec has a naming scheme that allows for
> vendor-specific and OS-specific properties.

Thanks, I think that would improve the proposal substantially by
making it more concrete.  Just to be clear, you mean
<https://www.devicetree.org/specifications/>, right?  What would
happen on platforms with, say, ACPI but no OF/FDT?


> > 2. Why is there now a third copy of essentially the same dictionary
> >   lookup API, for devhandle_t?  When would a driver choose device_*
> >   or devhandle_* and why?
> 
> If you have a kernel device object, use device_getprop_*().  If a
> kernel device object is not available (because maybe you're looking
> at a node from the underlying device tree that doesn't [yet] have a
> kernel device object associated with it), then use
> devhandle_getprop_*().

Can you give examples of where you would end up with a device handle,
in code that is not ACPI- or FDT- or OF-specific to begin with?

Or, would device_getprop(sc->sc_dev, ...) ever be any different from
devhandle_getprop(device_handle(sc->sc_dev), ...)?  If yes, when and
why would this happen?


> > I'm seeing a lot of new mechanism here but what I really want to see
> > is how this helps writing, maintaining, and auditing device drivers.
> 
> It's not super easy to demonstrate with the way our system currently
> works.  But, for example, there are things for which FDT has a set
> of bindings for a thing (like i2c HID, for example) and ACPI also
> has bindings for that thing.  We end up with #ifdefs or other ad hoc
> run-time checks.  As a first order approximation, every place you
> see `#ifdef FDT' in a driver is a failure to have some sort of
> unifying API that works across platform configuration schemes.

I grepped for #ifdef FDT to try to find examples of what you mean, and
here's what I found:

1. Various MD platform initialization logic, like
   arm_fdt_platform_init in aarch64_machdep.c and arm32_machdep.c.
   Not sure this is relevant.

2. fdt-related ddb commands.  Also not relevant here.

3. RTCs (i2c/ac100.c, i2c/as3722.c, i2c/pcf8563.c, i2c/twl4030.c),
   which use OF_finddevice(fdt_getprop("rtc0")) to check whether the
   device in question is the primary RTC, and if not, to skip
   todr_attach.  Would the device_getprop* API be able to replace
   this?

4. Attaching i2c buses (i2c/motoi2c.c), spi buses
   (arch/arm/imx/imxspi.c), gpio pins (i2c/twl4030.c,
   spi/ssdfb_spi.c), interrupt handlers (i2c/tcakp.c), and
   power/regulator controllers (i2c/as3722.c, i2c/tps65217pmic.c) with
   FDT, and some OF/FDT-specific logic under `if (devhandle_type(h) ==
   DEVHANDLE_TPYE_OF)'.  Don't think anything about device_getprop* is
   relevant here, but maybe I misunderstood?

I also grepped the thorpej-i2c-spi-conf branch for #ifdef FDT, but
except for a few more of cases of (4) under sys/dev/spi (mcp23s17.c,
mcp3k.c, ssdfb_spi.c) I didn't see anything else.  If i2c HID is a
good example, could you show how ihidev.c can handle both FDT and ACPI
with a unified API, and how that unified API would map to both FDT and
ACPI?

I think you have a clearer model in your head of a lot of things not
written down that aren't as clear to me.  It would be helpful if you
could illustrate things with concrete examples, or specific source
references, how device_getprop* would improve drivers.

It doesn't have to be working, running code -- just a sketch of what
the code would roughly look like, and a sketch of how it would
seamlessly handle multiple different platform configuration schemes.


> One of my goals here is that there can be a unifying API that allows
> Ethernet drivers to easily get it right (named something like
> ether_device_get_mac_address() or whatever) that itself under the
> covers uses a combination of device_getprop_*() and device_call()
> (to access a device tree [FDT, ACPI, whatever] or a
> machine-dependent method, if the platform has one).

Could we add ether_device_get_mac_address() now, fruitfully unified
across a couple platforms even if it has to use #ifdefs?  Later on it
could be changed to remove the #ifdefs, but it's always good to start
with several concrete instances before making an abstraction of them.


> > P.S.  I want to get a clearer idea of the higher-level purpose first
> > before going into implementation details, but I also want to put up
> > front that I'm very uncomfortable with the untyped runtime string
> > method name dispatch mechanism of device_call -- and I'd like to see
> > that addressed before we start committing to users of it that make it
> > harder to change.
> 
> As for the way device_call() works, propose an alternative with the
> following attributes: [...]

Here's a sketch of a typed version of what I think is essentially the
same thing the untyped device_call does:

typedef struct {
	void *private;
	struct devhandle_ops *ops;  /* `vtable', if you will */
} devhandle_t;

struct devhandle_ops_v1 {
	int (*dho_getprop_string)(devhandle_t, const char *, int, uint32_t *);
	...
};
...
struct devhandle_ops {
	unsigned version;
	struct devhandle_ops_v1 v1;
	struct devhandle_ops_v2 v2;
	...
};

(If consecutive versions isn't enough for some reason we could use a
typed key/value mapping instead, something like vops although I'm sure
we can do better than the awful vop table definitions.  But I'd also
like to get an idea of what semantics we need for the driver model
before committing to implementation details like this.)