Re: [RFC PATCH v4] platform/x86: panasonic-laptop: add fan speed mode for newer models

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The returned hwmon device pointer from `hwmon_device_register_with_info` is never assigned to `pcc->pwm_fan_hwmon` on success.
- [Medium] Missing control flow abort after an error in `pcc_pwm_fan_hwmon_read` allows the evaluation of an uninitialized stack variable.
- [Medium] Mixing POSIX error codes with `acpi_status` types causes continuous kernel log spam when reading the hwmon PWM attribute in AUTO mode.
- [Medium] Writing 'auto' mode via the hwmon sysfs interface erroneously passes a hwmon constant to the hardware ACPI function, forcing the hardware into 'manual' mode instead.
--

commit 0a2b607080f828361b9b1616ec43f15ca1686629
Author: Alex Yeo <alexyeo362@gmail.com>

platform/x86: panasonic-laptop: add fan speed mode for newer models

This patch introduces fan speed mode control for newer Panasonic laptop models,
specifically the CF-SR4. It adds ACPI-based reading and writing of PWM fan
speeds and modes, exposing them through hwmon and thermal sysfs interfaces.

> diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
> index b83113c26f885..ab1e0e549b300 100644
> --- a/drivers/platform/x86/panasonic-laptop.c
> +++ b/drivers/platform/x86/panasonic-laptop.c
[ ... ]
> @@ -507,6 +560,179 @@ static int set_optd_power_state(int new_state)
>  	return result;
>  }
>  
> +/* get OSPM fan mode */
> +
> +static int pcc_pwm_fan_mode_read(int *pwm_mode)
> +{
[ ... ]
> +/* read PWM fan speed */
> +
> +static int pcc_pwm_fan_speed_read(int *pwm_speed)
> +{
[ ... ]
> +	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> +		result = -ENODATA; /* Indeterminate value */
> +		goto out;
> +	}

This successfully returns the POSIX error -ENODATA when the fan is in automatic
mode. I will reference this below.

[ ... ]
> +/* set PWM fan speed */
> +
> +static int pcc_pwm_fan_speed_set(int set_pwm_speed)
> +{
[ ... ]
> +	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> +		status = pcc_pwm_fan_mode_set(HWMON_PCC_FAN_PWM_MANUAL);

[Severity: Medium]
Are we passing the wrong constant here? HWMON_PCC_FAN_PWM_MANUAL is defined
for the hwmon sysfs interface. Does pcc_pwm_fan_mode_set() expect the hardware
constant ACPI_PCC_FAN_PWM_MANUAL instead?

[ ... ]
> @@ -825,6 +1052,240 @@ static const struct attribute_group pcc_attr_group = {
>  	.is_visible	= pcc_sysfs_is_visible,
>  };
>  
> +/* hwmon */
[ ... ]
> +static int pcc_pwm_fan_hwmon_read(struct device *dev,
> +				  enum hwmon_sensor_types type, u32 attr,
> +				  int channel, long *val)
> +{
[ ... ]
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		mutex_lock(&pcc->pwm_fan_lock);
> +
> +		status = pcc_pwm_fan_speed_read(&pwm_speed);
> +		if (ACPI_FAILURE(status)) {
> +			pr_err("%s: failed to get fan pwm speed\n", __func__);
> +			result = -ENODATA;
> +		} else {

[Severity: Medium]
Will this cause continuous kernel log spam when the fan is in auto mode?

As noted above, pcc_pwm_fan_speed_read() returns the POSIX error -ENODATA when
in automatic mode. Here, the caller receives this negative integer into an
acpi_status variable.

Since -ENODATA is non-zero, ACPI_FAILURE() evaluates to true, which triggers
the error log. This could flood dmesg if userspace tools periodically read the
hwmon attribute while the fan is in auto mode.

[ ... ]
> +	case hwmon_pwm_enable:
> +		mutex_lock(&pcc->pwm_fan_lock);
> +
> +		status = pcc_pwm_fan_mode_read(&pwm_mode);
> +		if (ACPI_FAILURE(status)) {
> +			pr_err("%s: failed to get fan pwm mode\n", __func__);
> +			result = -ENODATA;
> +		}
> +		if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {

[Severity: Medium]
If pcc_pwm_fan_mode_read() fails, we log an error and set result = -ENODATA,
but execution continues to the next line. Does this mean we might evaluate the
uninitialized stack variable pwm_mode? Should there be a break or goto
statement in the error path?

[ ... ]
> +static int pcc_pwm_fan_hwmon_write(struct device *dev,
> +				   enum hwmon_sensor_types type, u32 attr,
> +				   int channel, long val)
> +{
[ ... ]
> +	switch (attr) {
> +	case hwmon_pwm_enable:
> +		if (val == HWMON_PCC_FAN_PWM_AUTO) {
> +			mutex_lock(&pcc->pwm_fan_lock);
> +			status = pcc_pwm_fan_mode_set(HWMON_PCC_FAN_PWM_AUTO);

[Severity: Medium]
Is the wrong constant being passed to pcc_pwm_fan_mode_set() here?

val == HWMON_PCC_FAN_PWM_AUTO matches the hwmon constant (2). However,
passing HWMON_PCC_FAN_PWM_AUTO to pcc_pwm_fan_mode_set() means we are sending
2 to the ACPI firmware instead of the expected hardware constant
ACPI_PCC_FAN_PWM_AUTO (0).

Since the firmware treats any value >0 as manual mode, does writing 'auto'
(2) to the sysfs attribute inadvertently force the hardware into manual mode?

[ ... ]
> @@ -1097,6 +1571,39 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
>  	}
>  
>  	i8042_install_filter(panasonic_i8042_filter, NULL);
> +
> +	/* check quirks */
> +	if (pcc->quirks) {
> +		/* has_ospm_pwm_fan - add hwmon and thermal if present */
> +		if (pcc->quirks->has_ospm_pwm_fan) {
> +			mutex_init(&pcc->pwm_fan_lock);
> +
> +			pr_info("has_ospm_pwm_fan quirk: adding hwmon for %s\n", dmi_id->ident);
> +
> +			hwmon_dev = hwmon_device_register_with_info(
> +				&pdev->dev, "panasonic_pwm_fan", pcc,
> +				&pcc_pwm_fan_hwmon_chip_info, NULL);
> +			if (IS_ERR(hwmon_dev)) {
> +				pr_err("has_ospm_pwm_fan: Failed to register hwmon device\n");
> +				/* not a critical error. just skip if error */
> +				pcc->pwm_fan_hwmon = NULL;
> +			}

[Severity: High]
If hwmon_device_register_with_info() succeeds, we never assign hwmon_dev to
pcc->pwm_fan_hwmon.

Because pcc->pwm_fan_hwmon remains NULL, the success path behaves as if the
registration failed when checking for cleanup.

> +
> +			/* proceed with thermal */
> +			pr_info("has_ospm_pwm_fan quirk: adding thermal for %s\n", dmi_id->ident);
> +			pcc->pwm_fan_cdev = thermal_cooling_device_register(
> +				"Panasonic_PWM_Fan", pcc, &pcc_pwm_fan_cdev_ops);
> +			if (IS_ERR(pcc->pwm_fan_cdev)) {
> +				pr_warn("has_ospm_pwm_fan: Failed to register thermal cdev\n");
> +				pcc->pwm_fan_cdev = NULL;
> +			}
> +
> +			/* cleanup: destroy mutex if both hwmon and thermal failed */
> +			if (!pcc->pwm_fan_hwmon && !pcc->pwm_fan_cdev)
> +				mutex_destroy(&pcc->pwm_fan_lock);

Since pcc->pwm_fan_hwmon is still NULL upon successful hwmon registration, if
thermal registration fails here, won't we prematurely destroy the mutex while
the hwmon device is still active and potentially using it?

[ ... ]
> @@ -1124,6 +1631,13 @@ static void acpi_pcc_hotkey_remove(struct platform_device *pdev)
>  	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
>  	struct pcc_acpi *pcc = acpi_driver_data(device);
>  
> +	if (pcc->pwm_fan_hwmon)
> +		hwmon_device_unregister(pcc->pwm_fan_hwmon);

Because pcc->pwm_fan_hwmon was never assigned, this unregistration is bypassed
on module unload. This will leak the hwmon device and leave its sysfs attributes
active. Accessing them after the module is unloaded would dereference freed
memory.

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