linux-kernel - Re: [PATCH v5 39/50] mtd: nand: omap2: switch to mtd_ooblayout

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]

Message-ID: <20160419144152.30a4027d@bbrezillon>
Date:	Tue, 19 Apr 2016 14:41:52 +0200
From:	Boris Brezillon <boris.brezillon@...e-electrons.com>
To:	Roger Quadros <rogerq@...com>
Cc:	David Woodhouse <dwmw2@...radead.org>,
	Brian Norris <computersforpeace@...il.com>,
	<linux-mtd@...ts.infradead.org>,
	Richard Weinberger <richard@....at>,
	<linux-mips@...ux-mips.org>,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Harvey Hunt <harvey.hunt@...tec.com>,
	Nicolas Ferre <nicolas.ferre@...el.com>,
	Stefan Agner <stefan@...er.ch>, <linux-sunxi@...glegroups.com>,
	Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
	punnaiah choudary kalluri <punnaia@...inx.com>,
	Robert Jarzmik <robert.jarzmik@...e.fr>,
	<devel@...verdev.osuosl.org>,
	Archit Taneja <architt@...eaurora.org>,
	<linux-samsung-soc@...r.kernel.org>,
	Kamal Dasu <kdasu.kdev@...il.com>,
	Josh Wu <rainyfeeling@...look.com>,
	Chen-Yu Tsai <wens@...e.org>, Kukjin Kim <kgene@...nel.org>,
	<bcm-kernel-feedback-list@...adcom.com>,
	Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>,
	Huang Shijie <shijie.huang@....com>,
	Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
	Haojian Zhuang <haojian.zhuang@...il.com>,
	Han Xu <b45815@...escale.com>,
	<linux-arm-kernel@...ts.infradead.org>,
	Priit Laes <plaes@...es.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	<linux-kernel@...r.kernel.org>, Ralf Baechle <ralf@...ux-mips.org>,
	Wenyou Yang <wenyou.yang@...el.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	<linux-api@...r.kernel.org>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	Daniel Mack <daniel@...que.org>
Subject: Re: [PATCH v5 39/50] mtd: nand: omap2: switch to mtd_ooblayout_ops

On Tue, 19 Apr 2016 15:30:39 +0300
Roger Quadros <rogerq@...com> wrote:

> On 19/04/16 14:22, Boris Brezillon wrote:
> > Hi Roger,
> > 
> > On Tue, 19 Apr 2016 13:28:50 +0300
> > Roger Quadros <rogerq@...com> wrote:
> > 
> >>> @@ -1921,6 +1927,9 @@ static int omap_nand_probe(struct platform_device *pdev)
> >>>  		nand_chip->ecc.correct          = omap_correct_data;
> >>>  		mtd_set_ooblayout(mtd, &omap_ooblayout_ops);
> >>>  		oobbytes_per_step		= nand_chip->ecc.bytes;
> >>> +
> >>> +		if (nand_chip->options & NAND_BUSWIDTH_16)
> >>> +			min_oobbytes		= 1;
> >>
> >> Shouldn't this have been
> >> 		if (!(nand_chip->options & NAND_BUSWIDTH_16)
> >> 			min_oobbytes		= 1;
> >> ?
> > 
> > Yep.
> > 
> >>
> >>>  		break;
> >>>  
> >>>  	case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW:
> >>> @@ -2038,10 +2047,8 @@ static int omap_nand_probe(struct platform_device *pdev)
> >>>  	}
> >>>  
> >>>  	/* check if NAND device's OOB is enough to store ECC signatures */
> >>> -	min_oobbytes = (oobbytes_per_step *
> >>> -			(mtd->writesize / nand_chip->ecc.size)) +
> >>> -		       (nand_chip->options & NAND_BUSWIDTH_16 ?
> >>> -			BADBLOCK_MARKER_LENGTH : 1);
> >>> +	min_oobbytes += (oobbytes_per_step *
> >>> +			 (mtd->writesize / nand_chip->ecc.size));
> >>>  	if (mtd->oobsize < min_oobbytes) {
> >>>  		dev_err(&info->pdev->dev,
> >>>  			"not enough OOB bytes required = %d, available=%d\n",
> >>>
> >>
> >> After the above changes BCH with HW ECC worked fine but BCH with SW ECC still failed.
> >> I had to fix it up with the below patch. This is mainly because chip->ecc.steps wasn't
> >> yet initialized before calling nand_bch_init().
> >>
> >> After the below patch it worked fine with bch4 (hw & sw), bch8 (hw & sw) and ham1.
> >> I couldn't yet verify bch16 though.
> > 
> 
> I just verified that bch16 works as well.
> 
> > Thanks for the fix, but I'd prefer fixing the bug for all soft BCH
> > users.
> > 
> > Could you try this patch?
> 
> I tried your patch and it worked fine.

Thanks, I'll provide a reworked nand/next branch soon.
BTW, is there anything to fix in my merge commit (the commit merging
your GPMC/OMAP changes in nand/next)?

> You will still need the below change to omap2.c
> 
> --
> cheers,
> -roger
> 
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 0abfba6..33c8fde 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -1715,7 +1715,7 @@ static int omap_sw_ooblayout_free(struct mtd_info *mtd, int section,
>  	struct nand_chip *chip = mtd_to_nand(mtd);
>  	int off = BADBLOCK_MARKER_LENGTH;
>  
> -	if (section)
> +	if (section >= chip->ecc.steps)
>  		return -ERANGE;

Sorry but I don't get why we need that one. Don't we have a single
oobfree section starting at the end of the ECC sections?


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com