From owner-freebsd-hackers@FreeBSD.ORG Tue Nov 8 20:47:10 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 99990106564A for ; Tue, 8 Nov 2011 20:47:10 +0000 (UTC) (envelope-from nparhar@gmail.com) Received: from mail-pz0-f44.google.com (mail-pz0-f44.google.com [209.85.210.44]) by mx1.freebsd.org (Postfix) with ESMTP id 6786E8FC17 for ; Tue, 8 Nov 2011 20:47:10 +0000 (UTC) Received: by pzk32 with SMTP id 32so4326591pzk.3 for ; Tue, 08 Nov 2011 12:47:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=yphAKVhJPVz/cCU6A8mNRtbWboiIH3cCHGaWNO0ksnI=; b=i9Eu/+K+2BlF4QMtqkDzXcqxw7Qqu1pulT8pKUEfZiY9r14Guw3ck3R/xn4HHVRmok oRjAMa98fDSStsRFnLHmku2y1QVVLGBbweib0cvU8WuMrF8uBlz4o1UkCN3ujWVsqV01 y4cr+4PrpNboHN+aBPCfrR1PbTfhYvYbuPElY= Received: by 10.68.33.227 with SMTP id u3mr3630106pbi.72.1320785229615; Tue, 08 Nov 2011 12:47:09 -0800 (PST) Received: from itx (c-107-3-142-221.hsd1.ca.comcast.net. [107.3.142.221]) by mx.google.com with ESMTPS id km16sm6930320pbb.9.2011.11.08.12.47.06 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 08 Nov 2011 12:47:07 -0800 (PST) Date: Tue, 8 Nov 2011 12:47:00 -0800 From: Navdeep Parhar To: John Baldwin Message-ID: <20111108204700.GA1542@itx> Mail-Followup-To: John Baldwin , freebsd-hackers@freebsd.org References: <201111081000.27332.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201111081000.27332.jhb@freebsd.org> User-Agent: Mutt/1.5.20 (2009-06-14) 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 20:47:10 -0000 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. */ Regards, Navdeep