Message-ID: <20220519151529.qkhlsjfkh6mebpqw@skbuf>
Date:   Thu, 19 May 2022 15:15:30 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Saravana Kannan <saravanak@...gle.com>
CC:     "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        John Stultz <john.stultz@...aro.org>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Russell King <rmk+kernel@...linux.org.uk>,
        Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: [RFC PATCH devicetree] of: property: mark "interrupts" as
 optional for fw_devlink

On Fri, May 13, 2022 at 05:26:19PM -0700, Saravana Kannan wrote:
> On Fri, May 13, 2022 at 1:13 PM Vladimir Oltean <vladimir.oltean@....com> wrote:
> >
> > I have a board with an Ethernet PHY whose driver currently works in poll
> > mode. I am in the process of making some changes through which it will
> > be updated to use interrupts. The changes are twofold.
> >
> > First, an irqchip driver needs to be written, and device trees need to
> > be updated. But old kernels need to work with the updated device trees
> > as well. From their perspective, the interrupt-parent is a missing
> > supplier, so fw_devlink will block the consumer from probing.
> >
> > Second, proper interrupt support is only expected to be fulfilled on a
> > subset of boards on which this irqchip driver runs. The driver detects
> > this and fails to probe on unsatisfied requirements, which should allow
> > the consumer driver to fall back to poll mode. But fw_devlink treats a
> > supplier driver that failed to probe the same as a supplier driver that
> > did not probe at all, so again, it blocks the consumer.
> 
> This is easy to fix. I can send a patch for this soon. So, if the
> driver matches the supplier and then fails the probe (except
> EPROBE_DEFER), we can stop blocking the consumer on that supplier.

Ok, FWIW I tried this but did not succeed. What I tried was to pass the
error code to device_links_no_driver(), and from there call
fw_devlink_unblock_consumers() if the error code was != EPROBE_DEFER.
But the relevant links were skipped by fw_devlink_relax_link() for some
reason I forget, perhaps the MANAGED flag was already set.

> > According to Saravana's commit a9dd8f3c2cf3 ("of: property: Add
> > fw_devlink support for optional properties"), the way to deal with this
> > issues is to mark the struct supplier_bindings associated with
> > "interrupts" and "interrupts-extended" as "optional". Optional actually
> > means that fw_devlink will no longer create a fwnode link to the
> > interrupt parent, unless we boot with "fw_devlink.strict".
> 
> The optional flag is really meant for DT properties where even if the
> supplier is present, the consumer might not use it.

This is equally true for "interrupts", even I have an example for that,
in commit 3c0f9d8bcf47 ("spi: spi-fsl-dspi: Always use the TCFQ devices
in poll mode").

> reasoning, fw_devlink doesn't wait for those suppliers to probe even
> if the driver is present. fw_devlink outright ignores those properties
> unless fw_devlink.strict=1 (default is = 0).
> For some more context on why I added the optional flag, see Greet's
> last paragraph in this email explaining IOMMUs:
> https://lore.kernel.org/lkml/CAMuHMdXft=pJXXqY-i_GQTr8FtFJePQ_drVHRMPAFUqSy4aNKA@mail.gmail.com/#t
> 
> I'm still not fully sold if the "optional" flag was the right way to
> fix it and honestly might just delete it.

Yeah, probably, but then the question becomes what else to do.

> > So practically speaking, interrupts are now not "handled as optional",
> > but rather "not handled" by fw_devlink. This has quite wide ranging
> > side effects,
> 
> Yeah, and a lot of other boards/systems might be depending on
> enforcing "interrupts" dependency. So this patch really won't work for
> those cases.
> 
> So, I have to Nack this patch. But I tried to address your use case
> and other similar cases with this recent patch:
> https://lore.kernel.org/lkml/20220429220933.1350374-1-saravanak@google.com/
> 
> If the time out is too long (10s) then you can reduce it for your
> board (set it to 1), but by default every device that could probe will
> probe and fw_devlink will no longer block those probes. Btw, I talked
> about this in LPC2021 but only finally got around to sending out this
> patch. Can you give it a shot please?

This patch does work, but indeed, the 10 second timeout is not too
convenient, and the whole idea is to not disturb existing setups, i.e.
those where things set up by the bootloader (kernel cmdline, FDT blob)
are fixed, only the kernel image is updated.

> > for example it happens to fix the case (linked below)
> > where we have a cyclic dependency between a parent being an interrupt
> > supplier to a child, fw_devlink blocking the child from probing, and the
> > parent waiting for the child to probe before the parent itself finishes
> > probing successfully. This isn't really the main thing I'm intending
> > with this change, but rather a side observation.
> >
> > The reason why I'm blaming the commit below is because old kernels
> > should work with updated device trees, and that commit is practically
> > where the support was added. IMHO it should have been backported to
> > older kernels exactly for DT compatibility reasons, but it wasn't.
> >
> > Fixes: a9dd8f3c2cf3 ("of: property: Add fw_devlink support for optional properties")
> > Link: https://lore.kernel.org/netdev/20210826074526.825517-2-saravanak@google.com/
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> > ---
> > Technically this patch targets the devicetree git tree, but I think it
> > needs an ack from device core maintainers and/or people who contributed
> > to the device links and fw_devlink, or deferred probing in general.
> >
> > With this patch in place, the way in which things will work is that:
> > - of_irq_get() will return -EPROBE_DEFER a number of times.
> > - fwnode_mdiobus_phy_device_register(), through
> >   driver_deferred_probe_check_state(), will wait until the initcall
> >   stage is over (simplifying a bit), then fall back to poll mode.
> > - The PHY driver will now finally probe successfully
> > - The PHY driver might defer probe for so long, that the Ethernet
> >   controller might actually get an -EPROBE_DEFER when calling
> >   phy_attach_direct() or one of the many of its derivatives.
> >   This happens because "phy-handle" support was removed from fw_devlink
> >   in commit 3782326577d4 ("Revert "of: property: fw_devlink: Add support
> >   for "phy-handle" property"").
> 
> The next DT property I add support to would be phy-handle. But to do
> so, I need to make sure Generic PHYs are probed through the normal
> binding process but still try to handle the case where the PHY
> framework calls device_bind_driver() directly. I've spent a lot of
> time thinking about this. I have had a tab open with the phy_device.c
> code for months in my laptop. It's still there :)
> 
> Once I add support for this, I can then add support for some of the
> other mdio-* properties and then finally try to enable default async
> boot for DT based systems.

The problem with relying on fw_devlink for anything serious is that it
will always be self-defeating until it is complete (and it will probably
never be complete). I tried to explain this before, here:
https://lore.kernel.org/netdev/20210901012826.iuy2bhvkrgahhrl7@skbuf/

In summary, if there's any parent in this device/fwnode hierarchy that
defers probe, the entire graph gets torn down, and fw_devlink won't hide
the -EPROBE_DEFER from phylink anymore.

This, plus I would like something that works now (i.e. something that
can make existing stable kernels accept a new DT blob with a PHY
converted from poll to IRQ mode).

Powered by blists - more mailing lists