From owner-freebsd-bugs@FreeBSD.ORG Tue Nov 11 16:10:06 2008 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 9F6D2106564A for ; Tue, 11 Nov 2008 16:10:06 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 959818FC1E for ; Tue, 11 Nov 2008 16:10:06 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.14.3/8.14.3) with ESMTP id mABGA62T093751 for ; Tue, 11 Nov 2008 16:10:06 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.3/8.14.3/Submit) id mABGA6Xb093750; Tue, 11 Nov 2008 16:10:06 GMT (envelope-from gnats) Date: Tue, 11 Nov 2008 16:10:06 GMT Message-Id: <200811111610.mABGA6Xb093750@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: Jaakko Heinonen Cc: Subject: Re: kern/104079: [fdc] kldunload fdc.ko leads to panic: mutex Giant owned X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Jaakko Heinonen List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 Nov 2008 16:10:06 -0000 The following reply was made to PR kern/104079; it has been noted by GNATS. From: Jaakko Heinonen 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