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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing break in async poll loop causes severe kernel log spam and unnecessary blocking on hard errors.
- [High] Synchronous read timeouts leave stale requests in the channel FIFO, causing all subsequent reads to process stale data and return mismatched sensor values.
--

--- Patch [2]: [PATCH v6 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver ---
commit ab6c0c0f95d3890d759e04d9d9aecd151d6406fd
Author: Tze Yee Ng <tze.yee.ng@altera.com>
Subject: hwmon: add Altera SoC FPGA hardware monitoring driver

This commit adds a hardware monitoring driver for Altera SoC FPGA devices using
the Stratix 10 service layer.

> diff --git a/drivers/hwmon/socfpga-hwmon.c b/drivers/hwmon/socfpga-hwmon.c
> new file mode 100644
> index 0000000000000..72418201b65d6
> --- /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)
> +{
[ ... ]
> +	ret = -ETIMEDOUT;
> +	while (!time_after(jiffies, deadline)) {
> +		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;

[Severity: High]
Does this loop need a break statement here when status < 0? 

If stratix10_svc_async_poll() returns a hard error, the code sets 
ret = -ETIMEDOUT but continues to loop for up to 2 seconds. This could
flood the kernel log with dev_alert() messages on every 1ms retry.

> +		} else if (status == 0) {
> +			ret = 0;
> +			break;
> +		}
> +		msleep(HWMON_RETRY_SLEEP_MS);
> +	}
[ ... ]
> +static int socfpga_hwmon_sync_read(struct device *dev,
> +				   enum hwmon_sensor_types type,
> +				   struct stratix10_svc_client_msg *msg)
> +{
[ ... ]
> +	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;

[Severity: High]
If a timeout occurs here, could the unfulfilled request remain in the
SVC channel's FIFO?

When the timeout hits, this path calls stratix10_svc_done() which stops the
worker thread, but it doesn't appear to remove the request from chan->svc_fifo.

Would the next sensor read enqueue a new request and receive the stale data
from this timed-out request, permanently desynchronizing all subsequent
sensor readings?

> +	}
> +
> +	ret = priv->last_err;
> +
> +status_done:
> +	stratix10_svc_done(priv->chan);
> +	return ret;
> +}

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