| Message ID | a750eadbd743c3a2d5dc8b35c25e0ee82f569e87.1283161023.git.viresh.kumar@st.com |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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
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.
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
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
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.
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
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 :-)
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.
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
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
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.
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.
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]; };