Re: [PATCH v3 2/4] hwmon: (gpd-fan): Initialize EC before registering hwmon device

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Reading a PWM value of 0 from the EC results in an integer underflow, causing sysfs reads to return a spurious error code.
- [High] The driver statically and exclusively requests standard Super I/O ports, preventing dynamic resource sharing and risking I/O collisions.
--

commit 56dbaa61ebc060459a8d3ef1f7c952d1f935af40
Author: Pei Xiao <xiaopei01@kylinos.cn>
Subject: hwmon: (gpd-fan): Initialize EC before registering hwmon device

This patch moves the EC initialization before the hardware monitoring
device registration. This prevents a race condition where sysfs reads
could return invalid values before the EC is properly initialized.

> Some buggy firmware won't initialize EC properly on boot. Before its
> initialization, reading RPM will always return 0, and writing PWM will have
> no effect. So move gpd_init_ec to before hwmon device register.

[Severity: High]
This is a pre-existing issue, but does the driver handle a valid PWM read
of 0 correctly?

Looking at gpd_wm2_read_pwm(), the raw PWM value 'var' is an unsigned 8-bit
integer. If the EC returns 0 (e.g., when the fan is stopped), the expression
'(var - 1)' mathematically underflows and promotes to a signed integer (-1).
Multiplying by 255 and passing it to DIV_ROUND_CLOSEST yields a negative value:

gpd_wm2_read_pwm() {
    ...
    return DIV_ROUND_CLOSEST((var - 1) * 255, (drvdata->pwm_max - 1));
}

This negative return value propagates up to gpd_fan_hwmon_read(), which
interprets it as an error code (-1 mapping to -EPERM), causing the sysfs read
for pwm1 to fail. Can this be updated to safely handle a PWM value of 0?

> diff --git a/drivers/hwmon/gpd-fan.c b/drivers/hwmon/gpd-fan.c
> index 7284babd4f5c9..745b3fb9e3a49 100644
> --- a/drivers/hwmon/gpd-fan.c
> +++ b/drivers/hwmon/gpd-fan.c
> @@ -643,6 +643,7 @@ static int gpd_fan_probe(struct platform_device *pdev)
>  
>  	dev_set_drvdata(dev, data);
>  
> +	gpd_init_ec(data);
>  	hwdev = devm_hwmon_device_register_with_info(dev,
>  						     DRIVER_NAME,

[Severity: High]
This isn't a bug introduced by this patch, but does this probe function
exclusively claim globally shared Super I/O ports?

Looking earlier in gpd_fan_probe(), the driver statically requests standard
Super I/O address and data ports exclusively for the lifetime of the module:

gpd_fan_probe() {
    ...
    region = devm_request_region(dev, res->start,
                                 resource_size(res), DRIVER_NAME);
    ...
}

Because Super I/O ports are globally shared, permanently claiming them prevents
other drivers (like watchdogs or RTCs) from probing. Should this use
request_muxed_region() to dynamically lock the ports around each I/O sequence
instead?

>  						     data,
> @@ -651,9 +652,6 @@ static int gpd_fan_probe(struct platform_device *pdev)
>  	if (IS_ERR(hwdev))
>  		return dev_err_probe(dev, PTR_ERR(hwdev),
>  				     "Failed to register hwmon device\n");
> -
> -	gpd_init_ec(data);
> -
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1780880972.git.xiaopei01@kylinos.cn?part=2