Re: [PATCH V4 2/2] mtd: extract TRX parser out of bcm47xxpart into a separated module

[Brian Norris <computersforpeace@gmail.com>]

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