linux-kernel - Re: [PATCH] mtd: nand: spi: Add initial support for Toshiba TC58CVG2S0H

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]

Date:   Wed, 28 Nov 2018 13:01:39 +0000
From:   Schrempf Frieder <frieder.schrempf@...tron.de>
To:     Boris Brezillon <boris.brezillon@...tlin.com>
CC:     Clément Péron <peron.clem@...il.com>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        "richard@....at" <richard@....at>,
        "dwmw2@...radead.org" <dwmw2@...radead.org>,
        "computersforpeace@...il.com" <computersforpeace@...il.com>,
        "marek.vasut@...il.com" <marek.vasut@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>
Subject: Re: [PATCH] mtd: nand: spi: Add initial support for Toshiba
 TC58CVG2S0H

Hi Boris,

On 28.11.18 13:53, Boris Brezillon wrote:
> On Tue, 27 Nov 2018 16:41:56 +0000
> Schrempf Frieder <frieder.schrempf@...tron.de> wrote:
> 
>>>>> +static int tc58cvg2s0h_ooblayout_ecc(struct mtd_info *mtd, int section,
>>>>> +                                  struct mtd_oob_region *region)
>>>>> +{
>>>>> +     if (section > 7)
>>>>> +             return -ERANGE;
>>>>> +
>>>>> +     region->offset = 128 + 16 * section;
>>>>> +     region->length = 16;
>>>
>>> Here you expose the ECC bits has 8 sections of 16 bytes.
>>> But regarding the datasheet this should not be accessed page 32.
>>> "The ECC parity code generated by internal ECC is stored in column
>>> addresses 4224-4351 and the users cannot access to these specific
>>> addresses when internal ECC is enabled."
> 
> 'when internal ECC is enabled' means that those bytes can be accessed
> when it's disabled. We should keep exposing the ECC byte sections. Note
> that even if ECC sections are not exposed, the core will read those
> bytes. They're probably filled with garbage in this case, but we don't
> care since we won't use them.
> 
>>
>> This is just to let the other layers know, where the bytes used for ECC
>> are. As long as the driver uses the on-chip ECC it won't write to this
>> area. So this is correct unless I misunderstood this concept. All the
>> other supported SPI NAND chips use the same approach.
> 
> Yes, and that's the right thing to do. We want to know where the ECC
> bytes are, especially when doing raw accesses.

Thank you for clarifying this. I will send a v2 of my patch and revert 
this change.

Thanks,
Frieder

> 
>>
>>>    
>>>>> +
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static int tc58cvg2s0h_ooblayout_free(struct mtd_info *mtd, int section,
>>>>> +                                   struct mtd_oob_region *region)
>>>>> +{
>>>>> +     if (section > 7)
>>>>> +             return -ERANGE;
>>>>> +
>>>>> +     region->offset = 2 + 16 * section;
>>>>> +     region->length = 14;
>>>
>>> This reserved 2 bytes for BBM for each section.
>>> But maybe we could declare this as 1 section of 128bytes:
>>>
>>> if (section)
>>>            return -ERANGE;
>>>
>>> region->offset = 2;
>>> region->length = 126;
> 
> I agree with this suggestion: if the free bytes are contiguous, it's
> better to expose them as a single section.
>