Re: [PATCH] mtd: fix calculating partition end address

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi Rafał,

Rafał Miłecki <rafal@xxxxxxxxxx> wrote on Mon, 9 Mar 2020 16:08:12
+0100:

> On 09.03.2020 15:22, Miquel Raynal wrote:
> > Rafał Miłecki <rafal@xxxxxxxxxx> wrote on Mon, 09 Mar 2020 15:19:10
> > +0100:
> >   
> >> On 2020-03-09 15:04, Miquel Raynal wrote:  
> >>> Rafał Miłecki <zajec5@xxxxxxxxx> wrote on Mon,  9 Mar 2020 08:44:45
> >>> +0100:  
> >>>    >>>> From: Rafał Miłecki <rafal@xxxxxxxxxx>  
> >>>>>> 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@xxxxxxxxxx>
> >>>> ---
> >>>>   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/