On 2/21/26 21:00, Enji Cooper wrote:
The branch main has been updated by ngie:
URL:
https://cgit.FreeBSD.org/src/commit/?id=eda36ae09dd1fab78bd377739fc5d6c65c61f5d7
commit eda36ae09dd1fab78bd377739fc5d6c65c61f5d7
Author: Enji Cooper <[email protected]>
AuthorDate: 2026-01-30 06:55:08 +0000
Commit: Enji Cooper <[email protected]>
CommitDate: 2026-02-22 01:59:06 +0000
asmc: resource cleanup simplifications
This change makes `asmc_detach(..)` reentrant by setting freed resources
to known invalid values when done, and makes `asmc_attach(..)` call
`asmc_detach(..)` instead of attempting to the semi-equivalent way of
cleaning up the driver resources allocated in `asmc_detach(..)`.
MFC after: 1 week
Differential Revision: https://reviews.freebsd.org/D55413
---
sys/dev/asmc/asmc.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/sys/dev/asmc/asmc.c b/sys/dev/asmc/asmc.c
index bff214c084c5..084b57331dd9 100644
--- a/sys/dev/asmc/asmc.c
+++ b/sys/dev/asmc/asmc.c
@@ -795,25 +795,21 @@ asmc_attach(device_t dev)
if (sc->sc_irq == NULL) {
device_printf(dev, "unable to allocate IRQ resource\n");
ret = ENXIO;
- goto err2;
+ goto err;
}
ret = bus_setup_intr(dev, sc->sc_irq, INTR_TYPE_MISC | INTR_MPSAFE,
asmc_sms_intrfast, NULL, dev, &sc->sc_cookie);
if (ret) {
device_printf(dev, "unable to setup SMS IRQ\n");
- goto err1;
+ goto err;
}
+
nosms:
return (0);
-err1:
- bus_release_resource(dev, SYS_RES_IRQ, sc->sc_rid_irq, sc->sc_irq);
-err2:
- bus_release_resource(dev, SYS_RES_IOPORT, sc->sc_rid_port,
- sc->sc_ioport);
- mtx_destroy(&sc->sc_mtx);
- if (sc->sc_sms_tq)
- taskqueue_free(sc->sc_sms_tq);
+
+err:
+ asmc_detach(dev);
This cleanup is good.
return (ret);
}
@@ -826,16 +822,25 @@ asmc_detach(device_t dev)
if (sc->sc_sms_tq) {
taskqueue_drain(sc->sc_sms_tq, &sc->sc_sms_task);
taskqueue_free(sc->sc_sms_tq);
+ sc->sc_sms_tq = NULL;
Does this actually matter? That is, I don't think there is any reason for
asmc_detach to be reentrant. New-bus (specifically device_attach()) does not
invoke DEVICE_DETACH if DEVICE_ATTACH fails. Instead, DEVICE_ATTACH routines
are expected to fully cleanup on failure. The changes to be reentrant here
don't hurt, but they generally just add clutter.
}
- if (sc->sc_cookie)
+ if (sc->sc_cookie) {
bus_teardown_intr(dev, sc->sc_irq, sc->sc_cookie);
- if (sc->sc_irq)
+ sc->sc_cookie = NULL;
+ }
+ if (sc->sc_irq) {
bus_release_resource(dev, SYS_RES_IRQ, sc->sc_rid_irq,
sc->sc_irq);
- if (sc->sc_ioport)
+ sc->sc_irq = NULL;
+ }
+ if (sc->sc_ioport) {
bus_release_resource(dev, SYS_RES_IOPORT, sc->sc_rid_port,
sc->sc_ioport);
- mtx_destroy(&sc->sc_mtx);
+ sc->sc_ioport = NULL;
+ }
+ if (mtx_initialized(&sc->sc_mtx)) {
Please do not use this. Code should know if the mutex is initialized or not.
That is, always initalize the mutex in attach before any failures that could
enter this path.
+ mtx_destroy(&sc->sc_mtx);
+ }
--
John Baldwin
