Re: [PATCH 1/1] mtd: ubi: skip programming unused bits in ubi headers

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hello,

On 06/11/2025 at 13:49:40 +08, Cheng Ming Lin <linchengming884@gmail.com> wrote:

> From: Cheng Ming Lin <chengminglin@mxic.com.tw>
>
> This patch prevents unnecessary programming of bits in ec_hdr and
> vid_hdr that are not used or read during normal UBI operation. These
> unused bits are typcially already set to 1 in erased flash and do not
> need to be explicitly programmed to 0 if they are not used.
>
> Programming such unused areas offers no functional benefit and may
> result in unnecessary flash wear, reducing the overall lifetime of the
> device. By skipping these writes, we preserve the flash state as much as
> possible and minimize wear caused by redundant operations.
>
> This change ensures that only necessary fields are written when preparing
> UBI headers, improving flash efficiency without affecting functionality.
>
> Additionally, the Kioxia TC58NVG1S3HTA00 datasheet (page 63) also notes
> that continuous program/erase cycling with a high percentage of '0' bits
> in the data pattern can accelerate block endurance degradation.
> This further supports avoiding large 0x00 patterns.
>
> Link: https://europe.kioxia.com/content/dam/kioxia/newidr/productinfo/datasheet/201910/DST_TC58NVG1S3HTA00-TDE_EN_31442.pdf
>
> Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw>

Thanks for this very clear and detailed commit log, as well as for the
well written cover letter. I am personally fine with the overall idea of
clearing these unused bits to 1. Yet, I have one concern regarding the
implementation, please see below.

> ---
>  drivers/mtd/ubi/io.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
> index a4999bce4..c21242a14 100644
> --- a/drivers/mtd/ubi/io.c
> +++ b/drivers/mtd/ubi/io.c
> @@ -868,6 +868,8 @@ int ubi_io_write_ec_hdr(struct ubi_device *ubi, int pnum,
>  		return -EROFS;
>  	}
>  
> +	memset((char *)ec_hdr + UBI_EC_HDR_SIZE, 0xFF, ubi->ec_hdr_alsize - UBI_EC_HDR_SIZE);
> +
>  	err = ubi_io_write(ubi, ec_hdr, pnum, 0, ubi->ec_hdr_alsize);
>  	return err;
>  }
> @@ -1150,6 +1152,11 @@ int ubi_io_write_vid_hdr(struct ubi_device *ubi, int pnum,
>  		return -EROFS;
>  	}
>  
> +	if (ubi->vid_hdr_shift)
> +		memset((char *)p, 0xFF, ubi->vid_hdr_alsize - UBI_VID_HDR_SIZE);
> +	else
> +		memset((char *)p + UBI_VID_HDR_SIZE, 0xFF, ubi->vid_hdr_alsize - UBI_VID_HDR_SIZE);

Here I am reaching the limits of my UBI knowledge, so I would prefer
Richard to (in)validate what I am saying, but AFAIU, the VID header can
be literally anywhere in the page, not just at the start or end of a
subpage, so in the vid_hdr_shift I would expect some extra maths to
happen, no?

Here is an excerpt of the main comment at the top of the io.c file:

    * As it was noted above, the VID header may start at a non-aligned
    * offset. For example, in case of a 2KiB page NAND flash with a 512
    * bytes sub-page, the VID header may reside at offset 1984 which is
    * the last 64 bytes of the * last sub-page (EC header is always at
    * offset zero).

I am not sure this is super common today though.

Thanks,
Miquèl