Re: [PATCH v2] rtc: m41t80: use watchdog core for watchdog support

[sashiko-bot@kernel.org]

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

New issues:
- [High] m41t80_wdt_set_timeout() modifies the watchdog timeout before validating the hardware transfer, and unconditionally starts the hardware timer.
- [Medium] The watchdog device's parent pointer is not assigned before registration, breaking sysfs topology.
- [Medium] The wdt_margin module parameter's default value shadows the Device Tree timeout-sec property.

Pre-existing issues:
- [High] The square-wave clock and OF provider registered during probe are never unregistered, causing a Use-After-Free.
- [High] Stack buffers are passed to i2c_transfer, violating DMA constraints.
--

--- Patch [1]: [PATCH v2] rtc: m41t80: use watchdog core for watchdog support ---
commit 71ede3ba0b7d96a1d64a82ae64e6033a3aa277f9
Author: Yuho Choi <dbgh9129@gmail.com>

rtc: m41t80: use watchdog core for watchdog support

The M41T80 watchdog uses a global misc device, reboot notifier, and
save_client pointer. This patch converts the driver to use the watchdog core,
embedding the watchdog_device in m41t80_data and relying on devres for
registration and cleanup.

> diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
> index 3c8c379392c14..ec2238d9170e0 100644
> --- a/drivers/rtc/rtc-m41t80.c
> +++ b/drivers/rtc/rtc-m41t80.c

[ ... ]

> -static int wdt_margin = WD_TIMO;
> +static int wdt_margin = M41T80_WDT_DEFAULT_TIMEOUT;
>  module_param(wdt_margin, int, 0);
>  MODULE_PARM_DESC(wdt_margin, "Watchdog timeout in seconds (default 60s)");
>  
> -static unsigned long wdt_is_open;
> -static int boot_flag;
> +static const struct watchdog_info m41t80_wdt_info = {
> +	.options = WDIOF_POWERUNDER | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
> +	.firmware_version = 1,
> +	.identity = "M41T80 Watchdog",
> +};

[Severity: Medium]
Since wdt_margin is statically initialized to M41T80_WDT_DEFAULT_TIMEOUT (60),
will watchdog_init_timeout() interpret this non-zero value as an explicit
driver override and completely ignore the Device Tree timeout-sec property?

It seems this renders the DT configuration unreachable unless the user
explicitly passes wdt_margin=0 at boot.

[ ... ]

> +static int m41t80_wdt_ping(struct watchdog_device *wdt)
>  {
> +	struct m41t80_data *m41t80 = watchdog_get_drvdata(wdt);
> +	struct i2c_client *client = m41t80->client;
>  	unsigned char i2c_data[2];
> -	struct i2c_msg msgs1[1] = {
> -		{
> -			.addr	= save_client->addr,
> -			.flags	= 0,
> -			.len	= 2,
> -			.buf	= i2c_data,
> -		},
> +	struct i2c_msg msg = {
> +		.addr = client->addr,
> +		.flags = 0,
> +		.len = 2,
> +		.buf = i2c_data,
>  	};

[Severity: High]
This is a pre-existing issue, but are these stack-allocated buffers safe for
I2C transfers?

In modern kernels with VMAP_STACK, the stack is not physically contiguous. If
the underlying I2C adapter relies on DMA for transfers, passing stack memory
(like i2c_data here, and i2c_buf in m41t80_wdt_stop) directly to i2c_transfer()
could cause memory corruption or kernel panics.

[ ... ]

> +static int m41t80_wdt_set_timeout(struct watchdog_device *wdt, unsigned int timeout)
>  {
> +	wdt->timeout = timeout;
>  
> -	mutex_lock(&m41t80_rtc_mutex);
> -	ret = wdt_ioctl(file, cmd, arg);
> -	mutex_unlock(&m41t80_rtc_mutex);
> -
> -	return ret;
> +	return m41t80_wdt_ping(wdt);
>  }

[Severity: High]
If the underlying I2C transfer fails in m41t80_wdt_ping(), won't the hardware
retain its old timeout while the watchdog core and driver retain the new,
unapplied wdt->timeout, causing an inconsistent state?

Also, if the watchdog was previously stopped, wouldn't unconditionally
calling m41t80_wdt_ping() here activate the hardware timer without setting
WDOG_ACTIVE in the watchdog core? This could lead to an unexpected system
reboot since the core won't ping an inactive watchdog.

[ ... ]

> @@ -1011,19 +842,26 @@ static int m41t80_probe(struct i2c_client *client)
>  	if (rc)
>  		return rc;
>  
> -	return 0;
> -}
> -
> -static void m41t80_remove(struct i2c_client *client)
> -{
>  #ifdef CONFIG_RTC_DRV_M41T80_WDT
> -	struct m41t80_data *clientdata = i2c_get_clientdata(client);
> -
> -	if (clientdata->features & M41T80_FEATURE_HT) {
> -		misc_deregister(&wdt_dev);
> -		unregister_reboot_notifier(&wdt_notifier);
> +	if (m41t80_data->features & M41T80_FEATURE_HT) {
> +		m41t80_data->wdt.info = &m41t80_wdt_info;
> +		m41t80_data->wdt.ops = &m41t80_wdt_ops;
> +		m41t80_data->wdt.timeout = M41T80_WDT_DEFAULT_TIMEOUT;
> +		m41t80_data->wdt.min_timeout = M41T80_WDT_MIN_TIMEOUT;
> +		m41t80_data->wdt.max_timeout = M41T80_WDT_MAX_TIMEOUT;

[Severity: Medium]
Is it intentional to omit setting the watchdog device's parent pointer
(m41t80_data->wdt.parent = &client->dev)?

Failing to set this will cause the watchdog core to create the device at the
root of the virtual sysfs hierarchy rather than nesting it under the physical
I2C client device, which can break sysfs topology and power management
ordering.

> +
> +		watchdog_init_timeout(&m41t80_data->wdt, wdt_margin, &client->dev);
> +		watchdog_stop_on_reboot(&m41t80_data->wdt);
> +		watchdog_stop_on_unregister(&m41t80_data->wdt);
> +		watchdog_set_drvdata(&m41t80_data->wdt, m41t80_data);
> +
> +		rc = devm_watchdog_register_device(&client->dev, &m41t80_data->wdt);
> +		if (rc)
> +			return rc;
>  	}
>  #endif

[Severity: High]
This is a pre-existing issue, but if devm_watchdog_register_device() fails and
returns an error, won't this leak Common Clock Framework pointers?

Earlier in probe, m41t80_sqw_register_clk() registers a clock and an OF
provider using non-managed APIs:

	clk = clk_register(&client->dev, &m41t80->sqw);
	if (!IS_ERR(clk))
		of_clk_add_provider(node, of_clk_src_simple_get, clk);

Neither the probe error paths nor the device unbind mechanism unregisters
these. If probe fails here, devres will automatically free the m41t80_data
struct, leaving CCF with dangling pointers to the embedded m41t80_data->sqw
and causing a use-after-free on subsequent clock accesses.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807013915.907604-1-dbgh9129@gmail.com?part=1