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

[sashiko-bot@kernel.org]

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.
- [Medium] Missing MODULE_ALIAS prevents automatic module loading.
- [Medium] Voltage readings lack SDM error code validation, leading to massive bogus voltage values being exported to userspace upon sensor error.
--

--- Patch [2]: [PATCH v4 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver ---
commit 356596525564df534eb70c8e10fde9c433f2dafd
Author: Tze Yee Ng <tze.yee.ng@altera.com>
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@altera.com>");
> +MODULE_AUTHOR("Tze Yee Ng <tze.yee.ng@altera.com>");
> +MODULE_DESCRIPTION("Altera SoC FPGA hardware monitoring driver");
> +MODULE_LICENSE("GPL");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709090153.21675-1-tze.yee.ng@altera.com?part=2