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@xxxxxxxxx> 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@xxxxxxxxx>
>>>> Suggested-by: Richard Weinberger <richard@xxxxxx>
>>>> Suggested-by: Andras Szemzo <szemzo.andras@xxxxxxxxx>
>>>
>>> Acked-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
>>>
>> 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