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

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

[Samuel Holland]

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.

Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

 drivers/of/property.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 11b922fde7af..7bd8390f2fba 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1213,7 +1213,6 @@ DEFINE_SIMPLE_PROP(iommus, "iommus", "#iommu-cells")
 DEFINE_SIMPLE_PROP(mboxes, "mboxes", "#mbox-cells")
 DEFINE_SIMPLE_PROP(io_channels, "io-channels", "#io-channel-cells")
 DEFINE_SIMPLE_PROP(io_backends, "io-backends", "#io-backend-cells")
-DEFINE_SIMPLE_PROP(interrupt_parent, "interrupt-parent", NULL)
 DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
 DEFINE_SIMPLE_PROP(power_domains, "power-domains", "#power-domain-cells")
 DEFINE_SIMPLE_PROP(hwlocks, "hwlocks", "#hwlock-cells")
@@ -1359,7 +1358,6 @@ static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_mboxes, },
 	{ .parse_prop = parse_io_channels, },
 	{ .parse_prop = parse_io_backends, },
-	{ .parse_prop = parse_interrupt_parent, },
 	{ .parse_prop = parse_dmas, .optional = true, },
 	{ .parse_prop = parse_power_domains, },
 	{ .parse_prop = parse_hwlocks, },
-- 
2.45.1

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

[Saravana Kannan]

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 remember the RISC-V DT for interrupts being a mess. So, want to make
sure you agree with these claims before I Ack it.

Thanks,
Saravana

>
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---
>
>  drivers/of/property.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 11b922fde7af..7bd8390f2fba 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1213,7 +1213,6 @@ DEFINE_SIMPLE_PROP(iommus, "iommus", "#iommu-cells")
>  DEFINE_SIMPLE_PROP(mboxes, "mboxes", "#mbox-cells")
>  DEFINE_SIMPLE_PROP(io_channels, "io-channels", "#io-channel-cells")
>  DEFINE_SIMPLE_PROP(io_backends, "io-backends", "#io-backend-cells")
> -DEFINE_SIMPLE_PROP(interrupt_parent, "interrupt-parent", NULL)
>  DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
>  DEFINE_SIMPLE_PROP(power_domains, "power-domains", "#power-domain-cells")
>  DEFINE_SIMPLE_PROP(hwlocks, "hwlocks", "#hwlock-cells")
> @@ -1359,7 +1358,6 @@ static const struct supplier_bindings of_supplier_bindings[] = {
>         { .parse_prop = parse_mboxes, },
>         { .parse_prop = parse_io_channels, },
>         { .parse_prop = parse_io_backends, },
> -       { .parse_prop = parse_interrupt_parent, },
>         { .parse_prop = parse_dmas, .optional = true, },
>         { .parse_prop = parse_power_domains, },
>         { .parse_prop = parse_hwlocks, },
> --
> 2.45.1
>

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

[Marc Zyngier]

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.

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

[Rob Herring]

On Thu, Nov 14, 2024 at 11:56:49AM -0800, Samuel Holland 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

typo

> 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.
> 
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>

I assume this should go to stable? It needs Fixes tags.

Otherwise, the change makes sense to me.

> ---
> 
>  drivers/of/property.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 11b922fde7af..7bd8390f2fba 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1213,7 +1213,6 @@ DEFINE_SIMPLE_PROP(iommus, "iommus", "#iommu-cells")
>  DEFINE_SIMPLE_PROP(mboxes, "mboxes", "#mbox-cells")
>  DEFINE_SIMPLE_PROP(io_channels, "io-channels", "#io-channel-cells")
>  DEFINE_SIMPLE_PROP(io_backends, "io-backends", "#io-backend-cells")
> -DEFINE_SIMPLE_PROP(interrupt_parent, "interrupt-parent", NULL)
>  DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
>  DEFINE_SIMPLE_PROP(power_domains, "power-domains", "#power-domain-cells")
>  DEFINE_SIMPLE_PROP(hwlocks, "hwlocks", "#hwlock-cells")
> @@ -1359,7 +1358,6 @@ static const struct supplier_bindings of_supplier_bindings[] = {
>  	{ .parse_prop = parse_mboxes, },
>  	{ .parse_prop = parse_io_channels, },
>  	{ .parse_prop = parse_io_backends, },
> -	{ .parse_prop = parse_interrupt_parent, },
>  	{ .parse_prop = parse_dmas, .optional = true, },
>  	{ .parse_prop = parse_power_domains, },
>  	{ .parse_prop = parse_hwlocks, },
> -- 
> 2.45.1
> 

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

[Samuel Holland]

Hi Rob,

On 2024-11-19 9:41 AM, Rob Herring wrote:
> On Thu, Nov 14, 2024 at 11:56:49AM -0800, Samuel Holland 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
> 
> typo
> 
>> 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.
>>
>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> 
> I assume this should go to stable? It needs Fixes tags.

What commit should I put in the Fixes tag? f265f06af194 ("of: property: Fix
fw_devlink handling of interrupts/interrupts-extended"), because it finished
making this code redundant? That commit didn't introduce any new bugs--this code
was always wrong--but I would be hesitant to backport this change any further,
because it might cause regressions without the "interrupts" property parsing in
place.

Regards,
Samuel

> Otherwise, the change makes sense to me.
> 
>> ---
>>
>>  drivers/of/property.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/of/property.c b/drivers/of/property.c
>> index 11b922fde7af..7bd8390f2fba 100644
>> --- a/drivers/of/property.c
>> +++ b/drivers/of/property.c
>> @@ -1213,7 +1213,6 @@ DEFINE_SIMPLE_PROP(iommus, "iommus", "#iommu-cells")
>>  DEFINE_SIMPLE_PROP(mboxes, "mboxes", "#mbox-cells")
>>  DEFINE_SIMPLE_PROP(io_channels, "io-channels", "#io-channel-cells")
>>  DEFINE_SIMPLE_PROP(io_backends, "io-backends", "#io-backend-cells")
>> -DEFINE_SIMPLE_PROP(interrupt_parent, "interrupt-parent", NULL)
>>  DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
>>  DEFINE_SIMPLE_PROP(power_domains, "power-domains", "#power-domain-cells")
>>  DEFINE_SIMPLE_PROP(hwlocks, "hwlocks", "#hwlock-cells")
>> @@ -1359,7 +1358,6 @@ static const struct supplier_bindings of_supplier_bindings[] = {
>>  	{ .parse_prop = parse_mboxes, },
>>  	{ .parse_prop = parse_io_channels, },
>>  	{ .parse_prop = parse_io_backends, },
>> -	{ .parse_prop = parse_interrupt_parent, },
>>  	{ .parse_prop = parse_dmas, .optional = true, },
>>  	{ .parse_prop = parse_power_domains, },
>>  	{ .parse_prop = parse_hwlocks, },
>> -- 
>> 2.45.1
>>

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

[Rob Herring]

On Tue, Nov 19, 2024 at 10:47 AM Samuel Holland
<samuel.holland@sifive.com> wrote:
>
> Hi Rob,
>
> On 2024-11-19 9:41 AM, Rob Herring wrote:
> > On Thu, Nov 14, 2024 at 11:56:49AM -0800, Samuel Holland 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
> >
> > typo
> >
> >> 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.
> >>
> >> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> >
> > I assume this should go to stable? It needs Fixes tags.
>
> What commit should I put in the Fixes tag? f265f06af194 ("of: property: Fix
> fw_devlink handling of interrupts/interrupts-extended"), because it finished
> making this code redundant? That commit didn't introduce any new bugs--this code
> was always wrong--but I would be hesitant to backport this change any further,
> because it might cause regressions without the "interrupts" property parsing in
> place.

I'd guess that f265f06af194 has been backported to everything with
7f00be96f125. I think we want either all 3 commits or none of them. If
something only works with a subset, then upstream is broken.

Rob

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

[Samuel Holland]

On 2024-11-19 11:49 AM, Rob Herring wrote:
> On Tue, Nov 19, 2024 at 10:47 AM Samuel Holland
> <samuel.holland@sifive.com> wrote:
>>
>> Hi Rob,
>>
>> On 2024-11-19 9:41 AM, Rob Herring wrote:
>>> On Thu, Nov 14, 2024 at 11:56:49AM -0800, Samuel Holland 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
>>>
>>> typo
>>>
>>>> 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.
>>>>
>>>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>>>
>>> I assume this should go to stable? It needs Fixes tags.
>>
>> What commit should I put in the Fixes tag? f265f06af194 ("of: property: Fix
>> fw_devlink handling of interrupts/interrupts-extended"), because it finished
>> making this code redundant? That commit didn't introduce any new bugs--this code
>> was always wrong--but I would be hesitant to backport this change any further,
>> because it might cause regressions without the "interrupts" property parsing in
>> place.
> 
> I'd guess that f265f06af194 has been backported to everything with
> 7f00be96f125. I think we want either all 3 commits or none of them. If
> something only works with a subset, then upstream is broken.

Of the current LTS branches, v5.10 has only 7f00be96f125. f265f06af194 was only
backported as far as v5.15, since it Fixes: 4104ca776ba3 ("of: property: Add
fw_devlink support for interrupts"), which was merged for v5.12. I will add this
same Fixes: tag for v2.

Regards,
Samuel