Re: [PATCH] spi-nor: Verify written data in paranoid mode

From: "Csókás Bence" <csokas.bence@prolan.hu>
To: Michael Walle <mwalle@kernel.org>,
	<linux-mtd@lists.infradead.org>, <linux-kernel@vger.kernel.org>
Cc: "Szentendrei, Tamás" <szentendrei.tamas@prolan.hu>,
	"Tudor Ambarus" <tudor.ambarus@linaro.org>,
	"Pratyush Yadav" <pratyush@kernel.org>,
	"Miquel Raynal" <miquel.raynal@bootlin.com>,
	"Richard Weinberger" <richard@nod.at>,
	"Vignesh Raghavendra" <vigneshr@ti.com>
Subject: Re: [PATCH] spi-nor: Verify written data in paranoid mode
Date: Wed, 16 Apr 2025 14:38:27 +0200	[thread overview]
Message-ID: <dd3c7cc0-a568-4046-b105-e6786b5c80f8@prolan.hu> (raw)
In-Reply-To: <D981O3AA6NK9.2EEVUPM62EV6S@kernel.org>

Hi,

On 2025. 04. 16. 13:59, Michael Walle wrote:
> Hi,
> 
>> Add MTD_SPI_NOR_PARANOID config option for verifying all written data to
>> prevent silent bit errors to be undetected, at the cost of halving SPI
>> bandwidth.
> 
> What is the use case for this? Why is it specific to SPI-NOR
> flashes? Or should it rather be an MTD "feature". I'm not sure
> whether this is the right way to do it, thus I'd love to hear more
> about the background story to this.

Well, our case is quite specific, but we wanted to provide a general 
solution for upstream. In our case we have a component in the data path 
that can cause a burst bit error, on average after about a hundred 
megabytes written.

We _could_ make it MTD-wide, in our case we only have a NOR Flash 
onboard so this is where we added it. If it were in the MTD core, where 
would it make sense?

* mtd_write()
* mtd_write_oob()
* mtd_write_oob_std()
* or somewhere else entirely?

>> Co-developed-by: Szentendrei, Tamás <szentendrei.tamas@prolan.hu>
>> Signed-off-by: Szentendrei, Tamás <szentendrei.tamas@prolan.hu>
>> Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
>> ---
>>   drivers/mtd/spi-nor/Kconfig | 10 ++++++++++
>>   drivers/mtd/spi-nor/core.c  | 33 +++++++++++++++++++++++++++++++++
>>   2 files changed, 43 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index 24cd25de2b8b..425ea9a22424 100644
>> --- a/drivers/mtd/spi-nor/Kconfig
>> +++ b/drivers/mtd/spi-nor/Kconfig
>> @@ -68,6 +68,16 @@ config MTD_SPI_NOR_SWP_KEEP
>>   
>>   endchoice
>>   
>> +config MTD_SPI_NOR_PARANOID
>> +	bool "Read back written data (paranoid mode)"
> 
> No kernel configs please. This doesn't scale. What if you have two
> flashes and one should have this and one does not?

Yes, we have thought about this, but concluded that "paranoid mode" is 
not device-specific, you either have a requirement to be extra sure 
about data integrity, or you can be fairly sure your system is sane, in 
both cases it is a judgement about the entire system. I also thought 
that maybe some devices could be exempt from this, contingent on a 
`no-paranoia` Device Tree property, to selectively sacrifice integrity 
for performance even in paranoid mode, but we only have one Flash 
anyway, so I didn't implement it.

> -michael

Bence

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/