Date: Tue, 11 Nov 2008 16:10:06 GMT From: Jaakko Heinonen <jh@saunalahti.fi> To: freebsd-bugs@FreeBSD.org Subject: Re: kern/104079: [fdc] kldunload fdc.ko leads to panic: mutex Giant owned Message-ID: <200811111610.mABGA6Xb093750@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/104079; it has been noted by GNATS.
From: Jaakko Heinonen <jh@saunalahti.fi>
To: bug-followup@FreeBSD.org, lynx.ripe@gmail.com
Cc:
Subject: Re: kern/104079: [fdc] kldunload fdc.ko leads to panic: mutex
Giant owned
Date: Tue, 11 Nov 2008 18:08:30 +0200
On module unload fd_detach() is called with Giant held. The problem is
that fd_detach() tries to acquire the GEOM topology lock (sx lock).
This is not allowed under Giant.
There are also other bugs related to loading/unloading the fdc module:
* Return value from g_modevent() is ignored. fdc_modevent() always
returns success even if g_modevent() fails.
* g_wither_geom() races against module unload. Withering is not
guaranteed to finish in finite time. If withering doesn't finish
before g_modevent() call the module unload fails. Combined with
g_modevent() return value ignorance the result is a panic in g_event
thread.
(Looks like g_wither_geom() race against module unload is a common
problem with many geom classes. atapicd is affected for sure.)
* When loading the module fdc_attach() must be completed before
fd_attach(). However fd_attach() runs before fdc_attach().
Following patch works around some of the problems. We can do withering
as geom event to avoid calling g_wither_geom() under Giant. The makefile
hack is to change the order of fd_attach() and fdc_attach(). If someone
knows better way to change ordering I am happy to hear.
(DRIVER_MODULE() macro doesn't take order (SI_ORDER_*) as argument.)
The patch doesn't address g_wither_geom() race (except checking
g_modevent() return value may prevent a panic in some cases).
%%%
Index: sys/modules/fdc/Makefile
===================================================================
--- sys/modules/fdc/Makefile (revision 184512)
+++ sys/modules/fdc/Makefile (working copy)
@@ -7,11 +7,12 @@ KMOD= fdc
SRCS= fdc.c fdc_cbus.c
.else
.PATH: ${.CURDIR}/../../dev/fdc
-SRCS= fdc.c fdc_isa.c fdc_pccard.c
+SRCS= fdc_isa.c fdc_pccard.c
.if ${MACHINE} == "i386" || ${MACHINE} == "amd64"
CFLAGS+= -I${.CURDIR}/../../contrib/dev/acpica
SRCS+= opt_acpi.h acpi_if.h fdc_acpi.c
.endif
+SRCS+= fdc.c
.endif
SRCS+= opt_fdc.h bus_if.h card_if.h device_if.h \
Index: sys/dev/fdc/fdc.c
===================================================================
--- sys/dev/fdc/fdc.c (revision 184512)
+++ sys/dev/fdc/fdc.c (working copy)
@@ -2012,15 +2012,22 @@ fd_attach(device_t dev)
return (0);
}
+static void
+fd_detach_geom(void *arg, int flag)
+{
+ struct fd_data *fd = arg;
+
+ g_topology_assert();
+ g_wither_geom(fd->fd_geom, ENXIO);
+}
+
static int
fd_detach(device_t dev)
{
struct fd_data *fd;
fd = device_get_softc(dev);
- g_topology_lock();
- g_wither_geom(fd->fd_geom, ENXIO);
- g_topology_unlock();
+ g_waitfor_event(fd_detach_geom, fd, M_WAITOK, NULL);
while (device_get_state(dev) == DS_BUSY)
tsleep(fd, PZERO, "fdd", hz/10);
callout_drain(&fd->toffhandle);
@@ -2049,8 +2056,7 @@ static int
fdc_modevent(module_t mod, int type, void *data)
{
- g_modevent(NULL, type, &g_fd_class);
- return (0);
+ return (g_modevent(NULL, type, &g_fd_class));
}
DRIVER_MODULE(fd, fdc, fd_driver, fd_devclass, fdc_modevent, 0);
%%%
--
Jaakko
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200811111610.mABGA6Xb093750>