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

[Richard Clark <richard.xnu.clark@gmail.com>]

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@linutronix.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;
>  	}
>  
> 
>