Re: Regression for NOR flash with multiple erase block regions

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

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

> > -----Original Message-----
> > From: Boris Brezillon [mailto:boris.brezillon@free-electrons.com]
> > Sent: den 25 september 2017 10:14
> > 
> > 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.g
> > > > > > it/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
> >   
> 
> Thank you for solving this problem!

No problem. Actually you're the one who found the bug and reported it
(and almost fixed it), so you're the one we should thank.

BTW, can you add your Tested-by/Reviewed-by on the last version? I'll
queue it for the next -rc.

Thanks,

Boris