Re: kern/56988 (Bluetooth stack initializes bt_lock too late)

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

The following reply was made to PR kern/56988; it has been noted by GNATS.

From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: Iain Hibbert <plunky%ogmig.net@localhost>
Cc: gnats-bugs%netbsd.org@localhost
Subject: Re: kern/56988 (Bluetooth stack initializes bt_lock too late)
Date: Sun, 1 Jan 2023 19:03:00 +0000

 > Date: Sun, 1 Jan 2023 13:24:39 +0000 (GMT)
 > From: Iain Hibbert <plunky%ogmig.net@localhost>
 >=20
 > I agree this is a bit grody, as the Bluetooth stack is not a module
 
 The stack doesn't have to be _loadable_ as a module to take advantage
 of module initialization.  (See, e.g., dev/cons.c.)
 
 The only grody part about what I suggested, in my opinion, is making
 it a _driver-class_ module, since it's not actually an autoconf device
 driver -- it just needs its initialization to run before configure
 runs.
 
 Using modules can also help to formalize dependencies between
 subsystems -- dependencies which are historically only informally
 implied by the topological sort encoded in main.
 
 > It seems that perhaps the domain init routines should be called before th=
 e=20
 > interfaces attach, but the other uses of .dom_init don't seem to have=20
 > actual devices attaching to the base of the domain and it hasn't been an=
 =20
 > issue for net/ as ifinit1() is listed especially in main() before=20
 > configure gets called.
 
 Calling domaininit earlier in main sounds plausible to me.  That might
 obviate the need for the weird `XXX must be after domaininit()'
 function ifinit_post.  Unclear if ifinit/domaininit have any ordering
 requirements, or if that XXX is even still applicable.
 
 > so, simpler solution could be to wrap the alloc in RUN_ONCE and also call=
 =20
 > bt_init() from hci_attach_pcb() to ensure the mutex is ready; this avoids=
 =20
 > entangling with module subsystem, does that sound ok?
 
 I'd like to avoid RUN_ONCE, except as a last resort; it generally
 indicates an incoherent design, and we have too many of those already.
 However, this should be fine for pullup to netbsd-10, to avoid any
 complicated fallout from changing module initialization order or
 domaininit/ifinit ordering in main.
 
 So here's a straw proposal, subject to actual testing:
 
 1. Pull up the RUN_ONCE approach to netbsd-10.
 
    Test: Insert kpause(10*hz) in bt_init_once, verify a machine with
    ubt(4) boots without crashing on the mutex.
 
 2. Try changing main from
 
         ifinit1
         ...
         configure
         ...
         ifinit
         domaininit
         ifinit_post
 
    to
 
         ifinit1
         domaininit
         ...
         configure
         ...
         ifinit
         ifinit_post
 
    and then merge ifinit and ifinit_post.  I skimmed all the .dom_init
    functions, and they look likely to be safe to use early on.  I also
    found a probably-unnecessary RUN_ONCE in key_init (arising from the
    time when percpu(9) couldn't be used that early).
 
 After that, I'm tempted to move everything from ifinit1 and
 ifinit_post into a single ifinit that happens before domaininit --
 what's left in ifinit and ifinit_post is...not much!
 
 Sound reasonable?