linux-kernel - Re: [PATCH] i8k: Add support for temperature sensor labels

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:	Sat, 29 Nov 2014 17:30:28 +0100
From:	Pali Rohár <pali.rohar@...il.com>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	Arnd Bergmann <arnd@...db.de>,
	"Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
	Steven Honeyman <stevenhoneyman@...il.com>,
	linux-kernel@...r.kernel.org,
	Gabriele Mazzotta <gabriele.mzt@...il.com>
Subject: Re: [PATCH] i8k: Add support for temperature sensor labels

On Saturday 29 November 2014 17:24:08 Guenter Roeck wrote:
> On 11/29/2014 08:04 AM, Pali Rohár wrote:
> > +static bool __init i8k_check_temp(int sensor)
> > +{
> > +	int err;
> > +
> > +	/*
> > +	 * Check if temperature sensor type is valid.
> > +	 *
> > +	 * If it is valid then sensor should work. But some
> > sensors are not +	 * available at any time. E.g GPU sensor
> > on Optimus/PowerExpress/Enduro +	 * card does not work (or
> > return bogus value) when card is turned off. +	 * So this
> > function should not fail in this case. +	 */
> > +	err = i8k_get_temp_type(sensor);
> > +	if (err >= 0)
> > +		return true;
> > +
> 
> Are you sure this function is provided for all systems ?
> I am a bit concerned that we may wrongly disable sensors this
> way, especially on older systems.
> 

I do not know if that function is provided on all systems. But 
this code does not disable sensors. If function fail, then we 
fallback to temperature read down. Return true means that we 
enable sensor.

> It might be safer to create another set of flags and enable
> the labels only if the sensor is known to exist and if
> reading its type (label) works.
> 
> > +	/*
> > +	 * Check if temperatrue reading does not fail.
> > +	 *
> 
> temperature
> 
> > +	 * Sometimes detection of temperature sensor type does not
> > work but +	 * reading temperature working fine. Sometimes
> > temperature value is too +	 * high and i8k_get_temp()
> > returns -ERANGE. But there is no reason to +	 * ignore
> > these temperature sensors.
> > +	 */
> > +	err = i8k_get_temp(sensor);
> > +	if (err >= 0 || err == -ERANGE)
> > +		return true;
> > +
> 
> Please simplify to
> 	return err >= 0 || err == -ERANGE;
> 

I'm using style:

check_function_1();
if (not_failed)
	return true;

check_function_2();
if (not_failed)
	return true;

return false; // disable sensor

So I think it is better to have same style for both checks...

> > +	return false;
> > +}
> > +
> > 
> >   static int __init i8k_init_hwmon(void)
> >   {
> >   
> >   	int err;
> > 
> > @@ -613,18 +684,13 @@ static int __init i8k_init_hwmon(void)
> > 
> >   	i8k_hwmon_flags = 0;
> >   	
> >   	/* CPU temperature attributes, if temperature reading is
> >   	OK */
> > 
> > -	err = i8k_get_temp(0);
> > -	if (err >= 0 || err == -ERANGE)
> > +	if (i8k_check_temp(0))
> 
> The introduction of i8k_check_temp() should be a separate
> patch.
> 
> Thanks,
> Guenter
> 

Ok.

> >   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1;
> > 
> > -	/* check for additional temperature sensors */
> > -	err = i8k_get_temp(1);
> > -	if (err >= 0 || err == -ERANGE)
> > +	if (i8k_check_temp(1))
> > 
> >   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2;
> > 
> > -	err = i8k_get_temp(2);
> > -	if (err >= 0 || err == -ERANGE)
> > +	if (i8k_check_temp(2))
> > 
> >   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3;
> > 
> > -	err = i8k_get_temp(3);
> > -	if (err >= 0 || err == -ERANGE)
> > +	if (i8k_check_temp(3))
> > 
> >   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
> >   	
> >   	/* Left fan attributes, if left fan is present */

-- 
Pali Rohár
pali.rohar@...il.com

Download attachment "signature.asc " of type "application/pgp-signature" (199 bytes)