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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Unconditional NULL pointer dereference on hardware without DMI quirks.
- [High] Resource leak and potential Use-After-Free due to direct return on probe error.
- [High] Race condition between hwmon sysfs and thermal cooling device operations.
- [Medium] Violation of hwmon ABI for 'pwm_enable' attribute.
- [Low] Compilation error due to missing 'quirks' field in 'struct pcc_acpi'.
- [Low] Always-false boundary check due to unsigned comparison.
- [Low] Idiomatically incorrect header inclusion for errno.
--

commit 62f80e95f10c4cb2e0e4a1e3a89a9f92dd8d3a2e
Author: Alex Yeo <alexyeo362@gmail.com>
Subject: platform/x86: panasonic-laptop: add fan speed mode for newer models

Summary: This patch adds fan speed mode support for newer Panasonic laptop
models, registering hwmon and thermal cooling devices. It relies on DMI quirks
to enable this feature and evaluates ACPI methods for retrieving and setting
the fan speeds.

> diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
> index b83113c26f885..7774620b27635 100644
> --- a/drivers/platform/x86/panasonic-laptop.c
> +++ b/drivers/platform/x86/panasonic-laptop.c
> @@ -136,6 +139,12 @@
>  #include <linux/types.h>
>  #include <linux/uaccess.h>
>  #include <acpi/video.h>
> +#include <linux/sysfs.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/thermal.h>
> +#include <linux/dmi.h>
> +#include <asm-generic/errno.h>

[Severity: Low]
Should this include <linux/errno.h> instead? It is usually preferred for
device drivers to use the generic linux header rather than the
architecture-specific one to ensure cross-architecture compatibility.

[ ... ]

>  struct pcc_acpi {
> -	acpi_handle		handle;
> -	unsigned long		num_sifr;
[ ... ]
> -	struct platform_device	*platform;
> +	acpi_handle			handle;
[ ... ]
> +	struct platform_device		*platform;
> +	struct thermal_cooling_device	*pwm_fan_cdev;
> +};

[Severity: Low]
Does struct pcc_acpi miss the definition for the 'quirks' field? Later in
the patch, the probe function assigns to pcc->quirks, which will cause a
compilation error since it is not defined in this struct.

[ ... ]

> +static int pcc_pwm_fan_speed_set(int set_pwm_speed)
> +{
> +	struct acpi_object_list input;
> +	union acpi_object param[1];
> +	acpi_status status;
> +	int pwm_mode;
> +	int result;
> +
> +	/* Get fan status. set to manual if not set */
> +	status = pcc_pwm_fan_mode_read(&pwm_mode);
[ ... ]
> +	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> +		status = pcc_pwm_fan_mode_set(HWMON_PCC_FAN_PWM_MANUAL);
[ ... ]
> +	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FSL",
> +				      &input, NULL);

[Severity: High]
Could this sequence race with other operations? This multi-step process
of reading the mode, changing it, and setting the speed isn't protected
by a lock. Since the thermal subsystem might poll this while userspace
writes to hwmon sysfs concurrently, could this interleave and corrupt the
hardware state?

[ ... ]

> +static int pcc_pwm_fan_hwmon_read(struct device *dev,
> +				  enum hwmon_sensor_types type, u32 attr,
> +				  int channel, long *val)
> +{
[ ... ]
> +	case hwmon_pwm_enable:
> +		status = pcc_pwm_fan_mode_read(&pwm_mode);
[ ... ]
> +		if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> +			*val = HWMON_PCC_FAN_PWM_AUTO;
> +		} else if (pwm_mode == HWMON_PCC_FAN_PWM_MANUAL) {

[Severity: Medium]
Is this returning the correct hwmon ABI value for automatic mode?
HWMON_PCC_FAN_PWM_AUTO is defined as 0, but the standard hwmon ABI expects
2 for automatic mode (and 1 for manual). This might confuse standard
userspace tools like fancontrol. This same ABI mismatch appears in
pcc_pwm_fan_hwmon_write() as well.

[ ... ]

> +static int pcc_pwm_fan_thermal_set_fan_pwm(struct thermal_cooling_device *cdev,
> +					   unsigned long state)
> +{
> +	acpi_status status;
> +	int set_pwm;
> +
> +	if (state > 100 || state < 0)
> +		return -EINVAL;

[Severity: Low]
Can 'state < 0' ever evaluate to true here? Since 'state' is an unsigned
long, it cannot be negative, making this check mathematically impossible.

[ ... ]

> @@ -981,8 +1377,10 @@ static int acpi_pcc_hotkey_resume(struct device *dev)
>  
>  static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
>  {
[ ... ]
> +	/*
> +	 * Perform quirk detection
> +	 */
> +	dmi_id = dmi_first_match(pcc_quirks);
> +	if (dmi_id) {
> +		pcc->quirks = dmi_id->driver_data;
> +		pr_info("Quirk detect: Enabled quirks for %s\n", dmi_id->ident);
> +	} else {
> +		pcc->quirks = NULL;
> +	}

[ ... ]

> +	/* has_ospm_pwm_fan - add hwmon and thermal if present */
> +	if (pcc->quirks->has_ospm_pwm_fan) {

[Severity: High]
Will this dereference a NULL pointer on unsupported models? If the DMI
match fails, pcc->quirks is explicitly set to NULL above. Checking
pcc->quirks->has_ospm_pwm_fan right after will cause a kernel panic
during the device probe on those unquirked devices.

> +		pr_info("has_ospm_pwm_fan quirk: adding hwmon for %s\n", dmi_id->ident);
> +		hwmon_dev = devm_hwmon_device_register_with_info(
> +			&pdev->dev, "panasonic_pwm_fan", NULL,
> +			&pcc_pwm_fan_hwmon_chip_info, NULL);
> +		if (IS_ERR(hwmon_dev)) {
> +			pr_err("has_ospm_pwm_fan: Failed to register hwmon device\n");
> +			return PTR_ERR(hwmon_dev);
> +		}

[Severity: High]
Does this direct return bypass the error cleanup sequence? Standard failure
paths in this function use 'goto out_notify_handler;' (and other labels) to
clean up resources. Returning directly leaks memory and leaves the ACPI
notify handler and input devices registered. If the module is later
unloaded, could this trigger a use-after-free when ACPI events fire?

> +
> +		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", NULL, &pcc_pwm_fan_cdev_ops);

[Severity: High]
Does registering the thermal cooling device directly bypass the hwmon core
serialization? The subsystem guidelines indicate that thermal and sysfs
operations are serialized by the hwmon core if attributes are registered
via the 'info' parameter. Registering this manually out-of-band seems to
leave the thermal polling operations unprotected, exposing the race
condition in pcc_pwm_fan_speed_set() mentioned earlier.

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