Basic Unsorted Block Image (UBI) support (part 1)

Tm93IGl0IGNhbiB1c2UgVUJJIHN1cHBvcnQgb24gVS1Cb290CgpTaWduZWQtb2ZmLWJ5OiBLeXVu Z21pbiBQYXJrIDxreXVuZ21pbi5wYXJrQHNhbXN1bmcuY29tPgotLS0KIE1ha2VmaWxlICAgICAg ICAgICAgICAgICAgICAgICB8ICAgIDEgKwogZHJpdmVycy9tdGQvTWFrZWZpbGUgICAgICAgICAg IHwgICAgMSArCiBkcml2ZXJzL210ZC9tdGRjb3JlLmMgICAgICAgICAgfCAgMTQyICsrKysKIGRy aXZlcnMvbXRkL210ZHBhcnQuYyAgICAgICAgICB8ICA1MzEgKysrKysrKysrKysrKwogZHJpdmVy

Now it can use UBI support on U-Boot

drivers/mtd/mtdcore.c new file +10 -0

+		if (!mtd_table[i]) {
+			mtd_table[i] = mtd;
+			mtd->index = i;
+			mtd->usecount = 0;
+
+			printf("mtd: Giving out device %d to %s\n",i, mtd->name);
+			/* No need to get a refcount on the module containing
+			   the notifier, since we hold the mtd_table_mutex */
+
+			/* We _know_ we aren't being removed, because

index 0000000..bf8a19f

drivers/mtd/ubi/build.c new file +35 -0

@@ -0,0 +1,1182 @@
+/*
+ * Copyright (c) International Business Machines Corp., 2006
+ * Copyright (c) Nokia Corporation, 2007
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
+ * the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * Author: Artem Bityutskiy (???????????????? ??????????),
+ *         Frank Haverkamp
+ */
+
+/*
+ * 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.
+ */
+

drivers/mtd/ubi/build.c new file +16 -0

+
+/**
+ * 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, otherwise 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];

Quoted 1 line

Now it can use UBI support on U-Boot

That's not really a good commit message, it seems.

drivers/mtd/mtdcore.c line 2

Quoted 6 lines

index 0000000..a9f494a

drivers/mtd/mtdcore.c new file +2 -0

@@ -0,0 +1,142 @@
+/*
+ * $Id: mtdcore.c,v 1.47 2005/11/07 11:14:20 gleixner Exp $

Please do not include versioninformation in files. We use git for this.

drivers/mtd/mtdcore.c line 6

Quoted 3 lines

drivers/mtd/mtdcore.c new file +3 -0

+ * Core registration and callback routines for MTD
+ * drivers and users.
+ */

GPL header missing - here and in many other files.

drivers/mtd/mtdcore.c line 18

Quoted 5 lines

drivers/mtd/mtdcore.c new file +5 -0

+int add_mtd_device(struct mtd_info *mtd)
+{
+	int i;
+
+	BUG_ON(mtd->writesize == 0);

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

What's that?

drivers/mtd/mtdcore.c line 20

Quoted 1 line

drivers/mtd/mtdcore.c new file +1 -0

+	for (i=0; i < MAX_MTD_DEVICES; i++)

Need "{...}" for multi-line statements.

Quoted 15 lines

drivers/mtd/mtdcore.c new file +15 -0

+		if (!mtd_table[i]) {
+			mtd_table[i] = mtd;
+			mtd->index = i;
+			mtd->usecount = 0;
+
+			printf("mtd: Giving out device %d to %s\n",i, mtd->name);
+			/* No need to get a refcount on the module containing
+			   the notifier, since we hold the mtd_table_mutex */
+
+			/* We _know_ we aren't being removed, because
+			   our caller is still holding us here. So none
+			   of this try_ nonsense, and no bitching about it
+			   either. :) */
+			return 0;
+		}

drivers/mtd/mtdcore.c line 40

Quoted 5 lines

drivers/mtd/mtdcore.c new file +5 -0

+
+	return 1;
+}
+
+/**

Please use plain "/*" as start of multiline comments. Here and everywhere.

drivers/mtd/mtdpart.c line 2

Quoted 8 lines

drivers/mtd/mtdpart.c new file +2 -0

new file mode 100644
index 0000000..113516a
@@ -0,0 +1,531 @@
+/*
+ * Simple MTD partitioning layer

Hm... We were talking about UBI support.

Do we really have to pull in the whole MTD layer from Linux for this?

I'm not exactly happy about this.

...

Quoted 12 lines

drivers/mtd/mtdpart.c new file +12 -0

+#if 0
+	if (unlikely(res)) {
+		if (res == -EUCLEAN)
+			mtd->ecc_stats.corrected++;
+		if (res == -EBADMSG)
+			mtd->ecc_stats.failed++;
+	}
+#endif
+	return res;
+}
+
+#if 0

...

Please do not add dead code.

Quoted 39 lines

index 0000000..bf8a19f

drivers/mtd/ubi/build.c new file +17 -0

@@ -0,0 +1,1182 @@
+/*
+ * Copyright (c) International Business Machines Corp., 2006
+ * Copyright (c) Nokia Corporation, 2007
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
+ * the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307

USA + *
+ * Author: Artem Bityutskiy (???????????????? ??????????),
+ * Frank Haverkamp
+ */

drivers/mtd/ubi/build.c new file +5 -0

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

drivers/mtd/ubi/build.c new file +1 -0

+

drivers/mtd/ubi/build.c line 45

Quoted 10 lines

drivers/mtd/ubi/build.c new file +10 -0

+#if 0
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/stringify.h>
+#include <linux/stat.h>
+#include <linux/miscdevice.h>
+#include <linux/log2.h>
+#include <linux/kthread.h>
+#endif

Please do not add dead code - here and everywhere.

Quoted 16 lines

drivers/mtd/ubi/build.c new file +3 -0

+
+/**
+ * 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, otherwise the UBI device + * number is returned.
+ */

drivers/mtd/ubi/build.c new file +7 -0

+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];

drivers/mtd/ubi/build.c line 222

Quoted 10 lines

drivers/mtd/ubi/build.c new file +10 -0

+
+		if (ubi && MAJOR(ubi->cdev.dev) == major) {
+			ubi_num = ubi->ubi_num;
+			break;
+		}
+	}
+	spin_unlock(&ubi_devices_lock);
+
+	return ubi_num;
+}

Hm... it does not make sense to me to pull in the whole Linux OS unfiltered.

Should we not try and restrict the code to the functions that are really needed and used in U-Boot?

We do not have any such stuff like major or minor device numbers here. And no permissions and al the othe rstuff.

drivers/mtd/ubi/build.c line 227

Quoted 4 lines

drivers/mtd/ubi/build.c new file +4 -0

+#if 0
+/* "Show" method for files in '/<sysfs>/class/ubi/ubiX/' */
+static ssize_t dev_attribute_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)

Argh... Again tons of dead code.

I stop reviewing here. It makes no sense.

Can you pelase discuss with Stefan internally how to reduce the amount of code you are importing?

That's directly imported from the Linux source. Most of your comments below also deal with this Linux code copying. I personally think it is a good idea to clone the code from Linux instead of writing a U-Boot specific UBI driver. We have done this before on other occasions and it makes perfect sense here too from my point of view. It makes porting easier and faster and less error prone and even more important, it makes porting of new versions, fixes etc easier.

Quoted 5 lines

Quoted 3 lines

+ * Core registration and callback routines for MTD
+ * drivers and users.
+ */

GPL header missing - here and in many other files.

Quoted 1 line

Need "{...}" for multi-line statements.

Again from the Linux original source. When we start changing this code because of coding-style issue, it will get very hard to compare those versions and add fixes in the future. So I vote for keeping the code as is. It's not that ugly, at least from my understanding.

Quoted 8 lines

drivers/mtd/mtdpart.c line 2

Quoted 4 lines

+++ b/drivers/mtd/mtdpart.c

drivers/mtd/mtdpart.c new file +2 -0

@@ -0,0 +1,531 @@
+/*
+ * Simple MTD partitioning layer

Hm... We were talking about UBI support.

Do we really have to pull in the whole MTD layer from Linux for this?

Quoted 1 line

Please do not add dead code.

Here I'm not sure which version I prefer. The one Kyungmin used, disabling > the not needed code via #if 0 (or something else, like #if UBI_LINUX), or completely removing it. Both has some advantages. But again for comparison> with the original Linux source it's perhaps better to just disable the code> . An "#if UBI_LINIX" is probably better than on "#if 0" though.

Of course we haven't. But it's hard to draw the line *if* you decide to include the Linux source. Most OS functions (like spin_lock()...) are defin> ed to no-ops in ubi_uboot.h. So it doesn't really increase the code size for U-Boot. It just keeps the source in-line with the Linux version.

Dear Stefan Roese,

Quoted 8 lines · In message <[email protected]> you wrote:

That's directly imported from the Linux source. Most of your comments below also deal with this Linux code copying. I personally think it is a good idea to clone the code from Linux instead of writing a U-Boot specific UBI driver. We have done this before on other occasions and it makes perfect sense here too from my point of view. It makes porting easier and faster and less error prone and even more important, it makes porting of new versions, fixes etc easier.

We agree on that. So maybe we should send coding style cleanup patches for the Linux kenrel?

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

+ * Core registration and callback routines for MTD
+ * drivers and users.
+ */

GPL header missing - here and in many other files.

This *is* a serious issue. And I don;t care wether the code comes from Linux or not, but I will not accept any code without absolutley clear licensing conditions.

Quoted 6 lines

Quoted 1 line

Need "{...}" for multi-line statements.

Again from the Linux original source. When we start changing this code because of coding-style issue, it will get very hard to compare those versions and add fixes in the future. So I vote for keeping the code as is. It's not that ugly, at least from my understanding.

It is a clear violation of Linux Coding Style requirements.

If we don;t want to fix it here, I suggest to fixit at the origin, i.e. in Linux.

Quoted 7 lines

Quoted 7 lines

drivers/mtd/mtdpart.c line 2

Quoted 4 lines

+++ b/drivers/mtd/mtdpart.c

drivers/mtd/mtdpart.c new file +2 -0

@@ -0,0 +1,531 @@
+/*
+ * Simple MTD partitioning layer

Hm... We were talking about UBI support.

Quoted 1 line

Quoted 1 line

Do we really have to pull in the whole MTD layer from Linux for this?

...

Quoted 1 line

Quoted 1 line

Please do not add dead code.

Quoted 6 lines

Here I'm not sure which version I prefer. The one Kyungmin used, disabling > the not needed code via #if 0 (or something else, like #if UBI_LINUX), or completely removing it. Both has some advantages. But again for comparison> with the original Linux source it's perhaps better to just disable the code> . An "#if UBI_LINIX" is probably better than on "#if 0" though.

There is two things I am concerned about:

1) Do we really need all these files?
2) Do we really need all of this code?

Lots of the code that was left in was obviously dealing with features we don;t have in U-Boot, and will not have: concepts like user IDs, file permissions, major and minor numbers, etc.

Quoted 4 lines

Of course we haven't. But it's hard to draw the line *if* you decide to include the Linux source. Most OS functions (like spin_lock()...) are defin> ed to no-ops in ubi_uboot.h. So it doesn't really increase the code size for U-Boot. It just keeps the source in-line with the Linux version.

On the other hand it makes it semi-impossible to review the code in U-Boot context, or even to get an understanding of flow of control etc.