Re: Regression for NOR flash with multiple erase block regions

Re: Regression for NOR flash with multiple erase block regions

[Chris Packham]

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/drivers/mtd/mtdpart.c?h=v4.13&id=1eeef2d7483a7e3f8d2dd2a5b9939b3b814dc549 
> ) 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.

Sorry about that. I think a fix would be to re-calculate the 
wr_alignment as we're looking at each erase block. Unfortunately I'm 
about to get on a plane for 13 hours so I'm pretty much a write off for 
the next couple of days.

I'll take a look when I get back on-line unless Boris beats me to it.

Re: Regression for NOR flash with multiple erase block regions

[Boris Brezillon]

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/drivers/mtd/mtdpart.c?h=v4.13&id=1eeef2d7483a7e3f8d2dd2a5b9939b3b814dc549 
> > ) 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.

Maybe MTD_NO_ERASE is set on your NOR, and ->writesize is used in place
of ->erasesize to check the alignment, but ->writesize is normally set
to 1 on NOR devices, so again, no real reasons for this failure.

> 
> Sorry about that. I think a fix would be to re-calculate the 
> wr_alignment as we're looking at each erase block. Unfortunately I'm 
> about to get on a plane for 13 hours so I'm pretty much a write off for 
> the next couple of days.
> 
> I'll take a look when I get back on-line unless Boris beats me to it.

RE: Regression for NOR flash with multiple erase block regions

[Mathias Thore]

> 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.

The flash in question has 4 erase blocks of size 0x8000 at the start, and then the rest of the erase blocks are size 0x20000. Here is some data from traces I added around the point of failure:

0x000000000000-0x000000008000 : "XYZ"
dbg: wr_alignment 0x00020000
dbg: parent->flags & MTD_NO_ERASE = 0
dbg: parent->writesize 0x00000001
dbg: parent->erasesize 0x00020000
dbg: slave->mtd.writesize 0x00000001
dbg: slave->mtd.erasesize 0x00008000
dbg: slave->offset 0x0000000000000000
dbg: slave->mtd.size 0x0000000000008000
dbg: remainder (start) 0x00000000
dbg: remainder (end) 0x00008000
mtd: partition "XYZ" doesn't end on an erase/write block -- force read-only

> 
> Maybe MTD_NO_ERASE is set on your NOR, and ->writesize is used in place
> of ->erasesize to check the alignment, but ->writesize is normally set
> to 1 on NOR devices, so again, no real reasons for this failure.
> >
> > Sorry about that. I think a fix would be to re-calculate the
> > wr_alignment as we're looking at each erase block. Unfortunately I'm
> > about to get on a plane for 13 hours so I'm pretty much a write off for
> > the next couple of days.
> >
> > I'll take a look when I get back on-line unless Boris beats me to it.

Re: Regression for NOR flash with multiple erase block regions

[Boris Brezillon]

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

RE: Regression for NOR flash with multiple erase block regions

[Mathias Thore]

> -----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.

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

Re: Regression for NOR flash with multiple erase block regions

[Boris Brezillon]

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

RE: Regression for NOR flash with multiple erase block regions

[Mathias Thore]

> -----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!
/Mathias


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

Re: Regression for NOR flash with multiple erase block regions

[Boris Brezillon]

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

Regression for NOR flash with multiple erase block regions

[Mathias Thore]

Hello,

Commit 1eeef2d7483a7e3f8d2dd2a5b9939b3b814dc549 included in Linux 4.13 ( https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/mtd/mtdpart.c?h=v4.13&id=1eeef2d7483a7e3f8d2dd2a5b9939b3b814dc549 ) 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.


Regards,
Mathias Thore