linux-kernel - Re: [PATCH v4] mtd: nand: Expand nand_ecc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 18 Sep 2010 20:24:50 +0300
From:	Artem Bityutskiy <dedekind1@...il.com>
To:	Brian Norris <computersforpeace@...il.com>
Cc:	Shinya Kuribayashi <shinya.kuribayashi.px@...esas.com>,
	David Woodhouse <dwmw2@...radead.org>,
	"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
	Sneha Narnakaje <nsnehaprabha@...com>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Kevin Cernekee <cernekee@...il.com>
Subject: Re: [PATCH v4] mtd: nand: Expand nand_ecc_layout, deprecate ioctl
 ECCGETLAYOUT

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?
-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/