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

[Stefan Eichenberger <eichest@gmail.com>]

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:
> > On Wed, Jan 15, 2025 at 09:15:24AM +0100, Thomas Gleixner wrote:
> >> 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
> 
> 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:
> 
> >             IRQ_TYPE_LEVEL_HIGH
> 
>   IRQ_TYPE_LEVEL_HIGH     = 0x00000004,
> 
> >  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.
> 
> > 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.
> 
> > 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