On Thu, 6 Jun 2002, Patrick Mochel wrote:
> Actually, I don't think the completion is needed at all. For one, it
> didn't work, and I found another bug. When we find a driver to a device,
> we inc the refcount. But, when we detach the driver via driver_detach(),
> we weren't decrementing it. The patch below does this. When you call
> driver_unregister(), it detaches the driver, then decs the refcount once,
> and Lo! it works.
Well, of course, when the refcounting is messed up, it doesn't work, but
at least it pointed out the error - I suppose after fixing that, it did
work?
> Second, Greg brought up an interesting point: if something is using a
> module, it should have already inc'd the refcount, and rmmod(8) won't let
> you remove it. If we can remove a module, but something is still using it,
> it's a bug. And, exposing those will get those fixed sooner...
That's not quite right. It's okay to be using a module while the module
refcount is zero, in fact it's very common. But you need to have a
module_exit() function which makes sure that the module is not used
anymore before it returns.
I think a good example are network drivers, since I think they get the
refcounting right. If you load your network driver, it'll register_netdev(),
but not inc the module use count. If someone ifconfig up's the interface,
only then the module use count is incremented and the ::open() method is
called. If you ifdown the interface, the module use count goes back to
zero after ::close() has been called. However, the struct net_device
is still registered with the network layer.
When you rmmod the module, which is possible since its use count is zero,
module_exit() will call unregister_netdev(), which initiate unregistering
and then sleep until noone else is using the struct net_device anymore.
At that point it's save to return to the module, which will kfree()
and exit.
(One thing is different there, though not really: struct net_device is
alloced dynamically, so the problem is to kfree() only after noone else
uses it, for struct driver it's most likely statically alloced and freed
after module_exit(), so that's why we have to wait for other users to go
away)
Anyway, the completion/wait is needed, there may be someone else using
struct driver at this moment (say someone doing driver_for_each_dev() on
it). Otherwise, the refcount is completely useless, between
driver_register() and driver_unregister() it will always be >= 1, so
inc'ing / dec'ing doesn't make any difference, the only point where it is
needed is exactly at driver_unregister() time.
> That said, I'm an offender of that rule with driverfs. It's the topic of
> another thread, and I haven't quite figured out the best solution. When we
> open a file belonging to a driver, we want to inc the module refcount.
> But, we don't currently have anyway to get at the module to do this.
Well, if you do the refcounting correctly, this should be doable, too.
I don't really have the time to go through drivers/base and fs/driverfs in
detail, but the general idea would be to following:
At driver_unregister() time, you need to unregister the dir_entry from its
parent, so that it won't be found anymore. However, the dir_entry still
is referenced from the dentry, so you hopefully inc'ed the
refcount for that. Eventually, the dentry will become unreferenced and
driverfs_d_delete_file() be called. You kfree() the dentry and put the
reference you had - if it was the last one, the usual mechanism
kicks in, your completion gets done and your module removed.
Alternatively, make it possible to dissociate the dentry and dir_entry
before, so you can do that, drop the ref count there and someone keeping
open a dir in /driversfs won't be able to delay the module unload
indefinitely. (The latter problem you'd surely have when you go
to driver::owner and use the to inc the use count)
You didn't select an easy problem to hack on, that's for sure. But I think
it's possible to get it right, just consistently make sure your refcount
is always right, and don't forget about indirect references e.g. via
list_entry in driversfs/inode.c.
--Kai
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Fri Jun 07 2002 - 22:00:28 EST