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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Unmasking watchdog interrupt without clearing persistent WDTF flag causes inescapable boot loop.
- [High] Watchdog IRQ handler silently clears timeout flag, aborting hardware resets and defeating the watchdog.
- [High] Missing WDOG_HW_RUNNING reporting causes spurious system resets if the watchdog is armed by the bootloader.
- [High] Requesting INTB without IRQF_SHARED breaks probe on boards tying INTA and INTB together.
- [Medium] Watchdog initialization occurs after RTC registration, creating a race condition exposing the uninitialized device.

Pre-existing issues:
- [High] Read-modify-write on CTRL2 causes lost hardware interrupts for RTC alarm and watchdog.
--

--- Patch [3]: [PATCH 3/4] rtc: pcf8525: Add watchdog support ---
commit 3cb38601cbc74987657b19d7480456e2da5babc7
Author: Shiv Prakash Gupta <shivprakash.gupta@nxp.com>

rtc: pcf8525: Add watchdog support

This commit adds watchdog support for the NXP PCF8525 real time clock device.

> diff --git a/drivers/rtc/rtc-pcf8525.c b/drivers/rtc/rtc-pcf8525.c
> index 7dfb4f0f64047..4ef648bdfc5b9 100644
> --- a/drivers/rtc/rtc-pcf8525.c
> +++ b/drivers/rtc/rtc-pcf8525.c
[ ... ]
> +static irqreturn_t pcf8525_wdt_irq(int irq, void *data)
> +{
> +	struct device *dev = data;
> +	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
> +	unsigned int ctrl2;
> +	int ret;
> +
> +	ret = regmap_read(pcf8525->regmap, PCF8525_REG_CTRL2, &ctrl2);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	if (!(ctrl2 & PCF8525_CTRL2_WDTF))
> +		return IRQ_NONE;
> +
> +	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL2,
> +				 PCF8525_CTRL2_WDTF, 0);

[Severity: High]
Does returning IRQ_HANDLED here without notifying the watchdog core or
invoking panic() silently abort the hardware reset? If INTB is routed to
a reset controller, de-asserting it before the reset cycle completes might
leave the system in a hung state rather than resetting it.

[Severity: High]
This isn't a bug introduced by this patch, but this read-modify-write
on the CTRL2 register extends a pre-existing race condition to the
watchdog handler thread.

CTRL2 contains write-0-to-clear flags like AF, MSF, and WDTF. Because
regmap_update_bits() reads the register and writes it back, if a hardware
flag asserts after the read but before the write, could this write back
a 0 and accidentally clear the newly asserted flag?

This same pattern exists in pcf8525_irq() when clearing AF, which could
cause lost hardware interrupts.

> +	if (ret)
> +		return IRQ_NONE;
> +
> +	return IRQ_HANDLED;
> +}
[ ... ]
> +static int pcf8525_watchdog_config(struct device *dev,
> +				   struct pcf8525 *pcf8525)
> +{
[ ... ]
> +	m1 |= PCF8525_MASK1_BIE |
> +	      PCF8525_MASK1_AIE |
> +	      PCF8525_MASK1_OSIE |
> +	      PCF8525_MASK1_SI |
> +	      PCF8525_MASK1_MI;
> +	m1 &= ~PCF8525_MASK1_WD_CD;
> +
> +	ret = regmap_write(pcf8525->regmap, PCF8525_REG_INTB_MASK1, m1);

[Severity: Critical]
If the watchdog resets the system, the battery-backed RTC retains the WDTF
(Watchdog Timeout Flag) set to 1. On reboot, when this unmasks the watchdog
interrupt on the INTB pin, will it immediately assert INTB since WDTF is
still 1? This regression could trigger the hardware reset again and cause
an inescapable boot loop.

> +	if (ret)
> +		return ret;
[ ... ]
> +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;
> +
> +	if (pcf8525->irq_intb > 0) {
> +		ret = devm_request_threaded_irq(dev, pcf8525->irq_intb,
> +						NULL, pcf8525_wdt_irq,
> +						IRQF_ONESHOT,
> +						"pcf8525-wdt", dev);

[Severity: High]
Many hardware designs tie open-drain I2C interrupts (INTA and INTB) to a
single SoC GPIO. If they are mapped to the same IRQ, will this request fail
because it lacks IRQF_SHARED and uses the same dev_id token (dev)?
This could result in a -EBUSY error and cause the entire device probe
to fail.

[ ... ]
> +	pcf8525->wdd.timeout = PCF8525_WD_DEFAULT_TIMEOUT_S;
> +	watchdog_init_timeout(&pcf8525->wdd, 0, dev);
> +	pcf8525->wdd.min_hw_heartbeat_ms = PCF8525_WD_MIN_HW_HEARTBEAT_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: High]
If the bootloader starts the watchdog, does unconditionally setting the
status without checking if the hardware timer is active cause issues?
Because it fails to set WDOG_HW_RUNNING, the watchdog core will not
initiate the ping worker, which might allow the timer to expire and
cause a spurious system reboot during boot.

[ ... ]
> @@ -902,6 +1139,10 @@ static int pcf8525_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> +	ret = pcf8525_watchdog_init(dev, pcf8525);
> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }

[Severity: Medium]
Could initializing the watchdog after RTC registration expose the RTC
device to userspace prematurely? If pcf8525_watchdog_init() fails,
probe will return an error and devres will unregister the RTC,
which might cause operations on the opened device to fail.

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