From owner-cvs-src@FreeBSD.ORG Mon Jun 19 18:24:05 2006 Return-Path: X-Original-To: cvs-src@freebsd.org Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 9954616A474; Mon, 19 Jun 2006 18:24:05 +0000 (UTC) (envelope-from iedowse@iedowse.com) Received: from nowhere.iedowse.com (nowhere.iedowse.com [82.195.144.75]) by mx1.FreeBSD.org (Postfix) with SMTP id 2B6D643D48; Mon, 19 Jun 2006 18:24:03 +0000 (GMT) (envelope-from iedowse@iedowse.com) Received: from localhost ([127.0.0.1] helo=iedowse.com) by nowhere.iedowse.com via local-iedowse id ; 19 Jun 2006 19:24:02 +0100 (IST) To: John Baldwin In-Reply-To: Your message of "Mon, 19 Jun 2006 09:15:50 EDT." <200606190915.50962.jhb@freebsd.org> Date: Mon, 19 Jun 2006 19:24:00 +0100 From: Ian Dowse Message-ID: <200606191924.aa78290@nowhere.iedowse.com> Cc: cvs-src@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/sys firmware.h src/sys/kern subr_firmware.c X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Jun 2006 18:24:05 -0000 In message <200606190915.50962.jhb@freebsd.org>, John Baldwin writes: >On Friday 16 June 2006 20:47, Ian Dowse wrote: >> - driver calls firmware_get, firmware image loaded and fp->file set to non-NULL >> - manually kldload some_module_that_depends_on_firmware_image >> - driver calls firmware_put, unloadentry called and sets fp->file = NULL > >In practice no modules depend on firmware modules. :) I think we should >take the approach of not clearing fp->file in unloadentry() however. >That would result in correct behavior in every case I can think of (or as >close to correct as you can get). In the above case the >linker_file_unload() would have fail leaving the firmware module around. How about the following patch? In the above case linker_file_unload() would actually succeed because the linker file reference count is just dropping from 2 to 1, so we'd get a negative reference count later if we kept fp->file non-NULL after it returns. Ian Index: subr_firmware.c =================================================================== RCS file: /home/ncvs/src/sys/kern/subr_firmware.c,v retrieving revision 1.2 diff -u -r1.2 subr_firmware.c --- subr_firmware.c 10 Jun 2006 17:04:07 -0000 1.2 +++ subr_firmware.c 19 Jun 2006 18:15:21 -0000 @@ -206,7 +206,7 @@ { struct firmware *fp; linker_file_t file; - int i; + int i, err; mtx_lock(&firmware_mtx); for (;;) { @@ -226,9 +226,18 @@ fp->file = NULL; mtx_unlock(&firmware_mtx); - linker_file_unload(file, LINKER_UNLOAD_NORMAL); - + err = linker_file_unload(file, LINKER_UNLOAD_NORMAL); mtx_lock(&firmware_mtx); + if (err) { + /* + * If linker_file_unload() failed then we still hold + * a reference on the module so it should not be + * possible for it to go away or be re-registered. + */ + KASSERT(fp->file == NULL, + ("firmware entry reused while referenced!")); + fp->file = file; + } } mtx_unlock(&firmware_mtx); }