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

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

On 26/05/2025 at 14:27:37 GMT, "Mahapatra, Amit Kumar" <amit.kumar-mahapatra@amd.com> wrote:

> [AMD Official Use Only - AMD Internal Distribution Only]
>
>> > [AMD Official Use Only - AMD Internal Distribution Only]
>> >
>> >> On 13/05/2025 at 14:45:39 GMT, "Mahapatra, Amit Kumar" <amit.kumar-
>> >> mahapatra@amd.com> wrote:
>> >>
>> >> > [AMD Official Use Only - AMD Internal Distribution Only]
>> >> >
>> >> > Hello Miquel,
>> >> >
>> >> >> >> > +           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.
>> >> >> >
>> >> >> > I see that the parse_mtd_partitions() API can return
>> >> >> > -EPROBE_DEFER during MTD device registration, but this behavior
>> >> >> > is specific to the parse_qcomsmem_part parser. None of the other
>> >> >> > parsers appear to support probe deferral. As discussed in RFC
>> >> >> > [1], the virtual concat feature is purely a fixed-partition
>> >> >> > capability, and based on my understanding, the fixed-partition parser does
>> not support probe deferral.
>> >> >> > Please let me know if you can think of any other probe deferral
>> >> >> > scenarios that might impact the virtual concat driver.
>> >> >>
>> >> >> That's true, but I kind of dislike the late_initcall, I fear it
>> >> >> might break in creative
>> >> ways.
>> >> >
>> >> > I understand, but since we require the partition information to be
>> >> > available, late_initcall seems to be the most suitable choice among
>> >> > the initcall levels—if we decide to proceed with using an initcall.
>> >> > Regarding potential failures, as far as I can tell, the operation
>> >> > would fail if, at the time of concatenation, one or more of the MTD
>> >> > devices involved in the concat are not yet available. In such a
>> >> > scenario, we can issue a kernel warning and exit gracefully. But,
>> >> > However, if you prefer to move away from using initcalls and have
>> >> > an alternative implementation approach in mind, please let us know.
>> >>
>> >> I am sorry but this does not work with modules, and we cannot ignore
>> >> this case I believe. More specifically, if a controller probe is
>> >> deferred (with EPROBE_DEFER or just prevented because some
>> >> dependencies are not yet satisfied), you'll get incorrectly defined mtd devices.
>> >
>> > Ok, an alternative solution could be to remove the initcall
>> > registration and instead invoke mtd_virt_concat_create_join()—which
>> > was previously registered as a late_initcall—directly from
>> mtd_device_parse_register().
>> > I believe this approach would address both of your concerns regarding
>> > module support and probe deferral. Additionally, we could consider
>> > moving the entire code from mtd_virt_concat.c into mtdconcat.c.
>> > Please let us know your take on this.
>>
>> What would this bring?
>>
>> Maybe we should trigger some kind of notifier after registering an mtd device and in
>> there attempt to gather all mtd devices required for the concatenation. Can you
>> please propose something like that?
>
> In the current patch, during MTD registration, if a device is
> part of a concatenated (concat) device, it is not registered individually.
> Instead, its information is stored in a concat-specific data structure, as
> it is not meant to be exposed as a standalone MTD device. As per my
> earlier proposal, once all individual MTD devices are registered,
> mtd_virt_concat_create_join() is called from
> mtd_device_parse_register() to scan this data structure and create the
> corresponding concat devices. Just to confirm, are you suggesting that
> mtd_virt_concat_create_join() should be triggered through a notifier
> instead? At the point when all individual MTD devices are registered,
> we already have the complete information required for concatenation.
> So, rather than relying on a listener notification, we cac directly call the
> API. Please let me know if I am missing anything here.

This approach does not stand because, afair, it relies on a single
late_initcall() which is too early. We want concatenation to work
regardless of the Kconfig selection =y or =m.

Thanks,
Miquèl