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

From: Pratyush Yadav
Date: Thu Nov 05 2020 - 08:21:49 EST


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