Message-ID: <87msazv3ln.fsf@bootlin.com>
Date: Mon, 26 May 2025 16:39:00 +0200
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: "Mahapatra, Amit Kumar" <amit.kumar-mahapatra@....com>
Cc: "richard@....at" <richard@....at>,  "vigneshr@...com" <vigneshr@...com>,
  "robh@...nel.org" <robh@...nel.org>,  "krzk+dt@...nel.org"
 <krzk+dt@...nel.org>,  "conor+dt@...nel.org" <conor+dt@...nel.org>,
  "linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
  "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
  "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,  "git
 (AMD-Xilinx)" <git@....com>,  "amitrkcian2002@...il.com"
 <amitrkcian2002@...il.com>,  Bernhard Frauendienst
 <kernel@...pam.obeliks.de>
Subject: Re: [PATCH v12 3/3] mtd: Add driver for concatenating devices

On 26/05/2025 at 14:27:37 GMT, "Mahapatra, Amit Kumar" <amit.kumar-mahapatra@....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@....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

Powered by blists - more mailing lists