From owner-freebsd-hackers@FreeBSD.ORG Tue Nov 8 21:44:32 2011 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 6FF351065677 for ; Tue, 8 Nov 2011 21:44:32 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 476AA8FC14 for ; Tue, 8 Nov 2011 21:44:32 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id F328346B3C; Tue, 8 Nov 2011 16:44:31 -0500 (EST) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 7D7BB8A02E; Tue, 8 Nov 2011 16:44:31 -0500 (EST) From: John Baldwin To: Navdeep Parhar Date: Tue, 8 Nov 2011 16:43:50 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p8; KDE/4.5.5; amd64; ; ) References: <201111081000.27332.jhb@freebsd.org> <20111108204700.GA1542@itx> In-Reply-To: <20111108204700.GA1542@itx> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201111081643.51080.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.6 (bigwig.baldwin.cx); Tue, 08 Nov 2011 16:44:31 -0500 (EST) Cc: freebsd-hackers@freebsd.org Subject: Re: incorrect parent refcounting in subr_firmware.c? 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: Tue, 08 Nov 2011 21:44:32 -0000 On Tuesday, November 08, 2011 3:47:00 pm Navdeep Parhar wrote: > On Tue, Nov 08, 2011 at 10:00:27AM -0500, John Baldwin wrote: > > On Wednesday, November 02, 2011 3:56:29 pm Navdeep Parhar wrote: > > > I built a KLD with multiple firmware images, as shown here: > > > > > > KMOD=foo > > > FIRMWS= foo.bin:foo:1.0.0.0 > > > FIRMWS+=bar.bin:bar:1.0.0.0 > > > FIRMWS+= ... > > > .include > > > > > > "foo" is the parent firmware and a firmware_get(foo) can autoload the > > > KLD. "bar" and the rest are available only if the KLD is loaded (by > > > whatever means). This is reasonable and works as expected. But if I > > > just get and then put "foo" back, the KLD is not unloaded automatically. > > > > > > The problem is that a reference is placed on the parent firmware when > > > the other firmwares are registered (during module load). I think this > > > reference should be placed during firmware_get on the child. > > > > > > What do people think about the attached patch? It fixes things for me. > > > > Hmm, what about the use case where a driver does: > > > > f = firmware_get("foo"); > > firmware_put(f) > > f = firmware_get("bar"); > > firmware_put(f) > > > > Is that going to trigger multiple loads/unloads with this change? > > Without the patch, the get(bar) always works. But that's because the > KLD is stuck in memory (it's not auto-unloaded even after the two puts > and it can't be unloaded manually either). > > With the patch, get(bar) does not work after put(foo, FIRMWARE_UNLOAD). > This is correct behaviour as documented in firmware(9) - the parent > firmware is the only one that the kernel can locate with a simple name > match. Once the KLD is unloaded, the kernel can't find bar. (The get > would probably work after a put(foo, 0) because the KLD won't get > unloaded without a FW_UNLOAD. But I'm not interested in keeping it > around forever so I always specify FW_UNLOAD). > > As long as I put all the images back this way, the KLD is auto-unloaded > at the end. Without the patch it just stays around forever. > > get(foo) /* autoloads foo.ko which has the "bar" image too. */ > get(bar) > put(bar, FIRMWARE_UNLOAD) > put(foo, FIRMWARE_UNLOAD) /* KLD auto-unloaded, user happy. */ Ahh, ok. I think this is fine. It might be worth updating the manpage to explicitly mention child firmware images and to explain this requirement (right now it doesn't mention extra firmware images at all). -- John Baldwin