| Message ID | 20170330123527.30181-2-zajec5@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Brian Norris |
| Headers | show |
Hi, On Thu, Mar 30, 2017 at 02:35:27PM +0200, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > This makes TRX parsing code reusable with other platforms and parsers. > > Please note this patch doesn't really change anything in the existing > code, just moves it. There is still some place for improvement (e.g. > working on non-hacky method of checking rootfs format) but it's not > really a subject of this change. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > V2: A totally rebased & refreshed version. > --- > drivers/mtd/Kconfig | 4 ++ > drivers/mtd/Makefile | 1 + > drivers/mtd/bcm47xxpart.c | 97 +------------------------------ > drivers/mtd/parsers/Kconfig | 8 +++ > drivers/mtd/parsers/Makefile | 1 + > drivers/mtd/parsers/parser_trx.c | 119 +++++++++++++++++++++++++++++++++++++++ > 6 files changed, 136 insertions(+), 94 deletions(-) > create mode 100644 drivers/mtd/parsers/Kconfig > create mode 100644 drivers/mtd/parsers/Makefile > create mode 100644 drivers/mtd/parsers/parser_trx.c > > diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig > index e83a279f1217..5a2d71729b9a 100644 > --- a/drivers/mtd/Kconfig > +++ b/drivers/mtd/Kconfig > @@ -155,6 +155,10 @@ config MTD_BCM47XX_PARTS > This provides partitions parser for devices based on BCM47xx > boards. > > +menu "Partition parsers" > +source "drivers/mtd/parsers/Kconfig" > +endmenu Is "parsers" a better name than "partitions"? I proposed moving everything to "partitions" previously. > + > comment "User Modules And Translation Layers" > > # > diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile > index 99bb9a1f6e16..151d60df303a 100644 > --- a/drivers/mtd/Makefile > +++ b/drivers/mtd/Makefile > @@ -13,6 +13,7 @@ obj-$(CONFIG_MTD_AFS_PARTS) += afs.o > obj-$(CONFIG_MTD_AR7_PARTS) += ar7part.o > obj-$(CONFIG_MTD_BCM63XX_PARTS) += bcm63xxpart.o > obj-$(CONFIG_MTD_BCM47XX_PARTS) += bcm47xxpart.o > +obj-y += parsers/ > > # 'Users' - code which presents functionality to userspace. > obj-$(CONFIG_MTD_BLKDEVS) += mtd_blkdevs.o > diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c > index d10fa6c8f074..66ce72fd1426 100644 > --- a/drivers/mtd/bcm47xxpart.c > +++ b/drivers/mtd/bcm47xxpart.c > @@ -43,7 +43,6 @@ > #define ML_MAGIC2 0x26594131 > #define TRX_MAGIC 0x30524448 > #define SHSQ_MAGIC 0x71736873 /* shsq (weird ZTE H218N endianness) */ > -#define UBI_EC_MAGIC 0x23494255 /* UBI# */ > > struct trx_header { > uint32_t magic; > @@ -62,89 +61,6 @@ static void bcm47xxpart_add_part(struct mtd_partition *part, const char *name, > part->mask_flags = mask_flags; > } > > -static const char *bcm47xxpart_trx_data_part_name(struct mtd_info *master, > - size_t offset) > -{ > - uint32_t buf; > - size_t bytes_read; > - int err; > - > - err = mtd_read(master, offset, sizeof(buf), &bytes_read, > - (uint8_t *)&buf); > - if (err && !mtd_is_bitflip(err)) { > - pr_err("mtd_read error while parsing (offset: 0x%X): %d\n", > - offset, err); > - goto out_default; > - } > - > - if (buf == UBI_EC_MAGIC) > - return "ubi"; > - > -out_default: > - 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; > -} > - > /** > * bcm47xxpart_bootpartition - gets index of TRX partition used by bootloader > * > @@ -362,17 +278,10 @@ static int bcm47xxpart_parse(struct mtd_info *master, > for (i = 0; i < trx_num; i++) { > struct mtd_partition *trx = &parts[trx_parts[i]]; > > - if (i == bcm47xxpart_bootpartition()) { > - int num_parts; > - > - num_parts = bcm47xxpart_parse_trx(master, trx, > - parts + curr_part, > - BCM47XXPART_MAX_PARTS - curr_part); > - if (num_parts > 0) > - curr_part += num_parts; > - } else { > + if (i == bcm47xxpart_bootpartition()) > + trx->format = "trx"; > + else > trx->name = "failsafe"; > - } > } > > *pparts = parts; > diff --git a/drivers/mtd/parsers/Kconfig b/drivers/mtd/parsers/Kconfig > new file mode 100644 > index 000000000000..ebb697a52698 > --- /dev/null > +++ b/drivers/mtd/parsers/Kconfig > @@ -0,0 +1,8 @@ > +config MTD_PARSER_TRX > + tristate "Parser for TRX format partitions" > + depends on MTD && (BCM47XX || ARCH_BCM_5301X) > + help > + TRX is a firmware format used by Broadcom on their devices. It > + may contain up to 3/4 partitions (depending on the version). > + This driver will parse TRX header and report at least two partitions: > + kernel and rootfs. > diff --git a/drivers/mtd/parsers/Makefile b/drivers/mtd/parsers/Makefile > new file mode 100644 > index 000000000000..4d9024e0be3b > --- /dev/null > +++ b/drivers/mtd/parsers/Makefile > @@ -0,0 +1 @@ > +obj-$(CONFIG_MTD_PARSER_TRX) += parser_trx.o > diff --git a/drivers/mtd/parsers/parser_trx.c b/drivers/mtd/parsers/parser_trx.c > new file mode 100644 > index 000000000000..f35e28148808 > --- /dev/null > +++ b/drivers/mtd/parsers/parser_trx.c > @@ -0,0 +1,119 @@ > +/* > + * Parser for TRX format partitions > + * > + * Copyright (C) 2012 - 2017 Rafał Miłecki <rafal@milecki.pl> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + */ > + > +#include <linux/module.h> > +#include <linux/slab.h> > +#include <linux/mtd/mtd.h> > +#include <linux/mtd/partitions.h> > + > +#define TRX_PARSER_MAX_PARTS 4 > + > +/* Magics */ > +#define UBI_EC_MAGIC 0x23494255 /* UBI# */ > + > +struct trx_header { > + uint32_t magic; > + uint32_t length; > + uint32_t crc32; > + uint16_t flags; > + uint16_t version; > + uint32_t offset[3]; > +} __packed; > + > +static const char *parser_trx_data_part_name(struct mtd_info *master, > + size_t offset) > +{ > + uint32_t buf; > + size_t bytes_read; > + int err; > + > + err = mtd_read(master, offset, sizeof(buf), &bytes_read, > + (uint8_t *)&buf); > + if (err && !mtd_is_bitflip(err)) { > + pr_err("mtd_read error while parsing (offset: 0x%X): %d\n", > + offset, err); > + goto out_default; > + } > + > + if (buf == UBI_EC_MAGIC) > + return "ubi"; > + > +out_default: > + return "rootfs"; > +} > + > +static int parser_trx_parse(struct mtd_info *mtd, > + const struct mtd_partition **pparts, > + struct mtd_part_parser_data *data) So, this function doesn't do any validation that this is *actually* a TRX partition? I think it needs to do *some* level of validation if it's going to be an independent parser driver like this. See my comments on your other patch. (I also can't say that I understand the parent bcm47xxpart format/parser very well; it really looks like a collection of hacks, so maybe validation is impossible...) > +{ > + struct mtd_partition *parts; > + struct mtd_partition *part; > + struct trx_header trx; > + size_t bytes_read; > + uint8_t curr_part = 0, i = 0; > + int err; > + > + parts = kzalloc(sizeof(struct mtd_partition) * TRX_PARSER_MAX_PARTS, > + GFP_KERNEL); > + if (!parts) > + return -ENOMEM; > + > + err = mtd_read(mtd, 0, sizeof(trx), &bytes_read, (uint8_t *)&trx); > + if (err) { > + pr_err("MTD reading error: %d\n", err); > + return err; You're leaking 'parts' here. (smatch noticed this.) > + } > + > + /* We have LZMA loader if offset[2] points to sth */ Not that this is new, but I have no idea what "sth" is. > + if (trx.offset[2]) { > + part = &parts[curr_part++]; > + part->name = "loader"; > + part->offset = trx.offset[i]; > + i++; > + } > + > + if (trx.offset[i]) { > + part = &parts[curr_part++]; > + part->name = "linux"; > + part->offset = trx.offset[i]; > + i++; > + } > + > + if (trx.offset[i]) { > + part = &parts[curr_part++]; > + part->name = parser_trx_data_part_name(mtd, trx.offset[i]); > + part->offset = trx.offset[i]; > + 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 : mtd->size; > + > + parts[i].size = next_part_offset - parts[i].offset; > + } > + > + *pparts = parts; > + return i; > +}; > + > +static struct mtd_part_parser mtd_parser_trx = { > + .parse_fn = parser_trx_parse, > + .name = "trx", > +}; > +module_mtd_part_parser(mtd_parser_trx); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("Parser for TRX format partitions"); Brian
On 05/11/2017 08:31 PM, Brian Norris wrote: > On Thu, Mar 30, 2017 at 02:35:27PM +0200, Rafał Miłecki wrote: >> From: Rafał Miłecki <rafal@milecki.pl> >> >> This makes TRX parsing code reusable with other platforms and parsers. >> >> Please note this patch doesn't really change anything in the existing >> code, just moves it. There is still some place for improvement (e.g. >> working on non-hacky method of checking rootfs format) but it's not >> really a subject of this change. >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> --- >> V2: A totally rebased & refreshed version. >> --- >> drivers/mtd/Kconfig | 4 ++ >> drivers/mtd/Makefile | 1 + >> drivers/mtd/bcm47xxpart.c | 97 +------------------------------ >> drivers/mtd/parsers/Kconfig | 8 +++ >> drivers/mtd/parsers/Makefile | 1 + >> drivers/mtd/parsers/parser_trx.c | 119 +++++++++++++++++++++++++++++++++++++++ >> 6 files changed, 136 insertions(+), 94 deletions(-) >> create mode 100644 drivers/mtd/parsers/Kconfig >> create mode 100644 drivers/mtd/parsers/Makefile >> create mode 100644 drivers/mtd/parsers/parser_trx.c >> >> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig >> index e83a279f1217..5a2d71729b9a 100644 >> --- a/drivers/mtd/Kconfig >> +++ b/drivers/mtd/Kconfig >> @@ -155,6 +155,10 @@ config MTD_BCM47XX_PARTS >> This provides partitions parser for devices based on BCM47xx >> boards. >> >> +menu "Partition parsers" >> +source "drivers/mtd/parsers/Kconfig" >> +endmenu > > Is "parsers" a better name than "partitions"? I proposed moving > everything to "partitions" previously. I didn't see that or forgot about that, sorry. Since this module contains a parser, I think "parsers" subdirectory may be a good choice. Did you plan to put anything else than parsers in the "partitions" subdir? >> +static const char *parser_trx_data_part_name(struct mtd_info *master, >> + size_t offset) >> +{ >> + uint32_t buf; >> + size_t bytes_read; >> + int err; >> + >> + err = mtd_read(master, offset, sizeof(buf), &bytes_read, >> + (uint8_t *)&buf); >> + if (err && !mtd_is_bitflip(err)) { >> + pr_err("mtd_read error while parsing (offset: 0x%X): %d\n", >> + offset, err); >> + goto out_default; >> + } >> + >> + if (buf == UBI_EC_MAGIC) >> + return "ubi"; >> + >> +out_default: >> + return "rootfs"; >> +} >> + >> +static int parser_trx_parse(struct mtd_info *mtd, >> + const struct mtd_partition **pparts, >> + struct mtd_part_parser_data *data) > > So, this function doesn't do any validation that this is *actually* a > TRX partition? I think it needs to do *some* level of validation if it's > going to be an independent parser driver like this. See my comments on > your other patch. > > (I also can't say that I understand the parent bcm47xxpart format/parser > very well; it really looks like a collection of hacks, so maybe > validation is impossible...) It is possible, I'll add it! >> +{ >> + struct mtd_partition *parts; >> + struct mtd_partition *part; >> + struct trx_header trx; >> + size_t bytes_read; >> + uint8_t curr_part = 0, i = 0; >> + int err; >> + >> + parts = kzalloc(sizeof(struct mtd_partition) * TRX_PARSER_MAX_PARTS, >> + GFP_KERNEL); >> + if (!parts) >> + return -ENOMEM; >> + >> + err = mtd_read(mtd, 0, sizeof(trx), &bytes_read, (uint8_t *)&trx); >> + if (err) { >> + pr_err("MTD reading error: %d\n", err); >> + return err; > > You're leaking 'parts' here. (smatch noticed this.) Right, thanks. >> + } >> + >> + /* We have LZMA loader if offset[2] points to sth */ > > Not that this is new, but I have no idea what "sth" is. I'll improve this comment.
diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig index e83a279f1217..5a2d71729b9a 100644 --- a/drivers/mtd/Kconfig +++ b/drivers/mtd/Kconfig @@ -155,6 +155,10 @@ config MTD_BCM47XX_PARTS This provides partitions parser for devices based on BCM47xx boards. +menu "Partition parsers" +source "drivers/mtd/parsers/Kconfig" +endmenu + comment "User Modules And Translation Layers" # diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile index 99bb9a1f6e16..151d60df303a 100644 --- a/drivers/mtd/Makefile +++ b/drivers/mtd/Makefile @@ -13,6 +13,7 @@ obj-$(CONFIG_MTD_AFS_PARTS) += afs.o obj-$(CONFIG_MTD_AR7_PARTS) += ar7part.o obj-$(CONFIG_MTD_BCM63XX_PARTS) += bcm63xxpart.o obj-$(CONFIG_MTD_BCM47XX_PARTS) += bcm47xxpart.o +obj-y += parsers/ # 'Users' - code which presents functionality to userspace. obj-$(CONFIG_MTD_BLKDEVS) += mtd_blkdevs.o diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c index d10fa6c8f074..66ce72fd1426 100644 --- a/drivers/mtd/bcm47xxpart.c +++ b/drivers/mtd/bcm47xxpart.c @@ -43,7 +43,6 @@ #define ML_MAGIC2 0x26594131 #define TRX_MAGIC 0x30524448 #define SHSQ_MAGIC 0x71736873 /* shsq (weird ZTE H218N endianness) */ -#define UBI_EC_MAGIC 0x23494255 /* UBI# */ struct trx_header { uint32_t magic; @@ -62,89 +61,6 @@ static void bcm47xxpart_add_part(struct mtd_partition *part, const char *name, part->mask_flags = mask_flags; } -static const char *bcm47xxpart_trx_data_part_name(struct mtd_info *master, - size_t offset) -{ - uint32_t buf; - size_t bytes_read; - int err; - - err = mtd_read(master, offset, sizeof(buf), &bytes_read, - (uint8_t *)&buf); - if (err && !mtd_is_bitflip(err)) { - pr_err("mtd_read error while parsing (offset: 0x%X): %d\n", - offset, err); - goto out_default; - } - - if (buf == UBI_EC_MAGIC) - return "ubi"; - -out_default: - 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; -} - /** * bcm47xxpart_bootpartition - gets index of TRX partition used by bootloader * @@ -362,17 +278,10 @@ static int bcm47xxpart_parse(struct mtd_info *master, for (i = 0; i < trx_num; i++) { struct mtd_partition *trx = &parts[trx_parts[i]]; - if (i == bcm47xxpart_bootpartition()) { - int num_parts; - - num_parts = bcm47xxpart_parse_trx(master, trx, - parts + curr_part, - BCM47XXPART_MAX_PARTS - curr_part); - if (num_parts > 0) - curr_part += num_parts; - } else { + if (i == bcm47xxpart_bootpartition()) + trx->format = "trx"; + else trx->name = "failsafe"; - } } *pparts = parts; diff --git a/drivers/mtd/parsers/Kconfig b/drivers/mtd/parsers/Kconfig new file mode 100644 index 000000000000..ebb697a52698 --- /dev/null +++ b/drivers/mtd/parsers/Kconfig @@ -0,0 +1,8 @@ +config MTD_PARSER_TRX + tristate "Parser for TRX format partitions" + depends on MTD && (BCM47XX || ARCH_BCM_5301X) + help + TRX is a firmware format used by Broadcom on their devices. It + may contain up to 3/4 partitions (depending on the version). + This driver will parse TRX header and report at least two partitions: + kernel and rootfs. diff --git a/drivers/mtd/parsers/Makefile b/drivers/mtd/parsers/Makefile new file mode 100644 index 000000000000..4d9024e0be3b --- /dev/null +++ b/drivers/mtd/parsers/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_MTD_PARSER_TRX) += parser_trx.o diff --git a/drivers/mtd/parsers/parser_trx.c b/drivers/mtd/parsers/parser_trx.c new file mode 100644 index 000000000000..f35e28148808 --- /dev/null +++ b/drivers/mtd/parsers/parser_trx.c @@ -0,0 +1,119 @@ +/* + * Parser for TRX format partitions + * + * Copyright (C) 2012 - 2017 Rafał Miłecki <rafal@milecki.pl> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + */ + +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/mtd/mtd.h> +#include <linux/mtd/partitions.h> + +#define TRX_PARSER_MAX_PARTS 4 + +/* Magics */ +#define UBI_EC_MAGIC 0x23494255 /* UBI# */ + +struct trx_header { + uint32_t magic; + uint32_t length; + uint32_t crc32; + uint16_t flags; + uint16_t version; + uint32_t offset[3]; +} __packed; + +static const char *parser_trx_data_part_name(struct mtd_info *master, + size_t offset) +{ + uint32_t buf; + size_t bytes_read; + int err; + + err = mtd_read(master, offset, sizeof(buf), &bytes_read, + (uint8_t *)&buf); + if (err && !mtd_is_bitflip(err)) { + pr_err("mtd_read error while parsing (offset: 0x%X): %d\n", + offset, err); + goto out_default; + } + + if (buf == UBI_EC_MAGIC) + return "ubi"; + +out_default: + return "rootfs"; +} + +static int parser_trx_parse(struct mtd_info *mtd, + const struct mtd_partition **pparts, + struct mtd_part_parser_data *data) +{ + struct mtd_partition *parts; + struct mtd_partition *part; + struct trx_header trx; + size_t bytes_read; + uint8_t curr_part = 0, i = 0; + int err; + + parts = kzalloc(sizeof(struct mtd_partition) * TRX_PARSER_MAX_PARTS, + GFP_KERNEL); + if (!parts) + return -ENOMEM; + + err = mtd_read(mtd, 0, sizeof(trx), &bytes_read, (uint8_t *)&trx); + if (err) { + pr_err("MTD reading error: %d\n", err); + return err; + } + + /* We have LZMA loader if offset[2] points to sth */ + if (trx.offset[2]) { + part = &parts[curr_part++]; + part->name = "loader"; + part->offset = trx.offset[i]; + i++; + } + + if (trx.offset[i]) { + part = &parts[curr_part++]; + part->name = "linux"; + part->offset = trx.offset[i]; + i++; + } + + if (trx.offset[i]) { + part = &parts[curr_part++]; + part->name = parser_trx_data_part_name(mtd, trx.offset[i]); + part->offset = trx.offset[i]; + 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 : mtd->size; + + parts[i].size = next_part_offset - parts[i].offset; + } + + *pparts = parts; + return i; +}; + +static struct mtd_part_parser mtd_parser_trx = { + .parse_fn = parser_trx_parse, + .name = "trx", +}; +module_mtd_part_parser(mtd_parser_trx); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("Parser for TRX format partitions");