Re: [PATCH] mtd/utils: sync with MTD ioctl interface rework to get rid of MEMGETOOBSEL/MEMSETOOBSEL

[Vitaly Wool <vwool@ru.mvista.com>]

Hi Todd,

Todd Poynor wrote:

> Hi Vitaly, my comments...
>
>> +++ util/nanddump.c    2 Dec 2005 06:54:11 -0000
>> @@ -163,6 +163,7 @@
>>      struct mtd_oob_buf oob = {0, 16, oobbuf};
>>      mtd_info_t meminfo;
>>      char pretty_buf[80];
>> +    uint32_t oobavail;
>>  
>>       process_options(argc, argv);
>>  
>> @@ -179,6 +180,13 @@
>>          exit (1);
>>      }
>>  
>> +    if (ioctl(fd, MEMGETOOBAVAIL, &oobavail) != 0) {
>> +        perror("unable to get NAND oobavail");
>> +        close(fd);
>> +        exit(1);
>> +    }
>
>
> How about fall back to all 16/64/etc. avail?
>
> For debugging purposes it can be nice to see the raw OOB, in case that 
> can be shoehorned in here somehow.

Well, the main idea of what I'm implementing is to hide the internal 
structure of the OOB data from user. Therefore it's no guaranteed way in 
this implementation to read the raw OOB.
On the other hand, not being able to get oobavail means a problem so I 
wouldn't try to read OOB in this situation.
However I can a) add a flag to both read_oob/write_oob whether to read 
oobavail or raw OOB and implement corresponding functionality/flag in 
nanddump, or) fall back to considering oobavail being equal to oobsize 
if getting oobavail fails but neither variant seems good to me...

>
>> +++ util/nandwrite.c    2 Dec 2005 06:54:11 -0000
>
>
>> -           "  -j, --jffs2           force jffs2 oob layout (legacy 
>> support)\n"
>> -           "  -y, --yaffs           force yaffs oob layout (legacy 
>> support)\n"
>
>
> Again, it may be good to keep a legacy mode around during the 
> transition period.

I'm afraid it's not possible. And this legacy support has been sheduled 
to go away long ago, I hope now is a good time :)

>
>>          if (writeoob) {
>>              /* Read OOB data from input file, exit on failure */
>> -            if ((cnt = read(ifd, oobreadbuf, meminfo.oobsize)) != 
>> meminfo.oobsize) {
>> +            if ((cnt = read(ifd, oobreadbuf, oobavail)) != oobavail) {
>
>
> This requires the input file to be tailored to the oobavail of a 
> specific destination device, reducing the benefit of autoplacement.  
> It may be best to continue to pad input files oob data to the full 
> oobsize, much like the data portion is handled.

Well, padding doesn't work in nandwrite if the image to be written 
contains OOB data. The idea as I see it is if you are trying to write 
the image with OOB data, you should know what you're doing, and that 
implies knowledge of the oobavail size.
On the other hand, it might be useful to implement an option for 
nandwrite which specifies what OOB data length user supposes (default 
will be the oobavail).

>
>> +                memcpy(oobbuf, oobreadbuf, oobavail);
>
>
> No need for oobreadbuf and copy if no OOB layout processing done anymore?
>
Yeah, thanks.
And 'noecc' parameter should also go away. An update of the patch will 
follow...

Vitaly