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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Fake WDIOF_PRETIMEOUT support breaks pretimeout functionality and watchdog reset guarantees.
- [High] Watchdog is not stopped during system suspend if it is running but not actively opened.
- [High] Unconditionally masking the INTB pin on driver unbind defeats the hardware watchdog reset guarantees.
- [High] Read-modify-write operations in pre-existing RTC handlers accidentally clear the newly-enabled watchdog flag (WDTF).
--

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

rtc: pcf8525: Add watchdog support

Adds watchdog support for NXP PCF8525 real time clock (RTC) device.

> diff --git a/drivers/rtc/rtc-pcf8525.c b/drivers/rtc/rtc-pcf8525.c
> --- a/drivers/rtc/rtc-pcf8525.c
> +++ b/drivers/rtc/rtc-pcf8525.c
[ ... ]
> @@ -879,6 +901,296 @@ static const struct rtc_class_ops pcf8525_rtc_ops = {
[ ... ]
> +static const struct watchdog_info pcf8525_wdt_info = {
> +	.identity = "NXP PCF8525 Watchdog",
> +	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_PRETIMEOUT,
> +};

[Severity: High]
Does advertising WDIOF_PRETIMEOUT here prevent the core from falling back to
hrtimer-based software pretimeout, despite the driver having no native
pretimeout support?

[ ... ]
> +static irqreturn_t pcf8525_wdt_irq(int irq, void *data)
> +{
[ ... ]
> +	/*
> +	 * W0C: clear WDTF. Include AF and MSF in the mask and write 1 to them
> +	 * so the RMW does not accidentally clear a concurrently asserted alarm
> +	 * or minute/second interrupt flag.
> +	 */
> +	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 IRQ_NONE;
> +

[Severity: High]
This isn't a bug introduced by this patch, but adding the WDTF flag exposes a
pre-existing issue in other handlers.

In pre-existing code like pcf8525_irq() and pcf8525_set_alarm(),
read-modify-write operations on PCF8525_REG_CTRL2 accidentally clear this
newly-enabled WDTF flag.

For example, in pcf8525_irq():

    regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL2,
                       PCF8525_CTRL2_AF | PCF8525_CTRL2_MSF,
                       PCF8525_CTRL2_MSF);

If WDTF asserts between the read and write, will writing 0 to it accidentally
clear the watchdog flag and lose the interrupt?

> +	watchdog_notify_pretimeout(&pcf8525->wdd);
> +	return IRQ_HANDLED;
> +}

[Severity: High]
If the final hardware timeout is unconditionally treated as a pretimeout here,
will a subsequent hardware reset ever occur if wired as an interrupt?

Without a secondary timer to perform the actual reset, deployments relying on
this watchdog for system recovery might silently fail to reboot.

[ ... ]
> +static void pcf8525_mask_intb(void *data)
> +{
> +	struct pcf8525 *pcf8525 = data;
> +	unsigned int m1;
> +	int ret;
> +
> +	ret = regmap_read(pcf8525->regmap, PCF8525_REG_INTB_MASK1, &m1);
> +	if (!ret) {
> +		m1 |= PCF8525_MASK1_WD_CD;
> +		regmap_write(pcf8525->regmap, PCF8525_REG_INTB_MASK1, m1);
> +	}
> +}
> +
> +static int pcf8525_watchdog_init(struct device *dev,
> +				 struct pcf8525 *pcf8525)
> +{
[ ... ]
> +	/* Register a cleanup action to re-mask INTB on driver unbind. */
> +	ret = devm_add_action_or_reset(dev, pcf8525_mask_intb, pcf8525);

[Severity: High]
Does unconditionally masking the INTB pin on driver unbind defeat the hardware
watchdog reset guarantees?

If the watchdog is configured with the nowayout feature, or closed without
the magic character, the core watchdog subsystem intentionally leaves the
hardware timer running on driver unbind to ensure the system is eventually
reset.

Will silencing the physical reset line here prevent the hardware from
asserting the reset?

[ ... ]
> +static int pcf8525_suspend(struct device *dev)
> +{
> +	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
> +
> +	if (IS_ENABLED(CONFIG_WATCHDOG_CORE) &&
> +	    device_property_read_bool(dev, "reset-source") &&
> +	    watchdog_active(&pcf8525->wdd))
> +		return pcf8525_wdt_stop(&pcf8525->wdd);
> +
> +	return 0;
> +}

[Severity: High]
If the hardware watchdog was started by the bootloader (meaning
WDOG_HW_RUNNING is set), will this fail to pause the watchdog during suspend?

Since watchdog_active() is only true when userspace has opened the device,
this fails to stop the HW if it is only running in the background. If the
ping worker is frozen during suspend, could the hardware timer reset the
system while sleeping?

> +
> +static int pcf8525_resume(struct device *dev)
> +{
> +	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
> +
> +	if (IS_ENABLED(CONFIG_WATCHDOG_CORE) &&
> +	    device_property_read_bool(dev, "reset-source") &&
> +	    watchdog_active(&pcf8525->wdd))
> +		return pcf8525_wdt_ping(&pcf8525->wdd);
> +
> +	return 0;
> +}

[Severity: High]
Similarly, does this fail to resume a hardware-running watchdog on wake since
it also relies on watchdog_active()?

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