From owner-freebsd-hackers@FreeBSD.ORG Sun Oct 18 13:17:15 2009 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 855DB106566C; Sun, 18 Oct 2009 13:17:15 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (skuns.zoral.com.ua [91.193.166.194]) by mx1.freebsd.org (Postfix) with ESMTP id 820628FC08; Sun, 18 Oct 2009 13:17:14 +0000 (UTC) Received: from deviant.kiev.zoral.com.ua (root@deviant.kiev.zoral.com.ua [10.1.1.148]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id n9IDGxKG054567 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 18 Oct 2009 16:16:59 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.3/8.14.3) with ESMTP id n9IDGxSX025562; Sun, 18 Oct 2009 16:16:59 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.3/8.14.3/Submit) id n9IDGu1q025561; Sun, 18 Oct 2009 16:16:56 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Sun, 18 Oct 2009 16:16:56 +0300 From: Kostik Belousov To: Andrew Gallatin Message-ID: <20091018131656.GP2160@deviant.kiev.zoral.com.ua> References: <4AD79126.8020104@cs.duke.edu> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="zROEGoKAXsG5UqGB" Content-Disposition: inline In-Reply-To: <4AD79126.8020104@cs.duke.edu> User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-4.4 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua Cc: freebsd-hackers@freebsd.org, sam@freebsd.org Subject: Re: namei (via firmware_get(9)) from taskq in 7.x X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 18 Oct 2009 13:17:15 -0000 --zROEGoKAXsG5UqGB Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Oct 15, 2009 at 05:16:22PM -0400, Andrew Gallatin wrote: > Hi, >=20 > I'm trying to re-initialize a NIC which uses firmware(9) > after a hardware fault. As part of the process, I need > to re-load the firmware using firmware_get(). If the > firmware kld is not resident, then the machine will panic > like this: >=20 > Fatal trap 12: page fault while in kernel mode > cpuid =3D 0; apic id =3D 00 > fault virtual address =3D 0x20 > fault code =3D supervisor read data, page not present > instruction pointer =3D 0x8:0xffffffff805b05d4 > stack pointer =3D 0x10:0xffffff8000080460 > frame pointer =3D 0x10:0xffffff8000080510 > code segment =3D base 0x0, limit 0xfffff, type 0x1b > =3D DPL 0, pres 1, long 1, def32 0, gran 1 > processor eflags =3D interrupt enabled, resume, IOPL =3D 0 > current process =3D 21 (swi5: +) > [thread pid 21 tid 100021 ] > Stopped at namei+0x174: movq 0x20(%rbx),%rax > db> bt > Tracing pid 21 tid 100021 td 0xffffff00013c3ae0 > namei() at namei+0x174 > vn_open_cred() at vn_open_cred+0x3a4 > linker_load_module() at linker_load_module+0x1f2 > linker_reference_module() at linker_reference_module+0xae > firmware_get() at firmware_get+0x136 > mxge_load_firmware() at mxge_load_firmware+0x2d > mxge_watchdog_task() at mxge_watchdog_task+0x2f6 > taskqueue_run() at taskqueue_run+0x9d > ithread_loop() at ithread_loop+0x17d > fork_exit() at fork_exit+0x11f > fork_trampoline() at fork_trampoline+0xe >=20 > Looking at it in gdb, it seems like the problem is that namei > is trying to use ndp->ni_cnd.cn_thread->td_proc->p_fd->fd_cdir > which is null in this context. >=20 > Can somebody tell me what kernel context it is safe to > call firmware_get() (and hence namei) from? Is there > a safe way to do it from a taskq? >=20 > FWIW, this seems to work fine (even from a callout context) > in 8 and higher. It is only 7 and earlier where I'm having > this problem. It seems that you want a merge of r178042,183614,184842,188057 (one of which is yours). Property changes on: . ___________________________________________________________________ Modified: svn:mergeinfo Merged /head/sys:r178042,183614,184842,188057 Index: kern/subr_firmware.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- kern/subr_firmware.c (revision 198202) +++ kern/subr_firmware.c (working copy) @@ -1,5 +1,5 @@ /*- - * Copyright (c) 2005, Sam Leffler + * Copyright (c) 2005-2008, Sam Leffler * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -41,7 +41,11 @@ #include #include #include +#include =20 +#include +#include + /* * Loadable firmware support. See sys/sys/firmware.h and firmware(9) * form more details on the subsystem. @@ -89,7 +93,7 @@ /* * 'file' is private info managed by the autoload/unload code. * Set at the end of firmware_get(), cleared only in the - * firmware_task, so the latter can depend on its value even + * firmware_unload_task, so the latter can depend on its value even * while the lock is not held. */ linker_file_t file; /* module file, if autoloaded */ @@ -121,14 +125,16 @@ static struct priv_fw firmware_table[FIRMWARE_MAX]; =20 /* - * module release are handled in a separate task as they might sleep. + * Firmware module operations are handled in a separate task as they + * might sleep and they require directory context to do i/o. */ -struct task firmware_task; +static struct taskqueue *firmware_tq; +static struct task firmware_unload_task; =20 /* * This mutex protects accesses to the firmware table. */ -struct mtx firmware_mtx; +static struct mtx firmware_mtx; MTX_SYSINIT(firmware, &firmware_mtx, "firmware table", MTX_DEF); =20 /* @@ -227,7 +233,7 @@ } else if (fp->refcnt !=3D 0) { /* cannot unregister */ err =3D EBUSY; } else { - linker_file_t x =3D fp->file; /* save value */ + linker_file_t x =3D fp->file; /* save value */ =20 if (fp->parent !=3D NULL) /* release parent reference */ fp->parent->refcnt--; @@ -244,6 +250,47 @@ return err; } =20 +static void +loadimage(void *arg, int npending) +{ + struct thread *td =3D curthread; + char *imagename =3D arg; + struct priv_fw *fp; + linker_file_t result; + int error; + + /* synchronize with the thread that dispatched us */ + mtx_lock(&firmware_mtx); + mtx_unlock(&firmware_mtx); + + if (td->td_proc->p_fd->fd_rdir =3D=3D NULL) { + printf("%s: root not mounted yet, no way to load image\n", + imagename); + goto done; + } + error =3D linker_reference_module(imagename, NULL, &result); + if (error !=3D 0) { + printf("%s: could not load firmware image, error %d\n", + imagename, error); + goto done; + } + + mtx_lock(&firmware_mtx); + fp =3D lookup(imagename, NULL); + if (fp =3D=3D NULL || fp->file !=3D NULL) { + mtx_unlock(&firmware_mtx); + if (fp =3D=3D NULL) + printf("%s: firmware image loaded, " + "but did not register\n", imagename); + (void) linker_release_module(imagename, NULL, NULL); + goto done; + } + fp->file =3D result; /* record the module identity */ + mtx_unlock(&firmware_mtx); +done: + wakeup_one(imagename); /* we're done */ +} + /* * Lookup and potentially load the specified firmware image. * If the firmware is not found in the registry, try to load a kernel @@ -254,9 +301,9 @@ const struct firmware * firmware_get(const char *imagename) { + struct task fwload_task; struct thread *td; struct priv_fw *fp; - linker_file_t result; =20 mtx_lock(&firmware_mtx); fp =3D lookup(imagename, NULL); @@ -265,29 +312,34 @@ /* * Image not present, try to load the module holding it. */ - mtx_unlock(&firmware_mtx); td =3D curthread; if (priv_check(td, PRIV_FIRMWARE_LOAD) !=3D 0 || securelevel_gt(td->td_ucred, 0) !=3D 0) { + mtx_unlock(&firmware_mtx); printf("%s: insufficient privileges to " "load firmware image %s\n", __func__, imagename); return NULL; } - (void) linker_reference_module(imagename, NULL, &result); + /*=20 + * Defer load to a thread with known context. linker_reference_module + * may do filesystem i/o which requires root & current dirs, etc. + * Also we must not hold any mtx's over this call which is problematic. + */ + if (!cold) { + TASK_INIT(&fwload_task, 0, loadimage, __DECONST(void *, + imagename)); + taskqueue_enqueue(firmware_tq, &fwload_task); + msleep(__DECONST(void *, imagename), &firmware_mtx, 0, + "fwload", 0); + } /* - * After loading the module, see if the image is registered now. + * After attempting to load the module, see if the image is registered. */ - mtx_lock(&firmware_mtx); fp =3D lookup(imagename, NULL); if (fp =3D=3D NULL) { mtx_unlock(&firmware_mtx); - printf("%s: failed to load firmware image %s\n", - __func__, imagename); - (void) linker_release_module(imagename, NULL, NULL); return NULL; } - fp->file =3D result; /* record the module identity */ - found: /* common exit point on success */ fp->refcnt++; mtx_unlock(&firmware_mtx); @@ -300,8 +352,8 @@ * to release the resource, but the flag is only advisory. * * If this is the last reference to the firmware image, and this is an - * autoloaded module, wake up the firmware_task to figure out what to do - * with the associated module. + * autoloaded module, wake up the firmware_unload_task to figure out + * what to do with the associated module. */ void firmware_put(const struct firmware *p, int flags) @@ -314,12 +366,53 @@ if (flags & FIRMWARE_UNLOAD) fp->flags |=3D FW_UNLOAD; if (fp->file) - taskqueue_enqueue(taskqueue_thread, &firmware_task); + taskqueue_enqueue(firmware_tq, &firmware_unload_task); } mtx_unlock(&firmware_mtx); } =20 /* + * Setup directory state for the firmware_tq thread so we can do i/o. + */ +static void +set_rootvnode(void *arg, int npending) +{ + struct thread *td =3D curthread; + struct proc *p =3D td->td_proc; + + FILEDESC_XLOCK(p->p_fd); + if (p->p_fd->fd_cdir =3D=3D NULL) { + p->p_fd->fd_cdir =3D rootvnode; + VREF(rootvnode); + } + if (p->p_fd->fd_rdir =3D=3D NULL) { + p->p_fd->fd_rdir =3D rootvnode; + VREF(rootvnode); + } + FILEDESC_XUNLOCK(p->p_fd); + + free(arg, M_TEMP); +} + +/* + * Event handler called on mounting of /; bounce a task + * into the task queue thread to setup it's directories. + */ +static void +firmware_mountroot(void *arg) +{ + struct task *setroot_task; + + setroot_task =3D malloc(sizeof(struct task), M_TEMP, M_NOWAIT); + if (setroot_task !=3D NULL) { + TASK_INIT(setroot_task, 0, set_rootvnode, setroot_task); + taskqueue_enqueue(firmware_tq, setroot_task); + } else + printf("%s: no memory for task!\n", __func__); +} +EVENTHANDLER_DEFINE(mountroot, firmware_mountroot, NULL, 0); + +/* * The body of the task in charge of unloading autoloaded modules * that are not needed anymore. * Images can be cross-linked so we may need to make multiple passes, @@ -383,11 +476,23 @@ firmware_modevent(module_t mod, int type, void *unused) { struct priv_fw *fp; - int i, err =3D EINVAL; + int i, err; =20 switch (type) { case MOD_LOAD: - TASK_INIT(&firmware_task, 0, unloadentry, NULL); + TASK_INIT(&firmware_unload_task, 0, unloadentry, NULL); + firmware_tq =3D taskqueue_create("taskqueue_firmware", M_WAITOK, + taskqueue_thread_enqueue, &firmware_tq); + /* NB: use our own loop routine that sets up context */ + (void) taskqueue_start_threads(&firmware_tq, 1, PWAIT, + "firmware taskq"); + if (rootvnode !=3D NULL) { + /*=20 + * Root is already mounted so we won't get an event; + * simulate one here. + */ + firmware_mountroot(NULL); + } return 0; =20 case MOD_UNLOAD: @@ -398,8 +503,9 @@ fp->flags |=3D FW_UNLOAD;; } mtx_unlock(&firmware_mtx); - taskqueue_enqueue(taskqueue_thread, &firmware_task); - taskqueue_drain(taskqueue_thread, &firmware_task); + taskqueue_enqueue(firmware_tq, &firmware_unload_task); + taskqueue_drain(firmware_tq, &firmware_unload_task); + err =3D 0; for (i =3D 0; i < FIRMWARE_MAX; i++) { fp =3D &firmware_table[i]; if (fp->fw.name !=3D NULL) { @@ -409,6 +515,8 @@ err =3D EINVAL; } } + if (err =3D=3D 0) + taskqueue_free(firmware_tq); return err; } return EINVAL; @@ -417,7 +525,7 @@ static moduledata_t firmware_mod =3D { "firmware", firmware_modevent, - 0 + NULL }; DECLARE_MODULE(firmware, firmware_mod, SI_SUB_DRIVERS, SI_ORDER_FIRST); MODULE_VERSION(firmware, 1); Property changes on: kern/subr_firmware.c ___________________________________________________________________ Modified: cvs2svn:cvs-rev - 1.9 + 1.10 Index: kern/subr_prf.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- kern/subr_prf.c (revision 198202) +++ kern/subr_prf.c (working copy) @@ -955,7 +955,7 @@ } =20 SYSCTL_PROC(_kern, OID_AUTO, msgbuf, CTLTYPE_STRING | CTLFLAG_RD, - 0, 0, sysctl_kern_msgbuf, "A", "Contents of kernel message buffer"); + NULL, 0, sysctl_kern_msgbuf, "A", "Contents of kernel message buffer"); =20 static int msgbuf_clearflag; =20 Property changes on: contrib/pf ___________________________________________________________________ Modified: svn:mergeinfo Merged /head/sys/contrib/pf:r178042,183614,184842,188057 --zROEGoKAXsG5UqGB Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (FreeBSD) iEYEARECAAYFAkrbFUgACgkQC3+MBN1Mb4g+lACfe8ghbHFVpCw/3L365t368NJ8 JpgAoJpaK9a2kKCBSUveUn4CcjsudYcU =pYL2 -----END PGP SIGNATURE----- --zROEGoKAXsG5UqGB--