[U-Boot] [PATCH] [UBI] Basic Unsorted Block Image (UBI) support (part 1)

[Stefan Roese]

Hi Wolfgang & Kyungmin,

Wolfgang, thanks for the internal discussion about this. I now know that you 
have no general concerns about the Linux "code cloning". But please find my 
original comments (written before the internal discussion) below.

On Tuesday 21 October 2008, Wolfgang Denk wrote:
> > Now it can use UBI support on U-Boot
>
> That's not really a good commit message, it seems.

Ack.

> > index 0000000..a9f494a
> > --- /dev/null
> > +++ b/drivers/mtd/mtdcore.c
> > @@ -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.

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.

> > + * Core registration and callback routines for MTD
> > + * drivers and users.
> > + */
>
> GPL header missing - here and in many other files.
>
> > +int add_mtd_device(struct mtd_info *mtd)
> > +{
> > +	int i;
> > +
> > +	BUG_ON(mtd->writesize == 0);
>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> What's that?

It's handled in the UBI Linux->U-Boot porting "layer" (include/ubi_uboot.h):

#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))

Again this has been done before:

[stefan at kubuntu u-boot (master)]$ git grep BUG_ON include/
include/common.h:#define BUG_ON(condition) do { if (unlikely((condition)!=0)) 
BUG(); } while(0)
include/linux/mtd/compat.h:#define BUG_ON(condition) do { if (condition) 
BUG(); } while(0)

> > +	for (i=0; i < MAX_MTD_DEVICES; i++)
>
> 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.

> > +		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;
> > +		}
> > +
> > +	return 1;
> > +}
> > +
> > +/**
>
> Please use plain "/*" as start of multiline comments. Here and
> everywhere.

Again I vote for not changing it.

> > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> > new file mode 100644
> > index 0000000..113516a
> > --- /dev/null
> > +++ b/drivers/mtd/mtdpart.c
> > @@ -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.
>
> ...
>
> > +#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.

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.

> > index 0000000..bf8a19f
> > --- /dev/null
> > +++ b/drivers/mtd/ubi/build.c
> > @@ -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. + */
> > +
> > +#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.
>
> > +
> > +/**
> > + * 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];
> > +
> > +		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.

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

> > +#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?

Kyungmin, do you have any ideas on parts of this code that can be 
removed/dropped completely? Other than the "#if 0" parts?

Thanks.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================