hwmon: add Altera SoC FPGA hardware monitoring support

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

New issues:
- [Medium] Hardware monitoring functionality is registered as a platform device rather than an auxiliary device, violating hwmon subsystem layering rules.

Pre-existing issues:
- [High] Asynchronous service layer APIs and resources are destroyed before client devices are unregistered, leading to a Use-After-Free and NULL pointer dereference race condition. --

--- Patch [1]: [PATCH v6 1/2] firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device --- commit 552947f984c2b5770daa9717c679362b5561cfa3
Author: Tze Yee Ng <[email protected]>

firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device

Add asynchronous Stratix 10 service layer support for hardware monitor temperature and voltage read commands in stratix10_svc_async_send() and stratix10_svc_async_prepare_response().

Register a socfpga-hwmon platform device from the service layer driver when hardware monitor support is enabled, similar to the RSU device.

Quoted 4 lines

drivers/firmware/stratix10-svc.c +0 -0

index c24ca5823078f..fc38afed5b7fe 100644

[ ... ]

Quoted 4 lines

drivers/firmware/stratix10-svc.c +0 -0

@@ -2013,16 +2028,38 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
  	if (ret)
  		goto err_put_device;
 

drivers/firmware/stratix10-svc.c static int stratix10_svc_drv_probe(struct platform_device *pdev) · line 2040

Quoted 10 lines

drivers/firmware/stratix10-svc.c +10 -0

+	if (IS_ENABLED(CONFIG_SENSORS_ALTERA_SOCFPGA_HWMON)) {
+		svc->stratix10_svc_hwmon =
+			platform_device_alloc(SOCFPGA_HWMON, 0);
+		if (!svc->stratix10_svc_hwmon) {
+			dev_err(dev, "failed to allocate %s device\n",
+				SOCFPGA_HWMON);
+		} else {
+			svc->stratix10_svc_hwmon->dev.parent = dev;
+
+			ret = platform_device_add(svc->stratix10_svc_hwmon);

[Severity: Medium]
Should the hardware monitoring functionality be implemented as an auxiliary device driver instead of a platform device?

The hardware monitoring subsystem guidelines state that when hardware monitoring is a secondary function of a chip, it should be implemented as an auxiliary device and reside in drivers/hwmon/.

Does registering socfpga-hwmon directly as a platform device from this firmware service driver bypass the architectural layering rules for hwmon?

[ ... ]

Quoted 9 lines

drivers/firmware/stratix10-svc.c +3 -0

@@ -2046,6 +2083,9 @@ static void stratix10_svc_drv_remove(struct platform_device *pdev)
  	struct stratix10_svc_controller *ctrl = platform_get_drvdata(pdev);
  	struct stratix10_svc *svc = ctrl->svc;
 
+	if (svc->stratix10_svc_hwmon)
+		platform_device_unregister(svc->stratix10_svc_hwmon);
+
  	stratix10_svc_async_exit(ctrl);
 

drivers/firmware/stratix10-svc.c static void stratix10_svc_drv_remove(struct platform_device *pdev) · line 2091

Quoted 1 line

drivers/firmware/stratix10-svc.c +0 -0

  	of_platform_depopulate(ctrl->dev);

[Severity: High]
This is a pre-existing issue, but can the destruction order in stratix10_svc_drv_remove() lead to a NULL pointer dereference or a use-after-free race condition?

When stratix10_svc_async_exit() is called, it destroys the async ID pool and sets actrl->invoke_fn to NULL:

drivers/firmware/stratix10-svc.c:stratix10_svc_async_exit() {
     ...
     ida_destroy(&actrl->async_id_pool);
     actrl->invoke_fn = NULL;
     ...
}

However, the RSU client device is unregistered after this destruction step via platform_device_unregister(svc->stratix10_svc_rsu).

If a user interacts with a client device's sysfs attribute while the module is being unloaded, could the still-active client device invoke stratix10_svc_async_send()?

If so, it would concurrently access freed asynchronous channels or attempt to execute actrl->invoke_fn(actrl, &args, &res) after it has been locklessly cleared, resulting in a kernel crash.

Would it be safer to unregister all child and client devices before the resources they depend on are torn down?

Quoted 60 lines · On 7/19/26 20:37, [email protected] wrote:

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

New issues:
- [Medium] Hardware monitoring functionality is registered as a platform device rather than an auxiliary device, violating hwmon subsystem layering rules.

Pre-existing issues:
- [High] Asynchronous service layer APIs and resources are destroyed before client devices are unregistered, leading to a Use-After-Free and NULL pointer dereference race condition.

Signature · 14 lines

--- Patch [1]: [PATCH v6 1/2] firmware: stratix10-svc: add async HWMON 
read commands and register socfpga-hwmon device ---
commit 552947f984c2b5770daa9717c679362b5561cfa3
Author: Tze Yee Ng <[email protected]>

firmware: stratix10-svc: add async HWMON read commands and register 
socfpga-hwmon device

Add asynchronous Stratix 10 service layer support for hardware monitor
temperature and voltage read commands in stratix10_svc_async_send() and
stratix10_svc_async_prepare_response().

Register a socfpga-hwmon platform device from the service layer driver
when hardware monitor support is enabled, similar to the RSU device.

Quoted 5 lines

drivers/firmware/ renamed from drivers/firmware/stratix10-svc.c +0 -0

stratix10-svc.c
index c24ca5823078f..fc38afed5b7fe 100644

drivers/firmware/stratix10-svc.c +0 -0

[ ... ]

Quoted 4 lines

drivers/firmware/stratix10-svc.c +0 -0

@@ -2013,16 +2028,38 @@ static int stratix10_svc_drv_probe(struct 
 platform_device *pdev)
       if (ret)
           goto err_put_device;

drivers/firmware/stratix10-svc.c static int stratix10_svc_drv_probe(struct · line 2040

Quoted 10 lines

drivers/firmware/stratix10-svc.c +10 -0

+    if (IS_ENABLED(CONFIG_SENSORS_ALTERA_SOCFPGA_HWMON)) {
+        svc->stratix10_svc_hwmon =
+            platform_device_alloc(SOCFPGA_HWMON, 0);
+        if (!svc->stratix10_svc_hwmon) {
+            dev_err(dev, "failed to allocate %s device\n",
+                SOCFPGA_HWMON);
+        } else {
+            svc->stratix10_svc_hwmon->dev.parent = dev;
+
+            ret = platform_device_add(svc->stratix10_svc_hwmon);

[Severity: Medium]
Should the hardware monitoring functionality be implemented as an auxiliary
device driver instead of a platform device?

The hardware monitoring subsystem guidelines state that when hardware monitoring is a secondary function of a chip, it should be implemented as an
auxiliary device and reside in drivers/hwmon/.

Quoted 4 lines

Does registering socfpga-hwmon directly as a platform device from this firmware service driver bypass the architectural layering rules for hwmon?

It should be implemented as auxiliary driver, but feel free to ignore since I only recently added this guidance to Sashiko.

Quoted 17 lines

[ ... ]

Quoted 10 lines

drivers/firmware/stratix10-svc.c +0 -0

@@ -2046,6 +2083,9 @@ static void stratix10_svc_drv_remove(struct 
 platform_device *pdev)

      struct stratix10_svc_controller *ctrl = platform_get_drvdata(pdev);
      struct stratix10_svc *svc = ctrl->svc;
+    if (svc->stratix10_svc_hwmon)
+        platform_device_unregister(svc->stratix10_svc_hwmon); +
      stratix10_svc_async_exit(ctrl); of_platform_depopulate(ctrl->dev);

[Severity: High]
This is a pre-existing issue, but can the destruction order in stratix10_svc_drv_remove() lead to a NULL pointer dereference or a use-after-free race condition?

Pre-existing, so your call if you want to fix it or not.

Thanks,
Guenter

Quoted 28 lines

When stratix10_svc_async_exit() is called, it destroys the async ID pool and sets actrl->invoke_fn to NULL:

drivers/firmware/stratix10-svc.c:stratix10_svc_async_exit() {
     ...
     ida_destroy(&actrl->async_id_pool); actrl->invoke_fn = NULL;
     ...
}

However, the RSU client device is unregistered after this destruction step
via platform_device_unregister(svc->stratix10_svc_rsu).

If a user interacts with a client device's sysfs attribute while the module
is being unloaded, could the still-active client device invoke stratix10_svc_async_send()?

If so, it would concurrently access freed asynchronous channels or attempt
to execute actrl->invoke_fn(actrl, &args, &res) after it has been locklessly
cleared, resulting in a kernel crash.

Would it be safer to unregister all child and client devices before the resources they depend on are torn down?

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing break in async poll loop causes severe kernel log spam and unnecessary blocking on hard errors.
- [High] Synchronous read timeouts leave stale requests in the channel FIFO, causing all subsequent reads to process stale data and return mismatched sensor values. --

--- Patch [2]: [PATCH v6 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver --- commit ab6c0c0f95d3890d759e04d9d9aecd151d6406fd
Author: Tze Yee Ng <[email protected]>
Subject: hwmon: add Altera SoC FPGA hardware monitoring driver

This commit adds a hardware monitoring driver for Altera SoC FPGA devices using the Stratix 10 service layer.

Quoted 5 lines

drivers/hwmon/socfpga-hwmon.c new file +0 -0

new file mode 100644
index 0000000000000..72418201b65d6

[ ... ]

Quoted 4 lines

drivers/hwmon/socfpga-hwmon.c new file +4 -0

+static int socfpga_hwmon_async_read(struct device *dev,
+				    enum hwmon_sensor_types type,
+				    struct stratix10_svc_client_msg *msg)
+{

[ ... ]

drivers/hwmon/socfpga-hwmon.c line 215

Quoted 5 lines

drivers/hwmon/socfpga-hwmon.c new file +5 -0

+	ret = -ETIMEDOUT;
+	while (!time_after(jiffies, deadline)) {
+		status = stratix10_svc_async_poll(priv->chan, handle, &data);
+		if (status == -EAGAIN) {
+			dev_dbg(dev, "Async message is still in progress\n");

drivers/hwmon/socfpga-hwmon.c line 218

Quoted 3 lines

drivers/hwmon/socfpga-hwmon.c new file +3 -0

+		} else if (status < 0) {
+			dev_alert(dev, "Failed to poll async message: %d\n", status);
+			ret = -ETIMEDOUT;

[Severity: High]
Does this loop need a break statement here when status < 0?

If stratix10_svc_async_poll() returns a hard error, the code sets ret = -ETIMEDOUT but continues to loop for up to 2 seconds. This could flood the kernel log with dev_alert() messages on every 1ms retry.

drivers/hwmon/socfpga-hwmon.c line 223

Quoted 5 lines

drivers/hwmon/socfpga-hwmon.c new file +5 -0

+		} else if (status == 0) {
+			ret = 0;
+			break;
+		}
+		msleep(HWMON_RETRY_SLEEP_MS);

Quoted 1 line

drivers/hwmon/socfpga-hwmon.c new file +1 -0

+	}

[ ... ]

Quoted 4 lines

drivers/hwmon/socfpga-hwmon.c new file +4 -0

+static int socfpga_hwmon_sync_read(struct device *dev,
+				   enum hwmon_sensor_types type,
+				   struct stratix10_svc_client_msg *msg)
+{

[ ... ]

drivers/hwmon/socfpga-hwmon.c line 272

Quoted 9 lines

drivers/hwmon/socfpga-hwmon.c new file +9 -0

+	ret = stratix10_svc_send(priv->chan, msg);
+	if (ret < 0)
+		goto status_done;
+
+	ret = wait_for_completion_timeout(&priv->completion, HWMON_TIMEOUT);
+	if (!ret) {
+		dev_err(priv->client.dev, "timeout waiting for SMC call\n");
+		ret = -ETIMEDOUT;
+		goto status_done;

[Severity: High]
If a timeout occurs here, could the unfulfilled request remain in the SVC channel's FIFO?

When the timeout hits, this path calls stratix10_svc_done() which stops the worker thread, but it doesn't appear to remove the request from chan->svc_fifo.

Would the next sensor read enqueue a new request and receive the stale data from this timed-out request, permanently desynchronizing all subsequent sensor readings?

Quoted 8 lines

drivers/hwmon/socfpga-hwmon.c new file +8 -0

+	}
+
+	ret = priv->last_err;
+
+status_done:
+	stratix10_svc_done(priv->chan);
+	return ret;
+}

Quoted 36 lines · On 7/19/26 20:36, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing break in async poll loop causes severe kernel log spam and unnecessary blocking on hard errors.
- [High] Synchronous read timeouts leave stale requests in the channel FIFO, causing all subsequent reads to process stale data and return mismatched sensor values.

Signature · 9 lines

--- Patch [2]: [PATCH v6 2/2] hwmon: add Altera SoC FPGA hardware 
monitoring driver ---
commit ab6c0c0f95d3890d759e04d9d9aecd151d6406fd
Author: Tze Yee Ng <[email protected]>
Subject: hwmon: add Altera SoC FPGA hardware monitoring driver

This commit adds a hardware monitoring driver for Altera SoC FPGA 
devices using
the Stratix 10 service layer.

Quoted 6 lines

drivers/hwmon/socfpga- renamed from drivers/hwmon/socfpga-hwmon.c +0 -0

hwmon.c
new file mode 100644
index 0000000000000..72418201b65d6

drivers/hwmon/socfpga-hwmon.c new file +0 -0

[ ... ]

Quoted 4 lines

drivers/hwmon/socfpga-hwmon.c new file +4 -0

+static int socfpga_hwmon_async_read(struct device *dev,
+                    enum hwmon_sensor_types type,
+                    struct stratix10_svc_client_msg *msg)
+{

[ ... ]

Quoted 5 lines

+    ret = -ETIMEDOUT;
+    while (!time_after(jiffies, deadline)) {
+        status = stratix10_svc_async_poll(priv->chan, handle, &data); +        if (status == -EAGAIN) {
+            dev_dbg(dev, "Async message is still in progress\n");

This also creates a lot of noise if debugging is active. I don't know how long the command usually takes, but it does seem to me that this debug message isn't really helpful.

Quoted 5 lines

Quoted 4 lines

+        } else if (status < 0) {
+            dev_alert(dev, "Failed to poll async message: %d\n", status);
+            ret = -ETIMEDOUT;

Quoted 7 lines

[Severity: High]
Does this loop need a break statement here when status < 0?

If stratix10_svc_async_poll() returns a hard error, the code sets ret = -ETIMEDOUT but continues to loop for up to 2 seconds. This could flood the kernel log with dev_alert() messages on every 1ms retry.

Sashiko has a good point, especially since the function returns -EINVAL which, if seen, seems to be  _real_ hard error and retrying seems pointless. Also, why replace -EINVAL with -ETIMEDOUT ? That would need an explanation.

On top of that, "ret = -ETIMEDOUT;" is pointless here since ret is already set to -ETIMEDOUT.

Oh, and the called code already displays an error message unless the parameters are bad.

Quoted 5 lines

Quoted 5 lines

+        } else if (status == 0) {
+            ret = 0;
+            break;
+        }
+        msleep(HWMON_RETRY_SLEEP_MS);

I would suggest to use usleep_range().

Quoted 25 lines

Quoted 1 line

+    }

[ ... ]

Quoted 4 lines

drivers/hwmon/socfpga-hwmon.c new file +4 -0

+static int socfpga_hwmon_sync_read(struct device *dev,
+                   enum hwmon_sensor_types type,
+                   struct stratix10_svc_client_msg *msg)
+{

[ ... ]

Quoted 10 lines

+    ret = stratix10_svc_send(priv->chan, msg);
+    if (ret < 0)
+        goto status_done;
+
+    ret = wait_for_completion_timeout(&priv->completion, HWMON_TIMEOUT);
+    if (!ret) {
+        dev_err(priv->client.dev, "timeout waiting for SMC call\n"); +        ret = -ETIMEDOUT;
+        goto status_done;

[Severity: High]
If a timeout occurs here, could the unfulfilled request remain in the SVC channel's FIFO?

When the timeout hits, this path calls stratix10_svc_done() which stops the
worker thread, but it doesn't appear to remove the request from chan-

Quoted 7 lines

Quoted 1 line

svc_fifo.

Would the next sensor read enqueue a new request and receive the stale data
from this timed-out request, permanently desynchronizing all subsequent sensor readings?

Please either address or explain why it is not a problem.

Thanks,
Guenter

Quoted 9 lines

Quoted 8 lines

+    }

drivers/hwmon/socfpga-hwmon.c new file +7 -0

+
+    ret = priv->last_err;
+
+status_done:
+    stratix10_svc_done(priv->chan);
+    return ret;
+}