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

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

[Huang Shijie]

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 with (Micron M29W256GL7AN6):
-------------------------------------------------------
root@freescale ~$ 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 fixes it.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/ubi/build.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 57deae9..a5edc64 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -640,7 +640,7 @@ static int io_init(struct ubi_device *ubi, int max_beb_per1024)
 	dbg_gen("sizeof(struct ubi_ainf_peb) %zu", sizeof(struct ubi_ainf_peb));
 	dbg_gen("sizeof(struct ubi_wl_entry) %zu", sizeof(struct ubi_wl_entry));
 
-	if (ubi->mtd->numeraseregions != 0) {
+	if (ubi->mtd->numeraseregions > 1) {
 		/*
 		 * Some flashes have several erase regions. Different regions
 		 * may have different eraseblock size and other
-- 
1.7.2.rc3

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

[Huang Shijie]

On Fri, Feb 14, 2014 at 11:18:52AM +0200, Artem Bityutskiy wrote:
> On Fri, 2014-02-14 at 16:08 +0800, Huang Shijie wrote:
> > 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 with (Micron M29W256GL7AN6):
> > -------------------------------------------------------
> > root@freescale ~$ 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 fixes it.
> > 
> > Signed-off-by: Huang Shijie <b32955@freescale.com>
> 
> Did you investigate the entire MTD subsystem an figure out what exactly
> 'numeraseregions=0' and 'numeraseregions=1' mean? What is the difference
> between these 2. Is there consistency?
i think the 'numeraseregions=0' and 'numeraseregions=1' is the same case.

For the NOR, there are maybe several erase regions, such as region A and B.
A is erased by the unit of 4K size, while B is erased by the unit of 64K.
Please correct me if i am wrong.

In actually, the UBI is cheated by the MTD code.

please see the allocate_partition().
---------------------------------------------------------------
	if (master->numeraseregions > 1) {
		/* Deal with variable erase size stuff */
		int i, max = master->numeraseregions;
		u64 end = slave->offset + slave->mtd.size;
		struct mtd_erase_region_info *regions = master->eraseregions;

		/* Find the first erase regions which is part of this
		 * partition. */
		for (i = 0; i < max && regions[i].offset <= slave->offset; i++)
			;
		/* The loop searched for the region _behind_ the first one */
		if (i > 0)
			i--;

		/* Pick biggest erasesize */
		for (; i < max && regions[i].offset < end; i++) {
			if (slave->mtd.erasesize < regions[i].erasesize) {
				slave->mtd.erasesize = regions[i].erasesize;
			}
		}
		BUG_ON(slave->mtd.erasesize == 0);
	} else {
		/* Single erase size */
		slave->mtd.erasesize = master->erasesize;
	}
---------------------------------------------------------------

If we set a slave partition, the slave->numeraseregions is 0 which makes the
UBI run well. As you see, the UBI is cheated in this case: 
   we do not assign any value to the slave partition's numeraseregions.

But if we do not have any _slave_ partitions, we will only have one master partition.
and the numeraseregions is kept. this is the case i meet.

> 
> E.g., if 'numeraseregions=1' should there be non-NULL
> 'mtd->eraseregions'? What UBI should do with that then?
> 
> I am fine to apply your patch, but I would like to understand the whole
> 'erase regions' thing - what is this, what exactly are the semantics,
> how UBI should use them, etc.
> 
> So far, for me 'numeraseregions=0' means there is no erase regions, and
> this is what all drivers UBI has been working so far have.
> 
> I have no idea what are 'numeraseregions=1' beats, and how to deal with
For the parallel NOR, the numeraseregions maybe 1 which means there is only
one erase region.  

thanks
Huang Shijie
> them.
> 
> Ideally, I'd love to see a repsons to this e-mail like this:
> 
> 
> From: Huang Shijie <b32955@freescale.com>, 
> Subject: mtd-www: document erase regions
> 
> This patch is against the mtd-www project and it adds a piece of
> documentation about MTD erase regions.
> 
> 
> :-)
> 
> Thanks!
> 
> -- 
> Best Regards,
> Artem Bityutskiy
> 
> 
> 

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

[Artem Bityutskiy]

On Fri, 2014-02-14 at 16:08 +0800, Huang Shijie wrote:
> 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 with (Micron M29W256GL7AN6):
> -------------------------------------------------------
> root@freescale ~$ 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 fixes it.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>

Did you investigate the entire MTD subsystem an figure out what exactly
'numeraseregions=0' and 'numeraseregions=1' mean? What is the difference
between these 2. Is there consistency?

E.g., if 'numeraseregions=1' should there be non-NULL
'mtd->eraseregions'? What UBI should do with that then?

I am fine to apply your patch, but I would like to understand the whole
'erase regions' thing - what is this, what exactly are the semantics,
how UBI should use them, etc.

So far, for me 'numeraseregions=0' means there is no erase regions, and
this is what all drivers UBI has been working so far have.

I have no idea what are 'numeraseregions=1' beats, and how to deal with
them.

Ideally, I'd love to see a repsons to this e-mail like this:


From: Huang Shijie <b32955@freescale.com>, 
Subject: mtd-www: document erase regions

This patch is against the mtd-www project and it adds a piece of
documentation about MTD erase regions.


:-)

Thanks!

-- 
Best Regards,
Artem Bityutskiy

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

[Artem Bityutskiy]

On Fri, 2014-02-14 at 17:11 +0800, Huang Shijie wrote:
> But if we do not have any _slave_ partitions, we will only have one
> master partition.
> and the numeraseregions is kept. this is the case i meet.

Why you prefer to change UBI instead of changing MTD so that master
partitions with no slave partitions would have numeraseregions=0.

And frankly, I did not clearly understand your answer.

And what erase regions have to do with master/slave?

Correct me if I am wrong below.

The erase region concept belongs to the flash chip. Some chips have
special regions, for some special purposes, and these, typically small,
regions have a bit different geometry.

Master/slave is a purely software concept. I never clearly understood
the model, but this is something related to partitions. We have a master
MTD device representing the entire chip, and slave MTD devices
representing partitions.

In your explanation you mix these 2 unrelated concepts, and I do not
understand why.

Yes, I could go and dig the code, but I prefer the patch submitter to
educate me instead, and demonstrate the depth of his understanding of
the subject :=) Sorry, I am lazy :-)

-- 
Best Regards,
Artem Bityutskiy

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

[Huang Shijie]

On Fri, Feb 14, 2014 at 01:22:54PM +0200, Artem Bityutskiy wrote:
> On Fri, 2014-02-14 at 17:11 +0800, Huang Shijie wrote:
> > But if we do not have any _slave_ partitions, we will only have one
> > master partition.
> > and the numeraseregions is kept. this is the case i meet.
> 
> Why you prefer to change UBI instead of changing MTD so that master
> partitions with no slave partitions would have numeraseregions=0.

the master partition's numeraseregions is get from the code, such as
cfi_amdstd_setup. In other word, the numeraseregions is get from the
probe.

i do not think change the MTD is a good idea.

> 
> And frankly, I did not clearly understand your answer.
sorry.

> 
> And what erase regions have to do with master/slave?
> 
> Correct me if I am wrong below.
> 
> The erase region concept belongs to the flash chip. Some chips have
> special regions, for some special purposes, and these, typically small,
> regions have a bit different geometry.
agree.

> 
> Master/slave is a purely software concept. I never clearly understood
> the model, but this is something related to partitions. We have a master
> MTD device representing the entire chip, and slave MTD devices
> representing partitions.
The slave is purely software concept.

But the master has some information from the chip, as you said, such as
the numeraseregions, the size of the flash.


thanks
Huang Shijie