Re: [PATCH v6 1/7] thermal/drivers/loongson2: Correct thermal sensor registration loop

[Daniel Lezcano <daniel.lezcano@oss.qualcomm.com>]

On 9/28/26 10:17, Binbin Zhou wrote:
> Hi Daniel:
> 
> Thanks for your review.
> 
> On Mon, Sep 28, 2026 at 3:15 PM Daniel Lezcano
> <daniel.lezcano@oss.qualcomm.com> wrote:
>>
>> On 9/28/26 06:44, Binbin Zhou wrote:
>>> The registration loop in loongson2_thermal_probe() incorrectly uses
>>> dev_err_probe() when the sensor is not present (-ENODEV). In that case,
>>> the driver should continue to the next sensor index rather than treating
>>> it as a fatal error.
>>>
>>> Fix this by correctly handling -ENODEV and only returning on other
>>> errors. Also add a final check to ensure at least one thermal zone was
>>> registered.
>>>
>>> Cc: stable@vger.kernel.org
>>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>>> Closes: https://sashiko.dev/#/patchset/cover.1783670011.git.zhoubinbin@loongson.cn?part=2
>>> Fixes: e7e3a7c35791 ("thermal/drivers/loongson-2: Add thermal management support")
>>> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
>>> ---
>>>    drivers/thermal/loongson2_thermal.c | 8 +++++---
>>>    1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/thermal/loongson2_thermal.c b/drivers/thermal/loongson2_thermal.c
>>> index 4d40fc706a53..99b7392fc68f 100644
>>> --- a/drivers/thermal/loongson2_thermal.c
>>> +++ b/drivers/thermal/loongson2_thermal.c
>>> @@ -160,16 +160,18 @@ static int loongson2_thermal_probe(struct platform_device *pdev)
>>>
>>>        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;
>>>
>>> -             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)
>>
>>
>> The [no]error paths are confusing and prone to error, eg: 'if there is
>> no error then check if there is an error', 'when exiting the loop
>> without registering any sensor, then we assume tzd is -ENODEV'.
>>
>> Please make a less cumbersome and implicit checks here. For example
>> count the number of sensors and then do the check against.
> 
> I thought about it, and you're right—I made the logic here too complicated.
> Let's use `sensor_count` as the counter. Is the following check logic clearer?
> 
>          u32 sensor_count = 0;
> 
>          for (unsigned int i = 0; i <= LOONGSON2_MAX_SENSOR_SEL_NUM; i++) {
>                  tzd = devm_thermal_of_zone_register(dev, i, data,
> data->chip_data->thermal_ops);
>                  if (IS_ERR(tzd)) {
>                          if (PTR_ERR(tzd) == -ENODEV)
>                                  continue;
> 
>                          return dev_err_probe(dev, PTR_ERR(tzd),
>                                               "failed to register sensor %d", i);
>                  }
> 
>                  sensor_count++;

If I'm not mistaken in the previous version, the loop exits at the first 
registered sensor

	if (!IS_ERR(tzd))
		break;

Here all the available sensors are registered

>                  break;
>          }
> 
>          if (!sensor_count)
>                  return dev_err_probe(dev, -ENODEV, "no thermal sensor
> registered");
> 
>