linux-kernel - Re: [PATCH 1/1] mtd: spi-nor: remove WARN_ONCE() message in spi_nor

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: <0b871c04-c105-ec94-441c-481e9773f19e@wedev4u.fr>
Date:   Wed, 7 Dec 2016 00:38:42 +0100
From:   Cyrille Pitchen <cyrille.pitchen@...ev4u.fr>
To:     Marek Vasut <marek.vasut@...il.com>,
        Cyrille Pitchen <cyrille.pitchen@...el.com>
Cc:     boris.brezillon@...e-electrons.com, computersforpeace@...il.com,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
        richard@....at
Subject: Re: [PATCH 1/1] mtd: spi-nor: remove WARN_ONCE() message in
 spi_nor_write()

Le 06/12/2016 à 20:00, Marek Vasut a écrit :
> On 12/06/2016 06:14 PM, Cyrille Pitchen wrote:
>> This patch removes the WARN_ONCE() test in spi_nor_write().
>> This macro triggers the display of a warning message almost every time we
>> use a UBI file-system because a write operation is performed at offset 64,
>> which is in the middle of the SPI NOR memory page. This is a valid
>> operation for ubifs.
> 
> Is that a valid operation for all spi nors ?
>

AFAIK, yes, it is. First we need to erase a sector or a block, then we
can send page program commands to write data into the memory. We cannot
write more than a page size within a single page program command but you
can write less and start in the middle of a page if you want.
I don't know whether we could cross the page boundary if we start
writing from the middle of a page as long as we write less data than a
single page size. However spi_nor_write() don't do so, this is why it
computes
page_remain = min_t(size_t, nor->page_size - page_offset, len - i)

Well, now looking at the Spansion S25FL127S datasheet, the address is
wrapped if we cross the page boundary:

"Depending on the device configuration, the page size can either be 256
or 512 bytes. Up to a page can be provided on SI after the 3-byte
address with instruction 02h or 4-byte address with instruction 12h has
been provided. If the 9 least significant address bits (A8-A0) are not
all 0, all transmitted data that goes beyond the end of the current page
are programmed from the start address of the same page (from the address
whose 9 least significant bits (A8-A0) are all 0) i.e. the address wraps
within the page aligned address boundaries. This is a result of only
requiring the user to enter one single page address to cover the entire
page boundary."

Then from Adesto AT25DF321A datasheet:
"If the starting memory address denoted by A23-A0 does not fall on an
even 256-byte page boundary (A7-A0 are not all 0), then special
circumstances regarding which memory locations to be programmed will
apply. In this situation, any data that is sent to the device that goes
beyond the end of the page will wrap around back to the beginning of the
same page. For example, if the starting address denoted by A23-A0 is
0000FEh, and three bytes of data are sent to the device, then the first
two bytes of data will be programmed at addresses 0000FEh and 0000FFh
while the last byte of data will be programmed at address 000000h. The
remaining bytes in the page (addresses 000001h through 0000FDh) will not
be programmed and will remain in the erased state (FFh). In addition, if
more than 256-bytes of data are sent to the device, then only the last
256-bytes sent will be latched into the internal buffer."

Besides, the wear leveling is handled by the ubi layer I guess, at the
spi-nor level we write raw data. Maybe Richard and Boris could tell us
more but talking with them I've understood that's it is normal for the
ubi layer to write at offset 64.

Best regards,

Cyrille

>> Hence this warning is pretty annoying and useless so we just remove it.
>>
>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@...el.com>
>> Suggested-by: Richard Weinberger <richard@....at>
>> Suggested-by: Andras Szemzo <szemzo.andras@...il.com>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index c9bcd05ca5d9..cdeb2c6b1492 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -1265,9 +1265,6 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>>  		ssize_t written;
>>  
>>  		page_offset = (to + i) & (nor->page_size - 1);
>> -		WARN_ONCE(page_offset,
>> -			  "Writing at offset %zu into a NOR page. Writing partial pages may decrease reliability and increase wear of NOR flash.",
>> -			  page_offset);
>>  		/* the size of data remaining on the first page */
>>  		page_remain = min_t(size_t,
>>  				    nor->page_size - page_offset, len - i);
>>
> 
>