linux-kernel - Re: [RFC PATCH v1 2/2] mtd: rawnand: meson: support for 512B ECC step size

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: <20230704115628.55320428@xps-13>
Date:   Tue, 4 Jul 2023 11:56:28 +0200
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Arseniy Krasnov <avkrasnov@...rdevices.ru>
Cc:     Liang Yang <liang.yang@...ogic.com>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Neil Armstrong <neil.armstrong@...aro.org>,
        Kevin Hilman <khilman@...libre.com>,
        Jerome Brunet <jbrunet@...libre.com>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        <oxffffaa@...il.com>, <kernel@...rdevices.ru>,
        <linux-mtd@...ts.infradead.org>, <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-amlogic@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v1 2/2] mtd: rawnand: meson: support for 512B ECC
 step size

Hi Arseniy,

avkrasnov@...rdevices.ru wrote on Tue, 4 Jul 2023 12:46:51 +0300:

> On 04.07.2023 12:41, Miquel Raynal wrote:
> > Hi Arseniy,
> > 
> > avkrasnov@...rdevices.ru wrote on Tue, 4 Jul 2023 12:23:03 +0300:
> >   
> >> On 04.07.2023 11:36, Miquel Raynal wrote:  
> >>> Hi Arseniy,
> >>>
> >>> AVKrasnov@...rdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300:
> >>>     
> >>>> Meson NAND supports both 512B and 1024B ECC step size.
> >>>>
> >>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@...rdevices.ru>
> >>>> ---
> >>>>  drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++--------
> >>>>  1 file changed, 35 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> >>>> index 345212e8c691..6cc4f63b86c8 100644
> >>>> --- a/drivers/mtd/nand/raw/meson_nand.c
> >>>> +++ b/drivers/mtd/nand/raw/meson_nand.c
> >>>> @@ -135,6 +135,7 @@ struct meson_nfc_nand_chip {
> >>>>  struct meson_nand_ecc {
> >>>>  	u32 bch;
> >>>>  	u32 strength;
> >>>> +	u32 size;
> >>>>  };
> >>>>  
> >>>>  struct meson_nfc_data {
> >>>> @@ -190,7 +191,8 @@ struct meson_nfc {
> >>>>  };
> >>>>  
> >>>>  enum {
> >>>> -	NFC_ECC_BCH8_1K		= 2,
> >>>> +	NFC_ECC_BCH8_512	= 1,
> >>>> +	NFC_ECC_BCH8_1K,
> >>>>  	NFC_ECC_BCH24_1K,
> >>>>  	NFC_ECC_BCH30_1K,
> >>>>  	NFC_ECC_BCH40_1K,
> >>>> @@ -198,15 +200,16 @@ enum {
> >>>>  	NFC_ECC_BCH60_1K,
> >>>>  };
> >>>>  
> >>>> -#define MESON_ECC_DATA(b, s)	{ .bch = (b),	.strength = (s)}
> >>>> +#define MESON_ECC_DATA(b, s, sz)	{ .bch = (b), .strength = (s), .size = (sz) }
> >>>>  
> >>>>  static struct meson_nand_ecc meson_ecc[] = {
> >>>> -	MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8),
> >>>> -	MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24),
> >>>> -	MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30),
> >>>> -	MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40),
> >>>> -	MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50),
> >>>> -	MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60),
> >>>> +	MESON_ECC_DATA(NFC_ECC_BCH8_512, 8,  512),
> >>>> +	MESON_ECC_DATA(NFC_ECC_BCH8_1K,  8,  1024),
> >>>> +	MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24, 1024),
> >>>> +	MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30, 1024),
> >>>> +	MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40, 1024),
> >>>> +	MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50, 1024),
> >>>> +	MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60, 1024),
> >>>>  };
> >>>>  
> >>>>  static int meson_nand_calc_ecc_bytes(int step_size, int strength)
> >>>> @@ -224,8 +227,27 @@ static int meson_nand_calc_ecc_bytes(int step_size, int strength)
> >>>>  
> >>>>  NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps,
> >>>>  		     meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60);
> >>>> -NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps,
> >>>> -		     meson_nand_calc_ecc_bytes, 1024, 8);
> >>>> +
> >>>> +static const int axg_stepinfo_strengths[] = { 8 };
> >>>> +static const struct nand_ecc_step_info axg_stepinfo_1024 = {
> >>>> +	.stepsize = 1024,
> >>>> +	.strengths = axg_stepinfo_strengths,
> >>>> +	.nstrengths = ARRAY_SIZE(axg_stepinfo_strengths)
> >>>> +};
> >>>> +
> >>>> +static const struct nand_ecc_step_info axg_stepinfo_512 = {
> >>>> +	.stepsize = 512,
> >>>> +	.strengths = axg_stepinfo_strengths,
> >>>> +	.nstrengths = ARRAY_SIZE(axg_stepinfo_strengths)
> >>>> +};
> >>>> +
> >>>> +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 };
> >>>> +
> >>>> +static const struct nand_ecc_caps meson_axg_ecc_caps = {
> >>>> +	.stepinfos = axg_stepinfo,
> >>>> +	.nstepinfos = ARRAY_SIZE(axg_stepinfo),
> >>>> +	.calc_ecc_bytes = meson_nand_calc_ecc_bytes,
> >>>> +};
> >>>>  
> >>>>  static struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand)
> >>>>  {
> >>>> @@ -1259,7 +1281,8 @@ static int meson_nand_bch_mode(struct nand_chip *nand)
> >>>>  		return -EINVAL;
> >>>>  
> >>>>  	for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) {
> >>>> -		if (meson_ecc[i].strength == nand->ecc.strength) {
> >>>> +		if (meson_ecc[i].strength == nand->ecc.strength &&
> >>>> +		    meson_ecc[i].size == nand->ecc.size) {
> >>>>  			meson_chip->bch_mode = meson_ecc[i].bch;
> >>>>  			return 0;
> >>>>  		}
> >>>> @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
> >>>>  	struct meson_nfc *nfc = nand_get_controller_data(nand);
> >>>>  	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
> >>>>  	struct mtd_info *mtd = nand_to_mtd(nand);
> >>>> -	int nsectors = mtd->writesize / 1024;
> >>>> +	int nsectors = mtd->writesize / 512;    
> >>>
> >>> This cannot be unconditional, right?    
> >>
> >> Hello Miquel!
> >>
> >> Yes, this code looks strange. 'nsectors' is used to calculate space in OOB
> >> that could be used by ECC engine (this value will be passed as 'oobavail'
> >> to 'nand_ecc_choose_conf()'). Idea of 512 is to consider "worst" case
> >> for ECC, e.g. minimal number of bytes for ECC engine (and at the same time
> >> maximum number of free bytes). For Meson, if ECC step size is 512, then we
> >> have 4 x 2 free bytes in OOB (if step size if 1024 then we have 2 x 2 free
> >> bytes in OOB).
> >>
> >> I think this code could be reworked in the following way:
> >>
> >> if ECC step size is already known here (from DTS), calculate 'nsectors' using
> >> given value (div by 512 for example). Otherwise calculate 'nsectors' in the
> >> current manner:  
> > 
> > It will always be known when these function are run. There is no
> > guessing here.  
> 
> Hm I checked, that but if step size is not set in DTS, here it will be 0, 
> then it will be selected in 'nand_ecc_choose_conf()' according provided 'ecc_caps'
> and 'oobavail'...
> 
> Anyway, I'll do the following thing:
> 
> int nsectors;
> 
> if (nand->ecc.size)
>     nsectors = mtd->writesize / nand->ecc.size; <--- this is for 512 ECC

You should set nand->ecc.size in ->attach_chip() instead.

> else
>     nsectors = mtd->writesize / 1024; <--- this is for default 1024 ECC
> 
> Thanks, Arseniy
> 
> >   
> >>
> >> int nsectors = mtd->writesize / 1024;
> >>
> >> Moreover 1024 is default ECC step size for this driver, so default behaviour
> >> will be preserved.  
> > 
> > Yes, otherwise you would break existing users.
> >   
> >>
> >> Thanks, Arseniy
> >>  
> >>>     
> >>>>  	int raw_writesize;
> >>>>  	int ret;
> >>>>      
> >>>
> >>>
> >>> Thanks,
> >>> Miquèl    
> > 
> > 
> > Thanks,
> > Miquèl  


Thanks,
Miquèl