hwmon: add Altera SoC FPGA hardware monitoring support
Subsystems (4)
[email protected]
maintainerv7 cover hwmon: add Altera SoC FPGA hardware monitoring support
From: Tze Yee Ng <[email protected]>
This series adds hardware monitor support for Altera SoC FPGA devices. Temperature and voltage sensors are accessed through the Stratix 10 service layer and Secure Device Manager (SDM).
Patch 1 adds async HWMON SMC support to stratix10-svc and registers the socfpga-hwmon platform device.
Patch 2 adds the socfpga-hwmon driver, documentation, Kconfig, and MAINTAINERS entry.
Changes in v7:
- In Patch 2, address maintainer and Sashiko review feedback on socfpga-hwmon:
- Break out of the async poll loop on hard errors and return the real poll status instead of rewriting it to -ETIMEDOUT
- Drop redundant async poll alert/debug messages (SVC already logs)
- Replace msleep(1) with usleep_range() for short retries
- On sync-mode timeout, wait for the outstanding SVC callback before calling stratix10_svc_done() so a leftover svc_fifo request cannot desynchronize later sensor reads; still report -ETIMEDOUT
- No functional changes in Patch 1
Changes in v6:
- Rebase on torvalds/master (given “Linux 7.2-rc4”)
- No functional changes in Patch 1 and Patch 2
Changes in v5:
- Rebase on dinguyen/socfpga_svc_fixes_for_v7.2
- Address Sashiko review feedback on socfpga-hwmon (Patch 2):
- Poll async responses until HWMON_TIMEOUT instead of a fixed 3-iteration retry loop (~3 ms), fixing premature timeouts on silicon
- Add MODULE_ALIAS("platform:socfpga-hwmon")
- No functional changes in Patch 1
Changes in v4:
- Address maintainer and review feedback on socfpga-hwmon (Patch 2):
- Register devm_add_action_or_reset() before devm_hwmon_device_register_with_info() to fix devres teardown order
- Remove unreferenced completion and pre-poll wait_for_completion_io_timeout() from async reads; poll directly with a retry loop after async_send()
- No functional changes in Patch 1
Changes in v3:
- Address review feedback on socfpga-hwmon (Patch 2):
- Fix 16-bit Q8.8 temperature sign extension
- Drop unused async callback; pass NULL to stratix10_svc_async_send()
- Document and retain pre-poll wait (RSU pattern; firmware needs time before async_poll())
- Align async poll retry behaviour with RSU
- Use uninterruptible wait_for_completion_timeout() for sync reads
- Handle -EINVAL and -EOPNOTSUPP when falling back to sync mode
- Defer SVC channel cleanup via devm_add_action_or_reset()
- No functional changes in Patch 1
Changes in v2:
- Drop altr,stratix10-hwmon DT binding and intel,stratix10-svc hwmon child property
- Drop Stratix 10 SoCDK DTS hwmon node
- Register socfpga-hwmon from stratix10-svc (RSU-style)
- Replace DT channel parsing with hardcoded Stratix 10 and Agilex tables
- Rename driver/module to socfpga-hwmon (CONFIG_SENSORS_ALTERA_SOCFPGA_HWMON)
- Add Agilex channel support
- Fix SDM value conversion (Q8.8 degrees Celsius and Q16 volts to hwmon millidegrees/millivolts)
- Improve sync-mode error handling via last_err
Previous version:
https://lore.kernel.org/all/[email protected]/
Tze Yee Ng (2):
firmware: stratix10-svc: add async HWMON read commands and register
socfpga-hwmon device
hwmon: add Altera SoC FPGA hardware monitoring driver
Documentation/hwmon/index.rst | 1 + Documentation/hwmon/socfpga-hwmon.rst | 34 ++ MAINTAINERS | 8 + drivers/firmware/stratix10-svc.c | 46 +- drivers/hwmon/Kconfig | 10 + drivers/hwmon/Makefile | 1 + drivers/hwmon/socfpga-hwmon.c | 586 +++++++++++++++++++ include/linux/firmware/intel/stratix10-smc.h | 38 ++ 8 files changed, 721 insertions(+), 3 deletions(-) create mode 100644 Documentation/hwmon/socfpga-hwmon.rst create mode 100644 drivers/hwmon/socfpga-hwmon.c
Signature · 1 line
2.43.7
[email protected]
maintainerPATCH v7 1/2 firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device
From: Tze Yee Ng <[email protected]>
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.
---
Changes in v7:
- No functional changes from v6
Changes in v6:
- No functional changes from v5
Changes in v5:
- No functional changes from v4
Changes in v3:
- No functional changes from v2
Changes in v2:
- Extend patch scope beyond async SMC support: register socfpga-hwmon platform device from stratix10-svc when CONFIG_SENSORS_ALTERA_SOCFPGA_HWMON is enabled
- Follow RSU-style registration; RSU probe error handling is unchanged
- Add err_unregister_clients to unregister hwmon and RSU on populate failure
- Unregister hwmon platform device in stratix10-svc remove()
--- drivers/firmware/stratix10-svc.c | 46 ++++++++++++++++++-- include/linux/firmware/intel/stratix10-smc.h | 38 ++++++++++++++++ 2 files changed, 81 insertions(+), 3 deletions(-)
drivers/firmware/stratix10-svc.c +43 -3
@@ -45,6 +45,7 @@/* stratix10 service layer clients */ #define STRATIX10_RSU "stratix10-rsu" +#define SOCFPGA_HWMON "socfpga-hwmon"/* Maximum number of SDM client IDs. */ #define MAX_SDM_CLIENT_IDS 16
@@ -104,9 +105,11 @@ struct stratix10_svc_chan;/** * struct stratix10_svc - svc private data * @stratix10_svc_rsu: pointer to stratix10 RSU device +* @stratix10_svc_hwmon: pointer to stratix10 HWMON device */ struct stratix10_svc {struct platform_device *stratix10_svc_rsu; +struct platform_device *stratix10_svc_hwmon; };/**
@@ -1329,6 +1332,14 @@ int stratix10_svc_async_send(struct stratix10_svc_chan *chan, void *msg, args.a0 =INTEL_SIP_SMC_ASYNC_RSU_NOTIFY ; args.a2 = p_msg->arg[0 ];break ; +case COMMAND_HWMON_READTEMP : + args.a0 =INTEL_SIP_SMC_ASYNC_HWMON_READTEMP ; + args.a2 = p_msg->arg[0 ]; +break ; +case COMMAND_HWMON_READVOLT : + args.a0 =INTEL_SIP_SMC_ASYNC_HWMON_READVOLT ; + args.a2 = p_msg->arg[0 ]; +break ;default :dev_err (ctrl->dev,"Invalid command ,%d\n" , p_msg->command); ret = -EINVAL ;
@@ -1422,6 +1433,10 @@ static int stratix10_svc_async_prepare_response(struct stratix10_svc_chan *chan,*/ data->kaddr1 = (void *)&handle->res;break ; +case COMMAND_HWMON_READTEMP : +case COMMAND_HWMON_READVOLT : + data->kaddr1 = (void *)&handle->res.a2; +break ;default :dev_alert (ctrl->dev,"Invalid command\n ,%d" , p_msg->command);
@@ -2013,16 +2028,38 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)if (ret) goto err_put_device; +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); +if (ret) { +dev_err (dev,"failed to add %s device: %d\n" , +SOCFPGA_HWMON , ret); +platform_device_put (svc->stratix10_svc_hwmon); + svc->stratix10_svc_hwmon =NULL ; + } + } + } + ret =of_platform_default_populate (dev_of_node (dev),NULL , dev);if (ret) - goto err_unregister_rsu_dev; + goto err_unregister_clients;pr_info ("Intel Service Layer Driver Initialized\n" );return 0 ; -err_unregister_rsu_dev: -platform_device_unregister (svc->stratix10_svc_rsu); +err_unregister_clients: +if (svc->stratix10_svc_hwmon) +platform_device_unregister (svc->stratix10_svc_hwmon); +if (svc->stratix10_svc_rsu) +platform_device_unregister (svc->stratix10_svc_rsu); goto err_free_fifos; err_put_device:platform_device_put (svc->stratix10_svc_rsu);
@@ -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);
include/linux/firmware/intel/stratix10-smc.h +38 -0
@@ -695,6 +695,44 @@ INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_COMPLETED_WRITE)#define INTEL_SIP_SMC_ASYNC_POLL \ INTEL_SIP_SMC_ASYNC_VAL(INTEL_SIP_SMC_ASYNC_FUNC_ID_POLL) +/** +* Request INTEL_SIP_SMC_ASYNC_HWMON_READTEMP +* Async call to request temperature +* +* Call register usage: +* a0 INTEL_SIP_SMC_ASYNC_HWMON_READTEMP +* a1 transaction job id +* a2 Temperature Channel +* a3-a17 not used +* +* Return status +* a0 INTEL_SIP_SMC_STATUS_OK, INTEL_SIP_SMC_STATUS_REJECTED +* or INTEL_SIP_SMC_STATUS_BUSY +* a1-a17 not used +*/ +#define INTEL_SIP_SMC_ASYNC_FUNC_ID_HWMON_READTEMP 0xE8 +#define INTEL_SIP_SMC_ASYNC_HWMON_READTEMP \ + INTEL_SIP_SMC_ASYNC_VAL(INTEL_SIP_SMC_ASYNC_FUNC_ID_HWMON_READTEMP) + +/** +* Request INTEL_SIP_SMC_ASYNC_HWMON_READVOLT +* Async call to request voltage +* +* Call register usage: +* a0 INTEL_SIP_SMC_ASYNC_HWMON_READVOLT +* a1 transaction job id +* a2 Voltage Channel +* a3-a17 not used +* +* Return status +* a0 INTEL_SIP_SMC_STATUS_OK, INTEL_SIP_SMC_STATUS_REJECTED +* or INTEL_SIP_SMC_STATUS_BUSY +* a1-a17 not used +*/ +#define INTEL_SIP_SMC_ASYNC_FUNC_ID_HWMON_READVOLT 0xE9 +#define INTEL_SIP_SMC_ASYNC_HWMON_READVOLT \ + INTEL_SIP_SMC_ASYNC_VAL(INTEL_SIP_SMC_ASYNC_FUNC_ID_HWMON_READVOLT) +/** * Request INTEL_SIP_SMC_ASYNC_RSU_GET_SPT * Async call to get RSU SPT from SDM.
Signature · 1 line
2.43.7
[email protected]
AI reviewRe: [PATCH v7 1/2] firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [Medium] The firmware driver registers a platform device for hardware monitoring instead of using the required auxiliary device framework.
Pre-existing issues:
- [High] The device node fw_np is used after its reference is automatically dropped by of_find_matching_node(), causing a Use-After-Free.
- [High] The async subsystem is destroyed before unregistering the RSU client and DT children, creating a race condition that leads to a Use-After-Free (UAF). --
--- Patch [1]: [PATCH v7 1/2] firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device --- commit 2168de7740e163afb68a564f62edd7410c642631
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. Register a socfpga-hwmon platform device from the service layer driver when hardware monitor support is enabled.
Quoted 4 lines
drivers/firmware/stratix10-svc.c +0
-0
[ ... ]
drivers/firmware/stratix10-svc.c static int stratix10_svc_drv_probe(struct platform_device *pdev) · line 2033
Quoted 7 lines
drivers/firmware/stratix10-svc.c +3
-0@@ -2013,16 +2028,38 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)if (ret) goto err_put_device; +if (IS_ENABLED (CONFIG_SENSORS_ALTERA_SOCFPGA_HWMON )) { + svc->stratix10_svc_hwmon = +platform_device_alloc (SOCFPGA_HWMON ,0 );
[Severity: Medium]
Does this code register a platform device for hardware monitoring instead of using the required auxiliary device framework?
According to the hardware monitoring subsystem guidelines, secondary hardware monitoring functionality in multi-function devices should be implemented as an auxiliary device on the auxiliary bus, rather than a standalone platform device.
Quoted 20 lines
drivers/firmware/stratix10-svc.c +17
-1+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); +if (ret) { +dev_err (dev,"failed to add %s device: %d\n" , +SOCFPGA_HWMON , ret); +platform_device_put (svc->stratix10_svc_hwmon); + svc->stratix10_svc_hwmon =NULL ; + } + } + } + ret =of_platform_default_populate (dev_of_node (dev),NULL , dev);if (ret) - goto err_unregister_rsu_dev; + goto err_unregister_clients;
[ ... ]
drivers/firmware/stratix10-svc.c static void stratix10_svc_drv_remove(struct platform_device *pdev) · line 2091
Quoted 10 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);of_platform_depopulate (ctrl->dev);
[Severity: High]
This is a pre-existing issue, but does this teardown order destroy the async infrastructure before safely unregistering the dependent child devices?
In stratix10_svc_drv_remove(), stratix10_svc_async_exit() is executed before platform_device_unregister() for the RSU child device. This frees the achan structures.
If a concurrent sysfs access on the still-registered child device calls stratix10_svc_async_send() and is preempted right after checking actrl->initialized:
if (!actrl->initialized) {
...And the removal thread proceeds to free achan, wouldn't the sysfs thread then resume and dereference the freed achan:
ret = ida_alloc_max(&achan->job_id_pool, MAX_SDM_JOB_IDS,
GFP_KERNEL);leading to a use-after-free?
[ ... ]
[Severity: High]
This is a pre-existing issue, but does of_find_matching_node() drop the reference to fw_np before it is used again in stratix10_svc_init()?
Looking at stratix10_svc_init():
np = of_find_matching_node(fw_np, stratix10_svc_drv_match);
if (!np)
return -ENODEV;
of_node_put(np);
ret = of_platform_populate(fw_np, stratix10_svc_drv_match, NULL, NULL);Since of_find_matching_node() automatically drops the reference on the from node (fw_np) before advancing, doesn't passing fw_np to of_platform_populate() immediately after result in a use-after-free?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
[email protected]
maintainerPATCH v7 2/2 hwmon: add Altera SoC FPGA hardware monitoring driver
In reply to [email protected]From: Tze Yee Ng <[email protected]>
Add a hardware monitor driver for Altera SoC FPGA devices using the Stratix 10 service layer. Sensor channels are selected based on the service layer compatible string.
---
Changes in v7:
- Break out of the async poll loop on hard errors; return the real poll status instead of -ETIMEDOUT
- Drop redundant async poll alert and -EAGAIN debug messages
- Replace msleep(1) with usleep_range() for short retries
- On sync-mode timeout, wait for the outstanding SVC callback before stratix10_svc_done() to avoid leftover svc_fifo entries desynchronizing later reads; still report -ETIMEDOUT to userspace
Changes in v6:
- No functional changes from v5
Changes in v5:
- Poll async responses until HWMON_TIMEOUT (2 s) instead of a fixed 3-iteration retry loop (~3 ms), fixing premature timeouts observed on silicon
- Add MODULE_ALIAS("platform:socfpga-hwmon")
Changes in v4:
- Register devm_add_action_or_reset() before devm_hwmon_device_register_with_info(); drop manual channel cleanup on hwmon registration failure
- Remove unreferenced async completion and pre-poll wait_for_completion_io_timeout(); poll directly after async_send() with the existing retry loop
Changes in v3:
- Fix 16-bit signed Q8.8 temperature conversion (cast through s16)
- Remove unused async callback; pass NULL to stratix10_svc_async_send()
- Keep wait_for_completion_io_timeout() before polling with comment explaining the service layer never invokes the callback but firmware needs time to complete the transaction (RSU pattern)
- Align async poll loop with RSU (retry on failure instead of aborting)
- Use wait_for_completion_timeout() for synchronous reads
- Handle -EINVAL and -EOPNOTSUPP when async client registration fails
- Defer SVC channel/async cleanup via devm_add_action_or_reset(); drop .remove()
Changes in v2:
- Drop altr,stratix10-hwmon OF compatible and DT channel parsing
- Select channels from hardcoded tables using parent SVC compatible (intel,stratix10-svc or intel,agilex-svc)
- Rename driver from stratix10-hwmon to socfpga-hwmon
- Rename Kconfig symbol to CONFIG_SENSORS_ALTERA_SOCFPGA_HWMON
- Add Agilex voltage and temperature channel tables
- Convert SDM Q8.8 degrees Celsius to hwmon millidegrees
- Convert SDM Q16 volts to hwmon millivolts
- Use socfpga_hwmon as hwmon sysfs device name
- Add last_err for synchronous SVC read error propagation
- Update Documentation/hwmon and MAINTAINERS accordingly
--- Documentation/hwmon/index.rst | 1 + Documentation/hwmon/socfpga-hwmon.rst | 34 ++ MAINTAINERS | 8 + drivers/hwmon/Kconfig | 10 + drivers/hwmon/Makefile | 1 + drivers/hwmon/socfpga-hwmon.c | 586 ++++++++++++++++++++++++++ 6 files changed, 640 insertions(+) create mode 100644 Documentation/hwmon/socfpga-hwmon.rst create mode 100644 drivers/hwmon/socfpga-hwmon.c
Documentation/hwmon/index.rst +1 -0
@@ -254,6 +254,7 @@ Hardware Monitoring Kernel Drivers sparx5-temp spd5118 stpddc60 + socfpga-hwmon surface_fan sy7636a-hwmon tc654
Documentation/hwmon/socfpga-hwmon.rst new file +34 -0
@@ -0,0 +1,34 @@ +.. SPDX-License-Identifier: GPL-2.0 + +Kernel driver socfpga-hwmon +============================= + +Supported chips: + + * Altera Stratix 10 SoC FPGA + * Altera Agilex SoC FPGA + +Authors: + - Nazim Amirul <[email protected]> + - Tze Yee Ng <[email protected]> + +Description +----------- + +This driver supports hardware monitoring for Altera SoC +FPGA devices through the Secure Device Manager and Stratix 10 service layer. + +The following sensor types are supported: + + * temperature + * voltage + +Usage Notes +----------- + +The stratix10-svc driver registers a socfpga-hwmon platform device when +hardware monitor support is enabled. Sensor channels are selected in the +driver based on the service layer compatible string: + + * intel,stratix10-svc + * intel,agilex-svc
MAINTAINERS +8 -0
@@ -965,6 +965,14 @@ L: [email protected] S: Maintained F: drivers/gpio/gpio-altera.c +ALTERA SoC FPGA HWMON DRIVER +M: Nazim Amirul <[email protected]> +M: Tze Yee Ng <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/hwmon/socfpga-hwmon.rst +F: drivers/hwmon/socfpga-hwmon.c + ALTERA TRIPLE SPEED ETHERNET DRIVER M: Boon Khai Ng <[email protected]> L: [email protected]
drivers/hwmon/Kconfig +10 -0
@@ -2160,6 +2160,16 @@ config SENSORS_SMSC47M192 This driver can also be built as a module. If so, the module will be called smsc47m192. +config SENSORS_ALTERA_SOCFPGA_HWMON + tristate "Altera SoC FPGA hardware monitoring features" + depends on INTEL_STRATIX10_SERVICE + help + If you say yes here you get support for the temperature and + voltage sensors of Altera SoC FPGA devices. + + This driver can also be built as a module. If so, the module + will be called socfpga-hwmon. + config SENSORS_SMSC47B397 tristate "SMSC LPC47B397-NC" depends on HAS_IOPORT
drivers/hwmon/Makefile +1 -0
@@ -221,6 +221,7 @@ obj-$(CONFIG_SENSORS_SMPRO) += smpro-hwmon.o obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o obj-$(CONFIG_SENSORS_SMSC47M1) += smsc47m1.o obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o +obj-$(CONFIG_SENSORS_ALTERA_SOCFPGA_HWMON) += socfpga-hwmon.o obj-$(CONFIG_SENSORS_SPARX5) += sparx5-temp.o obj-$(CONFIG_SENSORS_SPD5118) += spd5118.o obj-$(CONFIG_SENSORS_STTS751) += stts751.o
drivers/hwmon/socfpga-hwmon.c new file +586 -0
586 lines not on this page · raw message
Signature · 1 line
2.43.7
[email protected]
AI reviewRe: [PATCH v7 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The driver converts a timeout condition into an infinite wait on error paths, causing the calling…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The driver converts a timeout condition into an infinite wait on error paths, causing the calling process to hang in an unkillable D-state if the firmware fails to respond or drops the request. --
--- Patch [2]: [PATCH v7 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver --- commit 0a6e38858fabeb08e3528c04db8dd1a253733f50
Author: Tze Yee Ng <[email protected]>
hwmon: add Altera SoC FPGA hardware monitoring driver
This commit adds a hardware monitor driver for Altera SoC FPGA devices using the Stratix 10 service layer. Sensor channels are selected based on the service layer compatible string.
Quoted 4 lines
drivers/hwmon/socfpga-hwmon.c new file +0
-0
[ ... ]
Quoted 18 lines
drivers/hwmon/socfpga-hwmon.c new file +18
-0+static int socfpga_hwmon_sync_read (struct device *dev, +enum hwmon_sensor_types type, +struct stratix10_svc_client_msg *msg) +{ +struct socfpga_hwmon_priv *priv =dev_get_drvdata (dev); +int ret; + +reinit_completion (&priv->completion); + +if (type == hwmon_temp) + priv->client.receive_cb = socfpga_hwmon_readtemp_cb; +else + priv->client.receive_cb = socfpga_hwmon_readvolt_cb; + + ret =stratix10_svc_send (priv->chan, msg); +if (ret <0 ) + goto status_done; +
drivers/hwmon/socfpga-hwmon.c line 277
Quoted 10 lines
drivers/hwmon/socfpga-hwmon.c new file +10
-0+ ret =wait_for_completion_timeout (&priv->completion,HWMON_TIMEOUT ); +if (!ret) { +dev_err (priv->client.dev,"timeout waiting for SMC call\n" ); +/* +* stratix10_svc_done() stops the worker but does not flush +* svc_fifo. Wait for the outstanding callback before stopping +* so a leftover request cannot complete a later sensor read. +* Discard the late result and still report -ETIMEDOUT. +*/ +wait_for_completion (&priv->completion);
[Severity: High]
Does this unconditional wait_for_completion() cause the reading process to hang indefinitely in an unkillable D-state?
If the firmware fails to respond, or if the underlying service thread receives a busy or rejected status from the firmware during the SMC call, the service thread skips invoking the receive_cb callback entirely.
Because the callback is never invoked in these failure modes, wait_for_completion_timeout() correctly expires. However, the subsequent wait_for_completion() then blocks forever while the process still holds priv->lock, deadlocking all future sensor reads for the device.
Quoted 10 lines
drivers/hwmon/socfpga-hwmon.c new file +10
-0+ ret = -ETIMEDOUT ; + goto status_done; + } + + ret = priv->last_err; + +status_done: +stratix10_svc_done (priv->chan); +return ret; +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Guenter Roeck
maintainerRe: [PATCH v7 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver
In reply to [email protected]Quoted 9 lines · On 7/20/26 19:18, [email protected] wrote:
From: Tze Yee Ng <[email protected]>
Add a hardware monitor driver for Altera SoC FPGA devices using the Stratix 10 service layer. Sensor channels are selected based on the service layer compatible string.
Signed-off-by Nazim Amirul <[email protected]>Signed-off-by Tze Yee Ng <[email protected]>---
...
Quoted 19 lines
drivers/hwmon/socfpga-hwmon.c new file +19
-0+ +static int socfpga_hwmon_sync_read (struct device *dev, +enum hwmon_sensor_types type, +struct stratix10_svc_client_msg *msg) +{ +struct socfpga_hwmon_priv *priv =dev_get_drvdata (dev); +int ret; + +reinit_completion (&priv->completion); + +if (type == hwmon_temp) + priv->client.receive_cb = socfpga_hwmon_readtemp_cb; +else + priv->client.receive_cb = socfpga_hwmon_readvolt_cb; + + ret =stratix10_svc_send (priv->chan, msg); +if (ret <0 ) + goto status_done; +
drivers/hwmon/socfpga-hwmon.c line 277
Quoted 10 lines
drivers/hwmon/socfpga-hwmon.c new file +10
-0+ ret =wait_for_completion_timeout (&priv->completion,HWMON_TIMEOUT ); +if (!ret) { +dev_err (priv->client.dev,"timeout waiting for SMC call\n" ); +/* +* stratix10_svc_done() stops the worker but does not flush +* svc_fifo. Wait for the outstanding callback before stopping +* so a leftover request cannot complete a later sensor read. +* Discard the late result and still report -ETIMEDOUT. +*/ +wait_for_completion (&priv->completion);
Just like Sashiko, I am puzzled and do not understand this code.
Guenter
NG, TZE YEE
maintainerRe: [PATCH v7 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver
Quoted 44 lines · Guenter Roeck
Quoted 10 lines · On 7/20/26 19:18, [email protected] wrote:
From: Tze Yee Ng <[email protected]>
Add a hardware monitor driver for Altera SoC FPGA devices using the Stratix 10 service layer. Sensor channels are selected based on the service layer compatible string.
Signed-off-by: Nazim Amirul
<[email protected]>Signed-off-by Tze Yee Ng <[email protected]>---
...
Quoted 19 lines
drivers/hwmon/socfpga-hwmon.c new file +19
-0+ +static int socfpga_hwmon_sync_read (struct device *dev, + enum hwmon_sensor_types type, + struct stratix10_svc_client_msg *msg) +{ + struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev); + int ret; + + reinit_completion(&priv->completion); + + if (type == hwmon_temp) + priv->client.receive_cb = socfpga_hwmon_readtemp_cb; + else + priv->client.receive_cb = socfpga_hwmon_readvolt_cb; + + ret = stratix10_svc_send(priv->chan, msg); + if (ret < 0) + goto status_done; +drivers/hwmon/socfpga-hwmon.c line 277
Quoted 10 lines
drivers/hwmon/socfpga-hwmon.c new file +10
-0+ ret = wait_for_completion_timeout(&priv->completion, HWMON_TIMEOUT); + if (!ret) { + dev_err(priv->client.dev, "timeout waiting for SMC call\n"); + /* + * stratix10_svc_done() stops the worker but does not flush + * svc_fifo. Wait for the outstanding callback before stopping + * so a leftover request cannot complete a later sensor read. + * Discard the late result and still report -ETIMEDOUT. + */ + wait_for_completion(&priv->completion);Just like Sashiko, I am puzzled and do not understand this code.
Quoted 2 lines
Guenter
Hi Guenter,
Apologies, there shouldn't be another wait_for_completion if timeout. The new added wait_for_completion(&priv->completion) will be dropped in v8.
Regarding the FIFO concern: while svc_normal_to_secure_thread is running it dequeues each request and issues the SMC before the client timeout in the normal case, so there should be no leftover entry after a successful or failed transaction that invoked receive_cb.
The remaining caveat is if the worker exits via kthread_should_stop() (from stratix10_svc_done()) before dequeuing: svc_fifo is tied to the channel, not the thread, and stratix10_svc_done() does not flush it, so a queued request can remain and be processed by the next thread started on a later stratix10_svc_send(). We plan to address that with a FIFO flush in the SVC driver in a separate series.
Thanks,
Tze Yee