Re: [PATCH v2 2/4] mtd: parsers: Add Qcom SMEM parser

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Tue 17 Nov 11:48 CST 2020, Manivannan Sadhasivam wrote:

> NAND based Qualcomm platforms have the partition table populated in the
> Shared Memory (SMEM). Hence, add a parser for parsing the partitions
> from it.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/mtd/parsers/Kconfig        |   8 ++
>  drivers/mtd/parsers/Makefile       |   1 +
>  drivers/mtd/parsers/qcomsmempart.c | 169 +++++++++++++++++++++++++++++
>  3 files changed, 178 insertions(+)
>  create mode 100644 drivers/mtd/parsers/qcomsmempart.c
> 
> diff --git a/drivers/mtd/parsers/Kconfig b/drivers/mtd/parsers/Kconfig
> index e72354322f62..d90c30229052 100644
> --- a/drivers/mtd/parsers/Kconfig
> +++ b/drivers/mtd/parsers/Kconfig
> @@ -160,3 +160,11 @@ config MTD_REDBOOT_PARTS_READONLY
>  	  'FIS directory' images, enable this option.
>  
>  endif # MTD_REDBOOT_PARTS
> +
> +config MTD_QCOMSMEM_PARTS
> +	tristate "Qualcomm SMEM NAND flash partition parser"
> +	depends on MTD_NAND_QCOM || COMPILE_TEST
> +	depends on QCOM_SMEM
> +	help
> +	  This provides support for parsing partitions from Shared Memory (SMEM)
> +	  for NAND flash on Qualcomm platforms.
> diff --git a/drivers/mtd/parsers/Makefile b/drivers/mtd/parsers/Makefile
> index b0c5f62f9e85..50eb0b0a2210 100644
> --- a/drivers/mtd/parsers/Makefile
> +++ b/drivers/mtd/parsers/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_MTD_AFS_PARTS)		+= afs.o
>  obj-$(CONFIG_MTD_PARSER_TRX)		+= parser_trx.o
>  obj-$(CONFIG_MTD_SHARPSL_PARTS)		+= sharpslpart.o
>  obj-$(CONFIG_MTD_REDBOOT_PARTS)		+= redboot.o
> +obj-$(CONFIG_MTD_QCOMSMEM_PARTS)	+= qcomsmempart.o
> diff --git a/drivers/mtd/parsers/qcomsmempart.c b/drivers/mtd/parsers/qcomsmempart.c
> new file mode 100644
> index 000000000000..d8c2a3fa4dfe
> --- /dev/null
> +++ b/drivers/mtd/parsers/qcomsmempart.c
> @@ -0,0 +1,169 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Qualcomm SMEM NAND flash partition parser
> + *
> + * Copyright (C) 2020, Linaro Ltd.
> + */
> +
> +#include <linux/ctype.h>
> +#include <linux/module.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/partitions.h>
> +#include <linux/slab.h>
> +#include <linux/soc/qcom/smem.h>
> +
> +#define SMEM_AARM_PARTITION_TABLE	9
> +#define SMEM_APPS			0
> +
> +#define SMEM_FLASH_PART_MAGIC1		0x55ee73aa
> +#define SMEM_FLASH_PART_MAGIC2		0xe35ebddb
> +#define SMEM_FLASH_PTABLE_V3		3
> +#define SMEM_FLASH_PTABLE_V4		4
> +#define SMEM_FLASH_PTABLE_MAX_PARTS_V3	16
> +#define SMEM_FLASH_PTABLE_MAX_PARTS_V4	48
> +#define SMEM_FLASH_PTABLE_HDR_LEN	(4 * sizeof(u32))
> +#define SMEM_FLASH_PTABLE_NAME_SIZE	16
> +
> +/**
> + * struct smem_flash_pentry - SMEM Flash partition entry
> + * @name: Name of the partition
> + * @offset: Offset in blocks
> + * @length: Length of the partition in blocks
> + * @attr: Flags for this partition
> + */
> +struct smem_flash_pentry {
> +	char name[SMEM_FLASH_PTABLE_NAME_SIZE];
> +	u32 offset;

It would be nice if you noted that these are little endian (using
__le32) and used le32_to_cpu() below.


Apart from that I think this looks really good.

> +	u32 length;
> +	u8 attr;
> +} __packed __aligned(4);
> +
> +/**
> + * struct smem_flash_ptable - SMEM Flash partition table
> + * @magic1: Partition table Magic 1
> + * @magic2: Partition table Magic 2
> + * @version: Partition table version
> + * @numparts: Number of partitions in this ptable
> + * @pentry: Flash partition entries belonging to this ptable
> + */
> +struct smem_flash_ptable {
> +	u32 magic1;
> +	u32 magic2;
> +	u32 version;
> +	u32 numparts;
> +	struct smem_flash_pentry pentry[SMEM_FLASH_PTABLE_MAX_PARTS_V4];
> +} __packed __aligned(4);
> +
> +static int parse_qcomsmem_part(struct mtd_info *mtd,
> +			       const struct mtd_partition **pparts,
> +			       struct mtd_part_parser_data *data)
> +{
> +	struct smem_flash_pentry *pentry;
> +	struct smem_flash_ptable *ptable;
> +	size_t len = SMEM_FLASH_PTABLE_HDR_LEN;
> +	struct mtd_partition *parts;
> +	char *name, *c;
> +	int ret, i;
> +
> +	pr_debug("Parsing partition table info from SMEM\n");
> +	ptable = qcom_smem_get(SMEM_APPS, SMEM_AARM_PARTITION_TABLE, &len);
> +	if (IS_ERR(ptable)) {
> +		pr_err("Error reading partition table header\n");
> +		return PTR_ERR(ptable);
> +	}
> +
> +	/* Verify ptable magic */
> +	if (ptable->magic1 != SMEM_FLASH_PART_MAGIC1 ||
> +	    ptable->magic2 != SMEM_FLASH_PART_MAGIC2) {
> +		pr_err("Partition table magic verification failed\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Ensure that # of partitions is less than the max we have allocated */
> +	if (ptable->numparts > SMEM_FLASH_PTABLE_MAX_PARTS_V4) {
> +		pr_err("Partition numbers exceed the max limit\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Find out length of partition data based on table version */
> +	if (ptable->version <= SMEM_FLASH_PTABLE_V3) {
> +		len = SMEM_FLASH_PTABLE_HDR_LEN + SMEM_FLASH_PTABLE_MAX_PARTS_V3 *
> +			sizeof(struct smem_flash_pentry);
> +	} else if (ptable->version == SMEM_FLASH_PTABLE_V4) {
> +		len = SMEM_FLASH_PTABLE_HDR_LEN + SMEM_FLASH_PTABLE_MAX_PARTS_V4 *
> +			sizeof(struct smem_flash_pentry);
> +	} else {
> +		pr_err("Unknown ptable version (%d)", ptable->version);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Now that the partition table header has been parsed, verified
> +	 * and the length of the partition table calculated, read the
> +	 * complete partition table
> +	 */
> +	ptable = qcom_smem_get(SMEM_APPS, SMEM_AARM_PARTITION_TABLE, &len);
> +	if (IS_ERR_OR_NULL(ptable)) {
> +		pr_err("Error reading partition table\n");
> +		return PTR_ERR(ptable);
> +	}
> +
> +	parts = kcalloc(ptable->numparts, sizeof(*parts), GFP_KERNEL);
> +	if (!parts)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < ptable->numparts; 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;
> +		}
> +
> +		/* Convert name to lower case */
> +		for (c = name; *c != '\0'; c++)
> +			*c = tolower(*c);
> +
> +		parts[i].name = name;
> +		parts[i].offset = pentry->offset * mtd->erasesize;
> +		parts[i].mask_flags = pentry->attr;
> +		parts[i].size = pentry->length * mtd->erasesize;
> +		pr_debug("%d: %s offs=0x%08x size=0x%08x attr:0x%08x\n",
> +			 i, pentry->name, pentry->offset, pentry->length,
> +			 pentry->attr);
> +	}
> +
> +	pr_debug("SMEM partition table found: ver: %d len: %d\n",
> +		 ptable->version, ptable->numparts);
> +	*pparts = parts;
> +
> +	return i;

Had to check a few times, but afaict this is just ptable->numparts in
disguise... So how about just writing that instead?

Regards,
Bjorn

> +
> +out_free_parts:
> +	while (--i >= 0)
> +		kfree(parts[i].name);
> +	kfree(parts);
> +	*pparts = NULL;
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id qcomsmem_of_match_table[] = {
> +	{ .compatible = "qcom,smem-part" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, qcomsmem_of_match_table);
> +
> +static struct mtd_part_parser mtd_parser_qcomsmem = {
> +	.parse_fn = parse_qcomsmem_part,
> +	.name = "qcomsmem",
> +	.of_match_table = qcomsmem_of_match_table,
> +};
> +module_mtd_part_parser(mtd_parser_qcomsmem);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>");
> +MODULE_DESCRIPTION("Qualcomm SMEM NAND flash partition parser");
> -- 
> 2.17.1
>