Re: [PATCH] of: property: fw_devlink: Do not use interrupt-parent directly

[Marc Zyngier <maz@kernel.org>]

On Thu, 14 Nov 2024 20:24:34 +0000,
Saravana Kannan <saravanak@google.com> wrote:
> 
> On Thu, Nov 14, 2024 at 11:56 AM Samuel Holland
> <samuel.holland@sifive.com> wrote:
> >
> > commit 7f00be96f125 ("of: property: Add device link support for
> > interrupt-parent, dmas and -gpio(s)") started adding device links for
> > the interrupt-parent property. Later, commit f265f06af194 ("of:
> > property: Fix fw_devlink handling of interrupts/interrupts-extended")
> > added full support for parsing the interrupts and interrupts-extended
> > properties, which includes looking up the node of the parent domain.
> > This made the handler for the interrupt-parent property redundant.
> >
> > In fact, creating device links based solely on interrupt-parent is
> > problematic, because it can create spurious cycles. A node may have
> > this property without itself being an interrupt controller or consumer.
> > For example, this property is often present in the root node or a /soc
> > bus node to set the default interrupt parent for child nodes. However,
> > it is incorrect for the bus to depend on the interrupt controller, as
> > some of the bus's childre may not be interrupt consumers at all or may
> > have a different interrupt parent.
> >
> > Resolving these spurious dependency cycles can cause an incorrect probe
> > order for interrupt controller drivers. This was observed on a RISC-V
> > system with both an APLIC and IMSIC under /soc, where interrupt-parent
> > in /soc points to the APLIC, and the APLIC msi-parent points to the
> > IMSIC. fw_devlink found three dependency cycles and attempted to probe
> > the APLIC before the IMSIC. After applying this patch, there were no
> > dependency cycles and the probe order was correct.
> 
> Rob/Marc,
> 
> If the claim about the interrupt parent interpretation is correct
> across the board, I'm ok with this patch.

I agree with Samuel's analysis that "interrupt-parent" is not always
relevant to unsuspecting devices, given that it is often inherited
from a bus-wide or system-wide parent node. Collectively, "interrupts"
(which implicitly uses "interrupt-parent"), "interrupt-extended" and
"interrupt-map" should provide enough information to ensure correct
dependency tracking.

This is a notable departure from "msi-parent", which itself doesn't
require any extra specifier, and cannot be ignored here (I note the
absence of "msi-map" tracking though).

> I remember the RISC-V DT for interrupts being a mess. So, want to make

I'm afraid other architectures are not any better. RISC-V only has the
dubious advantage of coming up with backward designs, which doesn't
help.

> sure you agree with these claims before I Ack it.

This needs testing, but looks sane from my PoV (though it is early and
I need single coffee)... FWIW:

Acked-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.