Re: [PATCH 1/5] UBI: add UBI control device

[Artem Bityutskiy <dedekind@infradead.org>]

On Wed, 2007-12-19 at 15:11 +0100, Arnd Bergmann wrote:
> > 
> > This patch is a preparation to make UBI devices dynamic. It
> > adds an UBI control device which has dynamically allocated
> > major number and registers itself as "ubi_ctrl". It does not
> > do anything so far. The idea is that this device will allow
> > to attach/detach MTD devices from userspace.
> > 
> > This is symilar to what the Linux device mapper has.
> 
> I don't really like the idea of a separate device node, the
> point that device mapper does it is not a particularly strong
> argument.
> 
> A better alternative would be probably be to do it similar
> to the loopback device, where you attach the device node itself
> with a back-end (file in case of loop).

Pardon, I do not get this. Please, explain it some more. So, we have 2
MTD devices in the system - mtd0 and mtd1, and UBI is compiled in the
kernel. How do I create UBI device on top of mtd0?

> > + * When UBI is initialized, it attaches all the MTD devices specified as the
> > + * module load parameters or the kernel boot parameters. If MTD devices were
> > + * specified, UBI does not attach any MTD device, but it is possible to do
> > + * later using the "UBI control device".
> > + *
> > + * At the moment we only attach UBI devices by scanning, which will become a
> > + * bottleneck when flashes reach certain large size. Then one may improve UBI
> > + * and add other methods, although it does not seem to be easy to do.
> >   */
> 
> I'd say a more natural approach would be to use attributes in sysfs to allow the
> probing and destruction of the UBI devices. That way, you could use much
> simpler hooks into udev to create them, without the need of a special user
> ioctl interface.

Pardon? I register ubi_ctl within this thing which is called "Linux
Device Model" and I already automatically have udev
creating /dev/ubi_ctrl. device. And everything is already simple.
Please, elaborate.

> > +/* UBI control character device major number */
> > +static int ubi_ctrl_major;
> 
> Why do you need you own major number? I think if you really want this ioctl
> interface, a misc device would be completely sufficient.

I wanted to have /dev/ubi_ctrl, why not? What's wrong with this?

> > +	/* Register the control device in the Linux device system */
> > +	ubi_ctrl_dev.release = ctrl_dev_release;
> > +	ubi_ctrl_dev.devt = MKDEV(ubi_ctrl_major, 0);
> > +	ubi_ctrl_dev.class = ubi_class;
> > +	sprintf(&ubi_ctrl_dev.bus_id[0], "ubi_ctrl");
> > +	err = device_register(&ubi_ctrl_dev);
> > +	if (err) {
> > +		printk(KERN_ERR "UBI error: cannot register device\n");
> > +		goto out_version;
> > +	}
> 
> This looks like overengineering, you can simplify this a lot by using
> a misc device.

I will look what misc device is and if it is OK for me. And I do not see
anything complex here, to be frank :-)

>   
> >  	ubi_ltree_slab = kmem_cache_create("ubi_ltree_slab",
> >  					   sizeof(struct ubi_ltree_entry), 0,
> >  					   0, &ltree_entry_ctor);
> 
> How many objects to you expect to have in this cache? Is it just one
> object per device? That would hardly be worth creating a special cache.

Well, this is a subject of separate patch, because I just move this code
no. But the reason to have separate slab was that I have to optimize
this by having my own constructor.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)