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

On 23/7/2026 2:15 pm, [email protected] wrote:
> 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?
> 

Agreed — this is a valid concern.

On async timeout we call stratix10_svc_async_done(), which frees the
job ID via ida_free(). Poll is keyed by transaction_id, so if firmware
is still completing the old request, a later async_send() that reuses
that job ID could observe the delayed response.

A proper fix belongs in the SVC / ATF (e.g. cancel before
freeing the ID, or keep the ID allocated until firmware completion).
We will address that in a separate SVC series rather than papering over
it in the hwmon driver.

Please let me know if you would prefer a different approach.

> [ ... ]
>> +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?
> 

v8 drops the extra wait_for_completion() after timeout, then it reports 
-ETIMEDOUT and calls stratix10_svc_done(). As noted in my earlier reply 
to Guenter [1], the remaining concern is that stratix10_svc_done() stops 
the worker without flushing svc_fifo, so a request dequeued only after 
the client has already timed out could theoretically desynchronize a 
later read. We plan to address that with a FIFO flush in the service 
layer in a separate series.

[1] 
https://lore.kernel.org/all/[email protected]/

Thanks,
Tze Yee