linux-kernel - Re: [PATCH] mtd: gpmi-nand: do not fail setting ONFI timing mode if available

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:   Fri, 14 Jul 2017 19:31:43 +0200
From:   Boris Brezillon <boris.brezillon@...e-electrons.com>
To:     Han Xu <han.xu@....com>
Cc:     Miquel Raynal <miquel.raynal@...e-electrons.com>,
        "richard@....at" <richard@....at>,
        "linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
        "dwmw2@...radead.org" <dwmw2@...radead.org>,
        "computersforpeace@...il.com" <computersforpeace@...il.com>,
        "marek.vasut@...il.com" <marek.vasut@...il.com>,
        "cyrille.pitchen@...ev4u.fr" <cyrille.pitchen@...ev4u.fr>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mtd: gpmi-nand: do not fail setting ONFI timing mode if
 available

Hi Han,

Le Fri, 14 Jul 2017 14:53:39 +0000,
Han Xu <han.xu@....com> a écrit :

> On 07/13/2017 03:15 PM, Boris Brezillon wrote:
> > Hi Miquel,
> >
> > Le Thu, 13 Jul 2017 21:20:30 +0200,
> > Miquel Raynal <miquel.raynal@...e-electrons.com> a écrit :
> >  
> >> GPMI NFC driver fails to apply timing mode if the ->onfi_get_features()
> >> does not return the mode that was previously applied.
> >>
> >> We can assume that a nand chip supports a timing as long as it is
> >> read from the ONFI parameter page. Reading back a different mode than
> >> the one previously applied does not mean the mode is unsupported but
> >> that the nand chip does not implement the ONFI feature because it
> >> probably does not need to.
> >>
> >> The output of ->onfi_get_feature() is irrelevant so delete it.  
> > Having the NAND part that is not supporting the get/set(timing_mode)
> > feature explicitly mentioned in the commit message would help reviewers
> > understand why this patch is needed.
> >
> > Also mention that, even though the SET/GET_FEATURES(timing_mode) is
> > marked as required in the ONFI spec, this Macronix chip does not
> > support it which could be considered as a bug.
> >
> > Regards,
> >
> > Boris  
> 
> Yes, this is a Macronix chip bug and I have reproduced on my side, 
> ignoring the GET_FEATURE checking is a workaround and the chip will 
> still works in EDO mode 5, but I don't accept to remove the reasonable 
> checking code for a chip bug.

I understand why you're reluctant to remove this check just to make
one particular chip work correctly, but, on the other hand, if we were
only supporting non-broken NAND chip in mainline, plenty of boards
wouldn't be supported. Flash vendors tend to take liberties with
standards, that's a fact, and once the chip is out there's nothing we
can do about it, except add a workaround to support it.

So let's try to find a solution that makes everyone happy: now that we
have nand_manufacturer_ops, we can easily let manufacturer code flag
specific chip features as broken and let the core or drivers test for
it before using the feature.
This way, the gpmi-nand driver could check this flag before trying to
call ->onfi_set/get_features(TIMING).
Would that work for you?

BTW, that'd be great to have this driver converted to the
->setup_data_interface() approach at some point.

Regards,

Boris

> 
> >> Signed-off-by: Miquel Raynal <miquel.raynal@...e-electrons.com>
> >> ---
> >>   drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 7 -------
> >>   1 file changed, 7 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> >> index 141bd70a49c2..4d137145439c 100644
> >> --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> >> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> >> @@ -939,13 +939,6 @@ static int enable_edo_mode(struct gpmi_nand_data *this, int mode)
> >>   	if (ret)
> >>   		goto err_out;
> >>   
> >> -	/* [2] send GET FEATURE command to double-check the timing mode */
> >> -	memset(feature, 0, ONFI_SUBFEATURE_PARAM_LEN);
> >> -	ret = nand->onfi_get_features(mtd, nand,
> >> -				ONFI_FEATURE_ADDR_TIMING_MODE, feature);
> >> -	if (ret || feature[0] != mode)
> >> -		goto err_out;
> >> -
> >>   	nand->select_chip(mtd, -1);
> >>   
> >>   	/* [3] set the main IO clock, 100MHz for mode 5, 80MHz for mode 4. */