Skip to contentSkip to conversation

mtd: fix calculating partition end address

PATCH In review Rafał Miłecki 3 participants 12 messages Mar 9, 2020 – May 2, 2020 Memory technology devices (MTD)

lore
Share & tools
lore

Rafał Miłecki

author
#0 lore raw

PATCH 1/1 mtd: fix calculating partition end address

From: Rafał Miłecki <[email protected]>

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 <[email protected]>
---
 drivers/mtd/mtdpart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mtd/mtdpart.c +1 -1
index 7328c066c5ba..c683b432cc5e 100644
@@ -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;
Signature · 6 lines
2.25.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
maintainer
#1 lore raw

Hi Rafał,

Rafał Miłecki <[email protected]> wrote on Mon, 9 Mar 2020 08:44:45 +0100:

Quoted 17 lines

From: Rafał Miłecki <[email protected]>

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 <[email protected]>
---
 drivers/mtd/mtdpart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mtd/mtdpart.c +0 -0
index 7328c066c5ba..c683b432cc5e 100644

drivers/mtd/mtdpart.c static struct mtd_part *allocate_partition(struct mtd_info *parent, · line 527

Quoted 5 lines
drivers/mtd/mtdpart.c +1 -1
@@ -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/

#2 lore raw
Quoted 24 lines · Miquel Raynal

Rafał Miłecki <[email protected]> wrote on Mon, 9 Mar 2020 08:44:45 +0100:

Quoted 21 lines

From: Rafał Miłecki <[email protected]>

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 <[email protected]>
---
 drivers/mtd/mtdpart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mtd/mtdpart.c +0 -0
index 7328c066c5ba..c683b432cc5e 100644
@@ -524,7 +524,7 @@ static struct mtd_part *allocate_partition(struct 
 mtd_info *parent,
 			part->name);
 	}
Quoted 7 lines

drivers/mtd/mtdpart.c static struct mtd_part *allocate_partition(struct · line 529

Quoted 3 lines
drivers/mtd/mtdpart.c +1 -1
-	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.

Quoted 7 lines

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/

maintainer
#3 lore raw

Hi Rafał,

Rafał Miłecki <[email protected]> wrote on Mon, 09 Mar 2020 15:19:10 +0100:

Quoted 27 lines · Miquel Raynal
Quoted 19 lines

Rafał Miłecki <[email protected]> wrote on Mon, 9 Mar 2020 08:44:45 +0100:

Quoted 16 lines

From: Rafał Miłecki <[email protected]>

Quoted 1 line
Quoted 1 line

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.

Quoted 1 line
Quoted 1 line
Fixes 6750f61a13a0 ("mtd: improve calculating partition boundaries >> when checking for alignment")
Signed-off-by Rafał Miłecki <[email protected]>
---
 drivers/mtd/mtdpart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 1 line

drivers/mtd/mtdpart.c

Quoted 1 line
drivers/mtd/mtdpart.c +0 -0

index 7328c066c5ba..c683b432cc5e 100644

drivers/mtd/mtdpart.c +0 -0
@@ -524,7 +524,7 @@ static struct mtd_part *allocate_partition(struct >> mtd_info *parent,
 			part->name);
 	}
Quoted 6 lines

drivers/mtd/mtdpart.c static struct mtd_part *allocate_partition(struct >> mtd_info *parent, · line 527

Quoted 2 lines
drivers/mtd/mtdpart.c +1 -0
 >> -	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.

Quoted 2 lines

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

Quoted 11 lines
Quoted 9 lines

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/

#4 lore raw
Quoted 30 lines · Miquel Raynal

Rafał Miłecki <[email protected]> wrote on Mon, 09 Mar 2020 15:19:10 +0100:

Quoted 27 lines · Miquel Raynal
Quoted 25 lines

Rafał Miłecki <[email protected]> wrote on Mon, 9 Mar 2020 08:44:45 +0100:

Quoted 14 lines

From: Rafał Miłecki <[email protected]>

Quoted 1 line
Quoted 1 line

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.

Quoted 1 line
Quoted 1 line
Fixes 6750f61a13a0 ("mtd: improve calculating partition boundaries >> when checking for alignment")
Signed-off-by Rafał Miłecki <[email protected]>
---
  drivers/mtd/mtdpart.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 1 line

drivers/mtd/mtdpart.c

Quoted 1 line
drivers/mtd/mtdpart.c +0 -0

index 7328c066c5ba..c683b432cc5e 100644

drivers/mtd/mtdpart.c static struct mtd_part *allocate_partition(struct >> mtd_info *parent, · line 527

Quoted 4 lines
drivers/mtd/mtdpart.c +1 -0
@@ -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.

Quoted 5 lines
Quoted 2 lines

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/

maintainer
#5 lore raw

Hi Rafał,

Rafał Miłecki <[email protected]> wrote on Mon, 9 Mar 2020 16:08:12 +0100:

Quoted 46 lines · Miquel Raynal
Quoted 29 lines

Rafał Miłecki <[email protected]> wrote on Mon, 09 Mar 2020 15:19:10 +0100:

Quoted 26 lines · Miquel Raynal
Quoted 24 lines

Rafał Miłecki <[email protected]> wrote on Mon, 9 Mar 2020 08:44:45 +0100:

Quoted 14 lines
Quoted 2 lines
Quoted 2 lines
Quoted 1 line

From: Rafał Miłecki <[email protected]>

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.

Quoted 1 line
Quoted 1 line
Fixes 6750f61a13a0 ("mtd: improve calculating partition boundaries >> when checking for alignment")
Signed-off-by Rafał Miłecki <[email protected]>
---
  drivers/mtd/mtdpart.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 1 line

drivers/mtd/mtdpart.c

Quoted 1 line
drivers/mtd/mtdpart.c +0 -0

index 7328c066c5ba..c683b432cc5e 100644

drivers/mtd/mtdpart.c static struct mtd_part *allocate_partition(struct >> mtd_info *parent, · line 527

Quoted 4 lines
drivers/mtd/mtdpart.c +1 -0
@@ -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.

Quoted 5 lines
Quoted 2 lines

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)
Quoted 8 lines

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/

maintainer
#6 lore raw

Hi Rafal,

Miquel Raynal <[email protected]> wrote on Mon, 9 Mar 2020 16:22:23 +0100:

Quoted 60 lines

Hi Rafał,

Rafał Miłecki <[email protected]> wrote on Mon, 9 Mar 2020 16:08:12 +0100:

Quoted 46 lines · Miquel Raynal
Quoted 29 lines

Rafał Miłecki <[email protected]> wrote on Mon, 09 Mar 2020 15:19:10 +0100:

Quoted 26 lines · Miquel Raynal
Quoted 24 lines

Rafał Miłecki <[email protected]> wrote on Mon, 9 Mar 2020 08:44:45 +0100:

Quoted 14 lines
Quoted 2 lines
Quoted 2 lines
Quoted 1 line

From: Rafał Miłecki <[email protected]>

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.

Quoted 1 line
Quoted 1 line
Fixes 6750f61a13a0 ("mtd: improve calculating partition boundaries >> when checking for alignment")
Signed-off-by Rafał Miłecki <[email protected]>
---
  drivers/mtd/mtdpart.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 1 line

drivers/mtd/mtdpart.c

Quoted 1 line
drivers/mtd/mtdpart.c +0 -0

index 7328c066c5ba..c683b432cc5e 100644

drivers/mtd/mtdpart.c static struct mtd_part *allocate_partition(struct >> mtd_info *parent, · line 527

Quoted 4 lines
drivers/mtd/mtdpart.c +1 -0
@@ -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.

Quoted 5 lines
Quoted 2 lines

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)
Quoted 8 lines

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

Quoted 8 lines

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/

#7 lore raw
Quoted 67 lines · Miquel Raynal

Miquel Raynal <[email protected]> wrote on Mon, 9 Mar 2020 16:22:23 +0100:

Quoted 64 lines

Hi Rafał,

Rafał Miłecki <[email protected]> wrote on Mon, 9 Mar 2020 16:08:12 +0100:

Quoted 46 lines · Miquel Raynal
Quoted 29 lines

Rafał Miłecki <[email protected]> wrote on Mon, 09 Mar 2020 15:19:10 +0100:

Quoted 26 lines · Miquel Raynal
Quoted 24 lines

Rafał Miłecki <[email protected]> wrote on Mon, 9 Mar 2020 08:44:45 +0100:

Quoted 14 lines
Quoted 2 lines
Quoted 2 lines
Quoted 1 line

From: Rafał Miłecki <[email protected]>

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.

Quoted 1 line
Quoted 1 line
Fixes 6750f61a13a0 ("mtd: improve calculating partition boundaries >> when checking for alignment")
Signed-off-by Rafał Miłecki <[email protected]>
---
  drivers/mtd/mtdpart.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 1 line

drivers/mtd/mtdpart.c

Quoted 1 line
drivers/mtd/mtdpart.c +0 -0

index 7328c066c5ba..c683b432cc5e 100644

drivers/mtd/mtdpart.c static struct mtd_part *allocate_partition(struct >> mtd_info *parent, · line 527

Quoted 4 lines
drivers/mtd/mtdpart.c +1 -0
@@ -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.

Quoted 5 lines
Quoted 2 lines

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)
Quoted 8 lines

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

Quoted 8 lines
Quoted 4 lines

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

Quoted 5 lines
Quoted 2 lines

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/

maintainer
#8 lore raw

Hi Rafał,

Rafał Miłecki <[email protected]> wrote on Sat, 02 May 2020 21:36:24 +0200:

Quoted 78 lines · Miquel Raynal
Quoted 63 lines

Miquel Raynal <[email protected]> wrote on Mon, 9 Mar 2020 16:22:23 +0100:

Quoted 60 lines

Hi Rafał,

Quoted 1 line
Quoted 1 line

Rafał Miłecki <[email protected]> wrote on Mon, 9 Mar 2020 16:08:12

+0100:

Quoted 46 lines
Quoted 30 lines
Quoted 1 line · Miquel Raynal

Rafał Miłecki <[email protected]> wrote on Mon, 09 Mar 2020 15:19:10 +0100:

Quoted 26 lines · Miquel Raynal
Quoted 24 lines

Rafał Miłecki <[email protected]> wrote on Mon, 9 Mar 2020 08:44:45 +0100:

Quoted 14 lines
Quoted 2 lines
Quoted 2 lines
Quoted 1 line

From: Rafał Miłecki <[email protected]>

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.

Quoted 1 line
Quoted 1 line
Fixes 6750f61a13a0 ("mtd: improve calculating partition boundaries >> when checking for alignment")
Signed-off-by Rafał Miłecki <[email protected]>
---
  drivers/mtd/mtdpart.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 1 line

drivers/mtd/mtdpart.c

Quoted 1 line
drivers/mtd/mtdpart.c +0 -0

index 7328c066c5ba..c683b432cc5e 100644

drivers/mtd/mtdpart.c static struct mtd_part *allocate_partition(struct >> mtd_info *parent, · line 527

Quoted 4 lines
drivers/mtd/mtdpart.c +1 -0
@@ -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.

Quoted 5 lines
Quoted 2 lines

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)
Quoted 9 lines

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

Quoted 1 line

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

Quoted 7 lines
Quoted 3 lines
Quoted 2 lines
Quoted 2 lines

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

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

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/

maintainer
#9 lore raw
In reply to Rafał Miłecki

Hi Rafał,

Rafał Miłecki <[email protected]> wrote on Mon, 09 Mar 2020 15:19:10 +0100:

Quoted 35 lines · Miquel Raynal
Quoted 19 lines

Rafał Miłecki <[email protected]> wrote on Mon, 9 Mar 2020 08:44:45 +0100:

Quoted 16 lines

From: Rafał Miłecki <[email protected]>

Quoted 1 line
Quoted 1 line

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.

Quoted 1 line
Quoted 1 line
Fixes 6750f61a13a0 ("mtd: improve calculating partition boundaries >> when checking for alignment")
Signed-off-by Rafał Miłecki <[email protected]>
---
 drivers/mtd/mtdpart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 1 line

drivers/mtd/mtdpart.c

Quoted 1 line
drivers/mtd/mtdpart.c +0 -0

index 7328c066c5ba..c683b432cc5e 100644

drivers/mtd/mtdpart.c +0 -0
@@ -524,7 +524,7 @@ static struct mtd_part *allocate_partition(struct >> mtd_info *parent,
 			part->name);
 	}
Quoted 6 lines

drivers/mtd/mtdpart.c static struct mtd_part *allocate_partition(struct >> mtd_info *parent, · line 527

Quoted 2 lines
drivers/mtd/mtdpart.c +1 -0
 >> -	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.

Quoted 4 lines

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?

Quoted 5 lines
Quoted 3 lines

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/

#10 lore raw

Hey,

Quoted 38 lines · Miquel Raynal

Rafał Miłecki <[email protected]> wrote on Mon, 09 Mar 2020 15:19:10 +0100:

Quoted 35 lines · Miquel Raynal
Quoted 19 lines

Rafał Miłecki <[email protected]> wrote on Mon, 9 Mar 2020 08:44:45 +0100:

Quoted 16 lines

From: Rafał Miłecki <[email protected]>

Quoted 1 line
Quoted 1 line

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.

Quoted 1 line
Quoted 1 line
Fixes 6750f61a13a0 ("mtd: improve calculating partition boundaries >> when checking for alignment")
Signed-off-by Rafał Miłecki <[email protected]>
---
 drivers/mtd/mtdpart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 1 line

drivers/mtd/mtdpart.c

Quoted 1 line
drivers/mtd/mtdpart.c +0 -0

index 7328c066c5ba..c683b432cc5e 100644

drivers/mtd/mtdpart.c +0 -0
@@ -524,7 +524,7 @@ static struct mtd_part *allocate_partition(struct >> mtd_info *parent,
 			part->name);
 	}
Quoted 6 lines

drivers/mtd/mtdpart.c static struct mtd_part *allocate_partition(struct >> mtd_info *parent, · line 527

Quoted 2 lines
drivers/mtd/mtdpart.c +1 -0
 >> -	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.

Quoted 4 lines

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?

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

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/

maintainer
#11 lore raw

Hi Rafał,

Rafał Miłecki <[email protected]> wrote on Tue, 24 Mar 2020 23:06:05 +0100:

Quoted 41 lines

Hey,

Quoted 38 lines · Miquel Raynal

Rafał Miłecki <[email protected]> wrote on Mon, 09 Mar 2020 15:19:10 +0100:

Quoted 35 lines · Miquel Raynal
Quoted 19 lines

Rafał Miłecki <[email protected]> wrote on Mon, 9 Mar 2020 08:44:45 +0100:

Quoted 16 lines

From: Rafał Miłecki <[email protected]>

Quoted 1 line
Quoted 1 line

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.

Quoted 1 line
Quoted 1 line
Fixes 6750f61a13a0 ("mtd: improve calculating partition boundaries >> when checking for alignment")
Signed-off-by Rafał Miłecki <[email protected]>
---
 drivers/mtd/mtdpart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 1 line

drivers/mtd/mtdpart.c

Quoted 1 line
drivers/mtd/mtdpart.c +0 -0

index 7328c066c5ba..c683b432cc5e 100644

drivers/mtd/mtdpart.c +0 -0
@@ -524,7 +524,7 @@ static struct mtd_part *allocate_partition(struct >> mtd_info *parent,
 			part->name);
 	}
Quoted 7 lines

drivers/mtd/mtdpart.c static struct mtd_part *allocate_partition(struct >> mtd_info *parent, · line 527

Quoted 2 lines
drivers/mtd/mtdpart.c +1 -0
 >> -	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.

Quoted 1 line

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.

Quoted 7 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

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;

Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

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/