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 uncond=
itionally configures hardware registers for sensor 0.
- [High] The device's hardware interrupt is left enabled when probe fails o=
r the driver is unbound.
- [High] Potential NULL pointer dereference if `device_get_match_data()` re=
turns NULL.
--

--- Patch [1]: [PATCH v4 1/5] thermal/drivers/loongson2: Correct thermal se=
nsor 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/loongs=
on2_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 =3D device_get_match_data(dev);
	...
	if (data->chip_data->flags & LS2K2000_THSENS_OUT_FLAG) {
	...
}

> =20
>  	for (i =3D 0; i <=3D LOONGSON2_MAX_SENSOR_SEL_NUM; i++) {
>  		tzd =3D 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=3Dtrue), which enables the hardware inter=
rupt.

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 therm=
al
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 =3D data->chip_data->thermal_sensor_sel * 2;
	...
}

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

>  		if (!IS_ERR(tzd))
>  			break;
> =20
> -		if (PTR_ERR(tzd) !=3D -ENODEV)
> +		if (PTR_ERR(tzd) =3D=3D -ENODEV)
>  			continue;
> =20
> -		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);
>  	}
> =20
> +	if (IS_ERR(tzd))
> +		return dev_err_probe(dev, -ENODEV, "No thermal sensor registered\n");
> +
>  	ret =3D devm_request_threaded_irq(dev, irq, NULL, loongson2_thermal_irq=
_thread,
>  					IRQF_ONESHOT, "loongson2_thermal", tzd);
>  	if (ret < 0)

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/cover.1785829933.gi=
[email protected]?part=3D1