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

[NG, TZE YEE]

On 9/7/2026 5:15 pm, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] The asynchronous read path times out prematurely and frees the SMC transaction ID while the Secure Device Manager (SDM) may still be processing the request, causing ID reuse and state confusion.

Agreed. The async poll loop was limited to three iterations (~3 ms), 
while the sync path uses HWMON_TIMEOUT (2 s). That could call 
stratix10_svc_async_done() and recycle the job ID before SDM finished.

In v5, we will poll until jiffies + HWMON_TIMEOUT before declaring 
timeout and calling async_done(), matching the sync path timeout.

> - [Medium] Missing MODULE_ALIAS prevents automatic module loading.

Agreed. In v5, We will add:
MODULE_ALIAS("platform:socfpga-hwmon");
so the module auto-loads when stratix10-svc registers the platform 
device, consistent with other parent-registered hwmon drivers (e.g. 
raspberrypi-hwmon).

> - [Medium] Voltage readings lack SDM error code validation, leading to massive bogus voltage values being exported to userspace upon sensor error.

We checked the SDM specification: value-field error codes (ETEMP_*) are 
defined for temperature only, not for voltage. Voltage failures are 
reported through the SVC/SDM command status, which the driver already 
checks:

Async: data.status in socfpga_hwmon_async_read() before using the value
Sync: data->status == BIT(SVC_STATUS_OK) in the receive_cb before 
storing the value

> --
> 
> --- Patch [2]: [PATCH v4 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver ---
> commit 356596525564df534eb70c8e10fde9c433f2dafd
> Author: Tze Yee Ng <tze.yee.ng@xxxxxxxxxx>
> Subject: hwmon: add Altera SoC FPGA hardware monitoring driver
> 
> 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.
> 
>> diff --git a/drivers/hwmon/socfpga-hwmon.c b/drivers/hwmon/socfpga-hwmon.c
>> new file mode 100644
>> index 0000000000000..1cd3a06fec42b
>> --- /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 = {};
>> +	void *handle = NULL;
>> +	int status, index, ret;
> 
> [ ... ]
> 
>> +	ret = -ETIMEDOUT;
>> +	for (index = 0; index < HWMON_ASYNC_MSG_RETRY; index++) {
>> +		status = stratix10_svc_async_poll(priv->chan, handle, &data);
> 
> [Severity: High]
> The synchronous path uses a 2000ms timeout, but this loop times out after just
> 3 iterations (with a 1ms sleep), allowing only ~3ms. Is this polling duration
> sufficient for the worst-case response time from the Secure Device Manager
> (SDM)?
> >> +		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);
> 
> [Severity: High]
> If the async read times out, calling stratix10_svc_async_done() frees the
> transaction ID immediately.
> 
> Can this cause the ID to be reused for a new request while the SDM is still
> processing the original delayed request, violating the SMC protocol and
> potentially causing corrupted data to be returned?
> 
>> +	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 (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;
> 
> [Severity: Medium]
> The temperature reading path explicitly validates against generic SDM error
> codes (like ETEMP_INACTIVE or 0x80000000U).
> 
> Since voltage readings lack this check, will a sensor error code here be
> blindly converted and exported as a massive bogus voltage value to userspace
> instead of returning a proper Linux error code?
> 
>> +	return ret;
>> +}
> 
> [ ... ]
> 
>> +static struct platform_driver socfpga_hwmon_driver = {
>> +	.probe = socfpga_hwmon_probe,
>> +	.driver = {
>> +		.name = "socfpga-hwmon",
>> +	},
>> +};
> 
> [Severity: Medium]
> Since this driver binds to a platform device dynamically registered by the
> service layer (and lacks an id_table or of_match_table), is a MODULE_ALIAS
> declaration (like MODULE_ALIAS("platform:socfpga-hwmon")) needed here so that
> udev can automatically load the module when the device is registered?
> 
>> +module_platform_driver(socfpga_hwmon_driver);
>> +
>> +MODULE_AUTHOR("Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@xxxxxxxxxx>");
>> +MODULE_AUTHOR("Tze Yee Ng <tze.yee.ng@xxxxxxxxxx>");
>> +MODULE_DESCRIPTION("Altera SoC FPGA hardware monitoring driver");
>> +MODULE_LICENSE("GPL");
>