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

[Greg KH]

On Tue, Jan 06, 2004 at 05:26:09PM -0600, Hollis Blanchard wrote:
> On Jan 6, 2004, at 4:44 PM, Linda Xie wrote:
> >diff -Nru a/arch/ppc64/kernel/vio.c b/arch/ppc64/kernel/vio.c
> >--- a/arch/ppc64/kernel/vio.c	Tue Jan  6 16:29:17 2004
> >+++ b/arch/ppc64/kernel/vio.c	Tue Jan  6 16:29:17 2004
> >@@ -189,7 +189,7 @@
> > 		const struct vio_device_id* id;
> >
> > 		id = vio_match_device(driver->id_table, dev);
> >-		if (id && (0 < driver->probe(dev, id))) {
> >+		if (id && (0 == driver->probe(dev, id))) {
> > 			printk(KERN_DEBUG "%s: driver %s/%s took device
> > 			%p\n",
> > 				__FUNCTION__, id->type, id->compat, dev);
> > 			dev->driver = driver;
>
> You're right that the drivers return 0 on success, but all this code is
> about to be replaced with 2.6 driver model code anyways. The driver
> model gives us basic sysfs presense and list locking for free.
>
> Could you test this patch instead? It should require no driver changes.
> (I don't think the patch will be whitespace-wrapped but let me know.)

It is wrapped :(

> 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?  (I have no idea what "vio" is, is it a bus?)

Few comments on the patch below:

> +int vio_register_driver(struct vio_driver *viodrv)
>  {
> -	int count = 0;
> -	struct vio_dev *dev;
> -
> -	printk(KERN_DEBUG "%s: driver %s/%s registering\n", __FUNCTION__,
> -		drv->id_table[0].type, drv->id_table[0].type);
> +	printk(KERN_DEBUG "%s: driver %s registering\n", __FUNCTION__,
> +		viodrv->name);
>
> -	/* find matching devices not already claimed by other drivers and
> pass
> -	 * them to probe() */
> -	list_for_each_entry(dev, &vio_bus.devices, devices_list) {
> -		const struct vio_device_id* id;
> -
> -		if (dev->driver)
> -			continue; /* this device is already owned */
> -
> -		id = vio_match_device(drv->id_table, dev);
> -		if (drv && id) {
> -			if (0 == drv->probe(dev, id)) {
> -				printk(KERN_DEBUG "  took device %p\n", dev);
> -				dev->driver = drv;
> -				count++;
> -			}
> -		}
> -	}
> +	/* 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?

> -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
:)

> +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 :(

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.

> +static inline struct vio_driver *to_vio_driver(struct device_driver
> *drv)
> +{
> +	return container_of(drv, struct vio_driver, driver);
> +}
> +

It's ok to make that a macro instead.  It doesn't have to be a inline
function.

> +/* taken from pci_module_init() */
>  static inline int vio_module_init(struct vio_driver *drv)
>  {
> -        int rc = vio_register_driver (drv);
> +	int rc = vio_register_driver(drv);
>
> -        if (rc > 0)
> -                return 0;
> +	if (rc > 0)
> +		return 0;
>
> -        /* iff CONFIG_HOTPLUG and built into kernel, we should
> -         * leave the driver around for future hotplug events.
> -         * For the module case, a hotplug daemon of some sort
> -         * should load a module in response to an insert event. */
> +	/* iff CONFIG_HOTPLUG and built into kernel, we should
> +	 * leave the driver around for future hotplug events.
> +	 * For the module case, a hotplug daemon of some sort
> +	 * should load a module in response to an insert event. */
>  #if defined(CONFIG_HOTPLUG) && !defined(MODULE)
> -        if (rc == 0)
> -                return 0;
> +	if (rc == 0)
> +		return 0;
>  #else
> -        if (rc == 0)
> -                rc = -ENODEV;
> +	if (rc == 0)
> +		rc = -ENODEV;
>  #endif
>
> -        /* if we get here, we need to clean up vio driver instance
> -         * and return some sort of error */
> +	/* if we get here, we need to clean up vio driver instance
> +	 * and return some sort of error */
> +	vio_unregister_driver(drv);
>
> -        return rc;
> +	return rc;
>  }

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

Hope this helps,

greg k-h

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