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:   Fri, 11 Aug 2017 11:14:50 +0800
From:   Zhang Rui <rui.zhang@...el.com>
To:     Leo Yan <leo.yan@...aro.org>
Cc:     Daniel Lezcano <daniel.lezcano@...aro.org>, 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 Tue, 2017-08-08 at 21:29 +0800, 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.
> 
Now we will get three "thermal sensor %d has not bound" messages for
every normal probe, and an extra "failed to register thermal sensor:"
for a real failure probe?

If that's the case, as we are not using the sensors on purpose, why not
keep silence for -ENODEV?

thanks,
rui

> 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