[PATCH v1] watchdog: ni903x_wdt: Check ACPI_COMPANION() against NULL

[PATCH v1] watchdog: ni903x_wdt: Check ACPI_COMPANION() against NULL

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

Every platform driver can be forced to match a device that doesn't match
its list of device IDs because of device_match_driver_override(), so
platform drivers that rely on the existence of a device's ACPI companion
object need to verify its presence.

Accordingly, add a requisite ACPI_COMPANION() check against NULL to the
ni903x_wdt watchdog driver.

Fixes: d37ec2fbab55 ("watchdog: ni903x_wdt: Convert to a platform driver")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/watchdog/ni903x_wdt.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

--- a/drivers/watchdog/ni903x_wdt.c
+++ b/drivers/watchdog/ni903x_wdt.c
@@ -183,9 +183,14 @@ static int ni903x_acpi_probe(struct plat
 	struct device *dev = &pdev->dev;
 	struct watchdog_device *wdd;
 	struct ni903x_wdt *wdt;
+	acpi_handle handle;
 	acpi_status status;
 	int ret;
 
+	handle = ACPI_HANDLE(dev);
+	if (!handle)
+		return -ENODEV;
+
 	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
 	if (!wdt)
 		return -ENOMEM;
@@ -193,7 +198,7 @@ static int ni903x_acpi_probe(struct plat
 	platform_set_drvdata(pdev, wdt);
 	wdt->dev = dev;
 
-	status = acpi_walk_resources(ACPI_HANDLE(dev), METHOD_NAME__CRS,
+	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
 				     ni903x_resources, wdt);
 	if (ACPI_FAILURE(status) || wdt->io_base == 0) {
 		dev_err(dev, "failed to get resources\n");

Re: [PATCH v1] watchdog: ni903x_wdt: Check ACPI_COMPANION() against NULL

[Andy Shevchenko]

On Tue, May 12, 2026 at 06:22:57PM +0200, Rafael J. Wysocki wrote:

> Every platform driver can be forced to match a device that doesn't match
> its list of device IDs because of device_match_driver_override(), so
> platform drivers that rely on the existence of a device's ACPI companion
> object need to verify its presence.
> 
> Accordingly, add a requisite ACPI_COMPANION() check against NULL to the
> ni903x_wdt watchdog driver.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

...

> -	status = acpi_walk_resources(ACPI_HANDLE(dev), METHOD_NAME__CRS,
> +	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
>  				     ni903x_resources, wdt);

This smells like we can move to regular acpi_dev_*() resource APIs rather than
custom walking via _CRS. But I haven't looked into the code.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] watchdog: ni903x_wdt: Check ACPI_COMPANION() against NULL

[Guenter Roeck]

On Tue, May 12, 2026 at 06:22:57PM +0200, Rafael J. Wysocki wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> Every platform driver can be forced to match a device that doesn't match
> its list of device IDs because of device_match_driver_override(), so
> platform drivers that rely on the existence of a device's ACPI companion
> object need to verify its presence.
> 
> Accordingly, add a requisite ACPI_COMPANION() check against NULL to the
> ni903x_wdt watchdog driver.
> 
> Fixes: d37ec2fbab55 ("watchdog: ni903x_wdt: Convert to a platform driver")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Applied.

Thanks,
Guenter

Re: [PATCH v1] watchdog: ni903x_wdt: Check ACPI_COMPANION() against NULL

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The watchdog device is exposed to userspace before its hardware initialization is fully complete in `ni903x_acpi_probe`.
- [High] The driver's removal function races with the watchdog core worker and userspace, potentially leaving the hardware running after driver unload.
--

commit 6254d703c5994e457b8ddb7846a5094ab95cecda
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

watchdog: ni903x_wdt: Check ACPI_COMPANION() against NULL

This commit adds a check to ensure the ACPI companion object exists
before proceeding with the ni903x_wdt watchdog driver probe.

> diff --git a/drivers/watchdog/ni903x_wdt.c b/drivers/watchdog/ni903x_wdt.c
> --- a/drivers/watchdog/ni903x_wdt.c
> +++ b/drivers/watchdog/ni903x_wdt.c
> @@ -183,9 +183,14 @@ static int ni903x_acpi_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct watchdog_device *wdd;
>  	struct ni903x_wdt *wdt;
> +	acpi_handle handle;
>  	acpi_status status;
>  	int ret;
>  
> +	handle = ACPI_HANDLE(dev);
> +	if (!handle)
> +		return -ENODEV;
> +

This isn't a regression introduced by this commit, but while reviewing the
probe function modified here, I noticed a potential race condition.

In ni903x_acpi_probe(), the watchdog device is registered before the
hardware initialization is fully complete:

ni903x_acpi_probe() {
    ...
    ret = watchdog_register_device(wdd);
    if (ret)
        return ret;

    outb(NIWD_CONTROL_RESET | NIWD_CONTROL_MODE,
         wdt->io_base + NIWD_CONTROL);
    ...
}

Since the character device becomes visible to userspace immediately upon
registration, could a fast userspace process open the device and trigger
ni903x_wdd_start() before the hardware is initialized?

If ni903x_wdd_start() configures the watchdog by writing to the control
register, wouldn't the subsequent outb() in the probe thread clobber this
configuration without preserving the bits set by userspace?

This might leave the watchdog in an unpredictable state and prevent recovery
on a freeze.

There is also a similar pre-existing issue in the removal path.

If ni903x_acpi_remove() calls ni903x_wdd_stop() before
watchdog_unregister_device(), the device remains registered with the
watchdog core while stopping.

Could userspace or the watchdog core worker concurrently execute
ni903x_wdd_ping() during this time?

If ni903x_wdd_ping() performs a read-modify-write on the control register,
and ni903x_wdd_stop() writes a stop command to the same register, is it
possible for the ping operation to write back the active state and re-enable
the watchdog if the stop operation executes between the read and write?

If the driver then unloads while the hardware watchdog continues running
with no driver to pet it, will the system spuriously reboot?

Would it be safer to call watchdog_unregister_device() before stopping the
hardware, or perhaps use watchdog_stop_on_unregister()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/2280455.irdbgypaU6@rafael.j.wysocki?part=1