Date:   Mon, 12 Dec 2022 17:37:30 +0100
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Maxim Kiselev <bigunclemax@...il.com>
Cc:     Thorsten Leemhuis <regressions@...mhuis.info>,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Vignesh Raghavendra <vigneshr@...com>,
        Richard Weinberger <richard@....at>,
        Maxim Kochetkov <fido_max@...ox.ru>,
        Rafał Miłecki <rafal@...ecki.pl>,
        "regressions@...ts.linux.dev" <regressions@...ts.linux.dev>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Saravana Kannan <saravanak@...gle.com>
Subject: Re: nvmem-cells regression after adding 'call
 of_platform_populate() for MTD partitions'

Hi Maxim,

+ Rob & Krzysztof for the binding side
+ Greg, Rafael & Saravana for the device links side

bigunclemax@...il.com wrote on Mon, 12 Dec 2022 16:06:35 +0300:

> > At a first glance I don't get why the compatible would matter so much
> > here, can you point to some core DT logic that would have an effect? I
> > mean besides leading to the creation of a cell. IOW, what would be done
> > differently if the compatible was different?  
> 
> After adding a call of_platform_populate() for MTD partitions (bcdf0315)
> we got a call of 'device_add' for each mtd partition which have
> a 'compatible' property in its DT node. As a result any other devices
> which have a DT reference to 'nvmem-cell' compatible mtd partition
> got dependence from it (see attached log below). Now they will be waiting until
> a 'nvmem-cell' device will be probed. But this will never happen because there
> is no 'nvmem-cell' driver.
> 
> 
> Here is part of kernel log after bcdf0315 commit:
>   device: 'spi0.0': device_add
>   spi-nor spi0.0: mx66l51235f (65536 Kbytes)
>   7 fixed-partitions partitions found on MTD device spi0.0
>   device: 'f1010600.spi:m25p80@0:partitions:partition@1': device_add
>   device: 'platform:f1010600.spi:m25p80@0:partitions:partition@...platform:f1070000.ethernet':
> device_add
>   devices_kset: Moving f1070000.ethernet to end of list
>   platform f1070000.ethernet: Linked as a consumer to
> f1010600.spi:m25p80@0:partitions:partition@1
>   ethernet@...00 Dropping the fwnode link to partition@1
>   Creating 7 MTD partitions on "spi0.0":
>   platform f1070000.ethernet: error -EPROBE_DEFER: supplier
> f1010600.spi:m25p80@0:partitions:partition@1 not ready
>   devices_kset: Moving f1070000.ethernet to end of list
>   platform f1070000.ethernet: error -EPROBE_DEFER: supplier
> f1010600.spi:m25p80@0:partitions:partition@1 not ready
> 
> Here is part of kernel log with reverted bcdf0315 commit:
>   device: 'spi0.0': device_add
>   spi-nor spi0.0: mx66l51235f (65536 Kbytes)
>   7 fixed-partitions partitions found on MTD device spi0.0
>   Creating 7 MTD partitions on "spi0.0":
>   ethernet@...00 Dropping the fwnode link to partition@1
>   device: 'eth0': device_add
>   mvneta f1070000.ethernet eth0: Using device tree mac address de:fa:ce:db:ab:e1
> 
> Look at strings below which exists only in the first log (with bcdf0315):
>   device: 'f1010600.spi:m25p80@0:partitions:partition@1': device_add
>   device: 'platform:f1010600.spi:m25p80@0:partitions:partition@...platform:f1070000.ethernet':
> device_add
>   platform f1070000.ethernet: error -EPROBE_DEFER: supplier
> f1010600.spi:m25p80@0:partitions:partition@1 not ready
> 
> 
> > because this just works with eeproms (non-mtd cells), so let's find out.  
> This works for eeproms and even for whole mtd partitions because there
> are compatible drivers and those devices
> can be probed, ulike a mtd subnode which is compatible with
> non-existent 'nvmem-cell' driver.

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:

1. compatible = "nvmem-cells" was maybe not the right way to say "this
   is an nvmem-cells producer":
     1a. Maybe we could use the (new) nvmem-layout container [1]?
     1b. Or we could force users to use the user-otp container [2]?
     1c. Or replace the compatible by a property?

   1c is ugly, all three solutions would break the current bindings,
   hence I would really prefer to got for #2.

2. I don't understand the device link state/flags deeply enough to
   understand what they all convey exactly, but I guess we
   should be able to tell the driver model that the "device" (ie. the
   partition) was created by a driver which owns this device. In this
   case an mtd device owned by the mtd core was created by a parser,
   and there is no additional probe step that should be expected.

   Maybe there is a way to tell this to the driver core at device
   registration time?

   Or perhaps we should workaround it by calling one of:
	- device_links_driver_bound()
	- device_links_no_driver()
   from the mtd core, after registering all the mtd partition devices?

   I am really interested to understand better how the device links are
   expected to evolve, so any pointers would be appreciated.

[1] https://lore.kernel.org/all/20221114085659.847611-1-miquel.raynal@bootlin.com/T/#m5c6ed1efd675fda0e2cd174502d38c1565f0274b

[2] https://lore.kernel.org/all/20210424110608.15748-4-michael@walle.cc/

Thanks,
Miquèl

> 
> пн, 12 дек. 2022 г. в 12:14, Miquel Raynal <miquel.raynal@...tlin.com>:
> >
> > Hi Maxim,
> >
> > bigunclemax@...il.com wrote on Sun, 11 Dec 2022 11:26:29 +0300:
> >  
> > > >On 10.12.22 10:52, Maxim Kiselev wrote:  
> > > >>
> > > >> After applying  
> > > >
> > > >This makes me wonder: "applying" as in "applying it to some version that
> > > >doesn't contain this change normally" or as it "after it was applied to
> > > >mainline I have the following problem with vanilla kernel version <???>"?  
> > >
> > > Sorry for confusing you, I mean "after it was applied to mainline".
> > > I have this problem with vanilla kernel version 6.0.
> > >  
> > > >>> I faced with a problem that my ethernet device can't be probed because it
> > > >>> wait when 'nvmem-cells' device will be probed first.  
> > > >>
> > > >>FWIW, there is a discussion about a problems that at least to my
> > > >>untrained eyes looks similar:
> > > >>https://lore.kernel.org/all/Yyj7wJlqJkCwObRn@lx2k/  
> > >
> > > Yes it looks like the same issue.
> > >
> > >
> > > I think the root of the problem was the choice of 'compatible'
> > > device tree property to mark the mtd partition node as a nvmem provider.
> > >
> > > I'm talking about this part in 'mtd_nvmem_add' function.  
> > > > config.no_of_node = !of_device_is_compatible(node, "nvmem-cells");  
> > >
> > > Maybe we should change the 'compatible' property to something else?  
> >
> > At a first glance I don't get why the compatible would matter so much
> > here, can you point to some core DT logic that would have an effect? I
> > mean besides leading to the creation of a cell. IOW, what would be done
> > differently if the compatible was different?
> >
> > Can you also dump the device links (if you can reach a prompt) from
> > sysfs?
> >
> > In theory there should be a link between ethernet and spi-nor which is
> > fulfilled when the spi-nor device probes and leads to the creation of
> > device links. Maybe there is "something else" that the mtd core should
> > do, because this just works with eeproms (non-mtd cells), so let's find
> > out.
> >  
> > > сб, 10 дек. 2022 г. в 15:35, Thorsten Leemhuis <regressions@...mhuis.info>:  
> > > >
> > > > [CCing the regression mailing list, as it should be in the loop for all
> > > > regressions, as explained in
> > > > https://docs.kernel.org/admin-guide/reporting-regressions.html ]
> > > >
> > > > Hi, this is your Linux kernel regression tracker. Thx for the report.
> > > >
> > > > On 10.12.22 10:52, Maxim Kiselev wrote:  
> > > > >
> > > > > After applying  
> > > >
> > > > This makes me wonder: "applying" as in "applying it to some version that
> > > > doesn't contain this change normally" or as it "after it was applied to
> > > > mainline I have the following problem with vanilla kernel version <???>"?
> > > >  
> > > > > this commit 'mtd: call of_platform_populate() for MTD
> > > > > partitions' (bcdf0315),  
> > > >
> > > > CCing Rafał, who authored bcdf0315.
> > > >  
> > > > > I faced with a problem that my ethernet device can't be probed because it
> > > > > wait when 'nvmem-cells' device will be probed first.  
> > > >
> > > > FWIW, there is a discussion about a problems that at least to my
> > > > untrained eyes looks similar:
> > > > https://lore.kernel.org/all/Yyj7wJlqJkCwObRn@lx2k/
> > > >
> > > > Rafał, has some progress been made to resolve this?
> > > >
> > > > To me it sounds like this might warrant a "revert, and reapply later
> > > > when the cause for the regression was addressed". Rafał, it seems you
> > > > suggested something like that, but it doesn't look like that happened
> > > > for one reason or another. Or am I missing something?
> > > >
> > > > Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> > > >
> > > > P.S.: As the Linux kernel's regression tracker I deal with a lot of
> > > > reports and sometimes miss something important when writing mails like
> > > > this. If that's the case here, don't hesitate to tell me in a public
> > > > reply, it's in everyone's interest to set the public record straight.
> > > >  
> > > > > But there is no such driver which is compatible with 'nvmem-cells' because
> > > > > 'nvmem-cells' is just a mark used by the 'mtd_nvmem_add' function.
> > > > >
> > > > > So this leads to appeating of unresolved dependency for the ethernet device.
> > > > > And that's why the ethernet device can't be added and probed.
> > > > >
> > > > > Here is a part of kernel log when spi flash probe start:
> > > > >  
> > > > >> device: 'spi0': device_add
> > > > >> device: 'spi0.0': device_add
> > > > >> spi-nor spi0.0: mx66l51235f (65536 Kbytes)
> > > > >> 7 fixed-partitions partitions found on MTD device spi0.0  
> > > > >
> > > > > After 'm25p80' probe 'f1070000.ethernet' linked to 'partition@1' :
> > > > >  
> > > > >> device: 'f1010600.spi:m25p80@0:  
> > > > > partitions:partition@1': device_add  
> > > > >> device: 'platform:f1010600.spi:m25p80@0:partitions:partition@...platform:f1070000.ethernet': device_add
> > > > >> devices_kset: Moving f1070000.ethernet to end of list
> > > > >> platform f1070000.ethernet: Linked as a consumer to f1010600.spi:m25p80@0:partitions:partition@1
> > > > >> ethernet@...00 Dropping the fwnode link to partition@1  
> > > > >
> > > > > And as a result I got `-EPROBE_DEFER` for `f1070000.ethernet`
> > > > >  
> > > > >> platform f1070000.ethernet: error -EPROBE_DEFER: supplier f1010600.spi:m25p80@0:partitions:partition@1 not ready  
> > > > >
> > > > > Here is a part of my device tree:
> > > > >
> > > > >     enet1: ethernet@...00 {
> > > > >         status = "okay";
> > > > >         nvmem-cells = <&macaddr>;
> > > > >         nvmem-cell-names = "mac-address";
> > > > >         phy-mode = "rgmii";
> > > > >         phy = <&phy0>;
> > > > >     };
> > > > >
> > > > >     spi@...00 {
> > > > >         status = "okay";
> > > > >
> > > > >         m25p80@0 {
> > > > >             compatible = "mx66l51235l";
> > > > >             reg = <0>;
> > > > >             #address-cells = <1>;
> > > > >             #size-cells = <1>;
> > > > >
> > > > >             partitions {
> > > > >                 compatible = "fixed-partitions";
> > > > >                 #address-cells = <1>;
> > > > >                 #size-cells = <1>;
> > > > >
> > > > >                 partition@0 {
> > > > >                     reg = <0x00000000 0x000080000>;
> > > > >                     label = "SPI.U_BOOT";
> > > > >                 };
> > > > >
> > > > >                 partition@1 {
> > > > >                     compatible = "nvmem-cells";
> > > > >                     reg = <0x000A0000 0x00020000>;
> > > > >                     label = "SPI.INV_INFO";
> > > > >                     #address-cells = <1>;
> > > > >                     #size-cells = <1>;
> > > > >                     ranges = <0 0x000A0000 0x00020000>;
> > > > >
> > > > >                     macaddr: mac@6 {
> > > > >                         reg = <0x6 0x6>;
> > > > >                     };
> > > > >                 };
> > > > >
> > > > >             };
> > > > >         };
> > > > >     };
> > > > >
> > > > > In the example above 'ethernet@...00' requires 'macaddr: mac@6' which is
> > > > > located inside mtd 'partition@1' of 'm25p80@0' spi flash.  
> > > >
> > > > 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  
> >
> >
> > Thanks,
> > Miquèl  

Powered by blists - more mailing lists