Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/trunk]: src/sys autoconf(9): New functions for referenced attach/detach.



details:   https://anonhg.NetBSD.org/src/rev/2d5f399abe5b
branches:  trunk
changeset: 375926:2d5f399abe5b
user:      riastradh <riastradh%NetBSD.org@localhost>
date:      Mon May 22 14:58:22 2023 +0000

description:
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

diffstat:

 sys/kern/subr_autoconf.c |  189 +++++++++++++++++++++++++++++++++++++++++++---
 sys/sys/device.h         |    9 +-
 2 files changed, 184 insertions(+), 14 deletions(-)

diffs (truncated from 352 to 300 lines):

diff -r a90a08e824ce -r 2d5f399abe5b sys/kern/subr_autoconf.c
--- a/sys/kern/subr_autoconf.c  Mon May 22 14:07:37 2023 +0000
+++ b/sys/kern/subr_autoconf.c  Mon May 22 14:58:22 2023 +0000
@@ -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.



Home | Main Index | Thread Index | Old Index