Patchwork [28/74] Incrementing the ecc_pos array to contain 128 char

login
register
mail settings
Submitter Viresh KUMAR
Date Aug. 30, 2010, 10:43 a.m.
Message ID <a750eadbd743c3a2d5dc8b35c25e0ee82f569e87.1283161023.git.viresh.kumar@st.com>
Download mbox | patch
Permalink /patch/63018/
State New
Headers show

Comments

Viresh KUMAR - Aug. 30, 2010, 10:43 a.m.
From: Vipin Kumar <vipin.kumar@st.com>

For a page size of 4096, the number of oob bytes are 128 per page. This demands
for an increased ecc_pos array as well as ecc can be placed at more than 64
locations.

This patch is added because SPEAr platforms NAND controller has 13 bytes of ecc
for each 512 bytes of data. This results in 104 bytes of ecc in case of 4096
byte page.

Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 include/mtd/mtd-abi.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Artem Bityutskiy - Aug. 30, 2010, 12:14 p.m.
On Mon, 2010-08-30 at 16:13 +0530, Viresh KUMAR wrote:
> From: Vipin Kumar <vipin.kumar@st.com>
> 
> For a page size of 4096, the number of oob bytes are 128 per page. This demands
> for an increased ecc_pos array as well as ecc can be placed at more than 64
> locations.
> 
> This patch is added because SPEAr platforms NAND controller has 13 bytes of ecc
> for each 512 bytes of data. This results in 104 bytes of ecc in case of 4096
> byte page.
> 
> Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>

Nack, breaking ABI Is not allowed in Linux.
Vipin Kumar - Aug. 31, 2010, 6:34 a.m.
On 8/30/2010 5:44 PM, Artem Bityutskiy wrote:
> On Mon, 2010-08-30 at 16:13 +0530, Viresh KUMAR wrote:
>> From: Vipin Kumar <vipin.kumar@st.com>
>>
>> For a page size of 4096, the number of oob bytes are 128 per page. This demands
>> for an increased ecc_pos array as well as ecc can be placed at more than 64
>> locations.
>>
>> This patch is added because SPEAr platforms NAND controller has 13 bytes of ecc
>> for each 512 bytes of data. This results in 104 bytes of ecc in case of 4096
>> byte page.
>>
>> Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
>> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> 
> Nack, breaking ABI Is not allowed in Linux.
> 
Hello Artem,

I could not understand your point. Can you please elaborate. How does this patch 
break ABI

Regards
Vipin
Artem Bityutskiy - Aug. 31, 2010, 11:36 p.m.
Hi,

On Tue, 2010-08-31 at 12:04 +0530, Vipin Kumar wrote:
> > Nack, breaking ABI Is not allowed in Linux.
> I could not understand your point. Can you please elaborate. How does this patch 
> break ABI

You are changing data structure (struct nand_ecclayout) used for in MTD
ioctl. Tha ioctl is part of the Linux ABI. By changing the data
structure, you are breaking the ABI. This means that current binaries
would stop working with newer versions of the Linux kernel if we'd
accept your patch.
Vipin Kumar - Sept. 1, 2010, 4:13 a.m.
On 9/1/2010 5:06 AM, Artem Bityutskiy wrote:
> Hi,
> 
> On Tue, 2010-08-31 at 12:04 +0530, Vipin Kumar wrote:
>>> Nack, breaking ABI Is not allowed in Linux.
>> I could not understand your point. Can you please elaborate. How does this patch 
>> break ABI
> 
> You are changing data structure (struct nand_ecclayout) used for in MTD
> ioctl. Tha ioctl is part of the Linux ABI. By changing the data
> structure, you are breaking the ABI. This means that current binaries
> would stop working with newer versions of the Linux kernel if we'd
> accept your patch.
> 
Hello,

The only change that I have made is increasing the number of bytes to keep ecc. 
Since the ecc is generally kept in spare area, it makes sense to have the ecc 
locations to be equal to the maximum spare area possible.

A NAND page with a page size of 4096 would contain a spare area of 128 bytes. 
Now, ecc for the page can be less/more than 64 bytes(currently allocated for 
ecc positions) depending on the algorithm used to generate ecc. 
Incidently, in our case the ecc can fit in 104 bytes and this is still quite 
logical to place it in spare area since the linux image supports 4096 page but 
the problem is that the ecc locations supported by linux are less than the 
practically possible scenario so in effect this change is an improvement in linux

Please let me know if you disagree

Regards
Vipin
Artem Bityutskiy - Sept. 1, 2010, 10:45 a.m.
On Wed, 2010-09-01 at 09:43 +0530, Vipin Kumar wrote:
> On 9/1/2010 5:06 AM, Artem Bityutskiy wrote:
> > Hi,
> > 
> > On Tue, 2010-08-31 at 12:04 +0530, Vipin Kumar wrote:
> >>> Nack, breaking ABI Is not allowed in Linux.
> >> I could not understand your point. Can you please elaborate. How does this patch 
> >> break ABI
> > 
> > You are changing data structure (struct nand_ecclayout) used for in MTD
> > ioctl. Tha ioctl is part of the Linux ABI. By changing the data
> > structure, you are breaking the ABI. This means that current binaries
> > would stop working with newer versions of the Linux kernel if we'd
> > accept your patch.
> > 
> Hello,
> 
> The only change that I have made is increasing the number of bytes to keep ecc. 

Right, but this break ABI.

> Since the ecc is generally kept in spare area, it makes sense to have the ecc 
> locations to be equal to the maximum spare area possible.

May be.

> A NAND page with a page size of 4096 would contain a spare area of 128 bytes. 
> Now, ecc for the page can be less/more than 64 bytes(currently allocated for 
> ecc positions) depending on the algorithm used to generate ecc. 
> Incidently, in our case the ecc can fit in 104 bytes and this is still quite 
> logical to place it in spare area since the linux image supports 4096 page but 
> the problem is that the ecc locations supported by linux are less than the 
> practically possible scenario so in effect this change is an improvement in linux

Yes, this is historical and a bit unfortunate, but you cannot break ABI
even if you have reasons like that

> Please let me know if you disagree

Please, create an app which uses 'struct nand_ecclayout' and compile it
against the old headers. Check that it works. Then do you kernel
modification and run the same program (without re-compiling) and my
prediction is that it won't work. This is what I call ABI breaking which
is disallowed.
Vipin Kumar - Sept. 1, 2010, 11:04 a.m.
On 9/1/2010 4:15 PM, Artem Bityutskiy wrote:
> On Wed, 2010-09-01 at 09:43 +0530, Vipin Kumar wrote:
>> On 9/1/2010 5:06 AM, Artem Bityutskiy wrote:
>>> Hi,
>>>
>>> On Tue, 2010-08-31 at 12:04 +0530, Vipin Kumar wrote:
>>>>> Nack, breaking ABI Is not allowed in Linux.
>>>> I could not understand your point. Can you please elaborate. How does this patch 
>>>> break ABI
>>>
>>> You are changing data structure (struct nand_ecclayout) used for in MTD
>>> ioctl. Tha ioctl is part of the Linux ABI. By changing the data
>>> structure, you are breaking the ABI. This means that current binaries
>>> would stop working with newer versions of the Linux kernel if we'd
>>> accept your patch.
>>>
>> Hello,
>>
>> The only change that I have made is increasing the number of bytes to keep ecc. 
> 
> Right, but this break ABI.
> 
>> Since the ecc is generally kept in spare area, it makes sense to have the ecc 
>> locations to be equal to the maximum spare area possible.
> 
> May be.
> 
>> A NAND page with a page size of 4096 would contain a spare area of 128 bytes. 
>> Now, ecc for the page can be less/more than 64 bytes(currently allocated for 
>> ecc positions) depending on the algorithm used to generate ecc. 
>> Incidently, in our case the ecc can fit in 104 bytes and this is still quite 
>> logical to place it in spare area since the linux image supports 4096 page but 
>> the problem is that the ecc locations supported by linux are less than the 
>> practically possible scenario so in effect this change is an improvement in linux
> 

Hello David/Artem,

I got the point, but this change is essential (at least for me). It may be 
essential to others as well in near future. Please let me know how to handle more 
than 64 bytes of ecc

> Yes, this is historical and a bit unfortunate, but you cannot break ABI
> even if you have reasons like that
> 
>> Please let me know if you disagree
> 
> Please, create an app which uses 'struct nand_ecclayout' and compile it
> against the old headers. Check that it works. Then do you kernel
> modification and run the same program (without re-compiling) and my
> prediction is that it won't work. This is what I call ABI breaking which
> is disallowed.
> 

Regards
Vipin
Ryan Mallon - Sept. 1, 2010, 9:23 p.m.
On 09/01/2010 11:04 PM, Vipin Kumar wrote:
> On 9/1/2010 4:15 PM, Artem Bityutskiy wrote:
>> On Wed, 2010-09-01 at 09:43 +0530, Vipin Kumar wrote:
>>> On 9/1/2010 5:06 AM, Artem Bityutskiy wrote:
>>>> Hi,
>>>>
>>>> On Tue, 2010-08-31 at 12:04 +0530, Vipin Kumar wrote:
>>>>>> Nack, breaking ABI Is not allowed in Linux.
>>>>> I could not understand your point. Can you please elaborate. How does this patch 
>>>>> break ABI
>>>>
>>>> You are changing data structure (struct nand_ecclayout) used for in MTD
>>>> ioctl. Tha ioctl is part of the Linux ABI. By changing the data
>>>> structure, you are breaking the ABI. This means that current binaries
>>>> would stop working with newer versions of the Linux kernel if we'd
>>>> accept your patch.
>>>>
>>> Hello,
>>>
>>> The only change that I have made is increasing the number of bytes to keep ecc. 
>>
>> Right, but this break ABI.
>>
>>> Since the ecc is generally kept in spare area, it makes sense to have the ecc 
>>> locations to be equal to the maximum spare area possible.
>>
>> May be.
>>
>>> A NAND page with a page size of 4096 would contain a spare area of 128 bytes. 
>>> Now, ecc for the page can be less/more than 64 bytes(currently allocated for 
>>> ecc positions) depending on the algorithm used to generate ecc. 
>>> Incidently, in our case the ecc can fit in 104 bytes and this is still quite 
>>> logical to place it in spare area since the linux image supports 4096 page but 
>>> the problem is that the ecc locations supported by linux are less than the 
>>> practically possible scenario so in effect this change is an improvement in linux
>>
> 
> Hello David/Artem,
> 
> I got the point, but this change is essential (at least for me). It may be 
> essential to others as well in near future. Please let me know how to handle more 
> than 64 bytes of ecc
> 
>> Yes, this is historical and a bit unfortunate, but you cannot break ABI
>> even if you have reasons like that

We have run into this same problem using large page NAND chips. Our fix
in the past has been the same as presented here simply because it is
easy and for custom boards we do not care if we break the kernel ABI
since the kernel will only be used on our custom board.

However, we are interested in having a proper solution to this problem.
Basically the size of the eccpos array shouldn't be a fixed. Probably
there should be two ioctls, one to get the size of the eccpos array and
a second to get the actual array.

It is unfortunate that we already have the obsolete nand_oobinfo struct,
and any fix to this problem is probably going to obsolete the
nand_ecclayout struct, but we do need to fix this as the existing
infrastructure is not suitable for larger nand chips.

~Ryan
Kevin Cernekee - Sept. 1, 2010, 9:54 p.m.
On Wed, Sep 1, 2010 at 2:23 PM, Ryan Mallon <ryan@bluewatersys.com> wrote:
> However, we are interested in having a proper solution to this problem.

I would suggest using this patch as a starting point - could you
please review the changes and try it out?

http://git.infradead.org/users/dedekind/l2-mtd-2.6.git/commitdiff/b6d6ae730be2750fac166ed9df11ee6ea54d9160

Thanks.
Ryan Mallon - Sept. 1, 2010, 10:21 p.m.
On 09/02/2010 09:54 AM, Kevin Cernekee wrote:
> On Wed, Sep 1, 2010 at 2:23 PM, Ryan Mallon <ryan@bluewatersys.com> wrote:
>> However, we are interested in having a proper solution to this problem.
> 
> I would suggest using this patch as a starting point - could you
> please review the changes and try it out?
> 
> http://git.infradead.org/users/dedekind/l2-mtd-2.6.git/commitdiff/b6d6ae730be2750fac166ed9df11ee6ea54d9160

That patch still breaks the ABI by renaming struct nand_ecclayout to
nand_ecclayout_user. Any application using the old names will have to be
rewritten to compile against a new kernel.

The old interface should remain unchanged in that include/mtd/mtd-abi.h.
If an application using the old interface calls any of the ecc ioctls
for a nand chip with > 64 bytes ecc it should return an error.

I still think the eccpos field should be a pointer, so that it can
allocate as much space as is needed for the ecc. This also means that
the kernel doesn't need to be changed every time a new NAND chips
appears with a bigger ecc size.

~Ryan
Artem Bityutskiy - Sept. 1, 2010, 10:53 p.m.
On Thu, 2010-09-02 at 10:21 +1200, Ryan Mallon wrote:
> On 09/02/2010 09:54 AM, Kevin Cernekee wrote:
> > On Wed, Sep 1, 2010 at 2:23 PM, Ryan Mallon <ryan@bluewatersys.com> wrote:
> >> However, we are interested in having a proper solution to this problem.
> > 
> > I would suggest using this patch as a starting point - could you
> > please review the changes and try it out?
> > 
> > http://git.infradead.org/users/dedekind/l2-mtd-2.6.git/commitdiff/b6d6ae730be2750fac166ed9df11ee6ea54d9160
> 
> That patch still breaks the ABI by renaming struct nand_ecclayout to
> nand_ecclayout_user.

Hmm, would you please show why my binary file 'nandwrite' would have to
be re-built?

>  Any application using the old names will have to be
> rewritten to compile against a new kernel.

Not necessary. mtd-utils, for example, maintain their own copy of the
mtd-abi.h file, which I personally think is a good idea.

But if your apps depend on the kernel headers, then yes, you will need
to amend it or copy the old header into your project.

IOW, the patch breaks API by renaming, but not ABI, I think. I do not
see this as a major problem - just a small inconvenience.

> The old interface should remain unchanged in that include/mtd/mtd-abi.h.
> If an application using the old interface calls any of the ecc ioctls
> for a nand chip with > 64 bytes ecc it should return an error.

It will, because size of the structure is part of the ioctl number,
even. See the corresponding ioctl macro definition.

> I still think the eccpos field should be a pointer, so that it can
> allocate as much space as is needed for the ecc. This also means that
> the kernel doesn't need to be changed every time a new NAND chips
> appears with a bigger ecc size.

Sure, this is the whole point: go ahead and design the new ioctl, and
then deprecate the old one. Just invest men/hours into that and we are
all right :-)
Artem Bityutskiy - Sept. 1, 2010, 11:23 p.m.
Hi,

On Wed, 2010-09-01 at 16:34 +0530, Vipin Kumar wrote:
> I got the point, but this change is essential (at least for me). It may be 
> essential to others as well in near future. Please let me know how to handle more 
> than 64 bytes of ecc

Invent new ioctl, and declare the old one deprecated.
Ryan Mallon - Sept. 1, 2010, 11:37 p.m.
On 09/02/2010 10:53 AM, Artem Bityutskiy wrote:
> On Thu, 2010-09-02 at 10:21 +1200, Ryan Mallon wrote:
>> On 09/02/2010 09:54 AM, Kevin Cernekee wrote:
>>> On Wed, Sep 1, 2010 at 2:23 PM, Ryan Mallon <ryan@bluewatersys.com> wrote:
>>>> However, we are interested in having a proper solution to this problem.
>>>
>>> I would suggest using this patch as a starting point - could you
>>> please review the changes and try it out?
>>>
>>> http://git.infradead.org/users/dedekind/l2-mtd-2.6.git/commitdiff/b6d6ae730be2750fac166ed9df11ee6ea54d9160
>>
>> That patch still breaks the ABI by renaming struct nand_ecclayout to
>> nand_ecclayout_user.
> 
> Hmm, would you please show why my binary file 'nandwrite' would have to
> be re-built?
> 
>>  Any application using the old names will have to be
>> rewritten to compile against a new kernel.
> 
> Not necessary. mtd-utils, for example, maintain their own copy of the
> mtd-abi.h file, which I personally think is a good idea.
> 
> But if your apps depend on the kernel headers, then yes, you will need
> to amend it or copy the old header into your project.
> 
> IOW, the patch breaks API by renaming, but not ABI, I think. I do not
> see this as a major problem - just a small inconvenience.

Sorry, you are correct, it breaks the API not the ABI. I understood that
such changes were still to be avoided.

>> The old interface should remain unchanged in that include/mtd/mtd-abi.h.
>> If an application using the old interface calls any of the ecc ioctls
>> for a nand chip with > 64 bytes ecc it should return an error.
> 
> It will, because size of the structure is part of the ioctl number,
> even. See the corresponding ioctl macro definition.

Right, but I don't think it needs the complexity of the shrink_ecc
function from Kevin's patch. The old ioctl should return an error
(possibly with a kernel message) if the requested nand chip has > 64
bytes ecc. Otherwise it should just copy upto 64 bytes into the old
nand_ecclayout structure and return.

Any new structure should be totally independent and should, IMHO, have
an arbitrary sized array for the ecc. It should have its own new set of
ioctls, one for getting the size of the ecc and one for getting the
actual ecc data. Though I can't remember if ioctls support arbitrary
sized arguments to make this possible.

~Ryan
Ryan Mallon - Sept. 1, 2010, 11:43 p.m.
On 09/02/2010 11:37 AM, Ryan Mallon wrote:
> On 09/02/2010 10:53 AM, Artem Bityutskiy wrote:
>> On Thu, 2010-09-02 at 10:21 +1200, Ryan Mallon wrote:
>>> On 09/02/2010 09:54 AM, Kevin Cernekee wrote:
>>>> On Wed, Sep 1, 2010 at 2:23 PM, Ryan Mallon <ryan@bluewatersys.com> wrote:
>>>>> However, we are interested in having a proper solution to this problem.
>>>>
>>>> I would suggest using this patch as a starting point - could you
>>>> please review the changes and try it out?
>>>>
>>>> http://git.infradead.org/users/dedekind/l2-mtd-2.6.git/commitdiff/b6d6ae730be2750fac166ed9df11ee6ea54d9160
>>>
>>> That patch still breaks the ABI by renaming struct nand_ecclayout to
>>> nand_ecclayout_user.
>>
>> Hmm, would you please show why my binary file 'nandwrite' would have to
>> be re-built?
>>
>>>  Any application using the old names will have to be
>>> rewritten to compile against a new kernel.
>>
>> Not necessary. mtd-utils, for example, maintain their own copy of the
>> mtd-abi.h file, which I personally think is a good idea.
>>
>> But if your apps depend on the kernel headers, then yes, you will need
>> to amend it or copy the old header into your project.
>>
>> IOW, the patch breaks API by renaming, but not ABI, I think. I do not
>> see this as a major problem - just a small inconvenience.
> 
> Sorry, you are correct, it breaks the API not the ABI. I understood that
> such changes were still to be avoided.
> 
>>> The old interface should remain unchanged in that include/mtd/mtd-abi.h.
>>> If an application using the old interface calls any of the ecc ioctls
>>> for a nand chip with > 64 bytes ecc it should return an error.
>>
>> It will, because size of the structure is part of the ioctl number,
>> even. See the corresponding ioctl macro definition.
> 
> Right, but I don't think it needs the complexity of the shrink_ecc
> function from Kevin's patch. The old ioctl should return an error
> (possibly with a kernel message) if the requested nand chip has > 64
> bytes ecc. Otherwise it should just copy upto 64 bytes into the old
> nand_ecclayout structure and return.

Oops, I misread the patch. The shrink_ecclayout function is actually
fine. The only change I would make is to possible add a warning/error if
the data is being truncated.

~Ryan
Brian Norris - Sept. 2, 2010, 6:33 a.m.
On 9/1/2010 3:21 PM, Ryan Mallon wrote:
> On 09/02/2010 09:54 AM, Kevin Cernekee wrote:
>> On Wed, Sep 1, 2010 at 2:23 PM, Ryan Mallon <ryan@bluewatersys.com> wrote:
>> http://git.infradead.org/users/dedekind/l2-mtd-2.6.git/commitdiff/b6d6ae730be2750fac166ed9df11ee6ea54d9160
> 
> That patch still breaks the ABI by renaming struct nand_ecclayout to
> nand_ecclayout_user. Any application using the old names will have to be
> rewritten to compile against a new kernel.

I don't know how exactly all user-space apps deal with this, but isn't
that what the following typedef is for? (include/mtd/mtd-user.h)
	typedef struct nand_ecclayout nand_ecclayout_t;
changed to:
	typedef struct nand_ecclayout_user nand_ecclayout_t;

So any app that properly used nand_ecclayout_t would not need changes
even when using the new headers. Again, I don't know who has been using
what in user-space.

> The old interface should remain unchanged in that include/mtd/mtd-abi.h.
> If an application using the old interface calls any of the ecc ioctls
> for a nand chip with > 64 bytes ecc it should return an error.
> 
> I still think the eccpos field should be a pointer, so that it can
> allocate as much space as is needed for the ecc. This also means that
> the kernel doesn't need to be changed every time a new NAND chips
> appears with a bigger ecc size.

Yes, dynamic allocation would probably make more sense in the future.
However, it seems difficult (or impossible?) to create an
arbitrary-sized ioctl; the size is hard-coded into the ioctl definition.

I'm curious how many applications actually need to have the ecclayout.
nandwrite doesn't actually use it, just oobinfo.
Artem Bityutskiy - Sept. 2, 2010, 9:49 a.m.
On Wed, 2010-09-01 at 23:33 -0700, Brian Norris wrote:
> 
> I'm curious how many applications actually need to have the ecclayout.
> nandwrite doesn't actually use it, just oobinfo. 

That is good point - I think we should not accept new ioctl before the
submitter shows us how and for what he will use it.

My point is simple - OOB layout should be transparent to users-space.
Messing with this is error-prone.

Patch

diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
index 4debb45..ff3f30e 100644
--- a/include/mtd/mtd-abi.h
+++ b/include/mtd/mtd-abi.h
@@ -150,7 +150,7 @@  struct nand_oobfree {
  */
 struct nand_ecclayout {
 	__u32 eccbytes;
-	__u32 eccpos[64];
+	__u32 eccpos[128];
 	__u32 oobavail;
 	struct nand_oobfree oobfree[MTD_MAX_OOBFREE_ENTRIES];
 };