linux-kernel - Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 21 Aug 2017 12:06:17 +0200
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Leo Yan <leo.yan@...aro.org>, Zhang Rui <rui.zhang@...el.com>
Cc:     edubezval@...il.com,
        "open list:THERMAL" <linux-pm@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        kong.kongxinwei@...ilicon.com
Subject: Re: [PATCH] thermal/drivers/hisi: Remove confusing error message

On 08/08/2017 15:29, Leo Yan wrote:
> On Tue, Aug 08, 2017 at 08:48:51PM +0800, Zhang Rui wrote:
> 
> [...]
> 
>>>>> @@ -352,10 +353,9 @@ static int hisi_thermal_probe(struct
>>>>> platform_device *pdev)
>>>>>  		ret = hisi_thermal_register_sensor(pdev, data,
>>>>>  						   &data-
>>>>>>
>>>>>> sensors[i], i);
>>>>>  		if (ret)
>>>>> -			dev_err(&pdev->dev,
>>>>> -				"failed to register thermal
>>>>> sensor:
>>>>> %d\n", ret);
>>>>> -		else
>>>>> -			hisi_thermal_toggle_sensor(&data-
>>>>>>
>>>>>> sensors[i], true);
>>>>> +			continue;
>>>>> +
>>>>> +		hisi_thermal_toggle_sensor(&data->sensors[i],
>>>>> true);
>>>>>  	}
>>>>>  
>>>>>  	return 0;
>>>> With these removed, is there any other information in dmesg that
>>>> suggests this failure?
>>> The problem is there are always failures showed in dmesg. The init
>>> function is based on the assumption there is HISI_MAX_SENSORS sensors
>>> which is not true for the hi6220 and that raises at boot time errors.
>>>
>>> Why HISI_MAX_SENSORS(=4) while there is only one on hi6220 AFAIK? and
>>> this driver is only used for hi6220 (now).
>>>
>> right, I think we should remove one error log, and then change the
>> HISI_MAX_SENSORS to reflect the reality instead.
>>
>> XinWei and Leo,
>> can you please help check this?
> 
> Sure.
> 
> Here I am a bit confusion and I think this is a common question for
> SoC thermal driver.
> 
> Hi6220 does has 4 thermal sensors, but we now only use one sensor of
> them (thermal sensor id 2) to bind with thermal zone and other three
> sensors are not bound to any thermal zone. So this is the reason the
> booting reports the failure.
> 
> I think changing HISI_MAX_SENSORS value cannot resolve this issue, due
> we are using thermal id 2. How about below change? We change to use
> warning for sensors without binding, and remove redundant log.

Hi Leo,

a cleanest solution would be either:

 - add the 3 missing thermal sensors in the DT and default to the id 2

or

 - remove all the code assuming 4 sensors and deal with the one unique
sensor

No ?


> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> index 9c3ce34..6d34980 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -260,8 +260,6 @@ static int hisi_thermal_register_sensor(struct platform_device *pdev,
>         if (IS_ERR(sensor->tzd)) {
>                 ret = PTR_ERR(sensor->tzd);
>                 sensor->tzd = NULL;
> -               dev_err(&pdev->dev, "failed to register sensor id %d: %d\n",
> -                       sensor->id, ret);
>                 return ret;
>         }
>  
> @@ -351,7 +349,10 @@ static int hisi_thermal_probe(struct platform_device *pdev)
>         for (i = 0; i < HISI_MAX_SENSORS; ++i) {
>                 ret = hisi_thermal_register_sensor(pdev, data,
>                                                    &data->sensors[i], i);
> -               if (ret)
> +               if (ret == -ENODEV)
> +                       dev_warn(&pdev->dev,
> +                                "thermal sensor %d has not bound\n", i);
> +               else if (ret)
>                         dev_err(&pdev->dev,
>                                 "failed to register thermal sensor: %d\n", ret);
>                 else
> 
> Thanks,
> Leo Yan
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog