Re: General device properties API

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

> Date: Sat, 11 Sep 2021 17:37:32 -0700
> From: Jason Thorpe <thorpej%me.com@localhost>
> 
> > On Sep 11, 2021, at 4:29 PM, Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
> > 
> > Here's another sketch that is much more flexible and general.
> > Adapting it to link sets should be easy; the main point is that the
> > only untyped parts going through void * are limited to the definition
> > site of a generic operation; everything else is fully typed.
> 
> Looks quite verbose, but I'll study it some more.  The sketch seems
> somewhat incomplete, so I'm not really getting how you're suggesting
> it should be used.

To define an operation (a `device call' or a `generic') that can be
implemented and invoked, create a .h file and a .c file for the
interface.

- The .h file contains
  (a) a convenience typedef for the operation's signature,
  (b) a macro for creating a vtable entry, and
  (c) a wrapper for extracting the vtable entry into the correct type.

  (The i2c_enumerate_devices_typecheck declaration in the example
  isn't actually defined as a symbol -- it's just there to allow
  compile-time type-checking of I2C_ENUMERATE_DEVICES_METHOD, so it
  can detect when the function has the wrong type before converting to
  void *.)

- The .c file contains
  (a) a definition of a symbol so the linker guarantees it's unique
      (link-time namespace collision detection),
  (b) a definition of the wrapper function that extracts the vtable
      entry with the correct type.

  Nothing else needs to be in the .c file -- it's very small (a few
  bytes) and is just needed by any code that uses the interface.  The
  wrapper function could also be in the .h file as a static inline.

A lot of this can easily be gathered into a handful of macros.  E.g.,

i2c_bus_interface.h:	DECLARE_GENERIC(int, i2c_enumerate_devices,
			    (struct i2c_attach_args *, bool (*) [...]));
			#define	I2C_ENUMERATE_DEVICES_METHOD(fn) \
				METHOD(i2c_enumerate_devices, fn)

i2c_bus_interface.c:	DEFINE_GENERIC(i2c_enumerate_devices);


A component that implements an operation just puts it in its vtable
(or link set or whatever):

	#include "i2c_enumdevices.h"

	static const struct vtable_entry acpi_device_handle_methods[] = {
		I2C_ENUMERATE_DEVICES_METHOD(acpi_i2c_enumerate_devices),
		...
	};
	static const struct vtable acpi_device_handle_vtable = {
		acpi_device_handle_methods,
		__arraycount(acpi_device_handle_methods),
	};

(Adapting to a link set left as an exercise for the reader; the basic
mechanism remains the same -- a list of entries tagged by a defined
symbol, with a wrapper for the correct type of that defined symbol.)


A component that needs to use an operation pulls it out of the vtable
using the wrapper to use it:

	struct i2c_attach_args ia;
	int error;

	error = i2c_enumerate_devices(h->dh_vtab)(&ia,
	    i2c_enumerate_devices_callback, cookie);


> Another thing I'm trying to comprehend here is just what kind of
> typos and type errors you think will be prevented.  If the name of
> the `args' structure is wrong, it will either fail to compile
> (because the args structure name used doesn't exist, so accessing
> fields will fail to compile), or the error will be glaringly obvious
> because you used `spi_enumerate_devices_args' when you should have
> use `i2c_enumerate_devices_args' and the member names are different.

	struct spi_enumerate_device_args args = ...;

	/*
	 * runtime memory corruption -- simply operates on the wrong
	 * type of object, when it should be a trivially detected
	 * compile-time type error
	 */
	rv = device_call(h, "i2c-enumerate-devices", &args);

	/*
	 * quiet loss of functionality -- simply returns ENOTSUP at
	 * runtime, when it should be a trivially detected
	 * compile-time typo error
	 */
	rv = device_call(h, "spi-enumerate-device", &args);

	/*
	 * more runtime memory corruption -- I've been bitten by
	 * something like pcibus_attach_args vs pci_attach_args
	 * because autoconf is not type-safe
	 */
	static int
	acpi_i2c_enumerate_devices(device_t dev, devhandle_t handle, void *v)
	{
		struct i2cbus_enumerate_devices_args *args = v;

		... args->ia, args->callback ...
	}

These are basic mistakes that at least I make all the time and that we
have had the technology to completely rule out at compile-time for
decades; I don't think it should be surprising for me to object if new
systems entirely under our control can't detect them.


> > If this doesn't meet the attributes you laid out earlier either, I
> > would appreciate it if you could identify which ones I missed --
> > there's too much going on here to fit in my head at once.
> 
> [...]
> ==> Does not require referencing a symbol that would cause a link
> failure if a method is not available on some platform scheme.
> [...]
> But one problem your vtable sketch has is that is requires
> referencing a symbol (i2c_enumerate_devices_generic,
> i2c_enumerate_devices_typecheck, and i2c_enumerate_devices).

Why is this a requirement/problem?

The current mechanism still needs space in the kernel for the text of
the name (the string "i2c-enumerate-devices").  Why refuse to expose
that name to the linker so it can detect typos and namespace
collisions?


> I suppose I should adjust my #2 property above... in addition to not
> causing a link failure if e.g. ACPI doesn't provide some method, nor
> should there be a link failure if ACPI provides enumeration support
> for I2C but the I2C module isn't currently loaded.

Only a .c file for the interface -- not the whole i2c subsystem --
needs to be included.