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

On Thu, Jan 16, 2025 at 10:05:25PM +0100, Thomas Gleixner wrote:

Hi!

On Thu, Jan 16 2025 at 18:12, Stefan Eichenberger wrote:

quoted

On Wed, Jan 15, 2025 at 09:15:24AM +0100, Thomas Gleixner wrote:

quoted

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:

quoted

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

Ok. That explains it. CP SEI has:

static int mvebu_sei_cp_set_type(struct irq_data *data, unsigned int type)
{
        if ((type & IRQ_TYPE_SENSE_MASK) != IRQ_TYPE_EDGE_RISING)
                return -EINVAL;
        return 0;
}                                                                        

But the DT configuration requests:

quoted

            IRQ_TYPE_LEVEL_HIGH

  IRQ_TYPE_LEVEL_HIGH     = 0x00000004,

quoted

 genirq: Setting trigger mode 4 for irq 85 failed (irq_chip_set_type_parent+0x0/0x34)

which is not supported by CP SEI.

With your patch the type request is just stored in the data, but no
actual type setting happens. That's default behaviour for chips which do
not have a set_type() callback.

quoted

root@localhost:~# cat /sys/kernel/debug/irq/irqs/90
handler:  handle_edge_irq
device:   (null)

It seems with kernel 6.10 the controller device was not set correctly,
probably it was ignoring irq_set_type because of this. 

No. That's not related.

quoted

Do you by chance have an idea how to properly fix this or should I do
some more research?

It's unclear to me how the 6.10 kernel survives that. Do you have a
different device tree for those kernels?

I used the same device tree for the 6.10 kernel as I did for kernel
6.12. I didn't even recompile it. Unfortunately, I couldn't figure out
what exactly is different so far. However, on the 6.10 kernel I got the
following trace:
root@localhost:/sys/kernel/tracing# cat trace
# tracer: function_graph
#
# CPU  DURATION                  FUNCTION CALLS
# |     |   |                     |   |   |   |
 1)               |  devm_request_threaded_irq() {
 1)               |    request_threaded_irq() {
 0)               |      __irq_set_trigger() {
 0)               |        irq_chip_set_type_parent() {
 0)               |          irq_chip_set_type_parent() {
 0)   2.560 us    |            mvebu_sei_cp_set_type(); /* = 0x0 */
 0)   8.640 us    |          } /* irq_chip_set_type_parent = 0x0 */
 0) + 14.400 us   |        } /* irq_chip_set_type_parent = 0x0 */
 0) + 24.320 us   |      } /* __irq_set_trigger = 0x0 */
 0) * 13601.60 us |    } /* request_threaded_irq = 0x0 */
 0) * 13617.44 us |  } /* devm_request_threaded_irq = 0x0 */

It seems that irq_chip_set_type_parent is not failing. By adding some
debug messages to kernel/irq/manage.c I found that irqd_get_trigger_type
returns IRQF_TRIGGER_RISING even though it should return
IRQF_TRIGGER_HIGH according to the device tree. Maybe this was fixed
between 6.10 and 6.12 but I need to analyze that again in more detail.
[   91.069407] genirq: __setup_irq - kernel/irq/manage.c:1530, flags: 0x0
[   91.076042] genirq: __setup_irq - kernel/irq/manage.c:1533, flags: 0x1

For a possible solution, should I just change the device tree so that
IRQF_TRIGGER_RISING is used instead of IRQF_TRIGGER_HIGH?

Regards,
Stefan