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

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

[Arnd Bergmann]

On Wednesday 19 December 2007, Artem Bityutskiy wrote:
> From 4820ff7357b102cea37ec581d6d0ffd5a24348d2 Mon Sep 17 00:00:00 2001
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> Date: Sun, 16 Dec 2007 16:59:31 +0200
> Subject: [PATCH] UBI: add UBI control device
> 
> 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).


>  /*
> - * This file includes UBI initialization and building of UBI devices. At the
> - * moment UBI devices may only be added while UBI is initialized, but dynamic
> - * device add/remove functionality is planned. Also, 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.
> + * This file includes UBI initialization and building of UBI devices.
> + *
> + * 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.

  
>  #include <linux/err.h>
> @@ -70,6 +75,11 @@ struct kmem_cache *ubi_ltree_slab;
>  /* Slab cache for wear-leveling entries */
>  struct kmem_cache *ubi_wl_entry_slab;
>  
> +/* 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.

> +/* Linux device model object corresponding to the control UBI device */
> +static struct device ubi_ctrl_dev;

What is this device used for?

> +	/* Create base sysfs directory and sysfs files */
>  	ubi_class = class_create(THIS_MODULE, UBI_NAME_STR);
> -	if (IS_ERR(ubi_class))
> -		return PTR_ERR(ubi_class);
> +	if (IS_ERR(ubi_class)) {
> +		err = PTR_ERR(ubi_class);
> +		printk(KERN_ERR "UBI error: cannot create UBI class\n");
> +		goto out_cdev_unreg;
> +	}
>  
>  	err = class_create_file(ubi_class, &ubi_version);
> -	if (err)
> +	if (err) {
> +		printk(KERN_ERR "UBI error: cannot create sysfs file\n");
>  		goto out_class;
> +	}
> +
> +	/* 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.
  
>  	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.

	Arnd <><

Re: [PATCH 4/5] UBI: introduce attach ioctls

[Arnd Bergmann]

On Wednesday 19 December 2007, Artem Bityutskiy wrote:
> +/**
> + * struct ubi_attach_req - attach MTD device request.
> + * @vid_hdr_offset: VID header offset
> + * @data_offset: data offset
> + * @mtd_num: MTD device number to attach
> + * @padding: reserved for future, not used, has to be zeroed
> + *
> + * This data structure is used to specify MTD device UBI has to attach and the
> + * parameters it has to use. The "attach MTD device" ioctl returns the number
> + * of the newly created UBI device as the return value.
> + */
> +struct ubi_attach_req {
> +       int32_t vid_hdr_offset;
> +       int32_t data_offset;
> +       int32_t mtd_num;
> +       uint8_t padding[12];
>  };


Can you explain why you need to pass vid_hdr_offset /and/ data_offset here?
What is the difference between the two? Can't you autoprobe them if you
have the device?

The reason I'm asking is that I'd really like to make this a simple
attribute in sysfs, in the mtd object. The question there is what a
user would need to store into that attribute. The device is identified
implicitly already, but this looks like you still need two distint
integers in order to create an UBI device.

	Arnd <><

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

[Artem Bityutskiy]

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 (Битюцкий Артём)

Re: [PATCH 4/5] UBI: introduce attach ioctls

[Artem Bityutskiy]

On Wed, 2007-12-19 at 15:17 +0100, Arnd Bergmann wrote:
> > +struct ubi_attach_req {
> > +       int32_t vid_hdr_offset;
> > +       int32_t data_offset;
> > +       int32_t mtd_num;
> > +       uint8_t padding[12];
> >  };
> 
> 
> Can you explain why you need to pass vid_hdr_offset /and/ data_offset here?
> What is the difference between the two? Can't you autoprobe them if you
> have the device?

vid_hdr_offset is the offset of the VID header withing an eraseblock.
data_offs is where the data starts. We want to be able to let users
select the VID header offset. Vs data offset - indeed it is redundant
and might be dropped because UBI may just assume data starts at the next
min. I/O unit after the VID header.

We can autoprobe the VID header offset, unless the MTD device is empty,
in which case UBI automatically formats it.

> The reason I'm asking is that I'd really like to make this a simple
> attribute in sysfs, in the mtd object. The question there is what a
> user would need to store into that attribute. The device is identified
> implicitly already, but this looks like you still need two distint
> integers in order to create an UBI device.

If the MTD device is already UBI-formatted, the numbers may be read from
the media. Otherwise, not.

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

[PATCH 0/5] UBI: make UBI devices dynamic

[Artem Bityutskiy]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 935 bytes --]

Hi,

here is a patchset which makes UBI devices dynamic. This means,
that you may attach/detach MTD devices runtime, not just when
UBI module is loaded/unloaded. This is very convinient - you do
may compile UBI into the kernel and attach/detach needed MTD
devices later, when your init scripts (loaded from initrd) decide
which exactly MTD device(s) to attach. This also makes testing
a lot easier.

The idea is similar to what the device mapper has: UBI creates
an UBI control device, which registers itself in UBI sysfs
hierarchy (/sys/class/ubi/ubi_ctrl) with dynamically allocated
major and minor numbers. The control device has 2 ioctls -
attach MTD device and detach MTD device.

This patchset applies on top of my other UBI changes and fixes,
which you may find in the UBI git.

I CC Frank and Andreas, as one of the main UBI users. I CC
Arnd as an ioctl expert.

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

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

[Artem Bityutskiy]

>From 4820ff7357b102cea37ec581d6d0ffd5a24348d2 Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Date: Sun, 16 Dec 2007 16:59:31 +0200
Subject: [PATCH] UBI: add UBI control device

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.

The next things to do are:
* Fix UBI, because it now assumes UBI devices cannot go away
* Implement control device ioctls which will attach/detach MTD
  devices

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 drivers/mtd/ubi/build.c |   69 ++++++++++++++++++++++++++++++++++++++++-------
 drivers/mtd/ubi/cdev.c  |   10 +++++++
 drivers/mtd/ubi/ubi.h   |    3 +-
 3 files changed, 71 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index b3efb2f..8d05d96 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -21,11 +21,16 @@
  */
 
 /*
- * This file includes UBI initialization and building of UBI devices. At the
- * moment UBI devices may only be added while UBI is initialized, but dynamic
- * device add/remove functionality is planned. Also, 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.
+ * This file includes UBI initialization and building of UBI devices.
+ *
+ * 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.
  */
 
 #include <linux/err.h>
@@ -70,6 +75,11 @@ struct kmem_cache *ubi_ltree_slab;
 /* Slab cache for wear-leveling entries */
 struct kmem_cache *ubi_wl_entry_slab;
 
+/* UBI control character device major number */
+static int ubi_ctrl_major;
+
+/* Linux device model object corresponding to the control UBI device */
+static struct device ubi_ctrl_dev;
 
 /* "Show" method for files in '/<sysfs>/class/ubi/' */
 static ssize_t ubi_version_show(struct class *class, char *buf)
@@ -142,6 +152,9 @@ static ssize_t dev_attribute_show(struct device *dev,
 /* Fake "release" method for UBI devices */
 static void dev_release(struct device *dev) { }
 
+/* Fake "release" method for UBI control device */
+static void ctrl_dev_release(struct device *dev) { }
+
 /**
  * ubi_sysfs_init - initialize sysfs for an UBI device.
  * @ubi: UBI device description object
@@ -701,19 +714,45 @@ static int __init ubi_init(void)
 		return -EINVAL;
 	}
 
+	/* Allocate major number for the UBI control device and register it */
+	ubi_ctrl_major = register_chrdev(0, "ubi_ctrl",
+					 &ubi_ctrl_cdev_operations);
+	if (ubi_ctrl_major < 0) {
+		err = ubi_ctrl_major;
+		printk(KERN_ERR "UBI error: cannot register control device\n");
+		return err;
+	}
+
+	/* Create base sysfs directory and sysfs files */
 	ubi_class = class_create(THIS_MODULE, UBI_NAME_STR);
-	if (IS_ERR(ubi_class))
-		return PTR_ERR(ubi_class);
+	if (IS_ERR(ubi_class)) {
+		err = PTR_ERR(ubi_class);
+		printk(KERN_ERR "UBI error: cannot create UBI class\n");
+		goto out_cdev_unreg;
+	}
 
 	err = class_create_file(ubi_class, &ubi_version);
-	if (err)
+	if (err) {
+		printk(KERN_ERR "UBI error: cannot create sysfs file\n");
 		goto out_class;
+	}
+
+	/* 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;
+	}
 
 	ubi_ltree_slab = kmem_cache_create("ubi_ltree_slab",
 					   sizeof(struct ubi_ltree_entry), 0,
 					   0, &ltree_entry_ctor);
 	if (!ubi_ltree_slab)
-		goto out_version;
+		goto out_dev_unreg;
 
 	ubi_wl_entry_slab = kmem_cache_create("ubi_wl_entry_slab",
 						sizeof(struct ubi_wl_entry),
@@ -727,8 +766,11 @@ static int __init ubi_init(void)
 
 		cond_resched();
 		err = attach_mtd_dev(p->name, p->vid_hdr_offs, p->data_offs);
-		if (err)
+		if (err) {
+			printk(KERN_ERR "UBI error: cannot attach %s\n",
+			       p->name);
 			goto out_detach;
+		}
 	}
 
 	return 0;
@@ -739,10 +781,15 @@ out_detach:
 	kmem_cache_destroy(ubi_wl_entry_slab);
 out_ltree:
 	kmem_cache_destroy(ubi_ltree_slab);
+out_dev_unreg:
+	device_unregister(&ubi_ctrl_dev);
 out_version:
 	class_remove_file(ubi_class, &ubi_version);
 out_class:
 	class_destroy(ubi_class);
+out_cdev_unreg:
+	unregister_chrdev(ubi_ctrl_major, "ubi_ctrl");
+	printk(KERN_ERR "UBI error: cannot initialize UBI, error %d\n", err);
 	return err;
 }
 module_init(ubi_init);
@@ -756,8 +803,10 @@ static void __exit ubi_exit(void)
 			detach_mtd_dev(ubi_devices[i]);
 	kmem_cache_destroy(ubi_wl_entry_slab);
 	kmem_cache_destroy(ubi_ltree_slab);
+	device_unregister(&ubi_ctrl_dev);
 	class_remove_file(ubi_class, &ubi_version);
 	class_destroy(ubi_class);
+	unregister_chrdev(ubi_ctrl_major, "ubi_ctrl");
 }
 module_exit(ubi_exit);
 
diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index 22c15a3..bc900d2 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -28,6 +28,11 @@
  *
  * Major and minor numbers are assigned dynamically to both UBI and volume
  * character devices.
+ *
+ * Well, there is the third kind of character devices - the UBI control
+ * character device, which allows to manipulate by UBI devices - create and
+ * delete them. In other words, it is used for attaching and detaching MTD
+ * devices.
  */
 
 #include <linux/module.h>
@@ -693,6 +698,11 @@ static int ubi_cdev_ioctl(struct inode *inode, struct file *file,
 	return err;
 }
 
+/* UBI control character device operations */
+struct file_operations ubi_ctrl_cdev_operations = {
+	.owner = THIS_MODULE,
+};
+
 /* UBI character device operations */
 struct file_operations ubi_cdev_operations = {
 	.owner = THIS_MODULE,
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 7eae2a9..28fbb5b 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -235,7 +235,7 @@ struct ubi_wl_entry;
 
 /**
  * struct ubi_device - UBI device description structure
- * @dev: class device object to use the the Linux device model
+ * @dev: UBI device object to use the the Linux device model
  * @cdev: character device object to create character device
  * @ubi_num: UBI device number
  * @ubi_name: UBI device name
@@ -398,6 +398,7 @@ struct ubi_device {
 
 extern struct kmem_cache *ubi_ltree_slab;
 extern struct kmem_cache *ubi_wl_entry_slab;
+extern struct file_operations ubi_ctrl_cdev_operations;
 extern struct file_operations ubi_cdev_operations;
 extern struct file_operations ubi_vol_cdev_operations;
 extern struct ubi_device *ubi_devices[];
-- 
1.5.3.4

[PATCH 2/5] UBI: add UBI devices reference counting

[Artem Bityutskiy]

>From 5f796dcd856305efdc78974681d96b9c06815a13 Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Date: Mon, 17 Dec 2007 17:37:26 +0200
Subject: [PATCH] UBI: add UBI devices reference counting

This is one more step on the way to "removable" UBI devices. It
adds reference counting for UBI devices. Every time a volume on
this device is opened - the device's refcount is increased. It
is also increased if someone is reading any sysfs file of this
UBI device or of one of its volumes.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 drivers/mtd/ubi/build.c |  142 ++++++++++++++++++++++++++++++++++++++++++-----
 drivers/mtd/ubi/cdev.c  |   34 +++--------
 drivers/mtd/ubi/eba.c   |    5 ++
 drivers/mtd/ubi/kapi.c  |   59 ++++++++++++++------
 drivers/mtd/ubi/ubi.h   |    9 +++-
 drivers/mtd/ubi/vmt.c   |   14 +++--
 drivers/mtd/ubi/wl.c    |    1 +
 7 files changed, 201 insertions(+), 63 deletions(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 8d05d96..e60a646 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -63,9 +63,6 @@ static int mtd_devs = 0;
 /* MTD devices specification parameters */
 static struct mtd_dev_param mtd_dev_param[UBI_MAX_DEVICES];
 
-/* All UBI devices in system */
-struct ubi_device *ubi_devices[UBI_MAX_DEVICES];
-
 /* Root UBI "class" object (corresponds to '/<sysfs>/class/ubi/') */
 struct class *ubi_class;
 
@@ -81,6 +78,12 @@ static int ubi_ctrl_major;
 /* Linux device model object corresponding to the control UBI device */
 static struct device ubi_ctrl_dev;
 
+/* All UBI devices in system */
+static struct ubi_device *ubi_devices[UBI_MAX_DEVICES];
+
+/* Protects @ubi_devices and @ubi->ref_count */
+static DEFINE_SPINLOCK(ubi_devices_lock);
+
 /* "Show" method for files in '/<sysfs>/class/ubi/' */
 static ssize_t ubi_version_show(struct class *class, char *buf)
 {
@@ -116,37 +119,145 @@ static struct device_attribute dev_min_io_size =
 static struct device_attribute dev_bgt_enabled =
 	__ATTR(bgt_enabled, S_IRUGO, dev_attribute_show, NULL);
 
+/**
+ * ubi_get_device - get UBI device.
+ * @ubi_num: UBI device number
+ *
+ * This function returns UBI device description object for UBI device number
+ * @ubi_num, or %NULL if the device does not exist. This function increases the
+ * device reference count to prevent removal of the device. In other words, the
+ * device cannot be removed if its reference count is not zero.
+ */
+struct ubi_device *ubi_get_device(int ubi_num)
+{
+	struct ubi_device *ubi;
+
+	spin_lock(&ubi_devices_lock);
+	ubi = ubi_devices[ubi_num];
+	if (ubi) {
+		ubi_assert(ubi->ref_count >= 0);
+		ubi->ref_count += 1;
+		get_device(&ubi->dev);
+	}
+	spin_unlock(&ubi_devices_lock);
+
+	return ubi;
+}
+
+/**
+ * ubi_put_device - drop an UBI device reference.
+ * @ubi: UBI device description object
+ */
+void ubi_put_device(struct ubi_device *ubi)
+{
+	spin_lock(&ubi_devices_lock);
+	ubi->ref_count -= 1;
+	put_device(&ubi->dev);
+	spin_unlock(&ubi_devices_lock);
+}
+
+/**
+ * ubi_get_by_major - get UBI device description object by character device
+ *                    major number.
+ * @major: major number
+ *
+ * This function is similar to 'ubi_get_device()', but it searches the device
+ * by its major number.
+ */
+struct ubi_device *ubi_get_by_major(int major)
+{
+	int i;
+	struct ubi_device *ubi;
+
+	spin_lock(&ubi_devices_lock);
+	for (i = 0; i < UBI_MAX_DEVICES; i++) {
+		ubi = ubi_devices[i];
+		if (ubi && MAJOR(ubi->cdev.dev) == major) {
+			ubi_assert(ubi->ref_count >= 0);
+			ubi->ref_count += 1;
+			get_device(&ubi->dev);
+			spin_unlock(&ubi_devices_lock);
+			return ubi;
+		}
+	}
+	spin_unlock(&ubi_devices_lock);
+
+	return NULL;
+}
+
+/**
+ * ubi_major2num - get UBI device number by character device major number.
+ * @major: major number
+ *
+ * This function searches UBI device number object by its major number. If UBI
+ * device was not found, this function returns -ENODEV, othewise the UBI device
+ * number is returned.
+ */
+int ubi_major2num(int major)
+{
+	int i, ubi_num = -ENODEV;
+
+	spin_lock(&ubi_devices_lock);
+	for (i = 0; i < UBI_MAX_DEVICES; i++) {
+		struct ubi_device *ubi = ubi_devices[i];
+
+		if (ubi && MAJOR(ubi->cdev.dev) == major) {
+			ubi_num = ubi->ubi_num;
+			break;
+		}
+	}
+	spin_unlock(&ubi_devices_lock);
+
+	return ubi_num;
+}
+
 /* "Show" method for files in '/<sysfs>/class/ubi/ubiX/' */
 static ssize_t dev_attribute_show(struct device *dev,
 				  struct device_attribute *attr, char *buf)
 {
-	const struct ubi_device *ubi;
+	ssize_t ret;
+	struct ubi_device *ubi;
 
+	/*
+	 * The below code looks weird, but it actually makes sense. We get the
+	 * UBI device reference from the contained 'struct ubi_device'. But it
+	 * is unclear if the device was removed or not yet. Indeed, if the
+	 * device was removed before we increased its reference count,
+	 * 'ubi_get_device()' will return -ENODEV and we fail.
+	 *
+	 * Remember, 'struct ubi_device' is freed in the release function, so
+	 * we still can use 'ubi->ubi_num'.
+	 */
 	ubi = container_of(dev, struct ubi_device, dev);
+	ubi = ubi_get_device(ubi->ubi_num);
+	if (!ubi)
+		return -ENODEV;
+
 	if (attr == &dev_eraseblock_size)
-		return sprintf(buf, "%d\n", ubi->leb_size);
+		ret = sprintf(buf, "%d\n", ubi->leb_size);
 	else if (attr == &dev_avail_eraseblocks)
-		return sprintf(buf, "%d\n", ubi->avail_pebs);
+		ret = sprintf(buf, "%d\n", ubi->avail_pebs);
 	else if (attr == &dev_total_eraseblocks)
-		return sprintf(buf, "%d\n", ubi->good_peb_count);
+		ret = sprintf(buf, "%d\n", ubi->good_peb_count);
 	else if (attr == &dev_volumes_count)
-		return sprintf(buf, "%d\n", ubi->vol_count);
+		ret = sprintf(buf, "%d\n", ubi->vol_count);
 	else if (attr == &dev_max_ec)
-		return sprintf(buf, "%d\n", ubi->max_ec);
+		ret = sprintf(buf, "%d\n", ubi->max_ec);
 	else if (attr == &dev_reserved_for_bad)
-		return sprintf(buf, "%d\n", ubi->beb_rsvd_pebs);
+		ret = sprintf(buf, "%d\n", ubi->beb_rsvd_pebs);
 	else if (attr == &dev_bad_peb_count)
-		return sprintf(buf, "%d\n", ubi->bad_peb_count);
+		ret = sprintf(buf, "%d\n", ubi->bad_peb_count);
 	else if (attr == &dev_max_vol_count)
-		return sprintf(buf, "%d\n", ubi->vtbl_slots);
+		ret = sprintf(buf, "%d\n", ubi->vtbl_slots);
 	else if (attr == &dev_min_io_size)
-		return sprintf(buf, "%d\n", ubi->min_io_size);
+		ret = sprintf(buf, "%d\n", ubi->min_io_size);
 	else if (attr == &dev_bgt_enabled)
-		return sprintf(buf, "%d\n", ubi->thread_enabled);
+		ret = sprintf(buf, "%d\n", ubi->thread_enabled);
 	else
 		BUG();
 
-	return 0;
+	ubi_put_device(ubi);
+	return ret;
 }
 
 /* Fake "release" method for UBI devices */
@@ -671,6 +782,7 @@ static void detach_mtd_dev(struct ubi_device *ubi)
 	int ubi_num = ubi->ubi_num, mtd_num = ubi->mtd->index;
 
 	dbg_msg("detaching mtd%d from ubi%d", ubi->mtd->index, ubi_num);
+	ubi_assert(ubi->ref_count == 0);
 	uif_close(ubi);
 	ubi_eba_close(ubi);
 	ubi_wl_close(ubi);
diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index bc900d2..01978b5 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -56,23 +56,6 @@
 #endif
 
 /**
- * major_to_device - get UBI device object by character device major number.
- * @major: major number
- *
- * This function returns a pointer to the UBI device object.
- */
-static struct ubi_device *major_to_device(int major)
-{
-	int i;
-
-	for (i = 0; i < UBI_MAX_DEVICES; i++)
-		if (ubi_devices[i] && MAJOR(ubi_devices[i]->cdev.dev) == major)
-			return ubi_devices[i];
-	BUG();
-	return NULL;
-}
-
-/**
  * get_exclusive - get exclusive access to an UBI volume.
  * @desc: volume descriptor
  *
@@ -129,9 +112,11 @@ static void revoke_exclusive(struct ubi_volume_desc *desc, int mode)
 static int vol_cdev_open(struct inode *inode, struct file *file)
 {
 	struct ubi_volume_desc *desc;
-	const struct ubi_device *ubi = major_to_device(imajor(inode));
-	int vol_id = iminor(inode) - 1;
-	int mode;
+	int vol_id = iminor(inode) - 1, mode, ubi_num;
+
+	ubi_num = ubi_major2num(imajor(inode));
+	if (ubi_num < 0)
+		return ubi_num;
 
 	if (file->f_mode & FMODE_WRITE)
 		mode = UBI_READWRITE;
@@ -140,7 +125,7 @@ static int vol_cdev_open(struct inode *inode, struct file *file)
 
 	dbg_msg("open volume %d, mode %d", vol_id, mode);
 
-	desc = ubi_open_volume(ubi->ubi_num, vol_id, mode);
+	desc = ubi_open_volume(ubi_num, vol_id, mode);
 	if (IS_ERR(desc))
 		return PTR_ERR(desc);
 
@@ -586,9 +571,9 @@ static int ubi_cdev_ioctl(struct inode *inode, struct file *file,
 	if (!capable(CAP_SYS_RESOURCE))
 		return -EPERM;
 
-	ubi = major_to_device(imajor(inode));
-	if (IS_ERR(ubi))
-		return PTR_ERR(ubi);
+	ubi = ubi_get_by_major(imajor(inode));
+	if (!ubi)
+		return -ENODEV;
 
 	switch (cmd) {
 	/* Create volume command */
@@ -695,6 +680,7 @@ static int ubi_cdev_ioctl(struct inode *inode, struct file *file,
 		break;
 	}
 
+	ubi_put_device(ubi);
 	return err;
 }
 
diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index deb348d..677d779 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -339,6 +339,7 @@ int ubi_eba_unmap_leb(struct ubi_device *ubi, struct ubi_volume *vol,
 {
 	int err, pnum, vol_id = vol->vol_id;
 
+	ubi_assert(ubi->ref_count > 0);
 	ubi_assert(vol->ref_count > 0);
 
 	if (ubi->ro_mode)
@@ -389,6 +390,7 @@ int ubi_eba_read_leb(struct ubi_device *ubi, struct ubi_volume *vol, int lnum,
 	struct ubi_vid_hdr *vid_hdr;
 	uint32_t uninitialized_var(crc);
 
+	ubi_assert(ubi->ref_count > 0);
 	ubi_assert(vol->ref_count > 0);
 
 	err = leb_read_lock(ubi, vol_id, lnum);
@@ -614,6 +616,7 @@ int ubi_eba_write_leb(struct ubi_device *ubi, struct ubi_volume *vol, int lnum,
 	int err, pnum, tries = 0, vol_id = vol->vol_id;
 	struct ubi_vid_hdr *vid_hdr;
 
+	ubi_assert(ubi->ref_count > 0);
 	ubi_assert(vol->ref_count > 0);
 
 	if (ubi->ro_mode)
@@ -749,6 +752,7 @@ int ubi_eba_write_leb_st(struct ubi_device *ubi, struct ubi_volume *vol,
 	struct ubi_vid_hdr *vid_hdr;
 	uint32_t crc;
 
+	ubi_assert(ubi->ref_count > 0);
 	ubi_assert(vol->ref_count > 0);
 
 	if (ubi->ro_mode)
@@ -865,6 +869,7 @@ int ubi_eba_atomic_leb_change(struct ubi_device *ubi, struct ubi_volume *vol,
 	struct ubi_vid_hdr *vid_hdr;
 	uint32_t crc;
 
+	ubi_assert(ubi->ref_count > 0);
 	ubi_assert(vol->ref_count > 0);
 
 	if (ubi->ro_mode)
diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c
index 780c273..4ec3a33 100644
--- a/drivers/mtd/ubi/kapi.c
+++ b/drivers/mtd/ubi/kapi.c
@@ -30,23 +30,27 @@
  * @ubi_num: UBI device number
  * @di: the information is stored here
  *
- * This function returns %0 in case of success and a %-ENODEV if there is no
- * such UBI device.
+ * This function returns %0 in case of success, %-EINVAL if the UBI device
+ * number is invalid, and %-ENODEV if there is no such UBI device.
  */
 int ubi_get_device_info(int ubi_num, struct ubi_device_info *di)
 {
-	const struct ubi_device *ubi;
+	struct ubi_device *ubi;
 
-	if (ubi_num < 0 || ubi_num >= UBI_MAX_DEVICES ||
-	    !ubi_devices[ubi_num])
+	if (ubi_num < 0 || ubi_num >= UBI_MAX_DEVICES)
+		return -EINVAL;
+
+	ubi = ubi_get_device(ubi_num);
+	if (!ubi)
 		return -ENODEV;
 
-	ubi = ubi_devices[ubi_num];
 	di->ubi_num = ubi->ubi_num;
 	di->leb_size = ubi->leb_size;
 	di->min_io_size = ubi->min_io_size;
 	di->ro_mode = ubi->ro_mode;
 	di->cdev = ubi->cdev.dev;
+
+	ubi_put_device(ubi);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(ubi_get_device_info);
@@ -111,16 +115,23 @@ struct ubi_volume_desc *ubi_open_volume(int ubi_num, int vol_id, int mode)
 	    mode != UBI_EXCLUSIVE)
 		return ERR_PTR(-EINVAL);
 
-	ubi = ubi_devices[ubi_num];
+	/*
+	 * First of all, we have to get the UBI device to prevent its removal.
+	 */
+	ubi = ubi_get_device(ubi_num);
 	if (!ubi)
 		return ERR_PTR(-ENODEV);
 
-	if (vol_id < 0 || vol_id >= ubi->vtbl_slots)
-		return ERR_PTR(-EINVAL);
+	if (vol_id < 0 || vol_id >= ubi->vtbl_slots) {
+		err = -EINVAL;
+		goto out_put_ubi;
+	}
 
 	desc = kmalloc(sizeof(struct ubi_volume_desc), GFP_KERNEL);
-	if (!desc)
-		return ERR_PTR(-ENOMEM);
+	if (!desc) {
+		err = -ENOMEM;
+		goto out_put_ubi;
+	}
 
 	err = -ENODEV;
 	if (!try_module_get(THIS_MODULE))
@@ -188,6 +199,8 @@ out_unlock:
 	module_put(THIS_MODULE);
 out_free:
 	kfree(desc);
+out_put_ubi:
+	ubi_put_device(ubi);
 	return ERR_PTR(err);
 }
 EXPORT_SYMBOL_GPL(ubi_open_volume);
@@ -205,6 +218,7 @@ struct ubi_volume_desc *ubi_open_volume_nm(int ubi_num, const char *name,
 {
 	int i, vol_id = -1, len;
 	struct ubi_device *ubi;
+	struct ubi_volume_desc *ret;
 
 	dbg_msg("open volume %s, mode %d", name, mode);
 
@@ -218,7 +232,7 @@ struct ubi_volume_desc *ubi_open_volume_nm(int ubi_num, const char *name,
 	if (ubi_num < 0 || ubi_num >= UBI_MAX_DEVICES)
 		return ERR_PTR(-EINVAL);
 
-	ubi = ubi_devices[ubi_num];
+	ubi = ubi_get_device(ubi_num);
 	if (!ubi)
 		return ERR_PTR(-ENODEV);
 
@@ -234,10 +248,17 @@ struct ubi_volume_desc *ubi_open_volume_nm(int ubi_num, const char *name,
 	}
 	spin_unlock(&ubi->volumes_lock);
 
-	if (vol_id < 0)
-		return ERR_PTR(-ENODEV);
+	if (vol_id >= 0)
+		ret = ubi_open_volume(ubi_num, vol_id, mode);
+	else
+		ret = ERR_PTR(-ENODEV);
 
-	return ubi_open_volume(ubi_num, vol_id, mode);
+	/*
+	 * We should put the UBI device even in case of success, because
+	 * 'ubi_open_volume()' took a reference as well.
+	 */
+	ubi_put_device(ubi);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(ubi_open_volume_nm);
 
@@ -248,10 +269,11 @@ EXPORT_SYMBOL_GPL(ubi_open_volume_nm);
 void ubi_close_volume(struct ubi_volume_desc *desc)
 {
 	struct ubi_volume *vol = desc->vol;
+	struct ubi_device *ubi = vol->ubi;
 
 	dbg_msg("close volume %d, mode %d", vol->vol_id, desc->mode);
 
-	spin_lock(&vol->ubi->volumes_lock);
+	spin_lock(&ubi->volumes_lock);
 	switch (desc->mode) {
 	case UBI_READONLY:
 		vol->readers -= 1;
@@ -263,10 +285,11 @@ void ubi_close_volume(struct ubi_volume_desc *desc)
 		vol->exclusive = 0;
 	}
 	vol->ref_count -= 1;
-	spin_unlock(&vol->ubi->volumes_lock);
+	spin_unlock(&ubi->volumes_lock);
 
-	put_device(&vol->dev);
 	kfree(desc);
+	put_device(&vol->dev);
+	ubi_put_device(ubi);
 	module_put(THIS_MODULE);
 }
 EXPORT_SYMBOL_GPL(ubi_close_volume);
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 28fbb5b..da88abd 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -245,6 +245,7 @@ struct ubi_wl_entry;
  *                @beb_rsvd_level, @bad_peb_count, @good_peb_count, @vol_count,
  *                @vol->readers, @vol->writers, @vol->exclusive,
  *                @vol->ref_count, @vol->mapping and @vol->eba_tbl.
+ * @ref_count: count of references on the UBI device
  *
  * @rsvd_pebs: count of reserved physical eraseblocks
  * @avail_pebs: count of available physical eraseblocks
@@ -325,6 +326,7 @@ struct ubi_device {
 	int vol_count;
 	struct ubi_volume *volumes[UBI_MAX_VOLUMES+UBI_INT_VOL_COUNT];
 	spinlock_t volumes_lock;
+	int ref_count;
 
 	int rsvd_pebs;
 	int avail_pebs;
@@ -401,7 +403,6 @@ extern struct kmem_cache *ubi_wl_entry_slab;
 extern struct file_operations ubi_ctrl_cdev_operations;
 extern struct file_operations ubi_cdev_operations;
 extern struct file_operations ubi_vol_cdev_operations;
-extern struct ubi_device *ubi_devices[];
 extern struct class *ubi_class;
 
 /* vtbl.c */
@@ -479,6 +480,12 @@ int ubi_io_read_vid_hdr(struct ubi_device *ubi, int pnum,
 int ubi_io_write_vid_hdr(struct ubi_device *ubi, int pnum,
 			 struct ubi_vid_hdr *vid_hdr);
 
+/* build.c */
+struct ubi_device *ubi_get_device(int ubi_num);
+void ubi_put_device(struct ubi_device *ubi);
+struct ubi_device *ubi_get_by_major(int major);
+int ubi_major2num(int major);
+
 /*
  * ubi_rb_for_each_entry - walk an RB-tree.
  * @rb: a pointer to type 'struct rb_node' to to use as a loop counter
diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
index 3ed63dc..42d3dd7 100644
--- a/drivers/mtd/ubi/vmt.c
+++ b/drivers/mtd/ubi/vmt.c
@@ -71,11 +71,16 @@ static ssize_t vol_attribute_show(struct device *dev,
 {
 	int ret;
 	struct ubi_volume *vol = container_of(dev, struct ubi_volume, dev);
-	struct ubi_device *ubi = vol->ubi;
+	struct ubi_device *ubi;
+
+	ubi = ubi_get_device(vol->ubi->ubi_num);
+	if (!ubi)
+		return -ENODEV;
 
 	spin_lock(&ubi->volumes_lock);
 	if (!ubi->volumes[vol->vol_id]) {
 		spin_unlock(&ubi->volumes_lock);
+		ubi_put_device(ubi);
 		return -ENODEV;
 	}
 	/* Take a reference to prevent volume removal */
@@ -108,10 +113,12 @@ static ssize_t vol_attribute_show(struct device *dev,
 		/* This must be a bug */
 		ret = -EINVAL;
 
+	/* We've done the operation, drop volume and UBI device references */
 	spin_lock(&ubi->volumes_lock);
 	vol->ref_count -= 1;
 	ubi_assert(vol->ref_count >= 0);
 	spin_unlock(&ubi->volumes_lock);
+	ubi_put_device(ubi);
 	return ret;
 }
 
@@ -260,6 +267,7 @@ int ubi_create_volume(struct ubi_device *ubi, struct ubi_mkvol_req *req)
 	}
 	ubi->avail_pebs -= vol->reserved_pebs;
 	ubi->rsvd_pebs += vol->reserved_pebs;
+	spin_unlock(&ubi->volumes_lock);
 
 	vol->vol_id    = vol_id;
 	vol->alignment = req->alignment;
@@ -267,9 +275,7 @@ int ubi_create_volume(struct ubi_device *ubi, struct ubi_mkvol_req *req)
 	vol->vol_type  = req->vol_type;
 	vol->name_len  = req->name_len;
 	memcpy(vol->name, req->name, vol->name_len + 1);
-	vol->exclusive = 1;
 	vol->ubi = ubi;
-	spin_unlock(&ubi->volumes_lock);
 
 	/*
 	 * Finish all pending erases because there may be some LEBs belonging
@@ -350,8 +356,6 @@ int ubi_create_volume(struct ubi_device *ubi, struct ubi_mkvol_req *req)
 		goto out_sysfs;
 
 	spin_lock(&ubi->volumes_lock);
-	ubi->vol_count += 1;
-	vol->exclusive = 0;
 	ubi->volumes[vol_id] = vol;
 	spin_unlock(&ubi->volumes_lock);
 
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index f90416a..0dc7dc8 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -1246,6 +1246,7 @@ int ubi_wl_flush(struct ubi_device *ubi)
 	 * Make sure all the works which have been done in parallel are
 	 * finished.
 	 */
+	ubi_assert(ubi->ref_count > 0);
 	down_write(&ubi->work_sem);
 	up_write(&ubi->work_sem);
 
-- 
1.5.3.4

[PATCH 3/5] UBI: prepare attach and detach functions

[Artem Bityutskiy]

>From 8f4ccdb03cb35b00afd08d09391dc86ee485cd42 Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Date: Mon, 17 Dec 2007 20:33:20 +0200
Subject: [PATCH] UBI: prepare attach and detach functions

Prepare the attach and detach functions to by used outside of
module initialization:

* detach function checks reference count before detaching
* it kills the background thread as well

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 drivers/mtd/ubi/build.c |  232 +++++++++++++++++++++++++++++++++--------------
 drivers/mtd/ubi/ubi.h   |    5 +
 drivers/mtd/ubi/wl.c    |   16 +---
 3 files changed, 169 insertions(+), 84 deletions(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index e60a646..088d250 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -39,6 +39,7 @@
 #include <linux/stringify.h>
 #include <linux/stat.h>
 #include <linux/log2.h>
+#include <linux/kthread.h>
 #include "ubi.h"
 
 /* Maximum length of the 'mtd=' parameter */
@@ -81,6 +82,9 @@ static struct device ubi_ctrl_dev;
 /* All UBI devices in system */
 static struct ubi_device *ubi_devices[UBI_MAX_DEVICES];
 
+/* Serializes UBI devices creations and removals */
+DEFINE_MUTEX(ubi_devices_mutex);
+
 /* Protects @ubi_devices and @ubi->ref_count */
 static DEFINE_SPINLOCK(ubi_devices_lock);
 
@@ -190,7 +194,7 @@ struct ubi_device *ubi_get_by_major(int major)
  * @major: major number
  *
  * This function searches UBI device number object by its major number. If UBI
- * device was not found, this function returns -ENODEV, othewise the UBI device
+ * device was not found, this function returns -ENODEV, otherwise the UBI device
  * number is returned.
  */
 int ubi_major2num(int major)
@@ -617,84 +621,66 @@ static int io_init(struct ubi_device *ubi)
 }
 
 /**
- * attach_mtd_dev - attach an MTD device.
- * @mtd_dev: MTD device name or number string
+ * ubi_attach_mtd_dev - attach an MTD device.
+ * @mtd_dev: MTD device description object
  * @vid_hdr_offset: VID header offset
  * @data_offset: data offset
  *
  * This function attaches an MTD device to UBI. It first treats @mtd_dev as the
  * MTD device name, and tries to open it by this name. If it is unable to open,
  * it tries to convert @mtd_dev to an integer and open the MTD device by its
- * number. Returns zero in case of success and a negative error code in case of
- * failure.
+ * number. Returns new UBI device's number in case of success and a negative
+ * error code in case of failure.
+ *
+ * Note, the invocations of this function has to be serialized by the
+ * @ubi_devices_mutex.
  */
-static int attach_mtd_dev(const char *mtd_dev, int vid_hdr_offset,
-			  int data_offset)
+int ubi_attach_mtd_dev(struct mtd_info *mtd, int vid_hdr_offset,
+		       int data_offset)
 {
 	struct ubi_device *ubi;
-	struct mtd_info *mtd;
 	int i, err;
 
-	mtd = get_mtd_device_nm(mtd_dev);
-	if (IS_ERR(mtd)) {
-		int mtd_num;
-		char *endp;
-
-		if (PTR_ERR(mtd) != -ENODEV)
-			return PTR_ERR(mtd);
-
-		/*
-		 * Probably this is not MTD device name but MTD device number -
-		 * check this out.
-		 */
-		mtd_num = simple_strtoul(mtd_dev, &endp, 0);
-		if (*endp != '\0' || mtd_dev == endp) {
-			ubi_err("incorrect MTD device: \"%s\"", mtd_dev);
-			return -ENODEV;
-		}
-
-		mtd = get_mtd_device(NULL, mtd_num);
-		if (IS_ERR(mtd))
-			return PTR_ERR(mtd);
-	}
+	/* Do simple request verification, more is done by 'io_init()' */
+	if (vid_hdr_offset < 0 || data_offset < vid_hdr_offset)
+		return -EINVAL;
 
-	/* Check if we already have the same MTD device attached */
+	/*
+	 * Check if we already have the same MTD device attached.
+	 *
+	 * Note, this function assumes that UBI devices creations and deletions
+	 * are serialized, so it does not take the &ubi_devices_lock.
+	 */
 	for (i = 0; i < UBI_MAX_DEVICES; i++)
 		ubi = ubi_devices[i];
-		if (ubi && ubi->mtd->index == mtd->index) {
+		if (ubi && mtd->index == ubi->mtd->index) {
 			ubi_err("mtd%d is already attached to ubi%d",
 				mtd->index, i);
-			err = -EINVAL;
-			goto out_mtd;
+			return -EINVAL;
 		}
 
-	ubi = kzalloc(sizeof(struct ubi_device), GFP_KERNEL);
-	if (!ubi) {
-		err = -ENOMEM;
-		goto out_mtd;
-	}
-
-	ubi->mtd = mtd;
-
 	/* Search for an empty slot in the @ubi_devices array */
-	ubi->ubi_num = -1;
 	for (i = 0; i < UBI_MAX_DEVICES; i++)
-		if (!ubi_devices[i]) {
-			ubi->ubi_num = i;
+		if (!ubi_devices[i])
 			break;
-		}
 
-	if (ubi->ubi_num == -1) {
+	if (i == UBI_MAX_DEVICES) {
 		ubi_err("only %d UBI devices may be created", UBI_MAX_DEVICES);
-		err = -ENFILE;
-		goto out_free;
+		return -ENFILE;
 	}
 
-	dbg_msg("attaching mtd%d to ubi%d: VID header offset %d data offset %d",
-		ubi->mtd->index, ubi->ubi_num, vid_hdr_offset, data_offset);
+	ubi = kzalloc(sizeof(struct ubi_device), GFP_KERNEL);
+	if (!ubi)
+		return -ENOMEM;
 
+	ubi->mtd = mtd;
+	ubi->ubi_num = i;
 	ubi->vid_hdr_offset = vid_hdr_offset;
 	ubi->leb_start = data_offset;
+
+	dbg_msg("attaching mtd%d to ubi%d: VID header offset %d data offset %d",
+		mtd->index, ubi->ubi_num, vid_hdr_offset, data_offset);
+
 	err = io_init(ubi);
 	if (err)
 		goto out_free;
@@ -725,8 +711,16 @@ static int attach_mtd_dev(const char *mtd_dev, int vid_hdr_offset,
 	if (err)
 		goto out_detach;
 
-	ubi_msg("attached mtd%d to ubi%d", ubi->mtd->index, ubi->ubi_num);
-	ubi_msg("MTD device name:            \"%s\"", ubi->mtd->name);
+	ubi->bgt_thread = kthread_create(ubi_thread, ubi, ubi->bgt_name);
+	if (IS_ERR(ubi->bgt_thread)) {
+		err = PTR_ERR(ubi->bgt_thread);
+		ubi_err("cannot spawn \"%s\", error %d", ubi->bgt_name,
+			err);
+		goto out_uif;
+	}
+
+	ubi_msg("attached mtd%d to ubi%d", mtd->index, ubi->ubi_num);
+	ubi_msg("MTD device name:            \"%s\"", mtd->name);
 	ubi_msg("MTD device size:            %llu MiB", ubi->flash_size >> 20);
 	ubi_msg("physical eraseblock size:   %d bytes (%d KiB)",
 		ubi->peb_size, ubi->peb_size >> 10);
@@ -754,9 +748,13 @@ static int attach_mtd_dev(const char *mtd_dev, int vid_hdr_offset,
 		wake_up_process(ubi->bgt_thread);
 	}
 
+	err = ubi->ubi_num;
 	ubi_devices[ubi->ubi_num] = ubi;
-	return 0;
 
+	return err;
+
+out_uif:
+	uif_close(ubi);
 out_detach:
 	ubi_eba_close(ubi);
 	ubi_wl_close(ubi);
@@ -768,21 +766,57 @@ out_free:
 	vfree(ubi->dbg_peb_buf);
 #endif
 	kfree(ubi);
-out_mtd:
-	put_mtd_device(mtd);
 	return err;
 }
 
 /**
- * detach_mtd_dev - detach an MTD device.
- * @ubi: UBI device description object
+ * ubi_detach_mtd_dev - detach an MTD device.
+ * @ubi_num: UBI device number to detach from
+ * @anyway: detach MTD even if device reference count is not zero
+ *
+ * This function destroys an UBI device number @ubi_num and detaches the
+ * underlying MTD device. Returns zero in case of success and %-EBUSY if the
+ * UBI device is busy and cannot be destroyed, and %-EINVAL if it does not
+ * exist.
+ *
+ * Note, the invocations of this function has to be serialized by the
+ * @ubi_devices_mutex.
  */
-static void detach_mtd_dev(struct ubi_device *ubi)
+int ubi_detach_mtd_dev(int ubi_num, int anyway)
 {
-	int ubi_num = ubi->ubi_num, mtd_num = ubi->mtd->index;
+	struct ubi_device *ubi;
+
+	if (ubi_num < 0 || ubi_num >= UBI_MAX_DEVICES)
+		return -EINVAL;
+
+	spin_lock(&ubi_devices_lock);
+	ubi = ubi_devices[ubi_num];
+	if (!ubi) {
+		spin_lock(&ubi_devices_lock);
+		return -EINVAL;
+	}
+
+	if (ubi->ref_count) {
+		if (!anyway) {
+			spin_lock(&ubi_devices_lock);
+			return -EBUSY;
+		}
+		/* This may only happen if there is a bug */
+		ubi_err("%s reference count %d, destroy anyway",
+			ubi->ubi_name, ubi->ref_count);
+	}
+	ubi_devices[ubi->ubi_num] = NULL;
+	spin_unlock(&ubi_devices_lock);
+
+	dbg_msg("detaching mtd%d from ubi%d", ubi->mtd->index, ubi->ubi_num);
+
+	/*
+	 * Before freeing anything, we have to stop the background thread to
+	 * prevent it from doing anything on this device while we are freeing.
+	 */
+	if (ubi->bgt_thread)
+		kthread_stop(ubi->bgt_thread);
 
-	dbg_msg("detaching mtd%d from ubi%d", ubi->mtd->index, ubi_num);
-	ubi_assert(ubi->ref_count == 0);
 	uif_close(ubi);
 	ubi_eba_close(ubi);
 	ubi_wl_close(ubi);
@@ -793,9 +827,9 @@ static void detach_mtd_dev(struct ubi_device *ubi)
 #ifdef CONFIG_MTD_UBI_DEBUG
 	vfree(ubi->dbg_peb_buf);
 #endif
-	kfree(ubi_devices[ubi_num]);
-	ubi_devices[ubi_num] = NULL;
-	ubi_msg("mtd%d is detached from ubi%d", mtd_num, ubi_num);
+	ubi_msg("mtd%d is detached from ubi%d", ubi->mtd->index, ubi->ubi_num);
+	kfree(ubi);
+	return 0;
 }
 
 /**
@@ -812,6 +846,46 @@ static void ltree_entry_ctor(struct kmem_cache *cache, void *obj)
 	init_rwsem(&le->mutex);
 }
 
+/**
+ * find_mtd_device - open an MTD device by its name or number.
+ * @mtd_dev: name or number of the device
+ *
+ * This function tries to open and MTD device with name @mtd_dev, and if it
+ * fails, then it tries to interpret the @mtd_dev string as an ASCII-coded
+ * integer and open an MTD device with this number. Returns MTD device
+ * description object in case of success and a negative error code in case of
+ * failure.
+ */
+static struct mtd_info * __init open_mtd_device(const char *mtd_dev)
+{
+	struct mtd_info *mtd;
+
+	mtd = get_mtd_device_nm(mtd_dev);
+	if (IS_ERR(mtd)) {
+		int mtd_num;
+		char *endp;
+
+		if (PTR_ERR(mtd) != -ENODEV)
+			return mtd;
+
+		/*
+		 * Probably this is not MTD device name but MTD device number -
+		 * check this out.
+		 */
+		mtd_num = simple_strtoul(mtd_dev, &endp, 0);
+		if (*endp != '\0' || mtd_dev == endp) {
+			ubi_err("incorrect MTD device: \"%s\"", mtd_dev);
+			return ERR_PTR(-ENODEV);
+		}
+
+		mtd = get_mtd_device(NULL, mtd_num);
+		if (IS_ERR(mtd))
+			return mtd;
+	}
+
+	return mtd;
+}
+
 static int __init ubi_init(void)
 {
 	int err, i, k;
@@ -875,10 +949,21 @@ static int __init ubi_init(void)
 	/* Attach MTD devices */
 	for (i = 0; i < mtd_devs; i++) {
 		struct mtd_dev_param *p = &mtd_dev_param[i];
+		struct mtd_info *mtd;
 
 		cond_resched();
-		err = attach_mtd_dev(p->name, p->vid_hdr_offs, p->data_offs);
-		if (err) {
+
+		mtd = open_mtd_device(p->name);
+		if (IS_ERR(mtd)) {
+			err = PTR_ERR(mtd);
+			goto out_detach;
+		}
+
+		mutex_lock(&ubi_devices_mutex);
+		err = ubi_attach_mtd_dev(mtd, p->vid_hdr_offs, p->data_offs);
+		mutex_unlock(&ubi_devices_mutex);
+		if (err < 0) {
+			put_mtd_device(mtd);
 			printk(KERN_ERR "UBI error: cannot attach %s\n",
 			       p->name);
 			goto out_detach;
@@ -889,7 +974,11 @@ static int __init ubi_init(void)
 
 out_detach:
 	for (k = 0; k < i; k++)
-		detach_mtd_dev(ubi_devices[k]);
+		if (ubi_devices[k]) {
+			mutex_lock(&ubi_devices_mutex);
+			ubi_detach_mtd_dev(ubi_devices[k]->ubi_num, 1);
+			mutex_unlock(&ubi_devices_mutex);
+		}
 	kmem_cache_destroy(ubi_wl_entry_slab);
 out_ltree:
 	kmem_cache_destroy(ubi_ltree_slab);
@@ -911,8 +1000,11 @@ static void __exit ubi_exit(void)
 	int i;
 
 	for (i = 0; i < UBI_MAX_DEVICES; i++)
-		if (ubi_devices[i])
-			detach_mtd_dev(ubi_devices[i]);
+		if (ubi_devices[i]) {
+			mutex_lock(&ubi_devices_mutex);
+			ubi_detach_mtd_dev(ubi_devices[i]->ubi_num, 1);
+			mutex_unlock(&ubi_devices_mutex);
+		}
 	kmem_cache_destroy(ubi_wl_entry_slab);
 	kmem_cache_destroy(ubi_ltree_slab);
 	device_unregister(&ubi_ctrl_dev);
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index da88abd..49f87f4 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -404,6 +404,7 @@ extern struct file_operations ubi_ctrl_cdev_operations;
 extern struct file_operations ubi_cdev_operations;
 extern struct file_operations ubi_vol_cdev_operations;
 extern struct class *ubi_class;
+extern struct mutex ubi_devices_mutex;
 
 /* vtbl.c */
 int ubi_change_vtbl_record(struct ubi_device *ubi, int idx,
@@ -462,6 +463,7 @@ int ubi_wl_flush(struct ubi_device *ubi);
 int ubi_wl_scrub_peb(struct ubi_device *ubi, int pnum);
 int ubi_wl_init_scan(struct ubi_device *ubi, struct ubi_scan_info *si);
 void ubi_wl_close(struct ubi_device *ubi);
+int ubi_thread(void *u);
 
 /* io.c */
 int ubi_io_read(const struct ubi_device *ubi, void *buf, int pnum, int offset,
@@ -481,6 +483,9 @@ int ubi_io_write_vid_hdr(struct ubi_device *ubi, int pnum,
 			 struct ubi_vid_hdr *vid_hdr);
 
 /* build.c */
+int ubi_attach_mtd_dev(struct mtd_info *mtd, int vid_hdr_offset,
+		       int data_offset);
+int ubi_detach_mtd_dev(int ubi_num, int anyway);
 struct ubi_device *ubi_get_device(int ubi_num);
 void ubi_put_device(struct ubi_device *ubi);
 struct ubi_device *ubi_get_by_major(int major);
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 0dc7dc8..11d4e50 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -1299,7 +1299,7 @@ static void tree_destroy(struct rb_root *root)
  * ubi_thread - UBI background thread.
  * @u: the UBI device description object pointer
  */
-static int ubi_thread(void *u)
+int ubi_thread(void *u)
 {
 	int failures = 0;
 	struct ubi_device *ubi = u;
@@ -1397,18 +1397,10 @@ int ubi_wl_init_scan(struct ubi_device *ubi, struct ubi_scan_info *si)
 
 	sprintf(ubi->bgt_name, UBI_BGT_NAME_PATTERN, ubi->ubi_num);
 
-	ubi->bgt_thread = kthread_create(ubi_thread, ubi, ubi->bgt_name);
-	if (IS_ERR(ubi->bgt_thread)) {
-		err = PTR_ERR(ubi->bgt_thread);
-		ubi_err("cannot spawn \"%s\", error %d", ubi->bgt_name,
-			err);
-		return err;
-	}
-
 	err = -ENOMEM;
 	ubi->lookuptbl = kzalloc(ubi->peb_count * sizeof(void *), GFP_KERNEL);
 	if (!ubi->lookuptbl)
-		goto out_free;
+		return err;
 
 	list_for_each_entry_safe(seb, tmp, &si->erase, u.list) {
 		cond_resched();
@@ -1541,10 +1533,6 @@ static void protection_trees_destroy(struct ubi_device *ubi)
  */
 void ubi_wl_close(struct ubi_device *ubi)
 {
-	dbg_wl("disable \"%s\"", ubi->bgt_name);
-	if (ubi->bgt_thread)
-		kthread_stop(ubi->bgt_thread);
-
 	dbg_wl("close the UBI wear-leveling unit");
 
 	cancel_pending(ubi);
-- 
1.5.3.4

[PATCH 4/5] UBI: introduce attach ioctls

[Artem Bityutskiy]

>From c76b002b99f7d75fe0201bf80a675873e5b00231 Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Date: Tue, 18 Dec 2007 18:22:16 +0200
Subject: [PATCH] UBI: introduce attach ioctls

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 include/mtd/ubi-user.h |   40 +++++++++++++++++++++++++++++++++++-----
 1 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/include/mtd/ubi-user.h b/include/mtd/ubi-user.h
index fe06ded..b96d120 100644
--- a/include/mtd/ubi-user.h
+++ b/include/mtd/ubi-user.h
@@ -80,6 +80,15 @@
 /* Re-size an UBI volume */
 #define UBI_IOCRSVOL _IOW(UBI_IOC_MAGIC, 2, struct ubi_rsvol_req)
 
+/* IOCTL commands of the UBI control character device */
+
+#define UBI_CTRL_IOC_MAGIC 'o'
+
+/* Attach an MTD device */
+#define UBI_IOCATT _IOW(UBI_CTRL_IOC_MAGIC, 64, struct ubi_attach_req)
+/* Detach an MTD device */
+#define UBI_IOCDET _IOW(UBI_CTRL_IOC_MAGIC, 65, int32_t)
+
 /* IOCTL commands of UBI volume character devices */
 
 #define UBI_VOL_IOC_MAGIC 'O'
@@ -89,6 +98,9 @@
 /* An eraseblock erasure command, used for debugging, disabled by default */
 #define UBI_IOCEBER _IOW(UBI_VOL_IOC_MAGIC, 1, int32_t)
 
+/* Maximum MTD device name length supported by UBI */
+#define MAX_UBI_MTD_NAME_LEN 127
+
 /*
  * UBI volume type constants.
  *
@@ -97,19 +109,37 @@
  */
 enum {
 	UBI_DYNAMIC_VOLUME = 3,
-	UBI_STATIC_VOLUME = 4
+	UBI_STATIC_VOLUME = 4,
+};
+
+/**
+ * struct ubi_attach_req - attach MTD device request.
+ * @vid_hdr_offset: VID header offset
+ * @data_offset: data offset
+ * @mtd_num: MTD device number to attach
+ * @padding: reserved for future, not used, has to be zeroed
+ *
+ * This data structure is used to specify MTD device UBI has to attach and the
+ * parameters it has to use. The "attach MTD device" ioctl returns the number
+ * of the newly created UBI device as the return value.
+ */
+struct ubi_attach_req {
+	int32_t vid_hdr_offset;
+	int32_t data_offset;
+	int32_t mtd_num;
+	uint8_t padding[12];
 };
 
 /**
  * struct ubi_mkvol_req - volume description data structure used in
- * volume creation requests.
+ *                        volume creation requests.
  * @vol_id: volume number
  * @alignment: volume alignment
  * @bytes: volume size in bytes
  * @vol_type: volume type (%UBI_DYNAMIC_VOLUME or %UBI_STATIC_VOLUME)
- * @padding1: reserved for future, not used
+ * @padding1: reserved for future, not used, has to be zeroed
  * @name_len: volume name length
- * @padding2: reserved for future, not used
+ * @padding2: reserved for future, not used, has to be zeroed
  * @name: volume name
  *
  * This structure is used by userspace programs when creating new volumes. The
@@ -139,7 +169,7 @@ struct ubi_mkvol_req {
 	int8_t padding1;
 	int16_t name_len;
 	int8_t padding2[4];
-	char name[UBI_MAX_VOLUME_NAME+1];
+	char name[UBI_MAX_VOLUME_NAME + 1];
 } __attribute__ ((packed));
 
 /**
-- 
1.5.3.4

[PATCH 5/5] UBI: handle attach ioctl

[Artem Bityutskiy]

>From 1a2effa21c169a1bdde22778e9acceef22ec3383 Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Date: Tue, 18 Dec 2007 18:23:39 +0200
Subject: [PATCH] UBI: handle attach ioctl

Actually implement the MTD device attach/detach handlers.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 drivers/mtd/ubi/cdev.c |   80 +++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 65 insertions(+), 15 deletions(-)

diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index 01978b5..3eeacf7 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -44,17 +44,6 @@
 #include <asm/div64.h>
 #include "ubi.h"
 
-/*
- * Maximum sequence numbers of UBI and volume character device IOCTLs (direct
- * logical eraseblock erase is a debug-only feature).
- */
-#define UBI_CDEV_IOC_MAX_SEQ 2
-#ifndef CONFIG_MTD_UBI_DEBUG_USERSPACE_IO
-#define VOL_CDEV_IOC_MAX_SEQ 1
-#else
-#define VOL_CDEV_IOC_MAX_SEQ 2
-#endif
-
 /**
  * get_exclusive - get exclusive access to an UBI volume.
  * @desc: volume descriptor
@@ -582,8 +571,7 @@ static int ubi_cdev_ioctl(struct inode *inode, struct file *file,
 		struct ubi_mkvol_req req;
 
 		dbg_msg("create volume");
-		err = copy_from_user(&req, argp,
-				       sizeof(struct ubi_mkvol_req));
+		err = copy_from_user(&req, argp, sizeof(struct ubi_mkvol_req));
 		if (err) {
 			err = -EFAULT;
 			break;
@@ -647,8 +635,7 @@ static int ubi_cdev_ioctl(struct inode *inode, struct file *file,
 		struct ubi_rsvol_req req;
 
 		dbg_msg("re-size volume");
-		err = copy_from_user(&req, argp,
-				       sizeof(struct ubi_rsvol_req));
+		err = copy_from_user(&req, argp, sizeof(struct ubi_rsvol_req));
 		if (err) {
 			err = -EFAULT;
 			break;
@@ -684,8 +671,71 @@ static int ubi_cdev_ioctl(struct inode *inode, struct file *file,
 	return err;
 }
 
+static int ctrl_cdev_ioctl(struct inode *inode, struct file *file,
+			   unsigned int cmd, unsigned long arg)
+{
+	int err = 0;
+	void __user *argp = (void __user *)arg;
+
+	if (!capable(CAP_SYS_RESOURCE))
+		return -EPERM;
+
+	switch (cmd) {
+	/* Attach an MTD device command */
+	case UBI_IOCATT:
+	{
+		struct ubi_attach_req req;
+		struct mtd_info *mtd;
+
+		dbg_msg("attach MTD device");
+		err = copy_from_user(&req, argp, sizeof(struct ubi_attach_req));
+		if (err) {
+			err = -EFAULT;
+			break;
+		}
+
+		if (req.mtd_num < 0)
+			return -EINVAL;
+
+		mtd = get_mtd_device(NULL, req.mtd_num);
+		if (IS_ERR(mtd))
+			return PTR_ERR(mtd);
+
+		/* Note, request verification is done by the attach function */
+		err = ubi_attach_mtd_dev(mtd, req.vid_hdr_offset,
+					 req.data_offset);
+		if (err)
+			put_mtd_device(mtd);
+		break;
+	}
+
+	/* Detach an MTD device command */
+	case UBI_IOCDET:
+	{
+		int ubi_num;
+
+		dbg_msg("dettach MTD device");
+		err = get_user(ubi_num, (__user int32_t *)argp);
+		if (err) {
+			err = -EFAULT;
+			break;
+		}
+
+		err = ubi_detach_mtd_dev(ubi_num, 0);
+		break;
+	}
+
+	default:
+		err = -ENOTTY;
+		break;
+	}
+
+	return err;
+}
+
 /* UBI control character device operations */
 struct file_operations ubi_ctrl_cdev_operations = {
+	.ioctl = ctrl_cdev_ioctl,
 	.owner = THIS_MODULE,
 };
 
-- 
1.5.3.4

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

[Arnd Bergmann]

On Wednesday 19 December 2007, Artem Bityutskiy wrote:
> 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?

with the "/dev/loop"-like method, you'd do something like

	struct ubi_attach_req req = {
		.vid_hdr_offset = vid_hdr,
		.data_offset = data,
		.mtd_num = 0,
	};
	int fd = open("/dev/ubi0", ...);
	ioctl(fd, UBI_IOCATT, &req);

same as before, but instead of /dev/ubictrl, you'd use the actual ubi
device directly. Since this wasn't obvious, I may have misunderstood
something about ubi. Is there actually a device node for each ubi device?
Is it a block or character device?

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

The Linux device model is really good for actual devices. One thing it
does not handle that well is artificial interfaces that are not really
physical devices in your system.
The MTD devices are already represented in the device model. If you
want to represent UBI well, it should be connected to that.

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

A major number gives you a space of 2**32 devices, where you would only
need a single device. 
If you just call misc_register(), you get the exact same semantics for
/dev/ubi_ctrl, but need less code.
   
Note that I'm talking about three different alternatives here:

1. Use the ubi device node directly instead of a separate control
   device.
2. Replace the control device with text based attributes in the MTD
   objects in sysfs.
3. Do everything like you have already, just replace a full character
   device with the simpler misc_device.

You should only use one of these.

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

I thought constructors have mostly been deprecated, now that destructors
are no longer part of the kernel. At least they don't have any advantage
over a simple wrapper around kzalloc/initialize.

	Arnd <><

	Arnd <><

Re: [PATCH 4/5] UBI: introduce attach ioctls

[Arnd Bergmann]

On Wednesday 19 December 2007, Artem Bityutskiy wrote:
> On Wed, 2007-12-19 at 15:17 +0100, Arnd Bergmann wrote:
> > > +struct ubi_attach_req {
> > > +       int32_t vid_hdr_offset;
> > > +       int32_t data_offset;
> > > +       int32_t mtd_num;
> > > +       uint8_t padding[12];
> > >  };
> > 
> > Can you explain why you need to pass vid_hdr_offset /and/ data_offset here?
> > What is the difference between the two? Can't you autoprobe them if you
> > have the device?
> 
> vid_hdr_offset is the offset of the VID header withing an eraseblock.
> data_offs is where the data starts. We want to be able to let users
> select the VID header offset. Vs data offset - indeed it is redundant
> and might be dropped because UBI may just assume data starts at the next
> min. I/O unit after the VID header.
> 
> We can autoprobe the VID header offset, unless the MTD device is empty,
> in which case UBI automatically formats it.

ok, thanks for the explanation.

> > The reason I'm asking is that I'd really like to make this a simple
> > attribute in sysfs, in the mtd object. The question there is what a
> > user would need to store into that attribute. The device is identified
> > implicitly already, but this looks like you still need two distint
> > integers in order to create an UBI device.
> 
> If the MTD device is already UBI-formatted, the numbers may be read from
> the media. Otherwise, not.

Ok, let me suggest an idea, it probably needs more refinement, but maybe we
can come up with something better based on this:

You can have a simple attribute named 'ubi-probe' in each MTD device in sysfs.
this is only readable, and contains either '0' or '1' in ascii, telling you
whether a UBI device could be found for this device.
This only works for UBI formatted media, and the kernel does not format the
media itself.
IF you want to UBI-format a medium, use a user space tool that writes the
appropriate blocks directly to the /dev/mtd* character device.

	Arnd <><

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

[Artem Bityutskiy]

On Wed, 2007-12-19 at 16:51 +0100, Arnd Bergmann wrote: 
> > 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?
> 
> with the "/dev/loop"-like method, you'd do something like
> 
> 	struct ubi_attach_req req = {
> 		.vid_hdr_offset = vid_hdr,
> 		.data_offset = data,
> 		.mtd_num = 0,
> 	};
> 	int fd = open("/dev/ubi0", ...);
> 	ioctl(fd, UBI_IOCATT, &req);

Err, we do not have /dev/ubi0 and we want to create it, so we cannot
open it, because it does not exist. I may be very stupid, please,
explain more.

> same as before, but instead of /dev/ubictrl, you'd use the actual ubi
> device directly. Since this wasn't obvious, I may have misunderstood
> something about ubi. Is there actually a device node for each ubi device?
> Is it a block or character device?

It's neither block not character, although the device nodes are
character devices.

UBI device is similar to LVM in a sense. But it is not a block device.

MTD device - a physical flash. Just like HDD.
Example: /dev/mtd0, /dev/mtd1.

UBI device - a volume management system on top of MTD device - just like
volume group in LVM. UBI device may have UBI volumes.
Example, /dev/ubi0, dev/ubi1.

UBI volume - an entity within UBI device. Similar to logical volume
within volume group of LVM. Example: /dev/ubi0_0, /dev/ubi0_1.

> > > 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.
> 
> The Linux device model is really good for actual devices. One thing it
> does not handle that well is artificial interfaces that are not really
> physical devices in your system.

It actually works for UBI pretty well. I really, do not see any troubles
with it - it works fine for UBI.

> The MTD devices are already represented in the device model. If you
> want to represent UBI well, it should be connected to that.

This is not true. MTD is still out of this model (struct mtd_info does
not have a struct device object). And even if it were, UBI would just
have a reference to it as to a parent device, otherwise it would stay
the same.

UBI is connected to an MTD device similarly, as LVM volume group is
connected to HDD(s).

> A major number gives you a space of 2**32 devices, where you would only
> need a single device. 
> If you just call misc_register(), you get the exact same semantics for
> /dev/ubi_ctrl, but need less code.

Hmm, looks ok - I just did not know about this. Will try it, thanks!

> Note that I'm talking about three different alternatives here:
> 
> 1. Use the ubi device node directly instead of a separate control
>    device.

This does not really make sense for me to create /dev/ubi0 by means of
opening /dev/ubi0. Surely you do not suggest UBI to register all the
possible /dev/ubiX in the system, and then handle a "create yourself"
ioctl? What for?

> 2. Replace the control device with text based attributes in the MTD
>    objects in sysfs.

MTD is not is sysfs. And even if it were, I do not really see how would
it be possible.

> 3. Do everything like you have already, just replace a full character
>    device with the simpler misc_device.

I like this :-) Of course I did not mean I need so many device numbers -
its just lack of knowledge.

> 
> You should only use one of these.
> 
> > > >  	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.
> 
> I thought constructors have mostly been deprecated, now that destructors
> are no longer part of the kernel. At least they don't have any advantage
> over a simple wrapper around kzalloc/initialize.

Well, I do not at all insist this slab cache makes sense. The objects of
the slab are used to implement per-LEB locking, and each object has a
counter and a semaphore, and I did not want to set the counter to 0 and
initialize semaphore on _every_ allocation. But you may argue that LEBs
are locked when there is I/O on them, and it makes zero sense to save
few CPU cycles. Let me remove this as a separate patch.

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

Re: [PATCH 4/5] UBI: introduce attach ioctls

[Artem Bityutskiy]

On Wed, 2007-12-19 at 16:57 +0100, Arnd Bergmann wrote:
> > 
> > If the MTD device is already UBI-formatted, the numbers may be read from
> > the media. Otherwise, not.
> 
> Ok, let me suggest an idea, it probably needs more refinement, but maybe we
> can come up with something better based on this:
> 
> You can have a simple attribute named 'ubi-probe' in each MTD device in sysfs.

Err, why should MTD know something about what sits on top of it? Today
it is UBI, tommorrow it is something new and better, e.g., more
scalable.

> this is only readable, and contains either '0' or '1' in ascii, telling you
> whether a UBI device could be found for this device.

So MTD maintains ubi-probe attribute, and has some knowlege about UBI.
Well, it is technically possible, but I do not think it would be good
design. The same way we could teach MTD to probe if it has JFFS2 on it,
or something else...

> This only works for UBI formatted media, and the kernel does not format the
> media itself.
> IF you want to UBI-format a medium, use a user space tool that writes the
> appropriate blocks directly to the /dev/mtd* character device.

Yeah, user-space tools could format media. But it is so much appropriate
facility to have UBI being able to format it itself. It is really very
convenient. Flashes have special state - empty flash, and if the flash
is empty - UBI make it UBI-formatted. If the flash has some garbage -
UBI does not format it. 

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

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

[Artem Bityutskiy]

On Wed, 2007-12-19 at 16:51 +0100, Arnd Bergmann wrote:
> A major number gives you a space of 2**32 devices, where you would only
> need a single device. 
> If you just call misc_register(), you get the exact same semantics for
> /dev/ubi_ctrl, but need less code.

Err, this is not really appropriate - it registers my device under
"misc" class...

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

Re: [PATCH 4/5] UBI: introduce attach ioctls

[Arnd Bergmann]

On Wednesday 19 December 2007, Artem Bityutskiy wrote:
> On Wed, 2007-12-19 at 16:57 +0100, Arnd Bergmann wrote:
> > > 
> > > If the MTD device is already UBI-formatted, the numbers may be read from
> > > the media. Otherwise, not.
> > 
> > Ok, let me suggest an idea, it probably needs more refinement, but maybe we
> > can come up with something better based on this:
> > 
> > You can have a simple attribute named 'ubi-probe' in each MTD device in sysfs.
> 
> Err, why should MTD know something about what sits on top of it? Today
> it is UBI, tommorrow it is something new and better, e.g., more
> scalable.

The way you would implement this is to have the UBI code grab hold of all
the MTD devices, and create the ubi-probe attribute in them. This is
something that is easily possible with the device model, provided that
we can get a 'struct device' embedded into 'struct mtd_info'. I just realized
that this is currently missing.

> > this is only readable, and contains either '0' or '1' in ascii, telling you
> > whether a UBI device could be found for this device.
> 
> So MTD maintains ubi-probe attribute, and has some knowlege about UBI.
> Well, it is technically possible, but I do not think it would be good
> design. The same way we could teach MTD to probe if it has JFFS2 on it,
> or something else...

The probe on a JFFS2 formatted device is called 'mount', and we have a
perfectly fine system call interface for that ;-)

> > This only works for UBI formatted media, and the kernel does not format the
> > media itself.
> > IF you want to UBI-format a medium, use a user space tool that writes the
> > appropriate blocks directly to the /dev/mtd* character device.
> 
> Yeah, user-space tools could format media. But it is so much appropriate
> facility to have UBI being able to format it itself. It is really very
> convenient. Flashes have special state - empty flash, and if the flash
> is empty - UBI make it UBI-formatted. If the flash has some garbage -
> UBI does not format it. 

Ok, another idea: just create an UBI device for every MTD device, but don't
probe until the UBI device is first accessed. That way, you don't need
any dynamic registration, and you can use an ioctl on the device itself
in order to do the initial formatting with new parameters.

	Arnd <><

Re: [PATCH 4/5] UBI: introduce attach ioctls

[Josh Boyer]

On Thu, 20 Dec 2007 22:34:56 +0100
Arnd Bergmann <arnd@arndb.de> wrote:
> > > You can have a simple attribute named 'ubi-probe' in each MTD device in sysfs.
> > 
> > Err, why should MTD know something about what sits on top of it? Today
> > it is UBI, tommorrow it is something new and better, e.g., more
> > scalable.
> 
> The way you would implement this is to have the UBI code grab hold of all
> the MTD devices, and create the ubi-probe attribute in them. This is
> something that is easily possible with the device model, provided that
> we can get a 'struct device' embedded into 'struct mtd_info'. I just realized
> that this is currently missing.

You could.  Aside from this current discussion, what good would it do
though?

> > So MTD maintains ubi-probe attribute, and has some knowlege about UBI.
> > Well, it is technically possible, but I do not think it would be good
> > design. The same way we could teach MTD to probe if it has JFFS2 on it,
> > or something else...
> 
> The probe on a JFFS2 formatted device is called 'mount', and we have a
> perfectly fine system call interface for that ;-)

That's not a good analogy.  MTD still has absolutely no idea what is on
top of it.
 
> > Yeah, user-space tools could format media. But it is so much appropriate
> > facility to have UBI being able to format it itself. It is really very
> > convenient. Flashes have special state - empty flash, and if the flash
> > is empty - UBI make it UBI-formatted. If the flash has some garbage -
> > UBI does not format it. 
> 
> Ok, another idea: just create an UBI device for every MTD device, but don't
> probe until the UBI device is first accessed. That way, you don't need
> any dynamic registration, and you can use an ioctl on the device itself
> in order to do the initial formatting with new parameters.

Why would you carry the memory overhead for those devices if they're
never going to be used?

josh

Re: [PATCH 4/5] UBI: introduce attach ioctls

[Artem Bityutskiy]

On Thu, 2007-12-20 at 22:34 +0100, Arnd Bergmann wrote:
> > Err, why should MTD know something about what sits on top of it? Today
> > it is UBI, tommorrow it is something new and better, e.g., more
> > scalable.
> 
> The way you would implement this is to have the UBI code grab hold of all
> the MTD devices, and create the ubi-probe attribute in them. This is
> something that is easily possible with the device model, provided that
> we can get a 'struct device' embedded into 'struct mtd_info'. I just realized
> that this is currently missing.

Ok, now I see what you mean. Well, may be some day we make MTD
LDM-enabled. But apart of this, I really do not see any benefit in
ubi-probe stuff. It's about closer integration of 2 separate things,
which does not sound good for me.

> > Yeah, user-space tools could format media. But it is so much appropriate
> > facility to have UBI being able to format it itself. It is really very
> > convenient. Flashes have special state - empty flash, and if the flash
> > is empty - UBI make it UBI-formatted. If the flash has some garbage -
> > UBI does not format it. 
> 
> Ok, another idea: just create an UBI device for every MTD device, but don't
> probe until the UBI device is first accessed. That way, you don't need
> any dynamic registration, and you can use an ioctl on the device itself
> in order to do the initial formatting with new parameters.

Well, it is technically possible, but requires a lot of code which does
not really worth it. I still think having a control "misc" device is
quite elegant solution.

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

Re: [PATCH 4/5] UBI: introduce attach ioctls

[Frank Haverkamp]

[-- Attachment #1: Type: text/plain, Size: 3090 bytes --]

Hi Arnd and Artem,

On Wed, 2007-12-19 at 16:42 +0200, Artem Bityutskiy wrote:
> On Wed, 2007-12-19 at 15:17 +0100, Arnd Bergmann wrote:
> > > +struct ubi_attach_req {
> > > +       int32_t vid_hdr_offset;
> > > +       int32_t data_offset;
> > >  };
> > 
> > Can you explain why you need to pass vid_hdr_offset /and/ data_offset here?
> > What is the difference between the two?

Data in flash:
EC-hdr | VID-hdr | Data
-------> VID-hdr-offs
-----------------> Data-offset

>  Can't you autoprobe them if you
> > have the device?
> 
> vid_hdr_offset is the offset of the VID header withing an eraseblock.
> data_offs is where the data starts. We want to be able to let users
> select the VID header offset.

> Vs data offset - indeed it is redundant
> and might be dropped because UBI may just assume data starts at the next
> min. I/O unit after the VID header.

Traditionally NAND could only be written only in page-chunks e.g. 2KiB.
Thomas Gleixner introduced that subpages could be written too e.g. 512B.

In our usage example we decided, that we wanted to have the EC-hdr at
offset 0, and the VID-hdr at the end of the last subpage of page 0 in
the NAND-erase block. We wanted the data to start at a page boundary so
that e.g. JFFS2 can use the pagesize as minimum write size and not the
sub-pagesize.

Our layout:
page 0                  | page 1                   | ...
subpage | ... | subpage | subpage | ...  | subpage | ...
EC-hdr  | ... | VID-hdr | Data

Default layout (Artem, correct me if I am wrong):
page 0                          | page 1                   | ...
subpage | ...     | subpage ... | subpage | ...  | subpage | ...
EC-hdr  | VID-hdr | Data

> We can autoprobe the VID header offset, unless the MTD device is empty,
> in which case UBI automatically formats it.

I do not like auto-probing the VID hdr offset. In my eyes the user
should have the flexibility to define where his VID-hdr and data start
e.g. like in our case to have influence on if data is starting on
subpage- or page-boundaries. I also think that the autoprobe feature
would add more complexity to the code.

> > The reason I'm asking is that I'd really like to make this a simple
> > attribute in sysfs, in the mtd object. The question there is what a
> > user would need to store into that attribute. The device is identified
> > implicitly already, but this looks like you still need two distint
> > integers in order to create an UBI device.

I think with our current usage calculating the data offset as next free
min-io-offset would work, although the fact that we want to store our
data at a page-offset would be done implicitly than.

I wonder which layout Thomas Gleixner and Josh Boyer are using? Would it
work to calculate the data offset in your case?

Frank

-- 
IBM Deutschland Entwicklung GmbH, Schoenaicher Str. 220, 71032
Boeblingen, Vorsitzender des Aufsichtsrats: Martin Jetter
Geschäftsführung: Herbert Kircher, Sitz der Gesellschaft: Böblingen,
Registergericht: Amtsgericht Stuttgart, HRB 243294

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5269 bytes --]

Re: [PATCH 4/5] UBI: introduce attach ioctls

[Arnd Bergmann]

On Thursday 03 January 2008, Frank Haverkamp wrote:
> > We can autoprobe the VID header offset, unless the MTD device is empty,
> > in which case UBI automatically formats it.
> 
> I do not like auto-probing the VID hdr offset. In my eyes the user
> should have the flexibility to define where his VID-hdr and data start
> e.g. like in our case to have influence on if data is starting on
> subpage- or page-boundaries. I also think that the autoprobe feature
> would add more complexity to the code.

I agree that the user should have the flexibility to set these parameters,
but AFAICS, that is only relevant during initialization (formatting) of
the medium.

However, I find it essential that it's possible to do autoprobing of
existing media, by whatever means.
If you don't want to do it in the kernel, a user space helper might
be useful, preferably one that does not require ioctls, so it can be
done as a shell script.

	Arnd <><

Re: [PATCH 4/5] UBI: introduce attach ioctls

[Frank Haverkamp]

[-- Attachment #1: Type: text/plain, Size: 1486 bytes --]

Hi Arnd,

On Thu, 2008-01-03 at 16:05 +0100, Arnd Bergmann wrote:
> On Thursday 03 January 2008, Frank Haverkamp wrote:
> > > We can autoprobe the VID header offset, unless the MTD device is empty,
> > > in which case UBI automatically formats it.
> > 
> > I do not like auto-probing the VID hdr offset. In my eyes the user
> > should have the flexibility to define where his VID-hdr and data start
> > e.g. like in our case to have influence on if data is starting on
> > subpage- or page-boundaries. I also think that the autoprobe feature
> > would add more complexity to the code.
> 
> I agree that the user should have the flexibility to set these parameters,
> but AFAICS, that is only relevant during initialization (formatting) of
> the medium.
> 
> However, I find it essential that it's possible to do autoprobing of
> existing media, by whatever means.
> If you don't want to do it in the kernel, a user space helper might
> be useful, preferably one that does not require ioctls, so it can be
> done as a shell script.

Yes, a user-space tool to figure out the values seems to me a good idea.
It helps moving the complexity to user-space which you would otherwise
have in the kernel-code.

Frank
-- 
IBM Deutschland Entwicklung GmbH, Schoenaicher Str. 220, 71032
Boeblingen, Vorsitzender des Aufsichtsrats: Martin Jetter
Geschäftsführung: Herbert Kircher, Sitz der Gesellschaft: Böblingen,
Registergericht: Amtsgericht Stuttgart, HRB 243294

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5269 bytes --]