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

[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