Re: [PATCH 4/9] watchdog: keembay: Remove redundant dev_err_probe()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Race condition in probe leading to a NULL pointer dereference in the interrupt handler.
- [High] Watchdog clock may be disabled by the Common Clock Framework (CCF) late initcall, silently stopping the watchdog.
--

--- Patch [4]: [PATCH 4/9] watchdog: keembay: Remove redundant dev_err_probe() ---
commit ae07b07078ab54b39721c2064f0c148ab624e578
Author: Pan Chuang <[email protected]>
watchdog: keembay: Remove redundant dev_err_probe()

This commit removes redundant dev_err_probe() calls from devm_request_irq() 
error paths. The core devm_request_irq() already handles logging detailed 
error messages.

> diff --git a/drivers/watchdog/keembay_wdt.c b/drivers/watchdog/keembay_wdt.c
> index 2a39114dbc641..fa8c912e83aa0 100644
> --- a/drivers/watchdog/keembay_wdt.c
> +++ b/drivers/watchdog/keembay_wdt.c
> @@ -215,7 +215,7 @@ static int keembay_wdt_probe(struct platform_device *pdev)

[Severity: High]
This is a pre-existing issue, but does this driver safely handle the Common
Clock Framework late initcalls?

Looking at keembay_wdt_probe(), the driver fetches the clock but never
explicitly enables it:

drivers/watchdog/keembay_wdt.c:keembay_wdt_probe() {
    ...
    /* we do not need to enable the clock as it is enabled by default */
    wdt->clk = devm_clk_get(dev, NULL);
    ...
}

Because clk_prepare_enable() is never called, the CCF late initcall
(clk_disable_unused) will find this clock with a reference count of zero and
disable it to save power. When this happens, won't the watchdog hardware
silently stop ticking?

>  	ret = devm_request_irq(dev, wdt->th_irq, keembay_wdt_th_isr, 0,
>  			       "keembay-wdt", wdt);

[Severity: High]
This is a pre-existing issue, but can this early IRQ request cause a NULL
pointer dereference if the watchdog is already running?

If the bootloader has enabled the watchdog and a pre-timeout interrupt is
pending, devm_request_irq() will unmask the interrupt line and the ISR will
fire immediately.

The ISR calls keembay_wdt_set_pretimeout_reg(), which expects the watchdog
driver data to be initialized:

drivers/watchdog/keembay_wdt.c:keembay_wdt_set_pretimeout_reg() {
    ...
    struct keembay_wdt *wdt = watchdog_get_drvdata(wdog);
    ...
    keembay_wdt_writel(wdt, TIM_WATCHDOG_INT_THRES, th_val * wdt->rate);
    ...
}

Since watchdog_set_drvdata() is called further down in keembay_wdt_probe()
(after the IRQ is requested), watchdog_get_drvdata() would return NULL here,
leading to a crash when wdt->rate is accessed. 

Could the IRQ requests be moved after the watchdog structure is fully
initialized?

>  	if (ret)
> -		return dev_err_probe(dev, ret, "Failed to request IRQ for threshold\n");
> +		return ret;
>  
>  	wdt->to_irq = platform_get_irq_byname(pdev, "timeout");
>  	if (wdt->to_irq < 0)

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4