| Message ID | 20170110221525.9216-1-zajec5@gmail.com |
|---|---|
| State | Accepted |
| Commit | b522d7b0ebe3539340c2a6d46d787ae3d33bcb92 |
| Delegated to: | Brian Norris |
| Headers | show |
On 10 January 2017 at 23:15, Rafał Miłecki <zajec5@gmail.com> wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > This change simplifies main parsing loop logic a bit. In future it may > be useful for moving TRX support to separated module / parser (if we > implement support for them at some point). > Finally parsing TRX at the end puts us in a better position as we have > better flash layout knowledge. It may be useful e.g. if it appears there > is more than 1 TRX partition. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> Marek: you were commenting on V1, what do you think about V2? Could you Ack it?
On 01/10/2017 11:15 PM, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > This change simplifies main parsing loop logic a bit. In future it may > be useful for moving TRX support to separated module / parser (if we > implement support for them at some point). > Finally parsing TRX at the end puts us in a better position as we have > better flash layout knowledge. It may be useful e.g. if it appears there > is more than 1 TRX partition. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > V2: Keep using bcm47xxpart_add_part in new parsing function > --- > drivers/mtd/bcm47xxpart.c | 121 ++++++++++++++++++++++++++++------------------ > 1 file changed, 74 insertions(+), 47 deletions(-) > > diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c > index 283ff7e..1093025 100644 > --- a/drivers/mtd/bcm47xxpart.c > +++ b/drivers/mtd/bcm47xxpart.c > @@ -83,6 +83,67 @@ static const char *bcm47xxpart_trx_data_part_name(struct mtd_info *master, > return "rootfs"; > } > > +static int bcm47xxpart_parse_trx(struct mtd_info *master, > + struct mtd_partition *trx, > + struct mtd_partition *parts, > + size_t parts_len) > +{ > + struct trx_header header; > + size_t bytes_read; > + int curr_part = 0; > + int i, err; > + > + if (parts_len < 3) { > + pr_warn("No enough space to add TRX partitions!\n"); > + return -ENOMEM; > + } > + > + err = mtd_read(master, trx->offset, sizeof(header), &bytes_read, > + (uint8_t *)&header); > + if (err && !mtd_is_bitflip(err)) { > + pr_err("mtd_read error while reading TRX header: %d\n", err); Minor nit -- Can we make this dev_err() ? > + return err; > + } > + > + i = 0; > + > + /* We have LZMA loader if offset[2] points to sth */ > + if (header.offset[2]) { > + bcm47xxpart_add_part(&parts[curr_part++], "loader", > + trx->offset + header.offset[i], 0); > + i++; > + } > + > + if (header.offset[i]) { > + bcm47xxpart_add_part(&parts[curr_part++], "linux", > + trx->offset + header.offset[i], 0); > + i++; > + } > + > + if (header.offset[i]) { > + size_t offset = trx->offset + header.offset[i]; > + const char *name = bcm47xxpart_trx_data_part_name(master, > + offset); > + > + bcm47xxpart_add_part(&parts[curr_part++], name, offset, 0); > + i++; > + } > + > + /* > + * Assume that every partition ends at the beginning of the one it is > + * followed by. > + */ > + for (i = 0; i < curr_part; i++) { > + u64 next_part_offset = (i < curr_part - 1) ? > + parts[i + 1].offset : > + trx->offset + trx->size; > + > + parts[i].size = next_part_offset - parts[i].offset; > + } > + > + return curr_part; > +} > + > static int bcm47xxpart_parse(struct mtd_info *master, > const struct mtd_partition **pparts, > struct mtd_part_parser_data *data) > @@ -93,9 +154,7 @@ static int bcm47xxpart_parse(struct mtd_info *master, > size_t bytes_read; > uint32_t offset; > uint32_t blocksize = master->erasesize; > - struct trx_header *trx; > int trx_part = -1; > - int last_trx_part = -1; > int possible_nvram_sizes[] = { 0x8000, 0xF000, 0x10000, }; > int err; > > @@ -182,54 +241,14 @@ static int bcm47xxpart_parse(struct mtd_info *master, > > /* TRX */ > if (buf[0x000 / 4] == TRX_MAGIC) { > - if (BCM47XXPART_MAX_PARTS - curr_part < 4) { > - pr_warn("Not enough partitions left to register trx, scanning stopped!\n"); > - break; > - } > - > - trx = (struct trx_header *)buf; > + struct trx_header *trx; > > trx_part = curr_part; > bcm47xxpart_add_part(&parts[curr_part++], "firmware", > offset, 0); > > - i = 0; > - /* We have LZMA loader if offset[2] points to sth */ > - if (trx->offset[2]) { > - bcm47xxpart_add_part(&parts[curr_part++], > - "loader", > - offset + trx->offset[i], > - 0); > - i++; > - } > - > - if (trx->offset[i]) { > - bcm47xxpart_add_part(&parts[curr_part++], > - "linux", > - offset + trx->offset[i], > - 0); > - i++; > - } > - > - /* > - * Pure rootfs size is known and can be calculated as: > - * trx->length - trx->offset[i]. We don't fill it as > - * we want to have jffs2 (overlay) in the same mtd. > - */ > - if (trx->offset[i]) { > - const char *name; > - > - name = bcm47xxpart_trx_data_part_name(master, offset + trx->offset[i]); > - bcm47xxpart_add_part(&parts[curr_part++], > - name, > - offset + trx->offset[i], > - 0); > - i++; > - } > - > - last_trx_part = curr_part - 1; > - > /* Jump to the end of TRX */ > + trx = (struct trx_header *)buf; > offset = roundup(offset + trx->length, blocksize); > /* Next loop iteration will increase the offset */ > offset -= blocksize; > @@ -307,9 +326,17 @@ static int bcm47xxpart_parse(struct mtd_info *master, > parts[i + 1].offset : master->size; > > parts[i].size = next_part_offset - parts[i].offset; > - if (i == last_trx_part && trx_part >= 0) > - parts[trx_part].size = next_part_offset - > - parts[trx_part].offset; > + } > + > + /* If there was TRX parse it now */ > + if (trx_part >= 0) { > + int num_parts; > + > + num_parts = bcm47xxpart_parse_trx(master, &parts[trx_part], > + parts + curr_part, > + BCM47XXPART_MAX_PARTS - curr_part); > + if (num_parts > 0) > + curr_part += num_parts; > } > > *pparts = parts; >
On 02/09/2017 05:54 PM, Rafał Miłecki wrote: > On 10 January 2017 at 23:15, Rafał Miłecki <zajec5@gmail.com> wrote: >> From: Rafał Miłecki <rafal@milecki.pl> >> >> This change simplifies main parsing loop logic a bit. In future it may >> be useful for moving TRX support to separated module / parser (if we >> implement support for them at some point). >> Finally parsing TRX at the end puts us in a better position as we have >> better flash layout knowledge. It may be useful e.g. if it appears there >> is more than 1 TRX partition. >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > > Marek: you were commenting on V1, what do you think about V2? Could you Ack it? > Minor nit on 1/2 , but otherwise the whole series is Acked-by: Marek Vasut <marek.vasut@gmail.com> I will not claim I understand the TRX parsing though.
On 2017-02-09 18:28, Marek Vasut wrote: > On 01/10/2017 11:15 PM, Rafał Miłecki wrote: >> From: Rafał Miłecki <rafal@milecki.pl> >> >> This change simplifies main parsing loop logic a bit. In future it may >> be useful for moving TRX support to separated module / parser (if we >> implement support for them at some point). >> Finally parsing TRX at the end puts us in a better position as we have >> better flash layout knowledge. It may be useful e.g. if it appears >> there >> is more than 1 TRX partition. >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> --- >> V2: Keep using bcm47xxpart_add_part in new parsing function >> --- >> drivers/mtd/bcm47xxpart.c | 121 >> ++++++++++++++++++++++++++++------------------ >> 1 file changed, 74 insertions(+), 47 deletions(-) >> >> diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c >> index 283ff7e..1093025 100644 >> --- a/drivers/mtd/bcm47xxpart.c >> +++ b/drivers/mtd/bcm47xxpart.c >> @@ -83,6 +83,67 @@ static const char >> *bcm47xxpart_trx_data_part_name(struct mtd_info *master, >> return "rootfs"; >> } >> >> +static int bcm47xxpart_parse_trx(struct mtd_info *master, >> + struct mtd_partition *trx, >> + struct mtd_partition *parts, >> + size_t parts_len) >> +{ >> + struct trx_header header; >> + size_t bytes_read; >> + int curr_part = 0; >> + int i, err; >> + >> + if (parts_len < 3) { >> + pr_warn("No enough space to add TRX partitions!\n"); >> + return -ENOMEM; >> + } >> + >> + err = mtd_read(master, trx->offset, sizeof(header), &bytes_read, >> + (uint8_t *)&header); >> + if (err && !mtd_is_bitflip(err)) { >> + pr_err("mtd_read error while reading TRX header: %d\n", err); > > Minor nit -- Can we make this dev_err() ? The problem is parsers receive struct mtd_info (as an argument) with has empty dev. I just tried dev_info(&master->dev, "TEST TEST TEST :)\n"); and got this: [ 0.491576] (null): TEST TEST TEST :) I guess that's the reason why parse_mtd_partitions and mtd_device_parse_register also use pr_* functions. So this should be improved at mtd core subsystem first, then we can use it in drivers as well.
On 02/09/2017 07:05 PM, Rafał Miłecki wrote: > On 2017-02-09 18:28, Marek Vasut wrote: >> On 01/10/2017 11:15 PM, Rafał Miłecki wrote: >>> From: Rafał Miłecki <rafal@milecki.pl> >>> >>> This change simplifies main parsing loop logic a bit. In future it may >>> be useful for moving TRX support to separated module / parser (if we >>> implement support for them at some point). >>> Finally parsing TRX at the end puts us in a better position as we have >>> better flash layout knowledge. It may be useful e.g. if it appears there >>> is more than 1 TRX partition. >>> >>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >>> --- >>> V2: Keep using bcm47xxpart_add_part in new parsing function >>> --- >>> drivers/mtd/bcm47xxpart.c | 121 >>> ++++++++++++++++++++++++++++------------------ >>> 1 file changed, 74 insertions(+), 47 deletions(-) >>> >>> diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c >>> index 283ff7e..1093025 100644 >>> --- a/drivers/mtd/bcm47xxpart.c >>> +++ b/drivers/mtd/bcm47xxpart.c >>> @@ -83,6 +83,67 @@ static const char >>> *bcm47xxpart_trx_data_part_name(struct mtd_info *master, >>> return "rootfs"; >>> } >>> >>> +static int bcm47xxpart_parse_trx(struct mtd_info *master, >>> + struct mtd_partition *trx, >>> + struct mtd_partition *parts, >>> + size_t parts_len) >>> +{ >>> + struct trx_header header; >>> + size_t bytes_read; >>> + int curr_part = 0; >>> + int i, err; >>> + >>> + if (parts_len < 3) { >>> + pr_warn("No enough space to add TRX partitions!\n"); >>> + return -ENOMEM; >>> + } >>> + >>> + err = mtd_read(master, trx->offset, sizeof(header), &bytes_read, >>> + (uint8_t *)&header); >>> + if (err && !mtd_is_bitflip(err)) { >>> + pr_err("mtd_read error while reading TRX header: %d\n", err); >> >> Minor nit -- Can we make this dev_err() ? > > The problem is parsers receive struct mtd_info (as an argument) with has > empty > dev. I just tried > dev_info(&master->dev, "TEST TEST TEST :)\n"); > and got this: > [ 0.491576] (null): TEST TEST TEST :) > > I guess that's the reason why parse_mtd_partitions and > mtd_device_parse_register > also use pr_* functions. > > So this should be improved at mtd core subsystem first, then we can use > it in > drivers as well. Isn't this patch fixing just that? [PATCH] mtd: Add partition device node to mtd partition devices
On Thu, 9 Feb 2017 19:21:03 +0100 Marek Vasut <marek.vasut@gmail.com> wrote: > On 02/09/2017 07:05 PM, Rafał Miłecki wrote: > > On 2017-02-09 18:28, Marek Vasut wrote: > >> On 01/10/2017 11:15 PM, Rafał Miłecki wrote: > >>> From: Rafał Miłecki <rafal@milecki.pl> > >>> > >>> This change simplifies main parsing loop logic a bit. In future it may > >>> be useful for moving TRX support to separated module / parser (if we > >>> implement support for them at some point). > >>> Finally parsing TRX at the end puts us in a better position as we have > >>> better flash layout knowledge. It may be useful e.g. if it appears there > >>> is more than 1 TRX partition. > >>> > >>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > >>> --- > >>> V2: Keep using bcm47xxpart_add_part in new parsing function > >>> --- > >>> drivers/mtd/bcm47xxpart.c | 121 > >>> ++++++++++++++++++++++++++++------------------ > >>> 1 file changed, 74 insertions(+), 47 deletions(-) > >>> > >>> diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c > >>> index 283ff7e..1093025 100644 > >>> --- a/drivers/mtd/bcm47xxpart.c > >>> +++ b/drivers/mtd/bcm47xxpart.c > >>> @@ -83,6 +83,67 @@ static const char > >>> *bcm47xxpart_trx_data_part_name(struct mtd_info *master, > >>> return "rootfs"; > >>> } > >>> > >>> +static int bcm47xxpart_parse_trx(struct mtd_info *master, > >>> + struct mtd_partition *trx, > >>> + struct mtd_partition *parts, > >>> + size_t parts_len) > >>> +{ > >>> + struct trx_header header; > >>> + size_t bytes_read; > >>> + int curr_part = 0; > >>> + int i, err; > >>> + > >>> + if (parts_len < 3) { > >>> + pr_warn("No enough space to add TRX partitions!\n"); > >>> + return -ENOMEM; > >>> + } > >>> + > >>> + err = mtd_read(master, trx->offset, sizeof(header), &bytes_read, > >>> + (uint8_t *)&header); > >>> + if (err && !mtd_is_bitflip(err)) { > >>> + pr_err("mtd_read error while reading TRX header: %d\n", err); > >> > >> Minor nit -- Can we make this dev_err() ? > > > > The problem is parsers receive struct mtd_info (as an argument) with has > > empty > > dev. I just tried > > dev_info(&master->dev, "TEST TEST TEST :)\n"); > > and got this: > > [ 0.491576] (null): TEST TEST TEST :) > > > > I guess that's the reason why parse_mtd_partitions and > > mtd_device_parse_register > > also use pr_* functions. > > > > So this should be improved at mtd core subsystem first, then we can use > > it in > > drivers as well. > > Isn't this patch fixing just that? > [PATCH] mtd: Add partition device node to mtd partition devices > No, this patch is only adding a pointer to a device_node object (DT node). To solve the problem, we need to have the master dev name set before we register its partitions, and this is currently only the case if you enable CONFIG_MTD_PARTITIONED_MASTER.
On 02/09/2017 08:11 PM, Boris Brezillon wrote: > On Thu, 9 Feb 2017 19:21:03 +0100 > Marek Vasut <marek.vasut@gmail.com> wrote: > >> On 02/09/2017 07:05 PM, Rafał Miłecki wrote: >>> On 2017-02-09 18:28, Marek Vasut wrote: >>>> On 01/10/2017 11:15 PM, Rafał Miłecki wrote: >>>>> From: Rafał Miłecki <rafal@milecki.pl> >>>>> >>>>> This change simplifies main parsing loop logic a bit. In future it may >>>>> be useful for moving TRX support to separated module / parser (if we >>>>> implement support for them at some point). >>>>> Finally parsing TRX at the end puts us in a better position as we have >>>>> better flash layout knowledge. It may be useful e.g. if it appears there >>>>> is more than 1 TRX partition. >>>>> >>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >>>>> --- >>>>> V2: Keep using bcm47xxpart_add_part in new parsing function >>>>> --- >>>>> drivers/mtd/bcm47xxpart.c | 121 >>>>> ++++++++++++++++++++++++++++------------------ >>>>> 1 file changed, 74 insertions(+), 47 deletions(-) >>>>> >>>>> diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c >>>>> index 283ff7e..1093025 100644 >>>>> --- a/drivers/mtd/bcm47xxpart.c >>>>> +++ b/drivers/mtd/bcm47xxpart.c >>>>> @@ -83,6 +83,67 @@ static const char >>>>> *bcm47xxpart_trx_data_part_name(struct mtd_info *master, >>>>> return "rootfs"; >>>>> } >>>>> >>>>> +static int bcm47xxpart_parse_trx(struct mtd_info *master, >>>>> + struct mtd_partition *trx, >>>>> + struct mtd_partition *parts, >>>>> + size_t parts_len) >>>>> +{ >>>>> + struct trx_header header; >>>>> + size_t bytes_read; >>>>> + int curr_part = 0; >>>>> + int i, err; >>>>> + >>>>> + if (parts_len < 3) { >>>>> + pr_warn("No enough space to add TRX partitions!\n"); >>>>> + return -ENOMEM; >>>>> + } >>>>> + >>>>> + err = mtd_read(master, trx->offset, sizeof(header), &bytes_read, >>>>> + (uint8_t *)&header); >>>>> + if (err && !mtd_is_bitflip(err)) { >>>>> + pr_err("mtd_read error while reading TRX header: %d\n", err); >>>> >>>> Minor nit -- Can we make this dev_err() ? >>> >>> The problem is parsers receive struct mtd_info (as an argument) with has >>> empty >>> dev. I just tried >>> dev_info(&master->dev, "TEST TEST TEST :)\n"); >>> and got this: >>> [ 0.491576] (null): TEST TEST TEST :) >>> >>> I guess that's the reason why parse_mtd_partitions and >>> mtd_device_parse_register >>> also use pr_* functions. >>> >>> So this should be improved at mtd core subsystem first, then we can use >>> it in >>> drivers as well. >> >> Isn't this patch fixing just that? >> [PATCH] mtd: Add partition device node to mtd partition devices >> > > > No, this patch is only adding a pointer to a device_node object (DT > node). > To solve the problem, we need to have the master dev name set before we > register its partitions, and this is currently only the case if you > enable CONFIG_MTD_PARTITIONED_MASTER. > Ah hmmmm , then leave the pr_err() there , this can be fixed later. Acked-by: Marek Vasut <marek.vasut@gmail.com>
On Tue, Jan 10, 2017 at 11:15:24PM +0100, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > This change simplifies main parsing loop logic a bit. In future it may > be useful for moving TRX support to separated module / parser (if we > implement support for them at some point). > Finally parsing TRX at the end puts us in a better position as we have > better flash layout knowledge. It may be useful e.g. if it appears there > is more than 1 TRX partition. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > V2: Keep using bcm47xxpart_add_part in new parsing function Applied both to l2-mtd.git
diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c index 283ff7e..1093025 100644 --- a/drivers/mtd/bcm47xxpart.c +++ b/drivers/mtd/bcm47xxpart.c @@ -83,6 +83,67 @@ static const char *bcm47xxpart_trx_data_part_name(struct mtd_info *master, return "rootfs"; } +static int bcm47xxpart_parse_trx(struct mtd_info *master, + struct mtd_partition *trx, + struct mtd_partition *parts, + size_t parts_len) +{ + struct trx_header header; + size_t bytes_read; + int curr_part = 0; + int i, err; + + if (parts_len < 3) { + pr_warn("No enough space to add TRX partitions!\n"); + return -ENOMEM; + } + + err = mtd_read(master, trx->offset, sizeof(header), &bytes_read, + (uint8_t *)&header); + if (err && !mtd_is_bitflip(err)) { + pr_err("mtd_read error while reading TRX header: %d\n", err); + return err; + } + + i = 0; + + /* We have LZMA loader if offset[2] points to sth */ + if (header.offset[2]) { + bcm47xxpart_add_part(&parts[curr_part++], "loader", + trx->offset + header.offset[i], 0); + i++; + } + + if (header.offset[i]) { + bcm47xxpart_add_part(&parts[curr_part++], "linux", + trx->offset + header.offset[i], 0); + i++; + } + + if (header.offset[i]) { + size_t offset = trx->offset + header.offset[i]; + const char *name = bcm47xxpart_trx_data_part_name(master, + offset); + + bcm47xxpart_add_part(&parts[curr_part++], name, offset, 0); + i++; + } + + /* + * Assume that every partition ends at the beginning of the one it is + * followed by. + */ + for (i = 0; i < curr_part; i++) { + u64 next_part_offset = (i < curr_part - 1) ? + parts[i + 1].offset : + trx->offset + trx->size; + + parts[i].size = next_part_offset - parts[i].offset; + } + + return curr_part; +} + static int bcm47xxpart_parse(struct mtd_info *master, const struct mtd_partition **pparts, struct mtd_part_parser_data *data) @@ -93,9 +154,7 @@ static int bcm47xxpart_parse(struct mtd_info *master, size_t bytes_read; uint32_t offset; uint32_t blocksize = master->erasesize; - struct trx_header *trx; int trx_part = -1; - int last_trx_part = -1; int possible_nvram_sizes[] = { 0x8000, 0xF000, 0x10000, }; int err; @@ -182,54 +241,14 @@ static int bcm47xxpart_parse(struct mtd_info *master, /* TRX */ if (buf[0x000 / 4] == TRX_MAGIC) { - if (BCM47XXPART_MAX_PARTS - curr_part < 4) { - pr_warn("Not enough partitions left to register trx, scanning stopped!\n"); - break; - } - - trx = (struct trx_header *)buf; + struct trx_header *trx; trx_part = curr_part; bcm47xxpart_add_part(&parts[curr_part++], "firmware", offset, 0); - i = 0; - /* We have LZMA loader if offset[2] points to sth */ - if (trx->offset[2]) { - bcm47xxpart_add_part(&parts[curr_part++], - "loader", - offset + trx->offset[i], - 0); - i++; - } - - if (trx->offset[i]) { - bcm47xxpart_add_part(&parts[curr_part++], - "linux", - offset + trx->offset[i], - 0); - i++; - } - - /* - * Pure rootfs size is known and can be calculated as: - * trx->length - trx->offset[i]. We don't fill it as - * we want to have jffs2 (overlay) in the same mtd. - */ - if (trx->offset[i]) { - const char *name; - - name = bcm47xxpart_trx_data_part_name(master, offset + trx->offset[i]); - bcm47xxpart_add_part(&parts[curr_part++], - name, - offset + trx->offset[i], - 0); - i++; - } - - last_trx_part = curr_part - 1; - /* Jump to the end of TRX */ + trx = (struct trx_header *)buf; offset = roundup(offset + trx->length, blocksize); /* Next loop iteration will increase the offset */ offset -= blocksize; @@ -307,9 +326,17 @@ static int bcm47xxpart_parse(struct mtd_info *master, parts[i + 1].offset : master->size; parts[i].size = next_part_offset - parts[i].offset; - if (i == last_trx_part && trx_part >= 0) - parts[trx_part].size = next_part_offset - - parts[trx_part].offset; + } + + /* If there was TRX parse it now */ + if (trx_part >= 0) { + int num_parts; + + num_parts = bcm47xxpart_parse_trx(master, &parts[trx_part], + parts + curr_part, + BCM47XXPART_MAX_PARTS - curr_part); + if (num_parts > 0) + curr_part += num_parts; } *pparts = parts;