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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Reading the PWM value causes an arithmetic underflow and returns a negative error code when the hardware value is 0.
- [High] The driver permanently claims standard globally multiplexed Super I/O ports using `devm_request_region()`, breaking x86 concurrency and causing a Denial of Service for other drivers.
- [High] Missing suspend/resume power management callbacks to re-initialize the EC state after system sleep.
--

commit 5f38d453c010b9c77f2e24db2a05033fcf65d138
Author: Pei Xiao <xiaopei01@kylinos.cn>

hwmon: (gpd-fan): Initialize EC before registering hwmon device

The commit moves the gpd_init_ec() call to happen before registering
the hwmon device. This prevents a window where sysfs reads could
return invalid values if the EC is not yet initialized.

> 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);

[Severity: High]
This isn't a bug introduced by this patch, but since we are initializing the
EC here due to buggy firmware losing its state, does the EC also lose its
state across system sleep transitions?

Looking at gpd_fan_driver, the platform driver is missing power management
callbacks (pm_ops) for suspend and resume. When the system returns from
S3 suspend-to-RAM, will the EC return to an uninitialized state and silently
break thermal fan control since there is no resume hook to re-invoke
gpd_init_ec()?

Also, this is a pre-existing issue, but does using devm_request_region() in
gpd_fan_probe() permanently claim the globally multiplexed Super I/O ports
(0x2E-0x2F or 0x4E-0x4F)?

These ports are heavily shared across the motherboard. Standard practice is to
lock them dynamically for a single transaction using request_muxed_region(). By
locking them permanently via devm_request_region(), does this prevent other
drivers from claiming the multiplexer and cause them to fail probing?

Additionally, when interacting with these ports in gpd_ecram_read() and
gpd_ecram_write(), the multi-step outb/inb sequences are executed without any
locking. Could this race dangerously with other subsystems or ACPI?

>  	hwdev = devm_hwmon_device_register_with_info(dev,
>  						     DRIVER_NAME,
>  						     data,
>  						     &gpd_fan_chip_info,
>  						     NULL);
>  	if (IS_ERR(hwdev))
>  		return dev_err_probe(dev, PTR_ERR(hwdev),
>  				     "Failed to register hwmon device\n");
> -
> -	gpd_init_ec(data);
> -
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but could the calculation in gpd_wm2_read_pwm()
underflow and propagate a negative error code to userspace?

drivers/hwmon/gpd-fan.c:gpd_wm2_read_pwm() {
	...
	u8 var;

	gpd_ecram_read(drvdata, drvdata->pwm_write, &var);

	// Match gpd_generic_write_pwm(u8) below
	return DIV_ROUND_CLOSEST((var - 1) * 255, (drvdata->pwm_max - 1));
}

If the hardware returns a PWM value of 0, (var - 1) promotes to a signed integer
and evaluates to -1. Multiplying by 255 and dividing propagates this negative
result, causing the function to return a negative value, which is then reported
to userspace as a negative error code (e.g., -EPERM) instead of gracefully
reporting a valid 0 PWM state.

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