On Mon, Jul 14 2025 at 10:41, Wladislav Wiebe wrote: > This patch adds a mechanism to detect and warn about long-running IRQ # git grep 'This patch' Documentation/process/ Also please read: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog > +static int __init irqhandler_duration_check_setup(char *arg) > +{ > + unsigned long val; > + int ret; > + > + if (!arg) > + return 0; > + > + ret = kstrtoul(arg, 0, &val); > + if (ret) > + return ret; > + > + if (val > 0) { > + irqhandler_duration_threshold_us = val; > + static_branch_enable(&irqhandler_duration_check_enabled); > + } else { > + pr_err("Invalid irqhandler.duration_warn_us setting (%lu)\n", val); > + return -EINVAL; > + } > + > + return 0; > +} > +early_param("irqhandler.duration_warn_us", irqhandler_duration_check_setup); Why early_param? Nothing cares about this during early boot. > +static inline void irqhandler_duration_check(u64 ts_start, unsigned int irq, > + struct irqaction *action) > +{ > + u64 delta_us = (local_clock() - ts_start) >> 10; Lacks a comment that this is an intentional approximation. > + if (unlikely(delta_us > irqhandler_duration_threshold_us)) { > + pr_warn_ratelimited("[CPU%d] long duration on IRQ[%u:%ps], took: %llu us\n", > + smp_processor_id(), irq, action->handler, delta_us); Please align the arguments in the second line properly. https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#line-breaks > + } > +} > + > irqreturn_t __handle_irq_event_percpu(struct irq_desc *desc) > { > irqreturn_t retval = IRQ_NONE; > @@ -146,6 +184,7 @@ irqreturn_t __handle_irq_event_percpu(struct irq_desc *desc) > > for_each_action_of_desc(desc, action) { > irqreturn_t res; > + u64 ts_start; This wants to be in the if() branch where it is actually used. > /* > * If this IRQ would be threaded under force_irqthreads, mark it so. > @@ -155,7 +194,14 @@ irqreturn_t __handle_irq_event_percpu(struct irq_desc *desc) > lockdep_hardirq_threaded(); > > trace_irq_handler_entry(irq, action); > - res = action->handler(irq, action->dev_id); > + > + if (static_branch_unlikely(&irqhandler_duration_check_enabled)) { > + ts_start = local_clock(); > + res = action->handler(irq, action->dev_id); > + irqhandler_duration_check(ts_start, irq, action); > + } else > + res = action->handler(irq, action->dev_id); > + Even if not required by C, the else clause wants brackets too for symmetry. if (foo) bar(); else baz(); parses perfectly fine. if (foo) { do_stuff(); bar(); } else baz(); is asymmetrical and disturbs the reading flow, which is pattern based. The extra brackets just make it easier to parse: if (foo) { do_stuff(); bar(); } else { baz(); } See? Thanks, tglx
