Re: [PATCH 2/9] 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:
- [High] Potential NULL pointer dereference in `wdt_probe()` when matching via `driver_override`.
- [High] State contamination across unbind/bind cycles due to the use of shared global device structures.
- [Medium] The watchdog device's parent is not set, resulting in an incomplete sysfs topology and preventing property parsing.
- [Low] An uninitialized empty structure `struct wdt_pdata` is passed to `platform_device_register_data()`.
--

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

watchdog: w83627hf_wdt: Convert to platform driver model

This commit converts the w83627hf_wdt driver to use the Linux driver model
by registering it as a platform driver. It replaces direct device 
initialization and registration with a probe function and standard
platform device structures.

> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
> index 1529a4e168209..993e65ce4447c 100644
> --- a/drivers/watchdog/w83627hf_wdt.c
> +++ b/drivers/watchdog/w83627hf_wdt.c

[ ... ]

> @@ -464,6 +467,37 @@ 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]
Could platform_get_device_id() return a NULL pointer here if a privileged
user manually forces the device to bind to this driver using the
driver_override sysfs interface?

If so, does id->driver_data trigger a NULL pointer dereference since the
platform bus matching logic bypasses the id_table?

> +	int ret;
> +
> +	pr_info("WDT driver for %s Super I/O chip initialising\n", id->name);
> +
> +	snprintf(wdt_info.identity, sizeof(wdt_info.identity), "%s Watchdog",
> +		 id->name);
> +
> +	watchdog_init_timeout(&wdt_dev, timeout, NULL);

[Severity: High]
Does modifying statically allocated global structures like wdt_info and
wdt_dev instead of using dynamically allocated per-device state cause state
contamination across unbind/bind cycles?

If the driver is unbound and rebound via sysfs, the wdt_dev.status bitmask
retains accumulated state flags like WDOG_HW_RUNNING. If this flag is falsely
retained, wouldn't the watchdog core spawn a worker thread to automatically
ping the watchdog on behalf of the kernel, defeating the watchdog's purpose
if userspace hangs?

> +	watchdog_set_nowayout(&wdt_dev, nowayout);
> +	watchdog_stop_on_reboot(&wdt_dev);
> +
> +	ret = w83627hf_init(&wdt_dev, chip);
> +	if (ret) {
> +		pr_err("failed to initialize watchdog (err=%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_watchdog_register_device(&pdev->dev, &wdt_dev);

[Severity: Medium]
Is the sysfs topology broken here because the global wdt_dev struct lacks
a .parent = &pdev->dev assignment before registration?

Since the parent is not set and watchdog_init_timeout() earlier is passed a
NULL device pointer, won't the watchdog character device be created at the
root of the sysfs hierarchy instead of as a child of the platform device,
preventing standard properties like timeout-sec from being parsed?

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

[ ... ]

> @@ -533,35 +580,25 @@ static int __init wdt_init(void)
>  			return chip;
>  	}
>  
> -	pr_info("WDT driver for %s Super I/O chip initialising\n",
> -		chip_name[chip]);
> -
> -	snprintf(wdt_info.identity, sizeof(wdt_info.identity), "%s Watchdog",
> -		 chip_name[chip]);
> -
> -	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) {
> -		pr_err("failed to initialize watchdog (err=%d)\n", ret);
> -		return ret;
> -	}
> -
> -	ret = watchdog_register_device(&wdt_dev);
> +	ret = platform_driver_register(&wdt_driver);
>  	if (ret)
>  		return ret;
>  
> -	pr_info("initialized. timeout=%d sec (nowayout=%d)\n",
> -		wdt_dev.timeout, nowayout);
> +	wdt_pdev = platform_device_register_data(NULL, wdt_ids[chip].name,
> +						 PLATFORM_DEVID_NONE, &pdata,
> +						 sizeof(pdata));

[Severity: Low]
Does passing the uninitialized stack variable pdata serve a purpose here?
Although sizeof(pdata) evaluating to 0 makes it currently safe, would it
be clearer to pass NULL and 0 for the size instead?

> +	if (IS_ERR(wdt_pdev)) {
> +		platform_driver_unregister(&wdt_driver);
> +		return PTR_ERR(wdt_pdev);
> +	}
>  
> -	return ret;
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725-w83627hf_wdt-improvements-v1-0-4e9a1b4e8297@bootlin.com?part=2