Re: [PATCH 5/5] arm64: dts: socfpga: stratix10: add hwmon node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Hardware monitoring sysfs attributes remain active after the service channel is freed during driver removal, leading to a kernel panic.
- [High] Incorrect temperature scaling calculation reports temperatures 1000 times too small to the hwmon subsystem.
- [High] Asynchronous sysfs reads are effectively broken and will always block for the full 2-second timeout before falling back to manual polling.
--

commit a3baf92605bf6c042e451ccdb74f299304cebf79
Author: Tze Yee Ng <tze.yee.ng@altera.com>

arm64: dts: socfpga: stratix10: add hwmon node
    
Add an hwmon child node under the Stratix 10 service layer and describe
the SoCDK voltage and temperature sensors using the altr,stratix10-hwmon
compatible.

> diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts b/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts
> index e2a1cea7f3da9..01a8ffe430ed0 100644
> --- a/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts
> +++ b/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts
[ ... ]
> @@ -134,3 +134,36 @@ root: partition@4200000 {
>  		};
>  	};
>  };
> +
> +&temp_volt {
> +	status = "okay";

[Severity: High]
This isn't a bug introduced by this patch, but enabling this hwmon node exposes
a pre-existing lifecycle issue during driver removal that can lead to a kernel
panic.

The stratix10-hwmon driver uses devm_hwmon_device_register_with_info() for
registration, meaning the sysfs attributes are only unregistered during devres
cleanup after stratix10_hwmon_remove() returns. However, the remove function
synchronously frees the service channel (stratix10_svc_free_channel), which
sets chan->scl to NULL.

If a userspace process reads a sysfs attribute concurrently, the service thread
might unconditionally dereference chan->scl in svc_thread_recv_status_ok():

drivers/firmware/stratix10-svc.c:svc_thread_recv_status_ok() {
    ...
    p_data->chan->scl->receive_cb(p_data->chan->scl, cb_data);
    ...
}

Should the driver manage the service channel lifecycle via
devm_add_action_or_reset() so the channel strictly outlives the sysfs
attributes?

[Severity: High]
This is a pre-existing issue, but enabling this hwmon node exposes an incorrect
temperature scaling calculation.

The hwmon subsystem strictly requires temperature attributes to be reported in
millidegrees Celsius. The hardware reports the temperature in Q8.8 format.

In stratix10_hwmon_parse_temp(), the driver shifts the value right by 8 bits,
effectively returning raw degrees Celsius:

drivers/hwmon/stratix10-hwmon.c:stratix10_hwmon_parse_temp() {
    ...
    *val = (long)(s32)temperature / STRATIX10_HWMON_TEMP_FRAC_DIV;
    ...
}

Because it fails to multiply the result by 1000, a temperature of 50 degrees
Celsius is reported as 50 instead of 50000. Monitoring tools will mistakenly
display this as 0.050 degrees Celsius.

Could the conversion logic be updated to multiply the result by 1000 to
satisfy the hwmon core requirements?

[Severity: High]
This is a pre-existing issue, but enabling this hwmon node exposes a broken
asynchronous read path that induces a 2-second latency spike per read.

The stratix10_hwmon_async_read() function waits on a completion:

drivers/hwmon/stratix10-hwmon.c:stratix10_hwmon_async_read() {
    ...
    wait_ret = wait_for_completion_io_timeout(&completion, HWMON_TIMEOUT);
    ...
}

It expects the callback stored in handle->cb by stratix10_svc_async_send()
to be executed. However, the stratix10-svc driver never actually invokes this
callback anywhere in the service layer.

As a result, the completion is never signaled. Every sysfs read incurs a
mandatory 2-second timeout delay before it falls back to manual polling via
stratix10_svc_async_poll().

Is it possible to either implement the callback invocation in the service layer,
or adjust the async read approach to avoid this latency?

> +
> +	voltage {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		input@2 {
> +			label = "0.8V VCC";
> +			reg = <2>;
> +		};
[ ... ]

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