diff options
| author | riastradh <riastradh@NetBSD.org> | 2021-12-31 14:30:04 +0000 |
|---|---|---|
| committer | riastradh <riastradh@NetBSD.org> | 2021-12-31 14:30:04 +0000 |
| commit | 1bf8e094ee54209cabab072a29628c8574f64927 (patch) | |
| tree | eef50dfa74adf2da947a0d3bdc53649ab8ea375a /sys/dev | |
| parent | aa0eba2b8fb0524522739522d8f59ce8b5114c4d (diff) | |
sysmon(9): Fix callout/thread synchronization.
Callout may ONLY take sme_work_mtx, at IPL_SOFTCLOCK; MUST NOT touch
sme_mtx at IPL_NONE. All state the callout needs is serialized by
sme_work_mtx now:
- calls to sme_schedule_callout
- calls to sme_schedule_halt
- struct sysmon_envsys::sme_events_timeout
- struct sysmon_envsys::sme_events_list
- struct sysmon_envsys::sme_callout_state
- struct envsys_data::flags
=> yes, this is a little silly -- used for ENVSYS_FNEED_REFRESH
=> should maybe separate the static driver-defined features from
the state flags needed by sysmon_envsys but not important now
Sleeping under sme_work_mtx (except on other adaptive locks at
IPL_SOFTCLOCK) is forbidden. Calling out to the driver under
sme_work_mtx is forbidden.
This should properly fix:
https://mail-index.netbsd.org/tech-kern/2015/10/14/msg019511.html
PR kern/56592
Diffstat (limited to 'sys/dev')
| -rw-r--r-- | sys/dev/sysmon/sysmon_envsys.c | 42 | ||||
| -rw-r--r-- | sys/dev/sysmon/sysmon_envsys_events.c | 29 | ||||
| -rw-r--r-- | sys/dev/sysmon/sysmonvar.h | 17 |
3 files changed, 54 insertions, 34 deletions
diff --git a/sys/dev/sysmon/sysmon_envsys.c b/sys/dev/sysmon/sysmon_envsys.c index 04cd95b929a..012ad4d531c 100644 --- a/sys/dev/sysmon/sysmon_envsys.c +++ b/sys/dev/sysmon/sysmon_envsys.c @@ -1,4 +1,4 @@ -/* $NetBSD: sysmon_envsys.c,v 1.149 2021/12/31 11:05:41 riastradh Exp $ */ +/* $NetBSD: sysmon_envsys.c,v 1.150 2021/12/31 14:30:04 riastradh Exp $ */ /*- * Copyright (c) 2007, 2008 Juan Romero Pardines. @@ -64,7 +64,7 @@ */ #include <sys/cdefs.h> -__KERNEL_RCSID(0, "$NetBSD: sysmon_envsys.c,v 1.149 2021/12/31 11:05:41 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: sysmon_envsys.c,v 1.150 2021/12/31 14:30:04 riastradh Exp $"); #include <sys/param.h> #include <sys/types.h> @@ -531,7 +531,7 @@ sysmon_envsys_create(void) TAILQ_INIT(&sme->sme_sensors_list); LIST_INIT(&sme->sme_events_list); mutex_init(&sme->sme_mtx, MUTEX_DEFAULT, IPL_NONE); - mutex_init(&sme->sme_work_mtx, MUTEX_DEFAULT, IPL_NONE); + mutex_init(&sme->sme_work_mtx, MUTEX_DEFAULT, IPL_SOFTCLOCK); cv_init(&sme->sme_condvar, "sme_wait"); return sme; @@ -655,11 +655,13 @@ sysmon_envsys_sensor_detach(struct sysmon_envsys *sme, envsys_data_t *edata) if (oedata->flags & ENVSYS_FHAS_ENTROPY) rnd_detach_source(&oedata->rnd_src); sme_event_unregister_sensor(sme, edata); + mutex_enter(&sme->sme_work_mtx); if (LIST_EMPTY(&sme->sme_events_list)) { if (sme->sme_callout_state == SME_CALLOUT_READY) sme_events_halt_callout(sme); destroy = true; } + mutex_exit(&sme->sme_work_mtx); TAILQ_REMOVE(&sme->sme_sensors_list, edata, sensors_head); sme->sme_nsensors--; sysmon_envsys_release(sme, true); @@ -1324,18 +1326,12 @@ sme_remove_userprops(void) /* * Restore default timeout value. */ + mutex_enter(&sme->sme_work_mtx); sme->sme_events_timeout = SME_EVENTS_DEFTIMEOUT; - - /* - * Note that we need to hold the sme_mtx while calling - * sme_schedule_callout(). Thus to avoid dropping, - * reacquiring, and dropping it again, we just tell - * sme_envsys_release() that the mutex is already owned. - */ - mutex_enter(&sme->sme_mtx); sme_schedule_callout(sme); - sysmon_envsys_release(sme, true); - mutex_exit(&sme->sme_mtx); + mutex_exit(&sme->sme_work_mtx); + + sysmon_envsys_release(sme, false); } mutex_exit(&sme_global_mtx); } @@ -1350,6 +1346,7 @@ sme_add_property_dictionary(struct sysmon_envsys *sme, prop_array_t array, prop_dictionary_t dict) { prop_dictionary_t pdict; + uint64_t timo; const char *class; int error = 0; @@ -1374,15 +1371,15 @@ sme_add_property_dictionary(struct sysmon_envsys *sme, prop_array_t array, * ... * */ + mutex_enter(&sme->sme_work_mtx); if (sme->sme_events_timeout == 0) { sme->sme_events_timeout = SME_EVENTS_DEFTIMEOUT; - mutex_enter(&sme->sme_mtx); sme_schedule_callout(sme); - mutex_exit(&sme->sme_mtx); } + timo = sme->sme_events_timeout; + mutex_exit(&sme->sme_work_mtx); - if (!prop_dictionary_set_uint64(pdict, "refresh-timeout", - sme->sme_events_timeout)) { + if (!prop_dictionary_set_uint64(pdict, "refresh-timeout", timo)) { error = EINVAL; goto out; } @@ -1606,6 +1603,7 @@ sme_update_dictionary(struct sysmon_envsys *sme) { envsys_data_t *edata; prop_object_t array, dict, obj, obj2; + uint64_t timo; int error = 0; /* @@ -1634,8 +1632,10 @@ sme_update_dictionary(struct sysmon_envsys *sme) /* * Update the 'refresh-timeout' property. */ - if (!prop_dictionary_set_uint64(obj2, "refresh-timeout", - sme->sme_events_timeout)) + mutex_enter(&sme->sme_work_mtx); + timo = sme->sme_events_timeout; + mutex_exit(&sme->sme_work_mtx); + if (!prop_dictionary_set_uint64(obj2, "refresh-timeout", timo)) return EINVAL; /* @@ -1852,12 +1852,12 @@ sme_userset_dictionary(struct sysmon_envsys *sme, prop_dictionary_t udict, if (refresh_timo < 1) error = EINVAL; else { - mutex_enter(&sme->sme_mtx); + mutex_enter(&sme->sme_work_mtx); if (sme->sme_events_timeout != refresh_timo) { sme->sme_events_timeout = refresh_timo; sme_schedule_callout(sme); } - mutex_exit(&sme->sme_mtx); + mutex_exit(&sme->sme_work_mtx); } } return error; diff --git a/sys/dev/sysmon/sysmon_envsys_events.c b/sys/dev/sysmon/sysmon_envsys_events.c index 1f1917ba633..ec6a691319e 100644 --- a/sys/dev/sysmon/sysmon_envsys_events.c +++ b/sys/dev/sysmon/sysmon_envsys_events.c @@ -1,4 +1,4 @@ -/* $NetBSD: sysmon_envsys_events.c,v 1.122 2021/12/31 11:05:41 riastradh Exp $ */ +/* $NetBSD: sysmon_envsys_events.c,v 1.123 2021/12/31 14:30:04 riastradh Exp $ */ /*- * Copyright (c) 2007, 2008 Juan Romero Pardines. @@ -30,7 +30,7 @@ */ #include <sys/cdefs.h> -__KERNEL_RCSID(0, "$NetBSD: sysmon_envsys_events.c,v 1.122 2021/12/31 11:05:41 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: sysmon_envsys_events.c,v 1.123 2021/12/31 14:30:04 riastradh Exp $"); #include <sys/param.h> #include <sys/types.h> @@ -325,8 +325,11 @@ sme_event_register(prop_dictionary_t sdict, envsys_data_t *edata, &(edata->upropset)); out: - if ((error == 0 || error == EEXIST) && osee == NULL) + if ((error == 0 || error == EEXIST) && osee == NULL) { + mutex_enter(&sme->sme_work_mtx); LIST_INSERT_HEAD(&sme->sme_events_list, see, see_list); + mutex_exit(&sme->sme_work_mtx); + } mutex_exit(&sme->sme_mtx); @@ -373,11 +376,13 @@ sme_event_unregister_all(struct sysmon_envsys *sme) } } + mutex_enter(&sme->sme_work_mtx); if (LIST_EMPTY(&sme->sme_events_list) && sme->sme_callout_state == SME_CALLOUT_READY) { sme_events_halt_callout(sme); destroy = true; } + mutex_exit(&sme->sme_work_mtx); mutex_exit(&sme->sme_mtx); if (destroy) @@ -425,10 +430,12 @@ sme_event_unregister(struct sysmon_envsys *sme, const char *sensor, int type) sme_remove_event(see, sme); + mutex_enter(&sme->sme_work_mtx); if (LIST_EMPTY(&sme->sme_events_list)) { sme_events_halt_callout(sme); destroy = true; } + mutex_exit(&sme->sme_work_mtx); mutex_exit(&sme->sme_mtx); if (destroy) @@ -480,7 +487,10 @@ sme_remove_event(sme_event_t *see, struct sysmon_envsys *sme) KASSERT(mutex_owned(&sme->sme_mtx)); + mutex_enter(&sme->sme_work_mtx); LIST_REMOVE(see, see_list); + mutex_exit(&sme->sme_work_mtx); + kmem_free(see, sizeof(*see)); } @@ -570,8 +580,12 @@ sme_events_init(struct sysmon_envsys *sme) callout_init(&sme->sme_callout, CALLOUT_MPSAFE); callout_setfunc(&sme->sme_callout, sme_events_check, sme); + + mutex_enter(&sme->sme_work_mtx); sme->sme_callout_state = SME_CALLOUT_READY; sme_schedule_callout(sme); + mutex_exit(&sme->sme_work_mtx); + DPRINTF(("%s: events framework initialized for '%s'\n", __func__, sme->sme_name)); @@ -589,7 +603,7 @@ sme_schedule_callout(struct sysmon_envsys *sme) uint64_t timo; KASSERT(sme != NULL); - KASSERT(mutex_owned(&sme->sme_mtx)); + KASSERT(mutex_owned(&sme->sme_work_mtx)); if (sme->sme_callout_state != SME_CALLOUT_READY) return; @@ -599,7 +613,6 @@ sme_schedule_callout(struct sysmon_envsys *sme) else timo = SME_EVTIMO; - callout_stop(&sme->sme_callout); callout_schedule(&sme->sme_callout, timo); } @@ -743,7 +756,6 @@ sme_events_check(void *arg) mutex_exit(&sme->sme_work_mtx); return; } - mutex_enter(&sme->sme_mtx); LIST_FOREACH(see, &sme->sme_events_list, see_list) { workqueue_enqueue(sme->sme_wq, &see->see_wk, NULL); see->see_edata->flags |= ENVSYS_FNEED_REFRESH; @@ -751,7 +763,6 @@ sme_events_check(void *arg) } if (!sysmon_low_power) sme_schedule_callout(sme); - mutex_exit(&sme->sme_mtx); mutex_exit(&sme->sme_work_mtx); } @@ -779,11 +790,15 @@ sme_events_worker(struct work *wk, void *arg) * sme_envsys_refresh_sensor will not call the driver if the driver * does its own setting of the sensor value. */ + mutex_enter(&sme->sme_work_mtx); if ((edata->flags & ENVSYS_FNEED_REFRESH) != 0) { /* refresh sensor in device */ + mutex_exit(&sme->sme_work_mtx); sysmon_envsys_refresh_sensor(sme, edata); + mutex_enter(&sme->sme_work_mtx); edata->flags &= ~ENVSYS_FNEED_REFRESH; } + mutex_exit(&sme->sme_work_mtx); DPRINTFOBJ(("%s: (%s) desc=%s sensor=%d type=%d state=%d units=%d " "value_cur=%d upropset=0x%04x\n", __func__, sme->sme_name, edata->desc, diff --git a/sys/dev/sysmon/sysmonvar.h b/sys/dev/sysmon/sysmonvar.h index 8a60b7fff8b..69af7673de4 100644 --- a/sys/dev/sysmon/sysmonvar.h +++ b/sys/dev/sysmon/sysmonvar.h @@ -1,4 +1,4 @@ -/* $NetBSD: sysmonvar.h,v 1.51 2021/12/31 11:05:41 riastradh Exp $ */ +/* $NetBSD: sysmonvar.h,v 1.52 2021/12/31 14:30:04 riastradh Exp $ */ /*- * Copyright (c) 2000 Zembu Labs, Inc. @@ -185,14 +185,17 @@ struct sysmon_envsys { sysmon_envsys_lim_t *, uint32_t *); struct workqueue *sme_wq; /* the workqueue for the events */ - struct callout sme_callout; /* for the events */ - int sme_callout_state; /* state of the event's callout */ + struct callout sme_callout; /* for the events + * sme_work_mtx to schedule or halt */ + int sme_callout_state; /* state of the event's callout + * sme_work_mtx to read or write */ #define SME_CALLOUT_INVALID 0x0 /* callout is not initialized */ #define SME_CALLOUT_READY 0x1 /* callout is ready for use */ #define SME_CALLOUT_HALTED 0x2 /* callout can be destroyed */ - uint64_t sme_events_timeout; /* the timeout used in the callout */ + uint64_t sme_events_timeout; /* the timeout used in the callout + * sme_work_mtx to read or write */ /* * linked list for the sysmon envsys devices. @@ -201,11 +204,14 @@ struct sysmon_envsys { /* * linked list for the events that a device maintains. + * - sme_work_mtx OR sme_mtx to read + * - sme_work_mtx AND sme_mtx to write */ LIST_HEAD(, sme_event) sme_events_list; /* * tailq for the sensors that a device maintains. + * - sme_mtx to read or write */ TAILQ_HEAD(, envsys_data) sme_sensors_list; @@ -213,8 +219,7 @@ struct sysmon_envsys { * Locking/synchronization. */ int sme_busy; /* number of items on workqueue, - sme_mtx or sme_work_mtx to read, - both to write */ + * sme_work_mtx to read or write */ kmutex_t sme_mtx; kmutex_t sme_work_mtx; kcondvar_t sme_condvar; |
