Re: [PATCH] mtd: parsers: qcom: deal with partitions names on Netgear WAX218

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Rafał,

zajec5@gmail.com wrote on Mon, 17 Apr 2023 15:37:19 +0200:

> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Netgear WAX218 has two similar partitions: "rootfs" and "rootfs_1". They
> both contain device firmware (kernel + rootfs). Bootloader chooses the
> one to boot from depending on the "active_fw" variable.
> 
> The whole mechanism is used for safe firmware upgrades.
> 
> Sadly original bootloader on this device doesn't let know what firmware
> it has booted.
> 
> To run system reliably we need to know which kernel has been booted and
> mount proper rootfs accordingly. This patch registers currently used
> partition using the "firmware" name.

Let me try to understand this further: you need to know what kernel has
started and use the relevant rootfs, both stored in mtd partitions, so
you always mount the "firmware" partition as rootfs by changing the
partition name dynamically at boot time. Am I understanding this
correctly?

> It isn't clear how many devices will need this so just check for a
> WAX218 compatible string for now.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> Cc: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Cc: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/mtd/parsers/qcomsmempart.c | 92 +++++++++++++++++++++++++++---
>  1 file changed, 84 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/parsers/qcomsmempart.c b/drivers/mtd/parsers/qcomsmempart.c
> index 4311b89d8df0..352fb153745c 100644
> --- a/drivers/mtd/parsers/qcomsmempart.c
> +++ b/drivers/mtd/parsers/qcomsmempart.c
> @@ -54,6 +54,69 @@ struct smem_flash_ptable {
>  	struct smem_flash_pentry pentry[SMEM_FLASH_PTABLE_MAX_PARTS_V4];
>  } __packed __aligned(4);
>  
> +/**
> + * parse_qcomsmem_get_active_fw - get active firmware index
> + *
> + * Some devices have active and backup/fallback firmwares. This function returns index of the active
> + * one. It's useful for registering MTD partitions using clear names.
> + */
> +static int parse_qcomsmem_get_active_fw(struct mtd_info *mtd, struct smem_flash_ptable *ptable)
> +{
> +	bool found = false;
> +	size_t offset;
> +	size_t bytes;
> +	size_t size;
> +	char *buf;
> +	char *var;
> +	int err;
> +	int i;
> +
> +	if (!of_machine_is_compatible("netgear,wax218"))
> +		return -EOPNOTSUPP;
> +
> +	/* Find partition with environment data */
> +
> +	for (i = 0; i < le32_to_cpu(ptable->numparts); i++) {
> +		struct smem_flash_pentry *pentry = &ptable->pentry[i];
> +
> +		if (!strcmp(pentry->name, "0:APPSBLENV")) {
> +			offset = le32_to_cpu(pentry->offset) * mtd->erasesize;
> +			size = le32_to_cpu(pentry->length) * mtd->erasesize;
> +			found = true;
> +			break;
> +		}
> +	}
> +	if (!found)
> +		return -ENOENT;
> +
> +	/* Read it */
> +
> +	buf = kcalloc(1, size, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	err = mtd_read(mtd, offset, size, &bytes, buf);
> +	if ((err && !mtd_is_bitflip(err)) || bytes != size) {
> +		pr_err("Failed to read environment\n");
> +		return -EIO;
> +	}

Why can't we access this information through an nvmem cell thanks to
the U-boot env parser?

> +
> +	/* Find & parse entry with active firmware index */
> +
> +	for (var = buf + 4; var < buf + size && *var; var += strlen(var) + 1) {
> +		const char *prefix = "active_fw=";
> +		unsigned long idx;
> +
> +		if (strstarts(var, prefix)) {
> +			err = kstrtol(var + strlen(prefix), 0, &idx);
> +
> +			return err ? err : idx;
> +		}
> +	}
> +
> +	return -ENOENT;
> +}
> +
>  static int parse_qcomsmem_part(struct mtd_info *mtd,
>  			       const struct mtd_partition **pparts,
>  			       struct mtd_part_parser_data *data)
> @@ -64,6 +127,8 @@ static int parse_qcomsmem_part(struct mtd_info *mtd,
>  	struct smem_flash_ptable *ptable;
>  	struct mtd_partition *parts;
>  	char *name, *c;
> +	int active_fw;
> +	int fw_idx;
>  
>  	if (IS_ENABLED(CONFIG_MTD_SPI_NOR_USE_4K_SECTORS)
>  			&& mtd->type == MTD_NORFLASH) {
> @@ -123,24 +188,35 @@ static int parse_qcomsmem_part(struct mtd_info *mtd,
>  			numparts++;
>  	}
>  
> +	active_fw = parse_qcomsmem_get_active_fw(mtd, ptable);
> +
>  	parts = kcalloc(numparts, sizeof(*parts), GFP_KERNEL);
>  	if (!parts)
>  		return -ENOMEM;
>  
> +	fw_idx = -1;
>  	for (i = 0, j = 0; i < tmpparts; i++) {
>  		pentry = &ptable->pentry[i];
>  		if (pentry->name[0] == '\0')
>  			continue;
>  
> -		name = kstrdup(pentry->name, GFP_KERNEL);
> -		if (!name) {
> -			ret = -ENOMEM;
> -			goto out_free_parts;
> -		}
> +		if (active_fw >= 0 && strstarts(pentry->name, "rootfs")) {
> +			name = kstrdup(++fw_idx == active_fw ? "firmware" : "backup", GFP_KERNEL);

I'm really not a big fan of this logic.

Changing dynamically the partition name really sounds dangerous.

Does the kernel need to know? Or is it "just" the userspace? Could we
imagine another mechanism than the name to carry the information?

> +			if (!name) {
> +				ret = -ENOMEM;
> +				goto out_free_parts;
> +			}
> +		} else {
> +			name = kstrdup(pentry->name, GFP_KERNEL);
> +			if (!name) {
> +				ret = -ENOMEM;
> +				goto out_free_parts;
> +			}
>  
> -		/* Convert name to lower case */
> -		for (c = name; *c != '\0'; c++)
> -			*c = tolower(*c);
> +			/* Convert name to lower case */
> +			for (c = name; *c != '\0'; c++)
> +				*c = tolower(*c);
> +		}
>  
>  		parts[j].name = name;
>  		parts[j].offset = le32_to_cpu(pentry->offset) * mtd->erasesize;


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/