Re: [PATCH v2 1/4] 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] NULL Pointer Dereference via sysfs bind
- [Medium] Hardware Configuration Mismatch
- [Medium] Missing Teardown / Resource Leak
--

--- Patch [1]: [PATCH v2 1/4] thermal/drivers/loongson2: Correct thermal sensor registration loop ---
commit 381cd9ed04934716b53c8748d5c522210d6b54ff
Author: Binbin Zhou <[email protected]>

thermal/drivers/loongson2: Correct thermal sensor registration loop

The registration loop in loongson2_thermal_probe() incorrectly uses
dev_err_probe() when the sensor is not present (-ENODEV). Fix this by
correctly handling -ENODEV and only returning on other errors.

Closes: https://sashiko.dev/#/patchset/[email protected]?part=2

> diff --git a/drivers/thermal/loongson2_thermal.c b/drivers/thermal/loongson2_thermal.c
> index ea4dd2fb1f473..d7d221c3136d4 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 does device_get_match_data() return NULL
when binding the driver via sysfs to a platform device that lacks OF match
data?

In loongson2_thermal_probe(), data->chip_data is assigned from
device_get_match_data(dev) and then immediately dereferenced:

    data->chip_data = device_get_match_data(dev);
    ...
    if (data->chip_data->flags & LS2K2000_THSENS_OUT_FLAG) {

Could this cause a NULL pointer dereference crash if bound by name via sysfs?

>  
>  	for (i = 0; i <= LOONGSON2_MAX_SENSOR_SEL_NUM; i++) {
>  		tzd = devm_thermal_of_zone_register(dev, i, data, thermal_ops);
> -
>  		if (!IS_ERR(tzd))
>  			break;

[Severity: Medium]
This is a pre-existing issue, but does this code properly save the
discovered sensor index?

If the device tree specifies a non-zero sensor index, the actual sensor
index is discovered here but then discarded instead of being saved to
the instance data.

Later in loongson2_set_ctrl_regs(), the hardware register offset is
calculated using the hardcoded static match data rather than the index
actually discovered:

    int reg_off  = data->chip_data->thermal_sensor_sel * 2;

Could this cause the driver to report temperatures or configure thresholds
for the wrong hardware sensor (sensor 0) if the device tree specifies a
different index?

[ ... ]

> +	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);

[Severity: Medium]
This is a pre-existing issue, but is there a missing teardown for the
hardware interrupt state?

The probe function relies solely on devres for cleanup, but it does not
register a devm action to revert the hardware interrupt state initialized
by the thermal core:

    loongson2_thermal_set(data, 0, 0, false);

If the driver is manually unbound via sysfs or the module is unloaded, could
this leave the device actively asserting its interrupt line?

>  	if (ret < 0)

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