Re: [RESEND v4 5/5] rtc: pcf85363: add watchdog support with configurable step size

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

New issues:
- [High] Hardware timer value is not scaled by the selected clock frequency, resulting in radically incorrect watchdog timeouts.
- [High] The watchdog initializes with a 0-second hardware timeout if the device tree lacks a timeout property.
- [High] Missing `return` statement ignores watchdog initialization failures and breaks probe deferral.

Pre-existing issues:
- [High] Read-modify-write on the write-0-to-clear `CTRL_FLAGS` register silently drops concurrent hardware interrupts.
--

--- Patch [5]: [RESEND v4 5/5] rtc: pcf85363: add watchdog support with configurable step size ---
commit b334f1fe3594f36842ec186290e4071c7236a04d
Author: Lakshay Piplani <[email protected]>
Subject: rtc: pcf85363: add watchdog support with configurable step size

Add watchdog timer support to PCF85263/PCF85363 using the linux watchdog
subsystem. The driver programs the hardware watchdog timeout based on
the requested period.

diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index 665bbbb169b05..3eb87306c83c8 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c

[ ... ]

> +static int pcf85363_wdt_reload(struct pcf85363_watchdog *wd)
> +{
> +	u8 val;
> +
> +	val = ((wd->timeout_val & WD_TIMEOUT_MAX) << WD_TIMEOUT_SHIFT) |
> +	       (wd->clock_sel & WD_CLKSEL_MASK);
> +
> +	return regmap_write(wd->regmap, CTRL_WDOG, val);
> +}

[Severity: High]
Should the timeout value be scaled by the selected clock frequency (e.g., in
ticks) before it is written to the hardware?

It appears the raw seconds value assigned from pcf85363_wdt_set_timeout() is
written directly into the hardware register here. At frequencies higher than
1Hz, this would cause the timeout to expire significantly faster than
configured.

[ ... ]

> +static int pcf85363_wdt_ping(struct watchdog_device *wdd)
> +{
> +	struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
> +
> +	regmap_update_bits(wd->regmap, CTRL_FLAGS, FLAGS_WDF, 0);

[Severity: High]
This is a pre-existing issue, but since CTRL_FLAGS is a write-0-to-clear
register, can this read-modify-write cycle silently clear concurrent
hardware interrupts?

If an interrupt asserts immediately after the read step in
regmap_update_bits(), its flag will read as 0. The cycle will then write 0
back to that flag, potentially clearing the interrupt before it can be
handled. Pinging the watchdog in a background loop might exacerbate this.

[ ... ]

> +static int pcf85363_wdt_set_timeout(struct watchdog_device *wdd,
> +				    unsigned int timeout)
> +{
> +	struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
> +
> +	wd->timeout_val = clamp(timeout, WD_TIMEOUT_MIN, WD_TIMEOUT_MAX);

[Severity: High]
Is the driver storing the unscaled timeout in seconds directly into the
hardware timer value here?

If this value is later used in pcf85363_wdt_reload() without being adjusted
for the selected clock frequency, the hardware timeout may be radically
incorrect.

[ ... ]

> +static int pcf85363_watchdog_init(struct device *dev, struct regmap *regmap)
> +{
[ ... ]
> +	wd->wdd.parent = dev;
> +	wd->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
> +
> +	ret = watchdog_init_timeout(&wd->wdd, 0, dev);
> +	if (ret)
> +		wd->wdd.timeout = WD_DEFAULT_TIMEOUT;

[Severity: High]
Does watchdog_init_timeout() return 0 when the device tree lacks a timeout
property?

If it returns 0 without modifying wdd.timeout, this check would be bypassed,
leaving the watchdog initialized with a 0-second timeout. Setting
wd->wdd.timeout = WD_DEFAULT_TIMEOUT before calling watchdog_init_timeout()
might be safer.

[ ... ]

> @@ -685,6 +833,10 @@ static int pcf85363_probe(struct i2c_client *client)
>  			   PIN_IO_TSPM | PIN_IO_TSIM,
>  			   PIN_IO_TSPM | PIN_IO_TSIM);
>  
> +	ret = pcf85363_watchdog_init(dev, pcf85363->regmap);
> +	if (ret)
> +		dev_err_probe(dev, ret, "Watchdog init failed\n");
> +

[Severity: High]
Is a return statement missing here?

Without it, probe failures from pcf85363_watchdog_init() such as -ENOMEM
or -EPROBE_DEFER will be discarded, allowing the device probe to falsely
succeed with a broken or uninitialized watchdog.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5