[AMD Official Use Only - AMD Internal Distribution Only] Hello Miquel, > -----Original Message----- > From: Miquel Raynal <miquel.raynal@xxxxxxxxxxx> > Sent: Tuesday, March 18, 2025 9:23 PM > To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@xxxxxxx> > Cc: richard@xxxxxx; vigneshr@xxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; > conor+dt@xxxxxxxxxx; linux-mtd@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx>; > amitrkcian2002@xxxxxxxxx; Bernhard Frauendienst <kernel@xxxxxxxxxxxxxxxxx> > Subject: Re: [PATCH v12 3/3] mtd: Add driver for concatenating devices > > 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? Sure, I think that can be done, but I took this approach to hide the original devices because Boris mentioned in [1] that we are creating the original partitions even though the user probably doesn't need them. I believe he is right, as I can't think of any use case where the user would require the individual devices instead of the concatenated device. [1] https://lore.kernel.org/linux-mtd/20191209113506.41341ed4@xxxxxxxxxxxxx/ > > > 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? Agreed Regards, Amit > > > + 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
