[PATCH v2] hwmon: (npcm750-pwm-fan): stop fan timer on device detach

[PATCH v2] hwmon: (npcm750-pwm-fan): stop fan timer on device detach

[Hongyan Xu]

When a fan tach channel is present, npcm7xx_pwm_fan_probe() starts
fan_timer. The timer callback polls tach state and rearms the timer, but
the driver has no remove callback or devm cleanup action to stop it. On
device detach, the devm-managed driver data and I/O mappings can be
released while the timer is still pending or running.

Register a devm cleanup action before starting the timer and shut the
timer down synchronously from that action.

This issue was found by a static analysis tool.

Fixes: f1fd4a4db777 ("hwmon: Add NPCM7xx PWM and Fan driver")
Cc: stable@vger.kernel.org
Signed-off-by: Hongyan Xu <getshell@seu.edu.cn>
---
v2:
- pass the fan timer directly to the cleanup action
- keep the timer expiration assignment in its original location

v1: https://lore.kernel.org/linux-hwmon/20260728075535.251-1-getshell@seu.edu.cn/

 drivers/hwmon/npcm750-pwm-fan.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/hwmon/npcm750-pwm-fan.c b/drivers/hwmon/npcm750-pwm-fan.c
index aea0b8659f5..6ff2a6bc06c 100644
--- a/drivers/hwmon/npcm750-pwm-fan.c
+++ b/drivers/hwmon/npcm750-pwm-fan.c
@@ -358,6 +358,11 @@ static void npcm7xx_fan_polling(struct timer_list *t)
 	add_timer(&data->fan_timer);
 }
 
+static void npcm7xx_fan_cleanup(void *timer)
+{
+	timer_shutdown_sync(timer);
+}
+
 static inline void npcm7xx_fan_compute(struct npcm7xx_pwm_fan_data *data,
 				       u8 fan, u8 cmp, u8 fan_id, u8 flag_int,
 				       u8 flag_mode, u8 flag_clear)
@@ -1020,6 +1025,12 @@ static int npcm7xx_pwm_fan_probe(struct platform_device *pdev)
 				msecs_to_jiffies(NPCM7XX_FAN_POLL_TIMER_200MS);
 			timer_setup(&data->fan_timer,
 				    npcm7xx_fan_polling, 0);
+			ret = devm_add_action_or_reset(dev,
+						       npcm7xx_fan_cleanup,
+						       &data->fan_timer);
+			if (ret)
+				return ret;
+
 			add_timer(&data->fan_timer);
 			break;
 		}
-- 
2.50.1.windows.1

Re: [PATCH v2] hwmon: (npcm750-pwm-fan): stop fan timer on device detach

[sashiko-bot]

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

Pre-existing issues:
- [High] Invalid VIRQ subtraction for module index calculation in ISR.
- [High] Concurrent calls to `npcm7xx_pwm_config_set` cause a read-modify-write race on the shared PWM Control Register.
- [High] Lockless reads of `fan_cnt` in `npcm7xx_read_fan` create a TOCTOU race window that can lead to a divide-by-zero kernel panic.
- [High] The timer callback `npcm7xx_fan_polling` modifies hardware interrupt and counter registers locklessly, racing with the hardirq handler.
- [Medium] Missing bounds checking for device tree array inputs 'reg' and 'fan-tach-ch' leading to out-of-bounds array writes.
--

--- Patch [1]: [PATCH v2] hwmon: (npcm750-pwm-fan): stop fan timer on device detach ---
commit 2c3b8a1752b70bfe681c67583f80cad522def5fb
Author: Hongyan Xu <getshell@seu.edu.cn>

hwmon: (npcm750-pwm-fan): stop fan timer on device detach

When a fan tach channel is present, npcm7xx_pwm_fan_probe() starts
fan_timer. On device detach, the devm-managed driver data and I/O mappings
can be released while the timer is still pending or running. This patch
registers a devm cleanup action to shut the timer down synchronously.

> diff --git a/drivers/hwmon/npcm750-pwm-fan.c b/drivers/hwmon/npcm750-pwm-fan.c
> index aea0b8659f5f6..6ff2a6bc06c14 100644
> --- a/drivers/hwmon/npcm750-pwm-fan.c
> +++ b/drivers/hwmon/npcm750-pwm-fan.c
> @@ -358,6 +358,11 @@ static void npcm7xx_fan_polling(struct timer_list *t)
>  	add_timer(&data->fan_timer);
>  }

[Severity: High]
This is a pre-existing issue, but does this timer callback modify hardware
interrupt and counter registers locklessly, racing with the hardirq handler?

Looking at npcm7xx_fan_polling():

	for (i = data->fan_select; i < NPCM7XX_FAN_MAX_MODULE;
	      i = i + 4) {
		/* clear the flag and reset the counter (TCNT) */
		iowrite8(NPCM7XX_FAN_TICLR_CLEAR_ALL,
			 NPCM7XX_FAN_REG_TICLR(data->fan_base, i));

Here, the driver writes to NPCM7XX_FAN_REG_TICLR and modifies
NPCM7XX_FAN_REG_TCNT1 without holding the fan_lock.

Concurrently, the interrupt handler npcm7xx_fan_isr() (executing in hardirq
context) acquires fan_lock to read and selectively clear interrupt flags in
the same registers.

Could this unprotected concurrent hardware access erroneously clear pending
interrupts before the interrupt handler processes them?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the interrupt handler calculate an
out-of-bounds module index?

Looking at npcm7xx_fan_isr():

	module = irq - data->fan_irq[0];
	spin_lock_irqsave(&data->fan_lock[module], flags);

Linux virtual irq numbers are dynamically mapped and not guaranteed to be
monotonically contiguous. If the allocated irqs are not strictly
sequential, irq - data->fan_irq[0] could yield an arbitrary (and possibly
negative) index.

Can this lead to out-of-bounds memory accesses on data->fan_lock[module] and
potential memory corruption?

[Severity: Medium]
This is a pre-existing issue, but does the probe path lack bounds checking
for device tree array inputs?

Looking at npcm7xx_en_pwm_fan():

	data->pwm_present[pwm_port] = true;
...
	index = fan_ch[ch];
	data->fan_present[index] = true;

Values parsed from the device tree such as pwm_port (from 'reg') and index
(from 'fan-tach-ch') are used directly as array indices for bounded arrays
(data->pwm_present, data->cdev, data->fan_dev).

If the values exceed NPCM7XX_PWM_MAX_CHN_NUM (12) or
NPCM7XX_FAN_MAX_CHN_NUM, could a malformed device tree trigger out-of-bounds
array writes during probe?

[Severity: High]
This is a pre-existing issue, but can concurrent calls to
npcm7xx_pwm_config_set() cause a read-modify-write race on the shared PWM
control register?

Looking at npcm7xx_pwm_config_set():

	tmp_buf = ioread32(NPCM7XX_PWM_REG_CR(data->pwm_base, module));
	...
	iowrite32(tmp_buf, NPCM7XX_PWM_REG_CR(data->pwm_base, module));

This performs an unprotected read-modify-write on the shared PWM module
control register (NPCM7XX_PWM_REG_CR).

This function is called concurrently by sysfs (via hwmon core) and the
thermal subsystem (via the directly registered cooling device
npcm7xx_pwm_cool_ops). Because the thermal cooling device is registered
directly to the thermal core rather than through hwmon, hwmon's
serialization does not apply.

Could this race without a lock, leading to lost register updates and
incorrect PWM channel states?

[Severity: High]
This is a pre-existing issue, but do lockless reads of fan_cnt in
npcm7xx_read_fan() create a time-of-check to time-of-use race window?

Looking at npcm7xx_read_fan():

	if (data->fan_dev[channel].fan_cnt <= 0)
		return data->fan_dev[channel].fan_cnt;

	*val = 9000000 * 60 / data->fan_dev[channel].fan_cnt;

data->fan_dev[channel].fan_cnt is read without READ_ONCE() or a lock to
verify it is > 0, and then read a second time in the denominator for a speed
calculation.

Concurrently, npcm7xx_fan_start_capture() (called from the timer or
interrupt handler) can locklessly update fan_cnt to 0 upon a fan stall.

If the value changes to 0 between the two reads, could the division by zero
crash the system?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729100116.790-1-getshell@seu.edu.cn?part=1

Re: [PATCH v2] hwmon: (npcm750-pwm-fan): stop fan timer on device detach

[Guenter Roeck]

On Wed, Jul 29, 2026 at 06:01:16PM +0800, Hongyan Xu wrote:
> When a fan tach channel is present, npcm7xx_pwm_fan_probe() starts
> fan_timer. The timer callback polls tach state and rearms the timer, but
> the driver has no remove callback or devm cleanup action to stop it. On
> device detach, the devm-managed driver data and I/O mappings can be
> released while the timer is still pending or running.
> 
> Register a devm cleanup action before starting the timer and shut the
> timer down synchronously from that action.
> 
> This issue was found by a static analysis tool.
> 
> Fixes: f1fd4a4db777 ("hwmon: Add NPCM7xx PWM and Fan driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hongyan Xu <getshell@seu.edu.cn>

Applied.

Thanks,
Guenter