[PATCH] [MTD] [NAND]: nand_default_mark_blockbad() doesn't work when flash-based bbt not enabled

[PATCH] [MTD] [NAND]: nand_default_mark_blockbad() doesn't work when flash-based bbt not enabled

[Ricard Wanderlof]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1017 bytes --]


Hi,

When a flash-based BBT is not used, nand_default_mark_blockbad() is 
supposed to mark the block bad in the oob. However, it sets the wrong 
length variable so that no bad block marker is in fact written. This patch 
attempts to rectify that.

From: Ricard Wanderlof <ricardw at axis.com>
Signed-off-by: Ricard Wanderlof <ricardw at axis.com>

diff -ur git/drivers/mtd/nand/nand_base.c rw/drivers/mtd/nand/nand_base.c
--- git/drivers/mtd/nand/nand_base.c	2006-10-20 17:23:37.000000000 +0200
+++ rw/drivers/mtd/nand/nand_base.c	2006-10-20 17:23:56.000000000 +0200
@@ -362,7 +362,7 @@
  		 * access
  		 */
  		ofs += mtd->oobsize;
-		chip->ops.len = 2;
+		chip->ops.ooblen = 2;
  		chip->ops.datbuf = NULL;
  		chip->ops.oobbuf = buf;
  		chip->ops.ooboffs = chip->badblockpos & ~0x01;

--
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

[-- Attachment #2: patch file --]
[-- Type: TEXT/PLAIN, Size: 447 bytes --]

diff -ur git/drivers/mtd/nand/nand_base.c rw/drivers/mtd/nand/nand_base.c
--- git/drivers/mtd/nand/nand_base.c	2006-10-20 17:23:37.000000000 +0200
+++ rw/drivers/mtd/nand/nand_base.c	2006-10-20 17:23:56.000000000 +0200
@@ -362,7 +362,7 @@
 		 * access
 		 */
 		ofs += mtd->oobsize;
-		chip->ops.len = 2;
+		chip->ops.ooblen = 2;
 		chip->ops.datbuf = NULL;
 		chip->ops.oobbuf = buf;
 		chip->ops.ooboffs = chip->badblockpos & ~0x01;

Re: [PATCH] [MTD] [NAND]: nand_default_mark_blockbad() doesn't work when flash-based bbt not enabled

[Artem Bityutskiy]

On Fri, 2006-10-20 at 17:36 +0200, Ricard Wanderlof wrote:
>   		ofs += mtd->oobsize;
> -		chip->ops.len = 2;
> +		chip->ops.ooblen = 2;
>   		chip->ops.datbuf = NULL;
>   		chip->ops.oobbuf = buf;
>   		chip->ops.ooboffs = chip->badblockpos & ~0x01;

What is len and ooblen? Who initializes len then? I see len is used
later on.

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

Re: [PATCH] [MTD] [NAND]: nand_default_mark_blockbad() doesn't work when flash-based bbt not enabled

[Ricard Wanderlof]

On Fri, 20 Oct 2006, Artem Bityutskiy wrote:

> On Fri, 2006-10-20 at 17:36 +0200, Ricard Wanderlof wrote:
>>   		ofs += mtd->oobsize;
>> -		chip->ops.len = 2;
>> +		chip->ops.ooblen = 2;
>>   		chip->ops.datbuf = NULL;
>>   		chip->ops.oobbuf = buf;
>>   		chip->ops.ooboffs = chip->badblockpos & ~0x01;
>
> What is len and ooblen? Who initializes len then? I see len is used
> later on.

As I understand it, ops.len is the length of the ops.datbuf, and 
ops.ooblen is the length of the ops.oobbuf. In this case, we want to write 
two zeros to the OOB area, so we want to set ops.ooblen to 2, not ops.len. 
This is the way the function nand_fill_oob, further on in the call chain, 
expects to find the length of the data.

Hm, looking more carefully in the next function called, nand_do_write_oob, 
len _is_ used, however, I'm not sure that is correct. It is used in three 
cases, first in a debug printout (which incidentally displays the wrong 
function name), in a sanity check, and finally to set the return value. If 
you look at nand_fill_oob (called from nand_do_write_oob) it uses (only) 
ops.ooblen as the length counter. chip->ecc.write_oob is by default 
nand_write_oob_std, and always writes the whole oob area.

While testing the patch, I put some debugging printouts in nand_fill_oob. 
I could see other cases where it was being called correctly, i.e. with 
ops.ooblen set, e.g. when writing JFFS2 cleanmarkers. So I'm assuming that 
it is ops.ooblen that should be set, and that the references to ops.len in 
this call chain are not correct.

I'll test this on Monday when I get back to work; I don't have access to a 
machine to test on right now.

/Ricard
--
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

[PATCH] [MTD] [NAND]: nand_default_mark_blockbad() doesn't work when flash-based bbt not enabled (fwd)

[Ricard Wanderlof]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1537 bytes --]


(This is an updated patch replacing the one I sent on Friday).

When a flash-based BBT is not used, nand_default_mark_blockbad() is supposed 
to mark the block bad in the oob. However, it sets the wrong length variable 
so that no bad block marker is in fact written. This patch attempts to 
rectify that.

(As note, it seems to be that logically, it shouldn't be necessary to set 
both length variables, as one appears to be for the main buffer, and 
one for the oob buffer, but this is how it is done in several places, 
including the code for the mtd character device MEMWRITEOOB and MEMREADOOB
ioctls. I'm not sure if this is a temporary solution during some rework of 
the mtd infrastructure, or whether there is a deeper thought here.)


From: Ricard Wanderlof <ricardw at axis.com>
Signed-off-by: Ricard Wanderlof <ricardw at axis.com>

diff -ur git/drivers/mtd/nand/nand_base.c rw/drivers/mtd/nand/nand_base.c
--- git/drivers/mtd/nand/nand_base.c	2006-10-20 17:23:37.000000000 +0200
+++ rw/drivers/mtd/nand/nand_base.c	2006-10-23 08:56:45.000000000 +0200
@@ -362,7 +362,7 @@
  		 * access
  		 */
  		ofs += mtd->oobsize;
-		chip->ops.len = 2;
+		chip->ops.len = chip->ops.ooblen = 2;
  		chip->ops.datbuf = NULL;
  		chip->ops.oobbuf = buf;
  		chip->ops.ooboffs = chip->badblockpos & ~0x01;

--
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

[-- Attachment #2: patch file --]
[-- Type: TEXT/PLAIN, Size: 463 bytes --]

diff -ur git/drivers/mtd/nand/nand_base.c rw/drivers/mtd/nand/nand_base.c
--- git/drivers/mtd/nand/nand_base.c	2006-10-20 17:23:37.000000000 +0200
+++ rw/drivers/mtd/nand/nand_base.c	2006-10-23 08:56:45.000000000 +0200
@@ -362,7 +362,7 @@
 		 * access
 		 */
 		ofs += mtd->oobsize;
-		chip->ops.len = 2;
+		chip->ops.len = chip->ops.ooblen = 2;
 		chip->ops.datbuf = NULL;
 		chip->ops.oobbuf = buf;
 		chip->ops.ooboffs = chip->badblockpos & ~0x01;

Re: [PATCH] [MTD] [NAND]: nand_default_mark_blockbad() doesn't work when flash-based bbt not enabled (fwd)

[David Woodhouse]

On Mon, 2006-10-23 at 09:33 +0200, Ricard Wanderlof wrote:
> (As note, it seems to be that logically, it shouldn't be necessary to set 
> both length variables, as one appears to be for the main buffer, and 
> one for the oob buffer, but this is how it is done in several places, 
> including the code for the mtd character device MEMWRITEOOB and MEMREADOOB
> ioctls. I'm not sure if this is a temporary solution during some rework of 
> the mtd infrastructure, or whether there is a deeper thought here.)

Hm. That seems wrong to me. If we don't want to read or write the main
area, we should set ops.len to zero. And if that doesn't work, it should
probably be fixed.

I'd rather see a patch which fixes that.

-- 
dwmw2

Re: [PATCH] [MTD] [NAND]: nand_default_mark_blockbad() doesn't work when flash-based bbt not enabled

[Ricard Wanderlof]

On Mon, 23 Oct 2006, David Woodhouse wrote:

> On Mon, 2006-10-23 at 09:33 +0200, Ricard Wanderlof wrote:
>> (As note, it seems to be that logically, it shouldn't be necessary to set
>> both length variables, as one appears to be for the main buffer, and
>> one for the oob buffer, but this is how it is done in several places,
>> including the code for the mtd character device MEMWRITEOOB and MEMREADOOB
>> ioctls. I'm not sure if this is a temporary solution during some rework of
>> the mtd infrastructure, or whether there is a deeper thought here.)
>
> Hm. That seems wrong to me. If we don't want to read or write the main
> area, we should set ops.len to zero. And if that doesn't work, it should
> probably be fixed.
>
> I'd rather see a patch which fixes that.

It definitely seems confusing.

There are however several places where both ops->len and ops->ooblen are 
used together in some combination, not only in the mtdchar.c ioctl code. 
For instance, in nand.c:nand_do_read_oob() the variable readlen is 
initialized with ops->len, but in the actual read loop, it is decremented 
with ops->ooblen.

The structure using the ops struct was introduced 2006-05-29 by tglx 
(commit titled 'Rework the out of band handling completely').

Since the code as it stands seems to work (apart from the aformentioned 
markbad issue), I'd rather not dive too deep without more information 
about exactly how the ops->len and ops->ooblen fields are supposed to 
interact.

I'd still like to submit the patch, pending a potential clean up of the 
len/ooblen issue, because without the patch, bad block marking fails (in 
certain cases), whereas it works with the patch, and the way len/ooblen is 
handled seems consistent with other uses.

/Ricard
--
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [PATCH] [MTD] [NAND]: nand_default_mark_blockbad() doesn't work when flash-based bbt not enabled (fwd)

[Vitaly Wool]

David Woodhouse wrote:
> On Mon, 2006-10-23 at 09:33 +0200, Ricard Wanderlof wrote:
>   
>> (As note, it seems to be that logically, it shouldn't be necessary to set 
>> both length variables, as one appears to be for the main buffer, and 
>> one for the oob buffer, but this is how it is done in several places, 
>> including the code for the mtd character device MEMWRITEOOB and MEMREADOOB
>> ioctls. I'm not sure if this is a temporary solution during some rework of 
>> the mtd infrastructure, or whether there is a deeper thought here.)
>>     
>
> Hm. That seems wrong to me. If we don't want to read or write the main
> area, we should set ops.len to zero. And if that doesn't work, it should
> probably be fixed.
>
> I'd rather see a patch which fixes that.
>
>   
We discussed that w/ tglx some time ago and we couldn't find a 5 minute 
solution :)

Vitaly

Re: [PATCH] [MTD] [NAND]: nand_default_mark_blockbad() doesn't work when flash-based bbt not enabled

[Ricard Wanderlof]

On Tue, 24 Oct 2006, Vitaly Wool wrote:

>> Hm. That seems wrong to me. If we don't want to read or write the main
>> area, we should set ops.len to zero. And if that doesn't work, it should
>> probably be fixed.
>>
>> I'd rather see a patch which fixes that.
>>
>>
> We discussed that w/ tglx some time ago and we couldn't find a 5 minute
> solution :)

Was there a discussion on the mtd list? I scanned the archives back to 
April and couldn't find anything.

/Ricard
--
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [PATCH] [MTD] [NAND]: nand_default_mark_blockbad() doesn't work when flash-based bbt not enabled

[Vitaly Wool]

Ricard Wanderlof wrote:
> On Tue, 24 Oct 2006, Vitaly Wool wrote:
>
>   
>>> Hm. That seems wrong to me. If we don't want to read or write the main
>>> area, we should set ops.len to zero. And if that doesn't work, it should
>>> probably be fixed.
>>>
>>> I'd rather see a patch which fixes that.
>>>
>>>
>>>       
>> We discussed that w/ tglx some time ago and we couldn't find a 5 minute
>> solution :)
>>     
>
> Was there a discussion on the mtd list? I scanned the archives back to 
> April and couldn't find anything.
>   
No, we did chat in #mtd channel.

Anyway, here's what I think of that. It looks like the idea was to use 
ops.len in order to read more than ooblen bytes, i. e. more than one OOB 
area basically, and preserve ooblen to signal how many bytes there is in 
one oob. However, that seems misleading/redundant since you can always 
figure out how many bytes there are in oob, so it looks like making that 
all work w/ ooblen is better.

Does that make sense?
I'm going to work on that as soon as load permits...

Vitaly

Re: [PATCH] [MTD] [NAND]: nand_default_mark_blockbad() doesn't work when flash-based bbt not enabled

[Ricard Wanderlof]

On Thu, 26 Oct 2006, Vitaly Wool wrote:

> Anyway, here's what I think of that. It looks like the idea was to use 
> ops.len in order to read more than ooblen bytes, i. e. more than one OOB 
> area basically, and preserve ooblen to signal how many bytes there is in 
> one oob. However, that seems misleading/redundant since you can always 
> figure out how many bytes there are in oob, so it looks like making that 
> all work w/ ooblen is better.
>
> Does that make sense?

Well, I guess so, at least so far as "there was a good thought behind it 
all but it didn't really turn out as we'd expected" to my 
have-just-recently-studied-mtd-ears :-).

> I'm going to work on that as soon as load permits...

Yeah, I think I'd be up over my head in water if I tried to sort this out 
myself ...

/Ricard
--
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30