[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZtbA5Adh2acTExYq@den-build>
Date: Tue, 3 Sep 2024 15:55:16 +0800
From: Richard Clark <richard.xnu.clark@...il.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
torvalds@...ux-foundation.org
Subject: Re: [PATCH] irq: fix the interrupt trigger type override issue
Hi Thomas,
On Mon, Sep 02, 2024 at 04:39:33PM +0200, Thomas Gleixner wrote:
> Richard!
>
> On Mon, Sep 02 2024 at 20:50, richard clark wrote:
> > On Mon, Sep 2, 2024 at 5:51 PM Thomas Gleixner <tglx@...utronix.de> wrote:
> >> >> 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.
>
> So the mapping and the interrupt allocation persist even if nothing uses
> them. What a waste.
>
I checked the code and found that it's not the kernel to create the mapping,
it's by the driver calling platform_get_irq(...)/of_irq_get(...) to create.
>
> >> > 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...
>
> Fair enough. Though the logic in that map code is convoluted as hell and
> making it more convoluted does not really make it better.
>
> So let's look at how this works (or not):
>
> 1)
> map()
> allocate_irq();
> set_trigger_type(FW);
>
> 2)
> request_irq(type = DRV);
> set_trigger_type(DRV);
>
> 3)
> free_irq();
> // type is not reset to FW
>
> 4)
> map()
> irq_trigger_type() != NONE && != FW
> -> fail
>
> This results in a pile of questions:
>
> Why does #2 override the firmware, if the firmware defines a trigger
> type != NONE?
>
> Isn't the whole point of firmware defining the type that the driver
> does not need to care?
>
> If the firmware cannot does not know what the right thing is then it
> should say so by using type NONE and the type is using the hardware
> or interrupt driver default.
>
> That aside. What you are trying to do only works when #2 and #4 are
> coming from the exactly same context.
>
> What it does not catch is when the interrupt line is shared and there
> are two drivers where the first one fiddles with type on request_irq()
> and the second one relies on the firmware to do the right thing.
>
> Same problem if you unload the driver and remove the type from
> request_irq() flags because you figured out that this was bogus. Then
> you end up with the old setting when you load the recompiled driver
> again.
>
> That's all inconsistent. The proper solution would be to restore the
> firmware setting in free_irq() when the last action goes away.
>
> The question is whether it's worth the trouble. If not then we can just
> make all of this simple, trivial and incomplete instead of making it
> more complex and differently incomplete.
>
Ah, the mapping is created from of_irq_get(...) by driver, the kernel also
provides the mapping teardown interface - irq_dispose_mapping.
IMO, the right way for the driver is:
1) driver calls of_irq_get() to get the irq and create the mapping
2) driver *should* call irq_dispose_mapping() as the teardown of step 1.
3) free_irq is the teardown of the request_irq to free the irq and
its action.
So the original issue should be the bug of the driver not calling the
irq_dispose_mapping to release the mapping(and being persistent there).
The error message will not show if irq_dispose_mapping(...) called by
the driver.
Looks like the current implementation is correct, but I don't know if it's true
for the shared irq...
Thanks!
>
> Thanks,
>
> tglx
> ---
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -895,32 +895,14 @@ unsigned int irq_create_fwspec_mapping(s
> */
> virq = irq_find_mapping(domain, hwirq);
> if (virq) {
> - /*
> - * If the trigger type is not specified or matches the
> - * current trigger type then we are done so return the
> - * interrupt number.
> - */
> - if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq))
> - goto out;
> -
> - /*
> - * If the trigger type has not been set yet, then set
> - * it now and return the interrupt number.
> - */
> - if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
> + /* Preserve the trigger type set by request_irq() */
> + if (type != IRQ_TYPE_NONE && irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
> irq_data = irq_get_irq_data(virq);
> - if (!irq_data) {
> + if (irq_data)
> + irqd_set_trigger_type(irq_data, type);
> + else
> virq = 0;
> - goto out;
> - }
> -
> - irqd_set_trigger_type(irq_data, type);
> - goto out;
> }
> -
> - pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
> - hwirq, of_node_full_name(to_of_node(fwspec->fwnode)));
> - virq = 0;
> goto out;
> }
>
>
>
Powered by blists - more mailing lists