incorrect parent refcounting in subr_firmware.c?

[John Baldwin]

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 <bsd.kmod.mk>
> > > 
> > > "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