Re: Regression for NOR flash with multiple erase block regions

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

On Mon, 25 Sep 2017 08:05:03 +0000
Mathias Thore <Mathias.Thore@infinera.com> wrote:

> > -----Original Message-----
> > From: Boris Brezillon [mailto:boris.brezillon@free-electrons.com]
> > Sent: den 25 september 2017 09:30
> > 
> > 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/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.

Oops. I'll fix that and send a new version.

Thanks,

Boris

> 
> >   
> > --->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) {