by Saravana Kannan

[permalink] [raw]

Subject: Re: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions'

On Fri, Dec 16, 2022 at 3:05 AM Miquel Raynal <[email protected]> wrote:
>
> Hi Saravana, Maxim, Maxim,
>
> [email protected] wrote on Wed, 14 Dec 2022 13:53:54 -0800:
>
> > On Tue, Dec 13, 2022 at 8:54 AM Miquel Raynal <[email protected]> wrote:
> > >
> > > Hi Maxim,
> > >
> > > [email protected] wrote on Tue, 13 Dec 2022 14:02:34 +0300:
> > >
> > > > I looked closer at commit 658c4448bbbf and bcdf0315a61a, 5db1c2dbc04c16 commits.
> > > > Looks like we have two different features binded to one property - "compatible".
> > > >
> > > > From one side it is the ability to forward the subnode of the mtd
> > > > partition to the nvmem subsystem (658c4448bbbf and ac42c46f983e).
> > > > And from another side is the ability to use custom initialization of
> > > > the mtd partition (bcdf0315a61a and 5db1c2dbc04c16).
> > > >
> > > > What I mean:
> > > > According to ac42c46f983e I can create DT like this:
> > > > - |
> > > > partitions {
> > > > compatible = "fixed-partitions";
> > > > #address-cells = <1>;
> > > > #size-cells = <1>;
> > > >
> > > > partition@0 {
> > > > compatible = "nvmem-cells";
> > > > reg = <0x40000 0x10000>;
> > > > #address-cells = <1>;
> > > > #size-cells = <1>;
> > > > macaddr_gmac1: macaddr_gmac1@0 {
> > > > reg = <0x0 0x6>;
> > > > };
> > > > };
> > > > };
> > > >
> > > >
> > > > And according to 5db1c2dbc04c16 I can create DT like this:
> > > > - |
> > > > partitions {
> > > > compatible = "fixed-partitions";
> > > > #address-cells = <1>;
> > > > #size-cells = <1>;
> > > >
> > > > partition@0 {
> > > > compatible = "u-boot,env";
> > > > reg = <0x40000 0x10000>;
> > > > };
> > > > };
> > > >
> > > > But I can not use them both, because only one "compatible" property allowed.
> > > > This will be incorrect:
> > > > - |
> > > > partitions {
> > > > compatible = "fixed-partitions";
> > > > #address-cells = <1>;
> > > > #size-cells = <1>;
> > > >
> > > > partition@0 {
> > > > compatible = "u-boot,env"; # from ac42c46f983e
> > > > compatible = "nvmem-cells"; # from 5db1c2dbc04c
> > >
> > > What about:
> > >
> > > compatible = "u-boot,env", "nvmem-cells";
> > >
> > > instead? that should actually work.
> > >
> > > > reg = <0x40000 0x10000>;
> > > > #address-cells = <1>;
> > > > #size-cells = <1>;
> > > > macaddr_gmac1: macaddr_gmac1@0 {
> > > > reg = <0x0 0x6>;
> > > > };
> > > > };
> > > > };
> > > >
> > > > > compatible: Duplicate property name
> > > >
> > > > вт, 13 дек. 2022 г. в 12:46, Miquel Raynal <[email protected]>:
> > > > >
> > > > > Hi Maxim,
> > > > >
> > > > > [email protected] wrote on Mon, 12 Dec 2022 20:57:49 +0300:
> > > > >
> > > > > > Hi, Miquel!
> > > > > >
> > > > > > On 12.12.2022 19:37, Miquel Raynal wrote:
> > > > > >
> > > > > > > Let me try to recap the situation for all the people I just involved:
> > > > > > >
> > > > > > > * An Ethernet driver gets its mac address from an nvmem cell. The
> > > > > > > Ethernet controller DT node then has an "nvmem-cells" property
> > > > > > > pointing towards an nvmem cell.
> > > > > > > * The nvmem cell comes from an mtd partition.
> > > > > > > * The mtd partition is flagged with a particular compatible
> > > > > > > (which is also named "nvmem-cells") to tell the kernel that the node
> > > > > > > produces nvmem cells.
> > > > > > > * The mtd partition itself has no driver, but is the child node of a
> > > > > > > "partitions" container which has one (in this case,
> > > > > > > "fixed-partitions", see the snippet below).
> > > > > > >
> > > > > > > Because the "nvmem-cells" property of the Ethernet node points at the
> > > > > > > nvmem-cell node, the core create a device link between the Ethernet
> > > > > > > controller (consumer) and the mtd partition (producer).
> > > > > > >
> > > > > > > The device link in this case will never be satisfied because no driver
> > > > > > > matches the "nvmem-cells" compatible of the partition node.
> > > > > > >
> > > > > > > Reverting commit bcdf0315a61a ("mtd: call of_platform_populate() for MTD
> > > > > > > partitions") would IMHO not make much sense, the problem comes from the
> > > > > > > device link side and even there, there is nothing really "wrong",
> > > > > > > because I really expect the mtd device to be ready before the
> > > > > > > Ethernet controller probe, the device link is legitimate.
> > > > > > >
> > > > > > > So I would like to explore other alternatives. Here are a bunch of
> > > > > > > ideas, but I'm open:
> > > > > >
> > > > > > How about to create simple driver with compatible="nvmem-cell" and to move all the suff from main mtd driver which serves nvmem-cell to the probe function?
> > > > >
> > > > > This is probably worth the try but I doubt you can make it work without
> > > > > regressions because IIRC the nvmem registration happens no matter the
> > > > > compatible (not mentioning the user-otp and factory-otp cases). You can
> > > > > definitely try this out if you think you can come up with something
> > > > > though.
> > > > >
> > > > > But I would like to hear from the device-link gurus :) because even if
> > > > > we fix mtd with a "trick" like above, I guess we'll very likely find
> > > > > other corner cases like that and I am interested in understanding the
> > > > > rationale of what could be a proper fix.
> > > > >
> >
> > Responding to the whole thread.
> >
> > I'm going by Miquel's first email in which he cc'ed me and haven't
> > actually looked at the mtd code. Couple of comments:
> >
> > Independent of mtd/nvmem-cell, I generally frown on having a
> > compatible string for a child node that you don't treat as a device.
> > Even more so if you actually create a struct device for it and then
> > don't do anything else with it. That's just a waste of memory. So, in
> > general try to avoid that in the future if you can.
>
> Agreed, it didn't triggered any warnings in my head in the first place,
> sorry about that.
>
> > Also, there are flags the parent device's driver can set that'll tell
> > fw_devlink not to treat a specific DT node as a real device. So, if we
> > really need that I'll dig up and suggest a fix.
>
> Interesting, that would indeed very likely fix it.
>
> > Lastly and more importantly, I've a series[1] that stops depending on
> > the compatible property for fw_devlink to work. So it should be
> > smarter than it is today. But that series has known bugs for which I
> > gave test fixes in that thread. I plan to make a v2 of that series
> > with that fix and I'm expecting it'll fix a bunch of fw_devlink
> > issues.
> >
> > Feel free to give v1 + squashing the fixes a shot if you are excited
> > to try it. Otherwise, I'll try my best to get around to it this week
> > (kinda swamped though + holidays coming up, so no promises).
>
> Can you please include us in your next submission?
> * Maxim Kiselev <[email protected]>
> * Maxim Kochetkov <[email protected]>
> * Miquel Raynal <[email protected]>

Will do.

-Saravana

>
> > [1] - https://lore.kernel.org/lkml/[email protected]/
>
> Maxim, any chance you give this a try?
>
> Thanks,
> Miquèl

2023-02-24 16:15:10

by Regzbot (on behalf of Thorsten Leemhuis)

[permalink] [raw]

Subject: Re: Fwd: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions'

[TLDR: This mail in primarily relevant for Linux regression tracking. A
change or fix related to the regression discussed in this thread was
posted or applied, but it did not use a Link: tag to point to the
report, as Linus and the documentation call for. Things happen, no
worries -- but now the regression tracking bot needs to be told manually
about the fix. See link in footer if these mails annoy you.]

On 10.12.22 13:35, Thorsten Leemhuis wrote:
> [...]
> P.P.S.: let me add this to the regression tracking:
>
> #regzbot ^introduced bcdf0315
> #regzbot title mtd: ethernet device can't be probed anymore due to
> broken nvmem-cells dep
> #regzbot monitor: https://lore.kernel.org/all/Yyj7wJlqJkCwObRn@lx2k/
> #regzbot ignore-activity

#regzbot fix: mtd: mtdpart: Don't create platform device that'll never probe
#regzbot ignore-activity

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.