Re: [PATCH v4 3/4] rtc: pcf8525: Add watchdog support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] rtc: pcf8525: premature nINTB pin switch in pcf8525_watchdog_config()
- [High] rtc: pcf8525: uncanceled deferred ping worker in pcf8525_suspend()
- [Medium] rtc: pcf8525: unthrottled duplicate ping in pcf8525_wdt_set_timeout()
- [Medium] rtc: pcf8525: min_hw_heartbeat_ms equals min_timeout in pcf8525_watchdog_init()
- [Medium] rtc: pcf8525: missing WDIOF_MAGICCLOSE in pcf8525_wdt_info
- [Medium] rtc: pcf8525: missing watchdog_stop_on_unregister() in probe
- [Low] rtc: pcf8525: unquantized timeout in pcf8525_watchdog_init()
--

commit ef4793891aaa47f5f86ec5e83faae0a7013999b0
Author: Shiv Prakash Gupta <shivprakash.gupta@nxp.com>

rtc: pcf8525: Add watchdog support

This commit extends the PCF8525 RTC driver with optional hardware watchdog
support when the reset-source device property is present.

> diff --git a/drivers/rtc/rtc-pcf8525.c b/drivers/rtc/rtc-pcf8525.c
> index 3c5f15f95a9ed..f8fbc00bb464d 100644
> --- a/drivers/rtc/rtc-pcf8525.c
> +++ b/drivers/rtc/rtc-pcf8525.c

[ ... ]

> @@ -906,6 +923,211 @@ static const struct rtc_class_ops pcf8525_rtc_ops = {
>  	.param_set = pcf8525_param_set,
>  };
>  

[ ... ]

> +static int pcf8525_wdt_set_timeout(struct watchdog_device *wdd,
> +				   unsigned int timeout)
> +{
> +	unsigned int wd_val;
> +
> +	wd_val = pcf8525_wdt_timeout_to_val(timeout);
> +	/* Report the achieved timeout, not the requested value. */
> +	wdd->timeout = pcf8525_watchdog_get_period(wd_val,
> +						   PCF8525_WD_CLOCK_HZ_X1000);
> +
> +	return pcf8525_wdt_active_ping(wdd);
> +}

[Severity: Medium]
In pcf8525_wdt_set_timeout(), does calling pcf8525_wdt_active_ping() bypass
wdd->min_hw_heartbeat_ms and cause duplicate writes to PCF8525_REG_WD_VAL?

When userspace invokes ioctl(WDIOC_SETTIMEOUT), watchdog_ioctl() in
drivers/watchdog/watchdog_dev.c calls watchdog_set_timeout() and then
immediately calls watchdog_ping() -> __watchdog_ping():

	case WDIOC_SETTIMEOUT:
		if (get_user(val, p))
			return -EFAULT;
		err = watchdog_set_timeout(wdd, val);
		if (err < 0)
			return err;
		/* If the watchdog is active then we send a keepalive ping
		 * to make sure that the watchdog keep's running (and if
		 * possible that it takes the new timeout) */
		err = watchdog_ping(wdd);
		if (err < 0)
			return err;

Because pcf8525_wdt_set_timeout() -> pcf8525_wdt_active_ping() ->
pcf8525_wdt_ping() writes PCF8525_REG_WD_VAL directly when watchdog_active()
is true, it neither checks wdd->min_hw_heartbeat_ms (4000 ms) nor updates
wd_data->last_hw_keepalive. If less than 4000 ms has elapsed since the prior
hardware write, pcf8525_wdt_active_ping() writes to the hardware immediately
in violation of min_hw_heartbeat_ms, and if at least 4000 ms has elapsed,
pcf8525_wdt_active_ping() writes PCF8525_REG_WD_VAL and watchdog_ping()
immediately writes PCF8525_REG_WD_VAL a second time back-to-back. Also, if
regmap_write() in pcf8525_wdt_active_ping() fails, wdd->timeout has already
been overwritten.

Should pcf8525_wdt_set_timeout() only update wdd->timeout and let the
watchdog core's subsequent watchdog_ping() call handle the hardware write
and rate limiting?

> +
> +static const struct watchdog_info pcf8525_wdt_info = {
> +	.identity = "NXP PCF8525 Watchdog",
> +	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
> +};

[Severity: Medium]
Should pcf8525_wdt_info.options include WDIOF_MAGICCLOSE?

Without WDIOF_MAGICCLOSE, when CONFIG_WATCHDOG_NOWAYOUT is disabled and the
userspace watchdog daemon crashes or closes /dev/watchdog without writing
the magic 'V' character, watchdog_release() sees !(wdd->info->options &
WDIOF_MAGICCLOSE) and silently stops the hardware watchdog via
watchdog_stop() instead of allowing it to reset the system.

> +
> +static const struct watchdog_ops pcf8525_watchdog_ops = {
> +	.owner = THIS_MODULE,
> +	.start = pcf8525_wdt_start,
> +	.stop = pcf8525_wdt_stop,
> +	.ping = pcf8525_wdt_ping,
> +	.set_timeout = pcf8525_wdt_set_timeout,
> +};
> +
> +static int pcf8525_watchdog_config(struct device *dev,
> +				   struct pcf8525 *pcf8525)
> +{
> +	unsigned int m1, m2;
> +	int ret;
> +
> +	/*
> +	 * WD_VAL is write-only, so a watchdog started by firmware cannot be
> +	 * detected reliably. Stop it explicitly to establish a known state
> +	 * before registering the Linux watchdog device.
> +	 */
> +	ret = regmap_write(pcf8525->regmap, PCF8525_REG_WD_VAL,
> +			   PCF8525_WD_VAL_STOP);
> +	if (ret)
> +		return ret;
> +
> +	/* Configure nINTB/CLKOUT as nINTB open-drain interrupt output. */
> +	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CLKOUT,
> +				 PCF8525_CLKOUT_CLKOE, 0);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Select 1/4 Hz clock source: TF[1:0] = 10.
> +	 * Do NOT enable WD_CD yet; enable it only after clearing stale WDTF
> +	 * to avoid a spurious INTB assertion on a stale flag.
> +	 */
> +	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_WD_CTL,
> +				 PCF8525_WD_CTL_WD_CD |
> +				 PCF8525_WD_CTL_TF1 |
> +				 PCF8525_WD_CTL_TF0,
> +				 PCF8525_WD_CTL_TF1);
> +	if (ret)
> +		return ret;
> +
> +	/* Keep watchdog masked on INTA. */
> +	ret = regmap_read(pcf8525->regmap, PCF8525_REG_INTA_MASK1, &m1);
> +	if (ret)
> +		return ret;
> +
> +	m1 |= PCF8525_MASK1_WD_CD;
> +	ret = regmap_write(pcf8525->regmap, PCF8525_REG_INTA_MASK1, m1);
> +	if (ret)
> +		return ret;
> +
> +	/* Read INTB mask; mask all non-watchdog sources on INTB first. */
> +	ret = regmap_read(pcf8525->regmap, PCF8525_REG_INTB_MASK1, &m1);
> +	if (ret)
> +		return ret;
> +
> +	m1 |= PCF8525_MASK1_BIE |
> +	      PCF8525_MASK1_AIE |
> +	      PCF8525_MASK1_OSIE |
> +	      PCF8525_MASK1_SI |
> +	      PCF8525_MASK1_MI |
> +	      PCF8525_MASK1_WD_CD;   /* keep WD masked until WDTF is clear */
> +
> +	ret = regmap_write(pcf8525->regmap, PCF8525_REG_INTB_MASK1, m1);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(pcf8525->regmap, PCF8525_REG_INTB_MASK2, &m2);
> +	if (ret)
> +		return ret;
> +
> +	m2 |= PCF8525_MASK2_TSIE;
> +	ret = regmap_write(pcf8525->regmap, PCF8525_REG_INTB_MASK2, m2);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Clear stale WDTF before enabling WD_CD and unmasking INTB to
> +	 * avoid asserting the watchdog output immediately.
> +	 */
> +	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL2,
> +				 PCF8525_CTRL2_WDTF | PCF8525_CTRL2_AF |
> +				 PCF8525_CTRL2_MSF,
> +				 PCF8525_CTRL2_AF | PCF8525_CTRL2_MSF);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable WD_CD now that WDTF has been cleared. */
> +	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_WD_CTL,
> +				 PCF8525_WD_CTL_WD_CD,
> +				 PCF8525_WD_CTL_WD_CD);
> +	if (ret)
> +		return ret;
> +
> +	/* Unmask watchdog on INTB. */
> +	m1 &= ~PCF8525_MASK1_WD_CD;
> +	return regmap_write(pcf8525->regmap, PCF8525_REG_INTB_MASK1, m1);
> +}

[Severity: High]
Can clearing PCF8525_CLKOUT_CLKOE at the beginning of
pcf8525_watchdog_config() cause an immediate spurious board reset during
probe?

In pcf8525_probe(), before pcf8525_watchdog_init() ->
pcf8525_watchdog_config() runs, the driver already enables timestamp capture
and PCF8525_CTRL5_TSIE, and across a warm reboot or prior firmware activity,
stale interrupt flags (such as WDTF, AF, MSF, or TSF) and unmasked bits in
PCF8525_REG_INTB_MASK1 / PCF8525_REG_INTB_MASK2 or PCF8525_WD_CTL_WD_CD may
already be active.

Although the comments note that WD_CD and INTB_MASK1 must remain
disabled/masked until non-watchdog sources are masked and stale WDTF is
cleared, pcf8525_watchdog_config() switches the physical nINTB/CLKOUT pin to
active-low nINTB by clearing PCF8525_CLKOUT_CLKOE before clearing WD_CD,
before masking non-watchdog sources in PCF8525_REG_INTB_MASK1 and
PCF8525_REG_INTB_MASK2, and before clearing stale WDTF in PCF8525_REG_CTRL2.
During those intervening I2C transactions, any pending unmasked INTB source
can drive nINTB low and reset the system.

Should clearing PCF8525_CLKOUT_CLKOE in PCF8525_REG_CLKOUT be moved to the
end of pcf8525_watchdog_config(), after all INTB masks, stale flags, and
WD_CTL have been configured?

> +
> +static int pcf8525_watchdog_init(struct device *dev,
> +				 struct pcf8525 *pcf8525)
> +{
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_WATCHDOG_CORE) ||
> +	    !device_property_read_bool(dev, "reset-source"))
> +		return 0;
> +
> +	/*
> +	 * Configure the hardware first: this switches the nINTB/CLKOUT pin
> +	 * from clock output (CLKOE=1) to open-drain output before the
> +	 * watchdog is registered.
> +	 */
> +	ret = pcf8525_watchdog_config(dev, pcf8525);
> +	if (ret)
> +		return ret;
> +
> +	pcf8525->wdd.parent = dev;
> +	pcf8525->wdd.info = &pcf8525_wdt_info;
> +	pcf8525->wdd.ops = &pcf8525_watchdog_ops;
> +	pcf8525->wdd.min_timeout = pcf8525_watchdog_get_period(2,
> +							       PCF8525_WD_CLOCK_HZ_X1000);
> +	pcf8525->wdd.max_timeout = pcf8525_watchdog_get_period(255,
> +							       PCF8525_WD_CLOCK_HZ_X1000);
> +	pcf8525->wdd.timeout = PCF8525_WD_DEFAULT_TIMEOUT_S;
> +	watchdog_init_timeout(&pcf8525->wdd, 0, dev);

[Severity: Low]
When timeout-sec is provided in Device Tree with a value that is not a
multiple of the 4-second hardware period, watchdog_init_timeout() stores the
unquantized value into pcf8525->wdd.timeout, whereas pcf8525_wdt_ping()
programs a rounded-up multiple of 4 seconds into PCF8525_REG_WD_VAL. Should
pcf8525->wdd.timeout be normalized via pcf8525_wdt_timeout_to_val() and
pcf8525_watchdog_get_period() after watchdog_init_timeout() so that
WDIOC_GETTIMEOUT and sysfs report the actual hardware period, matching
pcf8525_wdt_set_timeout()?

> +	pcf8525->wdd.min_hw_heartbeat_ms = PCF8525_WD_MIN_HW_HEARTBEAT_MS;

[Severity: Medium]
Can the watchdog ever be kept alive when configured with min_timeout (4
seconds) while min_hw_heartbeat_ms is also 4000 ms?

In pcf8525_watchdog_init(), min_timeout is pcf8525_watchdog_get_period(2,
250) = 4 seconds (4000 ms), and min_hw_heartbeat_ms is set to
PCF8525_WD_MIN_HW_HEARTBEAT_MS (4000 ms). When timeout is 4 seconds,
pcf8525_wdt_ping() writes WD_VAL = 2 (1 tick of the 1/4 Hz clock, at most
4000 ms). However, in drivers/watchdog/watchdog_dev.c, __watchdog_ping()
computes earliest_keepalive = wd_data->last_hw_keepalive + 4000 ms and
defers any keepalive ping arriving before 4000 ms have elapsed until after
the 4-second hardware countdown has already expired.

Should min_timeout be increased (for example to 8 seconds, WD_VAL = 3) if
min_hw_heartbeat_ms must remain 4000 ms?

> +	pcf8525->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
> +
> +	watchdog_set_drvdata(&pcf8525->wdd, pcf8525);
> +	watchdog_stop_on_reboot(&pcf8525->wdd);
> +
> +	return devm_watchdog_register_device(dev, &pcf8525->wdd);
> +}

[Severity: Medium]
Should pcf8525_watchdog_init() also call
watchdog_stop_on_unregister(&pcf8525->wdd)?

Without WDOG_STOP_ON_UNREGISTER set, if the driver is unbound via sysfs
while the watchdog is active, or if devm_rtc_register_device() fails in
pcf8525_probe() after userspace has opened /dev/watchdog,
devm_watchdog_unregister_device() removes the watchdog device without
stopping the hardware countdown in PCF8525_REG_WD_VAL, leaving the system to
reset when the timer expires.

[ ... ]

> @@ -973,6 +1195,42 @@ static int pcf8525_unmask_irqs_inta(struct pcf8525 *pcf8525)
>  	return regmap_write(pcf8525->regmap, PCF8525_REG_INTA_MASK2, m2);
>  }
>  
> +static int pcf8525_suspend(struct device *dev)
> +{
> +	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
> +	int ret;
> +
> +	/* Stop watchdog first: failure here must abort suspend. */
> +	if (IS_ENABLED(CONFIG_WATCHDOG_CORE) &&
> +	    device_property_read_bool(dev, "reset-source") &&
> +	    watchdog_active(&pcf8525->wdd)) {
> +		ret = pcf8525_wdt_stop(&pcf8525->wdd);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int pcf8525_resume(struct device *dev)
> +{
> +	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
> +	int ret;
> +
> +	/* Ping watchdog first to avoid an imminent reset after resume. */
> +	if (IS_ENABLED(CONFIG_WATCHDOG_CORE) &&
> +	    device_property_read_bool(dev, "reset-source") &&
> +	    watchdog_active(&pcf8525->wdd)) {
> +		ret = pcf8525_wdt_ping(&pcf8525->wdd);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Can a deferred keepalive ping from the watchdog core fire after
pcf8525_suspend() has stopped the hardware watchdog?

Because pcf8525_watchdog_init() sets min_hw_heartbeat_ms = 4000 ms, a
userspace keepalive ping within 4 seconds prior to system suspend causes
__watchdog_ping() in drivers/watchdog/watchdog_dev.c to arm wd_data->timer,
which queues wd_data->work (watchdog_ping_work) on the non-freezable
watchdog_kworker kthread.

Since pcf8525_watchdog_init() does not call
watchdog_stop_ping_on_suspend(&pcf8525->wdd), wd_data->timer and
wd_data->work are not canceled on suspend, and WDOG_ACTIVE remains set when
pcf8525_suspend() calls pcf8525_wdt_stop(). If wd_data->timer expires after
pcf8525_suspend() runs (for example during s2idle or before noirq suspend),
watchdog_ping_work() sees watchdog_active() is true and calls
__watchdog_ping() -> pcf8525_wdt_ping(), either re-enabling the hardware
watchdog while the system is asleep or issuing an I2C transfer after the
parent I2C adapter has suspended. In addition, pcf8525_resume() calls
pcf8525_wdt_ping() directly without updating wd_data->last_hw_keepalive or
serializing with wd_data->lock.

Could pcf8525_watchdog_init() call watchdog_stop_ping_on_suspend() (or use
watchdog_dev_suspend() and watchdog_dev_resume() in pcf8525_suspend() and
pcf8525_resume()) so the watchdog core cancels any pending ping timer/worker
on suspend and updates last_hw_keepalive on resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030110139.1929109-1-shivprakash.gupta@nxp.com?part=3