On Thu, Dec 10, 2009 at 01:50:28AM +0000, Mindaugas Rasiukevicius wrote:
> Hello,
>
> > Module Name: src
> > Committed By: dyoung
> > Date: Thu Nov 12 19:10:31 UTC 2009
> >
> > Modified Files:
> > src/sys/kern: subr_autoconf.c
> > src/sys/sys: device.h
> >
> > Log Message:
> > Move a device-deactivation pattern that is replicated throughout
> > the system into config_deactivate(dev): deactivate dev and all of
> > its descendants. Block all interrupts while calling each device's
> > activation hook, ca_activate. Now it is possible to simplify or
> > to delete several device-activation hooks throughout the system.
> >
> > ...
> >
> > To generate a diff of this commit:
> > cvs rdiff -u -r1.186 -r1.187 src/sys/kern/subr_autoconf.c
> > cvs rdiff -u -r1.124 -r1.125 src/sys/sys/device.h
>
> - Can you tell what relevant code requires alldevs_mtx to be at IPL_HIGH?
>
> - Since alldevs_mtx became a spin-lock, it is no longer valid to assert for
> mutex _not_ being held. In other words, such cases are wrong:
>
> KASSERT(!mutex_owned(&alldevs_mtx));
Ah! Thanks.
> - You have added config_collect_garbage(), which is mostly called before
> config_alldevs_lock(). How about changing it to be used as/with last
> unlock? That is, collect the objects into a list, release the lock and
> then destroy all objects. Apart from avoiding unecessary unlock/relock
> dances, it would also be simpler.
Thank you for the suggestion. To make the change is easy. I have
attached a patch.
> - May I suggest to avoid "inverted" logic like this:
>
> if (rv != 0)
> ;
> else if (dev->dv_del_gen != 0)
> ;
> else {
> dev->dv_del_gen = alldevs_gen;
> alldevs_garbage = true;
> }
Thanks. I will change this to the short form.
Dave
--
David Young OJC Technologies
dyo...@ojctech.com Urbana, IL * (217) 278-3933
Index: sys/sys/device.h
===================================================================
RCS file: /cvsroot/src/sys/sys/device.h,v
retrieving revision 1.127
diff -u -p -u -p -r1.127 device.h
--- sys/sys/device.h 7 Dec 2009 19:45:13 -0000 1.127
+++ sys/sys/device.h 10 Dec 2009 18:17:54 -0000
@@ -181,6 +181,10 @@ struct device {
device_suspensor_t dv_bus_suspensors[DEVICE_SUSPENSORS_MAX];
device_suspensor_t dv_driver_suspensors[DEVICE_SUSPENSORS_MAX];
device_suspensor_t dv_class_suspensors[DEVICE_SUSPENSORS_MAX];
+ struct device_garbage {
+ device_t *dg_devs;
+ int dg_ndevs;
+ } dv_garbage;
};
/* dv_flags */
Index: sys/kern/subr_autoconf.c
===================================================================
RCS file: /cvsroot/src/sys/kern/subr_autoconf.c,v
retrieving revision 1.188
diff -u -p -u -p -r1.188 subr_autoconf.c
--- sys/kern/subr_autoconf.c 12 Nov 2009 23:16:28 -0000 1.188
+++ sys/kern/subr_autoconf.c 10 Dec 2009 18:17:54 -0000
@@ -166,12 +166,14 @@ static char *number(char *, int);
static void mapply(struct matchinfo *, cfdata_t);
static device_t config_devalloc(const device_t, const cfdata_t, const int *);
static void config_devdelete(device_t);
+static void config_devunlink(device_t, struct devicelist *);
static void config_makeroom(int, struct cfdriver *);
static void config_devlink(device_t);
static void config_alldevs_unlock(int);
static int config_alldevs_lock(void);
-static void config_collect_garbage(void);
+static void config_collect_garbage(struct devicelist *);
+static void config_dump_garbage(struct devicelist *);
static void pmflock_debug(device_t, const char *, int);
@@ -368,10 +370,11 @@ config_cfdriver_attach(struct cfdriver *
int
config_cfdriver_detach(struct cfdriver *cd)
{
+ struct devicelist garbage = TAILQ_HEAD_INITIALIZER(garbage);
int i, rc = 0, s;
s = config_alldevs_lock();
- config_collect_garbage();
+ config_collect_garbage(&garbage);
/* Make sure there are no active instances. */
for (i = 0; i < cd->cd_ndevs; i++) {
if (cd->cd_devs[i] != NULL) {
@@ -380,6 +383,7 @@ config_cfdriver_detach(struct cfdriver *
}
}
config_alldevs_unlock(s);
+ config_dump_garbage(&garbage);
if (rc != 0)
return rc;
@@ -441,6 +445,7 @@ config_cfattach_attach(const char *drive
int
config_cfattach_detach(const char *driver, struct cfattach *ca)
{
+ struct devicelist garbage = TAILQ_HEAD_INITIALIZER(garbage);
struct cfdriver *cd;
device_t dev;
int i, rc = 0, s;
@@ -450,7 +455,7 @@ config_cfattach_detach(const char *drive
return ESRCH;
s = config_alldevs_lock();
- config_collect_garbage();
+ config_collect_garbage(&garbage);
/* Make sure there are no active instances. */
for (i = 0; i < cd->cd_ndevs; i++) {
if ((dev = cd->cd_devs[i]) == NULL)
@@ -461,6 +466,7 @@ config_cfattach_detach(const char *drive
}
}
config_alldevs_unlock(s);
+ config_dump_garbage(&garbage);
if (rc != 0)
return rc;
@@ -1007,49 +1013,49 @@ config_devlink(device_t dev)
}
/*
- * Caller must hold alldevs_mtx. config_devdelete() may release and
- * re-acquire alldevs_mtx, so callers should re-check conditions such as
- * alldevs_nwrite == 0 && alldevs_nread == 0 after config_devdelete()
- * returns.
+ * Caller must hold alldevs_mtx.
*/
static void
-config_devdelete(device_t dev)
+config_devunlink(device_t dev, struct devicelist *garbage)
{
- device_lock_t dvl = device_getlock(dev);
- int priv = (dev->dv_flags & DVF_PRIV_ALLOC);
+ struct device_garbage *dg = &dev->dv_garbage;
struct cfdriver *cd = dev->dv_cfdriver;
- device_t *devs = NULL;
- int i, ndevs = 0;
+ int i;
KASSERT(mutex_owned(&alldevs_mtx));
- /* Unlink from device list. */
+ /* Unlink from device list. Link to garbage list. */
TAILQ_REMOVE(&alldevs, dev, dv_list);
+ TAILQ_INSERT_TAIL(garbage, dev, dv_list);
/* Remove from cfdriver's array. */
cd->cd_devs[dev->dv_unit] = NULL;
/*
- * If the device now has no units in use, deallocate its softc array.
+ * If the device now has no units in use, unlink its softc array.
*/
for (i = 0; i < cd->cd_ndevs; i++) {
if (cd->cd_devs[i] != NULL)
break;
}
- /* nothing found. unlink, now. deallocate below. */
+ /* Nothing found. Unlink, now. Deallocate, later. */
if (i == cd->cd_ndevs) {
- ndevs = cd->cd_ndevs;
- devs = cd->cd_devs;
+ dg->dg_ndevs = cd->cd_ndevs;
+ dg->dg_devs = cd->cd_devs;
cd->cd_devs = NULL;
cd->cd_ndevs = 0;
}
+}
- mutex_exit(&alldevs_mtx);
-
- KASSERT(!mutex_owned(&alldevs_mtx));
+static void
+config_devdelete(device_t dev)
+{
+ struct device_garbage *dg = &dev->dv_garbage;
+ int priv = (dev->dv_flags & DVF_PRIV_ALLOC);
+ device_lock_t dvl = device_getlock(dev);
- if (devs != NULL)
- kmem_free(devs, sizeof(device_t [ndevs]));
+ if (dg->dg_devs != NULL)
+ kmem_free(dg->dg_devs, sizeof(device_t[dg->dg_ndevs]));
cv_destroy(&dvl->dvl_cv);
mutex_destroy(&dvl->dvl_mtx);
@@ -1069,15 +1075,12 @@ config_devdelete(device_t dev)
kmem_free(dev->dv_private, dev->dv_cfattach->ca_devsize);
if (priv)
kmem_free(dev, sizeof(*dev));
-
- KASSERT(!mutex_owned(&alldevs_mtx));
-
- mutex_enter(&alldevs_mtx);
}
static device_t
config_devalloc(const device_t parent, const cfdata_t cf, const int *locs)
{
+ struct devicelist garbage = TAILQ_HEAD_INITIALIZER(garbage);
struct cfdriver *cd;
struct cfattach *ca;
size_t lname, lunit;
@@ -1103,7 +1106,7 @@ config_devalloc(const device_t parent, c
#ifndef __BROKEN_CONFIG_UNIT_USAGE
s = config_alldevs_lock();
- config_collect_garbage();
+ config_collect_garbage(&garbage);
if (cf->cf_fstate == FSTATE_STAR) {
for (myunit = cf->cf_unit; myunit < cd->cd_ndevs; myunit++)
if (cd->cd_devs[myunit] == NULL)
@@ -1118,6 +1121,7 @@ config_devalloc(const device_t parent, c
myunit = -1;
}
config_alldevs_unlock(s);
+ config_dump_garbage(&garbage);
if (myunit == -1)
return NULL;
#else
@@ -1334,13 +1338,10 @@ config_attach_pseudo(cfdata_t cf)
}
/*
- * Caller must hold alldevs_mtx. config_collect_garbage() may
- * release and re-acquire alldevs_mtx, so callers should re-check
- * conditions such as alldevs_nwrite == 0 && alldevs_nread == 0 after
- * config_collect_garbage() returns.
+ * Caller must hold alldevs_mtx.
*/
static void
-config_collect_garbage(void)
+config_collect_garbage(struct devicelist *garbage)
{
device_t dv;
@@ -1355,11 +1356,22 @@ config_collect_garbage(void)
alldevs_garbage = false;
break;
}
- config_devdelete(dv);
+ config_devunlink(dv, garbage);
}
KASSERT(mutex_owned(&alldevs_mtx));
}
+static void
+config_dump_garbage(struct devicelist *garbage)
+{
+ device_t dv;
+
+ while ((dv = TAILQ_FIRST(garbage)) != NULL) {
+ TAILQ_REMOVE(garbage, dv, dv_list);
+ config_devdelete(dv);
+ }
+}
+
/*
* Detach a device. Optionally forced (e.g. because of hardware
* removal) and quiet. Returns zero if successful, non-zero
@@ -1372,6 +1384,7 @@ config_collect_garbage(void)
int
config_detach(device_t dev, int flags)
{
+ struct devicelist garbage = TAILQ_HEAD_INITIALIZER(garbage);
struct cftable *ct;
cfdata_t cf;
const struct cfattach *ca;
@@ -1500,8 +1513,9 @@ out:
dev->dv_del_gen = alldevs_gen;
alldevs_garbage = true;
}
- config_collect_garbage();
+ config_collect_garbage(&garbage);
config_alldevs_unlock(s);
+ config_dump_garbage(&garbage);
return rv;
}
@@ -1864,19 +1878,20 @@ config_alldevs_unlock(int s)
device_t
device_lookup(cfdriver_t cd, int unit)
{
+ struct devicelist garbage = TAILQ_HEAD_INITIALIZER(garbage);
device_t dv;
int s;
s = config_alldevs_lock();
- config_collect_garbage();
+ config_collect_garbage(&garbage);
KASSERT(mutex_owned(&alldevs_mtx));
if (unit < 0 || unit >= cd->cd_ndevs)
dv = NULL;
else
dv = cd->cd_devs[unit];
config_alldevs_unlock(s);
+ config_dump_garbage(&garbage);
- KASSERT(!mutex_owned(&alldevs_mtx));
return dv;
}
@@ -1888,12 +1903,13 @@ device_lookup(cfdriver_t cd, int unit)
void *
device_lookup_private(cfdriver_t cd, int unit)
{
+ struct devicelist garbage = TAILQ_HEAD_INITIALIZER(garbage);
device_t dv;
int s;
void *priv;
s = config_alldevs_lock();
- config_collect_garbage();
+ config_collect_garbage(&garbage);
KASSERT(mutex_owned(&alldevs_mtx));
if (unit < 0 || unit >= cd->cd_ndevs)
priv = NULL;
@@ -1902,8 +1918,8 @@ device_lookup_private(cfdriver_t cd, int
else
priv = dv->dv_private;
config_alldevs_unlock(s);
+ config_dump_garbage(&garbage);
- KASSERT(!mutex_owned(&alldevs_mtx));
return priv;
}
