[RFC] the way to proceed with OOB data

[RFC] the way to proceed with OOB data

[Vitaly Wool]

Hi folks,

you may have noticed the RFC patches for OOB handling rework I've posted 
to this list about two weeks ago.
The main idea of those was a) to provide uniform method for handling OOB 
by such MTD users like JFFS2/YAFFS2 b) to hide the OOB internal 
structure from the users. However, people talking to me on that convined 
me that those changes were too radical for the moment and also pointed 
out the following problems that arise if my changes are accepted:
1. No more legacy support for jffs/yaffs OOB layouts. This is the point 
I don't consider to be major, since if something's deprecated it means 
that it'd go away at some point, so maybe now is the time? But maybe 
most of you guys think differently.
2. No more ability to read raw OOB data from the userspace which might 
turn to be a serious problem while debugging stuff. I must admit this is 
something we should preserve.
3. No binary compatibility for MTD utilities. Here I'm also willing to 
admit being too radical.

On the other hand, it's really necessary to provide means for  kernel 
MTD users not to mess with the oobinfos.
So, the question is: how to do it then? I see several ways of doing that.
1. Change the API for nand_read_ecc and nand_write_ecc, namely add OOB 
read/write length parameter. This is what Charles proposes; it's nice 
for MTD users but it will overcomplicate nand_read_ecc/nand_write_ecc, 
which are complicated enough at the moment.
2. Change the API for nand_read_oob/nand_write_oob so that it read/wrote 
either raw OOB or free OOB. This way has been discouraged by Artem 
during our discussion in #mtd, but this might be an option.
3. Leave everything as is and add a new functionto the MTD interface 
that will read/write only free OOB bytes as a single chunk of data.

Each of the options implies the need to extend ioctl interface to get 
oobavail/oob free data.

Any comments are welcome.

Vitaly

Re: [RFC] the way to proceed with OOB data

[Charles Manning]

On Thursday 15 December 2005 23:41, Vitaly Wool wrote:
> Hi folks,
>
> you may have noticed the RFC patches for OOB handling rework I've posted
> to this list about two weeks ago.
> The main idea of those was a) to provide uniform method for handling OOB
> by such MTD users like JFFS2/YAFFS2 b) to hide the OOB internal
> structure from the users. However, people talking to me on that convined
> me that those changes were too radical for the moment and also pointed
> out the following problems that arise if my changes are accepted:
> 1. No more legacy support for jffs/yaffs OOB layouts. This is the point
> I don't consider to be major, since if something's deprecated it means
> that it'd go away at some point, so maybe now is the time? But maybe
> most of you guys think differently.
> 2. No more ability to read raw OOB data from the userspace which might
> turn to be a serious problem while debugging stuff. I must admit this is
> something we should preserve.
> 3. No binary compatibility for MTD utilities. Here I'm also willing to
> admit being too radical.
>
> On the other hand, it's really necessary to provide means for  kernel
> MTD users not to mess with the oobinfos.
> So, the question is: how to do it then? I see several ways of doing that.
> 1. Change the API for nand_read_ecc and nand_write_ecc, namely add OOB
> read/write length parameter. This is what Charles proposes; it's nice
> for MTD users but it will overcomplicate nand_read_ecc/nand_write_ecc,
> which are complicated enough at the moment.

I disagree that it needs to make the current code more complicated.

I did submit a patch of sorts that was quite simple, but fractured the logic 
in do_read_ecc, do_read_ecc is pretty convoluted and effort should be made to 
not make it any more complicated.

I think the better solution is to just wrap the code better. ie:

static int nand_read_ecc (struct mtd_info *mtd, loff_t from, size_t len,
			  size_t * retlen, u_char * buf, u_char * oob_buf, struct nand_oobinfo 
*oobsel)
{

       if(buf) {    <-------- 
	/* use userspace supplied oobinfo, if zero */
	if (oobsel == NULL)
		oobsel = &mtd->oobinfo;
	return nand_do_read_ecc(mtd, from, len, retlen, buf, oob_buf, oobsel, 0xff);
       } else {   <--------------------
            /* just read the oob according to oobinfo */  
<---------------------
           call whatever local function <----------------------
      }  <----------------
}

This gives the neat interface without forcing more pain into do_read_ecc.


> 2. Change the API for nand_read_oob/nand_write_oob so that it read/wrote
> either raw OOB or free OOB. This way has been discouraged by Artem
> during our discussion in #mtd, but this might be an option.

> 3. Leave everything as is and add a new functionto the MTD interface
> that will read/write only free OOB bytes as a single chunk of data.

In some ways this is the best since all the other options have either  
compatability issues of one sort or another. It is also very clear what is 
going on. For this reason, this would be my preferred approach.

To explain:....
If we look at the nand_read_ecc soultion, there is compatability issue because 
if a new YAFFS is used with an old mtd then the fist time someone will know 
that something is wrong is when mtd oopses on a NULL buf. However, with an 
explicit read_oob_avail() function, we know at compile time.

Re: [RFC] the way to proceed with OOB data

[Vitaly Wool]

Charles Manning wrote:

>>1. Change the API for nand_read_ecc and nand_write_ecc, namely add OOB
>>read/write length parameter. This is what Charles proposes; it's nice
>>for MTD users but it will overcomplicate nand_read_ecc/nand_write_ecc,
>>which are complicated enough at the moment.
>>    
>>
>
>I disagree that it needs to make the current code more complicated.
>
>I did submit a patch of sorts that was quite simple, but fractured the logic 
>in do_read_ecc, do_read_ecc is pretty convoluted and effort should be made to 
>not make it any more complicated.
>
>I think the better solution is to just wrap the code better. ie:
>
>static int nand_read_ecc (struct mtd_info *mtd, loff_t from, size_t len,
>			  size_t * retlen, u_char * buf, u_char * oob_buf, struct nand_oobinfo 
>*oobsel)
>{
>
>       if(buf) {    <-------- 
>	/* use userspace supplied oobinfo, if zero */
>	if (oobsel == NULL)
>		oobsel = &mtd->oobinfo;
>	return nand_do_read_ecc(mtd, from, len, retlen, buf, oob_buf, oobsel, 0xff);
>       } else {   <--------------------
>            /* just read the oob according to oobinfo */  
><---------------------
>           call whatever local function <----------------------
>      }  <----------------
>  
>
That doesn't seem to be a comprehensive solution to me. You imply here 
that in case buf is NULL, OOB area of only one page is to be read which 
may be inconvenient in some cases. Basically that means that you need to 
specify two more parameters - the OOB data length to read and (ptr to 
the var for) the OOB data length actually read. On the other hand, this 
means you shouldn't ignore these parameters in nand_do_read_ecc call, 
thus it's either you make everything a lot more complicated or you get 
routime with quite a limited functionality for OOB reading/writing 
(suppose you have a long, say, cleanmarker which is pread across several 
pages of the eraseblock; you willthen have to figure out how many read 
calls to do in order to retrieve it all).
So, lemme a bit disagree with you, I hope that makes sense a bit :)

>>3. Leave everything as is and add a new functionto the MTD interface
>>that will read/write only free OOB bytes as a single chunk of data.
>>    
>>
>
>In some ways this is the best since all the other options have either  
>compatability issues of one sort or another. It is also very clear what is 
>going on. For this reason, this would be my preferred approach.
>  
>
Yeah, to me as well :)
I'm planning to work out something of a kind soon, maybe even before 
Christmas ;)

Best regards,
   Vitaly

Re: [RFC] the way to proceed with OOB data

[Charles Manning]

On Friday 16 December 2005 22:31, Vitaly Wool wrote:
> Charles Manning wrote:
> >>1. Change the API for nand_read_ecc and nand_write_ecc, namely add OOB
> >>read/write length parameter. This is what Charles proposes; it's nice
> >>for MTD users but it will overcomplicate nand_read_ecc/nand_write_ecc,
> >>which are complicated enough at the moment.
> >
> >I disagree that it needs to make the current code more complicated.
> >
> >I did submit a patch of sorts that was quite simple, but fractured the
> > logic in do_read_ecc, do_read_ecc is pretty convoluted and effort should
> > be made to not make it any more complicated.
> >
> >I think the better solution is to just wrap the code better. ie:
> >
> >static int nand_read_ecc (struct mtd_info *mtd, loff_t from, size_t len,
> >			  size_t * retlen, u_char * buf, u_char * oob_buf, struct nand_oobinfo
> >*oobsel)
> >{
> >
> >       if(buf) {    <--------
> >	/* use userspace supplied oobinfo, if zero */
> >	if (oobsel == NULL)
> >		oobsel = &mtd->oobinfo;
> >	return nand_do_read_ecc(mtd, from, len, retlen, buf, oob_buf, oobsel,
> > 0xff); } else {   <--------------------
> >            /* just read the oob according to oobinfo */
> ><---------------------
> >           call whatever local function <----------------------
> >      }  <----------------
>
> That doesn't seem to be a comprehensive solution to me. You imply here
> that in case buf is NULL, OOB area of only one page is to be read which
> may be inconvenient in some cases. Basically that means that you need to
> specify two more parameters - the OOB data length to read and (ptr to
> the var for) the OOB data length actually read. On the other hand, this
> means you shouldn't ignore these parameters in nand_do_read_ecc call,
> thus it's either you make everything a lot more complicated or you get
> routime with quite a limited functionality for OOB reading/writing
> (suppose you have a long, say, cleanmarker which is pread across several
> pages of the eraseblock; you willthen have to figure out how many read
> calls to do in order to retrieve it all).
> So, lemme a bit disagree with you, I hope that makes sense a bit :)

OK, I admit I'm looking at this through YAFFS eyes. YAFFS always writes one 
page at a time, so this would not be an issue.

Do any other fs or users write multiple NAND pages at a time? If so how do 
they handle any failures?

>
> >>3. Leave everything as is and add a new functionto the MTD interface
> >>that will read/write only free OOB bytes as a single chunk of data.
> >
> >In some ways this is the best since all the other options have either
> >compatability issues of one sort or another. It is also very clear what is
> >going on. For this reason, this would be my preferred approach.
>
> Yeah, to me as well :)
> I'm planning to work out something of a kind soon, maybe even before
> Christmas ;)

Cool :-) If you get that done by Christmas, I'll try to get the YAFFS side 
done by New Year. It is summer here so that is a bit of a challenge.

I don't think the programming effort is really going to be much different one 
way or another, it is just how to serve it up to the customers.

Thanx Vitaly.


-- Charles

Re: [RFC] the way to proceed with OOB data

[Thomas Gleixner]

On Sat, 2005-12-17 at 08:53 +1300, Charles Manning wrote:
> > So, lemme a bit disagree with you, I hope that makes sense a bit :)
> 
> OK, I admit I'm looking at this through YAFFS eyes. YAFFS always writes one 
> page at a time, so this would not be an issue.
> 
> Do any other fs or users write multiple NAND pages at a time? If so how do 
> they handle any failures?

Well, return an error and the number of safely written bytes. The fs
requested n bytes to be written and gets the number of actually written
bytes. It makes not much of a difference to handle such a scenario or
the write per page failure.

	tglx