linux-kernel - Re: [PATCH v2] mtd: fsl_elbc_nand Add ECC mode selection in DT

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 for Android: free password hash cracker in your pocket

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

Message-ID: <1432241934.27761.230.camel@freescale.com>
Date:	Thu, 21 May 2015 15:58:54 -0500
From:	Scott Wood <scottwood@...escale.com>
To:	Tomas Hlavacek <tmshlvck@...il.com>
CC:	<dwmw2@...radead.org>, <computersforpeace@...il.com>,
	<linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] mtd: fsl_elbc_nand Add ECC mode selection in DT

On Thu, 2015-05-21 at 13:46 +0200, Tomas Hlavacek wrote:
> Add device tree pointer to chip structure in order to allow turn off the
> HW ECC and force own ECC mode and ECC parameters. Newly supported entries
> are as per documentation: nand-ecc-mode, nand-ecc-step-size
> and nand-ecc-strength.

If you're using these properties with eLBC, the eLBC binding needs to be
updated to say that they're allowed for use with software ECC.

> @@ -677,8 +689,8 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd)
>  		priv->page_size = 1;
>  		setbits32(&lbc->bank[priv->bank].or, OR_FCM_PGS);
>  		/* adjust ecc setup if needed */
> -		if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
> -		    BR_DECC_CHK_GEN) {
> +		if (((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
> +		    BR_DECC_CHK_GEN) && (chip->ecc.mode == NAND_ECC_HW)) {

There's no need to check both.  If you're selecting software ECC in the
device tree then you must also remove ECC checking from BRn.

If you want to make Linux write to BRn to ensure that it's consistent
with ecc.mode after the DT init, fine, but don't just silently proceed
without fixing it if there's an inconsistency.

> @@ -786,6 +801,7 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
>  		chip->ecc.size = 512;
>  		chip->ecc.bytes = 3;
>  		chip->ecc.strength = 1;
> +		chip->ecc.write_subpage = fsl_elbc_write_subpage;
>  	} else {
>  		/* otherwise fall back to default software ECC */
>  		chip->ecc.mode = NAND_ECC_SOFT;

...but here you are relying on BRn to be set correctly before
nand_dt_init(), so I'm even more confused by what the previous change is
for.

-Scott

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