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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Timeouts in both synchronous and asynchronous read paths abandon pending requests without flushing queues or canceling them, causing permanent data desynchronization or hangs for subsequent reads.
--

--- Patch [2]: [PATCH v8 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver ---
commit 467543c9f9e3893d8c84995c5e7bc8bab31dfb20
Author: Tze Yee Ng <[email protected]>

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. It supports temperature and voltage
sensors 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)
> +{
[ ... ]
> +	ret = -ETIMEDOUT;
> +	while (!time_after(jiffies, deadline)) {
> +		status = stratix10_svc_async_poll(priv->chan, handle, &data);
> +		if (status == -EAGAIN) {
> +			/* still in progress */
> +		} else if (status < 0) {
> +			ret = status;
> +			break;
> +		} else if (status == 0) {
> +			ret = 0;
> +			break;
> +		}
> +		usleep_range(HWMON_RETRY_SLEEP_US, HWMON_RETRY_SLEEP_US * 2);
> +	}
> +
> +	if (ret) {
> +		dev_err(dev, "Failed to get async response\n");
> +		goto done;
> +	}
[ ... ]
> +done:
> +	stratix10_svc_async_done(priv->chan, handle);
> +	return ret;
> +}

[Severity: High]
If the polling loop times out, this calls stratix10_svc_async_done() which
immediately frees the job ID via ida_free().

Since the firmware might still be processing this request, could the next
async read reuse this job ID and intercept the delayed completion of this
timed-out request?

[ ... ]
> +static int socfpga_hwmon_sync_read(struct device *dev,
> +				   enum hwmon_sensor_types type,
> +				   struct stratix10_svc_client_msg *msg)
> +{
[ ... ]
> +	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;
> +}

[Severity: High]
If wait_for_completion_timeout() times out, this calls stratix10_svc_done(),
which stops the worker thread via kthread_stop() without flushing the pending
message from the channel's svc_fifo.

If the worker thread evaluates kthread_should_stop() before dequeuing the
message, will the stale message stay in the queue?

If so, could the next read spawn a new thread that dequeues the stale message
and returns wrong data (like returning a voltage reading for a temperature
request)?

Also, if the worker thread is stuck inside the SMC call, won't kthread_stop()
block indefinitely, hanging the reader process?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2