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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The driver converts a timeout condition into an infinite wait on error paths, causing the calling process to hang in an unkillable D-state if the firmware fails to respond or drops the request.
--

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

hwmon: add Altera SoC FPGA hardware monitoring driver

This commit 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
> --- /dev/null
> +++ b/drivers/hwmon/socfpga-hwmon.c

[ ... ]

> +static int socfpga_hwmon_sync_read(struct device *dev,
> +				   enum hwmon_sensor_types type,
> +				   struct stratix10_svc_client_msg *msg)
> +{
> +	struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	reinit_completion(&priv->completion);
> +
> +	if (type == hwmon_temp)
> +		priv->client.receive_cb = socfpga_hwmon_readtemp_cb;
> +	else
> +		priv->client.receive_cb = socfpga_hwmon_readvolt_cb;
> +
> +	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");
> +		/*
> +		 * stratix10_svc_done() stops the worker but does not flush
> +		 * svc_fifo. Wait for the outstanding callback before stopping
> +		 * so a leftover request cannot complete a later sensor read.
> +		 * Discard the late result and still report -ETIMEDOUT.
> +		 */
> +		wait_for_completion(&priv->completion);

[Severity: High]
Does this unconditional wait_for_completion() cause the reading process to
hang indefinitely in an unkillable D-state?

If the firmware fails to respond, or if the underlying service thread receives
a busy or rejected status from the firmware during the SMC call, the service
thread skips invoking the receive_cb callback entirely.

Because the callback is never invoked in these failure modes,
wait_for_completion_timeout() correctly expires. However, the subsequent
wait_for_completion() then blocks forever while the process still holds
priv->lock, deadlocking all future sensor reads for the device.

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

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