Regression for NOR flash with multiple erase block regions

[Mathias Thore]

> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon at free-electrons.com]
> Sent: den 25 september 2017 09:30
> 
> On Mon, 25 Sep 2017 06:28:18 +0000
> Mathias Thore <Mathias.Thore at infinera.com> wrote:
> 
> > > From: Boris Brezillon [mailto:boris.brezillon at free-electrons.com]
> > > Sent: den 22 september 2017 22:05
> > >
> > > On Fri, 22 Sep 2017 18:27:42 +0000
> > > Chris Packham <Chris.Packham at alliedtelesis.co.nz> wrote:
> > >
> > > > Hi Mathias,
> > > >
> > > > On 23/09/17 01:12, Mathias Thore wrote:
> > > > > Hello,
> > > > >
> > > > > Commit 1eeef2d7483a7e3f8d2dd2a5b9939b3b814dc549 included in
> > > > > Linux
> > > 4.13 (
> > > > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/c
> > > ommit/d
> > >
> rivers/mtd/mtdpart.c?h=v4.13&id=1eeef2d7483a7e3f8d2dd2a5b9939b3b814
> > > dc549
> > > > > ) introduces a regression for NOR flash with multiple erase
> > > > > block regions of different sizes.
> > > > >
> > > > > Only the largest erase block size seems to be considered when
> > > > > determining if partitions are aligned. Partitions in smaller
> > > > > regions will be mounted as read-only. With Linux 4.12 and
> > > > > earlier, read/write access was available for these partitions.
> > >
> > > I don't understand how this could work before this patch? I mean, we
> > > were previously using mtd_mod_by_eb() to check part alignment and
> > > this functions is just returning the remainder of the off /
> > > erasesize division. So, assuming the erasesize of your NOR did not
> > > change between 4.12 and 4.13, I don't see how this commit could
> > > cause the regression you're describing here.
> >
> > Looking at the earlier code, in the call to mtd_mod_by_eb, the parameter
> is slave->mtd. The slave mtd struct holds the correct erase size. The new
> code uses wr_alignment for all alignment tests, which comes from
> master/parent, and seems to always hold the largest possible erase size.
> 
> Oh indeed! I didn't notice that the initial mtd_mod_by_eb() test was done
> against the slave dev and not the master one.
> 
> Can you test the following patch and let me know it it solves your problem?

The patch does not compile in my 4.13 tree. However,

	if (!(slave->mtd.flags & MTD_NO_ERASE))
		wr_alignment = slave->mtd.erasesize;
or
	if (!(master->flags & MTD_NO_ERASE))
		wr_alignment = slave->mtd.erasesize;

do, and both work equally well to solve my problem.

> 
> --->8---
> From cdc8a5078ee330a645d1076a8289b411cffc4257 Mon Sep 17 00:00:00
> 2001
> From: Boris Brezillon <boris.brezillon at free-electrons.com>
> Date: Mon, 25 Sep 2017 09:14:00 +0200
> Subject: [PATCH] mtd: Fix partition alignment check on multi-erasesize
> devices
> 
> Commit 1eeef2d7483a ("mtd: handle partitioning on devices with 0
> erasesize") introduced a regression on heterogeneous erase region devices.
> Alignment of the partition was tested against the master eraseblock size
> which can be bigger than the slave one, thus leading to some partitions being
> marked as read-only.
> 
> Update wr_alignment to match this slave erasesize after this erasesize has
> been determined by picking the biggest erasesize of all the regions
> embedded in the MTD partition.
> 
> Reported-by: Mathias Thore <Mathias.Thore at infinera.com>
> Fixes: 1eeef2d7483a ("mtd: handle partitioning on devices with 0 erasesize")
> Cc: <stable at vger.kernel.org>
> Signed-off-by: Boris Brezillon <boris.brezillon at free-electrons.com>
> ---
>  drivers/mtd/mtdpart.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index
> 5736b0c90b33..8802185b7729 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -581,6 +581,14 @@ static struct mtd_part *allocate_partition(struct
> mtd_info *parent,
>  		slave->mtd.erasesize = parent->erasesize;
>  	}
> 
> +	/*
> +	 * Slave erasesize might differ from the master one if the
> master
> +	 * exposes several regions with different erasesize. Adjust
> +	 * wr_alignment accordingly.
> +	 */
> +	if (!(slave->flags & MTD_NO_ERASE))
> +		wr_alignment = slave->erasesize;
> +
>  	tmp = slave->offset;
>  	remainder = do_div(tmp, wr_alignment);
>  	if ((slave->mtd.flags & MTD_WRITEABLE) && remainder) {