Re: [PATCH v2 2/8] watchdog: w83627hf_wdt: Convert to platform driver model

[sashiko-bot@kernel.org]

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

New issues:
- [High] Potential NULL pointer dereference in `wdt_probe()` when the device is bound via sysfs `driver_override`.
- [High] Unbinding and rebinding the platform driver leaks watchdog status state, permanently breaking the watchdog.
- [Medium] The watchdog device's parent pointer is not initialized before registration, breaking the sysfs device hierarchy.

Pre-existing issues:
- [High] Hardware watchdog is left running if `devm_watchdog_register_device()` fails, guaranteeing an unintended system reboot.
--

--- Patch [2]: [PATCH v2 2/8] watchdog: w83627hf_wdt: Convert to platform driver model ---
commit d9d8da674e5ddb501516445617ff593614dca863
Author: Paul Louvel <paul.louvel@bootlin.com>
watchdog: w83627hf_wdt: Convert to platform driver model

This commit converts the w83627hf_wdt driver to the Linux driver model by
using a platform driver and platform device instead of the legacy module
init registration.

> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
> index 1529a4e168209..206ab91fdb2a9 100644
> --- a/drivers/watchdog/w83627hf_wdt.c
> +++ b/drivers/watchdog/w83627hf_wdt.c
[ ... ]
> @@ -464,6 +465,36 @@ static int wdt_find(int addr)
>  	return ret;
>  }
>  
> +static int wdt_probe(struct platform_device *pdev)
> +{
> +	const struct platform_device_id *id = platform_get_device_id(pdev);
> +	enum chips chip = id->driver_data;

[Severity: High]
Can platform_get_device_id() return NULL here if the device is bound via
sysfs driver_override? If the driver core's platform_match() succeeds by
matching the driver name and bypasses the id_table, id would be NULL,
causing a NULL pointer dereference when accessing id->driver_data.

> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	dev_info(dev, "WDT driver initialising\n");
> +
> +	snprintf(wdt_info.identity, sizeof(wdt_info.identity), "%s Watchdog",
> +		 id->name);
> +
> +	watchdog_init_timeout(&wdt_dev, timeout, NULL);
> +	watchdog_set_nowayout(&wdt_dev, nowayout);
> +	watchdog_stop_on_reboot(&wdt_dev);
> +
> +	ret = w83627hf_init(&wdt_dev, chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to initialize watchdog\n");
> +
> +	ret = devm_watchdog_register_device(&pdev->dev, &wdt_dev);

[Severity: Medium]
Should wdt_dev.parent be set to dev before calling
devm_watchdog_register_device()? Without this assignment, the watchdog
device is parented to the virtual sysfs root rather than the physical
platform device, which breaks udev hardware association rules.

[Severity: High]
Does unbinding and rebinding this platform driver leak watchdog status
state? Since wdt_dev is statically allocated and not zeroed during probe,
any state set during a previous bind (like the WDOG_ACTIVE bit) is
preserved. If an administrator unbinds and rebinds the driver, the core
will see WDOG_ACTIVE is already set and return success without calling
the hardware start routine, silently breaking the watchdog.

> +	if (ret)
> +		return ret;

[Severity: High]
This is a pre-existing issue, but does a failure in
devm_watchdog_register_device() leave the hardware watchdog running? If
the BIOS started the hardware watchdog before boot, w83627hf_init() resets
its timeout and leaves it ticking. Because probe returns an error and the
device node is never created, user-space has no way to ping the watchdog,
guaranteeing an unintended system reboot.

> +
> +	dev_info(dev, "initialized. timeout=%d sec (nowayout=%d)\n",
> +		 wdt_dev.timeout, nowayout);
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260726-w83627hf_wdt-improvements-v2-0-3645a2a6c022@bootlin.com?part=2