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  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:	Thu, 07 Aug 2014 18:29:23 +0200
From:	Goffredo Baroncelli <kreijack@...ind.it>
To:	Jean Delvare <jdelvare@...e.de>,
	Goffredo Baroncelli <kreijack@...il.com>
CC:	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/5] Add the "verbose" module option.

On 08/07/2014 10:52 AM, Jean Delvare wrote:
> Le Wednesday 06 August 2014 à 21:05 +0000, Goffredo Baroncelli a écrit :
>> Add a "verbose" option to control the message in the kernel log
>> verbose = 0   no message
>> verbose = 1   log only the fan speed changes
>> verbose = 2   log the fan speed changes and the temperature changes
>>
>> Signed-off-by: Goffredo Baroncelli <kreijack@...ind.it>
>> ---
>>  drivers/macintosh/therm_windtunnel.c | 39 +++++++++++++++++++++++-------------
>>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> Looks overall good, minor comments inline below.
> 
>> diff --git a/drivers/macintosh/therm_windtunnel.c b/drivers/macintosh/therm_windtunnel.c
>> index 1e50455..7c512db 100644
>> --- a/drivers/macintosh/therm_windtunnel.c
>> +++ b/drivers/macintosh/therm_windtunnel.c
>> @@ -44,7 +44,9 @@
>>  #include <asm/sections.h>
>>  #include <asm/macio.h>
>>  
>> -#define LOG_TEMP		0			/* continuously log temperature */
>> +static int verbose = 1;
>> +module_param(verbose, int, 0644);
>> +MODULE_PARM_DESC(verbose, "Verbosity level: 0=silent, 1=log the fan tuning, 2=log the temperature");
>>  
>>  static struct {
>>  	volatile int		running;
>> @@ -157,10 +159,6 @@ tune_fan( int fan_setting )
>>  	/* write_reg( x.fan, 0x24, val, 1 ); */
>>  	write_reg( x.fan, 0x25, val, 1 );
>>  	write_reg( x.fan, 0x20, 0, 1 );
>> -	print_temp("CPU-temp: ", x.temp );
>> -	if( x.casetemp )
>> -		print_temp(", Case: ", x.casetemp );
>> -	printk(",  Fan: %d (tuned %+d)\n", 11-fan_setting, x.fan_level-fan_setting );
>>  
>>  	x.fan_level = fan_setting;
>>  }
>> @@ -168,7 +166,7 @@ tune_fan( int fan_setting )
>>  static void
>>  poll_temp( void )
>>  {
>> -	int temp, i, level, casetemp;
>> +	int temp, i, level, casetemp, tempchanged;
>>  
>>  	temp = read_reg( x.thermostat, 0, 2 );
>>  
>> @@ -179,14 +177,6 @@ poll_temp( void )
>>  	casetemp = read_reg(x.fan, 0x0b, 1) << 8;
>>  	casetemp |= (read_reg(x.fan, 0x06, 1) & 0x7) << 5;
>>  
>> -	if( LOG_TEMP && x.temp != temp ) {
>> -		print_temp("CPU-temp: ", temp );
>> -		print_temp(", Case: ", casetemp );
>> -		printk(",  Fan: %d\n", 11-x.fan_level );
>> -	}
>> -	x.temp = temp;
>> -	x.casetemp = casetemp;
>> -
>>  	level = -1;
>>  	for( i=0; (temp & 0xffff) > fan_table[i].temp ; i++ )
>>  		;
>> @@ -200,6 +190,27 @@ poll_temp( void )
>>  		level = fan_table[i].fan_up_setting;
>>  	x.upind = i;
>>  
>> +	/*
>> +	 * if verbose >0 log each fan tuning
>> +	 * if verbose >1 log each cpu temperature change
> 
> Maybe this is just me but I think "verbose >= 1" and "verbose >= 2"
> would be easier to understand. Same in the code below.

correct, I am planning another revision due to Guenter concern. So 
I will correct it.
> 
>> +	 */
>> +	tempchanged = x.temp != temp || x.casetemp != casetemp;
>> +	if ((verbose > 1 && tempchanged) ||
>> +	    (verbose > 0 && level >= 0)) {
>> +		printk(KERN_INFO);
>> +		print_temp("CPU-temp: ", temp);
> 
> This can be written more efficiently as a single statement:
> 
> 		print_temp(KERN_INFO "CPU-temp: ", temp);

I suppose that KERN_* has to be in the beginning of the line. 

Because a single line is composed by several prink, KERN_INFO has 
to be only in the first printk. To me it seems more polite to have
one printk for the level, and the others (there are more than one) 
for the message parts.

> 
>> +		if (casetemp)
>> +			print_temp(", Case: ", casetemp);
>> +		if (level >= 0)
>> +			printk(", Fan: %d (tuned %+d)\n", 11-level,
>> +				x.fan_level-level);
>> +		else
>> +			printk(", Fan: %d\n", 11-x.fan_level);
>> +	}
>> +
>> +	x.temp = temp;
>> +	x.casetemp = casetemp;
>> +
>>  	if( level >= 0 )
>>  		tune_fan( level );
>>  }
> 
> Reviewed-by: Jean Delvare <jdelvare@...e.de>
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists