Re: [PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd->numeraseregions is 1

[Brian Norris <computersforpeace@gmail.com>]

(Artem, any opinions, since you had the most opinion last time this came
up?)

On Mon, Jul 25, 2016 at 12:31:39PM +0200, Richard Weinberger wrote:
> Am 25.07.2016 um 11:46 schrieb Rajeev Kumar:
> > If the master mtd does not have any slave mtd partitions,
> > and its numeraseregions is one(only has one erease block), and
> > we attach the master mtd with : ubiattach -m 0 -d 0
> > 
> > We will meet the error:
> > -------------------------------------------------------
> > root ~$ ubiattach /dev/ubi_ctrl -m 0 -d 0
> > UBI: attaching mtd0 to ubi0
> > UBI error: io_init: multiple regions, not implemented
> > ubiattach: error!: cannot attach mtd0
> >            error 22 (Invalid argument)
> > -------------------------------------------------------
> > 
> > In fact, if there is only one "erase block", we should not
> > prevent the attach.
> > 
> > This patch is tested against 3.14 kernel and only build test is
> > performed against current upstream master branch.
> 
> The more interesting question is, why is ->numeraseregions not 0?
> 
> The comment in the header says:
>         /* Data for variable erase regions. If numeraseregions is zero,
>          * it means that the whole device has erasesize as given above.
>          */
> 
> So, if your MTD erase regions with the same size, it should be 0.
> 
> IIRC we had such a discussion already on linux-mtd and it was not clear
> whether numeraseregions of 0 and 1 are equal or not.

I think 0 and 1 are essentially equal. But there's some potential room
for error (e.g., if the driver doesn't configure mtd->erasesize ==
mtd->eraseregions[0].erasesize, and similar). Also, I see some
problematic code like this in cfi_cmdset_0001.c:

	mtd->numeraseregions = cfi->cfiq->NumEraseRegions * cfi->numchips;

So, if there are two chips, but both have a single erase region, with
the same erasesize, we'll still end up with ->numeraseregions == 2. We
can't hack all MTD users to start searching the eraseregions[] array to
see if the device is actually uniform, even if it reports
numeraseregions > 0.

I'm inclined, then, to outlaw numeraseregions == 1 (to make it simpler for MTD
users to handle), and put code either in drivers or in mtdcore.c to be slightly
more intelligent (e.g., if the driver left numeraseregions == 1, then just do
some sanity checking and re-set numeraseregions to 0). It might be good to move
code like this from cfi_cmdset_0001.c into mtdcore.c at the same time too:

	if (offset != devsize) {
		/* Argh */
		printk(KERN_WARNING "Sum of regions (%lx) != total size of set of interleaved chips (%lx)\n", offset, devsize);
		goto setup_err;
	}

BTW, Rajeev, what devices are you testing? Just curious.

Regards,
Brian