Re: [PATCH 3/3] staging: vme: make match() driver specific toimprove non-VME64x support

[Emilio G. Cota]

Hi Manohar,

Great work! A couple of comments inlined below.

On Mon, Aug 29, 2011 at 11:02:50 +0200, Manohar Vanga wrote:
(snip)
> +static int __vme_register_driver_bus(struct vme_driver *drv,
> +	struct vme_bridge *bridge, unsigned int ndevs)
> +{
> +	int err;
> +	unsigned int i;
> +	struct vme_dev *vdev;
> +	struct vme_dev *tmp;
> +
> +	for (i = 0; i < ndevs; i++) {
> +		vdev = kzalloc(sizeof(struct vme_dev), GFP_KERNEL);
> +		if (!vdev) {
> +			err = -ENOMEM;
> +			goto err_alloc;
> +		}
> +		vdev->id.num = i;
>  		vdev->id.bus = bridge->num;
>  		vdev->id.slot = i + 1;
>  		vdev->bridge = bridge;
> +		vdev->dev.platform_data = drv;
> +		vdev->dev.release = vme_dev_release;
>  		vdev->dev.parent = bridge->parent;
>  		vdev->dev.bus = &vme_bus_type;
> -		/*
> -		 * We save a pointer to the bridge in platform_data so that we
> -		 * can get to it later. We keep driver_data for use by the
> -		 * driver that binds against the slot
> -		 */
> -		vdev->dev.platform_data = bridge;
> -		dev_set_name(&vdev->dev, "vme-%x.%x", bridge->num, i + 1);
> -
> -		retval = device_register(&vdev->dev);
> -		if (retval)
> -			goto err_reg;
> +		dev_set_name(&vdev->dev, "%s.%u-%u", drv->name, vdev->id.bus,
> +			vdev->id.num);
> +
> +		err = device_register(&vdev->dev);
> +		if (err)
> +			goto err_dev_reg;
> +
> +		if (vdev->dev.platform_data) {
> +			list_add_tail(&vdev->list, &drv->devices);
> +			drv->ndev++;
> +		} else {
> +			device_unregister(&vdev->dev);
> +			kfree(vdev);
> +		}
>  	}
> +	return 0;
>  
> -	return retval;
> -
> -err_reg:
> -	while (--i >= 0) {
> -		vdev = &bridge->dev[i];
> +err_dev_reg:
> +	kfree(vdev);
> +err_alloc:
> +	list_for_each_entry_safe(vdev, tmp, &drv->devices, list) {
> +		list_del(&vdev->list);
>  		device_unregister(&vdev->dev);

Here there seems to be a missing kfree(vdev), like the one after device_unregister()
a few lines earlier.

>  	}
> -	vme_remove_bus(bridge);
> -	return retval;
> +	return err;
>  }
> -EXPORT_SYMBOL(vme_register_bridge);

(snip)

> -int vme_register_driver(struct vme_driver *drv)
> +int vme_register_driver(struct vme_driver *drv, unsigned int ndevs)
>  {
> +	int err;
> +
>  	drv->driver.name = drv->name;
>  	drv->driver.bus = &vme_bus_type;
> -
> -	return driver_register(&drv->driver);
> +	INIT_LIST_HEAD(&drv->devices);
> +
> +	err = driver_register(&drv->driver);
> +	if (err)
> +		return err;
> +
> +	err = __vme_register_driver(drv, ndevs);
> +	if (!err & (drv->ndev == 0))

Probably you meant if (!err && drv->ndev == 0) here.

In fact one could imagine a driver that has no devices when it's
installed (it might get them later); so I'd remove this check.

> +		err = -ENODEV;
> +	if (err)
> +		vme_unregister_driver(drv);
> +	return err;
>  }
>  EXPORT_SYMBOL(vme_register_driver);


		Emilio
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/