| Message ID | ZtUuYXSq8g2Mphuq@den-build (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | irq: fix the interrupt trigger type override issue | expand |
Richard! On Mon, Sep 02 2024 at 11:17, Richard Clark wrote: The subsystem prefix is 'irqdomain' not 'irq' # 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 3) insmod() Should be exactly the same as #1 because the previous mapping was torn down 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 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. Thanks, tglx
Hi Thomas, On Mon, Sep 2, 2024 at 3:34 PM Thomas Gleixner <tglx@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
Richard! On Mon, Sep 02 2024 at 16:42, richard clark wrote: > On Mon, Sep 2, 2024 at 3:34 PM Thomas Gleixner <tglx@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? >> 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? >> 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. Thanks, tglx
Hi Thomas, On Mon, Sep 2, 2024 at 5:51 PM Thomas Gleixner <tglx@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@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
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; }
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; > } > > >
Richard! On Tue, Sep 03 2024 at 15:55, Richard Clark wrote: > On Mon, Sep 02, 2024 at 04:39:33PM +0200, Thomas Gleixner wrote: >> 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. :) > 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. Correct. Thanks, tglx
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index cea8f6874b1f..fb0be8e73c5b 100644 --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -895,25 +895,25 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) */ virq = irq_find_mapping(domain, hwirq); if (virq) { + irq_data = irq_get_irq_data(virq); + if (!irq_data) { + virq = 0; + goto out; + } + /* * If the trigger type is not specified or matches the - * current trigger type then we are done so return the - * interrupt number. + * current trigger type or has been set previously then we are done so + * return the interrupt number. */ - if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq)) + if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq) || + irqd_trigger_type_was_set(irq_data)) 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) { - irq_data = irq_get_irq_data(virq); - if (!irq_data) { - virq = 0; - goto out; - } - irqd_set_trigger_type(irq_data, type); goto out; }
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! 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: the irq_create_fwspec_mapping(...) will return the interrupt number directly if the trigger type has been set previously. Signed-off-by: Richard Clark <richard.xnu.clark@gmail.com> --- kernel/irq/irqdomain.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)