Re: [PATCH v4 1/5] thermal/drivers/loongson2: Correct thermal sensor registration loop

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

Pre-existing issues:
- [High] The driver allows registering any sensor ID from 0 to 3 but unconditionally configures hardware registers for sensor 0.
- [High] The device's hardware interrupt is left enabled when probe fails or the driver is unbound.
- [High] Potential NULL pointer dereference if `device_get_match_data()` returns NULL.
--

--- Patch [1]: [PATCH v4 1/5] thermal/drivers/loongson2: Correct thermal sensor registration loop ---
commit 04ee4726d4d6013e77a52a26bf79466c2069029a
Author: Binbin Zhou <[email protected]>

thermal/drivers/loongson2: Correct thermal sensor registration loop

This commit fixes the registration loop in loongson2_thermal_probe() by
correctly handling -ENODEV and only aborting on fatal errors. It also
ensures at least one thermal zone is registered before proceeding.

> diff --git a/drivers/thermal/loongson2_thermal.c b/drivers/thermal/loongson2_thermal.c
> index ea4dd2fb1f473..5f47fa51fb907 100644
> --- a/drivers/thermal/loongson2_thermal.c
> +++ b/drivers/thermal/loongson2_thermal.c
> @@ -161,16 +161,18 @@ static int loongson2_thermal_probe(struct platform_device *pdev)

[Severity: High]
This is a pre-existing issue, but if device_get_match_data() returns NULL
(for example, via manual sysfs binding), can this cause a NULL pointer
dereference earlier in this function?

drivers/thermal/loongson2_thermal.c:loongson2_thermal_probe() {
	...
	data->chip_data = device_get_match_data(dev);
	...
	if (data->chip_data->flags & LS2K2000_THSENS_OUT_FLAG) {
	...
}

>  
>  	for (i = 0; i <= LOONGSON2_MAX_SENSOR_SEL_NUM; i++) {
>  		tzd = devm_thermal_of_zone_register(dev, i, data, thermal_ops);

[Severity: High]
This isn't a bug introduced by this patch, but does this leave the hardware
interrupt enabled if probe fails later or the driver is unbound?

During devm_thermal_of_zone_register(), the thermal core registers the zone
and invokes tz->ops->set_trips(). This calls
loongson2_thermal_set(..., enable=true), which enables the hardware interrupt.

However, there appears to be no corresponding remove function in
loongson2_thermal_driver, nor a devm_add_action_or_reset() callback to
disable the interrupt if devm_request_threaded_irq() fails or the driver is
unbound.

[Severity: High]
This is also a pre-existing issue, but if the device tree specifies a thermal
zone for a sensor ID other than 0, will the hardware configuration match the
intended sensor?

The probe loop registers the zone for sensor index i and breaks, but
loongson2_set_ctrl_regs() uses a statically defined thermal_sensor_sel:

drivers/thermal/loongson2_thermal.c:loongson2_set_ctrl_regs() {
	int reg_off = data->chip_data->thermal_sensor_sel * 2;
	...
}

It seems this calculates the register offset ignoring the actually registered
sensor ID, potentially resulting in reading from and writing limits to the
hardware registers of sensor 0 instead.

>  		if (!IS_ERR(tzd))
>  			break;
>  
> -		if (PTR_ERR(tzd) != -ENODEV)
> +		if (PTR_ERR(tzd) == -ENODEV)
>  			continue;
>  
> -		return dev_err_probe(dev, PTR_ERR(tzd), "failed to register");
> +		return dev_err_probe(dev, PTR_ERR(tzd), "failed to register sensor %d\n", i);
>  	}
>  
> +	if (IS_ERR(tzd))
> +		return dev_err_probe(dev, -ENODEV, "No thermal sensor registered\n");
> +
>  	ret = devm_request_threaded_irq(dev, irq, NULL, loongson2_thermal_irq_thread,
>  					IRQF_ONESHOT, "loongson2_thermal", tzd);
>  	if (ret < 0)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1