* [PATCH] mtd: fix calculating partition end address @ 2020-03-09 7:44 Rafał Miłecki 2020-03-09 14:04 ` Miquel Raynal 0 siblings, 1 reply; 12+ messages in thread From: Rafał Miłecki @ 2020-03-09 7:44 UTC (permalink / raw) To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon Cc: Rafał Miłecki, linux-mtd From: Rafał Miłecki <rafal@milecki.pl> This fixes check for partitions that don't start at beginning of their parents. Missing partition's offset in formula could result in forcing read-only incorrectly. Fixes: 6750f61a13a0 ("mtd: improve calculating partition boundaries when checking for alignment") Signed-off-by: Rafał Miłecki <rafal@milecki.pl> --- drivers/mtd/mtdpart.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index 7328c066c5ba..c683b432cc5e 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -524,7 +524,7 @@ static struct mtd_part *allocate_partition(struct mtd_info *parent, part->name); } - tmp = part_absolute_offset(parent) + slave->mtd.size; + tmp = part_absolute_offset(parent) + slave->offset + slave->mtd.size; remainder = do_div(tmp, wr_alignment); if ((slave->mtd.flags & MTD_WRITEABLE) && remainder) { slave->mtd.flags &= ~MTD_WRITEABLE; -- 2.25.0 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] mtd: fix calculating partition end address 2020-03-09 7:44 [PATCH] mtd: fix calculating partition end address Rafał Miłecki @ 2020-03-09 14:04 ` Miquel Raynal 2020-03-09 14:19 ` Rafał Miłecki 0 siblings, 1 reply; 12+ messages in thread From: Miquel Raynal @ 2020-03-09 14:04 UTC (permalink / raw) To: Rafał Miłecki Cc: Rafał Miłecki, Richard Weinberger, linux-mtd, Vignesh Raghavendra, Boris Brezillon Hi Rafał, Rafał Miłecki <zajec5@gmail.com> wrote on Mon, 9 Mar 2020 08:44:45 +0100: > From: Rafał Miłecki <rafal@milecki.pl> > > This fixes check for partitions that don't start at beginning of their > parents. Missing partition's offset in formula could result in forcing > read-only incorrectly. > > Fixes: 6750f61a13a0 ("mtd: improve calculating partition boundaries when checking for alignment") > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > drivers/mtd/mtdpart.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > index 7328c066c5ba..c683b432cc5e 100644 > --- a/drivers/mtd/mtdpart.c > +++ b/drivers/mtd/mtdpart.c > @@ -524,7 +524,7 @@ static struct mtd_part *allocate_partition(struct mtd_info *parent, > part->name); > } > > - tmp = part_absolute_offset(parent) + slave->mtd.size; > + tmp = part_absolute_offset(parent) + slave->offset + slave->mtd.size; I think you are doing the change at the wrong place, if you want to check where the partition *starts* you should do it a few lines above. But I think the check should be here as well, probably. Anyway, I just applied on my local tree a patch rewriting a bit the partitioning scheme, could you please rebase on top of today's mtd/next and resend this patch updated? Here is the change that I've done at this place: - tmp = part_absolute_offset(parent) + slave->mtd.size; + tmp = mtd_get_master_ofs(child, 0) + child->size; Thanks! Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mtd: fix calculating partition end address 2020-03-09 14:04 ` Miquel Raynal @ 2020-03-09 14:19 ` Rafał Miłecki 2020-03-09 14:22 ` Miquel Raynal 2020-03-24 21:58 ` Miquel Raynal 0 siblings, 2 replies; 12+ messages in thread From: Rafał Miłecki @ 2020-03-09 14:19 UTC (permalink / raw) To: Miquel Raynal Cc: Richard Weinberger, Rafał Miłecki, linux-mtd, Vignesh Raghavendra, Boris Brezillon On 2020-03-09 15:04, Miquel Raynal wrote: > Rafał Miłecki <zajec5@gmail.com> wrote on Mon, 9 Mar 2020 08:44:45 > +0100: > >> From: Rafał Miłecki <rafal@milecki.pl> >> >> This fixes check for partitions that don't start at beginning of their >> parents. Missing partition's offset in formula could result in forcing >> read-only incorrectly. >> >> Fixes: 6750f61a13a0 ("mtd: improve calculating partition boundaries >> when checking for alignment") >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> --- >> drivers/mtd/mtdpart.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c >> index 7328c066c5ba..c683b432cc5e 100644 >> --- a/drivers/mtd/mtdpart.c >> +++ b/drivers/mtd/mtdpart.c >> @@ -524,7 +524,7 @@ static struct mtd_part *allocate_partition(struct >> mtd_info *parent, >> part->name); >> } >> >> - tmp = part_absolute_offset(parent) + slave->mtd.size; >> + tmp = part_absolute_offset(parent) + slave->offset + >> slave->mtd.size; > > I think you are doing the change at the wrong place, if you want to > check where the partition *starts* you should do it a few lines above. > But I think the check should be here as well, probably. The check where the partition *starts* is OK and I don't mean to change it. The bug is about calculating absolute *end* address of partition. > Anyway, I just applied on my local tree a patch rewriting a bit the > partitioning scheme, could you please rebase on top of today's > mtd/next and resend this patch updated? > > Here is the change that I've done at this place: > - tmp = part_absolute_offset(parent) + slave->mtd.size; > + tmp = mtd_get_master_ofs(child, 0) + child->size; I'll give it a try. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mtd: fix calculating partition end address 2020-03-09 14:19 ` Rafał Miłecki @ 2020-03-09 14:22 ` Miquel Raynal 2020-03-09 15:08 ` Rafał Miłecki 2020-03-24 21:58 ` Miquel Raynal 1 sibling, 1 reply; 12+ messages in thread From: Miquel Raynal @ 2020-03-09 14:22 UTC (permalink / raw) To: Rafał Miłecki Cc: Richard Weinberger, Rafał Miłecki, linux-mtd, Vignesh Raghavendra, Boris Brezillon Hi Rafał, Rafał Miłecki <rafal@milecki.pl> wrote on Mon, 09 Mar 2020 15:19:10 +0100: > On 2020-03-09 15:04, Miquel Raynal wrote: > > Rafał Miłecki <zajec5@gmail.com> wrote on Mon, 9 Mar 2020 08:44:45 > > +0100: > > > >> From: Rafał Miłecki <rafal@milecki.pl> > >> >> This fixes check for partitions that don't start at beginning of their > >> parents. Missing partition's offset in formula could result in forcing > >> read-only incorrectly. > >> >> Fixes: 6750f61a13a0 ("mtd: improve calculating partition boundaries >> when checking for alignment") > >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > >> --- > >> drivers/mtd/mtdpart.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> >> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > >> index 7328c066c5ba..c683b432cc5e 100644 > >> --- a/drivers/mtd/mtdpart.c > >> +++ b/drivers/mtd/mtdpart.c > >> @@ -524,7 +524,7 @@ static struct mtd_part *allocate_partition(struct >> mtd_info *parent, > >> part->name); > >> } > >> >> - tmp = part_absolute_offset(parent) + slave->mtd.size; > >> + tmp = part_absolute_offset(parent) + slave->offset + >> slave->mtd.size; > > > > I think you are doing the change at the wrong place, if you want to > > check where the partition *starts* you should do it a few lines above. > > But I think the check should be here as well, probably. > > The check where the partition *starts* is OK and I don't mean to change > it. The bug is about calculating absolute *end* address of partition. Can you detail a little bit then? Because I don't see the issue anymore even though I am convinced something is wrong here :) > > > > Anyway, I just applied on my local tree a patch rewriting a bit the > > partitioning scheme, could you please rebase on top of today's > > mtd/next and resend this patch updated? > > > > Here is the change that I've done at this place: > > - tmp = part_absolute_offset(parent) + slave->mtd.size; > > + tmp = mtd_get_master_ofs(child, 0) + child->size; > > I'll give it a try. Thanks, Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mtd: fix calculating partition end address 2020-03-09 14:22 ` Miquel Raynal @ 2020-03-09 15:08 ` Rafał Miłecki 2020-03-09 15:22 ` Miquel Raynal 0 siblings, 1 reply; 12+ messages in thread From: Rafał Miłecki @ 2020-03-09 15:08 UTC (permalink / raw) To: Miquel Raynal Cc: Richard Weinberger, Rafał Miłecki, linux-mtd, Vignesh Raghavendra, Boris Brezillon On 09.03.2020 15:22, Miquel Raynal wrote: > Rafał Miłecki <rafal@milecki.pl> wrote on Mon, 09 Mar 2020 15:19:10 > +0100: > >> On 2020-03-09 15:04, Miquel Raynal wrote: >>> Rafał Miłecki <zajec5@gmail.com> wrote on Mon, 9 Mar 2020 08:44:45 >>> +0100: >>> >>>> From: Rafał Miłecki <rafal@milecki.pl> >>>>>> This fixes check for partitions that don't start at beginning of their >>>> parents. Missing partition's offset in formula could result in forcing >>>> read-only incorrectly. >>>>>> Fixes: 6750f61a13a0 ("mtd: improve calculating partition boundaries >> when checking for alignment") >>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >>>> --- >>>> drivers/mtd/mtdpart.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c >>>> index 7328c066c5ba..c683b432cc5e 100644 >>>> --- a/drivers/mtd/mtdpart.c >>>> +++ b/drivers/mtd/mtdpart.c >>>> @@ -524,7 +524,7 @@ static struct mtd_part *allocate_partition(struct >> mtd_info *parent, >>>> part->name); >>>> } >>>>>> - tmp = part_absolute_offset(parent) + slave->mtd.size; >>>> + tmp = part_absolute_offset(parent) + slave->offset + >> slave->mtd.size; >>> >>> I think you are doing the change at the wrong place, if you want to >>> check where the partition *starts* you should do it a few lines above. >>> But I think the check should be here as well, probably. >> >> The check where the partition *starts* is OK and I don't mean to change >> it. The bug is about calculating absolute *end* address of partition. > > Can you detail a little bit then? Because I don't see the issue anymore > even though I am convinced something is wrong here :) Please consider following partitions layout: * bcm47xxsflash ├─ boot 0x000000000000-0x000000040000 └┬ firmware 0x000000040000-0x000001000000 ├─ linux 0x00000000001c-0x00000018f800 └┬ container 0x00000018f800-0x000000fc0000 ├─ foo 0x000000000000-0x000000630800 └─ bar 0x000000630800-0x000000e30800 (size: 0x800000) Existing (correct) *start* calculation: bar start: 0 + 0x040000 + 0x18f800 + 0x630800 = 0x800000 Existing (wrong) end calculation: bar end: 0 + 0x040000 + 0x18f800 + 0x800000 = 0x9cf800 Fixed (correct) end calculation: bar end: 0 + 0x040000 + 0x18f800 + 0x630800 + 0x800000 = 0x1000000 Because of that existing wrong end calculation I was getting: mtd: partition "bar" doesn't end on an erase/write block -- force read-only See: 2 bcm47xxpart partitions found on MTD device bcm47xxsflash Creating 2 MTD partitions on "bcm47xxsflash": 0x000000000000-0x000000040000 : "boot" 0x000000040000-0x000001000000 : "firmware" 2 trx partitions found on MTD device firmware Creating 2 MTD partitions on "firmware": 0x00000000001c-0x00000018f800 : "linux" mtd: partition "linux" doesn't start on an erase/write block boundary -- force read-only 0x00000018f800-0x000000fc0000 : "container" mtd: partition "container" doesn't start on an erase/write block boundary -- force read-only 2 container partitions found on MTD device container Creating 2 MTD partitions on "container": 0x000000000000-0x000000630800 : "foo" mtd: partition "foo" doesn't start on an erase/write block boundary -- force read-only 0x000000630800-0x000000e30800 : "bar" mtd: partition "bar" doesn't end on an erase/write block -- force read-only ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mtd: fix calculating partition end address 2020-03-09 15:08 ` Rafał Miłecki @ 2020-03-09 15:22 ` Miquel Raynal 2020-05-02 18:04 ` Miquel Raynal 0 siblings, 1 reply; 12+ messages in thread From: Miquel Raynal @ 2020-03-09 15:22 UTC (permalink / raw) To: Rafał Miłecki Cc: Richard Weinberger, Rafał Miłecki, linux-mtd, Vignesh Raghavendra, Boris Brezillon Hi Rafał, Rafał Miłecki <rafal@milecki.pl> wrote on Mon, 9 Mar 2020 16:08:12 +0100: > On 09.03.2020 15:22, Miquel Raynal wrote: > > Rafał Miłecki <rafal@milecki.pl> wrote on Mon, 09 Mar 2020 15:19:10 > > +0100: > > > >> On 2020-03-09 15:04, Miquel Raynal wrote: > >>> Rafał Miłecki <zajec5@gmail.com> wrote on Mon, 9 Mar 2020 08:44:45 > >>> +0100: > >>> >>>> From: Rafał Miłecki <rafal@milecki.pl> > >>>>>> This fixes check for partitions that don't start at beginning of their > >>>> parents. Missing partition's offset in formula could result in forcing > >>>> read-only incorrectly. > >>>>>> Fixes: 6750f61a13a0 ("mtd: improve calculating partition boundaries >> when checking for alignment") > >>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > >>>> --- > >>>> drivers/mtd/mtdpart.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>>> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > >>>> index 7328c066c5ba..c683b432cc5e 100644 > >>>> --- a/drivers/mtd/mtdpart.c > >>>> +++ b/drivers/mtd/mtdpart.c > >>>> @@ -524,7 +524,7 @@ static struct mtd_part *allocate_partition(struct >> mtd_info *parent, > >>>> part->name); > >>>> } > >>>>>> - tmp = part_absolute_offset(parent) + slave->mtd.size; > >>>> + tmp = part_absolute_offset(parent) + slave->offset + >> slave->mtd.size; > >>> > >>> I think you are doing the change at the wrong place, if you want to > >>> check where the partition *starts* you should do it a few lines above. > >>> But I think the check should be here as well, probably. > >> > >> The check where the partition *starts* is OK and I don't mean to change > >> it. The bug is about calculating absolute *end* address of partition. > > > > Can you detail a little bit then? Because I don't see the issue anymore > > even though I am convinced something is wrong here :) > > Please consider following partitions layout: > * bcm47xxsflash > ├─ boot 0x000000000000-0x000000040000 > └┬ firmware 0x000000040000-0x000001000000 > ├─ linux 0x00000000001c-0x00000018f800 > └┬ container 0x00000018f800-0x000000fc0000 > ├─ foo 0x000000000000-0x000000630800 > └─ bar 0x000000630800-0x000000e30800 (size: 0x800000) > > > Existing (correct) *start* calculation: > bar start: 0 + 0x040000 + 0x18f800 + 0x630800 = 0x800000 > > Existing (wrong) end calculation: > bar end: 0 + 0x040000 + 0x18f800 + 0x800000 = 0x9cf800 > > Fixed (correct) end calculation: > bar end: 0 + 0x040000 + 0x18f800 + 0x630800 + 0x800000 = 0x1000000 Ok I get it! I think mentioning "partitions that don't start at beginning of their parents", despite being true, was misleading to me as I understood "leaving extra space with the start of their parent". I suppose you also have the issue with "container" too? Anyway, I think the fix is fine. A better formulation for the commit log would be welcome :) (maybe adding this example is a good idea!) Thanks, Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mtd: fix calculating partition end address 2020-03-09 15:22 ` Miquel Raynal @ 2020-05-02 18:04 ` Miquel Raynal 2020-05-02 19:36 ` Rafał Miłecki 0 siblings, 1 reply; 12+ messages in thread From: Miquel Raynal @ 2020-05-02 18:04 UTC (permalink / raw) To: Rafał Miłecki Cc: Richard Weinberger, Rafał Miłecki, linux-mtd, Vignesh Raghavendra, Boris Brezillon Hi Rafal, Miquel Raynal <miquel.raynal@bootlin.com> wrote on Mon, 9 Mar 2020 16:22:23 +0100: > Hi Rafał, > > Rafał Miłecki <rafal@milecki.pl> wrote on Mon, 9 Mar 2020 16:08:12 > +0100: > > > On 09.03.2020 15:22, Miquel Raynal wrote: > > > Rafał Miłecki <rafal@milecki.pl> wrote on Mon, 09 Mar 2020 15:19:10 > > > +0100: > > > > > >> On 2020-03-09 15:04, Miquel Raynal wrote: > > >>> Rafał Miłecki <zajec5@gmail.com> wrote on Mon, 9 Mar 2020 08:44:45 > > >>> +0100: > > >>> >>>> From: Rafał Miłecki <rafal@milecki.pl> > > >>>>>> This fixes check for partitions that don't start at beginning of their > > >>>> parents. Missing partition's offset in formula could result in forcing > > >>>> read-only incorrectly. > > >>>>>> Fixes: 6750f61a13a0 ("mtd: improve calculating partition boundaries >> when checking for alignment") > > >>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > > >>>> --- > > >>>> drivers/mtd/mtdpart.c | 2 +- > > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > > >>>>>> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > > >>>> index 7328c066c5ba..c683b432cc5e 100644 > > >>>> --- a/drivers/mtd/mtdpart.c > > >>>> +++ b/drivers/mtd/mtdpart.c > > >>>> @@ -524,7 +524,7 @@ static struct mtd_part *allocate_partition(struct >> mtd_info *parent, > > >>>> part->name); > > >>>> } > > >>>>>> - tmp = part_absolute_offset(parent) + slave->mtd.size; > > >>>> + tmp = part_absolute_offset(parent) + slave->offset + >> slave->mtd.size; > > >>> > > >>> I think you are doing the change at the wrong place, if you want to > > >>> check where the partition *starts* you should do it a few lines above. > > >>> But I think the check should be here as well, probably. > > >> > > >> The check where the partition *starts* is OK and I don't mean to change > > >> it. The bug is about calculating absolute *end* address of partition. > > > > > > Can you detail a little bit then? Because I don't see the issue anymore > > > even though I am convinced something is wrong here :) > > > > Please consider following partitions layout: > > * bcm47xxsflash > > ├─ boot 0x000000000000-0x000000040000 > > └┬ firmware 0x000000040000-0x000001000000 > > ├─ linux 0x00000000001c-0x00000018f800 > > └┬ container 0x00000018f800-0x000000fc0000 > > ├─ foo 0x000000000000-0x000000630800 > > └─ bar 0x000000630800-0x000000e30800 (size: 0x800000) > > > > > > Existing (correct) *start* calculation: > > bar start: 0 + 0x040000 + 0x18f800 + 0x630800 = 0x800000 > > > > Existing (wrong) end calculation: > > bar end: 0 + 0x040000 + 0x18f800 + 0x800000 = 0x9cf800 > > > > Fixed (correct) end calculation: > > bar end: 0 + 0x040000 + 0x18f800 + 0x630800 + 0x800000 = 0x1000000 > > Ok I get it! I think mentioning "partitions that don't start at > beginning of their parents", despite being true, was misleading to me > as I understood "leaving extra space with the start of their parent". > > I suppose you also have the issue with "container" too? > > Anyway, I think the fix is fine. A better formulation for the commit > log would be welcome :) (maybe adding this example is a good idea!) I don't remember having applied this fix yet, would you mind resending this patch with an enhanced commit log (your example was a good one I think). Thanks, Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mtd: fix calculating partition end address 2020-05-02 18:04 ` Miquel Raynal @ 2020-05-02 19:36 ` Rafał Miłecki 2020-05-02 19:53 ` Miquel Raynal 0 siblings, 1 reply; 12+ messages in thread From: Rafał Miłecki @ 2020-05-02 19:36 UTC (permalink / raw) To: Miquel Raynal Cc: Richard Weinberger, Rafał Miłecki, linux-mtd, Vignesh Raghavendra, Boris Brezillon On 2020-05-02 20:04, Miquel Raynal wrote: > Miquel Raynal <miquel.raynal@bootlin.com> wrote on Mon, 9 Mar 2020 > 16:22:23 +0100: > >> Hi Rafał, >> >> Rafał Miłecki <rafal@milecki.pl> wrote on Mon, 9 Mar 2020 16:08:12 >> +0100: >> >> > On 09.03.2020 15:22, Miquel Raynal wrote: >> > > Rafał Miłecki <rafal@milecki.pl> wrote on Mon, 09 Mar 2020 15:19:10 >> > > +0100: >> > > >> > >> On 2020-03-09 15:04, Miquel Raynal wrote: >> > >>> Rafał Miłecki <zajec5@gmail.com> wrote on Mon, 9 Mar 2020 08:44:45 >> > >>> +0100: >> > >>> >>>> From: Rafał Miłecki <rafal@milecki.pl> >> > >>>>>> This fixes check for partitions that don't start at beginning of their >> > >>>> parents. Missing partition's offset in formula could result in forcing >> > >>>> read-only incorrectly. >> > >>>>>> Fixes: 6750f61a13a0 ("mtd: improve calculating partition boundaries >> when checking for alignment") >> > >>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> > >>>> --- >> > >>>> drivers/mtd/mtdpart.c | 2 +- >> > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >> > >>>>>> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c >> > >>>> index 7328c066c5ba..c683b432cc5e 100644 >> > >>>> --- a/drivers/mtd/mtdpart.c >> > >>>> +++ b/drivers/mtd/mtdpart.c >> > >>>> @@ -524,7 +524,7 @@ static struct mtd_part *allocate_partition(struct >> mtd_info *parent, >> > >>>> part->name); >> > >>>> } >> > >>>>>> - tmp = part_absolute_offset(parent) + slave->mtd.size; >> > >>>> + tmp = part_absolute_offset(parent) + slave->offset + >> slave->mtd.size; >> > >>> >> > >>> I think you are doing the change at the wrong place, if you want to >> > >>> check where the partition *starts* you should do it a few lines above. >> > >>> But I think the check should be here as well, probably. >> > >> >> > >> The check where the partition *starts* is OK and I don't mean to change >> > >> it. The bug is about calculating absolute *end* address of partition. >> > > >> > > Can you detail a little bit then? Because I don't see the issue anymore >> > > even though I am convinced something is wrong here :) >> > >> > Please consider following partitions layout: >> > * bcm47xxsflash >> > ├─ boot 0x000000000000-0x000000040000 >> > └┬ firmware 0x000000040000-0x000001000000 >> > ├─ linux 0x00000000001c-0x00000018f800 >> > └┬ container 0x00000018f800-0x000000fc0000 >> > ├─ foo 0x000000000000-0x000000630800 >> > └─ bar 0x000000630800-0x000000e30800 (size: 0x800000) >> > >> > >> > Existing (correct) *start* calculation: >> > bar start: 0 + 0x040000 + 0x18f800 + 0x630800 = 0x800000 >> > >> > Existing (wrong) end calculation: >> > bar end: 0 + 0x040000 + 0x18f800 + 0x800000 = 0x9cf800 >> > >> > Fixed (correct) end calculation: >> > bar end: 0 + 0x040000 + 0x18f800 + 0x630800 + 0x800000 = 0x1000000 >> >> Ok I get it! I think mentioning "partitions that don't start at >> beginning of their parents", despite being true, was misleading to me >> as I understood "leaving extra space with the start of their parent". >> >> I suppose you also have the issue with "container" too? >> >> Anyway, I think the fix is fine. A better formulation for the commit >> log would be welcome :) (maybe adding this example is a good idea!) > > I don't remember having applied this fix yet, would you mind resending > this patch with an enhanced commit log (your example was a good one I > think). It's not needed anymore. Please check discussion we got: -------- Original Message -------- Subject: Re: [PATCH] mtd: fix calculating partition end address Date: 2020-03-24 23:11 > > I would like to apply your fix this week, do you think you can rebase > > and resend soon? > > It's not needed anymore as you fixed this bug in your commit reworking > partitions. Great! ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mtd: fix calculating partition end address 2020-05-02 19:36 ` Rafał Miłecki @ 2020-05-02 19:53 ` Miquel Raynal 0 siblings, 0 replies; 12+ messages in thread From: Miquel Raynal @ 2020-05-02 19:53 UTC (permalink / raw) To: Rafał Miłecki Cc: Richard Weinberger, Rafał Miłecki, linux-mtd, Vignesh Raghavendra, Boris Brezillon Hi Rafał, Rafał Miłecki <rafal@milecki.pl> wrote on Sat, 02 May 2020 21:36:24 +0200: > On 2020-05-02 20:04, Miquel Raynal wrote: > > Miquel Raynal <miquel.raynal@bootlin.com> wrote on Mon, 9 Mar 2020 > > 16:22:23 +0100: > > > >> Hi Rafał, > >> >> Rafał Miłecki <rafal@milecki.pl> wrote on Mon, 9 Mar 2020 16:08:12 > >> +0100: > >> >> > On 09.03.2020 15:22, Miquel Raynal wrote: > >> > > Rafał Miłecki <rafal@milecki.pl> wrote on Mon, 09 Mar 2020 15:19:10 > >> > > +0100: > >> > > > >> > >> On 2020-03-09 15:04, Miquel Raynal wrote: > >> > >>> Rafał Miłecki <zajec5@gmail.com> wrote on Mon, 9 Mar 2020 08:44:45 > >> > >>> +0100: > >> > >>> >>>> From: Rafał Miłecki <rafal@milecki.pl> > >> > >>>>>> This fixes check for partitions that don't start at beginning of their > >> > >>>> parents. Missing partition's offset in formula could result in forcing > >> > >>>> read-only incorrectly. > >> > >>>>>> Fixes: 6750f61a13a0 ("mtd: improve calculating partition boundaries >> when checking for alignment") > >> > >>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > >> > >>>> --- > >> > >>>> drivers/mtd/mtdpart.c | 2 +- > >> > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >>>>>> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > >> > >>>> index 7328c066c5ba..c683b432cc5e 100644 > >> > >>>> --- a/drivers/mtd/mtdpart.c > >> > >>>> +++ b/drivers/mtd/mtdpart.c > >> > >>>> @@ -524,7 +524,7 @@ static struct mtd_part *allocate_partition(struct >> mtd_info *parent, > >> > >>>> part->name); > >> > >>>> } > >> > >>>>>> - tmp = part_absolute_offset(parent) + slave->mtd.size; > >> > >>>> + tmp = part_absolute_offset(parent) + slave->offset + >> slave->mtd.size; > >> > >>> > >> > >>> I think you are doing the change at the wrong place, if you want to > >> > >>> check where the partition *starts* you should do it a few lines above. > >> > >>> But I think the check should be here as well, probably. > >> > >> > >> > >> The check where the partition *starts* is OK and I don't mean to change > >> > >> it. The bug is about calculating absolute *end* address of partition. > >> > > > >> > > Can you detail a little bit then? Because I don't see the issue anymore > >> > > even though I am convinced something is wrong here :) > >> > > >> > Please consider following partitions layout: > >> > * bcm47xxsflash > >> > ├─ boot 0x000000000000-0x000000040000 > >> > └┬ firmware 0x000000040000-0x000001000000 > >> > ├─ linux 0x00000000001c-0x00000018f800 > >> > └┬ container 0x00000018f800-0x000000fc0000 > >> > ├─ foo 0x000000000000-0x000000630800 > >> > └─ bar 0x000000630800-0x000000e30800 (size: 0x800000) > >> > > >> > > >> > Existing (correct) *start* calculation: > >> > bar start: 0 + 0x040000 + 0x18f800 + 0x630800 = 0x800000 > >> > > >> > Existing (wrong) end calculation: > >> > bar end: 0 + 0x040000 + 0x18f800 + 0x800000 = 0x9cf800 > >> > > >> > Fixed (correct) end calculation: > >> > bar end: 0 + 0x040000 + 0x18f800 + 0x630800 + 0x800000 = 0x1000000 > >> >> Ok I get it! I think mentioning "partitions that don't start at > >> beginning of their parents", despite being true, was misleading to me > >> as I understood "leaving extra space with the start of their parent". > >> >> I suppose you also have the issue with "container" too? > >> >> Anyway, I think the fix is fine. A better formulation for the commit > >> log would be welcome :) (maybe adding this example is a good idea!) > > > > I don't remember having applied this fix yet, would you mind resending > > this patch with an enhanced commit log (your example was a good one I > > think). > > It's not needed anymore. Please check discussion we got: > > -------- Original Message -------- > Subject: Re: [PATCH] mtd: fix calculating partition end address > Date: 2020-03-24 23:11 > > > > I would like to apply your fix this week, do you think you can rebase > > > and resend soon? > > > > It's not needed anymore as you fixed this bug in your commit reworking > > partitions. > > Great! Yeah sorry about that, I got confused while listing remaining patches :) ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mtd: fix calculating partition end address 2020-03-09 14:19 ` Rafał Miłecki 2020-03-09 14:22 ` Miquel Raynal @ 2020-03-24 21:58 ` Miquel Raynal 2020-03-24 22:06 ` Rafał Miłecki 1 sibling, 1 reply; 12+ messages in thread From: Miquel Raynal @ 2020-03-24 21:58 UTC (permalink / raw) To: Rafał Miłecki Cc: Richard Weinberger, Rafał Miłecki, linux-mtd, Vignesh Raghavendra, Boris Brezillon Hi Rafał, Rafał Miłecki <rafal@milecki.pl> wrote on Mon, 09 Mar 2020 15:19:10 +0100: > On 2020-03-09 15:04, Miquel Raynal wrote: > > Rafał Miłecki <zajec5@gmail.com> wrote on Mon, 9 Mar 2020 08:44:45 > > +0100: > > > >> From: Rafał Miłecki <rafal@milecki.pl> > >> >> This fixes check for partitions that don't start at beginning of their > >> parents. Missing partition's offset in formula could result in forcing > >> read-only incorrectly. > >> >> Fixes: 6750f61a13a0 ("mtd: improve calculating partition boundaries >> when checking for alignment") > >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > >> --- > >> drivers/mtd/mtdpart.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> >> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > >> index 7328c066c5ba..c683b432cc5e 100644 > >> --- a/drivers/mtd/mtdpart.c > >> +++ b/drivers/mtd/mtdpart.c > >> @@ -524,7 +524,7 @@ static struct mtd_part *allocate_partition(struct >> mtd_info *parent, > >> part->name); > >> } > >> >> - tmp = part_absolute_offset(parent) + slave->mtd.size; > >> + tmp = part_absolute_offset(parent) + slave->offset + >> slave->mtd.size; > > > > I think you are doing the change at the wrong place, if you want to > > check where the partition *starts* you should do it a few lines above. > > But I think the check should be here as well, probably. > > The check where the partition *starts* is OK and I don't mean to change > it. The bug is about calculating absolute *end* address of partition. > > > > Anyway, I just applied on my local tree a patch rewriting a bit the > > partitioning scheme, could you please rebase on top of today's > > mtd/next and resend this patch updated? > > > > Here is the change that I've done at this place: > > - tmp = part_absolute_offset(parent) + slave->mtd.size; > > + tmp = mtd_get_master_ofs(child, 0) + child->size; > > I'll give it a try. I would like to apply your fix this week, do you think you can rebase and resend soon? Thanks, Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mtd: fix calculating partition end address 2020-03-24 21:58 ` Miquel Raynal @ 2020-03-24 22:06 ` Rafał Miłecki 2020-03-24 22:11 ` Miquel Raynal 0 siblings, 1 reply; 12+ messages in thread From: Rafał Miłecki @ 2020-03-24 22:06 UTC (permalink / raw) To: Miquel Raynal Cc: Richard Weinberger, Rafał Miłecki, linux-mtd, Vignesh Raghavendra, Boris Brezillon Hey, On 2020-03-24 22:58, Miquel Raynal wrote: > Rafał Miłecki <rafal@milecki.pl> wrote on Mon, 09 Mar 2020 15:19:10 > +0100: > >> On 2020-03-09 15:04, Miquel Raynal wrote: >> > Rafał Miłecki <zajec5@gmail.com> wrote on Mon, 9 Mar 2020 08:44:45 >> > +0100: >> > >> >> From: Rafał Miłecki <rafal@milecki.pl> >> >> >> This fixes check for partitions that don't start at beginning of their >> >> parents. Missing partition's offset in formula could result in forcing >> >> read-only incorrectly. >> >> >> Fixes: 6750f61a13a0 ("mtd: improve calculating partition boundaries >> when checking for alignment") >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> >> --- >> >> drivers/mtd/mtdpart.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c >> >> index 7328c066c5ba..c683b432cc5e 100644 >> >> --- a/drivers/mtd/mtdpart.c >> >> +++ b/drivers/mtd/mtdpart.c >> >> @@ -524,7 +524,7 @@ static struct mtd_part *allocate_partition(struct >> mtd_info *parent, >> >> part->name); >> >> } >> >> >> - tmp = part_absolute_offset(parent) + slave->mtd.size; >> >> + tmp = part_absolute_offset(parent) + slave->offset + >> slave->mtd.size; >> > >> > I think you are doing the change at the wrong place, if you want to >> > check where the partition *starts* you should do it a few lines above. >> > But I think the check should be here as well, probably. >> >> The check where the partition *starts* is OK and I don't mean to >> change >> it. The bug is about calculating absolute *end* address of partition. >> >> >> > Anyway, I just applied on my local tree a patch rewriting a bit the >> > partitioning scheme, could you please rebase on top of today's >> > mtd/next and resend this patch updated? >> > >> > Here is the change that I've done at this place: >> > - tmp = part_absolute_offset(parent) + slave->mtd.size; >> > + tmp = mtd_get_master_ofs(child, 0) + child->size; >> >> I'll give it a try. > > I would like to apply your fix this week, do you think you can rebase > and resend soon? It's not needed anymore as you fixed this bug in your commit reworking partitions. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mtd: fix calculating partition end address 2020-03-24 22:06 ` Rafał Miłecki @ 2020-03-24 22:11 ` Miquel Raynal 0 siblings, 0 replies; 12+ messages in thread From: Miquel Raynal @ 2020-03-24 22:11 UTC (permalink / raw) To: Rafał Miłecki Cc: Richard Weinberger, Rafał Miłecki, linux-mtd, Vignesh Raghavendra, Boris Brezillon Hi Rafał, Rafał Miłecki <rafal@milecki.pl> wrote on Tue, 24 Mar 2020 23:06:05 +0100: > Hey, > > On 2020-03-24 22:58, Miquel Raynal wrote: > > Rafał Miłecki <rafal@milecki.pl> wrote on Mon, 09 Mar 2020 15:19:10 > > +0100: > > > >> On 2020-03-09 15:04, Miquel Raynal wrote: > >> > Rafał Miłecki <zajec5@gmail.com> wrote on Mon, 9 Mar 2020 08:44:45 > >> > +0100: > >> > > >> >> From: Rafał Miłecki <rafal@milecki.pl> > >> >> >> This fixes check for partitions that don't start at beginning of their > >> >> parents. Missing partition's offset in formula could result in forcing > >> >> read-only incorrectly. > >> >> >> Fixes: 6750f61a13a0 ("mtd: improve calculating partition boundaries >> when checking for alignment") > >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > >> >> --- > >> >> drivers/mtd/mtdpart.c | 2 +- > >> >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> >> >> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > >> >> index 7328c066c5ba..c683b432cc5e 100644 > >> >> --- a/drivers/mtd/mtdpart.c > >> >> +++ b/drivers/mtd/mtdpart.c > >> >> @@ -524,7 +524,7 @@ static struct mtd_part *allocate_partition(struct >> mtd_info *parent, > >> >> part->name); > >> >> } > >> >> >> - tmp = part_absolute_offset(parent) + slave->mtd.size; > >> >> + tmp = part_absolute_offset(parent) + slave->offset + >> slave->mtd.size; > >> > > >> > I think you are doing the change at the wrong place, if you want to > >> > check where the partition *starts* you should do it a few lines above. > >> > But I think the check should be here as well, probably. > >> >> The check where the partition *starts* is OK and I don't mean to >> change > >> it. The bug is about calculating absolute *end* address of partition. > >> >> >> > Anyway, I just applied on my local tree a patch rewriting a bit the > >> > partitioning scheme, could you please rebase on top of today's > >> > mtd/next and resend this patch updated? > >> > > >> > Here is the change that I've done at this place: > >> > - tmp = part_absolute_offset(parent) + slave->mtd.size; > >> > + tmp = mtd_get_master_ofs(child, 0) + child->size; > >> >> I'll give it a try. > > > > I would like to apply your fix this week, do you think you can rebase > > and resend soon? > > It's not needed anymore as you fixed this bug in your commit reworking > partitions. Great! Thanks for the info Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-05-02 19:54 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-09 7:44 [PATCH] mtd: fix calculating partition end address Rafał Miłecki 2020-03-09 14:04 ` Miquel Raynal 2020-03-09 14:19 ` Rafał Miłecki 2020-03-09 14:22 ` Miquel Raynal 2020-03-09 15:08 ` Rafał Miłecki 2020-03-09 15:22 ` Miquel Raynal 2020-05-02 18:04 ` Miquel Raynal 2020-05-02 19:36 ` Rafał Miłecki 2020-05-02 19:53 ` Miquel Raynal 2020-03-24 21:58 ` Miquel Raynal 2020-03-24 22:06 ` Rafał Miłecki 2020-03-24 22:11 ` Miquel Raynal
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).