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] [day] [month] [year] [list]

Message-ID: <66b47505-c23d-2e7f-107b-68ab578b3b3c@sberdevices.ru>
Date:   Tue, 4 Jul 2023 20:07:45 +0300
From:   Arseniy Krasnov <avkrasnov@...rdevices.ru>
To:     Miquel Raynal <miquel.raynal@...tlin.com>
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



On 04.07.2023 18:32, Miquel Raynal wrote:
> Hi Arseniy,
> 
> avkrasnov@...rdevices.ru wrote on Tue, 4 Jul 2023 18:07:04 +0300:
> 
>> On 04.07.2023 16:41, Miquel Raynal wrote:
>>> Hi Arseniy,
>>>   
>>>>>>>> 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.    
>>>>
>>>> Sorry, didn't get it... if ECC step size is set in DTS, then here, in chip attach
>>>> callback it will be already known (DT part was processed in 'rawnand_dt_init()').
>>>> If ECC step size is unknown (e.g. 0 here), 'nand_ecc_choose_conf()' will set it
>>>> according provided ecc caps. What do You mean for "You should set ..." ?  
>>>
>>> The current approach is wrong, it decides the number of ECC chunks
>>> (called nsectors in the driver) and then asks the core to decide the
>>> number of ECC chunks to use.  
>>
>> Yes! I was also confused about that.
>>
>>>
>>> Just provide mtd->oobsize - 2 as last parameter and then rely on the
>>> core's logic to find the right ECC step-size/strength?
>>>
>>> There is no point in requesting a particular step size without a
>>> specific strength, or? So I believe you should provide both in the DTS
>>> if you want particular parameters to be applied, otherwise you can let
>>> the core decide what is best.  
>>
>> So I think this could be a separated patch as it doesn't rely on 512 step size ECC
>> support for Meson and may be it should be "Fix" tagged.
> 
> Yup! Thanks for cleaning so thoroughly this driver :)

Thanks again for review and details! :)

Thanks, Arseniy

> 
> Cheers,
> Miquèl