All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH devicetree] of: property: mark "interrupts" as optional for fw_devlink
@ 2022-05-13 20:12 Vladimir Oltean
  2022-05-14  0:26 ` Saravana Kannan
  0 siblings, 1 reply; 5+ messages in thread
From: Vladimir Oltean @ 2022-05-13 20:12 UTC (permalink / raw)
  To: devicetree, linux-kernel
  Cc: netdev, Saravana Kannan, Greg Kroah-Hartman, Rafael J. Wysocki,
	Robin Murphy, Geert Uytterhoeven, Rob Herring, Frank Rowand,
	John Stultz, Alvin Šipraga, Florian Fainelli, Andrew Lunn,
	Russell King, Heiner Kallweit

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.

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".

So practically speaking, interrupts are now not "handled as optional",
but rather "not handled" by fw_devlink. This has quite wide ranging
side effects, 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@nxp.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"").
- Until the PHY probes, the Ethernet controller may call
  phylink_fwnode_phy_connect() -> fwnode_phy_find_device(), and this
  will return NULL with an unspecified reason. This needs to be patched
  to return -EPROBE_DEFER instead of -ENODEV until
  driver_deferred_probe_check_state() says otherwise
- Even so, some drivers like DSA treat PHY connection errors as "soft"
  and continue probing. This is problematic because an -EPROBE_DEFER
  coming from the PHY will result in a missing net_device. What we want
  is to fix the backpressure all the way to the Ethernet controller
  probing.

This is to say, don't expect that all things start working just with
this single change. I'm copying some Ethernet driver maintainers as a
heads up about this fact, and my plan to address the other issues until
the above works.

 drivers/of/property.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 8e90071de6ed..a9ceb02e00d9 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1393,7 +1393,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_leds, },
 	{ .parse_prop = parse_backlight, },
 	{ .parse_prop = parse_gpio_compat, },
-	{ .parse_prop = parse_interrupts, },
+	{ .parse_prop = parse_interrupts, .optional = true, },
 	{ .parse_prop = parse_regulators, },
 	{ .parse_prop = parse_gpio, },
 	{ .parse_prop = parse_gpios, },
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [RFC PATCH devicetree] of: property: mark "interrupts" as optional for fw_devlink
  2022-05-13 20:12 [RFC PATCH devicetree] of: property: mark "interrupts" as optional for fw_devlink Vladimir Oltean
@ 2022-05-14  0:26 ` Saravana Kannan
  2022-05-19 15:15   ` Vladimir Oltean
  0 siblings, 1 reply; 5+ messages in thread
From: Saravana Kannan @ 2022-05-14  0:26 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: devicetree, linux-kernel, netdev, Greg Kroah-Hartman,
	Rafael J. Wysocki, Robin Murphy, Geert Uytterhoeven, Rob Herring,
	Frank Rowand, John Stultz, Alvin Šipraga, Florian Fainelli,
	Andrew Lunn, Russell King, Heiner Kallweit

On Fri, May 13, 2022 at 1:13 PM Vladimir Oltean <vladimir.oltean@nxp.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.

> 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. With that
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.

> 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?

> 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@nxp.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.

-Saravana

> - Until the PHY probes, the Ethernet controller may call
>   phylink_fwnode_phy_connect() -> fwnode_phy_find_device(), and this
>   will return NULL with an unspecified reason. This needs to be patched
>   to return -EPROBE_DEFER instead of -ENODEV until
>   driver_deferred_probe_check_state() says otherwise
> - Even so, some drivers like DSA treat PHY connection errors as "soft"
>   and continue probing. This is problematic because an -EPROBE_DEFER
>   coming from the PHY will result in a missing net_device. What we want
>   is to fix the backpressure all the way to the Ethernet controller
>   probing.
>
> This is to say, don't expect that all things start working just with
> this single change. I'm copying some Ethernet driver maintainers as a
> heads up about this fact, and my plan to address the other issues until
> the above works.
>
>  drivers/of/property.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 8e90071de6ed..a9ceb02e00d9 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1393,7 +1393,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
>         { .parse_prop = parse_leds, },
>         { .parse_prop = parse_backlight, },
>         { .parse_prop = parse_gpio_compat, },
> -       { .parse_prop = parse_interrupts, },
> +       { .parse_prop = parse_interrupts, .optional = true, },
>         { .parse_prop = parse_regulators, },
>         { .parse_prop = parse_gpio, },
>         { .parse_prop = parse_gpios, },
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC PATCH devicetree] of: property: mark "interrupts" as optional for fw_devlink
  2022-05-14  0:26 ` Saravana Kannan
@ 2022-05-19 15:15   ` Vladimir Oltean
  2022-05-19 20:00     ` Saravana Kannan
  0 siblings, 1 reply; 5+ messages in thread
From: Vladimir Oltean @ 2022-05-19 15:15 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: devicetree, linux-kernel, netdev, Greg Kroah-Hartman,
	Rafael J. Wysocki, Robin Murphy, Geert Uytterhoeven, Rob Herring,
	Frank Rowand, John Stultz, Alvin Šipraga,
	Florian Fainelli, Andrew Lunn, Russell King, Heiner Kallweit

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@nxp.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@nxp.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).

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC PATCH devicetree] of: property: mark "interrupts" as optional for fw_devlink
  2022-05-19 15:15   ` Vladimir Oltean
@ 2022-05-19 20:00     ` Saravana Kannan
  2022-05-20  7:02       ` Geert Uytterhoeven
  0 siblings, 1 reply; 5+ messages in thread
From: Saravana Kannan @ 2022-05-19 20:00 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: devicetree, linux-kernel, netdev, Greg Kroah-Hartman,
	Rafael J. Wysocki, Robin Murphy, Geert Uytterhoeven, Rob Herring,
	Frank Rowand, John Stultz, Alvin Šipraga,
	Florian Fainelli, Andrew Lunn, Russell King, Heiner Kallweit,
	Android Kernel Team

On Thu, May 19, 2022 at 8:15 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> 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@nxp.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.

My fix would be different so as to not break when you have more than 1
matching driver but the 1st one fails the probe. But ignoring that for
a moment, wouldn't you want the result you got?

If fw_devlink_relax_link() skips a link, it's because:
1. The link was also requested/created by the driver/framework, so
it's not fw_devlink's place to ignore it.
2. The link is already a type that won't block probing.

Anyway I'll send out a patch for "if all the drivers that match a
device fails to probe it" then don't block probing of its consumers
AFTER the timeout. We can't ignore it before the timeout though
because a better driver might be loaded soon.

>
> > > 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").

Then stop doing it :P

> > 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.

Once the patch I listed below settles down without issues, I might go
ahead and just delete this optional flag thing. Because the other
fw_devlink logic should cover those cases too.

There's still work to be done that might make this easier/cleaner in the future:
1. Adding a DT property that explicitly marks device A as not
dependent on B (Rob was already open to this -- with additional
context I don't want to type up here).
2. Adding kernel command line options that might allow people to say
stuff like "Device A doesn't depend on Device B independent of what DT
might say".

> > > 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,

I was being very conservating with 10s. For my test set up in a Pixel
6, I think something like 5s or even 3s worked fine (no device links
that would have worked fine were dropped). So maybe try a smaller
number for your case?

> 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.

I'm not sure I understand the concern. With my patch merged, if you
jump to a new kernel, the old DT should still work because of the 10s
timeout. If you add a new DT with new dependencies (but no driver for
them), it'll still work.

Btw, my patch is helping other instances of missing drivers outside of
your issue too. It's just a general "after most drivers are loaded you
can take a chill pill fw_devlink" patch.

> > > 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@nxp.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 honestly don't think it's self-defeating in its current state with
the timeout patch. It tries to enforce as many dependencies as it can
and then gets out of the way. I also think fw_devlink support for all
DT dependency bindings is not that far away from being complete. I
trawled through all the DT bindings several months back and didn't
find that many remaining DT properties that need to be added.

> 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.

Well, the parent is deferring the probe because it's waiting for the
child devices to probe successfully before returning. That's not a
guarantee driver core makes, so it's not on the top of the list to fix
as long as fw_devlink doesn't block the probes (it doesn't). Is the
"won't hide the -EPROBE_DEFER from phylink anymore" a serious problem
for you? If so, I can try to fix that for you.

Seriously, the biggest blocker right now is the use of
device_bind_driver() in the networking code. And that's on my top
priority of things to fix whenever I get a few days of uninterrupted
time to work on upstream stuff.

> 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).

With the timeout patch, you have this right? Maybe + my patch to deal
with the driver returning error case (I'll send that out soon).

-Saravana

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC PATCH devicetree] of: property: mark "interrupts" as optional for fw_devlink
  2022-05-19 20:00     ` Saravana Kannan
@ 2022-05-20  7:02       ` Geert Uytterhoeven
  0 siblings, 0 replies; 5+ messages in thread
From: Geert Uytterhoeven @ 2022-05-20  7:02 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Vladimir Oltean, devicetree, linux-kernel, netdev,
	Greg Kroah-Hartman, Rafael J. Wysocki, Robin Murphy,
	Geert Uytterhoeven, Rob Herring, Frank Rowand, John Stultz,
	Alvin Šipraga, Florian Fainelli, Andrew Lunn,
	Russell King, Heiner Kallweit, Android Kernel Team

Hi Saravana,

On Thu, May 19, 2022 at 10:00 PM Saravana Kannan <saravanak@google.com> wrote:
> There's still work to be done that might make this easier/cleaner in the future:
> 1. Adding a DT property that explicitly marks device A as not
> dependent on B (Rob was already open to this -- with additional
> context I don't want to type up here).
> 2. Adding kernel command line options that might allow people to say
> stuff like "Device A doesn't depend on Device B independent of what DT
> might say".

There are clearly cases where the hardware defines if the property
is optional or required, and a DT property would work.  However, in
general relaxing such dependencies involves a complex mix of hardware
capabilities, driver support, and system policies.
Examples:
  - A hardware block may support both DMA and PIO, or can require DMA,
  - DMA-capable devices can typically work without an IOMMU,
    unless all RAM lies outside the address space addressable by
    the DMA controller,
  - A driver may fall back to PIO if DMA is not available (yet),
    but doing so may not meet the required performance target,
  - Not using the IOMMU may violate anti-tampering policies.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-05-20  7:03 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-13 20:12 [RFC PATCH devicetree] of: property: mark "interrupts" as optional for fw_devlink Vladimir Oltean
2022-05-14  0:26 ` Saravana Kannan
2022-05-19 15:15   ` Vladimir Oltean
2022-05-19 20:00     ` Saravana Kannan
2022-05-20  7:02       ` Geert Uytterhoeven

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.