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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Returning `100` (max_state) for the thermal cooling device's current state when in AUTO mode defeats the thermal governor and permanently degrades cooling.
- [Medium] The driver assigns standard Linux error codes to an `acpi_status` variable and evaluates them using `ACPI_FAILURE()`.
- [Medium] The `pcc_pwm_fan_hwmon_read` function lacks bounds checking for the raw PWM speed returned by the hardware, potentially violating the hwmon ABI.
- [Low] The driver introduces a private mutex (`pwm_fan_lock`) instead of using the hwmon subsystem's locking mechanism.
- [Low] The global `quirks` variable is assigned in a DMI callback but never used.
--

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

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

This commit adds fan speed and mode support for newer Panasonic laptop models 
like the CF-SR4. It utilizes ACPI methods to read and write the fan profile 
and registers the fan with both the hwmon and thermal subsystems.

> diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
> index b83113c26f885..a1837106ff6b1 100644
> --- a/drivers/platform/x86/panasonic-laptop.c
> +++ b/drivers/platform/x86/panasonic-laptop.c
[ ... ]
> @@ -238,19 +258,53 @@ static const struct key_entry panasonic_keymap[] = {
>  };
>  
>  struct pcc_acpi {
[ ... ]
> +	struct thermal_cooling_device	*pwm_fan_cdev;
> +	struct device			*pwm_fan_hwmon;
> +	struct mutex			pwm_fan_lock;

[Severity: Low]
Does this driver need a private mutex for the hwmon attributes? 

The hardware monitoring subsystem guidelines dictate using hwmon_lock() and 
hwmon_unlock() for attributes registered outside the core, rather than 
implementing a driver-specific lock.

>  };
> +
> +/*
> + * Declare quirks and apply matches
> + */
> +static struct quirk_entry *quirks;
> +
> +static struct quirk_entry quirk_cf_sr4 = {
> +	.has_ospm_pwm_fan = true,
> +};
> +
> +/* DMI matching for quirks copied from asus-nb-wmi.c */
> +static int dmi_matched(const struct dmi_system_id *dmi)
> +{
> +	pr_info("Identified laptop model '%s'\n", dmi->ident);
> +	quirks = dmi->driver_data;

[Severity: Low]
Is this global quirks variable necessary? 

The dmi_matched() callback assigns the matched data here, but the driver 
appears to ignore this global variable later, instead directly assigning 
pcc->quirks = dmi_id->driver_data during probe.

> +	return 1;
> +}
[ ... ]
> @@ -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)
> +{
> +	struct pcc_acpi *pcc;
> +	acpi_status status;
> +	int pwm_speed;
> +	int pwm_mode;
> +	int result;
> +
> +	pcc = dev_get_drvdata(dev);
> +
> +	if (type != hwmon_pwm)
> +		return -EOPNOTSUPP;
> +
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		mutex_lock(&pcc->pwm_fan_lock);
> +
> +		status = pcc_pwm_fan_speed_read(&pwm_speed);
> +		if (ACPI_FAILURE(status)) {
> +			result = status;

[Severity: Medium]
Does this safely mix Linux error codes with ACPI status codes?

Helper functions like pcc_pwm_fan_speed_read() return negative standard
Linux error codes (e.g., -EINVAL), but here they are assigned to an
acpi_status variable and evaluated with ACPI_FAILURE().

While ACPI_FAILURE() might evaluate to true for negative values due to two's
complement representation, it violates the ACPICA API contract and can lead
to swallowed or mangled error codes being passed back to userspace.

> +		} else {
> +			*val = (pwm_speed * 255) / 100;

[Severity: Medium]
Could this calculation expose an out-of-bounds value to userspace? 

If the ACPI method returns a value greater than 100 (which can happen for 
unknown or indeterminate states like 255), this unchecked math will produce 
a value outside the standard 0-255 range mandated by the hwmon ABI for 
pwm attributes.

> +			result = 0;
> +		}
> +
> +		mutex_unlock(&pcc->pwm_fan_lock);
> +		return result;
[ ... ]
> +static int
> +pcc_pwm_fan_thermal_get_current_pwm(struct thermal_cooling_device *cdev,
> +				    unsigned long *state)
> +{
> +	struct pcc_acpi *pcc;
> +	acpi_status status;
> +	int current_pwm;
> +	int pwm_mode;
> +	int result;
> +
> +	pcc = cdev->devdata;
> +
> +	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;
> +		goto out;
> +	}
> +	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> +		*state = 100; /* Return failsafe value on EC mode */

[Severity: High]
Does returning 100 (the maximum state) while in AUTO mode paralyze the
thermal governor?

If the system overheats while the fan is in AUTO mode, the step_wise
governor will read the current state as 100 and assume it is at maximum
cooling capacity, doing nothing to ramp up the fan.

When the temperature drops, it will decrease the state to 99, invoking
set_cur_state(99). This immediately and permanently switches the fan to
MANUAL mode, which has a lower maximum speed limit than AUTO mode, degrading
cooling performance permanently.

> +		result = 0;
> +		goto out;
> +	}

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