| Message ID | e51a260aa56112648b5413c66d4036e489ae09f7.1312922073.git.bengardiner@nanometrics.ca |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 9, 2011 at 1:57 PM, Ben Gardiner <bengardiner@nanometrics.ca> wrote: > Add the ECC size and OOB available to that list. The other entries in the > ecclayout struct are arrays and are not as meaningful as eccsize and oobavail. > --- > I noticed that the struct nand_ecclayout_user and its corresponding ioctl > has been marked deprecated; however, since this is a 'debug' utility and at > least one person found it useful I thought it would be a good idea to propose > these changes anyways. While I agree with your rationale for ignoring the deprecation (it's OK for a debug utility), users of this feature should be warned that struct nand_ecclayout_user will not report all information if there are more ECC entries than "MTD_MAX_ECCPOS_ENTRIES" (i.e., 64). It will simply truncate ECC at 64. This may be a problem for large page flash with large ECC regions. It's a similar story for "oobavail" as well. And before the question is asked, I'm not sure how to implement my suggestion that "users of this feature should be warned". Perhaps just a comment in the code? Brian
Hi Brian, On Wed, Aug 10, 2011 at 1:07 PM, Brian Norris <computersforpeace@gmail.com> wrote: > On Tue, Aug 9, 2011 at 1:57 PM, Ben Gardiner <bengardiner@nanometrics.ca> wrote: >> Add the ECC size and OOB available to that list. The other entries in the >> ecclayout struct are arrays and are not as meaningful as eccsize and oobavail. >> --- >> I noticed that the struct nand_ecclayout_user and its corresponding ioctl >> has been marked deprecated; however, since this is a 'debug' utility and at >> least one person found it useful I thought it would be a good idea to propose >> these changes anyways. > > While I agree with your rationale for ignoring the deprecation (it's > OK for a debug utility), Thanks for the review -- and for agreeing with me :) > users of this feature should be warned that > struct nand_ecclayout_user will not report all information if there > are more ECC entries than "MTD_MAX_ECCPOS_ENTRIES" (i.e., 64). It will > simply truncate ECC at 64. This may be a problem for large page flash > with large ECC regions. It's a similar story for "oobavail" as well. > > And before the question is asked, I'm not sure how to implement my > suggestion that "users of this feature should be warned". Perhaps just > a comment in the code? Sounds reasonable to me -- I will insert a warning comment in v2. Best Regards, Ben Gardiner --- Nanometrics Inc. http://www.nanometrics.ca
On Tue, 2011-08-09 at 16:57 -0400, Ben Gardiner wrote: > The mtd_debug 'info' command reports a great deal of useful information about > the mtd device is it given for a query. > > Add the ECC size and OOB available to that list. The other entries in the > ecclayout struct are arrays and are not as meaningful as eccsize and oobavail. > > Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca> > > --- > > I noticed that the struct nand_ecclayout_user and its corresponding ioctl > has been marked deprecated; however, since this is a 'debug' utility and at > least one person found it useful I thought it would be a good idea to propose > these changes anyways. I think we could just kill depricated ioctls as soon as we make mtd-utils stop using them. If you add this dependency - it will be more difficult to kill them.
On Wed, 2011-08-10 at 10:07 -0700, Brian Norris wrote: > On Tue, Aug 9, 2011 at 1:57 PM, Ben Gardiner <bengardiner@nanometrics.ca> wrote: > > Add the ECC size and OOB available to that list. The other entries in the > > ecclayout struct are arrays and are not as meaningful as eccsize and oobavail. > > --- > > I noticed that the struct nand_ecclayout_user and its corresponding ioctl > > has been marked deprecated; however, since this is a 'debug' utility and at > > least one person found it useful I thought it would be a good idea to propose > > these changes anyways. > > While I agree with your rationale for ignoring the deprecation (it's > OK for a debug utility), users of this feature should be warned that > struct nand_ecclayout_user will not report all information if there > are more ECC entries than "MTD_MAX_ECCPOS_ENTRIES" (i.e., 64). It will > simply truncate ECC at 64. This may be a problem for large page flash > with large ECC regions. It's a similar story for "oobavail" as well. > > And before the question is asked, I'm not sure how to implement my > suggestion that "users of this feature should be warned". Perhaps just > a comment in the code? I guess we can do the following: 1. make sure mtd-utils does not use the depricated ioctl 2. release such mtd-utils 3. change the kernel and add a warning printk there 4. wait some time and kill the ioctl from the kernel.
On Wed, Aug 17, 2011 at 4:03 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > On Tue, 2011-08-09 at 16:57 -0400, Ben Gardiner wrote: >> The mtd_debug 'info' command reports a great deal of useful information about >> the mtd device is it given for a query. >> >> Add the ECC size and OOB available to that list. The other entries in the >> ecclayout struct are arrays and are not as meaningful as eccsize and oobavail. >> >> Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca> >> >> --- >> >> I noticed that the struct nand_ecclayout_user and its corresponding ioctl >> has been marked deprecated; however, since this is a 'debug' utility and at >> least one person found it useful I thought it would be a good idea to propose >> these changes anyways. > > I think we could just kill depricated ioctls as soon as we make > mtd-utils stop using them. If you add this dependency - it will be more > difficult to kill them. Fair enough -- I'm happy to hear a definitive answer; thank you, Artem. In PATCH 3/3 I use the same ioctl to get the ECC size for the purposes of producing a BER estimate. What is the right way of getting the ECC size now that ECCGETLAYOUT is deprecated? Is this even something that should be from userspace at all? Best Regards, Ben Gardiner --- Nanometrics Inc. http://www.nanometrics.ca
On Thu, 2011-08-18 at 10:01 -0400, Ben Gardiner wrote: > Fair enough -- I'm happy to hear a definitive answer; thank you, Artem. > > In PATCH 3/3 I use the same ioctl to get the ECC size for the purposes > of producing a BER estimate. What is the right way of getting the ECC > size now that ECCGETLAYOUT is deprecated? Is this even something that > should be from userspace at all? I do not know. I think we need to expose it via sysfs. I think that in general sysfs is better that "informational" ioctls.
On Mon, Aug 22, 2011 at 7:00 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > On Thu, 2011-08-18 at 10:01 -0400, Ben Gardiner wrote: >> Fair enough -- I'm happy to hear a definitive answer; thank you, Artem. >> >> In PATCH 3/3 I use the same ioctl to get the ECC size for the purposes >> of producing a BER estimate. What is the right way of getting the ECC >> size now that ECCGETLAYOUT is deprecated? Is this even something that >> should be from userspace at all? > > I do not know. I think we need to expose it via sysfs. I think that in > general sysfs is better that "informational" ioctls. Ok -- I've heard that said before and for what it's worth I agree -- sysfs is better. However, I'm seeing that there is no available sysfs mechanism at present. I'm sorry but I can't take on the time to introduce one although I agree it is best and I would like to be able to. Please drop patches 1/3 and 3/3 from consideration. Patch 2/3 'print number of bits corrected during test' is still useful and applicable to mtd-utils -- IMHO. Best Regards, Ben Gardiner --- Nanometrics Inc. http://www.nanometrics.ca
diff --git a/mtd_debug.c b/mtd_debug.c index b82dabe..ea2a119 100644 --- a/mtd_debug.c +++ b/mtd_debug.c @@ -74,6 +74,14 @@ static int getregions (int fd,struct region_info_user *regions,int *n) return (0); } +/* + * ECCGETLAYOUT + */ +static int getecclayout (int fd, struct nand_ecclayout_user *ecclayout) +{ + return (ioctl (fd,ECCGETLAYOUT,ecclayout)); +} + int erase_flash (int fd,u_int32_t offset,u_int32_t bytes) { int err; @@ -237,6 +245,7 @@ int showinfo (int fd) { int i,err,n; struct mtd_info_user mtd; + struct nand_ecclayout_user ecclayout; static struct region_info_user region[1024]; err = getmeminfo (fd,&mtd); @@ -246,6 +255,13 @@ int showinfo (int fd) return (1); } + err = getecclayout (fd,&ecclayout); + if (err < 0) + { + perror ("ECCGETLAYOUT"); + return (1); + } + err = getregions (fd,region,&n); if (err < 0) { @@ -330,6 +346,12 @@ int showinfo (int fd) printf ("\nmtd.oobsize = "); printsize (mtd.oobsize); + printf ("\necclayout.eccbytes = "); + printsize (ecclayout.eccbytes); + + printf ("\necclayout.oobavail = "); + printsize (ecclayout.oobavail); + printf ("\n" "regions = %d\n" "\n",
The mtd_debug 'info' command reports a great deal of useful information about the mtd device is it given for a query. Add the ECC size and OOB available to that list. The other entries in the ecclayout struct are arrays and are not as meaningful as eccsize and oobavail. Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca> --- I noticed that the struct nand_ecclayout_user and its corresponding ioctl has been marked deprecated; however, since this is a 'debug' utility and at least one person found it useful I thought it would be a good idea to propose these changes anyways. --- mtd_debug.c | 22 ++++++++++++++++++++++ 1 files changed, 22 insertions(+), 0 deletions(-)