Re: [PATCH v1] irqchip/irq-mvebu-icu: Fix irq_set_type for sei and nsr

Stefan Eichenberger
2025-01-16T17:12:49Z

Hi Thomas,

On Wed, Jan 15, 2025 at 09:15:24AM +0100, Thomas Gleixner wrote:
> On Tue, Dec 17 2024 at 12:15, Stefan Eichenberger wrote:
> > A regression was introduced in commit d929e4db22b6
> > ("irqchip/irq-mvebu-icu: Prepare for real per device MSI") that causes
> > the Armada thermal driver to fail during probe with the following error:
> > genirq: Setting trigger mode 4 for irq 85 failed (irq_chip_set_type_parent+0x0/0x34)
> > armada_thermal f2400000.system-controller:thermal-sensor@70: Cannot request threaded IRQ 85
> > armada_thermal f2400000.system-controller:thermal-sensor@70: probe with driver armada_thermal failed with error -22
> >
> > The issue occurs because irq_set_type is assigned to
> > irq_chip_set_type_parent, but the parent IRQ chip does not implement the
> > irq_set_type operation. This causes the trigger mode configuration to
> > fail.
> >
> > This patch resolves the issue by removing the irq_set_type assignment.
> > With no irq_set_type, __irq_set_trigger safely skips the trigger
> > configuration, restoring functionality to the thermal driver.
>
> I'm not convinced that this is correct.
>
> The original code had irq_chip_set_type_parent() for the NSR/SEI chips
> too and all what d929e4db22b6 does is to convert those chips over to the
> new platform MSI mechanism. Here are the original chips:
>
> static struct irq_chip mvebu_icu_nsr_chip = {
> .name = "ICU-NSR",
> ...
> .irq_set_type = irq_chip_set_type_parent,
> ...
> };
>
> static struct irq_chip mvebu_icu_sei_chip = {
> .name = "ICU-SEI",
> ...
> .irq_set_type = irq_chip_set_type_parent,
> ...
> };
>
> And looking at the potential platform MSI providers for MVEBU, then it
> turns out that GICP and SEI both have the irq_set_type() callback
> populated, though ODMI has not. So either this has never worked or there
> is something else fishy.
>
> Can you please enable CONFIG_GENERIC_IRQ_DEBUGFS, build/boot a 6.10
> kernel and provide the output of
>
> cat /sys/kernel/debug/irq/irq/$N
>
> where $N is the interrupt number of the thermal sensor.
>
> Then provide the same information for a current kernel with your patch
> applied.

You are right I somehow didn't look back far enough. I tested once with
kernel 6.12.5 and my patch applied:
root@localhost:~# uname -a
Linux localhost.localdomain 6.12.5+ #157 SMP PREEMPT Thu Jan 16 17:32:21 CET 2025 aarch64 GNU/Linux
root@localhost:~# cat /proc/interrupts |grep thermal
35: 0 0 0 0 AP SEI 18 Level f06f8000.system-controller:thermal-sensor@80
90: 0 0 0 0 SEI-ICU-SEI-f21e0000.interrupt-controller:inter 116 Edge f2400000.system-controller:thermal-sensor@70
91: 0 0 0 0 SEI-ICU-SEI-f61e0000.interrupt-controller:inter 116 Edge f6400000.system-controller:thermal-sensor@70
root@localhost:~# cat /sys/kernel/debug/irq/irqs/90
handler: handle_edge_irq
device: f21e0000.interrupt-controller:interrupt-controller@50
status: 0x00000000
istate: 0x00004000
ddepth: 0
wdepth: 0
dstate: 0x02400204
IRQ_TYPE_LEVEL_HIGH
IRQD_ACTIVATED
IRQD_IRQ_STARTED
IRQD_DEFAULT_TRIGGER_SET
node: -1
affinity: 0-3
effectiv:
domain: :cp0:config-space@f2000000:interrupt-controller@1e0000:interrupt-controller@50-16
hwirq: 0x74
chip: SEI-ICU-SEI-f21e0000.interrupt-controller:inter
flags: 0x80
IRQCHIP_SUPPORTS_LEVEL_MSI
parent:
domain: :ap807:config-space@f0000000:interrupt-controller@3f0200-2
hwirq: 0x0
chip: CP SEI
flags: 0x0
parent:
domain: :ap807:config-space@f0000000:interrupt-controller@3f0200-5
hwirq: 0x15
chip: SEI
flags: 0x0

And then with kernel 6.10.14 without any patches:
root@localhost:~# uname -a
Linux localhost.localdomain 6.10.14 #1 SMP PREEMPT Thu Jan 16 17:52:59 CET 2025 aarch64 GNU/Linux
root@localhost:~# cat /proc/interrupts |grep thermal
35: 0 0 0 0 AP SEI 18 Level f06f8000.system-controller:thermal-sensor@80
90: 0 0 0 0 ICU-SEI 116 Edge f2400000.system-controller:thermal-sensor@70
91: 0 0 0 0 ICU-SEI 116 Edge f6400000.system-controller:thermal-sensor@70
root@localhost:~# cat /sys/kernel/debug/irq/irqs/90
handler: handle_edge_irq
device: (null)
status: 0x00000001
istate: 0x00004000
ddepth: 0
wdepth: 0
dstate: 0x02400201
IRQ_TYPE_EDGE_RISING
IRQD_ACTIVATED
IRQD_IRQ_STARTED
IRQD_DEFAULT_TRIGGER_SET
node: -1
affinity: 0-3
effectiv:
domain: :cp0:config-space@f2000000:interrupt-controller@1e0000:interrupt-controller@50
hwirq: 0x74
chip: ICU-SEI
flags: 0x0
parent:
domain: :ap807:config-space@f0000000:interrupt-controller@3f0200-4
hwirq: 0x505a
chip: SEI pMSI
flags: 0x0
parent:
domain: :ap807:config-space@f0000000:interrupt-controller@3f0200-2
hwirq: 0x0
chip: CP SEI
flags: 0x0
parent:
domain: :ap807:config-space@f0000000:interrupt-controller@3f0200-5
hwirq: 0x15
chip: SEI
flags: 0x0

It seems with kernel 6.10 the controller device was not set correctly,
probably it was ignoring irq_set_type because of this. Do you by chance
have an idea how to properly fix this or should I do some more research?

Thanks and regards,
Stefan