Message ID | 4C746DE0.7000104@gmail.com |
---|---|
State | New, archived |
Headers | show |
On Tue, 2010-08-24 at 18:12 -0700, Brian Norris wrote: > My e-mail address has changed, since I am no longer working at Broadcom. > I will still be able to track messages to my old account if the MTD mailing > list is CC'd. Oh, does it mean you will stop loving MTD and we won't see steady flow of improvements for you? :-( BTW, I think you have been doing great job - MTD subsystem needs love badly! > Note that on the same subject (different thread) David suggested our new > struct be allocated dynamically: Yes, but I agree with your arguments and also think it is ok to keep it simple for now. So I'm applying this to my l2 tree, and then it is up to dwmw2 to take it or not. But I also have some requests about commentaries, so if you can re-send another version of this patch, it would be nice. But I take this one for now, it is good enough. > +/* > + * Copies (and truncates, if necessary) data from the larger struct, > + * nand_ecclayout, to the smaller, deprecated layout struct, > + * nand_ecclayout_user. This is necessary only to suppport the deprecated > + * API ioctl ECCGETLAYOUT while allowing all new functionality to use > + * nand_ecclayout flexibly (i.e. the struct may change size in new > + * releases without requiring major rewrites). > + */ I think a similar comment should exist in linux/mtd/mtd.h. Indeed, that file is our API with user-space, and our users will probably look at it, and it is nice to document the situation with 'struct nand_ecclayout_user' there. > +#define MTD_MAX_OOBFREE_ENTRIES_LARGE 32 > +#define MTD_MAX_ECCPOS_ENTRIES_LARGE 448 > +#define MTD_MAX_ECCPOS_ENTRIES_OLD 64 /* Previous maximum */ > +/* > + * Correct ECC layout control structure. This replaces old nand_ecclayout > + * (mtd-abi.h) that is exported via ECCGETLAYOUT ioctl. It should be expandable > + * in the future simply by the above macros. > + */ I find this comment confusing. First, "Correct ECC" -> "Internal ECC", because one could think "Correct ECC structure" means something like "structure which describes ECC corrections" or something like that. Also, I'd avoid mentioning things like "old nand_ecclayout", because with time this will be confusing. Could you please imagine that you are an MTD newbie reading the code in 2012 - you have no idea what was happening in the past in the ancient 2010. Thanks!
On Mon, 2010-08-30 at 13:20 +0300, Artem Bityutskiy wrote: > On Tue, 2010-08-24 at 18:12 -0700, Brian Norris wrote: > > My e-mail address has changed, since I am no longer working at Broadcom. > > I will still be able to track messages to my old account if the MTD mailing > > list is CC'd. > > Oh, does it mean you will stop loving MTD and we won't see steady flow > of improvements for you? :-( BTW, I think you have been doing great job > - MTD subsystem needs love badly! > > > Note that on the same subject (different thread) David suggested our new > > struct be allocated dynamically: > > Yes, but I agree with your arguments and also think it is ok to keep it > simple for now. So I'm applying this to my l2 tree, and then it is up to > dwmw2 to take it or not. > > But I also have some requests about commentaries, so if you can re-send > another version of this patch, it would be nice. But I take this one for > now, it is good enough. > > > +/* > > + * Copies (and truncates, if necessary) data from the larger struct, > > + * nand_ecclayout, to the smaller, deprecated layout struct, > > + * nand_ecclayout_user. This is necessary only to suppport the deprecated > > + * API ioctl ECCGETLAYOUT while allowing all new functionality to use > > + * nand_ecclayout flexibly (i.e. the struct may change size in new > > + * releases without requiring major rewrites). > > + */ > > I think a similar comment should exist in linux/mtd/mtd.h. Indeed, that > file is our API with user-space, and our users will probably look at it, > and it is nice to document the situation with 'struct > nand_ecclayout_user' there. > > > +#define MTD_MAX_OOBFREE_ENTRIES_LARGE 32 > > +#define MTD_MAX_ECCPOS_ENTRIES_LARGE 448 > > +#define MTD_MAX_ECCPOS_ENTRIES_OLD 64 /* Previous maximum */ > > +/* > > + * Correct ECC layout control structure. This replaces old nand_ecclayout > > + * (mtd-abi.h) that is exported via ECCGETLAYOUT ioctl. It should be expandable > > + * in the future simply by the above macros. > > + */ > > I find this comment confusing. First, "Correct ECC" -> "Internal ECC", > because one could think "Correct ECC structure" means something like > "structure which describes ECC corrections" or something like that. > > Also, I'd avoid mentioning things like "old nand_ecclayout", because > with time this will be confusing. Could you please imagine that you are > an MTD newbie reading the code in 2012 - you have no idea what was > happening in the past in the ancient 2010. Brian, would you address the small things I noticed in a follow-up patch?
On 9/18/2010 10:24 AM, Artem Bityutskiy wrote: > Brian, > > would you address the small things I noticed in a follow-up patch? Artem, Yes, I will. I'm sorry for the delay; I've been a little busy and putting some things off. Would you like a patch for the comments against your l2 tree or try and squash it with my previous commit and base it off of mtd-2.6 or some other option? Brian
On Sat, 2010-09-18 at 13:03 -0700, Brian Norris wrote: > On 9/18/2010 10:24 AM, Artem Bityutskiy wrote: > > Brian, > > > > would you address the small things I noticed in a follow-up patch? > > Artem, > > Yes, I will. I'm sorry for the delay; I've been a little busy and > putting some things off. Would you like a patch for the comments against > your l2 tree or try and squash it with my previous commit and base it > off of mtd-2.6 or some other option? No, against l2 tree is ok, I'll put it on top, thanks!
Hello, Now that I've finally gotten around to working on this, I have questions and comments. On 9/18/2010 10:24 AM, Artem Bityutskiy wrote: > On Mon, 2010-08-30 at 13:20 +0300, Artem Bityutskiy wrote: >> On Tue, 2010-08-24 at 18:12 -0700, Brian Norris wrote: >>> My e-mail address has changed, since I am no longer working at Broadcom. >>> I will still be able to track messages to my old account if the MTD mailing >>> list is CC'd. >> >> Oh, does it mean you will stop loving MTD and we won't see steady flow >> of improvements for you? :-( BTW, I think you have been doing great job >> - MTD subsystem needs love badly! No, this doesn't mean I will stop loving MTD, although as may be readily apparent by my delay, I may work a bit slower :) In fact, I will probably be picking up some more work shortly. >>> +/* >>> + * Copies (and truncates, if necessary) data from the larger struct, >>> + * nand_ecclayout, to the smaller, deprecated layout struct, >>> + * nand_ecclayout_user. This is necessary only to suppport the deprecated >>> + * API ioctl ECCGETLAYOUT while allowing all new functionality to use >>> + * nand_ecclayout flexibly (i.e. the struct may change size in new >>> + * releases without requiring major rewrites). >>> + */ >> >> I think a similar comment should exist in linux/mtd/mtd.h. Indeed, that >> file is our API with user-space, and our users will probably look at it, >> and it is nice to document the situation with 'struct >> nand_ecclayout_user' there. From the context, I assume you meant include/mtd/mtd-abi.h, not linux/mtd/mtd.h; am I correct? I will send the patch out shortly under the assumption that I am correct. Brian
On Sun, 2010-09-19 at 23:28 -0700, Brian Norris wrote: > >> Oh, does it mean you will stop loving MTD and we won't see steady flow > >> of improvements for you? :-( BTW, I think you have been doing great job > >> - MTD subsystem needs love badly! > > No, this doesn't mean I will stop loving MTD, although as may be readily > apparent by my delay, I may work a bit slower :) In fact, I will > probably be picking up some more work shortly. Nice :-) > From the context, I assume you meant include/mtd/mtd-abi.h, not > linux/mtd/mtd.h; am I correct? Probably yes :-)
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c index a825002..24d35ba 100644 --- a/drivers/mtd/mtdchar.c +++ b/drivers/mtd/mtdchar.c @@ -477,6 +477,39 @@ static int mtd_do_readoob(struct mtd_info *mtd, uint64_t start, return ret; } +/* + * Copies (and truncates, if necessary) data from the larger struct, + * nand_ecclayout, to the smaller, deprecated layout struct, + * nand_ecclayout_user. This is necessary only to suppport the deprecated + * API ioctl ECCGETLAYOUT while allowing all new functionality to use + * nand_ecclayout flexibly (i.e. the struct may change size in new + * releases without requiring major rewrites). + */ +static int shrink_ecclayout(const struct nand_ecclayout *from, + struct nand_ecclayout_user *to) +{ + int i; + + if (!from || !to) + return -EINVAL; + + memset(to, 0, sizeof(*to)); + + to->eccbytes = min((int)from->eccbytes, MTD_MAX_ECCPOS_ENTRIES_OLD); + for (i = 0; i < to->eccbytes; i++) + to->eccpos[i] = from->eccpos[i]; + + for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES; i++) { + if (from->oobfree[i].length == 0 && + from->oobfree[i].offset == 0) + break; + to->oobavail += from->oobfree[i].length; + to->oobfree[i] = from->oobfree[i]; + } + + return 0; +} + static int mtd_ioctl(struct file *file, u_int cmd, u_long arg) { struct mtd_file_info *mfi = file->private_data; @@ -812,14 +845,23 @@ static int mtd_ioctl(struct file *file, u_int cmd, u_long arg) } #endif + /* This ioctl is being deprecated - it truncates the ecc layout */ case ECCGETLAYOUT: { + struct nand_ecclayout_user *usrlay; + if (!mtd->ecclayout) return -EOPNOTSUPP; - if (copy_to_user(argp, mtd->ecclayout, - sizeof(struct nand_ecclayout))) - return -EFAULT; + usrlay = kmalloc(sizeof(*usrlay), GFP_KERNEL); + if (!usrlay) + return -ENOMEM; + + shrink_ecclayout(mtd->ecclayout, usrlay); + + if (copy_to_user(argp, usrlay, sizeof(*usrlay))) + ret = -EFAULT; + kfree(usrlay); break; } diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c index 2ac7367..70698e8 100644 --- a/drivers/mtd/nand/davinci_nand.c +++ b/drivers/mtd/nand/davinci_nand.c @@ -749,6 +749,9 @@ static int __init nand_davinci_probe(struct platform_device *pdev) * breaks userspace ioctl interface with mtd-utils. Once we * resolve this issue, NAND_ECC_HW_OOB_FIRST mode can be used * for the 4KiB page chips. + * + * TODO: Note that nand_ecclayout has now been expanded and can + * hold plenty of OOB entries. */ dev_warn(&pdev->dev, "no 4-bit ECC support yet " "for 4KiB-page NAND\n"); diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index 8485e42..03a1e95 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -110,6 +110,21 @@ struct mtd_oob_ops { uint8_t *oobbuf; }; +#define MTD_MAX_OOBFREE_ENTRIES_LARGE 32 +#define MTD_MAX_ECCPOS_ENTRIES_LARGE 448 +#define MTD_MAX_ECCPOS_ENTRIES_OLD 64 /* Previous maximum */ +/* + * Correct ECC layout control structure. This replaces old nand_ecclayout + * (mtd-abi.h) that is exported via ECCGETLAYOUT ioctl. It should be expandable + * in the future simply by the above macros. + */ +struct nand_ecclayout { + __u32 eccbytes; + __u32 eccpos[MTD_MAX_ECCPOS_ENTRIES_LARGE]; + __u32 oobavail; + struct nand_oobfree oobfree[MTD_MAX_OOBFREE_ENTRIES_LARGE]; +}; + struct mtd_info { u_char type; uint32_t flags; diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h index 274b619..930c8ac 100644 --- a/include/linux/mtd/partitions.h +++ b/include/linux/mtd/partitions.h @@ -39,7 +39,7 @@ struct mtd_partition { uint64_t size; /* partition size */ uint64_t offset; /* offset within the master MTD space */ uint32_t mask_flags; /* master MTD flags to mask out for this partition */ - struct nand_ecclayout *ecclayout; /* out of band layout for this partition (NAND only)*/ + struct nand_ecclayout *ecclayout; /* out of band layout for this partition (NAND only) */ }; #define MTDPART_OFS_NXTBLK (-2) diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h index 4debb45..5bce083 100644 --- a/include/mtd/mtd-abi.h +++ b/include/mtd/mtd-abi.h @@ -119,7 +119,7 @@ struct otp_info { #define OTPGETREGIONCOUNT _IOW('M', 14, int) #define OTPGETREGIONINFO _IOW('M', 15, struct otp_info) #define OTPLOCK _IOR('M', 16, struct otp_info) -#define ECCGETLAYOUT _IOR('M', 17, struct nand_ecclayout) +#define ECCGETLAYOUT _IOR('M', 17, struct nand_ecclayout_user) #define ECCGETSTATS _IOR('M', 18, struct mtd_ecc_stats) #define MTDFILEMODE _IO('M', 19) #define MEMERASE64 _IOW('M', 20, struct erase_info_user64) @@ -148,7 +148,7 @@ struct nand_oobfree { * ECC layout control structure. Exported to userspace for * diagnosis and to allow creation of raw images */ -struct nand_ecclayout { +struct nand_ecclayout_user { __u32 eccbytes; __u32 eccpos[64]; __u32 oobavail; diff --git a/include/mtd/mtd-user.h b/include/mtd/mtd-user.h index aa3c2f8..83327c8 100644 --- a/include/mtd/mtd-user.h +++ b/include/mtd/mtd-user.h @@ -29,6 +29,6 @@ typedef struct mtd_info_user mtd_info_t; typedef struct erase_info_user erase_info_t; typedef struct region_info_user region_info_t; typedef struct nand_oobinfo nand_oobinfo_t; -typedef struct nand_ecclayout nand_ecclayout_t; +typedef struct nand_ecclayout_user nand_ecclayout_t; #endif /* __MTD_USER_H__ */