* [PATCH v3 0/2] hwmon: add Altera SoC FPGA hardware monitoring support
@ 2026-07-03 8:08 tze.yee.ng
2026-07-03 8:08 ` [PATCH v3 1/2] firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device tze.yee.ng
2026-07-03 8:08 ` [PATCH v3 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver tze.yee.ng
0 siblings, 2 replies; 9+ messages in thread
From: tze.yee.ng @ 2026-07-03 8:08 UTC (permalink / raw)
To: Dinh Nguyen, linux-kernel, Guenter Roeck, Jonathan Corbet,
Shuah Khan, linux-hwmon, linux-doc
From: Tze Yee Ng <tze.yee.ng@altera.com>
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 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/cover.1782715159.git.tze.yee.ng@altera.com/
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 | 596 +++++++++++++++++++
include/linux/firmware/intel/stratix10-smc.h | 38 ++
8 files changed, 731 insertions(+), 3 deletions(-)
create mode 100644 Documentation/hwmon/socfpga-hwmon.rst
create mode 100644 drivers/hwmon/socfpga-hwmon.c
--
2.43.7
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v3 1/2] firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device 2026-07-03 8:08 [PATCH v3 0/2] hwmon: add Altera SoC FPGA hardware monitoring support tze.yee.ng @ 2026-07-03 8:08 ` tze.yee.ng 2026-07-03 8:19 ` sashiko-bot 2026-07-03 8:08 ` [PATCH v3 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver tze.yee.ng 1 sibling, 1 reply; 9+ messages in thread From: tze.yee.ng @ 2026-07-03 8:08 UTC (permalink / raw) To: Dinh Nguyen, linux-kernel, Guenter Roeck, Jonathan Corbet, Shuah Khan, linux-hwmon, linux-doc From: Tze Yee Ng <tze.yee.ng@altera.com> 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. Signed-off-by: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com> Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com> --- 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(-) diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c index 00e134e663c8..a72b03c37ea8 100644 --- a/drivers/firmware/stratix10-svc.c +++ b/drivers/firmware/stratix10-svc.c @@ -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; }; /** @@ -1323,6 +1326,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; @@ -1416,6 +1427,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); @@ -2000,16 +2015,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); @@ -2037,6 +2074,9 @@ static void stratix10_svc_drv_remove(struct platform_device *pdev) of_platform_depopulate(ctrl->dev); + if (svc->stratix10_svc_hwmon) + platform_device_unregister(svc->stratix10_svc_hwmon); + platform_device_unregister(svc->stratix10_svc_rsu); for (i = 0; i < SVC_NUM_CHANNEL; i++) { diff --git a/include/linux/firmware/intel/stratix10-smc.h b/include/linux/firmware/intel/stratix10-smc.h index 9116512169dc..18ac6fe96d9d 100644 --- a/include/linux/firmware/intel/stratix10-smc.h +++ b/include/linux/firmware/intel/stratix10-smc.h @@ -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. -- 2.43.7 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device 2026-07-03 8:08 ` [PATCH v3 1/2] firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device tze.yee.ng @ 2026-07-03 8:19 ` sashiko-bot 2026-07-06 8:36 ` NG, TZE YEE 0 siblings, 1 reply; 9+ messages in thread From: sashiko-bot @ 2026-07-03 8:19 UTC (permalink / raw) To: tze.yee.ng; +Cc: linux-hwmon Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Pre-existing issues: - [High] The async service layer is torn down before the child platform devices (HWMON and RSU) are unregistered, leading to a Use-After-Free and NULL pointer dereference if a concurrent async call is made. -- commit 2c6317a7aef89428e616ea8354fab157b27ff35f Author: Tze Yee Ng <tze.yee.ng@altera.com> 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 when hardware monitor support is enabled. > diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c > index c24ca5823078f..716b58f5a6170 100644 > --- a/drivers/firmware/stratix10-svc.c > +++ b/drivers/firmware/stratix10-svc.c [ ... ] > @@ -2050,6 +2087,9 @@ static void stratix10_svc_drv_remove(struct platform_device *pdev) > > of_platform_depopulate(ctrl->dev); > > + if (svc->stratix10_svc_hwmon) > + platform_device_unregister(svc->stratix10_svc_hwmon); > + [Severity: High] This is a pre-existing issue, but does unregistering the child devices here introduce a race condition with module removal? In stratix10_svc_drv_remove(), we tear down the async service layer before the child devices are unregistered: stratix10_svc_async_exit(ctrl); of_platform_depopulate(ctrl->dev); if (svc->stratix10_svc_hwmon) platform_device_unregister(svc->stratix10_svc_hwmon); If a sysfs read for the hwmon device (e.g., temp1_input) races with module unload, could it proceed to stratix10_svc_async_send() because the device is not yet unregistered? If so, it looks like stratix10_svc_async_send() will dereference the achan structure that was already freed: ret = ida_alloc_max(&achan->job_id_pool, MAX_SDM_JOB_IDS, GFP_KERNEL); And later attempt a NULL pointer dereference, as actrl->invoke_fn was set to NULL by stratix10_svc_async_exit(): actrl->invoke_fn(actrl, &args, &res); Should the child platform devices be unregistered before stratix10_svc_async_exit() is called? > platform_device_unregister(svc->stratix10_svc_rsu); > > for (i = 0; i < SVC_NUM_CHANNEL; i++) { -- Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783062999.git.tze.yee.ng@altera.com?part=1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device 2026-07-03 8:19 ` sashiko-bot @ 2026-07-06 8:36 ` NG, TZE YEE 2026-07-06 13:36 ` Dinh Nguyen 0 siblings, 1 reply; 9+ messages in thread From: NG, TZE YEE @ 2026-07-06 8:36 UTC (permalink / raw) To: Dinh Nguyen, linux-hwmon@vger.kernel.org On 3/7/2026 4:19 pm, sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: > > Pre-existing issues: > - [High] The async service layer is torn down before the child platform devices (HWMON and RSU) are unregistered, leading to a Use-After-Free and NULL pointer dereference if a concurrent async call is made. > -- > > commit 2c6317a7aef89428e616ea8354fab157b27ff35f > Author: Tze Yee Ng <tze.yee.ng@altera.com> > > 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 when hardware monitor support is enabled. > >> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c >> index c24ca5823078f..716b58f5a6170 100644 >> --- a/drivers/firmware/stratix10-svc.c >> +++ b/drivers/firmware/stratix10-svc.c > > [ ... ] > >> @@ -2050,6 +2087,9 @@ static void stratix10_svc_drv_remove(struct platform_device *pdev) >> >> of_platform_depopulate(ctrl->dev); >> >> + if (svc->stratix10_svc_hwmon) >> + platform_device_unregister(svc->stratix10_svc_hwmon); >> + > > [Severity: High] > This is a pre-existing issue, but does unregistering the child devices here > introduce a race condition with module removal? > > In stratix10_svc_drv_remove(), we tear down the async service layer before > the child devices are unregistered: > > stratix10_svc_async_exit(ctrl); > of_platform_depopulate(ctrl->dev); > if (svc->stratix10_svc_hwmon) > platform_device_unregister(svc->stratix10_svc_hwmon); > > If a sysfs read for the hwmon device (e.g., temp1_input) races with module > unload, could it proceed to stratix10_svc_async_send() because the device > is not yet unregistered? > > If so, it looks like stratix10_svc_async_send() will dereference the achan > structure that was already freed: > > ret = ida_alloc_max(&achan->job_id_pool, MAX_SDM_JOB_IDS, > GFP_KERNEL); > > And later attempt a NULL pointer dereference, as actrl->invoke_fn was set > to NULL by stratix10_svc_async_exit(): > > actrl->invoke_fn(actrl, &args, &res); > > Should the child platform devices be unregistered before > stratix10_svc_async_exit() is called? > >> platform_device_unregister(svc->stratix10_svc_rsu); >> >> for (i = 0; i < SVC_NUM_CHANNEL; i++) { > Hi, Agreed with Sashiko's comment. In stratix10_svc_drv_remove(), stratix10_svc_async_exit() currently runs before the hwmon and RSU platform devices are unregistered. A concurrent sysfs read could still reach stratix10_svc_async_send() and use resources already torn down by stratix10_svc_async_exit(). This ordering predates the hwmon registration in this series; RSU follows the same pattern today. The fix would be to unregister child platform devices (and depopulate) before calling stratix10_svc_async_exit(). We can include that reorder in this series, or send it as a separate stratix10-svc fix. Let us know your preference. Thanks, Tze Yee ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device 2026-07-06 8:36 ` NG, TZE YEE @ 2026-07-06 13:36 ` Dinh Nguyen 0 siblings, 0 replies; 9+ messages in thread From: Dinh Nguyen @ 2026-07-06 13:36 UTC (permalink / raw) To: NG, TZE YEE, linux-hwmon@vger.kernel.org On 7/6/26 03:36, NG, TZE YEE wrote: > On 3/7/2026 4:19 pm, sashiko-bot@kernel.org wrote: >> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: >> >> Pre-existing issues: >> - [High] The async service layer is torn down before the child platform devices (HWMON and RSU) are unregistered, leading to a Use-After-Free and NULL pointer dereference if a concurrent async call is made. >> -- >> >> commit 2c6317a7aef89428e616ea8354fab157b27ff35f >> Author: Tze Yee Ng <tze.yee.ng@altera.com> >> >> 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 when hardware monitor support is enabled. >> >>> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c >>> index c24ca5823078f..716b58f5a6170 100644 >>> --- a/drivers/firmware/stratix10-svc.c >>> +++ b/drivers/firmware/stratix10-svc.c >> >> [ ... ] >> >>> @@ -2050,6 +2087,9 @@ static void stratix10_svc_drv_remove(struct platform_device *pdev) >>> >>> of_platform_depopulate(ctrl->dev); >>> >>> + if (svc->stratix10_svc_hwmon) >>> + platform_device_unregister(svc->stratix10_svc_hwmon); >>> + >> >> [Severity: High] >> This is a pre-existing issue, but does unregistering the child devices here >> introduce a race condition with module removal? >> >> In stratix10_svc_drv_remove(), we tear down the async service layer before >> the child devices are unregistered: >> >> stratix10_svc_async_exit(ctrl); >> of_platform_depopulate(ctrl->dev); >> if (svc->stratix10_svc_hwmon) >> platform_device_unregister(svc->stratix10_svc_hwmon); >> >> If a sysfs read for the hwmon device (e.g., temp1_input) races with module >> unload, could it proceed to stratix10_svc_async_send() because the device >> is not yet unregistered? >> >> If so, it looks like stratix10_svc_async_send() will dereference the achan >> structure that was already freed: >> >> ret = ida_alloc_max(&achan->job_id_pool, MAX_SDM_JOB_IDS, >> GFP_KERNEL); >> >> And later attempt a NULL pointer dereference, as actrl->invoke_fn was set >> to NULL by stratix10_svc_async_exit(): >> >> actrl->invoke_fn(actrl, &args, &res); >> >> Should the child platform devices be unregistered before >> stratix10_svc_async_exit() is called? >> >>> platform_device_unregister(svc->stratix10_svc_rsu); >>> >>> for (i = 0; i < SVC_NUM_CHANNEL; i++) { >> > > Hi, > > Agreed with Sashiko's comment. In stratix10_svc_drv_remove(), > stratix10_svc_async_exit() currently runs before the hwmon and RSU > platform devices are unregistered. A concurrent sysfs read could still > reach stratix10_svc_async_send() and use resources already torn down by > stratix10_svc_async_exit(). > > This ordering predates the hwmon registration in this series; RSU > follows the same pattern today. The fix would be to unregister child > platform devices (and depopulate) before calling stratix10_svc_async_exit(). > > We can include that reorder in this series, or send it as a separate > stratix10-svc fix. > Please send that as a separate fix. Thanks, Dinh ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver 2026-07-03 8:08 [PATCH v3 0/2] hwmon: add Altera SoC FPGA hardware monitoring support tze.yee.ng 2026-07-03 8:08 ` [PATCH v3 1/2] firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device tze.yee.ng @ 2026-07-03 8:08 ` tze.yee.ng 2026-07-03 8:18 ` sashiko-bot 1 sibling, 1 reply; 9+ messages in thread From: tze.yee.ng @ 2026-07-03 8:08 UTC (permalink / raw) To: Dinh Nguyen, linux-kernel, Guenter Roeck, Jonathan Corbet, Shuah Khan, linux-hwmon, linux-doc From: Tze Yee Ng <tze.yee.ng@altera.com> 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 <muhammad.nazim.amirul.nazle.asmade@altera.com> Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com> --- 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 | 596 ++++++++++++++++++++++++++ 6 files changed, 650 insertions(+) create mode 100644 Documentation/hwmon/socfpga-hwmon.rst create mode 100644 drivers/hwmon/socfpga-hwmon.c diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst index 8b655e5d6b68..04b3bce60c98 100644 --- a/Documentation/hwmon/index.rst +++ b/Documentation/hwmon/index.rst @@ -244,6 +244,7 @@ Hardware Monitoring Kernel Drivers sparx5-temp spd5118 stpddc60 + socfpga-hwmon surface_fan sy7636a-hwmon tc654 diff --git a/Documentation/hwmon/socfpga-hwmon.rst b/Documentation/hwmon/socfpga-hwmon.rst new file mode 100644 index 000000000000..e5da42556a62 --- /dev/null +++ b/Documentation/hwmon/socfpga-hwmon.rst @@ -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 <muhammad.nazim.amirul.nazle.asmade@altera.com> + - Tze Yee Ng <tze.yee.ng@altera.com> + +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 diff --git a/MAINTAINERS b/MAINTAINERS index 2fb1c75afd16..5a032c9931c3 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -950,6 +950,14 @@ L: linux-gpio@vger.kernel.org S: Maintained F: drivers/gpio/gpio-altera.c +ALTERA SoC FPGA HWMON DRIVER +M: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com> +M: Tze Yee Ng <tze.yee.ng@altera.com> +L: linux-hwmon@vger.kernel.org +S: Maintained +F: Documentation/hwmon/socfpga-hwmon.rst +F: drivers/hwmon/socfpga-hwmon.c + ALTERA TRIPLE SPEED ETHERNET DRIVER M: Boon Khai Ng <boon.khai.ng@altera.com> L: netdev@vger.kernel.org diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index 14e4cea48acc..05a9c7a6647e 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -2112,6 +2112,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 diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 4788996aa137..9d1c5f1bf569 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -218,6 +218,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 diff --git a/drivers/hwmon/socfpga-hwmon.c b/drivers/hwmon/socfpga-hwmon.c new file mode 100644 index 000000000000..af1ca4f26665 --- /dev/null +++ b/drivers/hwmon/socfpga-hwmon.c @@ -0,0 +1,596 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Altera SoC FPGA hardware monitoring driver + * + * Copyright (c) 2026 Altera Corporation + * + * Authors: + * Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com> + * Tze Yee Ng <tze.yee.ng@altera.com> + */ + +#include <linux/bitops.h> +#include <linux/cleanup.h> +#include <linux/completion.h> +#include <linux/delay.h> +#include <linux/firmware/intel/stratix10-svc-client.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/jiffies.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of.h> +#include <linux/platform_device.h> + +#define HWMON_TIMEOUT msecs_to_jiffies(SVC_HWMON_REQUEST_TIMEOUT_MS) +#define HWMON_RETRY_SLEEP_MS 1U +#define HWMON_ASYNC_MSG_RETRY 3U +#define SOCFPGA_HWMON_MAXSENSORS 16 +#define SOCFPGA_HWMON_CHANNEL_MASK GENMASK(15, 0) +#define SOCFPGA_HWMON_PAGE_SHIFT 16 +#define SOCFPGA_HWMON_CHAN(page, channel) \ + (((page) << SOCFPGA_HWMON_PAGE_SHIFT) | \ + ((channel) & SOCFPGA_HWMON_CHANNEL_MASK)) +#define SOCFPGA_HWMON_ATTR_VISIBLE 0444 +/* Temperature from SDM is signed Q8.8 degrees Celsius (8 fractional bits). */ +#define SOCFPGA_HWMON_TEMP_FRAC_BITS 8 +#define SOCFPGA_HWMON_TEMP_FRAC_DIV BIT(SOCFPGA_HWMON_TEMP_FRAC_BITS) +#define SOCFPGA_HWMON_TEMP_MDEG_SCALE 1000 +/* Voltage from SDM is unsigned Q16 volts (16 fractional bits). */ +#define SOCFPGA_HWMON_VOLT_FRAC_BITS 16 +#define SOCFPGA_HWMON_VOLT_FRAC_DIV BIT(SOCFPGA_HWMON_VOLT_FRAC_BITS) +#define SOCFPGA_HWMON_VOLT_MV_SCALE 1000 + +#define ETEMP_INACTIVE 0x80000000U +#define ETEMP_TOO_OLD 0x80000001U +#define ETEMP_NOT_PRESENT 0x80000002U +#define ETEMP_TIMEOUT 0x80000003U +#define ETEMP_CORRUPT 0x80000004U +#define ETEMP_BUSY 0x80000005U +#define ETEMP_NOT_INITIALIZED 0x800000FFU + +struct socfpga_hwmon_channel { + u32 reg; + const char *label; +}; + +struct socfpga_hwmon_board_data { + const struct socfpga_hwmon_channel *temp; + unsigned int num_temp; + const struct socfpga_hwmon_channel *volt; + unsigned int num_volt; +}; + +struct socfpga_hwmon_priv { + struct stratix10_svc_chan *chan; + struct stratix10_svc_client client; + struct completion completion; + struct mutex lock; /* protect SVC calls */ + bool async; + int last_err; /* sync-mode SVC result; 0 on success */ + u32 temperature; + u32 voltage; + int temperature_channels; + int voltage_channels; + const char *temp_chan_names[SOCFPGA_HWMON_MAXSENSORS]; + const char *volt_chan_names[SOCFPGA_HWMON_MAXSENSORS]; + u32 temp_chan[SOCFPGA_HWMON_MAXSENSORS]; + u32 volt_chan[SOCFPGA_HWMON_MAXSENSORS]; +}; + +static umode_t socfpga_hwmon_is_visible(const void *dev, + enum hwmon_sensor_types type, + u32 attr, int chan) +{ + const struct socfpga_hwmon_priv *priv = dev; + + switch (type) { + case hwmon_temp: + if (chan < priv->temperature_channels) + return SOCFPGA_HWMON_ATTR_VISIBLE; + return 0; + case hwmon_in: + if (chan < priv->voltage_channels) + return SOCFPGA_HWMON_ATTR_VISIBLE; + return 0; + default: + return 0; + } +} + +static void socfpga_hwmon_readtemp_cb(struct stratix10_svc_client *client, + struct stratix10_svc_cb_data *data) +{ + struct socfpga_hwmon_priv *priv = client->priv; + + priv->last_err = -EIO; + if (data->status == BIT(SVC_STATUS_OK)) { + priv->last_err = 0; + priv->temperature = (u32)*(unsigned long *)data->kaddr1; + } else if (data->kaddr1) { + dev_err(client->dev, "%s failed with status 0x%x, value 0x%lx\n", + __func__, data->status, + *(unsigned long *)data->kaddr1); + } else { + dev_err(client->dev, "%s failed with status 0x%x\n", + __func__, data->status); + } + + complete(&priv->completion); +} + +static void socfpga_hwmon_readvolt_cb(struct stratix10_svc_client *client, + struct stratix10_svc_cb_data *data) +{ + struct socfpga_hwmon_priv *priv = client->priv; + + priv->last_err = -EIO; + if (data->status == BIT(SVC_STATUS_OK)) { + priv->last_err = 0; + priv->voltage = (u32)*(unsigned long *)data->kaddr1; + } else if (data->kaddr1) { + dev_err(client->dev, "%s failed with status 0x%x, value 0x%lx\n", + __func__, data->status, + *(unsigned long *)data->kaddr1); + } else { + dev_err(client->dev, "%s failed with status 0x%x\n", + __func__, data->status); + } + + complete(&priv->completion); +} + +static int socfpga_hwmon_parse_temp(long *val, u32 temperature) +{ + switch (temperature) { + case ETEMP_INACTIVE: + case ETEMP_NOT_PRESENT: + case ETEMP_CORRUPT: + case ETEMP_NOT_INITIALIZED: + return -EOPNOTSUPP; + case ETEMP_TIMEOUT: + case ETEMP_BUSY: + case ETEMP_TOO_OLD: + return -EAGAIN; + default: + /* SDM returns a 16-bit signed Q8.8 value in the low 16 bits. */ + *val = (long)(s16)(temperature & SOCFPGA_HWMON_CHANNEL_MASK) * + SOCFPGA_HWMON_TEMP_MDEG_SCALE / SOCFPGA_HWMON_TEMP_FRAC_DIV; + return 0; + } +} + +static int socfpga_hwmon_encode_temp_arg(u32 reg, u64 *arg) +{ + u32 page = (reg >> SOCFPGA_HWMON_PAGE_SHIFT) & SOCFPGA_HWMON_CHANNEL_MASK; + u32 channel = reg & SOCFPGA_HWMON_CHANNEL_MASK; + + if (channel >= SOCFPGA_HWMON_MAXSENSORS) + return -EINVAL; + + *arg = (1ULL << channel) | ((u64)page << SOCFPGA_HWMON_PAGE_SHIFT); + return 0; +} + +static int socfpga_hwmon_encode_volt_arg(u32 reg, u64 *arg) +{ + u32 channel = reg & SOCFPGA_HWMON_CHANNEL_MASK; + + if (channel >= SOCFPGA_HWMON_MAXSENSORS) + return -EINVAL; + + *arg = 1ULL << channel; + return 0; +} + +static int socfpga_hwmon_async_read(struct device *dev, + enum hwmon_sensor_types type, + struct stratix10_svc_client_msg *msg) +{ + struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev); + struct stratix10_svc_cb_data data = {}; + struct completion completion; + unsigned long wait_ret; + void *handle = NULL; + int status, index, ret; + + init_completion(&completion); + + for (index = 0; index < HWMON_ASYNC_MSG_RETRY; index++) { + status = stratix10_svc_async_send(priv->chan, msg, &handle, + NULL, NULL); + if (status == 0) + break; + dev_warn(dev, "Failed to send async message: %d", status); + msleep(HWMON_RETRY_SLEEP_MS); + } + + if (status && !handle) { + dev_err(dev, "Failed to send async message after %u retries: %d\n", + HWMON_ASYNC_MSG_RETRY, status); + return status; + } + + /* + * The service layer never invokes the async callback, but wait + * before polling anyway: RSU uses the same pattern and the firmware + * needs time to complete the transaction after async_send(). + */ + wait_ret = wait_for_completion_io_timeout(&completion, HWMON_TIMEOUT); + if (wait_ret > 0) + dev_dbg(dev, "Received async interrupt\n"); + else if (wait_ret == 0) + dev_dbg(dev, "Timeout occurred, trying to poll the response\n"); + + ret = -ETIMEDOUT; + for (index = 0; index < HWMON_ASYNC_MSG_RETRY; index++) { + status = stratix10_svc_async_poll(priv->chan, handle, &data); + if (status == -EAGAIN) { + dev_dbg(dev, "Async message is still in progress\n"); + } else if (status < 0) { + dev_alert(dev, "Failed to poll async message: %d\n", status); + ret = -ETIMEDOUT; + } else if (status == 0) { + ret = 0; + break; + } + msleep(HWMON_RETRY_SLEEP_MS); + } + + if (ret) { + dev_err(dev, "Failed to get async response\n"); + goto done; + } + + if (data.status) { + dev_err(dev, "%s returned 0x%x from SDM\n", __func__, + data.status); + ret = -EFAULT; + goto done; + } + + if (type == hwmon_temp) + priv->temperature = (u32)*(unsigned long *)data.kaddr1; + else + priv->voltage = (u32)*(unsigned long *)data.kaddr1; + + ret = 0; + +done: + stratix10_svc_async_done(priv->chan, handle); + return ret; +} + +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; + + 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; + } + + ret = priv->last_err; + +status_done: + stratix10_svc_done(priv->chan); + return ret; +} + +static int socfpga_hwmon_read(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int chan, long *val) +{ + struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev); + struct stratix10_svc_client_msg msg = {0}; + int ret; + + if (chan >= SOCFPGA_HWMON_MAXSENSORS) + return -EOPNOTSUPP; + + switch (type) { + case hwmon_temp: + ret = socfpga_hwmon_encode_temp_arg(priv->temp_chan[chan], + &msg.arg[0]); + if (ret) + return ret; + msg.command = COMMAND_HWMON_READTEMP; + break; + case hwmon_in: + ret = socfpga_hwmon_encode_volt_arg(priv->volt_chan[chan], + &msg.arg[0]); + if (ret) + return ret; + msg.command = COMMAND_HWMON_READVOLT; + break; + default: + return -EOPNOTSUPP; + } + + guard(mutex)(&priv->lock); + if (priv->async) + ret = socfpga_hwmon_async_read(dev, type, &msg); + else + ret = socfpga_hwmon_sync_read(dev, type, &msg); + if (ret) + return ret; + + if (type == hwmon_temp) + ret = socfpga_hwmon_parse_temp(val, priv->temperature); + else + /* Convert Q16 volts to millivolts for hwmon. */ + *val = (long)priv->voltage * SOCFPGA_HWMON_VOLT_MV_SCALE / + SOCFPGA_HWMON_VOLT_FRAC_DIV; + return ret; +} + +static int socfpga_hwmon_read_string(struct device *dev, + enum hwmon_sensor_types type, u32 attr, + int chan, const char **str) +{ + struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev); + + switch (type) { + case hwmon_in: + *str = priv->volt_chan_names[chan]; + return 0; + case hwmon_temp: + *str = priv->temp_chan_names[chan]; + return 0; + default: + return -EOPNOTSUPP; + } +} + +static const struct hwmon_ops socfpga_hwmon_ops = { + .is_visible = socfpga_hwmon_is_visible, + .read = socfpga_hwmon_read, + .read_string = socfpga_hwmon_read_string, +}; + +static const struct hwmon_channel_info *socfpga_hwmon_info[] = { + HWMON_CHANNEL_INFO(temp, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL), + HWMON_CHANNEL_INFO(in, + HWMON_I_INPUT | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_LABEL), + NULL +}; + +static const struct hwmon_chip_info socfpga_hwmon_chip_info = { + .ops = &socfpga_hwmon_ops, + .info = socfpga_hwmon_info, +}; + +static const struct socfpga_hwmon_channel s10_hwmon_volt_channels[] = { + { SOCFPGA_HWMON_CHAN(0, 2), "0.8V VCC" }, + { SOCFPGA_HWMON_CHAN(0, 3), "1.8V VCCIO_SDM" }, + { SOCFPGA_HWMON_CHAN(0, 6), "0.9V VCCERAM" }, +}; + +static const struct socfpga_hwmon_channel s10_hwmon_temp_channels[] = { + { SOCFPGA_HWMON_CHAN(0, 0), "Main Die SDM" }, +}; + +static const struct socfpga_hwmon_board_data s10_hwmon_board = { + .temp = s10_hwmon_temp_channels, + .num_temp = ARRAY_SIZE(s10_hwmon_temp_channels), + .volt = s10_hwmon_volt_channels, + .num_volt = ARRAY_SIZE(s10_hwmon_volt_channels), +}; + +static const struct socfpga_hwmon_channel agilex_hwmon_volt_channels[] = { + { SOCFPGA_HWMON_CHAN(0, 2), "0.8V VCC" }, + { SOCFPGA_HWMON_CHAN(0, 3), "1.8V VCCIO_SDM" }, + { SOCFPGA_HWMON_CHAN(0, 4), "1.8V VCCPT" }, + { SOCFPGA_HWMON_CHAN(0, 5), "1.2V VCCCRCORE" }, + { SOCFPGA_HWMON_CHAN(0, 6), "0.9V VCCH" }, + { SOCFPGA_HWMON_CHAN(0, 7), "0.8V VCCL" }, +}; + +static const struct socfpga_hwmon_channel agilex_hwmon_temp_channels[] = { + { SOCFPGA_HWMON_CHAN(0, 0), "Main Die SDM" }, + { SOCFPGA_HWMON_CHAN(1, 0), "Main Die corner bottom left max" }, + { SOCFPGA_HWMON_CHAN(2, 0), "Main Die corner top left max" }, + { SOCFPGA_HWMON_CHAN(3, 0), "Main Die corner bottom right max" }, + { SOCFPGA_HWMON_CHAN(4, 0), "Main Die corner top right max" }, +}; + +static const struct socfpga_hwmon_board_data agilex_hwmon_board = { + .temp = agilex_hwmon_temp_channels, + .num_temp = ARRAY_SIZE(agilex_hwmon_temp_channels), + .volt = agilex_hwmon_volt_channels, + .num_volt = ARRAY_SIZE(agilex_hwmon_volt_channels), +}; + +static const struct socfpga_hwmon_board_data * +socfpga_hwmon_get_board(struct device *dev) +{ + struct device_node *np = dev->of_node; + + if (!np) + return NULL; + + if (of_device_is_compatible(np, "intel,stratix10-svc")) + return &s10_hwmon_board; + if (of_device_is_compatible(np, "intel,agilex-svc")) + return &agilex_hwmon_board; + + return NULL; +} + +static int socfpga_hwmon_init_channels(struct device *dev, + const struct socfpga_hwmon_board_data *board, + struct socfpga_hwmon_priv *priv) +{ + unsigned int i; + + if (board->num_temp > SOCFPGA_HWMON_MAXSENSORS || + board->num_volt > SOCFPGA_HWMON_MAXSENSORS) + return -EINVAL; + + for (i = 0; i < board->num_temp; i++) { + priv->temp_chan_names[i] = board->temp[i].label; + priv->temp_chan[i] = board->temp[i].reg; + } + priv->temperature_channels = board->num_temp; + + for (i = 0; i < board->num_volt; i++) { + priv->volt_chan_names[i] = board->volt[i].label; + priv->volt_chan[i] = board->volt[i].reg; + } + priv->voltage_channels = board->num_volt; + + return 0; +} + +static void socfpga_hwmon_release_svc(void *data) +{ + struct socfpga_hwmon_priv *priv = data; + + if (priv->async) + stratix10_svc_remove_async_client(priv->chan); + stratix10_svc_free_channel(priv->chan); +} + +static int socfpga_hwmon_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device *parent = dev->parent; + const struct socfpga_hwmon_board_data *board; + struct socfpga_hwmon_priv *priv; + struct device *hwmon_dev; + int ret; + + if (!parent || !parent->of_node) { + dev_err(dev, "missing parent device node\n"); + return -ENODEV; + } + + board = socfpga_hwmon_get_board(parent); + if (!board) { + dev_err(dev, "unsupported service layer compatible\n"); + return -ENODEV; + } + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->client.dev = dev; + priv->client.priv = priv; + init_completion(&priv->completion); + mutex_init(&priv->lock); + + ret = socfpga_hwmon_init_channels(dev, board, priv); + if (ret) + return ret; + + priv->chan = stratix10_svc_request_channel_byname(&priv->client, + SVC_CLIENT_HWMON); + if (IS_ERR(priv->chan)) { + ret = PTR_ERR(priv->chan); + if (ret == -EPROBE_DEFER) + dev_dbg(dev, "service channel %s not ready, deferring probe\n", + SVC_CLIENT_HWMON); + else + dev_err(dev, "couldn't get service channel %s: %d\n", + SVC_CLIENT_HWMON, ret); + return ret; + } + + ret = stratix10_svc_add_async_client(priv->chan, false); + switch (ret) { + case 0: + priv->async = true; + break; + case -EINVAL: + case -EOPNOTSUPP: + /* + * stratix10_svc_add_async_client() returns -EINVAL when the + * async controller is not initialized; fall back to sync mode. + */ + dev_dbg(dev, "async operations not supported, using sync mode\n"); + priv->async = false; + break; + default: + dev_err(dev, "failed to add async client: %d\n", ret); + stratix10_svc_free_channel(priv->chan); + return ret; + } + + hwmon_dev = devm_hwmon_device_register_with_info(dev, "socfpga_hwmon", + priv, + &socfpga_hwmon_chip_info, + NULL); + if (IS_ERR(hwmon_dev)) { + if (priv->async) + stratix10_svc_remove_async_client(priv->chan); + stratix10_svc_free_channel(priv->chan); + return PTR_ERR(hwmon_dev); + } + + ret = devm_add_action_or_reset(dev, socfpga_hwmon_release_svc, priv); + if (ret) + return ret; + + platform_set_drvdata(pdev, priv); + return 0; +} + +static struct platform_driver socfpga_hwmon_driver = { + .probe = socfpga_hwmon_probe, + .driver = { + .name = "socfpga-hwmon", + }, +}; +module_platform_driver(socfpga_hwmon_driver); + +MODULE_AUTHOR("Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>"); +MODULE_AUTHOR("Tze Yee Ng <tze.yee.ng@altera.com>"); +MODULE_DESCRIPTION("Altera SoC FPGA hardware monitoring driver"); +MODULE_LICENSE("GPL"); -- 2.43.7 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver 2026-07-03 8:08 ` [PATCH v3 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver tze.yee.ng @ 2026-07-03 8:18 ` sashiko-bot 2026-07-06 8:59 ` NG, TZE YEE 0 siblings, 1 reply; 9+ messages in thread From: sashiko-bot @ 2026-07-03 8:18 UTC (permalink / raw) To: tze.yee.ng; +Cc: linux-hwmon Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Incorrect devm registration order causes a Use-After-Free vulnerability during device unbind or probe failure. - [Medium] Unreferenced `completion` causes a guaranteed maximum timeout delay on every asynchronous sensor read. -- commit 9e088a070299b309352aa9a88006e3927d0c50bd Author: Tze Yee Ng <tze.yee.ng@altera.com> hwmon: add Altera SoC FPGA hardware monitoring driver 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. > diff --git a/drivers/hwmon/socfpga-hwmon.c b/drivers/hwmon/socfpga-hwmon.c > new file mode 100644 > --- /dev/null > +++ b/drivers/hwmon/socfpga-hwmon.c [ ... ] > +static int socfpga_hwmon_async_read(struct device *dev, > + enum hwmon_sensor_types type, > + struct stratix10_svc_client_msg *msg) > +{ > + struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev); > + struct stratix10_svc_cb_data data = {}; > + struct completion completion; > + unsigned long wait_ret; > + void *handle = NULL; > + int status, index, ret; > + > + init_completion(&completion); > + > + for (index = 0; index < HWMON_ASYNC_MSG_RETRY; index++) { > + status = stratix10_svc_async_send(priv->chan, msg, &handle, > + NULL, NULL); > + if (status == 0) > + break; > + dev_warn(dev, "Failed to send async message: %d", status); > + msleep(HWMON_RETRY_SLEEP_MS); > + } > + > + if (status && !handle) { > + dev_err(dev, "Failed to send async message after %u retries: %d\n", > + HWMON_ASYNC_MSG_RETRY, status); > + return status; > + } > + > + /* > + * The service layer never invokes the async callback, but wait > + * before polling anyway: RSU uses the same pattern and the firmware > + * needs time to complete the transaction after async_send(). > + */ > + wait_ret = wait_for_completion_io_timeout(&completion, HWMON_TIMEOUT); [Severity: Medium] Will this cause a guaranteed timeout delay on every sensor read? In socfpga_hwmon_async_read(), a local completion is initialized but is never passed to any asynchronous function or callback (the function stratix10_svc_async_send() receives NULL for the callback). Because the completion can never be signaled, the execution will unconditionally block in an uninterruptible sleep for the entire HWMON_TIMEOUT duration on every single sensor read. Only after this maximum timeout expires does the function proceed to poll for the response. > + if (wait_ret > 0) > + dev_dbg(dev, "Received async interrupt\n"); [ ... ] > +static int socfpga_hwmon_probe(struct platform_device *pdev) > +{ [ ... ] > + hwmon_dev = devm_hwmon_device_register_with_info(dev, "socfpga_hwmon", > + priv, > + &socfpga_hwmon_chip_info, > + NULL); > + if (IS_ERR(hwmon_dev)) { > + if (priv->async) > + stratix10_svc_remove_async_client(priv->chan); > + stratix10_svc_free_channel(priv->chan); > + return PTR_ERR(hwmon_dev); > + } > + > + ret = devm_add_action_or_reset(dev, socfpga_hwmon_release_svc, priv); [Severity: High] Does this devres registration order create a use-after-free vulnerability during device unbind? In socfpga_hwmon_probe(), the hwmon device is registered via devm_hwmon_device_register_with_info() before the cleanup action socfpga_hwmon_release_svc is added via devm_add_action_or_reset(). Because devres unwinds actions in reverse order of addition (LIFO), upon device unbind, socfpga_hwmon_release_svc will execute first, freeing the service layer channel (priv->chan). However, the hwmon device will still be active and its sysfs attributes accessible to user space during this time. A concurrent sysfs read can invoke socfpga_hwmon_read(), which will attempt to use the already-freed priv->chan, resulting in a use-after-free. > + if (ret) > + return ret; > + > + platform_set_drvdata(pdev, priv); > + return 0; > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783062999.git.tze.yee.ng@altera.com?part=2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver 2026-07-03 8:18 ` sashiko-bot @ 2026-07-06 8:59 ` NG, TZE YEE 2026-07-06 14:58 ` Guenter Roeck 0 siblings, 1 reply; 9+ messages in thread From: NG, TZE YEE @ 2026-07-06 8:59 UTC (permalink / raw) To: linux-hwmon@vger.kernel.org, Guenter Roeck, Jonathan Corbet, Shuah Khan On 3/7/2026 4:18 pm, sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: > - [High] Incorrect devm registration order causes a Use-After-Free vulnerability during device unbind or probe failure. > - [Medium] Unreferenced `completion` causes a guaranteed maximum timeout delay on every asynchronous sensor read. > -- > > commit 9e088a070299b309352aa9a88006e3927d0c50bd > Author: Tze Yee Ng <tze.yee.ng@altera.com> > > hwmon: add Altera SoC FPGA hardware monitoring driver > > 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. > >> diff --git a/drivers/hwmon/socfpga-hwmon.c b/drivers/hwmon/socfpga-hwmon.c >> new file mode 100644 >> --- /dev/null >> +++ b/drivers/hwmon/socfpga-hwmon.c > [ ... ] >> +static int socfpga_hwmon_async_read(struct device *dev, >> + enum hwmon_sensor_types type, >> + struct stratix10_svc_client_msg *msg) >> +{ >> + struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev); >> + struct stratix10_svc_cb_data data = {}; >> + struct completion completion; >> + unsigned long wait_ret; >> + void *handle = NULL; >> + int status, index, ret; >> + >> + init_completion(&completion); >> + >> + for (index = 0; index < HWMON_ASYNC_MSG_RETRY; index++) { >> + status = stratix10_svc_async_send(priv->chan, msg, &handle, >> + NULL, NULL); >> + if (status == 0) >> + break; >> + dev_warn(dev, "Failed to send async message: %d", status); >> + msleep(HWMON_RETRY_SLEEP_MS); >> + } >> + >> + if (status && !handle) { >> + dev_err(dev, "Failed to send async message after %u retries: %d\n", >> + HWMON_ASYNC_MSG_RETRY, status); >> + return status; >> + } >> + >> + /* >> + * The service layer never invokes the async callback, but wait >> + * before polling anyway: RSU uses the same pattern and the firmware >> + * needs time to complete the transaction after async_send(). >> + */ >> + wait_ret = wait_for_completion_io_timeout(&completion, HWMON_TIMEOUT); > > [Severity: Medium] > Will this cause a guaranteed timeout delay on every sensor read? > > In socfpga_hwmon_async_read(), a local completion is initialized but is > never passed to any asynchronous function or callback (the function > stratix10_svc_async_send() receives NULL for the callback). > > Because the completion can never be signaled, the execution will > unconditionally block in an uninterruptible sleep for the entire > HWMON_TIMEOUT duration on every single sensor read. Only after this maximum > timeout expires does the function proceed to poll for the response. > >> + if (wait_ret > 0) >> + dev_dbg(dev, "Received async interrupt\n"); > [ ... ] >> +static int socfpga_hwmon_probe(struct platform_device *pdev) >> +{ > [ ... ] >> + hwmon_dev = devm_hwmon_device_register_with_info(dev, "socfpga_hwmon", >> + priv, >> + &socfpga_hwmon_chip_info, >> + NULL); >> + if (IS_ERR(hwmon_dev)) { >> + if (priv->async) >> + stratix10_svc_remove_async_client(priv->chan); >> + stratix10_svc_free_channel(priv->chan); >> + return PTR_ERR(hwmon_dev); >> + } >> + >> + ret = devm_add_action_or_reset(dev, socfpga_hwmon_release_svc, priv); > > [Severity: High] > Does this devres registration order create a use-after-free vulnerability > during device unbind? > > In socfpga_hwmon_probe(), the hwmon device is registered via > devm_hwmon_device_register_with_info() before the cleanup action > socfpga_hwmon_release_svc is added via devm_add_action_or_reset(). > > Because devres unwinds actions in reverse order of addition (LIFO), upon > device unbind, socfpga_hwmon_release_svc will execute first, freeing the > service layer channel (priv->chan). > > However, the hwmon device will still be active and its sysfs attributes > accessible to user space during this time. A concurrent sysfs read can > invoke socfpga_hwmon_read(), which will attempt to use the already-freed > priv->chan, resulting in a use-after-free. > >> + if (ret) >> + return ret; >> + >> + platform_set_drvdata(pdev, priv); >> + return 0; >> +} > Hi maintainers, 1. [High] Incorrect devm registration order causes a Use-After-Free vulnerability during device unbind or probe failure. Agreed. v4 will register devm_add_action_or_reset() before devm_hwmon_device_register_with_info(), matching other hwmon drivers (e.g. pwm-fan, qnap-mcu-hwmon). On hwmon registration failure, probe returns and devm runs the release action automatically, so manual channel cleanup on that path is no longer needed. 2. [Medium] Unreferenced `completion` causes a guaranteed maximum timeout delay on every asynchronous sensor read. Agreed that the completion is never signaled because the service layer does not invoke the async callback. rsu_send_async_msg() in stratix10-rsu.c uses the same send → wait → poll pattern, so we followed RSU for consistency with the existing Stratix 10 async SVC client. We are happy to change this if you prefer — for example a deadline-based poll loop without the fixed wait, or a proper fix in stratix10-svc to invoke the callback. Please let us know your preference. We will send v4 with the devm fix; the async wait approach can follow your guidance above. Thanks, Tze Yee ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver 2026-07-06 8:59 ` NG, TZE YEE @ 2026-07-06 14:58 ` Guenter Roeck 0 siblings, 0 replies; 9+ messages in thread From: Guenter Roeck @ 2026-07-06 14:58 UTC (permalink / raw) To: NG, TZE YEE, linux-hwmon@vger.kernel.org, Jonathan Corbet, Shuah Khan On 7/6/26 01:59, NG, TZE YEE wrote: > On 3/7/2026 4:18 pm, sashiko-bot@kernel.org wrote: >> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: >> - [High] Incorrect devm registration order causes a Use-After-Free vulnerability during device unbind or probe failure. >> - [Medium] Unreferenced `completion` causes a guaranteed maximum timeout delay on every asynchronous sensor read. >> -- >> >> commit 9e088a070299b309352aa9a88006e3927d0c50bd >> Author: Tze Yee Ng <tze.yee.ng@altera.com> >> >> hwmon: add Altera SoC FPGA hardware monitoring driver >> >> 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. >> >>> diff --git a/drivers/hwmon/socfpga-hwmon.c b/drivers/hwmon/socfpga-hwmon.c >>> new file mode 100644 >>> --- /dev/null >>> +++ b/drivers/hwmon/socfpga-hwmon.c >> [ ... ] >>> +static int socfpga_hwmon_async_read(struct device *dev, >>> + enum hwmon_sensor_types type, >>> + struct stratix10_svc_client_msg *msg) >>> +{ >>> + struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev); >>> + struct stratix10_svc_cb_data data = {}; >>> + struct completion completion; >>> + unsigned long wait_ret; >>> + void *handle = NULL; >>> + int status, index, ret; >>> + >>> + init_completion(&completion); >>> + >>> + for (index = 0; index < HWMON_ASYNC_MSG_RETRY; index++) { >>> + status = stratix10_svc_async_send(priv->chan, msg, &handle, >>> + NULL, NULL); >>> + if (status == 0) >>> + break; >>> + dev_warn(dev, "Failed to send async message: %d", status); >>> + msleep(HWMON_RETRY_SLEEP_MS); >>> + } >>> + >>> + if (status && !handle) { >>> + dev_err(dev, "Failed to send async message after %u retries: %d\n", >>> + HWMON_ASYNC_MSG_RETRY, status); >>> + return status; >>> + } >>> + >>> + /* >>> + * The service layer never invokes the async callback, but wait >>> + * before polling anyway: RSU uses the same pattern and the firmware >>> + * needs time to complete the transaction after async_send(). >>> + */ >>> + wait_ret = wait_for_completion_io_timeout(&completion, HWMON_TIMEOUT); >> >> [Severity: Medium] >> Will this cause a guaranteed timeout delay on every sensor read? >> >> In socfpga_hwmon_async_read(), a local completion is initialized but is >> never passed to any asynchronous function or callback (the function >> stratix10_svc_async_send() receives NULL for the callback). >> >> Because the completion can never be signaled, the execution will >> unconditionally block in an uninterruptible sleep for the entire >> HWMON_TIMEOUT duration on every single sensor read. Only after this maximum >> timeout expires does the function proceed to poll for the response. >> >>> + if (wait_ret > 0) >>> + dev_dbg(dev, "Received async interrupt\n"); >> [ ... ] >>> +static int socfpga_hwmon_probe(struct platform_device *pdev) >>> +{ >> [ ... ] >>> + hwmon_dev = devm_hwmon_device_register_with_info(dev, "socfpga_hwmon", >>> + priv, >>> + &socfpga_hwmon_chip_info, >>> + NULL); >>> + if (IS_ERR(hwmon_dev)) { >>> + if (priv->async) >>> + stratix10_svc_remove_async_client(priv->chan); >>> + stratix10_svc_free_channel(priv->chan); >>> + return PTR_ERR(hwmon_dev); >>> + } >>> + >>> + ret = devm_add_action_or_reset(dev, socfpga_hwmon_release_svc, priv); >> >> [Severity: High] >> Does this devres registration order create a use-after-free vulnerability >> during device unbind? >> >> In socfpga_hwmon_probe(), the hwmon device is registered via >> devm_hwmon_device_register_with_info() before the cleanup action >> socfpga_hwmon_release_svc is added via devm_add_action_or_reset(). >> >> Because devres unwinds actions in reverse order of addition (LIFO), upon >> device unbind, socfpga_hwmon_release_svc will execute first, freeing the >> service layer channel (priv->chan). >> >> However, the hwmon device will still be active and its sysfs attributes >> accessible to user space during this time. A concurrent sysfs read can >> invoke socfpga_hwmon_read(), which will attempt to use the already-freed >> priv->chan, resulting in a use-after-free. >> >>> + if (ret) >>> + return ret; >>> + >>> + platform_set_drvdata(pdev, priv); >>> + return 0; >>> +} >> > > Hi maintainers, > > 1. [High] Incorrect devm registration order causes a Use-After-Free > vulnerability during device unbind or probe failure. > > Agreed. v4 will register devm_add_action_or_reset() before > devm_hwmon_device_register_with_info(), matching other hwmon drivers > (e.g. pwm-fan, qnap-mcu-hwmon). On hwmon registration failure, probe > returns and devm runs the release action automatically, so manual > channel cleanup on that path is no longer needed. > > 2. [Medium] Unreferenced `completion` causes a guaranteed maximum > timeout delay on every asynchronous sensor read. > > Agreed that the completion is never signaled because the service layer > does not invoke the async callback. rsu_send_async_msg() in > stratix10-rsu.c uses the same send → wait → poll pattern, so we followed > RSU for consistency with the existing Stratix 10 async SVC client. > Sorry, I am not willing to accept the "other code is buggy, therefore it is ok to introduce the same bug here" argument (Is that some new trend ? I hear it so often lately that I am getting tired of it). You'll have to provide a better argument explaining why it would make sense to call wait_for_completion_io_timeout() if it is known to always time out. You might as well just call usleep_range() or similar; while that would still be bad code, at least it would not be misleading. Just add a comment explaining the reason when you do that. Guenter ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-07-06 14:58 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-07-03 8:08 [PATCH v3 0/2] hwmon: add Altera SoC FPGA hardware monitoring support tze.yee.ng 2026-07-03 8:08 ` [PATCH v3 1/2] firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device tze.yee.ng 2026-07-03 8:19 ` sashiko-bot 2026-07-06 8:36 ` NG, TZE YEE 2026-07-06 13:36 ` Dinh Nguyen 2026-07-03 8:08 ` [PATCH v3 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver tze.yee.ng 2026-07-03 8:18 ` sashiko-bot 2026-07-06 8:59 ` NG, TZE YEE 2026-07-06 14:58 ` Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox