[PATCH][2.6] set up vio_dev's driver field

[Greg KH]

On Fri, Jan 09, 2004 at 01:18:46PM -0600, Hollis Blanchard wrote:
> On Jan 9, 2004, at 12:29 PM, Greg KH wrote:
> >On Tue, Jan 06, 2004 at 05:26:09PM -0600, Hollis Blanchard wrote:
> >>Comments from Greg KH also welcome, though Linda's mail prompted me to
> >>send this out before I've double-checked everything. :) In particular
> >>I
> >>had to create a static struct device to act as the VIO bus device,
> >>since the virtual bus doesn't have an actual root struct device
> >>(unlike
> >>PCI and USB)...
> >
> >Ick, ick, ick.  _Please_ never make a struct device static.  Bad things
> >will happen if you get your reference counting wrong.  Hm, actually it
> >looks like it will get messed up if the release() function gets called
> >for it.
> >
> >Are you doing this to get a "parent" device to hang everything else off
> >of?
>
> Yes, exactly. Without such a parent device, the devices will be listed
> at the same level as PCI busses. I think they could be more organized
> than that; Open Firmware for example creates a "vdevice" directory at
> the same level as the PCI busses.

Ah, ok.  See below for more...

> >(I have no idea what "vio" is, is it a bus?)
>
> "vio" means "virtual IO", such as virtual ethernet or virtual SCSI.
> These can also be hotplugged...

But can your "parent" device ever go away?  Can the vio code be built as
a module?

> >>-int vio_unregister_driver(struct vio_driver *driver)
> >>+int vio_unregister_driver(struct vio_driver *viodrv)
> >
> >Why return anything here?  Who cares at unregister time?  Are you going
> >to fail something if it doesn't happen?  It always will be unregistered
> >:)
>
> Habit I guess. Almost all functions can fail... Here, what if the
> driver is in use when somebody tried to unregister it? Right now,
> vio_unregister_driver() is very simple, but maybe in the future it will
> become more complicated, and then we'll wish we had that return code
> and didn't have to modify callers. So habit, and planning for the
> future.

But my point is, what could you ever do if unregister fails?

> >>+static int __init
> >> vio_bus_init(void)
> >> {
> >> 	struct device_node *node_vroot, *node_vdev;
> >>+	int err;
> >>
> >>-	INIT_LIST_HEAD(&vio_bus.devices);
> >>+	err = bus_register(&vio_bus_type);
> >>+	if (err)
> >>+		return err;
> >>+
> >>+	/* the parent of all vio devices */
> >>+	memset(&vio_bus_device, 0, sizeof(struct device));
> >>+	strcpy(vio_bus_device.bus_id, "vio");
> >>+	device_register(&vio_bus_device);
> >
> >Oops, you just set up the "release" function of your static device to
> >call kfree when it is unregistered.  Not good :(
>
> Hmm, how would it become unregistered?

You never unload this code?  It can't be a module?  You never clean up
all of your devices at shutdown time?  If you do, this needs to be
dynamically created.

> >If you don't have a place on the system bus, just keep the first
> >device's parent as NULL.  That way it will be placed at the top of the
> >device directory properly.  This is how pci busses work.
>
> There could potentially be dozens of virtual devices (console,
> ethernet, scsi). Right now the "devices" sysfs directory contains only
> directories (pci*, legacy, system). Without a fake VIO parent device,
> there will be no VIO analog to the PCI busses here, and all virtual
> devices (with names like "vty at 30000000" and "llan at 3000010") will appear
> in this directory. Is that what you want? Would that impact tools like
> lsbus?

No, a "fake" parent device is fine in this case.  But what kind of
devices do these VIO devices hang off of?  They should have some kind of
addressable device as a parent, right?  But if not, then yes, a "fake"
parent device is ok.

thanks,

greg k-h

** Sent via the linuxppc64-dev mail list. See http://lists.linuxppc.org/