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

[Iain Hibbert <plunky%ogmig.net@localhost>]

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

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

 On Sun, 1 Jan 2023, Taylor R Campbell wrote:
 
 > > Date: Sun, 1 Jan 2023 13:24:39 +0000 (GMT)
 > > From: Iain Hibbert <plunky%ogmig.net@localhost>
 > > 
 > > 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.)
 
 Well ok, but I think that might also be grody :)
 
 > Using modules can also help to formalize dependencies between
 > subsystems -- dependencies which are historically only informally
 > implied by the topological sort encoded in main.
 
 Its true, and I have thought that as a subsytem netbt/ may be more suited 
 to modularity than many others. However, it is not one as yet and I 
 thought of something before which could be difficult though I can't recall 
 it just now.
 
 > > so, simpler solution could be to wrap the alloc in RUN_ONCE and also call 
 > > bt_init() from hci_attach_pcb() to ensure the mutex is ready; this avoids 
 > > 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.
 
 Ok, and I note that RUN_ONCE is a little racy as it doesn't hold 
 another caller back, and this function should be quick but it does 
 alloc(PR_WAITOK)
 
 > 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.
 
 I can do that, but I'm not sure its useful? Thinking it through if kpause 
 was before the alloc the second caller would crash taking the mutex, and 
 if after the alloc it would never crash just delay the boot whichever got 
 there first.
 
 The alloc should not sleep this early in boot, but if it did then a second 
 caller would also crash.
 
 In fact, I'm coming around to the module_init method. The mutex should be 
 ready before anything can look at it. To be clear, I don't like using 
 module init, its a cheat to attach an init function to something like that 
 just because it happens at the right time. But, using RUN_ONCE doesn't 
 seem correct either; it is not for synchronization.
 
 iain