[PATCH 1/1] mtd: spi-nor: remove WARN_ONCE() message in spi_nor_write()

[PATCH 1/1] mtd: spi-nor: remove WARN_ONCE() message in spi_nor_write()

[Cyrille Pitchen]

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.

Hence this warning is pretty annoying and useless so we just remove it.

Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
Suggested-by: Richard Weinberger <richard@nod.at>
Suggested-by: Andras Szemzo <szemzo.andras@gmail.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);
-- 
2.7.4

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

[Marek Vasut]

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 ?

> Hence this warning is pretty annoying and useless so we just remove it.
> 
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> Suggested-by: Richard Weinberger <richard@nod.at>
> Suggested-by: Andras Szemzo <szemzo.andras@gmail.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);
> 


-- 
Best regards,
Marek Vasut

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

[Cyrille Pitchen]

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@atmel.com>
>> Suggested-by: Richard Weinberger <richard@nod.at>
>> Suggested-by: Andras Szemzo <szemzo.andras@gmail.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);
>>
> 
> 

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

[Marek Vasut]

On 12/07/2016 12:38 AM, Cyrille Pitchen wrote:
> 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 wasn't aware this partial and even unaligned programming was available
on all chips, OK.

> 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)

No, I don't think we can, I believe the PP would wrap around and program
the same page from the beginning.

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

Yeah, this matches my mental model.

> "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.

I'd understand RMW, but pure write seems a bit odd.


-- 
Best regards,
Marek Vasut

RE: [PATCH 1/1] mtd: spi-nor: remove WARN_ONCE() message in spi_nor_write()

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

Hi Cyrille,

> -----Original Message-----
> From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf
> Of Marek Vasut
> Sent: Wednesday, December 07, 2016 4:07 AM
> To: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>; Cyrille Pitchen
> <cyrille.pitchen@atmel.com>
> Cc: boris.brezillon@free-electrons.com; computersforpeace@gmail.com;
> linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.org;
> richard@nod.at
> Subject: Re: [PATCH 1/1] mtd: spi-nor: remove WARN_ONCE() message in
> spi_nor_write()
> 
> On 12/07/2016 12:38 AM, Cyrille Pitchen wrote:
> > 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.
> 
Technically you can, but for some chips this warning is indeed right, and at 
least force the user to take a look. See this:

http://www.macronix.com/Lists/ApplicationNote/Attachments/1606/AN0302V1%20-%20MX25L_G%20Serial%20Flash%20Programming%20Guide.pdf

Thanks,
Marcin

> I wasn't aware this partial and even unaligned programming was available on
> all chips, OK.
> 
> > 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)
> 
> No, I don't think we can, I believe the PP would wrap around and program
> the same page from the beginning.
> 
> > Well, now looking at the Spansion S25FL127S datasheet, the address is
> > wrapped if we cross the page boundary:
> 
> Yeah, this matches my mental model.
> 
> > "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.
> 
> I'd understand RMW, but pure write seems a bit odd.
> 
> 
> --
> Best regards,
> Marek Vasut
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Boris Brezillon]

On Wed, 7 Dec 2016 04:07:05 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 12/07/2016 12:38 AM, Cyrille Pitchen wrote:
> > 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 wasn't aware this partial and even unaligned programming was available
> on all chips, OK.
> 
> > 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)  
> 
> No, I don't think we can, I believe the PP would wrap around and program
> the same page from the beginning.
> 
> > Well, now looking at the Spansion S25FL127S datasheet, the address is
> > wrapped if we cross the page boundary:  
> 
> Yeah, this matches my mental model.
> 
> > "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.  
> 
> I'd understand RMW, but pure write seems a bit odd.
> 
> 

Well, UBI trusts what the MTD framwork says, and here [1] you say that
the minimum write unit is a byte, so UBI tries to optimize flash usage
by putting the EC and VID headers next to each other.
Now, if you want UBI to act differently, customize ->writesize on a,
but, by doing that, you may introduce regressions because some users
are already using UBI with this layout (ECH @0, VIDH @64).

[1]http://lxr.free-electrons.com/source/drivers/mtd/spi-nor/spi-nor.c#L1368

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

[Cyrille Pitchen]

Le 07/12/2016 à 07:21, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> Hi Cyrille,
> 
>> -----Original Message-----
>> From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf
>> Of Marek Vasut
>> Sent: Wednesday, December 07, 2016 4:07 AM
>> To: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>; Cyrille Pitchen
>> <cyrille.pitchen@atmel.com>
>> Cc: boris.brezillon@free-electrons.com; computersforpeace@gmail.com;
>> linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.org;
>> richard@nod.at
>> Subject: Re: [PATCH 1/1] mtd: spi-nor: remove WARN_ONCE() message in
>> spi_nor_write()
>>
>> On 12/07/2016 12:38 AM, Cyrille Pitchen wrote:
>>> 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.
>>
> Technically you can, but for some chips this warning is indeed right, and at 
> least force the user to take a look. See this:
> 
> http://www.macronix.com/Lists/ApplicationNote/Attachments/1606/AN0302V1%20-%20MX25L_G%20Serial%20Flash%20Programming%20Guide.pdf
> 

This Macronix document recommends to align both the start offset and the
length to a 16-byte boundary. However the WARN_ONCE() macro only checks the
start offset but doesn't test the length. Also it tests a page-size
alignment, which is a stronger constraint than a 16-byte alignment.

In the case of Macronix mx25l_g memories, when the UBI layer writes at
offset 64, the warning is a false positive, isn't it?

Also WARN_ONCE() dumps the call stack making people think of a kernel oops,
displayed once per boot when mounting a ubifs partition.

If the issue exists, printing a warning does not fix it.

So what should we do?

Best regards,

Cyrille

> Thanks,
> Marcin
> 
>> I wasn't aware this partial and even unaligned programming was available on
>> all chips, OK.
>>
>>> 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)
>>
>> No, I don't think we can, I believe the PP would wrap around and program
>> the same page from the beginning.
>>
>>> Well, now looking at the Spansion S25FL127S datasheet, the address is
>>> wrapped if we cross the page boundary:
>>
>> Yeah, this matches my mental model.
>>
>>> "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.
>>
>> I'd understand RMW, but pure write seems a bit odd.
>>
>>
>> --
>> Best regards,
>> Marek Vasut
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 

RE: [PATCH 1/1] mtd: spi-nor: remove WARN_ONCE() message in spi_nor_write()

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

> -----Original Message-----
> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
> Sent: Wednesday, December 07, 2016 2:08 PM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> <marcin.krzeminski@nokia.com>; Marek Vasut <marek.vasut@gmail.com>;
> Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
> Cc: boris.brezillon@free-electrons.com; computersforpeace@gmail.com;
> linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.org;
> richard@nod.at
> Subject: Re: [PATCH 1/1] mtd: spi-nor: remove WARN_ONCE() message in
> spi_nor_write()
> 
> Le 07/12/2016 à 07:21, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> > Hi Cyrille,
> >
> >> -----Original Message-----
> >> From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On
> >> Behalf Of Marek Vasut
> >> Sent: Wednesday, December 07, 2016 4:07 AM
> >> To: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>; Cyrille Pitchen
> >> <cyrille.pitchen@atmel.com>
> >> Cc: boris.brezillon@free-electrons.com; computersforpeace@gmail.com;
> >> linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.org;
> >> richard@nod.at
> >> Subject: Re: [PATCH 1/1] mtd: spi-nor: remove WARN_ONCE() message in
> >> spi_nor_write()
> >>
> >> On 12/07/2016 12:38 AM, Cyrille Pitchen wrote:
> >>> 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.
> >>
> > Technically you can, but for some chips this warning is indeed right,
> > and at least force the user to take a look. See this:
> >
> >
> http://www.macronix.com/Lists/ApplicationNote/Attachments/1606/AN030
> 2V
> > 1%20-%20MX25L_G%20Serial%20Flash%20Programming%20Guide.pdf
> >
> 
> This Macronix document recommends to align both the start offset and the
> length to a 16-byte boundary. However the WARN_ONCE() macro only
> checks the start offset but doesn't test the length. Also it tests a page-size
> alignment, which is a stronger constraint than a 16-byte alignment.
> 
> In the case of Macronix mx25l_g memories, when the UBI layer writes at
> offset 64, the warning is a false positive, isn't it?
They are also saying about writing full pages at once (chapter 5).
> 
> Also WARN_ONCE() dumps the call stack making people think of a kernel
> oops, displayed once per boot when mounting a ubifs partition.
> 
> If the issue exists, printing a warning does not fix it.
> 
If it generate to many noise and confuse users then this patch is really needed.

Thanks,
Marcin
> So what should we do?
> 
> Best regards,
> 
> Cyrille
> 
> > Thanks,
> > Marcin
> >
> >> I wasn't aware this partial and even unaligned programming was
> >> available on all chips, OK.
> >>
> >>> 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)
> >>
> >> No, I don't think we can, I believe the PP would wrap around and
> >> program the same page from the beginning.
> >>
> >>> Well, now looking at the Spansion S25FL127S datasheet, the address
> >>> is wrapped if we cross the page boundary:
> >>
> >> Yeah, this matches my mental model.
> >>
> >>> "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.
> >>
> >> I'd understand RMW, but pure write seems a bit odd.
> >>
> >>
> >> --
> >> Best regards,
> >> Marek Vasut
> >>
> >> ______________________________________________________
> >> Linux MTD discussion mailing list
> >> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> >

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

[Boris Brezillon]

On Tue, 6 Dec 2016 18:14:24 +0100
Cyrille Pitchen <cyrille.pitchen@atmel.com> 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.
> 
> Hence this warning is pretty annoying and useless so we just remove it.
> 
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> Suggested-by: Richard Weinberger <richard@nod.at>
> Suggested-by: Andras Szemzo <szemzo.andras@gmail.com>

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.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);

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

[Cyrille Pitchen]

Le 08/12/2016 à 16:31, Boris Brezillon a écrit :
> On Tue, 6 Dec 2016 18:14:24 +0100
> Cyrille Pitchen <cyrille.pitchen@atmel.com> 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.
>>
>> Hence this warning is pretty annoying and useless so we just remove it.
>>
>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
>> Suggested-by: Richard Weinberger <richard@nod.at>
>> Suggested-by: Andras Szemzo <szemzo.andras@gmail.com>
> 
> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> 
Applied to git://github.com/spi-nor/linux.git

>> ---
>>  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);
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 

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

[Brian Norris]

On Tue, Dec 13, 2016 at 05:43:52PM +0100, Cyrille Pitchen wrote:
> Le 08/12/2016 à 16:31, Boris Brezillon a écrit :
> > On Tue, 6 Dec 2016 18:14:24 +0100
> > Cyrille Pitchen <cyrille.pitchen@atmel.com> 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.
> >>
> >> Hence this warning is pretty annoying and useless so we just remove it.
> >>
> >> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> >> Suggested-by: Richard Weinberger <richard@nod.at>
> >> Suggested-by: Andras Szemzo <szemzo.andras@gmail.com>
> > 
> > Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > 
> Applied to git://github.com/spi-nor/linux.git

Do you have any idea on how to handle or communicate this better? I
recall Michal added this because he was adding new write looping that
didn't previously exist; we might create partial-page writes because his
SPI controller was too dumb to make larger ones.

Anyway, since this is hitting real (and more or less false positive)
use cases, then this patch is probably good.

Brian

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

[Cyrille Pitchen]

Hi Brian,

Le 09/02/2017 à 03:23, Brian Norris a écrit :
> On Tue, Dec 13, 2016 at 05:43:52PM +0100, Cyrille Pitchen wrote:
>> Le 08/12/2016 à 16:31, Boris Brezillon a écrit :
>>> On Tue, 6 Dec 2016 18:14:24 +0100
>>> Cyrille Pitchen <cyrille.pitchen@atmel.com> 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.
>>>>
>>>> Hence this warning is pretty annoying and useless so we just remove it.
>>>>
>>>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
>>>> Suggested-by: Richard Weinberger <richard@nod.at>
>>>> Suggested-by: Andras Szemzo <szemzo.andras@gmail.com>
>>>
>>> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>>>
>> Applied to git://github.com/spi-nor/linux.git
> 
> Do you have any idea on how to handle or communicate this better? I
> recall Michal added this because he was adding new write looping that
> didn't previously exist; we might create partial-page writes because his
> SPI controller was too dumb to make larger ones.
> 
> Anyway, since this is hitting real (and more or less false positive)
> use cases, then this patch is probably good.
> 
> Brian
> 

Some people had complained and reported "bugs" about this warning during
kernel boot especially when using ubifs.
If I remember, someone even suggested that Michal's patch should be
reverted but I think except for the WARN_ONCE() his patch is good and
should be kept. So I proposed the remove only the WARN_ONCE() line.

spi_nor_scan() sets mtd->writesize to 1, so for upper mtd layers such as
ubi, it is a valid operation to write less than a page size. Besides, if we
changed the mtd->writesize value to nor->page_size for instance, it would
have an impact on existing ubi file systems which may then appear as
corrupted if I've understood correctly. Maybe Richard can confirm this point?

Also, the warning doesn't fix anything: everything works just as before.
So IF the driver was buggy before, it is still buggy today.
Displaying such an intrusive warning (backtrace and so on) "scares" people
(at the first look, we think a kernel oops/crash has occurred) but
currently we are not able to propose any solution.

In most, if not all, cases this warning is a false positive as for most
memories it is valid to write a single page with more than one Page Program
commands or to write data starting from the middle of the page.

The only hardware limitation at the SPI NOR side is that we can't cross the
page boundary. If we would do so, internally the SPI NOR memory would wrap
the address instead of incrementing it hence the additional data would be
written at the beginning of the same page and not at the beginning of the
next one.
That why Michal's patch should be kept, IMHO.

I hope those additional explanations will help you :)

Best regards,

Cyrille

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

[Brian Norris]

On Thu, Feb 09, 2017 at 02:50:02PM +0100, Cyrille Pitchen wrote:
> In most, if not all, cases this warning is a false positive as for most
> memories it is valid to write a single page with more than one Page Program
> commands or to write data starting from the middle of the page.

Right, I agree that many cases were false positives, and the WARN is
rather scary, so it's definitely good to drop the WARN. And I've already
pulled that patch in, so we're good :)

> The only hardware limitation at the SPI NOR side is that we can't cross the
> page boundary.

Is it the *only* limitation? Marcin pointed out that at least for some
Macronix flash:

http://www.macronix.com/Lists/ApplicationNote/Attachments/1606/AN0302V1%20-%20MX25L_G%20Serial%20Flash%20Programming%20Guide.pdf

  "it is strongly recommended that data be written to the MX25L_G using
  full 256-byte pages"

.

Now I don't claim the original WARN captured this recommendation
correctly, but I was just nudging people to consider the original
intention.

> If we would do so, internally the SPI NOR memory would wrap
> the address instead of incrementing it hence the additional data would be
> written at the beginning of the same page and not at the beginning of the
> next one.

Brian