Re: [PATCH 0/3] mtd: Make sure UBIFS does not do multi-pass page programming on flashes that don't support it

[Vignesh Raghavendra]

On 11/3/20 6:15 PM, Pratyush Yadav wrote:
> On 03/11/20 05:05PM, Vignesh Raghavendra wrote:
>>
>>
>> On 11/1/20 3:14 AM, Richard Weinberger wrote:
>>> On Tue, Oct 27, 2020 at 12:24 PM Pratyush Yadav <p.yadav@xxxxxx> wrote:
>>>>> [0] https://lore.kernel.org/linux-mtd/20201005153138.6437-1-p.yadav@xxxxxx/
>>>>
>>>> Ping. Any comments on the series?
>>>
>>> From the UBIFS point of view I'd like to avoid as many device specific
>>> settings as possible.
>>> We check already for NOR flash, checking for NOR *and* SPI_NOR_NO_MULTI_PASS_PP
>>> feels a bit clumsy.
>>>
>>> Tudor, what do you think about SPI_NOR_NO_MULTI_PASS_PP?
>>> This kind of NOR seems to be a little NAND'ish. Maybe we can hide this detail
>>> in the mtd framework?
>>>
>>
>> Agree with Richard. I don't see need for SPI_NOR_NO_MULTI_PASS_PP. From
>> MTD point of view setting mtd->writesize to be equal to pagesize should
>> be enough. Its upto clients of MTD devices to ensure there is no multi
>> pass programming within a "writesize" block.
> 
> That is what I initially thought too but then I realized that multi-pass 
> programming is completely different from page-size programming. Instead 
> of writing 4 bytes twice, you can zero out the entire page in one single 
> operation. You would be compliant with the write size requirement but 
> you still do multi-pass programming because you did not erase the page 
> before this operation.
> 

Right...

> It is also not completely correct to say the Cypress S28 flash has a 
> write size of 256. You _can_ write one byte if you want. You just can't 
> write to that page again without erasing it first. For example, if a 
> file system only wants to write 128 bytes on a page, it can do so 
> without having to write the whole page. It just needs to make sure it 
> doesn't write to it again without erasing first.
> 

As per documentation:
mtd_info::writesize: "In case of ECC-ed NOR it is of ECC block size"

This means, it is block on which ECC is calculated on ECC-ed NOR and
thus needs to be erased every time before being updated.

Looking at flash datasheet, this seems to be 16 bytes.

So mtd->writesize = 16 and not 256 (or pagesize)


Also, It does not imply length of data being written has to be multiple
of it. At least NAND subsystem does not seem to care that during  writes
len < mtd->writesize[1].

> nor_erase_prepare() was written to handle quirks of some specific 
> devices. Not every device starts filling zeroes from the end of a page. 
> So we have device-specific code in UBIFS already. You will obviously 
> need device-specific settings to have control over that code.
> 

UBIFS intends to be robust against rogue power cuts and therefore would
need to ensure some consistency during erase which explains flash
specific quirk here.

> One might argue that we should move nor_erase_prepare() out of UBIFS. 
> But requiring a flash to start erasing from the start of the page is a 
> UBIFS-specific requirement. Other users of a flash might not care about 
> it at all.
> 

Yes. But I don't see much harm done.

> And so we have ourselves a bit of a conundrum. Adding 
> SPI_NOR_NO_MULTI_PASS_PP is IMHO the least disruptive answer. If the 
> file system wants to do multi-pass page programming on NOR flashes, how 
> else do we tell it not to do it for this specific flash?
> 

I see don't see need for SPI_NOR_NO_MULTI_PASS_PP as
SPI_NOR_NO_MULTI_PASS_PP is implied within a ECC block and writesize is
supposed to represent the same.

>> If this is not clear in the current documentation of struct mtd, then
>> that can be updated.
> 

[1]
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L4166