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

[Thomas Gleixner <tglx@linutronix.de>]

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.

>> > 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.

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;
 	}