Re: Regression for NOR flash with multiple erase block regions

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Mon, 25 Sep 2017 06:28:18 +0000
Mathias Thore <Mathias.Thore@infinera.com> wrote:

> > From: Boris Brezillon [mailto:boris.brezillon@free-electrons.com]
> > Sent: den 22 september 2017 22:05
> > 
> > On Fri, 22 Sep 2017 18:27:42 +0000
> > Chris Packham <Chris.Packham@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/commit/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?

--->8---
From cdc8a5078ee330a645d1076a8289b411cffc4257 Mon Sep 17 00:00:00 2001
From: Boris Brezillon <boris.brezillon@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@infinera.com>
Fixes: 1eeef2d7483a ("mtd: handle partitioning on devices with 0 erasesize")
Cc: <stable@vger.kernel.org>
Signed-off-by: Boris Brezillon <boris.brezillon@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) {