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

[Mahapatra, Amit Kumar]

[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