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

[richard clark]

Hi Thomas,

On Mon, Sep 2, 2024 at 5:51 PM Thomas Gleixner <tglx at linutronix.de> wrote:
>
> Richard!
>
> On Mon, Sep 02 2024 at 16:42, richard clark wrote:
> > On Mon, Sep 2, 2024 at 3:34 PM Thomas Gleixner <tglx at linutronix.de> wrote:
> >> 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.
>
> So what creates the mapping? If the driver creates it then why doesn't
> it unmap it when it exits?
>
Kernel allocates an irq and creates the mapping during its
initialization when parsing the device's interrupt firmware, not the
driver does that.
>
> >> 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:
>
> That's exactly the question. Why does the mapping persist?
>
AFAICT the kernel creates the mapping to bind the allocated irq to its
firmware node, thus this {key, value} pair can be used persistently in
the future while not re-iterate the cycle again. irq and the mapping
can be regarded as the part of the kernel, not the scope of the
driver.
>
> >> 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.
>
> I can see how that can happen, but what's missing is the information why
> this mapping persists and why it's tried to be set up again.
>
As I mentioned, it doesn't try to set up again. It will just lookup
the mapping from the tree for the persistent reason when driver try to
request the irq...

Thanks,
>
> Thanks,
>
>         tglx