Re: [PATCH v6 5/5] rtc: pcf85363: add watchdog support with configurable step size

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The watchdog is left running on driver unbind without an IRQ handler, leading to an unhandled interrupt storm.
- [High] The watchdog timer is left running during system suspend, which will cause spurious wakeups.
- [Medium] Un-ratelimited dev_err() in the threaded IRQ handler can cause a log storm and consume 100% of a CPU core if clearing the watchdog flag fails.
--

commit 5bad6cdbd177d2e1690c8afe67f641dfa821a364
Author: Lakshay Piplani <lakshay.piplani@nxp.com>
Subject: rtc: pcf85363: add watchdog support with configurable step size

This commit adds watchdog support to PCF85263/PCF85363 using the watchdog 
subsystem. The chip has no reset output, so expiry is only signalled via 
the WDF flag routed to INTA as an alarm-only watchdog.

> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index 22661abd9ebb5..aa45d71e6ab58 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
[ ... ]
> @@ -512,6 +539,15 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
>  		handled = true;
>  	}
>  
> +	if (flags & FLAGS_WDF) {
> +		dev_warn_ratelimited(&pcf85363->rtc->dev,
> +				     "watchdog timer expired\n");
> +		if (pcf85363_clear_flags(pcf85363, FLAGS_WDF))
> +			dev_err(&pcf85363->rtc->dev,
> +				"failed to clear watchdog flag\n");
> +		handled = true;
> +	}
> +

[Severity: Medium]
If the I2C write inside pcf85363_clear_flags() fails (e.g., due to a 
transient bus error), this path prints a non-ratelimited error and 
returns IRQ_HANDLED.

Because the physical level-triggered INTA line would remain asserted in 
hardware, won't the IRQ immediately re-fire and loop infinitely? Could 
this result in a CPU lockup and a log storm from the un-ratelimited 
dev_err()? 

Should the error log be rate-limited, or the interrupt disabled if it 
cannot be successfully cleared?

[ ... ]
> +static int pcf85363_watchdog_init(struct device *dev, struct pcf85363 *pcf85363)
> +{
> +	struct pcf85363_watchdog *wd;
[ ... ]
> +	watchdog_set_drvdata(&wd->wdd, wd);
> +
> +	return devm_watchdog_register_device(dev, &wd->wdd);
> +}

[Severity: High]
If a user unbinds the driver via sysfs while the watchdog is running, 
will the devm-managed IRQ handler be freed while the hardware watchdog 
continues to count down? 

When the hardware timer subsequently expires and asserts the INTA line, 
could this cause an unhandled interrupt storm that disables the shared 
IRQ line? 

Should WDOG_STOP_ON_UNREGISTER be set on the watchdog device, or a 
custom devm cleanup action be registered to halt the hardware timer 
on unbind?

[Severity: High]
Does the watchdog timer continue running during system suspend? 

Since INTA is configured as a system wakeup source earlier in the driver, 
if the watchdog is active when the system goes to sleep, won't it 
eventually expire and cause an unexpected, spurious system wakeup? 

Should dev_pm_ops be provided to halt the hardware timer during suspend 
and resume it upon waking up?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914092126.3757418-1-lakshay.piplani@nxp.com?part=5