Re: [PATCH] hwmon: (gpio-fan) take fan_data->lock in gpio_fan_shutdown()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] If `pm_runtime_put_sync` fails in `set_fan_speed`, early return without updating `fan_data->speed_index` causes hardware/software state desync and PM runtime usage count underflow on subsequent calls.
- [High] The interrupt handler fan_alarm_irq_handler() unconditionally returns IRQ_NONE despite successfully handling the interrupt by scheduling work.
- [High] Missing cancellation of asynchronous work `alarm_work` on device unbind, leading to potential Use-After-Free.
- [High] Probe error paths increment the runtime PM `disable_depth` and underflow `usage_count` without a prior enable, permanently breaking runtime PM on reprobe.
--

commit 873147f80fe3dd0bae36a6073b5c5cdc81be1416
Author: Cong Nguyen <congnt264@gmail.com>

hwmon: (gpio-fan) take fan_data->lock in gpio_fan_shutdown()

set_fan_speed() writes the control GPIOs one bit at a time. Every
other caller locks around it; gpio_fan_shutdown() doesn't. If it races
a locked caller, the GPIO writes can interleave and leave the fan at a
speed neither caller asked for.

Link: https://lore.kernel.org/r/20260830152150.27F5F1F000E9@smtp.kernel.org

> diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
> index 084828e1e2817..10c91bc33b020 100644
> --- a/drivers/hwmon/gpio-fan.c
> +++ b/drivers/hwmon/gpio-fan.c
> @@ -606,8 +606,11 @@ static void gpio_fan_shutdown(struct platform_device *pdev)
>  {
>  	struct gpio_fan_data *fan_data = platform_get_drvdata(pdev);
>  
> -	if (fan_data->gpios)
> +	if (fan_data->gpios) {
> +		mutex_lock(&fan_data->lock);
>  		set_fan_speed(fan_data, 0);

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in
set_fan_speed() itself when it is called here.

If pm_runtime_put_sync() fails inside set_fan_speed(), it returns an error
without updating fan_data->speed_index:

drivers/hwmon/gpio-fan.c:set_fan_speed() {
    ...
	if (fan_data->speed_index > 0 && speed_index == 0) {
		int ret;

		ret = pm_runtime_put_sync(fan_data->dev);
		if (ret < 0 && ret != -ENOSYS)
			return ret;
	}

	fan_data->speed_index = speed_index;
    ...
}

Does this early return leave the driver's state machine desynchronized from
the PM runtime reference count? The next time set_fan_speed() is called to
turn off the fan, could it see speed_index is still > 0 and call
pm_runtime_put_sync() again, decrementing the count a second time and causing
a usage count underflow?

[Severity: High]
This is a pre-existing issue, but there appears to be another PM runtime
imbalance in the probe error paths involving gpio_fan_stop().

drivers/hwmon/gpio-fan.c:gpio_fan_stop() {
    ...
	set_fan_speed(data, 0);
	...
	pm_runtime_disable(fan_data->dev);
    ...
}

If a standard probe failure like EPROBE_DEFER occurs before
pm_runtime_enable() is called, would gpio_fan_stop() execute
set_fan_speed(..., 0) and underflow the usage count since a prior get was
never executed? 

Furthermore, would calling pm_runtime_disable() on this error path
incorrectly increment the disable_depth, permanently breaking runtime PM for
the device across reprobes?

> +		mutex_unlock(&fan_data->lock);
> +	}
>  }

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but I also noticed a potential
spurious interrupt issue in the alarm handler.

drivers/hwmon/gpio-fan.c:fan_alarm_irq_handler() {
	struct gpio_fan_data *fan_data = dev_id;

	schedule_work(&fan_data->alarm_work);

	return IRQ_NONE;
}

Since this unconditionally returns IRQ_NONE despite successfully handling the
interrupt by scheduling work, could this trigger the kernel's spurious
interrupt detector? If the alarm GPIO toggles enough times to reach the
threshold, might the kernel forcefully disable this shared IRQ line, breaking
this and potentially other devices sharing the line?

[Severity: High]
This is a pre-existing issue, but is there a missing teardown step for the
asynchronous work initialized in fan_alarm_init()?

drivers/hwmon/gpio-fan.c:fan_alarm_init() {
    ...
	INIT_WORK(&fan_data->alarm_work, fan_alarm_notify);
    ...
}

The asynchronous work is initialized here but never accompanied by a cleanup
or cancellation devm action. If the device is unbound via sysfs immediately
after the interrupt fires, could the devm framework free fan_data while
alarm_work is still pending or running on the background workqueue? Would
this lead to a use-after-free when fan_alarm_notify() dereferences the freed
fan_data?

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