Re: [PATCH v12 3/3] mtd: Add driver for concatenating devices

[Miquel Raynal]

On 05/02/2025 at 19:07:30 +0530, Amit Kumar Mahapatra <amit.kumar-mahapatra@xxxxxxx> wrote:

> Introducing CONFIG_VIRT_CONCAT to separate the legacy flow from the
> new

CONFIG_MTD_VIRT_CONCAT

> approach, where individual partitions within a concatenated partition are
> not registered, as they are likely not needed by the user.

I am not a big fan of this choice. We had issues with hiding things to
the user in the first place. Could we find a way to expose both the
original mtd devices as well as the virtually concatenated partitions?

> Solution is focusing on fixed-partitions description only because it
> depends on device boundaries.
>
> Suggested-by: Bernhard Frauendienst <kernel@xxxxxxxxxxxxxxxxx>
> Suggested-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@xxxxxxx>
> ---
>  drivers/mtd/Kconfig           |   8 ++
>  drivers/mtd/Makefile          |   1 +
>  drivers/mtd/mtd_virt_concat.c | 254 ++++++++++++++++++++++++++++++++++
>  drivers/mtd/mtdcore.c         |   7 +
>  drivers/mtd/mtdpart.c         |   6 +
>  include/linux/mtd/concat.h    |  24 ++++
>  6 files changed, 300 insertions(+)
>  create mode 100644 drivers/mtd/mtd_virt_concat.c
>
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index 796a2eccbef0..3dade7c469df 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -206,6 +206,14 @@ config MTD_PARTITIONED_MASTER
>  	  the parent of the partition device be the master device, rather than
>  	  what lies behind the master.
>  
> +config MTD_VIRT_CONCAT
> +	tristate "Virtual concatenated MTD devices"
> +	help
> +	  The driver enables the creation of a virtual MTD device

                                          of virtual MTD devices

> +	  by concatenating multiple physical MTD devices into a single
> +	  entity. This allows for the creation of partitions larger than
> +	  the individual physical chips, extending across chip boundaries.
> +

...

> +static int __init mtd_virt_concat_create_join(void)
> +{
> +	struct mtd_virt_concat_node *item;
> +	struct mtd_concat *concat;
> +	struct mtd_info *mtd;
> +	ssize_t name_sz;
> +	char *name;
> +	int ret;
> +
> +	list_for_each_entry(item, &concat_node_list, head) {
> +		concat = item->concat;
> +		mtd = &concat->mtd;
> +		/* Create the virtual device */
> +		name_sz = snprintf(NULL, 0, "%s-%s%s-concat",
> +				   concat->subdev[0]->name,
> +				   concat->subdev[1]->name,
> +				   concat->num_subdev > MIN_DEV_PER_CONCAT ?
> +				   "-+" : "");
> +		name = kmalloc(name_sz + 1, GFP_KERNEL);
> +		if (!name) {
> +			mtd_virt_concat_put_mtd_devices(concat);
> +			return -ENOMEM;
> +		}
> +
> +		sprintf(name, "%s-%s%s-concat",
> +			concat->subdev[0]->name,
> +			concat->subdev[1]->name,
> +			concat->num_subdev > MIN_DEV_PER_CONCAT ?
> +			"-+" : "");
> +
> +		mtd = mtd_concat_create(concat->subdev, concat->num_subdev, name);
> +		if (!mtd) {
> +			kfree(name);
> +			return -ENXIO;
> +		}
> +
> +		/* Arbitrary set the first device as parent */

Here we may face runtime PM issues. At some point the device model
expects a single parent per struct device, but here we have two. I do
not have any hints at the moment on how we could solve that.

> +		mtd->dev.parent = concat->subdev[0]->dev.parent;
> +		mtd->dev = concat->subdev[0]->dev;
> +
> +		/* Register the platform device */
> +		ret = mtd_device_register(mtd, NULL, 0);
> +		if (ret)
> +			goto destroy_concat;
> +	}
> +
> +	return 0;
> +
> +destroy_concat:
> +	mtd_concat_destroy(mtd);
> +
> +	return ret;
> +}
> +
> +late_initcall(mtd_virt_concat_create_join);

The current implementation does not support probe deferrals, I believe
it should be handled.

> +static void __exit mtd_virt_concat_exit(void)
> +{
> +	mtd_virt_concat_destroy_joins();
> +	mtd_virt_concat_destroy_items();
> +}
> +module_exit(mtd_virt_concat_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Bernhard Frauendienst <kernel@xxxxxxxxxxxxxxxxx>");
> +MODULE_AUTHOR("Amit Kumar Mahapatra <amit.kumar-mahapatra@xxxxxxx>");
> +MODULE_DESCRIPTION("Virtual concat MTD device driver");
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 724f917f91ba..2264fe81810f 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -34,6 +34,7 @@
>  
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/partitions.h>
> +#include <linux/mtd/concat.h>
>  
>  #include "mtdcore.h"
>  
> @@ -1067,6 +1068,12 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  			goto out;
>  	}
>  
> +	if (IS_ENABLED(CONFIG_MTD_VIRT_CONCAT)) {

Maybe IS_REACHABLE() is more relevant?

> +		ret = mtd_virt_concat_node_create();
> +		if (ret < 0)
> +			goto out;
> +	}
> +
>  	/* Prefer parsed partitions over driver-provided fallback */
>  	ret = parse_mtd_partitions(mtd, types, parser_data);
>  	if (ret == -EPROBE_DEFER)

Thanks,
Miquèl