Module Name: src
Committed By: riastradh
Date: Mon May 22 14:58:22 UTC 2023
Modified Files:
src/sys/kern: subr_autoconf.c
src/sys/sys: device.h
Log Message:
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, &c.) 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.
Proposed on tech-kern:
https://mail-index.netbsd.org/tech-kern/2023/05/10/msg028889.html
To generate a diff of this commit:
cvs rdiff -u -r1.310 -r1.311 src/sys/kern/subr_autoconf.c
cvs rdiff -u -r1.185 -r1.186 src/sys/sys/device.h
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/kern/subr_autoconf.c
diff -u src/sys/kern/subr_autoconf.c:1.310 src/sys/kern/subr_autoconf.c:1.311
--- src/sys/kern/subr_autoconf.c:1.310 Fri Apr 21 17:35:43 2023
+++ src/sys/kern/subr_autoconf.c Mon May 22 14:58:22 2023
@@ -1,4 +1,4 @@
-/* $NetBSD: subr_autoconf.c,v 1.310 2023/04/21 17:35:43 riastradh Exp $ */
+/* $NetBSD: subr_autoconf.c,v 1.311 2023/05/22 14:58:22 riastradh Exp $ */
/*
* Copyright (c) 1996, 2000 Christopher G. Demetriou
@@ -77,7 +77,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: subr_autoconf.c,v 1.310 2023/04/21 17:35:43 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: subr_autoconf.c,v 1.311 2023/05/22 14:58:22 riastradh Exp $");
#ifdef _KERNEL_OPT
#include "opt_ddb.h"
@@ -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, &store);
+ 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 @@ config_found(device_t parent, void *aux,
aprint_normal("%s", msgs[pret]);
}
- return NULL;
+ dev = NULL;
+
+out: KERNEL_UNLOCK_ONE(NULL);
+ return dev;
+}
+
+/*
+ * config_found(parent, aux, print, cfargs)
+ *
+ * Legacy entry point for callers whose use of the returned
+ * device_t is not delimited by device_release.
+ *
+ * The caller is required to hold the kernel lock as a fragile
+ * defence against races.
+ *
+ * Callers should ignore the return value or be converted to
+ * config_found_acquire with a matching device_release once they
+ * have finished with the returned device_t.
+ */
+device_t
+config_found(device_t parent, void *aux, cfprint_t print,
+ const struct cfargs * const cfargs)
+{
+ device_t dev;
+
+ KASSERT(KERNEL_LOCKED_P());
+
+ dev = config_found_acquire(parent, aux, print, cfargs);
+ if (dev == NULL)
+ return NULL;
+ device_release(dev);
+
+ return dev;
}
/*
@@ -1708,6 +1744,8 @@ config_add_attrib_dict(device_t dev)
/*
* Attach a found device.
+ *
+ * Returns the device referenced, to be released with device_release.
*/
static device_t
config_attach_internal(device_t parent, cfdata_t cf, void *aux, cfprint_t print,
@@ -1778,6 +1816,12 @@ config_attach_internal(device_t parent,
*/
config_pending_incr(dev);
+ /*
+ * Prevent concurrent detach from destroying the device_t until
+ * the caller has released the device.
+ */
+ device_acquire(dev);
+
/* Call the driver's attach function. */
(*dev->dv_cfattach->ca_attach)(parent, dev, aux);
@@ -1813,15 +1857,47 @@ config_attach_internal(device_t parent,
}
device_t
-config_attach(device_t parent, cfdata_t cf, void *aux, cfprint_t print,
+config_attach_acquire(device_t parent, cfdata_t cf, void *aux, cfprint_t print,
const struct cfargs *cfargs)
{
struct cfargs_internal store;
+ device_t dev;
+
+ KERNEL_LOCK(1, NULL);
+ dev = config_attach_internal(parent, cf, aux, print,
+ cfargs_canonicalize(cfargs, &store));
+ KERNEL_UNLOCK_ONE(NULL);
+
+ return dev;
+}
+
+/*
+ * config_attach(parent, cf, aux, print, cfargs)
+ *
+ * Legacy entry point for callers whose use of the returned
+ * device_t is not delimited by device_release.
+ *
+ * The caller is required to hold the kernel lock as a fragile
+ * defence against races.
+ *
+ * Callers should ignore the return value or be converted to
+ * config_attach_acquire with a matching device_release once they
+ * have finished with the returned device_t.
+ */
+device_t
+config_attach(device_t parent, cfdata_t cf, void *aux, cfprint_t print,
+ const struct cfargs *cfargs)
+{
+ device_t dev;
KASSERT(KERNEL_LOCKED_P());
- return config_attach_internal(parent, cf, aux, print,
- cfargs_canonicalize(cfargs, &store));
+ dev = config_attach_acquire(parent, cf, aux, print, cfargs);
+ if (dev == NULL)
+ return NULL;
+ device_release(dev);
+
+ return dev;
}
/*
@@ -1834,7 +1910,7 @@ config_attach(device_t parent, cfdata_t
* name by the attach routine.
*/
device_t
-config_attach_pseudo(cfdata_t cf)
+config_attach_pseudo_acquire(cfdata_t cf, void *aux)
{
device_t dev;
@@ -1867,8 +1943,14 @@ config_attach_pseudo(cfdata_t cf)
*/
config_pending_incr(dev);
+ /*
+ * Prevent concurrent detach from destroying the device_t until
+ * the caller has released the device.
+ */
+ device_acquire(dev);
+
/* Call the driver's attach function. */
- (*dev->dv_cfattach->ca_attach)(ROOT, dev, NULL);
+ (*dev->dv_cfattach->ca_attach)(ROOT, dev, aux);
/*
* Allow other threads to acquire references to the device now
@@ -1893,6 +1975,36 @@ out: KERNEL_UNLOCK_ONE(NULL);
}
/*
+ * config_attach_pseudo(cf)
+ *
+ * Legacy entry point for callers whose use of the returned
+ * device_t is not delimited by device_release.
+ *
+ * The caller is required to hold the kernel lock as a fragile
+ * defence against races.
+ *
+ * Callers should ignore the return value or be converted to
+ * config_attach_pseudo_acquire with a matching device_release
+ * once they have finished with the returned device_t. As a
+ * bonus, config_attach_pseudo_acquire can pass a non-null aux
+ * argument into the driver's attach routine.
+ */
+device_t
+config_attach_pseudo(cfdata_t cf)
+{
+ device_t dev;
+
+ KASSERT(KERNEL_LOCKED_P());
+
+ dev = config_attach_pseudo_acquire(cf, NULL);
+ if (dev == NULL)
+ return dev;
+ device_release(dev);
+
+ return dev;
+}
+
+/*
* Caller must hold alldevs_lock.
*/
static void
@@ -2000,9 +2112,12 @@ config_detach_exit(device_t dev)
* Note that this code wants to be run from a process context, so
* that the detach can sleep to allow processes which have a device
* open to run and unwind their stacks.
+ *
+ * Caller must hold a reference with device_acquire or
+ * device_lookup_acquire.
*/
int
-config_detach(device_t dev, int flags)
+config_detach_release(device_t dev, int flags)
{
struct alldevs_foray af;
struct cftable *ct;
@@ -2031,6 +2146,7 @@ config_detach(device_t dev, int flags)
* attached.
*/
rv = config_detach_enter(dev);
+ device_release(dev);
if (rv) {
KERNEL_UNLOCK_ONE(NULL);
return rv;
@@ -2186,6 +2302,32 @@ out:
}
/*
+ * config_detach(dev, flags)
+ *
+ * Legacy entry point for callers that have not acquired a
+ * reference to dev.
+ *
+ * The caller is required to hold the kernel lock as a fragile
+ * defence against races.
+ *
+ * Callers should be converted to use device_acquire under a lock
+ * taken also by .ca_childdetached to synchronize access to the
+ * device_t, and then config_detach_release ouside the lock.
+ * Alternatively, most drivers detach children only in their own
+ * detach routines, which can be done with config_detach_children
+ * instead.
+ */
+int
+config_detach(device_t dev, int flags)
+{
+
+ KASSERT(KERNEL_LOCKED_P());
+
+ device_acquire(dev);
+ return config_detach_release(dev, flags);
+}
+
+/*
* config_detach_commit(dev)
*
* Issued by a driver's .ca_detach routine to notify anyone
@@ -2740,7 +2882,7 @@ retry: if (unit < 0 || unit >= cd->cd_nd
mutex_enter(&alldevs_lock);
goto retry;
}
- localcount_acquire(dv->dv_localcount);
+ device_acquire(dv);
}
mutex_exit(&alldevs_lock);
mutex_exit(&config_misc_lock);
@@ -2749,9 +2891,30 @@ retry: if (unit < 0 || unit >= cd->cd_nd
}
/*
+ * device_acquire:
+ *
+ * Acquire a reference to a device. It is the caller's
+ * responsibility to ensure that the device's .ca_detach routine
+ * cannot return before calling this. Caller must release the
+ * reference with device_release or config_detach_release.
+ */
+void
+device_acquire(device_t dv)
+{
+
+ /*
+ * No lock because the caller has promised that this can't
+ * change concurrently with device_acquire.
+ */
+ KASSERTMSG(!dv->dv_detach_done, "%s",
+ dv == NULL ? "(null)" : device_xname(dv));
+ localcount_acquire(dv->dv_localcount);
+}
+
+/*
* device_release:
*
- * Release a reference to a device acquired with
+ * Release a reference to a device acquired with device_acquire or
* device_lookup_acquire.
*/
void
Index: src/sys/sys/device.h
diff -u src/sys/sys/device.h:1.185 src/sys/sys/device.h:1.186
--- src/sys/sys/device.h:1.185 Wed Aug 24 11:19:25 2022
+++ src/sys/sys/device.h Mon May 22 14:58:22 2023
@@ -1,4 +1,4 @@
-/* $NetBSD: device.h,v 1.185 2022/08/24 11:19:25 riastradh Exp $ */
+/* $NetBSD: device.h,v 1.186 2023/05/22 14:58:22 riastradh Exp $ */
/*
* Copyright (c) 2021 The NetBSD Foundation, Inc.
@@ -554,14 +554,20 @@ device_t config_found(device_t, void *,
device_t config_rootfound(const char *, void *);
device_t config_attach(device_t, cfdata_t, void *, cfprint_t,
const struct cfargs *);
+device_t config_found_acquire(device_t, void *, cfprint_t,
+ const struct cfargs *);
+device_t config_attach_acquire(device_t, cfdata_t, void *, cfprint_t,
+ const struct cfargs *);
int config_match(device_t, cfdata_t, void *);
int config_probe(device_t, cfdata_t, void *);
bool ifattr_match(const char *, const char *);
device_t config_attach_pseudo(cfdata_t);
+device_t config_attach_pseudo_acquire(cfdata_t, void *);
int config_detach(device_t, int);
+int config_detach_release(device_t, int);
int config_detach_children(device_t, int flags);
void config_detach_commit(device_t);
bool config_detach_all(int);
@@ -588,6 +594,7 @@ device_t device_lookup(cfdriver_t, int);
void *device_lookup_private(cfdriver_t, int);
device_t device_lookup_acquire(cfdriver_t, int);
+void device_acquire(device_t);
void device_release(device_t);
void device_register(device_t, void *);
