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

[Hollis Blanchard]

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.

> (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...

> Few comments on the patch below:
>
<snip>
>> +	/* fill in 'struct device' fields */
>> +	viodrv->driver.name = viodrv->name;
>> +	viodrv->driver.bus = &vio_bus_type;
>> +	viodrv->driver.probe = vio_bus_probe;
>> +	viodrv->driver.remove = vio_bus_remove;
>
> Don't you mean "driver" structure in that comment?

Yup, thanks.

>> -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.

>> +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?

> 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?

>> +/* taken from pci_module_init() */
>>  static inline int vio_module_init(struct vio_driver *drv)
<snip>

> Eeek!  I want to fix that code in pci_module_init() so it doesn't do
> this at all.  Please don't copy that horrible function.  Just register
> the driver with a call to vio_register_driver() and drop the whole
> vio_module_init() completly.  I'll be doing that for pci soon, and
> there's no reason you want to duplicate this broken logic (you always
> want your module probe to succeed, for lots of reasons...)

Good to know. PCI has been the model for the VIO code. I figured there
was a reason for this function, even if I didn't understand it... :)

--
Hollis Blanchard
IBM Linux Technology Center


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