[PATCH] irq: fix the interrupt trigger type override issue

[richard clark]

Hi Thomas,

On Mon, Sep 2, 2024 at 3:34 PM Thomas Gleixner <tglx at linutronix.de> wrote:
>
> Richard!
>
> On Mon, Sep 02 2024 at 11:17, Richard Clark wrote:
>
> The subsystem prefix is 'irqdomain' not 'irq'
>
Right, will correct it.
>
> # git log --online $FILE
>
> gives you a decent hint.
>
> > In current implementation, the trigger type in 'flags' when calling request_irq
> > will override the type value get from the firmware(dt/acpi node) if they are
> > not consistent, and the overrided trigger type value will be retained by irq_data,
> > consequently the type value get from the firmware will not match the retained one
> > next time in case the virq is available.
> >
> > Thus below error message will be observed by the __2nd__ 'insmod' within the
> > 'insmod - rmmod - insmod' operation sequence for the same device driver kernel
> > module, in which request_irq(..., IRQ_TYPE_LEVEL_HIGH, ...) is used:
> >
> >       irq: type mismatch, failed to map hwirq-182 for interrupt-controller!
>
> How so?
>
> 1) insmod()
>      irq_create_fwspec_mapping(fwspec)
>        irq_domain_translate(fwspec, ... &type); <- Sets type to the FW value
>
>        virq = irq_find_mapping(domain, hwirq);
>        if (virq) {
>          // Path not taken
>        }
>
>        // Map interrupt
>        ...
>
>        irqd_set_trigger_type(..., type);
>
> 2) rmmod()
>      tears down mapping
>
This just tears down the action allocated and installed by
request_irq(...), but does not teardown the irq's node inserted in the
revmap_tree.
>
> 3) insmod()
>
>       Should be exactly the same as #1 because the previous mapping was
>       torn down by rmmod()
>
Not the same exactly, the {irq, irq_data} will still be in the
revmap_tree, so it will enter another path in this case:
        virq = irq_find_mapping(domain, hwirq);
        if (virq) {
               // will be in this path for the next 'insmod' since the
{virq, irq_data} not be teardowned by free_irq(...) called by rmmod
       }

>
> Even if the first mapping is not torn down by rmmod(), which is a bug in
> itself, then the type is exactly the same as the firmware describes it, no?
>
> So how exactly does that happen what you describe?
>
The logic is if the trigger type specified by request_irq(...) is not
consistent with the firmware one, the request_irq will override the
FW. We need to keep this logic the same as when we insmod the same
kmod next time -- override the FW's too instead of returning a
mismatch type error.
>
> > The corresponding 'interrupts' property of that device node is:
> >       interrupts = <0 150 1>;
> >
> > This commit fixes the above issue by adding a new checker -
> > irqd_trigger_type_was_set:
>
> This commit is equaly redundant as 'This patch'
>
> # git grep 'This patch' Documentation/process/
>
> Also 'new checker' is not really a technical term.
>
Ah, will correct it. Thanks!
>
> Thanks,
>
>         tglx