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

[Pratyush Yadav]

On 05/11/20 05:51PM, Vignesh Raghavendra wrote:
> 
> 
> 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)

Ok.
 
> 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].

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

Yes. There is no arguing if this is needed. The only question is how to 
skip it on flashes that don't support doing this.
 
> > 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.

Ok. So we can control the execution of nor_erase_prepare() with 
mtd->writesize. Will re-roll. Thanks.
 
> >> 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

-- 
Regards,
Pratyush Yadav
Texas Instruments India