Re: [PATCH v6 4/5] clocksource/timer-econet-en751221: Disable IRQ until cevt registered

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Incorrect assumption that clockevents_config_and_register() can fail.
- [Low] Unnecessary manual setting of IRQ_NOAUTOEN flag.
--

commit f75e71c2c11de4c792309a75e268796b0c4679bf
Author: Caleb James DeLisle <cjd@cjdns.fr>
clocksource/timer-econet-en751221: Disable IRQ until cevt registered

This commit addresses a race condition where the timer interrupt may
trigger before clockevents_config_and_register() sets up the event
handler. It resolves this by enabling the per-CPU IRQ only after the
clockevent device is fully registered and adds a NULL check for the
event handler.

> Eliminate a race condition where cevt_interrupt may trigger before
> clockevents_config_and_register has been called, and dev->event_handler
> is at that point NULL. Additionally, NULL check dev->event_handler in
> interrupt callback just in case clockevents_config_and_register failed.

[Severity: Medium]
Is the reasoning about clockevents_config_and_register() failing accurate
here? 

Looking at the implementation of clockevents_config_and_register(), it
returns void and does not have a failure path.

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c
> @@ -75,7 +75,10 @@ static irqreturn_t cevt_interrupt(int irq, void *dev_id)
>  		return IRQ_NONE;
>  
>  	iowrite32(ioread32(reg_count(cpu)), reg_compare(cpu));
> -	dev->event_handler(dev);
> +
> +	if (dev->event_handler)
> +		dev->event_handler(dev);
> +
>  	return IRQ_HANDLED;
>  }

[Severity: Medium]
Is this NULL check necessary? 

Because clockevents_register_device() unconditionally initializes the event
handler to a valid function (such as clockevents_handle_noop),
dev->event_handler should never be NULL once registration is complete.

[ ... ]

> @@ -178,6 +180,8 @@ static int __init timer_init(struct device_node *np)
>  		goto out_membase;
>  	}
>  
> +	irq_set_status_flags(econet_timer.irq, IRQ_NOAUTOEN);
> +
>  	ret = request_percpu_irq(econet_timer.irq, cevt_interrupt, np->name,
>  				 &econet_timer_pcpu);

[Severity: Low]
Is this manual status flag update needed? 

When an IRQ is configured as a per-CPU devid interrupt by the irqchip driver
via irq_set_percpu_devid(), the IRQ_NOAUTOEN flag is inherently applied
alongside IRQ_PER_CPU and IRQ_NOTHREAD. Setting it manually here appears
redundant.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528140046.2897674-1-cjd@cjdns.fr?part=4