Re: [PATCH v1] hwmon: (yogafan) Add support for Lenovo Yoga/Legion fan monitoring

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unprivileged Denial of Service of the RLLag telemetry filter via fast polling (shared state pollution).
- [Medium] Silent bypass of WMI Coexistence Guard due to Kconfig mismatch (`IS_REACHABLE` misuse).
--

commit db32fd276d89299ba906703e55172f8ec3415249
Author: Sergio Melas <sergiomelas@gmail.com>

hwmon: (yogafan) Add support for Lenovo Yoga/Legion fan monitoring

This driver provides fan speed monitoring for Lenovo Yoga, Legion, and
IdeaPad laptops by interfacing with the Embedded Controller (EC) via ACPI.
It implements a Rate-Limited Lag filter to smooth out low-resolution sampling
in the embedded controller.

> diff --git a/drivers/hwmon/yogafan.c b/drivers/hwmon/yogafan.c
> index 605cc928f21f3..7d66d563ec236 100644
> --- a/drivers/hwmon/yogafan.c
> +++ b/drivers/hwmon/yogafan.c

[ ... ]

> @@ -50,48 +62,109 @@ struct yoga_fan_data {
>  static void apply_rllag_filter(struct yoga_fan_data *data, int idx, long raw_rpm)
>  {
>  	ktime_t now = ktime_get_boottime();
> -	s64 dt_ms = ktime_to_ms(ktime_sub(now, data->last_sample[idx]));
> +	s64 raw_dt_ms;
>  	long delta, step, limit, alpha;
>  	s64 temp_num;
> +	u32 dt_ms;

[ ... ]

> -	if (data->last_sample[idx] == 0 || dt_ms > MAX_SAMPLING) {
> +	/* 3. Auto-Reset Logic: Snap to hardware value after long gaps (>5s) */
> +	/*   Ref: [TAG: INIT_STATE, STALE_DATA_THRESHOLD] */
> +	raw_dt_ms = ktime_to_ms(ktime_sub(now, data->last_sample[idx]));
> +
> +	if (data->last_sample[idx] == 0 || raw_dt_ms < MIN_SAMPLING || raw_dt_ms > MAX_SAMPLING) {
>  		data->filtered_val[idx] = raw_rpm;
>  		data->last_sample[idx] = now;
>  		return;
>  	}

[Severity: High]
Does combining the MIN_SAMPLING check into this auto-reset block create an
unintended bypass of the filter?

If an unprivileged user queries the fanX_input sysfs attribute faster than
MIN_SAMPLING (100ms), raw_dt_ms < MIN_SAMPLING evaluates to true.

Because this block sets data->last_sample[idx] = now, the time delta never
accumulates to exceed 100ms on continuous fast polling. This seems to snap
filtered_val to raw_rpm repeatedly, entirely defeating the filter's smoothing
logic.

[ ... ]

> @@ -206,6 +298,17 @@ static int yoga_fan_probe(struct platform_device *pdev)
>  	struct device *hwmon_dev;
>  	int i;
>  
> +	/* Check for WMI interfaces that handle fan/thermal management. */
> +	/*  If present, we yield to the WMI driver to prevent double-reporting. */
> +#if IS_REACHABLE(CONFIG_ACPI_WMI)
> +	if (wmi_has_guid(LENOVO_WMI_OTHER_MODE_GUID) &&
> +	    wmi_has_guid(LENOVO_CAPABILITY_DATA_00_GUID) &&
> +	    wmi_has_guid(LENOVO_FAN_TEST_DATA_GUID)) {
> +		dev_info(&pdev->dev, "Lenovo WMI management interface detected; yielding to WMI driver\n");
> +		return -ENODEV;
> +	}
> +#endif

[Severity: Medium]
Is IS_REACHABLE the correct macro to use here for the WMI coexistence guard?

If the driver is built-in (CONFIG_YOGAFAN=y) but ACPI WMI is built as a
module (CONFIG_ACPI_WMI=m), IS_REACHABLE(CONFIG_ACPI_WMI) will evaluate to
false.

This silently compiles out the guard, which could lead to hardware race
conditions between this driver and the WMI driver once the module loads.

Would it be safer to use IS_ENABLED() alongside a Kconfig dependency like
depends on ACPI_WMI || ACPI_WMI=n?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260627084039.47158-1-sergiomelas@gmail.com?part=1