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:	Mon, 30 Aug 2010 13:20:47 +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 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!

-- 
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/