> Date: Wed, 10 May 2023 01:08:27 +
> From: Taylor R Campbell
>
> I propose to add new config_*_acquire/release functions: [...]
Updated patch so that the legacy config_* operations require the
caller to hold the kernel lock, while the new config_*_acquire/release
ones do not (but take it internally until we make autoconf internals
MP-safe).
This way you can eliminate KERNEL_LOCKs around finding and detaching
children, without introducing races with concurrent drvctl -d:
// no KERNEL_LOCK(1, NULL);
dev = config_found_acquire(...);
mutex_enter(>sc_childlock);
sc->sc_dev = dev;
mutex_exit(>sc_childlock);
device_release(dev);
// no KERNEL_UNLOCK_ONE(NULL);
// no KERNEL_LOCK(1, NULL);
mutex_enter(>sc_childlock);
if ((dev = sc->sc_dev) != NULL)
device_acquire(dev);
mutex_exit(>sc_childlock);
config_detach_release(dev, ...);
// no KERNEL_UNLOCK_ONE(NULL);
Also included:
- Conversion of dk(4) to config_*_acquire/release -- lightly tested.
- Conversion of usb(4) to config_*_acquire/release -- compile-tested.
(Coming up is a series of other changes to fix related races in dk(4)
and simplify some of the logic. Also need to think about how devices
like vnd(4), cgd(4), , can safely work -- these logical drivers
sometimes do config_attach_pseudo in open, and sometimes do
config_detach in close, so their needs are a bit different from
drivers for physical hardware devices.)
>From baba80e17fbb254e50b242d71dc7e1de9bdc5520 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell
Date: Fri, 26 Aug 2022 10:35:08 +
Subject: [PATCH] autoconf(9): New functions for referenced attach/detach.
New functions:
- config_found_acquire(dev, aux, print, cfargs)
- config_attach_acquire(parent, cf, aux, print, cfargs)
- config_attach_pseudo_acquire(cf, aux)
- config_detach_release(dev, flags)
- device_acquire(dev)
The config_*_acquire functions are like the non-acquire versions, but
they return a referenced device_t, which is guaranteed to be safe to
use until released. The device's detach function may run while it is
referenced, but the device_t will not be freed and the parent's
.ca_childdetached routine will not be called.
=> config_attach_pseudo_acquire additionally lets you pass an aux
argument to the device's .ca_attach routine, unlike
config_attach_pseudo which always passes NULL.
=> Eventually, config_found, config_attach, and config_attach_pseudo
should be made to return void, because use of the device_t they
return is unsafe without the kernel lock and difficult to use
safely even with the kernel lock or in a UP system. For now,
they require the caller to hold the kernel lock, while
config_*_acquire do not.
config_detach_release is like device_release and then config_detach,
but avoids the race inherent with that sequence.
=> Eventually, config_detach should be eliminated, because getting at
the device_t it needs is unsafe without the kernel lock and
difficult to use safely even with the kernel lock or in a UP
system. For now, it requires the caller to hold the kernel lock,
while config_detach_release does not.
device_acquire acquires a reference to a device. It never fails and
can be used in thread context (but not interrupt context, hard or
soft). Caller is responsible for ensuring that the device_t cannot
be freed; in other words, the device_t must be made unavailable to
any device_acquire callers before the .ca_detach function returns.
Typically device_acquire will be used in a read section (mutex,
rwlock, pserialize, ) in a data structure lookup, with
corresponding logic in the .ca_detach function to remove the device
from the data structure and wait for all read sections to complete.
---
sys/kern/subr_autoconf.c | 185 ---
sys/sys/device.h | 7 ++
2 files changed, 181 insertions(+), 11 deletions(-)
diff --git a/sys/kern/subr_autoconf.c b/sys/kern/subr_autoconf.c
index e60be2d7aad8..55c04ff8ca9e 100644
--- a/sys/kern/subr_autoconf.c
+++ b/sys/kern/subr_autoconf.c
@@ -1259,17 +1259,21 @@ static const char * const msgs[] = {
* not configured, call the given `print' function and return NULL.
*/
device_t
-config_found(device_t parent, void *aux, cfprint_t print,
+config_found_acquire(device_t parent, void *aux, cfprint_t print,
const struct cfargs * const cfargs)
{
cfdata_t cf;
struct cfargs_internal store;
const struct cfargs_internal * const args =
cfargs_canonicalize(cfargs, );
+ device_t dev;
+
+ KERNEL_LOCK(1, NULL);
cf = config_search_internal(parent, aux, args);
if (cf != NULL) {
- return config_attach_internal(parent, cf, aux, print, args);
+ dev = config_attach_internal(parent, cf, aux, print, args);
+ goto out;
}
if (print) {
@@ -1283,7 +1287,39 @@
