kbd: merge linker set drivers into standard kbd driver list

[kevans]

Summary

This leads to the revert of rS355806; for slightly increased complexity in driver registration, this reduces duplication in keyboard registration and driver switch lookup and leaves us with one authoritative source for currently registered drivers. The reduced duplication later is nice as we have more procedure duplicated in keyboard setup.

keyboard_driver->registered is used to more quickly detect bogus adds/removes, but we must still walk the list if it's not set to make sure it hasn't been added via linker set. From KPI consumers' perspective, nothing changes- kbd_add_driver/kbd_delete_driver must still be balanced. The exception is that a driver may now also kbd_delete_driver to remove itself from the driver list even if it's only registered by linker set, in case some init was not successful or something else has gone wrong.

Detection for already-registered drivers in kbd_add_driver has improved, as the previous SLIST_NEXT(driver) != NULL check would not have caught a driver that's at the tail end.

[kevans]

Simplify; we can de-constify the pointer in the linker set and avoid allocation/duplicate driver structs laying around. Individual members of the keyboard_driver will likely get const'ified in a later change, as these shouldn't be modified after initialization.

[imp]

sys/dev/kbd/kbd.c

168 ↗

(On Diff #65714)

Since this means that registered will always be 0 below, no?

185 ↗

(On Diff #65714)

How can this be false?
since p == driver.

197–204 ↗

(On Diff #65714)

why don't we do the above when we mark the driver as registers?

[kevans]

sys/dev/kbd/kbd.c

168 ↗

(On Diff #65714)

Hmm... Apparently I have local diff that didn't make it in. This got renamed to kbd_add_driver_internal and takes another parameter to indicate whether it's linker set addition or not

185 ↗

(On Diff #65714)

True- will fix. This should likely be an assertion instead as it should be tautological unless we've screwed up our accounting somewhere.

197–204 ↗

(On Diff #65714)

In my local version registered is only set when it's not a linker set addition to keep the balance

[kevans]

sys/dev/kbd/kbd.c

185 ↗

(On Diff #65714)

whoops, yeah, that's nonsensical... just ripping it out.

[kevans]

Revise! Closer to what the diff should have been (apologies for the mixup), but:

• with some feedback applied
• driver loop in kbd_add_driver eliminated; it was an artifact from when the linker set drivers were const and I stupidly allocated a copy of the driver struct for them
• loosened up restrictions in kbd_delete_driver so one can kbd_delete_driver as an in-kernel-only driver and no longer be considered as a kbd driver (not used currently, but we have no reason to object)

[imp]

sys/dev/kbd/kbd.c

169 ↗

(On Diff #65840)

Add a note saying you can't just always require the module for simplicity since the keyboard has to work before modules are resolved, loaded and their SYSINITs run.

185 ↗

(On Diff #65840)

why not ditch the linker_set arg, and the second flag and just return here?

197 ↗

(On Diff #65840)

Just have a KBDF_REGISTERED and be done with it, if you also do above.

[kevans]

sys/dev/kbd/kbd.c

185 ↗

(On Diff #65840)

So, the problem is that we tentatively have to allow a single kbd_add_driver to succeed whether or not the linker set has been merged in. Places like kbdmux_modevent care about the success of this, so we have two or three paths (and I already know you hate one =-P):

1. Continue to #ifdef out the add/delete driver bits in modevents
2. Differentiate between linker set additions and post-merge additions
3. Always succeed and just do nothing if the driver's already been registered

There's probably not much point in trying to detect duplicate adds -- a single kbd_delete_driver will unregister -- so #3 may be a great option here.

[kevans]

I guess #4 is to just use a registered int in the keyboard_driver instead, then kbd_add_driver can be:

int error;

error = kbd_add_driver_internal(driver);
if (error == 0)
    ++kbd->registered;
return (error);

and kbd_add_driver_internal just doesn't touch registered. Then the linker set merge bits use the _internal implementation and similarly don't touch registered.

[kevans]

Rework to remove the distinction between linker set additions and normal additions. kbd_add_driver can just silently succeed for now if it's already been registered so that kbdmux's modevent continues to work as needed.

There's now only the single flag, but I'm leaving it as a flags field for now so I can later consider adding some kind of 'busy' flag if we want to reject deleting drivers while there's still a keyboard registered with it.

[kevans]

I'm going to move this to being called from cninit rather than via SYSINIT, either in another revision or pre-commit if you have no further comments. It occurs to me that we traditionally make built-in drivers available at least that early, so we should maintain that and later work will move it to being called in cninit() anyways as we'll need some kbd locking available in syscons/vt

[imp]

I think we are ready here. Looks simple enough to get the job dobe