I found myself needing this in a bad way while converting linux1394 to
use the new device model.
The ieee1394 stack has a tree like model, similar to USB. We have hosts,
that have nodes as children, and each node has unit directories which
describe functionality the node supports. Each one of these is
represented by a device now, which is parented accordingly.
Now, I wanted to remove all the internal handling we had for the
functionality that is now handled by the device model. All the lists,
the callbacks, probing, binding, etc. However, I couldn't do this
because there is no way for removing one device and having all of it's
children subsequently removed aswell. While a device keeps a list of its
children, nothing made sure that list was empty before it was removed.
That meant I would have to re-add a lot of the same house-keeping that
the device model promised me I could remove. Linked lists and more
callbacks.
I tried making device_unregister() in turn go down the list of children
and call device_unregister() before calling device_del() on itself. But
some core internals didn't like that, and loading a new pci device
caused it to crap up.
So I added a new callback to the device stucture called remove. This
callback is done when device_del is about to remove a device from the
tree. I've used this internally to make sure I can walk the list of
children myself, and also do some other cleanups.
It did this on a per-device context as opposed to a per-class or
per-bus, because some devices may need special callbacks. Consider a
psuedo or emulated device that is created outside the bus's normal
context (e.g. an emulated sbp2 device attached via userspace to the
local firewire node, using raw1394's kernel/userspace interface).
Also, I did consider the release callback, but that's run just a little
too late in the game for this purpose.
So, here's my simple patch. I'd really like this to be applied to the
proper kernel. I really can't see how the driver model is working
without walking children on unregister, but this atleast allows you to
handle it yourself.
--- linux-2.5.64.orig/drivers/base/core.c 2003-03-05 08:49:39.000000000 -0500
+++ linux-2.5.64/drivers/base/core.c 2003-03-09 11:32:43.000000000 -0500
@@ -266,6 +266,11 @@
{
struct device * parent = dev->parent;
+ /* Let the device know it needs to leave us, possibly
+ * deconfiguring itself or some such. */
+ if (dev->remove)
+ dev->remove(dev);
+
if (parent) {
down(&device_sem);
list_del_init(&dev->node);
--- linux-2.5.64.orig/include/linux/device.h 2003-03-05 08:49:52.000000000 -0500
+++ linux-2.5.64/include/linux/device.h 2003-03-09 09:04:18.000000000 -0500
@@ -254,6 +254,7 @@
u64 *dma_mask; /* dma mask (if dma'able device) */
void (*release)(struct device * dev);
+ void (*remove)(struct device * dev);
};
static inline struct device *
On Sun, Mar 09, 2003 at 01:14:13PM -0500, Ben Collins wrote:
>
> So I added a new callback to the device stucture called remove. This
> callback is done when device_del is about to remove a device from the
> tree. I've used this internally to make sure I can walk the list of
> children myself, and also do some other cleanups.
But don't you really want to remove the children before you remove the
parent? If you do this patch, then the remove() function will have to
clean up the children first, right? Can we handle the core recursion
with the current locks properly?
Yes, for USB we still have a list of a device's children, as we need
them for various things, and the current driver model only has a parent
pointer, not a child pointer (which is good, as for USB we can have
multiple children). So in the function where we know a USB device is
disconnected, we walk our list of children and disconnect them in a
depth-first order. With this patch I don't see how it helps me push
code into the driver core.
Confused,
greg k-h
On Sun, Mar 09, 2003 at 04:11:02PM -0800, Greg KH wrote:
> On Sun, Mar 09, 2003 at 01:14:13PM -0500, Ben Collins wrote:
> >
> > So I added a new callback to the device stucture called remove. This
> > callback is done when device_del is about to remove a device from the
> > tree. I've used this internally to make sure I can walk the list of
> > children myself, and also do some other cleanups.
>
> But don't you really want to remove the children before you remove the
> parent? If you do this patch, then the remove() function will have to
> clean up the children first, right? Can we handle the core recursion
> with the current locks properly?
Actually, with this patch, the dev->remove(dev) is called before the
driver model does any cleanup. So you can cleanup children at that
point, and the parent device is still sane.
The reason for this is I would like to be able to unregister a node's
device from several places without worrying about other things that need
to be done. One call.
> Yes, for USB we still have a list of a device's children, as we need
> them for various things, and the current driver model only has a parent
> pointer, not a child pointer (which is good, as for USB we can have
> multiple children). So in the function where we know a USB device is
> disconnected, we walk our list of children and disconnect them in a
> depth-first order. With this patch I don't see how it helps me push
> code into the driver core.
I haven't looked into USB in depth, but consider this. Without the
patch, to cleanup a device:
void ieee1394_remove_node(struct node_entry *ne)
{
...
list_for_each(..., &ne->device.children) {
device_unregister(list_to_dev(lh));
}
device_unregister(&ne->device);
}
Then to remove a device, this function must always be called, so that
the unit-directories get removed. What happens if the PCI bus gets
yanked out from underneath us? How does the OHCI card's callbacks get me
back down to this point? Without a lot of extra infrastructure, the
nodes and unit directories get left hanging.
Instead I now do this, with the patch.
void ieee1394_remove_node(struct device *dev)
{
list_for_each(..., &ne->device.children) {
device_unregister(list_to_dev(lh));
}
}
...
/* Where the dev is created */
...
ne->device.remove = ieee1394_remove_node;
device_register(&ne->device);
Now, no matter where it's called from, doing...
device_unregister(&ne->device);
...will make sure my remove callback is executed, so the children
devices get unregistered aswell. I extend this to the host device
and I have a recursive remocal scheme that is safe no matter where my
devices get unregistered. Whole lot simpler that adding in a lot of
failsafe's and checks.
--
Debian - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
Deqo - http://www.deqo.com/
> So, here's my simple patch. I'd really like this to be applied to the
> proper kernel. I really can't see how the driver model is working
> without walking children on unregister, but this atleast allows you to
> handle it yourself.
The assumption is that the bus driver will take care of cleaning up all
the children before unregistering the parent. This place a bit more
responsibility on the bus driver, but it keeps it simple in the core.
That's not to say that it can't change in the future, but I don't want to
take that step right now. There are a lot of implications WRT locking and
recursion that need to be worked out, and I'd rather wait on making these
kind of core changes.
-pat
On Mon, Mar 10, 2003 at 09:45:12AM -0600, Patrick Mochel wrote:
>
> > So, here's my simple patch. I'd really like this to be applied to the
> > proper kernel. I really can't see how the driver model is working
> > without walking children on unregister, but this atleast allows you to
> > handle it yourself.
>
> The assumption is that the bus driver will take care of cleaning up all
> the children before unregistering the parent. This place a bit more
> responsibility on the bus driver, but it keeps it simple in the core.
>
> That's not to say that it can't change in the future, but I don't want to
> take that step right now. There are a lot of implications WRT locking and
> recursion that need to be worked out, and I'd rather wait on making these
> kind of core changes.
That's fine, I can deal with that. But this patch only allows you to add
a ->remove member to a device, which will aide in ensuring that.
Currently there's not even a check in the driver core for whether a
device has children when it is removed. Adding the remove function will
make it easier for the bus to validate a device, sanity check it, and
cleanup after it before it gets demolished from the core.
Maybe you need to {get,put}_device() wrap the call to remove, but that
shouldn't be a big problem with locking (really, the patch is simple).
--
Debian - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
Deqo - http://www.deqo.com/
> Instead I now do this, with the patch.
>
> void ieee1394_remove_node(struct device *dev)
> {
> list_for_each(..., &ne->device.children) {
> device_unregister(list_to_dev(lh));
> }
> }
>
> ...
> /* Where the dev is created */
> ...
> ne->device.remove = ieee1394_remove_node;
> device_register(&ne->device);
>
> Now, no matter where it's called from, doing...
>
> device_unregister(&ne->device);
>
> ...will make sure my remove callback is executed, so the children
> devices get unregistered aswell. I extend this to the host device
> and I have a recursive remocal scheme that is safe no matter where my
> devices get unregistered. Whole lot simpler that adding in a lot of
> failsafe's and checks.
But, you can do exactly the same with just a few added lines in the
ieee1394 core - just have a wrapper that calls ->remove() (in the 1394
device structure), then calls device_unregister() for the device.
Your ->remove() can do anything you like, including removing all the
children, much as I assume it does now. So, behavior is exactly as you
want, and it keeps it out of the core for now.
I much prefer this, as I would like to see it eventually, but I'd rather
see the implications worked out before it's generalized.
Thanks,
-pat
> I much prefer this, as I would like to see it eventually, but I'd rather
> see the implications worked out before it's generalized.
Then I have to be concerned about parts of the driver model removing
parents of my devices without my knowing it. Didn't PCI already go
through this problem with bus's being removed?
If my PCI devices gets removed, it simply calls my PCI callbacks, but
then my PCI drivers have to link into the core and call remove on all
the host devices, then node devices, then unit directories. All this has
to happen manually, and it puts the burden all the way down the tree,
when it should remain only in the bus.
It also does not help the case where something emulates an IEEE-1394
node on the locally handled bus. If it creates a node, and then behind
that, creates unit directories, and then attaches some other sort of
children unknown to the ieee1394 core. There's no possible way that
device can safely be removed by the ieee1394 core. So then I have to
export all sorts of extra functionality to provide the same thing this
2 line callback can do.
I'm not sure what the problem is in allowing the bus driver to know when
a device is about to be removed for some reason. At the very least it
makes for a good sanity check mechanism.
--
Debian - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
Deqo - http://www.deqo.com/
On Mon, Mar 10, 2003 at 11:55:48AM -0500, Ben Collins wrote:
> > I much prefer this, as I would like to see it eventually, but I'd rather
> > see the implications worked out before it's generalized.
>
> Then I have to be concerned about parts of the driver model removing
> parents of my devices without my knowing it. Didn't PCI already go
> through this problem with bus's being removed?
Not that I know of, no. The PCI core knows when it is removing busses,
as it is the one doing this.
> If my PCI devices gets removed, it simply calls my PCI callbacks, but
> then my PCI drivers have to link into the core and call remove on all
> the host devices, then node devices, then unit directories.
Um, don't you have to do this already today, if someone unloads your pci
driver? I don't see what the driver core has to do with that.
> I'm not sure what the problem is in allowing the bus driver to know when
> a device is about to be removed for some reason. At the very least it
> makes for a good sanity check mechanism.
As the bus driver was the one who asked for the device to go away in the
first place, why isn't this just extra information?
Still confused,
greg k-h
It's less work to add all this overhead back into my subsystem than to
argue about it's worthiness. It seemed obviously in-line with the
driver-model's purpose to have a remove callback, but maybe my way of
thinking is expecting too much from the driver core, and I should do the
work myself in the subsystem.
thanks
--
Debian - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
Deqo - http://www.deqo.com/