Re: [PATCH v2 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver

[NG, TZE YEE]

On 29/6/2026 3:42 pm, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] Mixing devm_hwmon_device_register_with_info with manual cleanup in remove() creates a Use-After-Free race condition during device unbind.
> - [High] Asynchronous hardware monitoring reads always time out, introducing a severe 2-second blocking delay per read.
> - [High] Incorrect error code check causes probe failure on firmware lacking async support instead of falling back to sync mode.
> - [High] A timeout or signal interruption during a synchronous read abandons a pending SMC call, causing its late response to prematurely complete and corrupt a subsequent read.
> - [Medium] Incorrect sign extension of Q8.8 temperature values will misinterpret negative temperatures.
> --
> 
> commit afeb154cb3aefd3ae7be5eaa69494d4428dd9007
> Author: Tze Yee Ng <tze.yee.ng@xxxxxxxxxx>
> 
> 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
>> index 0000000000000..1104b603f5b41
>> --- /dev/null
>> +++ b/drivers/hwmon/socfpga-hwmon.c
> 
> [ ... ]
> 
>> +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:
>> +		/* Convert Q8.8 degrees Celsius to millidegrees for hwmon. */
>> +		*val = (long)(s32)temperature * SOCFPGA_HWMON_TEMP_MDEG_SCALE /
>> +			SOCFPGA_HWMON_TEMP_FRAC_DIV;
>> +		return 0;
>> +	}
>> +}
> 
> [Severity: Medium]
> Does this correctly sign-extend negative temperatures?
> 
> The Stratix 10 SDM firmware returns temperature as a 16-bit signed Q8.8 value
> inside a u32. Since negative values aren't sign-extended to 32 bits before
> this cast (for example, -1.0 is returned as 0x0000FF00), casting directly to
> s32 evaluates to 65280 instead of -256.
> 
> Could we cast to s16 first to ensure the sign bit is properly extended?
> 
> [ ... ]
> 

>> +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,
>> +						  socfpga_hwmon_async_callback,
>> +						  &completion);
>> +		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;
>> +	}
>> +
>> +	wait_ret = wait_for_completion_io_timeout(&completion, HWMON_TIMEOUT);
> 
> [Severity: High]
> Does the service layer ever invoke the provided callback?
> 
> Looking at stratix10_svc_async_send() in the firmware driver, it saves
> socfpga_hwmon_async_callback but it appears the service layer never actually
> calls it anywhere in its execution path.
> 
> Will this cause wait_for_completion_io_timeout() to always block for the full
> 2-second timeout before falling back to stratix10_svc_async_poll() below?
> 
> [ ... ]
> 
>> +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_interruptible_timeout(&priv->completion,
>> +							HWMON_TIMEOUT);
>> +	if (!ret) {
>> +		dev_err(priv->client.dev, "timeout waiting for SMC call\n");
>> +		ret = -ETIMEDOUT;
>> +		goto status_done;
>> +	}
>> +	if (ret < 0) {
>> +		dev_err(priv->client.dev, "error %d waiting for SMC call\n", ret);
>> +		goto status_done;
>> +	}
>> +
>> +	ret = priv->last_err;
>> +
>> +status_done:
>> +	stratix10_svc_done(priv->chan);
>> +	return ret;
>> +}
> 
> [Severity: High]
> If this wait is interrupted by a signal, does this leave a pending firmware
> response that could corrupt a subsequent read?
> 
> When stratix10_svc_done() is called on the error paths, it stops the kthread
> but does not cancel the in-flight hardware command or flush chan->svc_fifo.
> 
> When a subsequent sensor read calls reinit_completion() and spawns a new
> kthread, couldn't the old abandoned response unconditionally call the active
> receive_cb and trigger a premature completion with stale data?
> 
> [ ... ]
> 
>> +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;
>> +	}
> 
> [ ... ]
> 
>> +	ret = stratix10_svc_add_async_client(priv->chan, false);
>> +	switch (ret) {
>> +	case 0:
>> +		priv->async = true;
>> +		break;
>> +	case -EINVAL:
>> +		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;
>> +	}
> 
> [Severity: High]
> Is this checking the correct error code for unsupported async operations?
> 
> When the firmware lacks async support, stratix10_svc_add_async_client()
> returns -EOPNOTSUPP. Since this switch statement checks for -EINVAL, will
> the driver fall into the default case and fail to probe entirely on
> unsupported hardware instead of correctly falling back to synchronous mode?
> 
>> +
>> +	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);
>> +	}
>> +
>> +	platform_set_drvdata(pdev, priv);
>> +	return 0;
>> +}
>> +
>> +static void socfpga_hwmon_remove(struct platform_device *pdev)
>> +{
>> +	struct socfpga_hwmon_priv *priv = platform_get_drvdata(pdev);
>> +
>> +	if (priv->async)
>> +		stratix10_svc_remove_async_client(priv->chan);
>> +	stratix10_svc_free_channel(priv->chan);
>> +}
> 
> [Severity: High]
> Can this mixture of devm and manual cleanup cause a use-after-free during
> device unbind?
> 
> The hwmon device is registered with devm_hwmon_device_register_with_info(),
> so its unregistration is delayed until after socfpga_hwmon_remove() returns.
> 
> This allows concurrent sysfs reads to continue while socfpga_hwmon_remove()
> frees the async client and channel. A concurrent socfpga_hwmon_async_read()
> could then attempt to allocate from the freed achan->job_id_pool.
> 
> Should the channel cleanup be deferred via devm_add_action_or_reset() to
> ensure it runs strictly after the hwmon device is unregistered?
> 

Hi maintainers,

Thank you for the review. Please see my reply to sashiko's review.

1. [High] devm_hwmon + manual cleanup UAF
Agreed. In v3, channel/async cleanup will be deferred via 
devm_add_action_or_reset() so it runs after the devm hwmon device is 
unregistered; .remove() was dropped.

2. [High] Async reads always time out (2s delay)
Agreed that the async callback is never invoked by the service layer. In 
v3, we will pass NULL to stratix10_svc_async_send(). The pre-poll 
wait_for_completion_io_timeout() remains as a necessary delay before 
polling (same pattern as RSU); without it, the first poll returns 
INTEL_SIP_SMC_STATUS_ERROR on Stratix 10. The poll loop will matches RSU 
and retries on failure.

3. [High] Wrong error code for async fallback
stratix10_svc_add_async_client() returns -EINVAL when the async 
controller is not initialized. We will handle both -EINVAL and 
-EOPNOTSUPP when falling back to sync mode.

4. [High] Abandoned SMC on sync timeout/signal
In v3, we will fix sync reads to use uninterruptible 
wait_for_completion_timeout(). A late orphaned response remains a 
limitation of the current sync SVC interface, shared with RSU.

5. [Medium] Temperature sign extension
Agreed. SDM returns a 16-bit signed Q8.8 value in the low 16 bits. In 
v3, we will mask and cast through s16 before scaling.

Tze Yee