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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] rtc: pcf85363: spurious WDF handling in pcf85363_rtc_handle_irq()
--

commit 16cc51c19e1316f4895386e586c6d2ab87d77232
Author: Lakshay Piplani <lakshay.piplani@nxp.com>

rtc: pcf85363: add watchdog support with configurable step size

This commit adds alarm-only watchdog support to the PCF85263/PCF85363 RTC
driver using the watchdog subsystem, with dynamic 1 Hz and 0.25 Hz step
selection, interrupt handling via INTA, and suspend/resume support.

> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index 71b82f92a0031..e5ebfad1a2cea 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c

[ ... ]

> @@ -540,6 +572,21 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
>  		}
>  	}
>  
> +	if (flags & FLAGS_WDF) {
> +		dev_warn_ratelimited(&pcf85363->rtc->dev,
> +				     "watchdog timer expired\n");
> +
> +		err = pcf85363_clear_flags(pcf85363, FLAGS_WDF);
> +		if (err) {
> +			dev_err_ratelimited(&pcf85363->rtc->dev,
> +					    "failed to clear watchdog flag: %d\n",
> +					    err);
> +			ack_failed = true;
> +		} else {
> +			handled = true;
> +		}
> +	}

[Severity: Medium]
Can pcf85363_rtc_handle_irq() in drivers/rtc/rtc-pcf85363.c log false
"watchdog timer expired" warnings and return spurious IRQ_HANDLED on a
shared interrupt line when FLAGS_WDF is latched in CTRL_FLAGS while
INT_WDIE is masked in CTRL_INTA_EN?

On PCF85263/PCF85363, CTRL_FLAGS latches FLAGS_WDF whenever the CTRL_WDOG
counter reaches 0, regardless of whether INT_WDIE is set in CTRL_INTA_EN
(which only controls routing of WDF to the INTA pin).

In pcf85363_suspend(), INT_WDIE is masked while the CTRL_WDOG counter is
left running in repeat mode:

	/* Disconnect WDF from the wakeup-capable INTA line; keep counting. */
	ret = regmap_update_bits(pcf85363->regmap, CTRL_INTA_EN, INT_WDIE, 0);
	if (ret)
		return ret;

	wd->suspended = true;

During any system sleep longer than the watchdog timeout, the counter
expires and latches FLAGS_WDF. On wakeup, resume_device_irqs() in
dpm_resume_noirq() re-enables device interrupts before dpm_resume() runs
pcf85363_resume(). If an RTC alarm (FLAGS_A1F), timestamp (FLAGS_TSR1F,
FLAGS_TSR2F, FLAGS_TSR3F), battery switch-over (FLAGS_BSF), or another
device on a shared IRQ line fires before pcf85363_resume() runs,
pcf85363_rtc_handle_irq() executes while wd->suspended is still true and
INT_WDIE is still masked, reporting a false watchdog expiry and setting
handled = true.

A similar state can also occur in two other paths:

1. In pcf85363_probe(), INT_WDIE is masked and only FLAGS_A2F | FLAGS_PIF
are cleared before devm_request_threaded_irq() is called. When
CONFIG_WATCHDOG=n, pcf85363_watchdog_init() returns early without clearing
a stale FLAGS_WDF or halting a bootloader- or battery-backed CTRL_WDOG
counter.

2. In pcf85363_wdt_stop(), CTRL_WDOG is halted and INT_WDIE is masked
without clearing any FLAGS_WDF that latched concurrently (or that latches
later if writing CTRL_WDOG fails while masking INT_WDIE succeeds).

Should pcf85363_rtc_handle_irq() ignore (or silently clear without setting
handled = true or logging a warning) FLAGS_WDF when the watchdog interrupt
is not armed, and should pcf85363_probe() and pcf85363_wdt_stop() also
ensure FLAGS_WDF is cleared when masking INT_WDIE?

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