[lm-sensors]

[lm-sensors]

[wore]

I don't know about 2.6 kernel, but I wrote for 2.4 kernel.
Currently working on my 2.4 kernel box with no trouble.
All you have to do is port to 2.6 kernel. :-)

My mail and code(attached) are there.
http://lists.lm-sensors.org/pipermail/lm-sensors/2005-April/011709.html

Regards,
wore

[lm-sensors]

[Mauro Carvalho Chehab]

Greg,

	Please don't send this patch to Andrew or mainstream. I'll apply it on
my tree and I will send it at the end my patchset series, for not
breaking these.

	I have several patches that are including newer IDs on
linux/media/i2c-id.h and I'm just preparing a patch, on V4L tree, to
remove this file from kernel.

	If this is applied, it may break dozens of patches I've already
prepared (at
http://wwwlinuxtv.org/download/video4linux/patches/2.6.14-rc5)

	Maybe it would be wiser if patches that touches drivers/media devices
maintained on V4L go first to me, and I send to Andrew. This way, it
would help us not to redo an entire patchset for fixing broken stuff
because they would be included on a wrong order.

Cheers,
Mauro.


Em Qua, 2005-10-26 ?s 21:17 +0200, Jean Delvare escreveu:
> Content-Disposition: inline; filename=i2c-id-clean-redefinitions-2.patch
> 
> Second round of i2c IDs redefinition cleanup.
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> CC: Mauro Carvalho Chehab <mchehab@brturbo.com.br>
> CC: John Klar <linpvr@projectplasma.com>
> 
> ---
>  drivers/media/video/bt832.c             |    1 -
>  drivers/media/video/msp3400.c           |    1 -
>  drivers/media/video/saa6588.c           |    2 --
>  drivers/media/video/saa7134/saa6752hs.c |    2 --
>  drivers/media/video/saa7134/saa7134.h   |    1 -
>  drivers/media/video/tda7432.c           |    1 -
>  drivers/media/video/tda9875.c           |    1 -
>  drivers/media/video/tda9887.c           |    1 -
>  drivers/media/video/tvaudio.c           |    3 +--
>  drivers/media/video/tveeprom.c          |    4 ----
>  include/linux/i2c-id.h                  |    3 +++
>  include/media/id.h                      |   35 -------------------------------
>  12 files changed, 4 insertions(+), 51 deletions(-)
> 
> --- linux-2.6.14-rc5.orig/drivers/media/video/tvaudio.c	2005-10-23 17:42:05.000000000 +0200
> +++ linux-2.6.14-rc5/drivers/media/video/tvaudio.c	2005-10-24 19:43:05.000000000 +0200
> @@ -31,7 +31,6 @@
>  #include <linux/smp_lock.h>
>  
>  #include <media/audiochip.h>
> -#include <media/id.h>
>  
>  #include "tvaudio.h"
>  
> @@ -1438,7 +1437,7 @@
>  	},
>  	{
>  		.name       = "pic16c54 (PV951)",
> -		.id         = I2C_DRIVERID_PIC16C54_PV951,
> +		.id         = I2C_DRIVERID_PIC16C54_PV9,
>  		.insmodopt  = &pic16c54,
>  		.addr_lo    = I2C_PIC16C54 >> 1,
>  		.addr_hi    = I2C_PIC16C54>> 1,
> --- linux-2.6.14-rc5.orig/include/linux/i2c-id.h	2005-10-24 19:38:22.000000000 +0200
> +++ linux-2.6.14-rc5/include/linux/i2c-id.h	2005-10-24 19:43:05.000000000 +0200
> @@ -99,6 +99,9 @@
>  #define I2C_DRIVERID_MAX6900	63	/* MAX6900 real-time clock	*/
>  #define I2C_DRIVERID_SAA7114H	64	/* video decoder		*/
>  #define I2C_DRIVERID_DS1374	65	/* DS1374 real time clock	*/
> +#define I2C_DRIVERID_TDA9874	66	/* TV sound decoder		*/
> +#define I2C_DRIVERID_SAA6752HS	67	/* MPEG2 encoder		*/
> +#define I2C_DRIVERID_TVEEPROM	68	/* TV EEPROM			*/
>  
> 
>  #define I2C_DRIVERID_EXP0	0xF0	/* experimental use id's	*/
> --- linux-2.6.14-rc5.orig/include/media/id.h	2005-09-13 21:21:25.000000000 +0200
> +++ /dev/null	1970-01-01 00:00:00.000000000 +0000
> @@ -1,35 +0,0 @@
> -/*
> - */
> -
> -/* FIXME: this temporarely, until these are included in linux/i2c-id.h */
> -
> -/* drivers */
> -#ifndef  I2C_DRIVERID_TVMIXER
> -# define I2C_DRIVERID_TVMIXER I2C_DRIVERID_EXP0
> -#endif
> -#ifndef  I2C_DRIVERID_TVAUDIO
> -# define I2C_DRIVERID_TVAUDIO I2C_DRIVERID_EXP1
> -#endif
> -
> -/* chips */
> -#ifndef  I2C_DRIVERID_DPL3518
> -# define I2C_DRIVERID_DPL3518 I2C_DRIVERID_EXP2
> -#endif
> -#ifndef  I2C_DRIVERID_TDA9873
> -# define I2C_DRIVERID_TDA9873 I2C_DRIVERID_EXP3
> -#endif
> -#ifndef  I2C_DRIVERID_TDA9875
> -# define I2C_DRIVERID_TDA9875 I2C_DRIVERID_EXP0+4
> -#endif
> -#ifndef  I2C_DRIVERID_PIC16C54_PV951
> -# define I2C_DRIVERID_PIC16C54_PV951 I2C_DRIVERID_EXP0+5
> -#endif
> -#ifndef  I2C_DRIVERID_TDA7432
> -# define I2C_DRIVERID_TDA7432 I2C_DRIVERID_EXP0+6
> -#endif
> -#ifndef  I2C_DRIVERID_TDA9874
> -# define I2C_DRIVERID_TDA9874 I2C_DRIVERID_EXP0+7
> -#endif
> -#ifndef  I2C_DRIVERID_SAA6752HS
> -# define I2C_DRIVERID_SAA6752HS I2C_DRIVERID_EXP0+8
> -#endif
> --- linux-2.6.14-rc5.orig/drivers/media/video/tveeprom.c	2005-10-23 17:42:05.000000000 +0200
> +++ linux-2.6.14-rc5/drivers/media/video/tveeprom.c	2005-10-24 19:43:05.000000000 +0200
> @@ -590,10 +590,6 @@
>  /* run, just call the exported tveeprom_* directly, there is no point in   */
>  /* using the indirect way via i2c_driver->command()                        */
>  
> -#ifndef I2C_DRIVERID_TVEEPROM
> -# define I2C_DRIVERID_TVEEPROM I2C_DRIVERID_EXP2
> -#endif
> -
>  static unsigned short normal_i2c[] = {
>  	0xa0 >> 1,
>  	I2C_CLIENT_END,
> --- linux-2.6.14-rc5.orig/drivers/media/video/bt832.c	2005-10-23 17:42:05.000000000 +0200
> +++ linux-2.6.14-rc5/drivers/media/video/bt832.c	2005-10-24 19:43:05.000000000 +0200
> @@ -32,7 +32,6 @@
>  #include <linux/slab.h>
>  
>  #include <media/audiochip.h>
> -#include <media/id.h>
>  #include "bttv.h"
>  #include "bt832.h"
>  
> --- linux-2.6.14-rc5.orig/drivers/media/video/msp3400.c	2005-10-23 17:42:05.000000000 +0200
> +++ linux-2.6.14-rc5/drivers/media/video/msp3400.c	2005-10-24 19:43:05.000000000 +0200
> @@ -54,7 +54,6 @@
>  #include <asm/pgtable.h>
>  
>  #include <media/audiochip.h>
> -#include <media/id.h>
>  #include "msp3400.h"
>  
>  #define OPMODE_AUTO    -1
> --- linux-2.6.14-rc5.orig/drivers/media/video/saa6588.c	2005-10-23 17:42:05.000000000 +0200
> +++ linux-2.6.14-rc5/drivers/media/video/saa6588.c	2005-10-24 19:43:05.000000000 +0200
> @@ -31,8 +31,6 @@
>  #include <linux/wait.h>
>  #include <asm/uaccess.h>
>  
> -#include <media/id.h>
> -
>  #include "rds.h"
>  
>  /* Addresses to scan */
> --- linux-2.6.14-rc5.orig/drivers/media/video/saa7134/saa6752hs.c	2005-10-23 17:42:05.000000000 +0200
> +++ linux-2.6.14-rc5/drivers/media/video/saa7134/saa6752hs.c	2005-10-24 19:43:05.000000000 +0200
> @@ -13,8 +13,6 @@
>  #include <linux/init.h>
>  #include <linux/crc32.h>
>  
> -#include <media/id.h>
> -
>  #define MPEG_VIDEO_TARGET_BITRATE_MAX  27000
>  #define MPEG_VIDEO_MAX_BITRATE_MAX     27000
>  #define MPEG_TOTAL_TARGET_BITRATE_MAX  27000
> --- linux-2.6.14-rc5.orig/drivers/media/video/saa7134/saa7134.h	2005-09-13 21:21:09.000000000 +0200
> +++ linux-2.6.14-rc5/drivers/media/video/saa7134/saa7134.h	2005-10-24 19:43:05.000000000 +0200
> @@ -32,7 +32,6 @@
>  
>  #include <media/tuner.h>
>  #include <media/audiochip.h>
> -#include <media/id.h>
>  #include <media/ir-common.h>
>  #include <media/video-buf.h>
>  #include <media/video-buf-dvb.h>
> --- linux-2.6.14-rc5.orig/drivers/media/video/tda7432.c	2005-10-23 17:42:05.000000000 +0200
> +++ linux-2.6.14-rc5/drivers/media/video/tda7432.c	2005-10-24 19:43:05.000000000 +0200
> @@ -50,7 +50,6 @@
>  
>  #include "bttv.h"
>  #include <media/audiochip.h>
> -#include <media/id.h>
>  
>  #ifndef VIDEO_AUDIO_BALANCE
>  # define VIDEO_AUDIO_BALANCE 32
> --- linux-2.6.14-rc5.orig/drivers/media/video/tda9875.c	2005-10-23 17:42:05.000000000 +0200
> +++ linux-2.6.14-rc5/drivers/media/video/tda9875.c	2005-10-24 19:43:05.000000000 +0200
> @@ -32,7 +32,6 @@
>  
>  #include "bttv.h"
>  #include <media/audiochip.h>
> -#include <media/id.h>
>  
>  static int debug; /* insmod parameter */
>  module_param(debug, int, S_IRUGO | S_IWUSR);
> --- linux-2.6.14-rc5.orig/drivers/media/video/tda9887.c	2005-10-23 17:42:05.000000000 +0200
> +++ linux-2.6.14-rc5/drivers/media/video/tda9887.c	2005-10-24 19:43:05.000000000 +0200
> @@ -11,7 +11,6 @@
>  
>  #include <media/audiochip.h>
>  #include <media/tuner.h>
> -#include <media/id.h>
>  
>  /* Chips:
>     TDA9885 (PAL, NTSC)
> 
Cheers, 
Mauro.

[lm-sensors]

[Greg KH]

On Wed, Oct 26, 2005 at 06:15:44PM -0200, Mauro Carvalho Chehab wrote:
> Greg,
> 
> 	Please don't send this patch to Andrew or mainstream. I'll apply it on
> my tree and I will send it at the end my patchset series, for not
> breaking these.
> 
> 	I have several patches that are including newer IDs on
> linux/media/i2c-id.h and I'm just preparing a patch, on V4L tree, to
> remove this file from kernel.

But we are removing this file, as it's just not needed.  Why would you
add more ids to it?

thanks,

greg k-h

[lm-sensors]

[Jean Delvare]

Hi Mauro,

> Please don't send this patch to Andrew or mainstream. I'll apply it on
> my tree and I will send it at the end my patchset series, for not
> breaking these.

Fine with me, as long as it ultimately gets done.

My motivation for this cleanup is that I plan to work soon on a new way
for drivers to instantiate i2c "clients". This new way will most
certainly require i2c IDs, so I need to have an accurate view of how
these are currently used. Having them centralized, as they were
supposed to be in the first place, should make my analysis easier, and
should reduce the risk to get things wrong.

Note that the new client instantiation method should suit the
media/video drivers needs much better than the old "general probing"
way did (which, with the RTC drivers, is actually the reason why I want
to implement this). I'll make sure to CC you and the v4l list on my
proposal to get your feedback.

> I have several patches that are including newer IDs on
> linux/media/i2c-id.h and I'm just preparing a patch, on V4L tree, to
> remove this file from kernel.

I guess we must read this as: adding news IDs to linux/i2c-id.h, and
removing the media/id.h file?

> If this is applied, it may break dozens of patches I've already
> prepared (at
> http://www.linuxtv.org/download/video4linux/patches/2.6.14-rc5)
> 
> Maybe it would be wiser if patches that touches drivers/media devices
> maintained on V4L go first to me, and I send to Andrew. This way, it
> would help us not to redo an entire patchset for fixing broken stuff
> because they would be included on a wrong order.

I did send this patch to you and the v4l list two days ago, stating
that I would send it to Greg quickly. I did not get any answer, so I
did. When a patch of mine gets in your way, just say so and I'll delay
it or leave it to you altogether.

I try to CC you an all i2c patches that affect media/video drivers, but
not all of these can go through your path to Andrew rather than mine.
Changes to these drivers which are needed because of a change to
i2c-core or the core i2c structures must obviously be done all at once,
so I have to handle them. Two such changes are in the works right now,
the second being Laurent Riffard's .owner and .name cleanup patchset.
Relevant patches have been sent to the v4l list already.

Thanks,
-- 
Jean Delvare

[lm-sensors]

[Mauro Carvalho Chehab]

Em Qua, 2005-10-26 ?s 22:46 +0200, Jean Delvare escreveu:
> Hi Mauro,
> 
> > Please don't send this patch to Andrew or mainstream. I'll apply it on
> > my tree and I will send it at the end my patchset series, for not
> > breaking these.
> 
> Fine with me, as long as it ultimately gets done.
	Good! As I said, our plan is to keep it updated and we are including
several new boards on 2.6.15 (merging and fixing several kernel drivers
developed outside V4L), and there are already some newer i2c ids on
current CVS and there are more to come.
> 
> My motivation for this cleanup is that I plan to work soon on a new way
> for drivers to instantiate i2c "clients". This new way will most
> certainly require i2c IDs, so I need to have an accurate view of how
> these are currently used. Having them centralized, as they were
> supposed to be in the first place, should make my analysis easier, and
> should reduce the risk to get things wrong.
	For sure!
> 
> Note that the new client instantiation method should suit the
> media/video drivers needs much better than the old "general probing"
> way did (which, with the RTC drivers, is actually the reason why I want
> to implement this). I'll make sure to CC you and the v4l list on my
> proposal to get your feedback.
	Ok. We all have to win with this. We've created an experimental area at
V4L tree for testing purposes. Maybe we can create a branch there to
test the newer I2C core interfaces and improve it, without breaking the
current development.
	From V4L side of view it would be better if we wait until 2.6.17 for
the newer I2C core, since, our goal to 2.6.15 is to include, with
experimental status, several newer boards like: em28xx based USB boards
(already on V4L tree), WinTV PVR USB2, ivtv and maybe usbvision. 2.6.16
should be reserved for bug fixing these devices and cleaning up USB
stuff. IMHO, it is not a good idea to change I2C interface during the
merging stuff (since lots of efforts should be made to fix
incompatibitilies and having them all using almost the same
CodingStyle).
> 
> > I have several patches that are including newer IDs on
> > linux/media/i2c-id.h and I'm just preparing a patch, on V4L tree, to
> > remove this file from kernel.
> 
> I guess we must read this as: adding news IDs to linux/i2c-id.h, and
> removing the media/id.h file?
	Yes :-)
> 
> > If this is applied, it may break dozens of patches I've already
> > prepared (at
> > http://www.linuxtv.org/download/video4linux/patches/2.6.14-rc5)
> > 
> > Maybe it would be wiser if patches that touches drivers/media devices
> > maintained on V4L go first to me, and I send to Andrew. This way, it
> > would help us not to redo an entire patchset for fixing broken stuff
> > because they would be included on a wrong order.
> 
> I did send this patch to you and the v4l list two days ago, stating
> that I would send it to Greg quickly. I did not get any answer, so I
> did. When a patch of mine gets in your way, just say so and I'll delay
> it or leave it to you altogether.
	Hmm... I didn't received it.
> I try to CC you an all i2c patches that affect media/video drivers, but
> not all of these can go through your path to Andrew rather than mine.
	We have to take care with this... otherwise some patches will break the
other's patches. 
> Changes to these drivers which are needed because of a change to
> i2c-core or the core i2c structures must obviously be done all at once,
> so I have to handle them. Two such changes are in the works right now,
> the second being Laurent Riffard's .owner and .name cleanup patchset.
> Relevant patches have been sent to the v4l list already.
	This I've received it, but I'm not sure yet how to handle it.

	This patch, if applied before the newer patchsets, will break several
of our patches. Also, since it was generated against -mm and before we
send our patchset, it is not covering all supported cards anymore (we've
included em2820, two new audio decoders and two new video decoders, all
using I2C stuff).

	It would be nice if the patches that touches video stuff were generated
against V4L tree, but it will break submission against Greg's tree.
	Maybe we can define a procedure for these patches, like sending first
to me, then to Greg. I'm open to suggestions.
> 
> Thanks,
Cheers, 
Mauro.

[lm-sensors]

[Mauro Carvalho Chehab]

Em Qua, 2005-10-26 ?s 13:39 -0700, Greg KH escreveu:
> On Wed, Oct 26, 2005 at 06:15:44PM -0200, Mauro Carvalho Chehab wrote:
> > Greg,
> > 
> > 	Please don't send this patch to Andrew or mainstream. I'll apply it on
> > my tree and I will send it at the end my patchset series, for not
> > breaking these.
> > 
> > 	I have several patches that are including newer IDs on
> > linux/media/i2c-id.h and I'm just preparing a patch, on V4L tree, to
> > remove this file from kernel.
> 
> But we are removing this file, as it's just not needed.  Why would you
> add more ids to it?
Greg, 
	We had included some newer IDs on our old media/id.h for some newer
drivers during development on V4L tree, that should also be moved to
linux/i2c-id.h, before removing media/id.h.
	I'm working on this patch already. After it, we will stop using the old
media/id.h, using linux/i2c-id.h instead.
> 
> thanks,
> 
> greg k-h
> 
Cheers, 
Mauro.

[lm-sensors]

[Jean Delvare]

Hi Mauro,

> From V4L side of view it would be better if we wait until 2.6.17 for
> the newer I2C core, since, our goal to 2.6.15 is to include, with
> experimental status, several newer boards like: em28xx based USB boards
> (already on V4L tree), WinTV PVR USB2, ivtv and maybe usbvision. 2.6.16
> should be reserved for bug fixing these devices and cleaning up USB
> stuff.

You may decide that there will be no new v4l driver in 2.6.16, the
choice is up to you, but you can't force other maintainers to stop any
non-bugfix change for 6 or 8 week.

And there is nothing like "the newer I2C core". There are continuous
changes happening.

> IMHO, it is not a good idea to change I2C interface during the
> merging stuff (since lots of efforts should be made to fix
> incompatibitilies and having them all using almost the same
> CodingStyle).

You know, i2c changes don't only affect media/video drivers. They also
affect i2c bus drivers, including framebuffer I2C/DDC support, almost
all hardware monitoring drivers, and a number of platform-specific
drivers. I can't wait for all these areas to stop their development
before I submit a change to the i2c core.

I don't see how "lots of efforts" would be needed to deal with this.
Almost all changes will trigger a warning or compilation error if a
given driver wasn't updated accordingly, so we'll notice soon enough.
And most fixes after that are one-liners. It is also well known that
you have to pay particular attention to new drivers because they might
slip through mass changes. This ain't specific to the media/video
drivers, nor to i2c.

> We have to take care with this... otherwise some patches will break the
> other's patches.

This happens all the time, and there's nothing to be afraid of. Broken
patches can be fixed. If this is a problem for you, I suspect you are
not using the adequate tools.

(Subliminal hint: quilt rocks.)

Thanks,
-- 
Jean Delvare

[lm-sensors]

[Jean Delvare]

Hi Mauro,

> > But we are removing this file, as it's just not needed.  Why would you
> > add more ids to it?
>
> We had included some newer IDs on our old media/id.h for some newer
> drivers during development on V4L tree, that should also be moved to
> linux/i2c-id.h, before removing media/id.h.
>
> I'm working on this patch already. After it, we will stop using the old
> media/id.h, using linux/i2c-id.h instead.

OK, Greg and I have agreed that we would drop my patch and let you do
something similar on your side instead. This should be easier for you
that way.

Thanks,
-- 
Jean Delvare

[lm-sensors]

[Dieter Jurzitza]

Hi Rudolf,
Am Mittwoch, 3. Mai 2006 23:37 schrieben Sie:
> Hello Dieter
> Now my questions:
>
> Does it start to work after reboot? 
No. But this is a mobo issue I see with all 2460 Tyan Tiger independent of 
this problem.
> After cold boot? 
No! The reset button does *not* cure the problem. This is different from 
"normal" behaviour"
> After power unplug? 
Yes. Nothing else that helps.
> Have you ACPI in kernel? 
Yes. Any info required on that?
> Have you compiled/loaded/in kernel the thermal module?
I load:
i2c-amd756
i2c-isa
w83781d
eeprom
(in this sequence) during boot. This is done via /etc/sysconfig/lm_sensors. I 
do not know about i2c-dev. However, lsmod | grep i2c gives the following 
output:
i2c_sensor              7936  2 eeprom,w83781d
i2c_isa                 6400  0 
i2c_algo_bit           13448  0 
i2c_amd756             11908  0 
i2c_core               27136  7 
eeprom,w83781d,i2c_sensor,i2c_isa,i2c_algo_bit,ivtv_tveeprom,i2c_amd756
I do not see i2c-dev being loaded. However, it exists compiled as a module, 
therefore I am convinced that it is not residing in the kernel.


> If it will happen again in future you may try following:
It will :-(

This is my homework, I'll be back with you as soon as I have results to tell.
>
> 1) obtain the smb base addr
> 	AMD756_smba = xxx
> Should be in your debug log, you may use value from older log because it
> does not change. Alternatively you may also look into cat /proc/ioports
>
> then you may use the isadump tool
> 2)
> isadump -f xxx
>
> (please replace the xxx with the base address from the log) This will allow
> us to check the status of the SMBDATA and SMBCLK lines and see if they are
> stuck low. Eventually we can try to excersise the DATA and CLK lines to see
> if they are working properly.
>
> You may also try to provoke the failure by running this:
>
> modprobe i2c-dev
> modprobe i2c-amd756
> while true; do i2cdump -y 0 0x2d b > /dev/null; done
Thanks for helping that far. I will unload the debug kernel now as it spits 
about 400 MByte of i2c-debug logs into /var/log/messages.

Takte care,



Dieter Jurzitza


-- 
-----------------------------------------------------------

                               |
                                \
                 /\_/\           |
                | ~x~ |/-----\   /
                 \   /-       \_/
  ^^__   _        /  _  ____   /
 <??__ \- \_/     |  |/    |  |
  ||  ||         _| _|    _| _|

if you really want to see the pictures above - use some font
with constant spacing like courier! :-)
-----------------------------------------------------------

[lm-sensors]

[Dieter Jurzitza]

Dear listmembers, dear Rudolf,
these are my findings after doing what you suggested:
please find attached two files. One contains the output cat /proc/ioports plus 
the result of 
/usr/sbin/isadump -f 0x80E0
with the Chips being dead, the other one after cycling power and reboot.
One remark: when I use the kernel with debugging on, the failures seem to be 
more rare. I think this could be caused by the fact that all those printk's 
slow the accesses on the smbus somewhat down. But this is just guessing, 
nothing serious.

Thanks again,
take care



Dieter Jurzitza

-- 
-----------------------------------------------------------

                               |
                                \
                 /\_/\           |
                | ~x~ |/-----\   /
                 \   /-       \_/
  ^^__   _        /  _  ____   /
 <??__ \- \_/     |  |/    |  |
  ||  ||         _| _|    _| _|

if you really want to see the pictures above - use some font
with constant spacing like courier! :-)
-----------------------------------------------------------
-------------- next part --------------
A non-text attachment was scrubbed...
Name: healthystatus.zip
Type: application/x-zip
Size: 518 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060505/89fa4bd7/healthystatus.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: errorstatus.zip
Type: application/x-zip
Size: 961 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060505/89fa4bd7/errorstatus.bin

[lm-sensors]

[Laurent Pinchart]

Hi Rudolf,

> > I'm facing a small architecture problem on an embedded (MPC8248) design.
> > The processor has a hardware I2C bus for which a driver exists in the
> > Denx linuxppc kernel tree at rsync://www.denx.de/git/linux-2.6-denx (the
> > driver is called i2c-mpc8260, does anyone know if there are plans to
> > integrate it in mainline kernels ?).
>
> No we dont know.
>
> > Our hardware platform has a DSL daughterboard on which a PCA9557 GPIO I2C
> > chip is used to control several DSL functions (resetting chipsets,
> > connecting to the line, ...). The DSL and PCA9557 chipsets are thus both
> > part of a single "device", but are not connected to the same bus (the DSL
> > chipset is a platform device, while the PCA9557 is connected on the I2C
> > bus).
>
> Good.
>
> > I'm developping the DSL chipset driver, and the needed arises to control
> > the GPIO lines from inside the driver. I've had a look at some PCI
> > drivers which need to control I2C devices, and all of them have the I2C
> > chips connected to an I2C bus controlled by the PCI device. Getting a
> > reference to those chips is thus quite easy. I'm a bit puzzled regarding
> > how to do the same for my design. Could anyone advice me on how to get a
> > reference to the PCA9557 from the DSL platform driver in a clean way ?
>
> Well this is a bit problem. There are three ways:
>
> 1) create EXPORT_SYMBOL(my_api) with defined API and call it from your PCI
> device to the PCA driver some RTC drivers did this

That's not very clean, but would work.

> 2) there is,  now obsoleted, method with the i2c command callback. This may
> be removed in the future.

I'd rather not use obsoleted methods.

> 3) create a class interface for this stuff and register the device in the
> class I'm taking this aproach to create an watchdog class even for i2c
> devices...

I'll try that, as it seems to be the cleaner approach. I suppose I'm not the 
only one who is interested in a GPIO class.

> And because perhaps you will not want to have your driver integrated into
> kernel, and because it is a very special think you may use the #1 method
> without problems.

I'll go for #1 first as our deadlines are quite tight, and will try to 
implement #3 a bit later. Regarding integrating the driver into the mainline 
kernel, I unfortunately can't do that as the specs are covered by an NDA :-(

> Please note that if the PCA driver needs to be called 
> from interrupts then you will need some workqueues. See some other RTC
> drivers for inspiration. (pre 2.6.17 because I think 2.6.17 has already a
> RTC class)

Thanks.

Laurent Pinchart

[lm-sensors]

[Udo van den Heuvel]

in modprobe.conf:

options w83627hf reset=1


I have a VIA EK8000 which has a similar problem. This is a workaround.
Please post if it works.

[lm-sensors]

[Thomas Dohl]

Hi Udo,

thank you very much!
Yes it works.

Regards
Thomas Dohl


Udo van den Heuvel schrieb:
> in modprobe.conf:
> 
> options w83627hf reset=1
> 
> 
> I have a VIA EK8000 which has a similar problem. This is a workaround.
> Please post if it works.
> 

[lm-sensors]

[Udo van den Heuvel]

Thomas Dohl wrote:
> Hi Udo,
> 
> thank you very much!
> Yes it works.

So we might have the same problem.
In my situation I have a VIA EK8000 which has the fans running during a
boot until the kernel loads/starts. Then the fans stop. So is it a Linux
issue?
When the w83627hf module is loaded with the reset=1 parameter the fans
start again. Fancontrol is functional (although I do not use it).

What can we do to find out more?

[lm-sensors]

[Thomas Dohl]

Hi Udo,

I've found a strange bug or what ever in the Module. :)
I'll try to explain it.
If I boot Linux with the module "w83627hf", everything
is ok. But if I load the module with the workaround, like
"modprobe w83627hf reset=1" I get a 23?C higher temperature
of the second sensor.

At boottime:
temp1: 34?C
temp2: 33?C

after modprobe with reset=1
temp1: 34?C
temp2: 56?C (23?C more than it is)

This doesn't change after rmmod w83627hf, modprobe w83627hf.
Only after reboot, the values return to normal.

What could that be?

Regards
Thomas Dohl
(from Germany)


Udo van den Heuvel schrieb:
> in modprobe.conf:
> 
> options w83627hf reset=1
> 
> 
> I have a VIA EK8000 which has a similar problem. This is a workaround.
> Please post if it works.
> 

[lm-sensors]

[Christophe de Rivière]

Le Dimanche 7 Janvier 2007 15:30, Rudolf Marek a ?crit?:
> Hello,
>
> > But now that i want to jump to a newer distro, none of them is able to
> > detect this smbus in spite of the quirks (see above) that is present in
> > the kernel : $ cat /usr/src/linux/drivers/pci/quirks.c |grep -i w1
> >                         case 0x184b: /* W1N notebook */
> > The same lack occured occured on ubuntu 6.10 , suse 10.1, mandriva 2007
> > and suse 10.2 with recent kernels ranging from 2.6.16 to 2.6.18 . No
> > smbus is appears on the lspci for any of these distro.
> >[...]
> Answer is simple. There were some problems with suspend resume, when the
> smbus was enabled. (from 2.6.17-2.6.19) It is fixed in 2.6.20. You need to
> disable the ACPI sleep support to re-enable the quirk. I guess it is not
> what you want.
> http://www.lm-sensors.org/browser/lm-sensors/trunk/prog/hotplug/unhide_ICH_
>SMBus?format=raw

You are simply great ! It simply worked like a charm this morning on a suse 
10.2 and i suppose it will work on any other distro. !

> > The sensor monitor and fan speed control is a very interesting featur[...]
>
> Please can you attach the DSDT table to your mail?
>
> cat /proc/acpi/dsdt > /tmp/dsdt.bin

Attached to this email.

> Rudolf

Thanks again for your generous help !!!

-- 
christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dsdt
Type: application/octet-stream
Size: 27981 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20070108/827fafb3/attachment-0001.obj 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20070108/827fafb3/attachment-0001.bin 

Re: [lm-sensors]

[jk]

Jean Delvare <khali <at> linux-fr.org> writes:
> On 08 Apr 2007 16:17:31 -0400, lmsensors <at> kosowsky.org wrote:
> > On 06 Apr 2007 13:21:59 -0400, jk wrote:
> > I think though that part of the problem is that many of the user-space
> > programs and documentation in the lm_sensors tarball (at least in
> > version 2.10.1) are not updated for recent 2.6 kernels.
> 
> True. Their number is hopefully shrinking, but there may be a couple
> scripts still not ported.
> 
> > For example, the program sens_update_rrd looks in either
> > /proc/sys/dev/sensors (which is obsolete) or in /sys/bus/i2c/devices
> > (which gives the problems I have).
> 
> /proc/sys/dev/sensors isn't really obsolete, it is the right place to
> look at for 2.4 kernels.
> 
> sens_update_rrd is not maintained as far as I know so I am not
> surprised if you have some problems with it.
> 
> > Instead it should just go to /sys/class/hwmon and be done with it!
> 
> No, doing so would break compatility with older kernels. The right
> thing to do is to look in /sys/class/hwmon first and fallback to old
> places if that didn't work. This is what libsensors does.
> 
> > Also, just as a random example, the documentation for sensors.conf
> > when discussing about the BUS STATEMENT (which I found by your reference
> > below) only talks about /proc/bus/i2c which is obsolete for 2.6
> > kernels. Similarly, it references the program
> > prog/config/grab_busses.sh which only works in 2.4 kernels.
> > 
> > So, I guess the better question is whether anybody plans on updating
> > the documentation and auxiliary programs in the lm_sensors tarball to
> > reflect 2.6 kernels in general and "later" 2.6 kernels in particular.
> 
> What about you? Maybe you could stop complaining and actually help the
> project?

I believe I have been "asking questions" rather than "complaining". Usually,
packages have existing maintainers and it would be a bit presumptuous of me as a
newbie to lm_sensors to start rewriting base code without making sure first that
I really understand the issue and the reason for the existing situation (which
you and others have now just explained to me) and second that no one else is
currently maintaining the code (which indeed it seems that no one else is doing).

And in fact, I have now adjusted the code to first check for existence of hwmon
(and I also wrote another cleaner version that only looks at hwmon but I
understand why you may not want that so as to preserve backwards compatibility.)

Anyway, here is the diff for the version that preserves backwards compatibility. 

--- sens_update_rrd             2007-04-08 01:18:16.000000000 -0400
+++ sens_update_rrd.new         2007-04-15 03:42:15.000000000 -0400
@@ -30,6 +30,7 @@
 then
        echo "usage: $0 database.rrd sensor"
        echo "       sensor example: w83781d-isa-0290 (2.4) or 0-0290 (2.6)"
+       echo "           or hwmon0 (later 2.6)"
        exit 1
 fi
 #
@@ -38,19 +39,23 @@

 SENSDIR=/proc/sys/dev/sensors
 SDIR=/sys/bus/i2c/devices
-if [ ! -d $SENSDIR ]
+HWMONDIR=/sys/class/hwmon
+SENSDEV=$2
+if [ -d $HWMONDIR ]
 then
-       if [ ! -d $SDIR ]
+       SYSFS=1
+       SENSDIR=$HWMONDIR
+       SENSDEV=$SENSDEV/device
+elif [ -d $SDIR ]
        then
-               echo $0: 'No sensors found! (modprobe sensor modules?)'
-               exit 1
-       else
                SYSFS=1
                SENSDIR=$SDIR
-       fi
+elif [ ! -d $SENSDIR]
+then
+       echo $0: 'No sensors found! (modprobe sensor modules?)'
+       exit 1
 fi

-SENSDEV=$2
 SENS=$SENSDIR/$SENSDEV
 if [ ! -r $SENS ]
 then


> 
> I don't use sens_update_rrd myself, nor prog/config/grab_busses.sh, nor
> bus statements. You do. Why would you expect me or anyone else to fix
> them?

Did I ever say I expected you to? But it is not an unreasonable expectation for
there to be a maintainer of code or barring that some indication that the code
is no longer supported. Also, I would think that even if the rrd stuff is seldom
used that the documentation for a basic config file like sensors.conf should be
up to date and should not just reference obsolete kernel 2.4 methods which by
now are four years out of date.

In any case, I am happy to help where I can, but I don't appreciate being
accused of complaining or of having unreasonable expectations that I never
myself expressed or implied.







_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Dieter Rogiest]

On Thursday 10 May 2007 18:36:31 Dieter Rogiest wrote:
> Core0 Temp:
>              +17°C
> Core1 Temp:
>              +25°C
>
> How come temperature of Core0 is so low? Does this mean that Core0 is not
> used by my kernel (uname -r: 2.6.20.11-tmb-desktop-1mdvsmp )?
> What programs should I use so that Core0 is also used
> Are there shell commands by which I can let Core0 and Core1 work to their
> maximum?

I tried KPovModeler (Povray) and rendered a Pinocchio scene (am_pinoc.kpm).
I ran sensors while it was rendering:
Core0 Temp:
             +29°C
Core1 Temp:
             +36°C
Now I think Core0 is used by the povray rendering.

Dieter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Hans-Jürgen Koch]

Am Donnerstag 10 Mai 2007 19:36 schrieb Dieter Rogiest:

> > What programs should I use so that Core0 is also used
> > Are there shell commands by which I can let Core0 and Core1 work to their
> > maximum?

Compile a kernel with make -j4. Or have a look at the cpuburn package.

Hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Stephen Cormier]

[-- Attachment #1.1: Type: text/plain, Size: 1330 bytes --]

On May 10, 2007 01:36:31 pm Dieter Rogiest wrote:
> My cpu is an Athlon 64 X2 Dual core 5600+.
> I installed lm_sensors-2.10.2 on Mandriva Linux 2007.1.
>
> When I run sensors I get these temperatures:
> k8temp-pci-00c3
> Adapter: PCI adapter
> Core0 Temp:
>              +17°C
> Core1 Temp:
>              +25°C
>
> How come temperature of Core0 is so low? Does this mean that Core0 is not
> used by my kernel (uname -r: 2.6.20.11-tmb-desktop-1mdvsmp )?
> What programs should I use so that Core0 is also used
> Are there shell commands by which I can let Core0 and Core1 work to their
> maximum?

From the looks of it both cores work as shown by your other post. When I had 
both of the Opterons I ran in my 939 board they showed the same thing you see 
one of the cores was always at least 8 or 10 C higher than the other now with 
my X2 3800 (512k cache) they are within a degree of each other at all times. 
I checked and seen the chip you have has the same 1mb cache as the Opterons 
so I would go with the idea that is how them chips run, one core always much 
hotter than the other since this would be three that I have seen the 
temperatures for and they all have the same type of readings.

> Dieter
>

Stephen

-- 
GPG Pubic Key: http://users.eastlink.ca/~stephencormier/publickey.asc

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Rudolf Marek]

Hello all,

Simple explanation would be that they are not properly calibrated.

To "burn" on both cores issue on two terminals:

dd if=/dev/urandom of=/dev/null

dd if=/dev/urandom of=/dev/null


check with top command. if both eats CPU

Anyway, I think the CPU temp is not properly calibrated.

Rudolf

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Dieter Rogiest]

On Thursday 10 May 2007 21:31:39 Rudolf Marek wrote:
> To "burn" on both cores issue on two terminals:
>
> dd if=/dev/urandom of=/dev/null
>
> dd if=/dev/urandom of=/dev/null
Yes, I checked it with Ksysguard monitor: I let it show system load for CPU0 
and CPU1. They both go up to 99%. 
When I stop the "burn" command in the second terminal, the system load in CPU0 
drops to zero.
> Anyway, I think the CPU temp is not properly calibrated.
Is it possible to calibrate them? Or is the difference between Core0 and Core1 
set in the hardware?

Dieter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Rudolf Marek]

Hi again,

> Is it possible to calibrate them? Or is the difference between Core0 and Core1 
> set in the hardware?

No it is not. Just for a record, please can you write back here your CPU info?

You may obtain it with this command:

egrep  'model|stepp|fam' /proc/cpuinfo


Thanks,

Rudolf

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Dieter Rogiest]

On Friday 11 May 2007 00:28:00 Rudolf Marek wrote:
> No it is not. Just for a record, please can you write back here your CPU
> info?
> egrep  'model|stepp|fam' /proc/cpuinfo
cpu family      : 15
model           : 67
model name      : AMD Athlon(tm) 64 X2 Dual Core Processor 5600+
stepping        : 3
cpu family      : 15
model           : 67
model name      : AMD Athlon(tm) 64 X2 Dual Core Processor 5600+
(processor 0 and processor 1 have the same info of course)

Dieter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Jean Delvare]

Hi Christian,

On Tue, 24 Jul 2007 11:35:37 +0200, Christian Hohnstaedt wrote:
> writing values < 0°C to TLow and THigh of the LM75 is not 
> supported by the driver, since it strtoul()s instead
> of strtol()ing.
> 
> Trivial fix attached.

Good catch, however your fix is not correct:

> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index a40166f..4fa3220 100644
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c
> @@ -95,7 +95,7 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *da,
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct lm75_data *data = i2c_get_clientdata(client);
>  	int nr = attr->index;
> -	unsigned long temp = simple_strtoul(buf, NULL, 10);
> +	int temp = simple_strtol(buf, NULL, 10);
>  
>  	mutex_lock(&data->update_lock);
>  	data->temp[nr] = LM75_TEMP_TO_REG(temp);

temp should be a long rather than an int. Otherwise the input value
might wrap before LM75_TEMP_TO_REG has a chance to clamp it.

Your patch also lacks a Signed-off-by line (see
Documentation/SubmittingPatches, section 12.)

Care to resend an updated patch?

Thanks,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Christian Hohnstaedt]

On Tue, Jul 24, 2007 at 02:06:04PM +0200, Jean Delvare wrote:
> Hi Christian,
> 
> On Tue, 24 Jul 2007 11:35:37 +0200, Christian Hohnstaedt wrote:
> > writing values < 0°C to TLow and THigh of the LM75 is not 
> > supported by the driver, since it strtoul()s instead
> > of strtol()ing.
> > 
> > Trivial fix attached.
> 
> Good catch, however your fix is not correct:
> 
> > diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> > index a40166f..4fa3220 100644
> > --- a/drivers/hwmon/lm75.c
> > +++ b/drivers/hwmon/lm75.c
> > @@ -95,7 +95,7 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *da,
> >  	struct i2c_client *client = to_i2c_client(dev);
> >  	struct lm75_data *data = i2c_get_clientdata(client);
> >  	int nr = attr->index;
> > -	unsigned long temp = simple_strtoul(buf, NULL, 10);
> > +	int temp = simple_strtol(buf, NULL, 10);
> >  
> >  	mutex_lock(&data->update_lock);
> >  	data->temp[nr] = LM75_TEMP_TO_REG(temp);
> 
> temp should be a long rather than an int. Otherwise the input value
> might wrap before LM75_TEMP_TO_REG has a chance to clamp it.

Since LM75_TEMP_TO_REG() expects an int, I thought it would be best
to make temp an int here, to reduce confusion.

> 
> Your patch also lacks a Signed-off-by line (see
> Documentation/SubmittingPatches, section 12.)

Didn't expect to have to add it to this 10 char change :-)
However, playing save is always good.

> 
> Care to resend an updated patch?
No, but I still think my fix is correct.
Please confirm and I will send the patch with Signed-off-by line


Christian Hohnstaedt

-- 
Christian Hohnstaedt
Software Engineer

Innominate Security Technologies AG  /protecting industrial networks/
tel: +49.30.6392-3285 fax: +49.30.6392-3307
Albert-Einstein-Strasse 14, D-12489 Berlin, Germany
http://www.innominate.com

Register Court: AG Charlottenburg, HR B 81603
Management Board: Joachim Fietz, Dirk Seewald
Chairman of the Supervisory Board: Edward M. Stadum

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Jean Delvare]

On Tue, 24 Jul 2007 14:57:53 +0200, Christian Hohnstaedt wrote:
> On Tue, Jul 24, 2007 at 02:06:04PM +0200, Jean Delvare wrote:
> > On Tue, 24 Jul 2007 11:35:37 +0200, Christian Hohnstaedt wrote:
> > > diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> > > index a40166f..4fa3220 100644
> > > --- a/drivers/hwmon/lm75.c
> > > +++ b/drivers/hwmon/lm75.c
> > > @@ -95,7 +95,7 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *da,
> > >  	struct i2c_client *client = to_i2c_client(dev);
> > >  	struct lm75_data *data = i2c_get_clientdata(client);
> > >  	int nr = attr->index;
> > > -	unsigned long temp = simple_strtoul(buf, NULL, 10);
> > > +	int temp = simple_strtol(buf, NULL, 10);
> > >  
> > >  	mutex_lock(&data->update_lock);
> > >  	data->temp[nr] = LM75_TEMP_TO_REG(temp);
> > 
> > temp should be a long rather than an int. Otherwise the input value
> > might wrap before LM75_TEMP_TO_REG has a chance to clamp it.
> 
> Since LM75_TEMP_TO_REG() expects an int, I thought it would be best
> to make temp an int here, to reduce confusion.

Good point. But what this means then is that LM75_TEMP_TO_REG() should
take a long as a parameter rather than an int.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Christian Hohnstaedt]

On Tue, Jul 24, 2007 at 03:09:57PM +0200, Jean Delvare wrote:
> On Tue, 24 Jul 2007 14:57:53 +0200, Christian Hohnstaedt wrote:
> > On Tue, Jul 24, 2007 at 02:06:04PM +0200, Jean Delvare wrote:
> > > On Tue, 24 Jul 2007 11:35:37 +0200, Christian Hohnstaedt wrote:
> > > > diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> > > > index a40166f..4fa3220 100644
> > > > --- a/drivers/hwmon/lm75.c
> > > > +++ b/drivers/hwmon/lm75.c
> > > > @@ -95,7 +95,7 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *da,
> > > >  	struct i2c_client *client = to_i2c_client(dev);
> > > >  	struct lm75_data *data = i2c_get_clientdata(client);
> > > >  	int nr = attr->index;
> > > > -	unsigned long temp = simple_strtoul(buf, NULL, 10);
> > > > +	int temp = simple_strtol(buf, NULL, 10);
> > > >  
> > > >  	mutex_lock(&data->update_lock);
> > > >  	data->temp[nr] = LM75_TEMP_TO_REG(temp);
> > > 
> > > temp should be a long rather than an int. Otherwise the input value
> > > might wrap before LM75_TEMP_TO_REG has a chance to clamp it.
> > 
> > Since LM75_TEMP_TO_REG() expects an int, I thought it would be best
> > to make temp an int here, to reduce confusion.
> 
> Good point. But what this means then is that LM75_TEMP_TO_REG() should
> take a long as a parameter rather than an int.

Sounds reasonable.

Then LM75_TEMP_FROM_REG() should also return a long instead of an int.

Other drivers using LM75_TEMP_TO_REG:

driver          temp.type   func
------------------------------------
asb100.c        long        strtoul
ds1621.c        direct      strtoul
w83627e?hf.c    u32         strtoul
w83781d.c       s32         strtol

Looks like there is some kind of general inconsistency :-)


Christian Hohnstaedt

-- 
Christian Hohnstaedt
Software Engineer

Innominate Security Technologies AG  /protecting industrial networks/
tel: +49.30.6392-3285 fax: +49.30.6392-3307
Albert-Einstein-Strasse 14, D-12489 Berlin, Germany
http://www.innominate.com

Register Court: AG Charlottenburg, HR B 81603
Management Board: Joachim Fietz, Dirk Seewald
Chairman of the Supervisory Board: Edward M. Stadum

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Jean Delvare]

Hi Christian,

On Tue, 24 Jul 2007 15:43:28 +0200, Christian Hohnstaedt wrote:
> On Tue, Jul 24, 2007 at 03:09:57PM +0200, Jean Delvare wrote:
> > Good point. But what this means then is that LM75_TEMP_TO_REG() should
> > take a long as a parameter rather than an int.
> 
> Sounds reasonable.
> 
> Then LM75_TEMP_FROM_REG() should also return a long instead of an int.

This doesn't really matter. A long is needed for LM75_TEMP_TO_REG()
because that's what simple_strtol() gives, but in the other direction,
we use sprintf, which can handle both an int or a long.

> Other drivers using LM75_TEMP_TO_REG:
> 
> driver          temp.type   func
> ------------------------------------
> asb100.c        long        strtoul
> ds1621.c        direct      strtoul
> w83627e?hf.c    u32         strtoul
> w83781d.c       s32         strtol
> 
> Looks like there is some kind of general inconsistency :-)

Indeed. Can you please send a patch fixing all the affected drivers?

Thanks,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Christian Hohnstaedt]

On Sun, Aug 12, 2007 at 01:13:52PM +0200, Jean Delvare wrote:
> Hi Christian,
> 
> On Tue, 24 Jul 2007 15:43:28 +0200, Christian Hohnstaedt wrote:
> > On Tue, Jul 24, 2007 at 03:09:57PM +0200, Jean Delvare wrote:
> > > Good point. But what this means then is that LM75_TEMP_TO_REG() should
> > > take a long as a parameter rather than an int.
> > 
> > Sounds reasonable.
> > 
> > Then LM75_TEMP_FROM_REG() should also return a long instead of an int.
> 
> This doesn't really matter. A long is needed for LM75_TEMP_TO_REG()
> because that's what simple_strtol() gives, but in the other direction,
> we use sprintf, which can handle both an int or a long.

I will keep the "static inline int LM75_TEMP_FROM_REG(u16 reg)" ,
because I don't want to change all the sprintf("%d") to sprintf("%ld").

What about w83627hf.c
	return sprintf(buf,"%ld\n", \
		(long)LM75_TEMP_FROM_REG(data->reg##_add[nr-2]));
?

> 
> > Other drivers using LM75_TEMP_TO_REG:
> > 
> > driver          temp.type   func
> > ------------------------------------
> > asb100.c        long        strtoul
> > ds1621.c        direct      strtoul
> > w83627e?hf.c    u32         strtoul
> > w83781d.c       s32         strtol
> > 
> > Looks like there is some kind of general inconsistency :-)
> 
> Indeed. Can you please send a patch fixing all the affected drivers?

I could, but I only can test the LM75 driver.

Christian Hohnstaedt

-- 
Christian Hohnstaedt
Software Engineer

Innominate Security Technologies AG  /protecting industrial networks/
tel: +49.30.6392-3285 fax: +49.30.6392-3307
Albert-Einstein-Strasse 14, D-12489 Berlin, Germany
http://www.innominate.com

Register Court: AG Charlottenburg, HR B 81603
Management Board: Joachim Fietz, Dirk Seewald
Chairman of the Supervisory Board: Edward M. Stadum

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Jean Delvare]

Hi Christian,

On Mon, 13 Aug 2007 10:39:00 +0200, Christian Hohnstaedt wrote:
> On Sun, Aug 12, 2007 at 01:13:52PM +0200, Jean Delvare wrote:
> > On Tue, 24 Jul 2007 15:43:28 +0200, Christian Hohnstaedt wrote:
> > > Then LM75_TEMP_FROM_REG() should also return a long instead of an int.
> > 
> > This doesn't really matter. A long is needed for LM75_TEMP_TO_REG()
> > because that's what simple_strtol() gives, but in the other direction,
> > we use sprintf, which can handle both an int or a long.
> 
> I will keep the "static inline int LM75_TEMP_FROM_REG(u16 reg)" ,
> because I don't want to change all the sprintf("%d") to sprintf("%ld").

Agreed.

> What about w83627hf.c
> 	return sprintf(buf,"%ld\n", \
> 		(long)LM75_TEMP_FROM_REG(data->reg##_add[nr-2]));
> ?

The cast to long could be removed together with the "l" length modifier
in the format string, independently of the rest of your patch.

> > > Other drivers using LM75_TEMP_TO_REG:
> > > 
> > > driver          temp.type   func
> > > ------------------------------------
> > > asb100.c        long        strtoul
> > > ds1621.c        direct      strtoul
> > > w83627e?hf.c    u32         strtoul
> > > w83781d.c       s32         strtol
> > > 
> > > Looks like there is some kind of general inconsistency :-)
> > 
> > Indeed. Can you please send a patch fixing all the affected drivers?
> 
> I could, but I only can test the LM75 driver.

I can test the ds1621, w83627hf, w83627ehf and w83781d drivers (given
enough time) when you're done.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Christian Hohnstaedt]

  - replace differing temperature variable types by long
  - use strtol() instead of strtoul() for conversion

Signed-off-by: Christian Hohnstaedt <chohnstaedt@innominate.com>
---
 drivers/hwmon/ad7418.c    |    2 +-
 drivers/hwmon/asb100.c    |    4 ++--
 drivers/hwmon/ds1621.c    |    2 +-
 drivers/hwmon/lm75.c      |    2 +-
 drivers/hwmon/lm75.h      |    2 +-
 drivers/hwmon/w83627ehf.c |    2 +-
 drivers/hwmon/w83627hf.c  |    6 +++---
 drivers/hwmon/w83781d.c   |    2 +-
 8 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/hwmon/ad7418.c b/drivers/hwmon/ad7418.c
index cc8b624..879ea72 100644
--- a/drivers/hwmon/ad7418.c
+++ b/drivers/hwmon/ad7418.c
@@ -172,7 +172,7 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *devattr,
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct i2c_client *client = to_i2c_client(dev);
 	struct ad7418_data *data = i2c_get_clientdata(client);
-	int temp = simple_strtol(buf, NULL, 10);
+	long temp = simple_strtol(buf, NULL, 10);
 
 	mutex_lock(&data->lock);
 	data->temp[attr->index] = LM75_TEMP_TO_REG(temp);
diff --git a/drivers/hwmon/asb100.c b/drivers/hwmon/asb100.c
index 57b1c7b..55d92e1 100644
--- a/drivers/hwmon/asb100.c
+++ b/drivers/hwmon/asb100.c
@@ -143,7 +143,7 @@ static int FAN_FROM_REG(u8 val, int div)
 
 /* TEMP: 0.001C/bit (-128C to +127C)
    REG: 1C/bit, two's complement */
-static u8 TEMP_TO_REG(int temp)
+static u8 TEMP_TO_REG(long temp)
 {
 	int ntemp = SENSORS_LIMIT(temp, ASB100_TEMP_MIN, ASB100_TEMP_MAX);
 	ntemp += (ntemp<0 ? -500 : 500);
@@ -448,7 +448,7 @@ static ssize_t set_##reg(struct device *dev, const char *buf, \
 { \
 	struct i2c_client *client = to_i2c_client(dev); \
 	struct asb100_data *data = i2c_get_clientdata(client); \
-	unsigned long val = simple_strtoul(buf, NULL, 10); \
+	long val = simple_strtol(buf, NULL, 10); \
  \
 	mutex_lock(&data->update_lock); \
 	switch (nr) { \
diff --git a/drivers/hwmon/ds1621.c b/drivers/hwmon/ds1621.c
index 1212d6b..5a861f9 100644
--- a/drivers/hwmon/ds1621.c
+++ b/drivers/hwmon/ds1621.c
@@ -151,7 +151,7 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *da,
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
 	struct i2c_client *client = to_i2c_client(dev);
 	struct ds1621_data *data = ds1621_update_client(dev);
-	u16 val = LM75_TEMP_TO_REG(simple_strtoul(buf, NULL, 10));
+	u16 val = LM75_TEMP_TO_REG(simple_strtol(buf, NULL, 10));
 
 	mutex_lock(&data->update_lock);
 	data->temp[attr->index] = val;
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index a40166f..4fa3220 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -95,7 +95,7 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *da,
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lm75_data *data = i2c_get_clientdata(client);
 	int nr = attr->index;
-	unsigned long temp = simple_strtoul(buf, NULL, 10);
+	long temp = simple_strtol(buf, NULL, 10);
 
 	mutex_lock(&data->update_lock);
 	data->temp[nr] = LM75_TEMP_TO_REG(temp);
diff --git a/drivers/hwmon/lm75.h b/drivers/hwmon/lm75.h
index af7dc65..7c93454 100644
--- a/drivers/hwmon/lm75.h
+++ b/drivers/hwmon/lm75.h
@@ -33,7 +33,7 @@
 
 /* TEMP: 0.001C/bit (-55C to +125C)
    REG: (0.5C/bit, two's complement) << 7 */
-static inline u16 LM75_TEMP_TO_REG(int temp)
+static inline u16 LM75_TEMP_TO_REG(long temp)
 {
 	int ntemp = SENSORS_LIMIT(temp, LM75_TEMP_MIN, LM75_TEMP_MAX);
 	ntemp += (ntemp<0 ? -250 : 250);
diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index c51ae2e..e3ee30b 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -835,7 +835,7 @@ store_##reg(struct device *dev, struct device_attribute *attr, \
 	struct w83627ehf_data *data = dev_get_drvdata(dev); \
 	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr); \
 	int nr = sensor_attr->index; \
-	u32 val = simple_strtoul(buf, NULL, 10); \
+	long val = simple_strtol(buf, NULL, 10); \
  \
 	mutex_lock(&data->update_lock); \
 	data->reg[nr] = LM75_TEMP_TO_REG(val); \
diff --git a/drivers/hwmon/w83627hf.c b/drivers/hwmon/w83627hf.c
index 7a4a15f..0866739 100644
--- a/drivers/hwmon/w83627hf.c
+++ b/drivers/hwmon/w83627hf.c
@@ -263,7 +263,7 @@ static inline u8 FAN_TO_REG(long rpm, int div)
 
 /* TEMP: 0.001C/bit (-128C to +127C)
    REG: 1C/bit, two's complement */
-static u8 TEMP_TO_REG(int temp)
+static u8 TEMP_TO_REG(long temp)
 {
         int ntemp = SENSORS_LIMIT(temp, TEMP_MIN, TEMP_MAX);
         ntemp += (ntemp<0 ? -500 : 500);
@@ -642,9 +642,9 @@ static ssize_t \
 store_temp_##reg (struct device *dev, const char *buf, size_t count, int nr) \
 { \
 	struct w83627hf_data *data = dev_get_drvdata(dev); \
-	u32 val; \
+	long val; \
 	 \
-	val = simple_strtoul(buf, NULL, 10); \
+	val = simple_strtol(buf, NULL, 10); \
 	 \
 	mutex_lock(&data->update_lock); \
 	 \
diff --git a/drivers/hwmon/w83781d.c b/drivers/hwmon/w83781d.c
index f85b48f..5783995 100644
--- a/drivers/hwmon/w83781d.c
+++ b/drivers/hwmon/w83781d.c
@@ -410,7 +410,7 @@ static ssize_t store_temp_##reg (struct device *dev, \
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(da); \
 	struct w83781d_data *data = dev_get_drvdata(dev); \
 	int nr = attr->index; \
-	s32 val; \
+	long val; \
 	 \
 	val = simple_strtol(buf, NULL, 10); \
 	 \
-- 
1.5.0.1


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Jean Delvare]

Hi Christian,

On Wed, 15 Aug 2007 17:32:03 +0200, Christian Hohnstaedt wrote:
>   - replace differing temperature variable types by long
>   - use strtol() instead of strtoul() for conversion

Note: I've taken a look at the simple_strtol implementation and found
that it is calling simple_strtoul(), and that simple_strtoul doesn't
handle overflows. After all these functions are called "simple" for a
reason... This means that there's little point in being very strict in
the driver code on that front.

Still, I like this patch, that's a nice cleanup.

> Signed-off-by: Christian Hohnstaedt <chohnstaedt@innominate.com>
> ---
>  drivers/hwmon/ad7418.c    |    2 +-
>  drivers/hwmon/asb100.c    |    4 ++--
>  drivers/hwmon/ds1621.c    |    2 +-
>  drivers/hwmon/lm75.c      |    2 +-
>  drivers/hwmon/lm75.h      |    2 +-
>  drivers/hwmon/w83627ehf.c |    2 +-
>  drivers/hwmon/w83627hf.c  |    6 +++---
>  drivers/hwmon/w83781d.c   |    2 +-
>  8 files changed, 11 insertions(+), 11 deletions(-)

Test results:

ds1261: all OK
w83627ehf: temp1 not OK (can't write negative limits)
w83627hf: all OK
w83781d: all OK

Please fix the w83627ehf temp1 case and resubmit.

Thanks,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Christian Hohnstaedt]

On Wed, Aug 15, 2007 at 09:28:58PM +0200, Jean Delvare wrote:
> Hi Christian,
> 
> > ---
> >  drivers/hwmon/ad7418.c    |    2 +-
> >  drivers/hwmon/asb100.c    |    4 ++--
> >  drivers/hwmon/ds1621.c    |    2 +-
> >  drivers/hwmon/lm75.c      |    2 +-
> >  drivers/hwmon/lm75.h      |    2 +-
> >  drivers/hwmon/w83627ehf.c |    2 +-
> >  drivers/hwmon/w83627hf.c  |    6 +++---
> >  drivers/hwmon/w83781d.c   |    2 +-
> >  8 files changed, 11 insertions(+), 11 deletions(-)
> 
> Test results:
> 
> ds1261: all OK
> w83627ehf: temp1 not OK (can't write negative limits)
Wasn't catched by searching for TEMP_TO_REG -> fixed.

> w83627hf: all OK
> w83781d: all OK
> 
> Please fix the w83627ehf temp1 case and resubmit.
I additionally found this issue in

drivers/hwmon/adm1021.c
drivers/hwmon/lm77.c
drivers/hwmon/lm93.c

and fixed them appropriately.
Patch follows, please test it.

Christian Hohnstaedt

-- 
Christian Hohnstaedt
Software Engineer

Innominate Security Technologies AG  /protecting industrial networks/
tel: +49.30.6392-3285 fax: +49.30.6392-3307
Albert-Einstein-Strasse 14, D-12489 Berlin, Germany
http://www.innominate.com

Register Court: AG Charlottenburg, HR B 81603
Management Board: Joachim Fietz, Dirk Seewald
Chairman of the Supervisory Board: Edward M. Stadum

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Hans de Goede]

Mark M. Hoffman wrote:
> Hi Jean:
> 
>> Jean Delvare wrote:
>>> Additionally, I think that the examples are a bit difficult to read,
>>> one level of indentation would probably be better than the begin/end
>>> code markers. Thus, I propose the following incremental patch:
>>>
>>>  Documentation/hwmon/sysfs-interface |   30 ++++++++++++++----------------
>>>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> * Hans de Goede <j.w.r.degoede@hhs.nl> [2007-10-07 13:37:53 +0200]:
>> I agree with all changes, Mark pleaee apply:
>>
>> Acked-by: Hans de Goede <j.w.r.degoede@hhs.nl>
> 
> Could you please re-generate this one against the hwmon testing tree and
> w/ a Signed-off-by?
> 

I must admit I'm a total git noob, can you give me a git one liner to get your 
testing tree (IOT the "cvs co" equivalent of git)?

Thanks & Regards,

Hans



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Mark M. Hoffman]

Hi Hans:

* Hans de Goede <j.w.r.degoede@hhs.nl> [2007-10-07 20:38:07 +0200]:
> Mark M. Hoffman wrote:
> >Hi Jean:
> >
> >>Jean Delvare wrote:
> >>>Additionally, I think that the examples are a bit difficult to read,
> >>>one level of indentation would probably be better than the begin/end
> >>>code markers. Thus, I propose the following incremental patch:
> >>>
> >>> Documentation/hwmon/sysfs-interface |   30 
> >>> ++++++++++++++----------------
> >>> 1 file changed, 14 insertions(+), 16 deletions(-)
> >
> >* Hans de Goede <j.w.r.degoede@hhs.nl> [2007-10-07 13:37:53 +0200]:
> >>I agree with all changes, Mark pleaee apply:
> >>
> >>Acked-by: Hans de Goede <j.w.r.degoede@hhs.nl>
> >
> >Could you please re-generate this one against the hwmon testing tree and
> >w/ a Signed-off-by?
> >
> 
> I must admit I'm a total git noob, can you give me a git one liner to get 
> your testing tree (IOT the "cvs co" equivalent of git)?

Sorry I was ambiguous... I was asking for Jean to do this since it was his
patch.  Really, I just need his Signed-off-by - I can take care of the rest.

Nonetheless, here's what you do...

1) Make a clone of my repo:

$ git clone git://lm-sensors.org/kernel/mhoffman/hwmon-2.6.git hwmon.git

2) cd to it

$ cd hwmon.git

3) You can list my public branches like this:

$ git branch -r

(mnemonic '-r' for remote)

4) You can checkout and create a local branch which is based on a remote
branch, e.g.

$ git checkout -b testing origin/testing

At this point, you've done the "cvs co" equivalent; you can ignore git now
and create patches based on this if you prefer.

* * * * *

Although, I suppose it would be easier for you and maybe Jean if I just publish
a patch series myself.  It shouldn't be too hard to script that up; I'll look
into it.

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Hans de Goede]

Jean Delvare wrote:
> On Wed, 31 Oct 2007 09:42:38 +0100, Hans de Goede wrote:
>> Hi All,
>>
>> As discussed on the lm-sensors list, using the fixed scaling factors from the
>> fscpos datasheet for the fscher and newer models as wel, was nood a good idea,
>> as the actual scaling factors may be different and can be read from OEM DMI
>> entries.
>>
>> This patch changes the behaviour of the merged FSC familiy fschmd driver to be
>> the same as the old standalone fscher driver for fscher and newer models,
>> leaving the scaling to userspace.
>>
>> This also preserves compatibility of existing sensors.conf files.
>>
>> Signed-of-by: Hans de Goede <j.w.r.degoede@hhs.nl>
>>
>> ---
>>
>> Jean, can you review this please?
>> Mark, can you send this to Linus for 2.6.24 please, otherwise we will have one 
>> official kernel release with different scaling for the voltage inputs of the 
>> fschmd compared to the next releases.
>>
>>
>> And yes I tested my patch this time.
> 
> Assuming that you definitely excluded the possibility to retrieve the
> DMI data from the kernel

Definitely is a big word, I've looked at the DMI parser currently in the 
kernel, tried to come up with a way to get the info there which would be more 
generic then just being an fschmd specific hack and couldn't. Then I slept a 
night on it and still couldn't come up with something clean, esp since there 
are many type 185 entries in the DMI table of FSC machines, of which we need 
only one.

> and you have additional information about the
> Heimdall and Heracles also using DMI data for the scaling
That indeed has been ackknowledge by my Siemens contact.

Regards,

Hans


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Dominik Geyer]

Bruno Prémont <bonbons <at> linux-vserver.org> writes:

> 
> On a Commell LE-365 I get readings for fan speed that are broken about
> 10% of the time.
> The board has a Winbond W83697 chip (I can't check the exact chip name
> on the board as it's hidden by a heatsink, the photo from Commell, as
> as well as the manual indicate a HG, the kenrel reports:
>   [    3.572831] w83627hf: Found W83697HF chip at 0x290
>   [    3.572831] WDT driver for the Winbond(TM) W83627HF/THF/HG Super I/O 
chip initialising.
>   [    3.572831] w83627hf/thf/hg WDT: Watchdog already running. Resetting 
timeout to 60 sec
>   [    3.572831] w83627hf/thf/hg WDT: initialized. timeout` sec 
(nowayout=0)
> )

Hm, thats strange. I've got a VIA-EPIA-EK8000EG with a W83697HG as well and 
didn't have any wrong readings with fan_input (2.6.25.3 kernel).

By the way: I noticed that you are using the w83627hf_wdt in your 
kernel-config. Did you already test the watchdog? On my board the w83627hf_wdt 
loads correctly but does not work. I'm using the w83697_wdt instead. 

--
Dominik Geyer


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Frank Myhr]

Andrew Paprocki <andrew <at> ishiboo.com> writes:
> The IT8712F v0.9.1 datasheet applies to revisions >= 0x8 (J).
> The driver was incorrectly attempting to enable 16-bit fan
> readings on rev 0x7 (I) which led to incorrect RPM values.
> 
> Signed-off-by: Andrew Paprocki <andrew <at> ishiboo.com>
> Tested-by: John Gumb <john.gumb <at> tandberg.com>

The IT8712F v0.81 datasheet appears to apply to rev 0x7 (I),
and shows 16-bit fan tachs. But John's experience seems to
indicate that the datasheet is in error.


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Phillip Pi]

> > On Tue, Mar 09, 2010 at 07:11:47PM +0200, Zeev Tarantov wrote:
> > >  [   11.329457] w83627ehf: Found W83627EHG chip at 0x290
> > >  [   11.329585] ACPI: I/O resource w83627ehf [0x295-0x296] conflicts with ACPI region SEN1 [0x295-0x296]
> > >
> > >  [   11.329659] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver
> > >
> > >  Does "acpi_enforce_resources=lax" fix this for you?
> >
> > I believe so, but I was told that this isn't a good idea?
> 
> If your BIOS uses NMI to access the monitoring chip, because there is
> no locking and access to the chip is uncoordinated between the BIOS
> and the OS driver, the chip can receive strange commands, enter
> invalid states, change configuration dangerously, etc.
> The bad scenario I can come up with is that the BIOS has a feature to
> automatically control the speed of fans and the voltage of
> circuits/chips based on temperature sensors. Because the chip is
> accessed simultaneously by BIOS and OS, it returns something strange
> as a temperature reading. The BIOS then sets some bad value as voltage
> or fan speed, which might result in a crash or even physical damage to
> hardware (over-voltage, overheating, etc.).
> Another problem is the system might just spontaneously shutdown to
> protect the hardware from overheating, despite the temperatures being
> all normal, because of a strange temp reading due to simultaneous
> access to the chip.
> In laptops, power management features are more advanced and more
> necessary than in desktops. Sometimes they can't be disabled because
> the manufacturer doesn't want customers returning melted computers
> after foolishly disabling overheating protection both in the BIOS and
> in the OS.
> If the computer is a desktop, I'd consider disabling those features in
> the BIOS and relying solely on the OS and userspace tools.
> If you use the computer as is for a long time with the OS driver
> enabled and it doesn't crash and you don't see weird readings in the
> monitoring system, then I'd consider it safe; meaning your BIOS isn't
> touching anything. The computer I'm typing on has been used for two
> years that way without any issues.

Thanks. The only thing my friend and I noticed was the voltage values 
weren't correct. Everything else looked OK. It's hard to compare to 
BIOS' readings due to reboots. ;)

I do not use any automatic fan control (always at max with the third
party CPU fan, Thermaltake Silent Boost K8 A1838 model). I did use AMD's
Cool'n'Quiet and PowerNow-K8, but someone said this is OK to use with
lm_sensors.

Unless I missed options in my CMOS to disable features and to let 
lm_sensors/drivers/modules do the readings? FYI, 
http://www.msi.com/index.php?func=downloadfile&dnoA17&type=manual for 
the PDF manual copies for my old MSI K8N Neo4-F motherboard/mobo.
-- 
Quote of the Week: "This isn't a war. It never was a war, any more than
there's war between man and ants." --artilleryman from H.G. Wells' The
War of the Worlds
  /\___/\
 / /\ /\ \         Phil./Ant @ http://antfarm.ma.cx (Personal Web Site)
| |o   o| |                Ant's Quality Foraged Links: http://aqfl.net
   \ _ /                 E-mail: philpi@earthlink.net or ant@zimage.com
    ( )

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[lm-sensors]

[Zamzit]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1: Type: text/plain; charset="windows-1252http-equivContent-Type", Size: 1371 bytes --]

   ZAMZIT.com - buy it, sell it, Zamzit!
  

Dear friend,

We would like to invite you to join the all new auction site taking the web by storm. If you have tried ebay and you are fed up of paying the extortionate end of auction fees, like millions of other members, than check out http://www.zamzit.com/ - we only charge a small listing fee and no end of auction fees, you keep all of your profit.

If you register for free today you can take advantage of our promotional offer, you will receive a £15/$25 credit on your account which you can use to start selling your items. Act today as this will only be available for a limited time, click on the link below.

https://www.zamzit.com/login.php

We hope to see you at Zamzit.com soon.




 


Kindest Regards,


The Zamzit Team

 
www.zamzit.com  


To help stop Zamzit emails being seen as spam, please add us to your address book!


Copyright © 2010 Zamzit Ltd. All rights reserved.
Designated trademarks and brand are property of their respective owners.
The Zamzit logo is a trademarks of Zamzit Ltd.
To view our Terms and Conditions and Privacy Policy please visit our website.
Place of Reg: England & Wales. Reg No: 6038207. VAT Reg No: GB 902 2175 65

var googlead = "<\/script>\n\ \n\ <\/script>\n\ "; if (location.href.indexOf('http:') == 0) { document.write(googlead); }  

[-- Attachment #1.2: Type: text/html, Size: 5370 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Patch[1/2]X86:Adding_Notification_Support_to_therm_throt.c

[R, Durgadoss]

Hi,

I am submitting a patch to add core thermal threshold notification
Support to therm_throt.c. These thresholds are supported by the 
IA32_THERM_INTERRUPT register. The status/log for the same is monitored
using the IA32_THERM_STATUS register. The necessary #defines are in
msr-index.h. A call back is added to mce.h, to further notify the
thermal stack, about the threshold events.

This patch is generated against stable Linux-2.6 kernel.

Kindly review and merge.
---------------------------------------------------------------------------------
From: Durgadoss R <durgadoss.r@intel.com>

Date: Sun, 19 Dec 2010 22:42:45 +0530
Subject: [PATCH 1/2] X86:Adding_core_threshold_notification_to_therm_throt.c

This patch adds code to therm_throt.c to notify core thermal threshold
events. These thresholds are supported by the IA32_THERM_INTERRUPT register.
The status/log for the same is monitored using the IA32_THERM_STATUS register.
The necessary #defines are in msr-index.h. A call back is added to mce.h, to
further notify the thermal stack, about the threshold events.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>

---
 arch/x86/include/asm/mce.h               |    3 ++
 arch/x86/include/asm/msr-index.h         |   12 +++++++++
 arch/x86/kernel/cpu/mcheck/therm_throt.c |   40 ++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index c62c13c..eb16e94 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -223,6 +223,9 @@ void intel_init_thermal(struct cpuinfo_x86 *c);
 
 void mce_log_therm_throt_event(__u64 status);
 
+/* Interrupt Handler for core thermal thresholds */
+extern int (*platform_thermal_notify)(__u64 msr_val);
+
 #ifdef CONFIG_X86_THERMAL_VECTOR
 extern void mcheck_intel_therm_init(void);
 #else
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 3ea3dc4..a9de090 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -253,6 +253,18 @@
 #define PACKAGE_THERM_INT_LOW_ENABLE		(1 << 1)
 #define PACKAGE_THERM_INT_PLN_ENABLE		(1 << 24)
 
+/* Thermal Thresholds Support */
+#define THERM_INT_THRESHOLD0_ENABLE    (1 << 15)
+#define THERM_SHIFT_THRESHOLD0        8
+#define THERM_MASK_THRESHOLD0          (0x7f << THERM_SHIFT_THRESHOLD0)
+#define THERM_INT_THRESHOLD1_ENABLE    (1 << 23)
+#define THERM_SHIFT_THRESHOLD1        16
+#define THERM_MASK_THRESHOLD1          (0x7f << THERM_SHIFT_THRESHOLD1)
+#define THERM_STATUS_THRESHOLD0        (1 << 6)
+#define THERM_LOG_THRESHOLD0           (1 << 7)
+#define THERM_STATUS_THRESHOLD1        (1 << 8)
+#define THERM_LOG_THRESHOLD1           (1 << 9)
+
 /* MISC_ENABLE bits: architectural */
 #define MSR_IA32_MISC_ENABLE_FAST_STRING	(1ULL << 0)
 #define MSR_IA32_MISC_ENABLE_TCC		(1ULL << 1)
diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index 4b68326..e12246f 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -53,8 +53,13 @@ struct thermal_state {
 	struct _thermal_state core_power_limit;
 	struct _thermal_state package_throttle;
 	struct _thermal_state package_power_limit;
+	struct _thermal_state core_thresh0;
+	struct _thermal_state core_thresh1;
 };
 
+/* Callback to handle core threshold interrupts */
+int (*platform_thermal_notify)(__u64 msr_val);
+
 static DEFINE_PER_CPU(struct thermal_state, thermal_state);
 
 static atomic_t therm_throt_en	= ATOMIC_INIT(0);
@@ -200,6 +205,22 @@ static int therm_throt_process(bool new_event, int event, int level)
 	return 0;
 }
 
+static int thresh_event_valid(int event)
+{
+	struct _thermal_state *state;
+	unsigned int this_cpu = smp_processor_id();
+	struct thermal_state *pstate = &per_cpu(thermal_state, this_cpu);
+	u64 now = get_jiffies_64();
+
+	state = (event == 0) ? &pstate->core_thresh0 : &pstate->core_thresh1;
+
+	if (time_before64(now, state->next_check))
+		return 0;
+
+	state->next_check = now + CHECK_INTERVAL;
+	return 1;
+}
+
 #ifdef CONFIG_SYSFS
 /* Add/Remove thermal_throttle interface for CPU device: */
 static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
@@ -313,6 +334,22 @@ device_initcall(thermal_throttle_init_device);
 #define PACKAGE_THROTTLED	((__u64)2 << 62)
 #define PACKAGE_POWER_LIMIT	((__u64)3 << 62)
 
+static void notify_thresholds(__u64 msr_val)
+{
+	/* check whether the interrupt handler is defined;
+	 * otherwise simply return
+	 */
+	if (!platform_thermal_notify)
+		return;
+
+	/* lower threshold reached */
+	if ((msr_val & THERM_LOG_THRESHOLD0) &&	thresh_event_valid(0))
+		platform_thermal_notify(msr_val);
+	/* higher threshold reached */
+	if ((msr_val & THERM_LOG_THRESHOLD1) && thresh_event_valid(1))
+		platform_thermal_notify(msr_val);
+}
+
 /* Thermal transition interrupt handler */
 static void intel_thermal_interrupt(void)
 {
@@ -321,6 +358,9 @@ static void intel_thermal_interrupt(void)
 
 	rdmsrl(MSR_IA32_THERM_STATUS, msr_val);
 
+	/* Check for violation of core thermal thresholds*/
+	notify_thresholds(msr_val);
+
 	if (therm_throt_process(msr_val & THERM_STATUS_PROCHOT,
 				THERMAL_THROTTLING_EVENT,
 				CORE_LEVEL) != 0)
-- 
1.6.5.2

[lm-sensors]

[R, Durgadoss]

Hi,

I am submitting a patch to add core thermal threshold notification
Support to therm_throt.c. These thresholds are supported by the 
IA32_THERM_INTERRUPT register. The status/log for the same is monitored
using the IA32_THERM_STATUS register. The necessary #defines are in
msr-index.h. A call back is added to mce.h, to further notify the
thermal stack, about the threshold events.

This patch is generated against stable Linux-2.6 kernel.

Kindly review and merge.
---------------------------------------------------------------------------------
From: Durgadoss R <durgadoss.r@intel.com>

Date: Sun, 19 Dec 2010 22:42:45 +0530
Subject: [PATCH 1/2] X86:Adding_core_threshold_notification_to_therm_throt.c

This patch adds code to therm_throt.c to notify core thermal threshold
events. These thresholds are supported by the IA32_THERM_INTERRUPT register.
The status/log for the same is monitored using the IA32_THERM_STATUS register.
The necessary #defines are in msr-index.h. A call back is added to mce.h, to
further notify the thermal stack, about the threshold events.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>

---
 arch/x86/include/asm/mce.h               |    3 ++
 arch/x86/include/asm/msr-index.h         |   12 +++++++++
 arch/x86/kernel/cpu/mcheck/therm_throt.c |   40 ++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index c62c13c..eb16e94 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -223,6 +223,9 @@ void intel_init_thermal(struct cpuinfo_x86 *c);
 
 void mce_log_therm_throt_event(__u64 status);
 
+/* Interrupt Handler for core thermal thresholds */
+extern int (*platform_thermal_notify)(__u64 msr_val);
+
 #ifdef CONFIG_X86_THERMAL_VECTOR
 extern void mcheck_intel_therm_init(void);
 #else
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 3ea3dc4..a9de090 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -253,6 +253,18 @@
 #define PACKAGE_THERM_INT_LOW_ENABLE		(1 << 1)
 #define PACKAGE_THERM_INT_PLN_ENABLE		(1 << 24)
 
+/* Thermal Thresholds Support */
+#define THERM_INT_THRESHOLD0_ENABLE    (1 << 15)
+#define THERM_SHIFT_THRESHOLD0        8
+#define THERM_MASK_THRESHOLD0          (0x7f << THERM_SHIFT_THRESHOLD0)
+#define THERM_INT_THRESHOLD1_ENABLE    (1 << 23)
+#define THERM_SHIFT_THRESHOLD1        16
+#define THERM_MASK_THRESHOLD1          (0x7f << THERM_SHIFT_THRESHOLD1)
+#define THERM_STATUS_THRESHOLD0        (1 << 6)
+#define THERM_LOG_THRESHOLD0           (1 << 7)
+#define THERM_STATUS_THRESHOLD1        (1 << 8)
+#define THERM_LOG_THRESHOLD1           (1 << 9)
+
 /* MISC_ENABLE bits: architectural */
 #define MSR_IA32_MISC_ENABLE_FAST_STRING	(1ULL << 0)
 #define MSR_IA32_MISC_ENABLE_TCC		(1ULL << 1)
diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index 4b68326..e12246f 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -53,8 +53,13 @@ struct thermal_state {
 	struct _thermal_state core_power_limit;
 	struct _thermal_state package_throttle;
 	struct _thermal_state package_power_limit;
+	struct _thermal_state core_thresh0;
+	struct _thermal_state core_thresh1;
 };
 
+/* Callback to handle core threshold interrupts */
+int (*platform_thermal_notify)(__u64 msr_val);
+
 static DEFINE_PER_CPU(struct thermal_state, thermal_state);
 
 static atomic_t therm_throt_en	= ATOMIC_INIT(0);
@@ -200,6 +205,22 @@ static int therm_throt_process(bool new_event, int event, int level)
 	return 0;
 }
 
+static int thresh_event_valid(int event)
+{
+	struct _thermal_state *state;
+	unsigned int this_cpu = smp_processor_id();
+	struct thermal_state *pstate = &per_cpu(thermal_state, this_cpu);
+	u64 now = get_jiffies_64();
+
+	state = (event = 0) ? &pstate->core_thresh0 : &pstate->core_thresh1;
+
+	if (time_before64(now, state->next_check))
+		return 0;
+
+	state->next_check = now + CHECK_INTERVAL;
+	return 1;
+}
+
 #ifdef CONFIG_SYSFS
 /* Add/Remove thermal_throttle interface for CPU device: */
 static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
@@ -313,6 +334,22 @@ device_initcall(thermal_throttle_init_device);
 #define PACKAGE_THROTTLED	((__u64)2 << 62)
 #define PACKAGE_POWER_LIMIT	((__u64)3 << 62)
 
+static void notify_thresholds(__u64 msr_val)
+{
+	/* check whether the interrupt handler is defined;
+	 * otherwise simply return
+	 */
+	if (!platform_thermal_notify)
+		return;
+
+	/* lower threshold reached */
+	if ((msr_val & THERM_LOG_THRESHOLD0) &&	thresh_event_valid(0))
+		platform_thermal_notify(msr_val);
+	/* higher threshold reached */
+	if ((msr_val & THERM_LOG_THRESHOLD1) && thresh_event_valid(1))
+		platform_thermal_notify(msr_val);
+}
+
 /* Thermal transition interrupt handler */
 static void intel_thermal_interrupt(void)
 {
@@ -321,6 +358,9 @@ static void intel_thermal_interrupt(void)
 
 	rdmsrl(MSR_IA32_THERM_STATUS, msr_val);
 
+	/* Check for violation of core thermal thresholds*/
+	notify_thresholds(msr_val);
+
 	if (therm_throt_process(msr_val & THERM_STATUS_PROCHOT,
 				THERMAL_THROTTLING_EVENT,
 				CORE_LEVEL) != 0)
-- 
1.6.5.2


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Patch[1/2]X86:Adding_Notification_Support_to_therm_throt.c

[R, Durgadoss]

Hi,

I am submitting a patch to add core thermal threshold notification
Support to therm_throt.c. These thresholds are supported by the 
IA32_THERM_INTERRUPT register. The status/log for the same is monitored
using the IA32_THERM_STATUS register. The necessary #defines are in
msr-index.h. A call back is added to mce.h, to further notify the
thermal stack, about the threshold events.

This patch is generated against stable Linux-2.6 kernel.

Kindly review and merge.
---------------------------------------------------------------------------------
From: Durgadoss R <durgadoss.r@intel.com>

Date: Sun, 19 Dec 2010 22:42:45 +0530
Subject: [PATCH 1/2] X86:Adding_core_threshold_notification_to_therm_throt.c

This patch adds code to therm_throt.c to notify core thermal threshold
events. These thresholds are supported by the IA32_THERM_INTERRUPT register.
The status/log for the same is monitored using the IA32_THERM_STATUS register.
The necessary #defines are in msr-index.h. A call back is added to mce.h, to
further notify the thermal stack, about the threshold events.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>

---
 arch/x86/include/asm/mce.h               |    3 ++
 arch/x86/include/asm/msr-index.h         |   12 +++++++++
 arch/x86/kernel/cpu/mcheck/therm_throt.c |   40 ++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index c62c13c..eb16e94 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -223,6 +223,9 @@ void intel_init_thermal(struct cpuinfo_x86 *c);
 
 void mce_log_therm_throt_event(__u64 status);
 
+/* Interrupt Handler for core thermal thresholds */
+extern int (*platform_thermal_notify)(__u64 msr_val);
+
 #ifdef CONFIG_X86_THERMAL_VECTOR
 extern void mcheck_intel_therm_init(void);
 #else
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 3ea3dc4..a9de090 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -253,6 +253,18 @@
 #define PACKAGE_THERM_INT_LOW_ENABLE		(1 << 1)
 #define PACKAGE_THERM_INT_PLN_ENABLE		(1 << 24)
 
+/* Thermal Thresholds Support */
+#define THERM_INT_THRESHOLD0_ENABLE    (1 << 15)
+#define THERM_SHIFT_THRESHOLD0        8
+#define THERM_MASK_THRESHOLD0          (0x7f << THERM_SHIFT_THRESHOLD0)
+#define THERM_INT_THRESHOLD1_ENABLE    (1 << 23)
+#define THERM_SHIFT_THRESHOLD1        16
+#define THERM_MASK_THRESHOLD1          (0x7f << THERM_SHIFT_THRESHOLD1)
+#define THERM_STATUS_THRESHOLD0        (1 << 6)
+#define THERM_LOG_THRESHOLD0           (1 << 7)
+#define THERM_STATUS_THRESHOLD1        (1 << 8)
+#define THERM_LOG_THRESHOLD1           (1 << 9)
+
 /* MISC_ENABLE bits: architectural */
 #define MSR_IA32_MISC_ENABLE_FAST_STRING	(1ULL << 0)
 #define MSR_IA32_MISC_ENABLE_TCC		(1ULL << 1)
diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index 4b68326..e12246f 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -53,8 +53,13 @@ struct thermal_state {
 	struct _thermal_state core_power_limit;
 	struct _thermal_state package_throttle;
 	struct _thermal_state package_power_limit;
+	struct _thermal_state core_thresh0;
+	struct _thermal_state core_thresh1;
 };
 
+/* Callback to handle core threshold interrupts */
+int (*platform_thermal_notify)(__u64 msr_val);
+
 static DEFINE_PER_CPU(struct thermal_state, thermal_state);
 
 static atomic_t therm_throt_en	= ATOMIC_INIT(0);
@@ -200,6 +205,22 @@ static int therm_throt_process(bool new_event, int event, int level)
 	return 0;
 }
 
+static int thresh_event_valid(int event)
+{
+	struct _thermal_state *state;
+	unsigned int this_cpu = smp_processor_id();
+	struct thermal_state *pstate = &per_cpu(thermal_state, this_cpu);
+	u64 now = get_jiffies_64();
+
+	state = (event == 0) ? &pstate->core_thresh0 : &pstate->core_thresh1;
+
+	if (time_before64(now, state->next_check))
+		return 0;
+
+	state->next_check = now + CHECK_INTERVAL;
+	return 1;
+}
+
 #ifdef CONFIG_SYSFS
 /* Add/Remove thermal_throttle interface for CPU device: */
 static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
@@ -313,6 +334,22 @@ device_initcall(thermal_throttle_init_device);
 #define PACKAGE_THROTTLED	((__u64)2 << 62)
 #define PACKAGE_POWER_LIMIT	((__u64)3 << 62)
 
+static void notify_thresholds(__u64 msr_val)
+{
+	/* check whether the interrupt handler is defined;
+	 * otherwise simply return
+	 */
+	if (!platform_thermal_notify)
+		return;
+
+	/* lower threshold reached */
+	if ((msr_val & THERM_LOG_THRESHOLD0) &&	thresh_event_valid(0))
+		platform_thermal_notify(msr_val);
+	/* higher threshold reached */
+	if ((msr_val & THERM_LOG_THRESHOLD1) && thresh_event_valid(1))
+		platform_thermal_notify(msr_val);
+}
+
 /* Thermal transition interrupt handler */
 static void intel_thermal_interrupt(void)
 {
@@ -321,6 +358,9 @@ static void intel_thermal_interrupt(void)
 
 	rdmsrl(MSR_IA32_THERM_STATUS, msr_val);
 
+	/* Check for violation of core thermal thresholds*/
+	notify_thresholds(msr_val);
+
 	if (therm_throt_process(msr_val & THERM_STATUS_PROCHOT,
 				THERMAL_THROTTLING_EVENT,
 				CORE_LEVEL) != 0)

[lm-sensors]

[R, Durgadoss]

Hi,

I am submitting a patch to add core thermal threshold notification
Support to therm_throt.c. These thresholds are supported by the 
IA32_THERM_INTERRUPT register. The status/log for the same is monitored
using the IA32_THERM_STATUS register. The necessary #defines are in
msr-index.h. A call back is added to mce.h, to further notify the
thermal stack, about the threshold events.

This patch is generated against stable Linux-2.6 kernel.

Kindly review and merge.
---------------------------------------------------------------------------------
From: Durgadoss R <durgadoss.r@intel.com>

Date: Sun, 19 Dec 2010 22:42:45 +0530
Subject: [PATCH 1/2] X86:Adding_core_threshold_notification_to_therm_throt.c

This patch adds code to therm_throt.c to notify core thermal threshold
events. These thresholds are supported by the IA32_THERM_INTERRUPT register.
The status/log for the same is monitored using the IA32_THERM_STATUS register.
The necessary #defines are in msr-index.h. A call back is added to mce.h, to
further notify the thermal stack, about the threshold events.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>

---
 arch/x86/include/asm/mce.h               |    3 ++
 arch/x86/include/asm/msr-index.h         |   12 +++++++++
 arch/x86/kernel/cpu/mcheck/therm_throt.c |   40 ++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index c62c13c..eb16e94 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -223,6 +223,9 @@ void intel_init_thermal(struct cpuinfo_x86 *c);
 
 void mce_log_therm_throt_event(__u64 status);
 
+/* Interrupt Handler for core thermal thresholds */
+extern int (*platform_thermal_notify)(__u64 msr_val);
+
 #ifdef CONFIG_X86_THERMAL_VECTOR
 extern void mcheck_intel_therm_init(void);
 #else
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 3ea3dc4..a9de090 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -253,6 +253,18 @@
 #define PACKAGE_THERM_INT_LOW_ENABLE		(1 << 1)
 #define PACKAGE_THERM_INT_PLN_ENABLE		(1 << 24)
 
+/* Thermal Thresholds Support */
+#define THERM_INT_THRESHOLD0_ENABLE    (1 << 15)
+#define THERM_SHIFT_THRESHOLD0        8
+#define THERM_MASK_THRESHOLD0          (0x7f << THERM_SHIFT_THRESHOLD0)
+#define THERM_INT_THRESHOLD1_ENABLE    (1 << 23)
+#define THERM_SHIFT_THRESHOLD1        16
+#define THERM_MASK_THRESHOLD1          (0x7f << THERM_SHIFT_THRESHOLD1)
+#define THERM_STATUS_THRESHOLD0        (1 << 6)
+#define THERM_LOG_THRESHOLD0           (1 << 7)
+#define THERM_STATUS_THRESHOLD1        (1 << 8)
+#define THERM_LOG_THRESHOLD1           (1 << 9)
+
 /* MISC_ENABLE bits: architectural */
 #define MSR_IA32_MISC_ENABLE_FAST_STRING	(1ULL << 0)
 #define MSR_IA32_MISC_ENABLE_TCC		(1ULL << 1)
diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index 4b68326..e12246f 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -53,8 +53,13 @@ struct thermal_state {
 	struct _thermal_state core_power_limit;
 	struct _thermal_state package_throttle;
 	struct _thermal_state package_power_limit;
+	struct _thermal_state core_thresh0;
+	struct _thermal_state core_thresh1;
 };
 
+/* Callback to handle core threshold interrupts */
+int (*platform_thermal_notify)(__u64 msr_val);
+
 static DEFINE_PER_CPU(struct thermal_state, thermal_state);
 
 static atomic_t therm_throt_en	= ATOMIC_INIT(0);
@@ -200,6 +205,22 @@ static int therm_throt_process(bool new_event, int event, int level)
 	return 0;
 }
 
+static int thresh_event_valid(int event)
+{
+	struct _thermal_state *state;
+	unsigned int this_cpu = smp_processor_id();
+	struct thermal_state *pstate = &per_cpu(thermal_state, this_cpu);
+	u64 now = get_jiffies_64();
+
+	state = (event = 0) ? &pstate->core_thresh0 : &pstate->core_thresh1;
+
+	if (time_before64(now, state->next_check))
+		return 0;
+
+	state->next_check = now + CHECK_INTERVAL;
+	return 1;
+}
+
 #ifdef CONFIG_SYSFS
 /* Add/Remove thermal_throttle interface for CPU device: */
 static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
@@ -313,6 +334,22 @@ device_initcall(thermal_throttle_init_device);
 #define PACKAGE_THROTTLED	((__u64)2 << 62)
 #define PACKAGE_POWER_LIMIT	((__u64)3 << 62)
 
+static void notify_thresholds(__u64 msr_val)
+{
+	/* check whether the interrupt handler is defined;
+	 * otherwise simply return
+	 */
+	if (!platform_thermal_notify)
+		return;
+
+	/* lower threshold reached */
+	if ((msr_val & THERM_LOG_THRESHOLD0) &&	thresh_event_valid(0))
+		platform_thermal_notify(msr_val);
+	/* higher threshold reached */
+	if ((msr_val & THERM_LOG_THRESHOLD1) && thresh_event_valid(1))
+		platform_thermal_notify(msr_val);
+}
+
 /* Thermal transition interrupt handler */
 static void intel_thermal_interrupt(void)
 {
@@ -321,6 +358,9 @@ static void intel_thermal_interrupt(void)
 
 	rdmsrl(MSR_IA32_THERM_STATUS, msr_val);
 
+	/* Check for violation of core thermal thresholds*/
+	notify_thresholds(msr_val);
+
 	if (therm_throt_process(msr_val & THERM_STATUS_PROCHOT,
 				THERMAL_THROTTLING_EVENT,
 				CORE_LEVEL) != 0)

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Patch[1/2]X86:Adding_Notification_Support_to_therm_throt.c

[R, Durgadoss]

Hi,

I am submitting a patch to add core thermal threshold notification
Support to therm_throt.c. These thresholds are supported by the 
IA32_THERM_INTERRUPT register. The status/log for the same is monitored
using the IA32_THERM_STATUS register. The necessary #defines are in
msr-index.h. A call back is added to mce.h, to further notify the
thermal stack, about the threshold events.

This patch is generated against stable Linux-2.6 kernel.

Kindly review and merge.
---------------------------------------------------------------------------------
From: Durgadoss R <durgadoss.r@intel.com>

Date: Sun, 19 Dec 2010 22:42:45 +0530
Subject: [PATCH 1/2] X86:Adding_core_threshold_notification_to_therm_throt.c

This patch adds code to therm_throt.c to notify core thermal threshold
events. These thresholds are supported by the IA32_THERM_INTERRUPT register.
The status/log for the same is monitored using the IA32_THERM_STATUS register.
The necessary #defines are in msr-index.h. A call back is added to mce.h, to
further notify the thermal stack, about the threshold events.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>

---
 arch/x86/include/asm/mce.h               |    3 ++
 arch/x86/include/asm/msr-index.h         |   12 +++++++++
 arch/x86/kernel/cpu/mcheck/therm_throt.c |   40 ++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index c62c13c..eb16e94 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -223,6 +223,9 @@ void intel_init_thermal(struct cpuinfo_x86 *c);
 
 void mce_log_therm_throt_event(__u64 status);
 
+/* Interrupt Handler for core thermal thresholds */
+extern int (*platform_thermal_notify)(__u64 msr_val);
+
 #ifdef CONFIG_X86_THERMAL_VECTOR
 extern void mcheck_intel_therm_init(void);
 #else
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 3ea3dc4..a9de090 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -253,6 +253,18 @@
 #define PACKAGE_THERM_INT_LOW_ENABLE		(1 << 1)
 #define PACKAGE_THERM_INT_PLN_ENABLE		(1 << 24)
 
+/* Thermal Thresholds Support */
+#define THERM_INT_THRESHOLD0_ENABLE    (1 << 15)
+#define THERM_SHIFT_THRESHOLD0        8
+#define THERM_MASK_THRESHOLD0          (0x7f << THERM_SHIFT_THRESHOLD0)
+#define THERM_INT_THRESHOLD1_ENABLE    (1 << 23)
+#define THERM_SHIFT_THRESHOLD1        16
+#define THERM_MASK_THRESHOLD1          (0x7f << THERM_SHIFT_THRESHOLD1)
+#define THERM_STATUS_THRESHOLD0        (1 << 6)
+#define THERM_LOG_THRESHOLD0           (1 << 7)
+#define THERM_STATUS_THRESHOLD1        (1 << 8)
+#define THERM_LOG_THRESHOLD1           (1 << 9)
+
 /* MISC_ENABLE bits: architectural */
 #define MSR_IA32_MISC_ENABLE_FAST_STRING	(1ULL << 0)
 #define MSR_IA32_MISC_ENABLE_TCC		(1ULL << 1)
diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index 4b68326..e12246f 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -53,8 +53,13 @@ struct thermal_state {
 	struct _thermal_state core_power_limit;
 	struct _thermal_state package_throttle;
 	struct _thermal_state package_power_limit;
+	struct _thermal_state core_thresh0;
+	struct _thermal_state core_thresh1;
 };
 
+/* Callback to handle core threshold interrupts */
+int (*platform_thermal_notify)(__u64 msr_val);
+
 static DEFINE_PER_CPU(struct thermal_state, thermal_state);
 
 static atomic_t therm_throt_en	= ATOMIC_INIT(0);
@@ -200,6 +205,22 @@ static int therm_throt_process(bool new_event, int event, int level)
 	return 0;
 }
 
+static int thresh_event_valid(int event)
+{
+	struct _thermal_state *state;
+	unsigned int this_cpu = smp_processor_id();
+	struct thermal_state *pstate = &per_cpu(thermal_state, this_cpu);
+	u64 now = get_jiffies_64();
+
+	state = (event == 0) ? &pstate->core_thresh0 : &pstate->core_thresh1;
+
+	if (time_before64(now, state->next_check))
+		return 0;
+
+	state->next_check = now + CHECK_INTERVAL;
+	return 1;
+}
+
 #ifdef CONFIG_SYSFS
 /* Add/Remove thermal_throttle interface for CPU device: */
 static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
@@ -313,6 +334,22 @@ device_initcall(thermal_throttle_init_device);
 #define PACKAGE_THROTTLED	((__u64)2 << 62)
 #define PACKAGE_POWER_LIMIT	((__u64)3 << 62)
 
+static void notify_thresholds(__u64 msr_val)
+{
+	/* check whether the interrupt handler is defined;
+	 * otherwise simply return
+	 */
+	if (!platform_thermal_notify)
+		return;
+
+	/* lower threshold reached */
+	if ((msr_val & THERM_LOG_THRESHOLD0) &&	thresh_event_valid(0))
+		platform_thermal_notify(msr_val);
+	/* higher threshold reached */
+	if ((msr_val & THERM_LOG_THRESHOLD1) && thresh_event_valid(1))
+		platform_thermal_notify(msr_val);
+}
+
 /* Thermal transition interrupt handler */
 static void intel_thermal_interrupt(void)
 {
@@ -321,6 +358,9 @@ static void intel_thermal_interrupt(void)
 
 	rdmsrl(MSR_IA32_THERM_STATUS, msr_val);
 
+	/* Check for violation of core thermal thresholds*/
+	notify_thresholds(msr_val);
+
 	if (therm_throt_process(msr_val & THERM_STATUS_PROCHOT,
 				THERMAL_THROTTLING_EVENT,
 				CORE_LEVEL) != 0)
-- 
1.6.5.2


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[lm-sensors]

[R, Durgadoss]

Hi,

I am submitting a patch to add core thermal threshold notification
Support to therm_throt.c. These thresholds are supported by the 
IA32_THERM_INTERRUPT register. The status/log for the same is monitored
using the IA32_THERM_STATUS register. The necessary #defines are in
msr-index.h. A call back is added to mce.h, to further notify the
thermal stack, about the threshold events.

This patch is generated against stable Linux-2.6 kernel.

Kindly review and merge.
---------------------------------------------------------------------------------
From: Durgadoss R <durgadoss.r@intel.com>

Date: Sun, 19 Dec 2010 22:42:45 +0530
Subject: [PATCH 1/2] X86:Adding_core_threshold_notification_to_therm_throt.c

This patch adds code to therm_throt.c to notify core thermal threshold
events. These thresholds are supported by the IA32_THERM_INTERRUPT register.
The status/log for the same is monitored using the IA32_THERM_STATUS register.
The necessary #defines are in msr-index.h. A call back is added to mce.h, to
further notify the thermal stack, about the threshold events.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>

---
 arch/x86/include/asm/mce.h               |    3 ++
 arch/x86/include/asm/msr-index.h         |   12 +++++++++
 arch/x86/kernel/cpu/mcheck/therm_throt.c |   40 ++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index c62c13c..eb16e94 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -223,6 +223,9 @@ void intel_init_thermal(struct cpuinfo_x86 *c);
 
 void mce_log_therm_throt_event(__u64 status);
 
+/* Interrupt Handler for core thermal thresholds */
+extern int (*platform_thermal_notify)(__u64 msr_val);
+
 #ifdef CONFIG_X86_THERMAL_VECTOR
 extern void mcheck_intel_therm_init(void);
 #else
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 3ea3dc4..a9de090 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -253,6 +253,18 @@
 #define PACKAGE_THERM_INT_LOW_ENABLE		(1 << 1)
 #define PACKAGE_THERM_INT_PLN_ENABLE		(1 << 24)
 
+/* Thermal Thresholds Support */
+#define THERM_INT_THRESHOLD0_ENABLE    (1 << 15)
+#define THERM_SHIFT_THRESHOLD0        8
+#define THERM_MASK_THRESHOLD0          (0x7f << THERM_SHIFT_THRESHOLD0)
+#define THERM_INT_THRESHOLD1_ENABLE    (1 << 23)
+#define THERM_SHIFT_THRESHOLD1        16
+#define THERM_MASK_THRESHOLD1          (0x7f << THERM_SHIFT_THRESHOLD1)
+#define THERM_STATUS_THRESHOLD0        (1 << 6)
+#define THERM_LOG_THRESHOLD0           (1 << 7)
+#define THERM_STATUS_THRESHOLD1        (1 << 8)
+#define THERM_LOG_THRESHOLD1           (1 << 9)
+
 /* MISC_ENABLE bits: architectural */
 #define MSR_IA32_MISC_ENABLE_FAST_STRING	(1ULL << 0)
 #define MSR_IA32_MISC_ENABLE_TCC		(1ULL << 1)
diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index 4b68326..e12246f 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -53,8 +53,13 @@ struct thermal_state {
 	struct _thermal_state core_power_limit;
 	struct _thermal_state package_throttle;
 	struct _thermal_state package_power_limit;
+	struct _thermal_state core_thresh0;
+	struct _thermal_state core_thresh1;
 };
 
+/* Callback to handle core threshold interrupts */
+int (*platform_thermal_notify)(__u64 msr_val);
+
 static DEFINE_PER_CPU(struct thermal_state, thermal_state);
 
 static atomic_t therm_throt_en	= ATOMIC_INIT(0);
@@ -200,6 +205,22 @@ static int therm_throt_process(bool new_event, int event, int level)
 	return 0;
 }
 
+static int thresh_event_valid(int event)
+{
+	struct _thermal_state *state;
+	unsigned int this_cpu = smp_processor_id();
+	struct thermal_state *pstate = &per_cpu(thermal_state, this_cpu);
+	u64 now = get_jiffies_64();
+
+	state = (event = 0) ? &pstate->core_thresh0 : &pstate->core_thresh1;
+
+	if (time_before64(now, state->next_check))
+		return 0;
+
+	state->next_check = now + CHECK_INTERVAL;
+	return 1;
+}
+
 #ifdef CONFIG_SYSFS
 /* Add/Remove thermal_throttle interface for CPU device: */
 static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
@@ -313,6 +334,22 @@ device_initcall(thermal_throttle_init_device);
 #define PACKAGE_THROTTLED	((__u64)2 << 62)
 #define PACKAGE_POWER_LIMIT	((__u64)3 << 62)
 
+static void notify_thresholds(__u64 msr_val)
+{
+	/* check whether the interrupt handler is defined;
+	 * otherwise simply return
+	 */
+	if (!platform_thermal_notify)
+		return;
+
+	/* lower threshold reached */
+	if ((msr_val & THERM_LOG_THRESHOLD0) &&	thresh_event_valid(0))
+		platform_thermal_notify(msr_val);
+	/* higher threshold reached */
+	if ((msr_val & THERM_LOG_THRESHOLD1) && thresh_event_valid(1))
+		platform_thermal_notify(msr_val);
+}
+
 /* Thermal transition interrupt handler */
 static void intel_thermal_interrupt(void)
 {
@@ -321,6 +358,9 @@ static void intel_thermal_interrupt(void)
 
 	rdmsrl(MSR_IA32_THERM_STATUS, msr_val);
 
+	/* Check for violation of core thermal thresholds*/
+	notify_thresholds(msr_val);
+
 	if (therm_throt_process(msr_val & THERM_STATUS_PROCHOT,
 				THERMAL_THROTTLING_EVENT,
 				CORE_LEVEL) != 0)
-- 
1.6.5.2


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: Patch[1/2]X86:Adding_Notification_Support_to_therm_throt.c

[Guenter Roeck]

On Mon, Jan 03, 2011 at 06:52:04AM -0500, R, Durgadoss wrote:
> Hi,
> 
> I am submitting a patch to add core thermal threshold notification
> Support to therm_throt.c. These thresholds are supported by the 
> IA32_THERM_INTERRUPT register. The status/log for the same is monitored
> using the IA32_THERM_STATUS register. The necessary #defines are in
> msr-index.h. A call back is added to mce.h, to further notify the
> thermal stack, about the threshold events.
> 
> This patch is generated against stable Linux-2.6 kernel.
> 
> Kindly review and merge.
> ---------------------------------------------------------------------------------
> From: Durgadoss R <durgadoss.r@intel.com>
> 
> Date: Sun, 19 Dec 2010 22:42:45 +0530
> Subject: [PATCH 1/2] X86:Adding_core_threshold_notification_to_therm_throt.c
> 
> This patch adds code to therm_throt.c to notify core thermal threshold
> events. These thresholds are supported by the IA32_THERM_INTERRUPT register.
> The status/log for the same is monitored using the IA32_THERM_STATUS register.
> The necessary #defines are in msr-index.h. A call back is added to mce.h, to
> further notify the thermal stack, about the threshold events.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> 
If this is just a resubmit, you might instead consider trying to figure out
why none of the x86 maintainers replied. Documentation/Submit* might help,
as well as comparing your set of patches with other (accepted) patches.

If you made some changes, it would be expected to see the patch version and
to list the changes made. As it is, the headline and description of your patches
did not change for the last two (or three) submissions, so there is no easy way
to determine if anything has changed, nor is it possible to identify the most recent
version from the mailing list archive.

Guenter 

Re: [lm-sensors]

[Guenter Roeck]

On Mon, Jan 03, 2011 at 06:52:04AM -0500, R, Durgadoss wrote:
> Hi,
> 
> I am submitting a patch to add core thermal threshold notification
> Support to therm_throt.c. These thresholds are supported by the 
> IA32_THERM_INTERRUPT register. The status/log for the same is monitored
> using the IA32_THERM_STATUS register. The necessary #defines are in
> msr-index.h. A call back is added to mce.h, to further notify the
> thermal stack, about the threshold events.
> 
> This patch is generated against stable Linux-2.6 kernel.
> 
> Kindly review and merge.
> ---------------------------------------------------------------------------------
> From: Durgadoss R <durgadoss.r@intel.com>
> 
> Date: Sun, 19 Dec 2010 22:42:45 +0530
> Subject: [PATCH 1/2] X86:Adding_core_threshold_notification_to_therm_throt.c
> 
> This patch adds code to therm_throt.c to notify core thermal threshold
> events. These thresholds are supported by the IA32_THERM_INTERRUPT register.
> The status/log for the same is monitored using the IA32_THERM_STATUS register.
> The necessary #defines are in msr-index.h. A call back is added to mce.h, to
> further notify the thermal stack, about the threshold events.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> 
If this is just a resubmit, you might instead consider trying to figure out
why none of the x86 maintainers replied. Documentation/Submit* might help,
as well as comparing your set of patches with other (accepted) patches.

If you made some changes, it would be expected to see the patch version and
to list the changes made. As it is, the headline and description of your patches
did not change for the last two (or three) submissions, so there is no easy way
to determine if anything has changed, nor is it possible to identify the most recent
version from the mailing list archive.

Guenter 

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

RE: Patch[1/2]X86:Adding_Notification_Support_to_therm_throt.c

[R, Durgadoss]

Hi Guenter,

> > From: Durgadoss R <durgadoss.r@intel.com>
> >
> > Date: Sun, 19 Dec 2010 22:42:45 +0530
> > Subject: [PATCH 1/2] X86:Adding_core_threshold_notification_to_therm_throt.c
> >
> > This patch adds code to therm_throt.c to notify core thermal threshold
> > events. These thresholds are supported by the IA32_THERM_INTERRUPT register.
> > The status/log for the same is monitored using the IA32_THERM_STATUS
> register.
> > The necessary #defines are in msr-index.h. A call back is added to mce.h, to
> > further notify the thermal stack, about the threshold events.
> >
> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> >
> If this is just a resubmit, you might instead consider trying to figure out
> why none of the x86 maintainers replied. Documentation/Submit* might help,
> as well as comparing your set of patches with other (accepted) patches.
> 

This is a resubmission. I thought people might be on holidays..that's why
resubmitted today. Anyway, I shall try to figure out the reason. Thanks
for pointing it out Guenter..

Any comments on the hwmon patch 2/2 ?

Thanks,
Durga

Re: [lm-sensors]

[R, Durgadoss]

Hi Guenter,

> > From: Durgadoss R <durgadoss.r@intel.com>
> >
> > Date: Sun, 19 Dec 2010 22:42:45 +0530
> > Subject: [PATCH 1/2] X86:Adding_core_threshold_notification_to_therm_throt.c
> >
> > This patch adds code to therm_throt.c to notify core thermal threshold
> > events. These thresholds are supported by the IA32_THERM_INTERRUPT register.
> > The status/log for the same is monitored using the IA32_THERM_STATUS
> register.
> > The necessary #defines are in msr-index.h. A call back is added to mce.h, to
> > further notify the thermal stack, about the threshold events.
> >
> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> >
> If this is just a resubmit, you might instead consider trying to figure out
> why none of the x86 maintainers replied. Documentation/Submit* might help,
> as well as comparing your set of patches with other (accepted) patches.
> 

This is a resubmission. I thought people might be on holidays..that's why
resubmitted today. Anyway, I shall try to figure out the reason. Thanks
for pointing it out Guenter..

Any comments on the hwmon patch 2/2 ?

Thanks,
Durga

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: Patch[1/2]X86:Adding_Notification_Support_to_therm_throt.c

[Guenter Roeck]

On Mon, Jan 03, 2011 at 10:11:59AM -0500, R, Durgadoss wrote:
> Hi Guenter,
> 
> > > From: Durgadoss R <durgadoss.r@intel.com>
> > >
> > > Date: Sun, 19 Dec 2010 22:42:45 +0530
> > > Subject: [PATCH 1/2] X86:Adding_core_threshold_notification_to_therm_throt.c
> > >
> > > This patch adds code to therm_throt.c to notify core thermal threshold
> > > events. These thresholds are supported by the IA32_THERM_INTERRUPT register.
> > > The status/log for the same is monitored using the IA32_THERM_STATUS
> > register.
> > > The necessary #defines are in msr-index.h. A call back is added to mce.h, to
> > > further notify the thermal stack, about the threshold events.
> > >
> > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > >
> > If this is just a resubmit, you might instead consider trying to figure out
> > why none of the x86 maintainers replied. Documentation/Submit* might help,
> > as well as comparing your set of patches with other (accepted) patches.
> > 
> 
> This is a resubmission. I thought people might be on holidays..that's why
> resubmitted today. Anyway, I shall try to figure out the reason. Thanks
> for pointing it out Guenter..
> 
> Any comments on the hwmon patch 2/2 ?
> 
Not right now. Futile to spend more time on it unless the x86 changes are accepted.

Guenter

Re: [lm-sensors]

[Guenter Roeck]

On Mon, Jan 03, 2011 at 10:11:59AM -0500, R, Durgadoss wrote:
> Hi Guenter,
> 
> > > From: Durgadoss R <durgadoss.r@intel.com>
> > >
> > > Date: Sun, 19 Dec 2010 22:42:45 +0530
> > > Subject: [PATCH 1/2] X86:Adding_core_threshold_notification_to_therm_throt.c
> > >
> > > This patch adds code to therm_throt.c to notify core thermal threshold
> > > events. These thresholds are supported by the IA32_THERM_INTERRUPT register.
> > > The status/log for the same is monitored using the IA32_THERM_STATUS
> > register.
> > > The necessary #defines are in msr-index.h. A call back is added to mce.h, to
> > > further notify the thermal stack, about the threshold events.
> > >
> > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > >
> > If this is just a resubmit, you might instead consider trying to figure out
> > why none of the x86 maintainers replied. Documentation/Submit* might help,
> > as well as comparing your set of patches with other (accepted) patches.
> > 
> 
> This is a resubmission. I thought people might be on holidays..that's why
> resubmitted today. Anyway, I shall try to figure out the reason. Thanks
> for pointing it out Guenter..
> 
> Any comments on the hwmon patch 2/2 ?
> 
Not right now. Futile to spend more time on it unless the x86 changes are accepted.

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[tip:x86/hwmon] x86, hwmon: Add core threshold notification to therm_throt.c

[tip-bot for R, Durgadoss]

Commit-ID:  9e76a97efd31a08cb19d0ba12013b8fb4ad3e474
Gitweb:     http://git.kernel.org/tip/9e76a97efd31a08cb19d0ba12013b8fb4ad3e474
Author:     R, Durgadoss <durgadoss.r@intel.com>
AuthorDate: Mon, 3 Jan 2011 17:22:04 +0530
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Mon, 3 Jan 2011 08:30:30 -0800

x86, hwmon: Add core threshold notification to therm_throt.c

This patch adds code to therm_throt.c to notify core thermal threshold
events. These thresholds are supported by the IA32_THERM_INTERRUPT register.
The status/log for the same is monitored using the IA32_THERM_STATUS register.
The necessary #defines are in msr-index.h. A call back is added to mce.h, to
further notify the thermal stack, about the threshold events.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
LKML-Reference: <D6D887BA8C9DFF48B5233887EF04654105C1251710@bgsmsx502.gar.corp.intel.com>
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/include/asm/mce.h               |    3 ++
 arch/x86/include/asm/msr-index.h         |   12 +++++++++
 arch/x86/kernel/cpu/mcheck/therm_throt.c |   40 ++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index c62c13c..eb16e94 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -223,6 +223,9 @@ void intel_init_thermal(struct cpuinfo_x86 *c);
 
 void mce_log_therm_throt_event(__u64 status);
 
+/* Interrupt Handler for core thermal thresholds */
+extern int (*platform_thermal_notify)(__u64 msr_val);
+
 #ifdef CONFIG_X86_THERMAL_VECTOR
 extern void mcheck_intel_therm_init(void);
 #else
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 6b89f5e..622c80b 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -253,6 +253,18 @@
 #define PACKAGE_THERM_INT_LOW_ENABLE		(1 << 1)
 #define PACKAGE_THERM_INT_PLN_ENABLE		(1 << 24)
 
+/* Thermal Thresholds Support */
+#define THERM_INT_THRESHOLD0_ENABLE    (1 << 15)
+#define THERM_SHIFT_THRESHOLD0        8
+#define THERM_MASK_THRESHOLD0          (0x7f << THERM_SHIFT_THRESHOLD0)
+#define THERM_INT_THRESHOLD1_ENABLE    (1 << 23)
+#define THERM_SHIFT_THRESHOLD1        16
+#define THERM_MASK_THRESHOLD1          (0x7f << THERM_SHIFT_THRESHOLD1)
+#define THERM_STATUS_THRESHOLD0        (1 << 6)
+#define THERM_LOG_THRESHOLD0           (1 << 7)
+#define THERM_STATUS_THRESHOLD1        (1 << 8)
+#define THERM_LOG_THRESHOLD1           (1 << 9)
+
 /* MISC_ENABLE bits: architectural */
 #define MSR_IA32_MISC_ENABLE_FAST_STRING	(1ULL << 0)
 #define MSR_IA32_MISC_ENABLE_TCC		(1ULL << 1)
diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index 4b68326..e12246f 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -53,8 +53,13 @@ struct thermal_state {
 	struct _thermal_state core_power_limit;
 	struct _thermal_state package_throttle;
 	struct _thermal_state package_power_limit;
+	struct _thermal_state core_thresh0;
+	struct _thermal_state core_thresh1;
 };
 
+/* Callback to handle core threshold interrupts */
+int (*platform_thermal_notify)(__u64 msr_val);
+
 static DEFINE_PER_CPU(struct thermal_state, thermal_state);
 
 static atomic_t therm_throt_en	= ATOMIC_INIT(0);
@@ -200,6 +205,22 @@ static int therm_throt_process(bool new_event, int event, int level)
 	return 0;
 }
 
+static int thresh_event_valid(int event)
+{
+	struct _thermal_state *state;
+	unsigned int this_cpu = smp_processor_id();
+	struct thermal_state *pstate = &per_cpu(thermal_state, this_cpu);
+	u64 now = get_jiffies_64();
+
+	state = (event == 0) ? &pstate->core_thresh0 : &pstate->core_thresh1;
+
+	if (time_before64(now, state->next_check))
+		return 0;
+
+	state->next_check = now + CHECK_INTERVAL;
+	return 1;
+}
+
 #ifdef CONFIG_SYSFS
 /* Add/Remove thermal_throttle interface for CPU device: */
 static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
@@ -313,6 +334,22 @@ device_initcall(thermal_throttle_init_device);
 #define PACKAGE_THROTTLED	((__u64)2 << 62)
 #define PACKAGE_POWER_LIMIT	((__u64)3 << 62)
 
+static void notify_thresholds(__u64 msr_val)
+{
+	/* check whether the interrupt handler is defined;
+	 * otherwise simply return
+	 */
+	if (!platform_thermal_notify)
+		return;
+
+	/* lower threshold reached */
+	if ((msr_val & THERM_LOG_THRESHOLD0) &&	thresh_event_valid(0))
+		platform_thermal_notify(msr_val);
+	/* higher threshold reached */
+	if ((msr_val & THERM_LOG_THRESHOLD1) && thresh_event_valid(1))
+		platform_thermal_notify(msr_val);
+}
+
 /* Thermal transition interrupt handler */
 static void intel_thermal_interrupt(void)
 {
@@ -321,6 +358,9 @@ static void intel_thermal_interrupt(void)
 
 	rdmsrl(MSR_IA32_THERM_STATUS, msr_val);
 
+	/* Check for violation of core thermal thresholds*/
+	notify_thresholds(msr_val);
+
 	if (therm_throt_process(msr_val & THERM_STATUS_PROCHOT,
 				THERMAL_THROTTLING_EVENT,
 				CORE_LEVEL) != 0)

RE: [tip:x86/hwmon] x86, hwmon: Add core threshold notification to therm_throt.c

[R, Durgadoss]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5975 bytes --]

Hi Peter,
Thanks for accepting the patch.

Thanks,
Durga
> -----Original Message-----
> From: tip tree robot [mailto:bounces.tip@hpa.at.zytor.com] On Behalf Of tip-bot
> for R, Durgadoss
> Sent: Tuesday, January 04, 2011 1:51 PM
> To: linux-tip-commits@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; R, Durgadoss; hpa@zytor.com;
> mingo@redhat.com; tglx@linutronix.de; hpa@linux.intel.com
> Subject: [tip:x86/hwmon] x86, hwmon: Add core threshold notification to
> therm_throt.c
> 
> Commit-ID:  9e76a97efd31a08cb19d0ba12013b8fb4ad3e474
> Gitweb:     http://git.kernel.org/tip/9e76a97efd31a08cb19d0ba12013b8fb4ad3e474
> Author:     R, Durgadoss <durgadoss.r@intel.com>
> AuthorDate: Mon, 3 Jan 2011 17:22:04 +0530
> Committer:  H. Peter Anvin <hpa@linux.intel.com>
> CommitDate: Mon, 3 Jan 2011 08:30:30 -0800
> 
> x86, hwmon: Add core threshold notification to therm_throt.c
> 
> This patch adds code to therm_throt.c to notify core thermal threshold
> events. These thresholds are supported by the IA32_THERM_INTERRUPT register.
> The status/log for the same is monitored using the IA32_THERM_STATUS register.
> The necessary #defines are in msr-index.h. A call back is added to mce.h, to
> further notify the thermal stack, about the threshold events.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> LKML-Reference:
> <D6D887BA8C9DFF48B5233887EF04654105C1251710@bgsmsx502.gar.corp.intel.com>
> Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
> ---
>  arch/x86/include/asm/mce.h               |    3 ++
>  arch/x86/include/asm/msr-index.h         |   12 +++++++++
>  arch/x86/kernel/cpu/mcheck/therm_throt.c |   40 ++++++++++++++++++++++++++++++
>  3 files changed, 55 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index c62c13c..eb16e94 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -223,6 +223,9 @@ void intel_init_thermal(struct cpuinfo_x86 *c);
> 
>  void mce_log_therm_throt_event(__u64 status);
> 
> +/* Interrupt Handler for core thermal thresholds */
> +extern int (*platform_thermal_notify)(__u64 msr_val);
> +
>  #ifdef CONFIG_X86_THERMAL_VECTOR
>  extern void mcheck_intel_therm_init(void);
>  #else
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-
> index.h
> index 6b89f5e..622c80b 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -253,6 +253,18 @@
>  #define PACKAGE_THERM_INT_LOW_ENABLE		(1 << 1)
>  #define PACKAGE_THERM_INT_PLN_ENABLE		(1 << 24)
> 
> +/* Thermal Thresholds Support */
> +#define THERM_INT_THRESHOLD0_ENABLE    (1 << 15)
> +#define THERM_SHIFT_THRESHOLD0        8
> +#define THERM_MASK_THRESHOLD0          (0x7f << THERM_SHIFT_THRESHOLD0)
> +#define THERM_INT_THRESHOLD1_ENABLE    (1 << 23)
> +#define THERM_SHIFT_THRESHOLD1        16
> +#define THERM_MASK_THRESHOLD1          (0x7f << THERM_SHIFT_THRESHOLD1)
> +#define THERM_STATUS_THRESHOLD0        (1 << 6)
> +#define THERM_LOG_THRESHOLD0           (1 << 7)
> +#define THERM_STATUS_THRESHOLD1        (1 << 8)
> +#define THERM_LOG_THRESHOLD1           (1 << 9)
> +
>  /* MISC_ENABLE bits: architectural */
>  #define MSR_IA32_MISC_ENABLE_FAST_STRING	(1ULL << 0)
>  #define MSR_IA32_MISC_ENABLE_TCC		(1ULL << 1)
> diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c
> b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> index 4b68326..e12246f 100644
> --- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
> +++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> @@ -53,8 +53,13 @@ struct thermal_state {
>  	struct _thermal_state core_power_limit;
>  	struct _thermal_state package_throttle;
>  	struct _thermal_state package_power_limit;
> +	struct _thermal_state core_thresh0;
> +	struct _thermal_state core_thresh1;
>  };
> 
> +/* Callback to handle core threshold interrupts */
> +int (*platform_thermal_notify)(__u64 msr_val);
> +
>  static DEFINE_PER_CPU(struct thermal_state, thermal_state);
> 
>  static atomic_t therm_throt_en	= ATOMIC_INIT(0);
> @@ -200,6 +205,22 @@ static int therm_throt_process(bool new_event, int event,
> int level)
>  	return 0;
>  }
> 
> +static int thresh_event_valid(int event)
> +{
> +	struct _thermal_state *state;
> +	unsigned int this_cpu = smp_processor_id();
> +	struct thermal_state *pstate = &per_cpu(thermal_state, this_cpu);
> +	u64 now = get_jiffies_64();
> +
> +	state = (event == 0) ? &pstate->core_thresh0 : &pstate->core_thresh1;
> +
> +	if (time_before64(now, state->next_check))
> +		return 0;
> +
> +	state->next_check = now + CHECK_INTERVAL;
> +	return 1;
> +}
> +
>  #ifdef CONFIG_SYSFS
>  /* Add/Remove thermal_throttle interface for CPU device: */
>  static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
> @@ -313,6 +334,22 @@ device_initcall(thermal_throttle_init_device);
>  #define PACKAGE_THROTTLED	((__u64)2 << 62)
>  #define PACKAGE_POWER_LIMIT	((__u64)3 << 62)
> 
> +static void notify_thresholds(__u64 msr_val)
> +{
> +	/* check whether the interrupt handler is defined;
> +	 * otherwise simply return
> +	 */
> +	if (!platform_thermal_notify)
> +		return;
> +
> +	/* lower threshold reached */
> +	if ((msr_val & THERM_LOG_THRESHOLD0) &&	thresh_event_valid(0))
> +		platform_thermal_notify(msr_val);
> +	/* higher threshold reached */
> +	if ((msr_val & THERM_LOG_THRESHOLD1) && thresh_event_valid(1))
> +		platform_thermal_notify(msr_val);
> +}
> +
>  /* Thermal transition interrupt handler */
>  static void intel_thermal_interrupt(void)
>  {
> @@ -321,6 +358,9 @@ static void intel_thermal_interrupt(void)
> 
>  	rdmsrl(MSR_IA32_THERM_STATUS, msr_val);
> 
> +	/* Check for violation of core thermal thresholds*/
> +	notify_thresholds(msr_val);
> +
>  	if (therm_throt_process(msr_val & THERM_STATUS_PROCHOT,
>  				THERMAL_THROTTLING_EVENT,
>  				CORE_LEVEL) != 0)
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: Patch[1/2]X86:Adding_Notification_Support_to_therm_throt.c

[R, Durgadoss]

Hi Guenter,

> > > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > > >
> > > If this is just a resubmit, you might instead consider trying to figure out
> > > why none of the x86 maintainers replied. Documentation/Submit* might help,
> > > as well as comparing your set of patches with other (accepted) patches.
> > >
> >
> > This is a resubmission. I thought people might be on holidays..that's why
> > resubmitted today. Anyway, I shall try to figure out the reason. Thanks
> > for pointing it out Guenter..
> >
> > Any comments on the hwmon patch 2/2 ?
> >
> Not right now. Futile to spend more time on it unless the x86 changes are
> accepted.

The x86 patch has been accepted. Please find the details below:

Commit-ID:  9e76a97efd31a08cb19d0ba12013b8fb4ad3e474
Gitweb:     http://git.kernel.org/tip/9e76a97efd31a08cb19d0ba12013b8fb4ad3e474
Author:     R, Durgadoss <durgadoss.r@intel.com>
AuthorDate: Mon, 3 Jan 2011 17:22:04 +0530
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Mon, 3 Jan 2011 08:30:30 -0800

Could you please let me know your comments on the hwmon patch[2/2] ?

Thanks,
Durga

Re: [lm-sensors]

[R, Durgadoss]

Hi Guenter,

> > > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > > >
> > > If this is just a resubmit, you might instead consider trying to figure out
> > > why none of the x86 maintainers replied. Documentation/Submit* might help,
> > > as well as comparing your set of patches with other (accepted) patches.
> > >
> >
> > This is a resubmission. I thought people might be on holidays..that's why
> > resubmitted today. Anyway, I shall try to figure out the reason. Thanks
> > for pointing it out Guenter..
> >
> > Any comments on the hwmon patch 2/2 ?
> >
> Not right now. Futile to spend more time on it unless the x86 changes are
> accepted.

The x86 patch has been accepted. Please find the details below:

Commit-ID:  9e76a97efd31a08cb19d0ba12013b8fb4ad3e474
Gitweb:     http://git.kernel.org/tip/9e76a97efd31a08cb19d0ba12013b8fb4ad3e474
Author:     R, Durgadoss <durgadoss.r@intel.com>
AuthorDate: Mon, 3 Jan 2011 17:22:04 +0530
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Mon, 3 Jan 2011 08:30:30 -0800

Could you please let me know your comments on the hwmon patch[2/2] ?

Thanks,
Durga

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Guenter Roeck]

On Thu, 2011-01-20 at 02:45 -0500, R, Durgadoss wrote:
> Hi,
> 
> This patch adds the core temperature threshold support to
> coretemp.c. These thresholds can be configured via the sysfs interfaces
> temp1_max and temp1_max_hyst. An interrupt is generated when CPU temperature
> goes above temp1_max or drops below temp1_max_hyst.
> 
> This patch needs the x86 patch, that can be downloaded
> from here:
> http://git.kernel.org/tip/9e76a97efd31a08cb19d0ba12013b8fb4ad3e474
> 
> ---------------------------------------------
> From: Durgadoss R <durgadoss.r@intel.com>
> 
> Date: Wed, 19 Jan 2011 03:52:19 +0530
> Subject: PATCH[1/1] hwmon:Adding_Threshold_support_to_coretemp
> 
> This patch adds the core temperature threshold support to
> coretemp.c. These thresholds can be configured via the sysfs interfaces
> temp1_max and temp1_max_hyst. An interrupt is generated when CPU temperature
> goes above temp1_max or drops below temp1_max_hyst.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> 
Did you try building coretemp, with this patch applied, as module ?

Guess not. platform_thermal_notify is not exported, resulting in the
following error message.

ERROR: "platform_thermal_notify" [drivers/hwmon/coretemp.ko] undefined!
make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

Guenter



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Yu, Fenghua]

> -----Original Message-----
> From: R, Durgadoss
> Sent: Wednesday, January 19, 2011 11:46 PM
> To: Yu, Fenghua; Guenter Roeck; khali@linux-fr.org
> Cc: lm-sensors@lm-sensors.org
> Subject: patch[1/1]:hwmon:Adding_Threshold_support_to_coretemp
> 
> Hi,
> 
> This patch adds the core temperature threshold support to
> coretemp.c. These thresholds can be configured via the sysfs interfaces
> temp1_max and temp1_max_hyst. An interrupt is generated when CPU
> temperature
> goes above temp1_max or drops below temp1_max_hyst.
> 
> This patch needs the x86 patch, that can be downloaded
> from here:
> http://git.kernel.org/tip/9e76a97efd31a08cb19d0ba12013b8fb4ad3e474
> 
> ---------------------------------------------
> From: Durgadoss R <durgadoss.r@intel.com>
> 
> Date: Wed, 19 Jan 2011 03:52:19 +0530
> Subject: PATCH[1/1] hwmon:Adding_Threshold_support_to_coretemp
> 
> This patch adds the core temperature threshold support to
> coretemp.c. These thresholds can be configured via the sysfs interfaces
> temp1_max and temp1_max_hyst. An interrupt is generated when CPU
> temperature
> goes above temp1_max or drops below temp1_max_hyst.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> 
> ---
>  Documentation/hwmon/coretemp |    8 ++
>  drivers/hwmon/coretemp.c     |  222
> ++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 210 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/hwmon/coretemp
> b/Documentation/hwmon/coretemp
> index 25568f8..2b89ae1 100644
> --- a/Documentation/hwmon/coretemp
> +++ b/Documentation/hwmon/coretemp
> @@ -29,6 +29,14 @@ the Out-Of-Spec bit. Following table summarizes the
> exported sysfs files:
> 
>  temp1_input	 - Core temperature (in millidegrees Celsius).
>  temp1_max	 - All cooling devices should be turned on (on Core2).
> +		   Initialized with IA32_TEMPERATURE_TARGET if supported,
> +		   otherwise initialized with (tjmax - 20). When the CPU
> +		   temperature reaches this temperature, an interrupt is
> +		   generated and temp1_max_alarm is set.
> +temp1_max_hyst	 - If the CPU temperature falls below this
> temperature,
> +		   an interrupt is generated and temp1_max_alarm is reset.
> +temp1_max_alarm - Set if the temperature reaches or exceeds temp1_max.
> +		   Reset if the temperature drops to or below
> temp1_max_hyst.
>  temp1_crit	 - Maximum junction temperature (in millidegrees Celsius).
>  temp1_crit_alarm - Set when Out-of-spec bit is set, never clears.
>  		   Correct CPU operation is no longer guaranteed.
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index 42de98d..0d09255 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -36,17 +36,21 @@
>  #include <asm/msr.h>
>  #include <asm/processor.h>
>  #include <asm/smp.h>
> +#include <asm/mce.h>
> +#include <asm/apic.h>
> 
>  #define DRVNAME	"coretemp"
> 
> -typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_TTARGET, SHOW_LABEL,
> -		SHOW_NAME } SHOW;
> +enum attributes { SHOW_TEMP, SHOW_TJMAX, CORE_TTARGET, CORE_TMIN,
> SHOW_LABEL,
> +		SHOW_NAME, SHOW_CRIT_ALARM, SHOW_MAX_ALARM };
> 
>  /*
>   * Functions declaration
>   */
> 
>  static struct coretemp_data *coretemp_update_device(struct device
> *dev);
> +static void update_alarm(struct coretemp_data *data);
> +static int set_core_threshold(struct coretemp_data *data, int temp,
> int thres);
> 
>  struct coretemp_data {
>  	struct device *hwmon_dev;
> @@ -59,7 +63,9 @@ struct coretemp_data {
>  	int temp;
>  	int tjmax;
>  	int ttarget;
> -	u8 alarm;
> +	int tmin;
> +	u8 max_alarm;
> +	u8 crit_alarm;
>  };
> 
>  /*
> @@ -83,9 +89,16 @@ static ssize_t show_name(struct device *dev, struct
> device_attribute
>  static ssize_t show_alarm(struct device *dev, struct device_attribute
>  			  *devattr, char *buf)
>  {
> -	struct coretemp_data *data = coretemp_update_device(dev);
> -	/* read the Out-of-spec log, never clear */
> -	return sprintf(buf, "%d\n", data->alarm);
> +	struct sensor_device_attribute *attr > to_sensor_dev_attr(devattr);
> +	struct coretemp_data *data = dev_get_drvdata(dev);
> +
> +	update_alarm(data);
> +	if (attr->index = SHOW_CRIT_ALARM) {
> +		/* read the Out-of-spec log, never clear */
> +		return sprintf(buf, "%d\n", data->crit_alarm);
> +	}
> +
> +	return sprintf(buf, "%d\n", data->max_alarm);
>  }
> 
>  static ssize_t show_temp(struct device *dev,
> @@ -93,33 +106,67 @@ static ssize_t show_temp(struct device *dev,
>  {
>  	struct sensor_device_attribute *attr > to_sensor_dev_attr(devattr);
>  	struct coretemp_data *data = coretemp_update_device(dev);
> -	int err;
> +	int err = -EINVAL;
> 
> -	if (attr->index = SHOW_TEMP)
> +	switch (attr->index) {
> +	case SHOW_TEMP:
>  		err = data->valid ? sprintf(buf, "%d\n", data->temp) : -
> EAGAIN;
> -	else if (attr->index = SHOW_TJMAX)
> +		break;
> +	case SHOW_TJMAX:
>  		err = sprintf(buf, "%d\n", data->tjmax);
> -	else
> +		break;
> +	case CORE_TTARGET:
>  		err = sprintf(buf, "%d\n", data->ttarget);
> +		break;
> +	case CORE_TMIN:
> +		err = sprintf(buf, "%d\n", data->tmin);
> +		break;
> +	}
> +
>  	return err;
>  }
> 
> +
> +static ssize_t store_temp(struct device *dev,
> +		struct device_attribute *devattr, const char *buf, size_t
> count)
> +{
> +	struct sensor_device_attribute *attr > to_sensor_dev_attr(devattr);
> +	struct coretemp_data *data = dev_get_drvdata(dev);
> +	unsigned long val;
> +	int err;
> +
> +	if (strict_strtoul(buf, 10, &val))
> +		return -EINVAL;
> +
> +	err = set_core_threshold(data, val, attr->index);
> +
> +	return err ? err : count;
> +}
> +
>  static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL,
>  			  SHOW_TEMP);
>  static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO, show_temp, NULL,
>  			  SHOW_TJMAX);
> -static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO, show_temp, NULL,
> -			  SHOW_TTARGET);
> -static DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL);
> +static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp,
> +						store_temp, CORE_TTARGET);
> +static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO,
> show_temp,
> +						store_temp, CORE_TMIN);
> +static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL,
> +							SHOW_CRIT_ALARM);
> +static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, show_alarm, NULL,
> +							SHOW_MAX_ALARM);
>  static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_name, NULL,
> SHOW_LABEL);
>  static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, SHOW_NAME);
> 
>  static struct attribute *coretemp_attributes[] = {
>  	&sensor_dev_attr_name.dev_attr.attr,
>  	&sensor_dev_attr_temp1_label.dev_attr.attr,
> -	&dev_attr_temp1_crit_alarm.attr,
> +	&sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
>  	&sensor_dev_attr_temp1_input.dev_attr.attr,
>  	&sensor_dev_attr_temp1_crit.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
>  	NULL
>  };
> 
> @@ -127,6 +174,26 @@ static const struct attribute_group coretemp_group
> = {
>  	.attrs = coretemp_attributes,
>  };
> 
> +static void update_alarm(struct coretemp_data *data)
> +{
> +	u32 eax, edx;
> +
> +	rdmsr_on_cpu(data->id, MSR_IA32_THERM_STATUS, &eax, &edx);
> +
> +	/* Update the critical temperature alarm */
> +	data->crit_alarm = (eax >> 5) & 1;
> +
> +	/* Temperature reached threshold1 */
> +	if (eax & THERM_LOG_THRESHOLD1)
> +		data->max_alarm = 1;
> +	/* Temperature reached threshold0 */
> +	else if (eax & THERM_LOG_THRESHOLD0)
> +		data->max_alarm = 0;
> +	/* If none of these cases, don't update max_alarm */
> +
> +	return;
> +}
> +
>  static struct coretemp_data *coretemp_update_device(struct device
> *dev)
>  {
>  	struct coretemp_data *data = dev_get_drvdata(dev);
> @@ -138,7 +205,6 @@ static struct coretemp_data
> *coretemp_update_device(struct device *dev)
> 
>  		data->valid = 0;
>  		rdmsr_on_cpu(data->id, MSR_IA32_THERM_STATUS, &eax, &edx);
> -		data->alarm = (eax >> 5) & 1;
>  		/* update only if data has been valid */
>  		if (eax & 0x80000000) {
>  			data->temp = data->tjmax - (((eax >> 16)
> @@ -298,6 +364,110 @@ static void __devinit get_ucode_rev_on_cpu(void
> *edx)
>  	rdmsr(MSR_IA32_UCODE_REV, eax, *(u32 *)edx);
>  }
> 
> +#ifdef CONFIG_X86_MCE
> +/* Platform thermal Interrupt Handler */
> +static int coretemp_interrupt(__u64 msr_val)
> +{
> +	if (msr_val & THERM_LOG_THRESHOLD0) {
> +		if (!(msr_val & THERM_STATUS_THRESHOLD0))
> +			pr_info(":Lower Threshold Reached\n");

You need to print core id to identify which core hits the interrupt.

> +		/* Reset the Threshold0 interrupt */
> +		wrmsrl(MSR_IA32_THERM_STATUS, msr_val &
> ~THERM_LOG_THRESHOLD0);
> +	}
> +
> +	if (msr_val & THERM_LOG_THRESHOLD1) {
> +		if (msr_val & THERM_STATUS_THRESHOLD1)
> +			pr_info(":Upper Threshold Reached\n");

Ditto.

> +		/* Reset the Threshold1 interrupt */
> +		wrmsrl(MSR_IA32_THERM_STATUS, msr_val &
> ~THERM_LOG_THRESHOLD1);
> +	}
> +
> +	return 0;
> +}
> +#endif
> +
> +static void configure_apic(void *info)
> +{
> +	u32 l;
> +	int *flag = (int *)info;
> +
> +	l = apic_read(APIC_LVTTHMR);
> +
> +	if (*flag)	/* Non-Zero flag Masks the APIC */
> +		apic_write(APIC_LVTTHMR, l | APIC_LVT_MASKED);
> +	else		/* Zero flag UnMasks the APIC */
> +		apic_write(APIC_LVTTHMR, l & ~APIC_LVT_MASKED);
> +}
> +
> +static int set_core_threshold(struct coretemp_data *data, int temp,
> int thres)
> +{
> +	u32 eax, edx;
> +	int diff;
> +	int flag = 1;
> +
> +	if (temp > data->tjmax)
> +		return -EINVAL;
> +
> +	mutex_lock(&data->update_lock);
> +
> +	diff = (data->tjmax - temp)/1000;
> +
> +	/* Mask the APIC */
> +	smp_call_function_single(data->id, &configure_apic, &flag, 1);
> +
> +	rdmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, &eax, &edx);
> +
> +	if (thres = CORE_TMIN) {
> +		eax = (eax & ~THERM_MASK_THRESHOLD0) |
> +					(diff << THERM_SHIFT_THRESHOLD0);
> +		data->tmin = temp;
> +	} else {
> +		eax = (eax & ~THERM_MASK_THRESHOLD1) |
> +					(diff << THERM_SHIFT_THRESHOLD1);
> +		data->ttarget = temp;
> +	}
> +
> +	wrmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, eax, edx);
> +
> +	/* Unmask the APIC */
> +	flag = 0;
> +	smp_call_function_single(data->id, &configure_apic, &flag, 1);
> +
> +	mutex_unlock(&data->update_lock);
> +	return 0;
> +}
> +
> +static int config_thresh_intrpt(struct coretemp_data *data, int
> enable)
> +{
> +	u32 eax, edx;
> +	int flag = 1; /* Non-Zero Flag masks the apic */
> +
> +	smp_call_function_single(data->id, &configure_apic, &flag, 1);
> +
> +	rdmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, &eax, &edx);
> +
> +	if (enable) {
> +		eax |= (THERM_INT_THRESHOLD0_ENABLE |
> +						THERM_INT_THRESHOLD1_ENABLE);
> +	} else {
> +		eax &= (~(THERM_INT_THRESHOLD0_ENABLE |
> +						THERM_INT_THRESHOLD1_ENABLE));
> +	}
> +
> +	wrmsr_on_cpu(data->id, MSR_IA32_THERM_INTERRUPT, eax, edx);
> +
> +	flag = 0; /*Flag should be zero to unmask the apic */
> +	smp_call_function_single(data->id, &configure_apic, &flag, 1);
> +
> +#ifdef CONFIG_X86_MCE
> +	if (enable)
> +		platform_thermal_notify = coretemp_interrupt;
> +	else
> +		platform_thermal_notify = NULL;
> +#endif

You need a lock to protect platform_thermal_notify. There is a race condition b/w notify_thresholds() and this function. If notify_thresholds() is calling platform_thermal_notify while this function is called, system will be crashed.

> +	return 0;
> +}
> +
>  static int __devinit coretemp_probe(struct platform_device *pdev)
>  {
>  	struct coretemp_data *data;
> @@ -351,6 +521,10 @@ static int __devinit coretemp_probe(struct
> platform_device *pdev)
>  	}
> 
>  	data->tjmax = get_tjmax(c, data->id, &pdev->dev);
> +	/* Initialize ttarget value. If IA32_TEMPERATURE_TARGET is
> +	 * supported, this value will be overwritten below
> +	 */
> +	data->ttarget = data->tjmax - 20000;
>  	platform_set_drvdata(pdev, data);
> 
>  	/*
> @@ -368,13 +542,21 @@ static int __devinit coretemp_probe(struct
> platform_device *pdev)
>  		} else {
>  			data->ttarget = data->tjmax -
>  					(((eax >> 8) & 0xff) * 1000);
> -			err = device_create_file(&pdev->dev,
> -					&sensor_dev_attr_temp1_max.dev_attr);
> -			if (err)
> -				goto exit_free;
>  		}
>  	}
> 
> +	/* Threshold support is available if the ACPI flag (bit 22) is
> set */
> +	if (cpu_has(c, X86_FEATURE_ACPI)) {
> +		/* Enable threshold interrupt support */
> +		config_thresh_intrpt(data, 1);
> +
> +		/* Set Initial Core thresholds.
> +		 * The lower and upper threshold values here are assumed
> +		 */
> +		set_core_threshold(data, 0, CORE_TMIN);
> +		set_core_threshold(data, data->ttarget, CORE_TTARGET);
> +	}
> +
>  	if ((err = sysfs_create_group(&pdev->dev.kobj, &coretemp_group)))
>  		goto exit_dev;
> 
> @@ -404,7 +586,7 @@ static int __devexit coretemp_remove(struct
> platform_device *pdev)
> 
>  	hwmon_device_unregister(data->hwmon_dev);
>  	sysfs_remove_group(&pdev->dev.kobj, &coretemp_group);
> -	device_remove_file(&pdev->dev,
> &sensor_dev_attr_temp1_max.dev_attr);
> +	config_thresh_intrpt(data, 0);
>  	platform_set_drvdata(pdev, NULL);
>  	kfree(data);
>  	return 0;
> --
> 1.6.5.2


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[R, Durgadoss]

Hi Fenghua,

Could you please let us know your comments on this patch ?

Thanks,
Durga

> -----Original Message-----
> From: lm-sensors-bounces@lm-sensors.org [mailto:lm-sensors-bounces@lm-
> sensors.org] On Behalf Of R, Durgadoss
> Sent: Friday, March 25, 2011 6:34 PM
> To: Guenter Roeck; lm-sensors@lm-sensors.org
> Subject: [lm-sensors] [PATCH:hwmon:v4:Resend]Merging_pkgtemp_with_coretemp
>
> Hi Guenter,
>
> This patch merges the pkgtemp driver with the coretemp driver.
> This merged driver creates one hwmon device per physical CPU i.e
> per Physical Package. Also, the sysfs interfaces per core are created
> as each core comes online and removed when it goes offline.
>
> v1 of this patch:
> Basic Merging of pkgtemp with coretemp. This creates one hwmon device
> per core.
> v2 of this patch:
> Fixed some Data structure related comments from v1. This also
> creates one hwmon device per core.
> v3 of this patch:
> This version creates one hwmon device per physical package, but
> no appropriate support for CPU hotplug.
> v4 of this patch:
> This creates one hwmon device per package.
> Added appropriate support for CONFIG_HOTPLUG_CPU.
>
> ---
> From: Durgadoss R <durgadoss.r@intel.com>
>
> Date: Thu, 3 Mar 2011 02:37:40 +0530
> Subject: [PATCH:hwmon:v4:Resend]Merging_pkgtemp_with_coretemp
>
> This patch merges the pkgtemp driver with the coretemp driver.
> This merged driver creates one hwmon device per physical CPU i.e
> per Physical Package. Also, the sysfs interfaces per core are created
> as each core comes online and removed when it goes offline.
>
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
>
> ---
>  drivers/hwmon/coretemp.c |  618 ++++++++++++++++++++++++++++++++++------------
>  1 files changed, 455 insertions(+), 163 deletions(-)
>
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index 42de98d..2667828 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -39,119 +39,140 @@
>
>  #define DRVNAME        "coretemp"
>
> -typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_TTARGET, SHOW_LABEL,
> -               SHOW_NAME } SHOW;
> +#define CORES_PER_CPU          16      /* No of Real Cores per CPU */
> +#define CORETEMP_NAME_LENGTH   17      /* String Length of attrs */
> +#define MAX_ATTRS              5       /* Maximum no of per-core attrs */
>
>  /*
> - * Functions declaration
> + * Per-Core Temperature Data
> + * @last_updated: The time when the current temperature value was updated
> + *             earlier (in jiffies).
> + * @cpu_core_id: The CPU Core from which temperature values should be read
> + *             This value is passed as "id" field to rdmsr/wrmsr functions.
> + * @status_reg: One of IA32_THERM_STATUS or IA32_PACKAGE_THERM_STATUS,
> + *             from where the temperature values should be read.
> + * @is_pkg_data: If this is 1, the temp_data holds pkgtemp data.
> + *             Otherwise, temp_data holds coretemp data.
>   */
> -
> -static struct coretemp_data *coretemp_update_device(struct device *dev);
> -
> -struct coretemp_data {
> -       struct device *hwmon_dev;
> -       struct mutex update_lock;
> -       const char *name;
> -       u32 id;
> -       u16 core_id;
> -       char valid;             /* zero until following fields are valid */
> -       unsigned long last_updated;     /* in jiffies */
> +struct temp_data {
>         int temp;
> -       int tjmax;
>         int ttarget;
> -       u8 alarm;
> +       int tjmax;
> +       u8 crit_alarm;
> +       u8 max_alarm;
> +       unsigned long last_updated;
> +       unsigned int cpu;
> +       u32 cpu_core_id;
> +       u32 status_reg;
> +       bool is_pkg_data;
> +       struct sensor_device_attribute sd_attrs[MAX_ATTRS];
> +       char attr_name[MAX_ATTRS][CORETEMP_NAME_LENGTH];
> +       struct mutex update_lock;
>  };
>
>  /*
> - * Sysfs stuff
> + * Platform Data per Physical CPU
> + * @core_count:                Number of real cores(not HT ones) in a CPU
> + * @phys_proc_id:      The physical CPU id
>   */
> +struct platform_data {
> +       struct device *hwmon_dev;
> +       int core_count;
> +       u16 phys_proc_id;
> +       struct temp_data *core_data[CORES_PER_CPU];
> +       struct device_attribute name_attr;
> +};
> +
> +/* Function Declarations */
> +static void remove_core(struct platform_data *data, struct device *dev, int
> i);
> +
> +static ssize_t show_name(struct device *dev,
> +                               struct device_attribute *devattr, char *buf)
> +{
> +       return sprintf(buf, "%s\n", DRVNAME);
> +}
>
> -static ssize_t show_name(struct device *dev, struct device_attribute
> -                         *devattr, char *buf)
> +static ssize_t show_label(struct device *dev,
> +                               struct device_attribute *devattr, char *buf)
>  {
> -       int ret;
>         struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -       struct coretemp_data *data = dev_get_drvdata(dev);
> +       struct platform_data *pdata = dev_get_drvdata(dev);
> +       struct temp_data *tdata = pdata->core_data[attr->index];
>
> -       if (attr->index = SHOW_NAME)
> -               ret = sprintf(buf, "%s\n", data->name);
> -       else    /* show label */
> -               ret = sprintf(buf, "Core %d\n", data->core_id);
> -       return ret;
> +       if (tdata->is_pkg_data)
> +               return sprintf(buf, "Physical id %d\n", pdata->phys_proc_id);
> +
> +       return sprintf(buf, "Core %d\n", tdata->cpu_core_id);
>  }
>
> -static ssize_t show_alarm(struct device *dev, struct device_attribute
> -                         *devattr, char *buf)
> +static ssize_t show_crit_alarm(struct device *dev,
> +                               struct device_attribute *devattr, char *buf)
>  {
> -       struct coretemp_data *data = coretemp_update_device(dev);
> -       /* read the Out-of-spec log, never clear */
> -       return sprintf(buf, "%d\n", data->alarm);
> +       u32 eax, edx;
> +       struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +       struct platform_data *pdata = dev_get_drvdata(dev);
> +       struct temp_data *tdata = pdata->core_data[attr->index];
> +
> +       rdmsr_on_cpu(tdata->cpu_core_id, tdata->status_reg, &eax, &edx);
> +       tdata->crit_alarm = (eax >> 5) & 1;
> +
> +       return sprintf(buf, "%d\n", tdata->crit_alarm);
>  }
>
> -static ssize_t show_temp(struct device *dev,
> -                        struct device_attribute *devattr, char *buf)
> +static ssize_t show_tjmax(struct device *dev,
> +                               struct device_attribute *devattr, char *buf)
>  {
>         struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -       struct coretemp_data *data = coretemp_update_device(dev);
> -       int err;
> +       struct platform_data *pdata = dev_get_drvdata(dev);
>
> -       if (attr->index = SHOW_TEMP)
> -               err = data->valid ? sprintf(buf, "%d\n", data->temp) : -EAGAIN;
> -       else if (attr->index = SHOW_TJMAX)
> -               err = sprintf(buf, "%d\n", data->tjmax);
> -       else
> -               err = sprintf(buf, "%d\n", data->ttarget);
> -       return err;
> +       return sprintf(buf, "%d\n", pdata->core_data[attr->index]->tjmax);
>  }
>
> -static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL,
> -                         SHOW_TEMP);
> -static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO, show_temp, NULL,
> -                         SHOW_TJMAX);
> -static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO, show_temp, NULL,
> -                         SHOW_TTARGET);
> -static DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL);
> -static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_name, NULL, SHOW_LABEL);
> -static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, SHOW_NAME);
> -
> -static struct attribute *coretemp_attributes[] = {
> -       &sensor_dev_attr_name.dev_attr.attr,
> -       &sensor_dev_attr_temp1_label.dev_attr.attr,
> -       &dev_attr_temp1_crit_alarm.attr,
> -       &sensor_dev_attr_temp1_input.dev_attr.attr,
> -       &sensor_dev_attr_temp1_crit.dev_attr.attr,
> -       NULL
> -};
> +static ssize_t show_ttarget(struct device *dev,
> +                               struct device_attribute *devattr, char *buf)
> +{
> +       struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +       struct platform_data *pdata = dev_get_drvdata(dev);
>
> -static const struct attribute_group coretemp_group = {
> -       .attrs = coretemp_attributes,
> -};
> +       return sprintf(buf, "%d\n", pdata->core_data[attr->index]->ttarget);
> +}
>
> -static struct coretemp_data *coretemp_update_device(struct device *dev)
> +static int update_curr_temp(struct temp_data *tdata, u32 eax, int tjmax)
>  {
> -       struct coretemp_data *data = dev_get_drvdata(dev);
> +       int err = -EINVAL;
>
> -       mutex_lock(&data->update_lock);
> +       mutex_lock(&tdata->update_lock);
> +       /*
> +        * Update the current temperature only if:
> +        * 1. The time interval has elapsed _and_
> +        * 2. The data is valid
> +        */
> +       if (time_after(jiffies, tdata->last_updated + HZ) &&
> +                                               (eax & 0x80000000)) {
> +               tdata->temp = tjmax - (((eax >> 16) & 0x7f) * 1000);
> +               tdata->last_updated = jiffies;
> +               err = 0;
> +       }
>
> -       if (!data->valid || time_after(jiffies, data->last_updated + HZ)) {
> -               u32 eax, edx;
> +       mutex_unlock(&tdata->update_lock);
> +       return err;
> +}
>
> -               data->valid = 0;
> -               rdmsr_on_cpu(data->id, MSR_IA32_THERM_STATUS, &eax, &edx);
> -               data->alarm = (eax >> 5) & 1;
> -               /* update only if data has been valid */
> -               if (eax & 0x80000000) {
> -                       data->temp = data->tjmax - (((eax >> 16)
> -                                                       & 0x7f) * 1000);
> -                       data->valid = 1;
> -               } else {
> -                       dev_dbg(dev, "Temperature data invalid (0x%x)\n", eax);
> -               }
> -               data->last_updated = jiffies;
> -       }
> +static ssize_t show_temp(struct device *dev,
> +                               struct device_attribute *devattr, char *buf)
> +{
> +       u32 eax, edx;
> +       struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +       struct platform_data *pdata = dev_get_drvdata(dev);
> +       struct temp_data *tdata = pdata->core_data[attr->index];
> +       int err;
>
> -       mutex_unlock(&data->update_lock);
> -       return data;
> +       rdmsr_on_cpu(tdata->cpu_core_id, tdata->status_reg, &eax, &edx);
> +       err = update_curr_temp(tdata, eax, tdata->tjmax);
> +       if (err)
> +               return err;
> +
> +       return sprintf(buf, "%d\n", tdata->temp);
>  }
>
>  static int __devinit adjust_tjmax(struct cpuinfo_x86 *c, u32 id, struct device
> *dev)
> @@ -298,115 +319,242 @@ static void __devinit get_ucode_rev_on_cpu(void *edx)
>         rdmsr(MSR_IA32_UCODE_REV, eax, *(u32 *)edx);
>  }
>
> -static int __devinit coretemp_probe(struct platform_device *pdev)
> +static int get_pkg_tjmax(int cpu, struct device *dev)
>  {
> -       struct coretemp_data *data;
> -       struct cpuinfo_x86 *c = &cpu_data(pdev->id);
> +       int default_tjmax = 100000;     /* 100 degree celsius */
>         int err;
> -       u32 eax, edx;
> +       u32 eax, edx, val;
>
> -       if (!(data = kzalloc(sizeof(struct coretemp_data), GFP_KERNEL))) {
> -               err = -ENOMEM;
> -               dev_err(&pdev->dev, "Out of memory\n");
> -               goto exit;
> +       err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET, &eax, &edx);
> +       if (!err) {
> +               val = (eax >> 16) & 0xff;
> +               if ((val > 80) && (val < 120))
> +                       return val * 1000;
>         }
> +       dev_warn(dev, "Unable to read Pkg-TjMax from CPU:%d\n", cpu);
> +       return default_tjmax;
> +}
>
> -       data->id = pdev->id;
> -#ifdef CONFIG_SMP
> -       data->core_id = c->cpu_core_id;
> -#endif
> -       data->name = "coretemp";
> -       mutex_init(&data->update_lock);
> +static int create_name_attr(struct platform_data *pdata, struct device *dev)
> +{
> +       pdata->name_attr.attr.name = "name";
> +       pdata->name_attr.attr.mode = S_IRUGO;
> +       pdata->name_attr.show = show_name;
> +       return device_create_file(dev, &pdata->name_attr);
> +}
>
> -       /* test if we can access the THERM_STATUS MSR */
> -       err = rdmsr_safe_on_cpu(data->id, MSR_IA32_THERM_STATUS, &eax, &edx);
> -       if (err) {
> -               dev_err(&pdev->dev,
> -                       "Unable to access THERM_STATUS MSR, giving up\n");
> -               goto exit_free;
> +static int create_core_attrs(struct temp_data *tdata, struct device *dev,
> +                               int attr_no)
> +{
> +       int err, i;
> +       ssize_t (*rd_ptr[MAX_ATTRS]) (struct device *dev,
> +                       struct device_attribute *devattr, char *buf) = {
> +                       show_label, show_crit_alarm, show_ttarget,
> +                       show_temp, show_tjmax };
> +       const char *names[MAX_ATTRS] = { "temp%d_label", "temp%d_crit_alarm",
> +                                       "temp%d_max", "temp%d_input",
> +                                       "temp%d_crit" };
> +
> +       /* Increment attr_no since the sysfs interfaces start with temp1_* */
> +       int sysfs_attr_no = attr_no + 1;
> +
> +       for (i = 0; i < MAX_ATTRS; i++) {
> +               snprintf(tdata->attr_name[i], CORETEMP_NAME_LENGTH, names[i],
> +                       sysfs_attr_no);
> +               tdata->sd_attrs[i].dev_attr.attr.name = tdata->attr_name[i];
> +               tdata->sd_attrs[i].dev_attr.attr.mode = S_IRUGO;
> +               tdata->sd_attrs[i].dev_attr.show = rd_ptr[i];
> +               tdata->sd_attrs[i].dev_attr.store = NULL;
> +               tdata->sd_attrs[i].index = attr_no;
> +               err = device_create_file(dev, &tdata->sd_attrs[i].dev_attr);
> +               if (err)
> +                       goto exit_free;
>         }
> +       return 0;
> +
> +exit_free:
> +       while (--i >= 0)
> +               device_remove_file(dev, &tdata->sd_attrs[i].dev_attr);
> +       return err;
> +}
> +
> +static void remove_attrs(struct device *dev, struct temp_data *tdata)
> +{
> +       int i;
>
> -       /* Check if we have problem with errata AE18 of Core processors:
> -          Readings might stop update when processor visited too deep sleep,
> -          fixed for stepping D0 (6EC).
> -       */
> +       /* Remove the sysfs attributes */
> +       for (i = 0; i < MAX_ATTRS; i++)
> +               device_remove_file(dev, &tdata->sd_attrs[i].dev_attr);
> +}
>
> +static void update_ttarget(__u8 cpu_model, struct temp_data *tdata,
> +                               struct device *dev)
> +{
> +       int err;
> +       u32 eax, edx;
> +
> +       /*
> +        * Read the still undocumented IA32_TEMPERATURE_TARGET. It exists
> +        * on older CPUs but not in this register,
> +        * Atoms don't have it either.
> +        */
> +       if ((cpu_model > 0xe) && (cpu_model != 0x1c)) {
> +               err = rdmsr_safe_on_cpu(tdata->cpu_core_id,
> +                               MSR_IA32_TEMPERATURE_TARGET, &eax, &edx);
> +               if (err) {
> +                       dev_warn(dev,
> +                       "Unable to read IA32_TEMPERATURE_TARGET MSR\n");
> +
> +               } else {
> +                       tdata->ttarget = tdata->tjmax -
> +                                       (((eax >> 8) & 0xff) * 1000);
> +               }
> +       }
> +}
> +
> +static int chk_ucode_version(struct cpuinfo_x86 *c,
> +                               struct platform_device *pdev)
> +{
> +       int err;
> +       u32 edx;
> +
> +       /*
> +        * Check if we have problem with errata AE18 of Core processors:
> +        * Readings might stop update when processor visited too deep sleep,
> +        * fixed for stepping D0 (6EC).
> +        */
>         if ((c->x86_model = 0xe) && (c->x86_mask < 0xc)) {
>                 /* check for microcode update */
> -               err = smp_call_function_single(data->id, get_ucode_rev_on_cpu,
> -                                              &edx, 1);
> +               err = smp_call_function_single(pdev->id, get_ucode_rev_on_cpu,
> +                                                               &edx, 1);
>                 if (err) {
>                         dev_err(&pdev->dev,
>                                 "Cannot determine microcode revision of "
> -                               "CPU#%u (%d)!\n", data->id, err);
> -                       err = -ENODEV;
> -                       goto exit_free;
> +                               "CPU#%u (%d)!\n", pdev->id, err);
> +                       return -ENODEV;
>                 } else if (edx < 0x39) {
> -                       err = -ENODEV;
>                         dev_err(&pdev->dev,
>                                 "Errata AE18 not fixed, update BIOS or "
>                                 "microcode of the CPU!\n");
> -                       goto exit_free;
> +                       return -ENODEV;
>                 }
>         }
> +       return 0;
> +}
>
> -       data->tjmax = get_tjmax(c, data->id, &pdev->dev);
> -       platform_set_drvdata(pdev, data);
> +static struct temp_data *init_temp_data(int cpu, int core_id, int pkg_flag)
> +{
> +       struct temp_data *tdata;
> +
> +       tdata = kzalloc(sizeof(struct temp_data), GFP_KERNEL);
> +       if (!tdata)
> +               return NULL;
> +
> +       tdata->status_reg = pkg_flag ? MSR_IA32_PACKAGE_THERM_STATUS :
> +                                                       MSR_IA32_THERM_STATUS;
> +       tdata->is_pkg_data = pkg_flag;
> +       tdata->cpu_core_id = core_id;
> +       tdata->cpu = cpu;
> +       mutex_init(&tdata->update_lock);
> +       return tdata;
> +}
>
> -       /*
> -        * read the still undocumented IA32_TEMPERATURE_TARGET. It exists
> -        * on older CPUs but not in this register,
> -        * Atoms don't have it either.
> -        */
> +static int create_core_data(struct platform_data *pdata,
> +                               struct platform_device *pdev,
> +                               unsigned int cpu, int pkg_flag)
> +{
> +       struct temp_data *tdata;
> +       struct cpuinfo_x86 *c = &cpu_data(cpu);
> +       u32 eax, edx;
> +       int err;
> +       int core_id = c->cpu_core_id;
>
> -       if ((c->x86_model > 0xe) && (c->x86_model != 0x1c)) {
> -               err = rdmsr_safe_on_cpu(data->id, MSR_IA32_TEMPERATURE_TARGET,
> -                   &eax, &edx);
> -               if (err) {
> -                       dev_warn(&pdev->dev, "Unable to read"
> -                                       " IA32_TEMPERATURE_TARGET MSR\n");
> -               } else {
> -                       data->ttarget = data->tjmax -
> -                                       (((eax >> 8) & 0xff) * 1000);
> -                       err = device_create_file(&pdev->dev,
> -                                       &sensor_dev_attr_temp1_max.dev_attr);
> -                       if (err)
> -                               goto exit_free;
> -               }
> +       tdata = init_temp_data(cpu, core_id, pkg_flag);
> +       if (!tdata)
> +               return -ENOMEM;
> +
> +       /* Test if we can access the status register */
> +       err = rdmsr_safe_on_cpu(core_id, tdata->status_reg, &eax, &edx);
> +       if (err)
> +               goto exit_free;
> +
> +       /* Create sysfs interfaces */
> +       err = create_core_attrs(tdata, &pdev->dev, pdata->core_count);
> +       if (err)
> +               goto exit_free;
> +
> +       /* Get Critical Temperature */
> +       if (pkg_flag)
> +               tdata->tjmax = get_pkg_tjmax(pdev->id, &pdev->dev);
> +       else
> +               tdata->tjmax = get_tjmax(c, core_id, &pdev->dev);
> +
> +       update_ttarget(c->x86_model, tdata, &pdev->dev);
> +
> +       pdata->core_data[pdata->core_count] = tdata;
> +       pdata->core_count++;
> +
> +       return 0;
> +exit_free:
> +       kfree(tdata);
> +       return err;
> +}
> +
> +static int __devinit coretemp_probe(struct platform_device *pdev)
> +{
> +       struct platform_data *pdata;
> +       struct cpuinfo_x86 *c = &cpu_data(pdev->id);
> +       int err;
> +
> +       /* Initialize the per-package data structures */
> +       pdata = kzalloc(sizeof(struct platform_data), GFP_KERNEL);
> +       if (!pdata) {
> +               dev_err(&pdev->dev, "Out of memory\n");
> +               return -ENOMEM;
>         }
>
> -       if ((err = sysfs_create_group(&pdev->dev.kobj, &coretemp_group)))
> -               goto exit_dev;
> +       pdata->core_count = 0;
> +       pdata->phys_proc_id = c->phys_proc_id;
>
> -       data->hwmon_dev = hwmon_device_register(&pdev->dev);
> -       if (IS_ERR(data->hwmon_dev)) {
> -               err = PTR_ERR(data->hwmon_dev);
> -               dev_err(&pdev->dev, "Class registration failed (%d)\n",
> -                       err);
> -               goto exit_class;
> +       err = create_name_attr(pdata, &pdev->dev);
> +       if (err)
> +               goto exit_free;
> +
> +       /* Check the microcode version of the CPU */
> +       err = chk_ucode_version(c, pdev);
> +       if (err)
> +               goto exit_name;
> +
> +       platform_set_drvdata(pdev, pdata);
> +
> +       pdata->hwmon_dev = hwmon_device_register(&pdev->dev);
> +       if (IS_ERR(pdata->hwmon_dev)) {
> +               err = PTR_ERR(pdata->hwmon_dev);
> +               dev_err(&pdev->dev, "Class registration failed (%d)\n", err);
> +               goto exit_name;
>         }
>
>         return 0;
>
> -exit_class:
> -       sysfs_remove_group(&pdev->dev.kobj, &coretemp_group);
> -exit_dev:
> -       device_remove_file(&pdev->dev, &sensor_dev_attr_temp1_max.dev_attr);
> +exit_name:
> +       device_remove_file(&pdev->dev, &pdata->name_attr);
>  exit_free:
> -       kfree(data);
> -exit:
> +       kfree(pdata);
>         return err;
>  }
>
>  static int __devexit coretemp_remove(struct platform_device *pdev)
>  {
> -       struct coretemp_data *data = platform_get_drvdata(pdev);
> +       struct platform_data *pdata = platform_get_drvdata(pdev);
> +       int i;
>
> -       hwmon_device_unregister(data->hwmon_dev);
> -       sysfs_remove_group(&pdev->dev.kobj, &coretemp_group);
> -       device_remove_file(&pdev->dev, &sensor_dev_attr_temp1_max.dev_attr);
> +       for (i = pdata->core_count - 1; i >= 0; --i)
> +               remove_core(pdata, &pdev->dev, i);
> +
> +       device_remove_file(&pdev->dev, &pdata->name_attr);
> +       hwmon_device_unregister(pdata->hwmon_dev);
>         platform_set_drvdata(pdev, NULL);
> -       kfree(data);
> +       kfree(pdata);
>         return 0;
>  }
>
> @@ -432,6 +580,96 @@ struct pdev_entry {
>  static LIST_HEAD(pdev_list);
>  static DEFINE_MUTEX(pdev_list_mutex);
>
> +static struct platform_device *get_pdev(int phys_proc_id)
> +{
> +       struct pdev_entry *p;
> +
> +       mutex_lock(&pdev_list_mutex);
> +
> +       list_for_each_entry(p, &pdev_list, list)
> +               if (p->phys_proc_id = phys_proc_id) {
> +                       mutex_unlock(&pdev_list_mutex);
> +                       return p->pdev;
> +               }
> +
> +       mutex_unlock(&pdev_list_mutex);
> +       return NULL;
> +}
> +
> +static int get_core_indx(struct platform_data *pdata, int core_id)
> +{
> +       int i;
> +
> +       for (i = 0; i < pdata->core_count; i++) {
> +               if (pdata->core_data[i]->cpu_core_id = core_id &&
> +                       !pdata->core_data[i]->is_pkg_data)
> +                       return i;
> +       }
> +       return -ENODEV;
> +}
> +
> +static void add_core(unsigned int cpu, int pkg_flag)
> +{
> +       int indx, err;
> +       struct platform_data *pdata;
> +       struct cpuinfo_x86 *c = &cpu_data(cpu);
> +       struct platform_device *pdev = get_pdev(c->phys_proc_id);
> +
> +       if (!pdev)
> +               return;
> +
> +       pdata = platform_get_drvdata(pdev);
> +
> +       /* If this core is a HT one, do not create any interfaces */
> +       indx = get_core_indx(pdata, c->cpu_core_id);
> +       if (indx >= 0) {
> +               dev_info(&pdev->dev, "A HT core (%u) is onlined\n", cpu);
> +               return;
> +       }
> +
> +       err = create_core_data(pdata, pdev, cpu, pkg_flag);
> +       if (err) {
> +               dev_err(&pdev->dev, "Onlining Core %u on Pkg %d failed\n",
> +                       cpu, c->phys_proc_id);
> +       }
> +}
> +
> +static void remove_core(struct platform_data *pdata, struct device *dev,
> +                       int indx)
> +{
> +       int i;
> +       int max = pdata->core_count - 1;
> +
> +       /* Remove all sysfs attrs for this core */
> +       remove_attrs(dev, pdata->core_data[indx]);
> +
> +       /* Shift the core_data elements */
> +       for (i = indx; i < max; i++)
> +               pdata->core_data[i] = pdata->core_data[i + 1];
> +
> +       /* Free the _last_ element */
> +       kfree(pdata->core_data[max]);
> +       pdata->core_data[max] = NULL;
> +
> +       pdata->core_count--;
> +
> +       /*
> +        * If count is _only_ one and the device is pkg temp
> +        * remove those interfaces and get rid off this 'pdev' entry
> +        * in the pdev_entry list.
> +        *
> +        * The pkg temp is alive as long as atleast one of the
> +        * cores inside the pkg is online. And it is removed
> +        * when all cores in a pkg go offline.
> +        */
> +       if (pdata->core_count = 1 && pdata->core_data[0]->is_pkg_data) {
> +               remove_attrs(dev, pdata->core_data[0]);
> +               kfree(pdata->core_data[0]);
> +               pdata->core_data[0] = NULL;
> +               pdata->core_count--;
> +       }
> +}
> +
>  static int __cpuinit coretemp_device_add(unsigned int cpu)
>  {
>         int err;
> @@ -503,28 +741,81 @@ exit:
>         return err;
>  }
>
> -static void __cpuinit coretemp_device_remove(unsigned int cpu)
> +static void __cpuinit coretemp_device_remove(int phys_proc_id)
>  {
>         struct pdev_entry *p;
> -       unsigned int i;
>
>         mutex_lock(&pdev_list_mutex);
>         list_for_each_entry(p, &pdev_list, list) {
> -               if (p->cpu != cpu)
> +               if (p->phys_proc_id != phys_proc_id)
>                         continue;
>
>                 platform_device_unregister(p->pdev);
>                 list_del(&p->list);
>                 mutex_unlock(&pdev_list_mutex);
>                 kfree(p);
> -               for_each_cpu(i, cpu_sibling_mask(cpu))
> -                       if (i != cpu && !coretemp_device_add(i))
> -                               break;
>                 return;
>         }
>         mutex_unlock(&pdev_list_mutex);
>  }
>
> +static void get_core_online(unsigned int cpu)
> +{
> +       struct cpuinfo_x86 *c = &cpu_data(cpu);
> +       struct platform_device *pdev = get_pdev(c->phys_proc_id);
> +
> +       if (!pdev) {
> +               /*
> +                * We are bringing the _first_ core in this pkg
> +                * online. So initialize per-pkg data structures and
> +                * then bring this core online.
> +                */
> +               coretemp_device_add(cpu);
> +               if (cpu_has(c, X86_FEATURE_PTS))
> +                       add_core(cpu, 1);
> +       }
> +       /*
> +        * Physical CPU device already exists.
> +        * So, just bring this core online.
> +        */
> +       add_core(cpu, 0);
> +}
> +
> +
> +static void put_core_offline(unsigned int cpu)
> +{
> +       int indx;
> +       struct platform_data *pdata;
> +       struct cpuinfo_x86 *c = &cpu_data(cpu);
> +       struct platform_device *pdev = get_pdev(c->phys_proc_id);
> +
> +       /* If the physical CPU device does not exist, just return */
> +       if (!pdev)
> +               return;
> +
> +       pdata = platform_get_drvdata(pdev);
> +
> +       indx = get_core_indx(pdata, c->cpu_core_id);
> +       if (indx < 0) {
> +               dev_info(&pdev->dev, "Core %d does not exist\n",
> +                       c->cpu_core_id);
> +               return;
> +       }
> +
> +       if (pdata->core_data[indx]->cpu = cpu)
> +               remove_core(pdata, &pdev->dev, indx);
> +       else
> +               dev_info(&pdev->dev, "A HT CPU (%u) is offlined\n", cpu);
> +
> +       /*
> +        * If all cores in this pkg are offline,
> +        * remove this device from the pdev_entry list and
> +        * do pkg level clean ups
> +        */
> +       if (pdata->core_count = 0)
> +               coretemp_device_remove(c->phys_proc_id);
> +}
> +
>  static int __cpuinit coretemp_cpu_callback(struct notifier_block *nfb,
>                                  unsigned long action, void *hcpu)
>  {
> @@ -533,10 +824,10 @@ static int __cpuinit coretemp_cpu_callback(struct
> notifier_block *nfb,
>         switch (action) {
>         case CPU_ONLINE:
>         case CPU_DOWN_FAILED:
> -               coretemp_device_add(cpu);
> +               get_core_online(cpu);
>                 break;
>         case CPU_DOWN_PREPARE:
> -               coretemp_device_remove(cpu);
> +               put_core_offline(cpu);
>                 break;
>         }
>         return NOTIFY_OK;
> @@ -546,6 +837,7 @@ static struct notifier_block coretemp_cpu_notifier
> __refdata = {
>         .notifier_call = coretemp_cpu_callback,
>  };
>
> +
>  static int __init coretemp_init(void)
>  {
>         int i, err = -ENODEV;
> @@ -559,7 +851,7 @@ static int __init coretemp_init(void)
>                 goto exit;
>
>         for_each_online_cpu(i)
> -               coretemp_device_add(i);
> +               get_core_online(i);
>
>  #ifndef CONFIG_HOTPLUG_CPU
>         if (list_empty(&pdev_list)) {
> --
> 1.7.4


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Guenter Roeck]

On Fri, Mar 25, 2011 at 09:04:29AM -0400, R, Durgadoss wrote:
> Hi Guenter,
> 
> This patch merges the pkgtemp driver with the coretemp driver.
> This merged driver creates one hwmon device per physical CPU i.e
> per Physical Package. Also, the sysfs interfaces per core are created
> as each core comes online and removed when it goes offline.
> 
> v1 of this patch:
> Basic Merging of pkgtemp with coretemp. This creates one hwmon device
> per core.
> v2 of this patch:
> Fixed some Data structure related comments from v1. This also
> creates one hwmon device per core.
> v3 of this patch:
> This version creates one hwmon device per physical package, but
> no appropriate support for CPU hotplug.
> v4 of this patch:
> This creates one hwmon device per package.
> Added appropriate support for CONFIG_HOTPLUG_CPU.
> 
> ---
> From: Durgadoss R <durgadoss.r@intel.com>
> 
> Date: Thu, 3 Mar 2011 02:37:40 +0530
> Subject: [PATCH:hwmon:v4:Resend]Merging_pkgtemp_with_coretemp
> 
> This patch merges the pkgtemp driver with the coretemp driver.
> This merged driver creates one hwmon device per physical CPU i.e
> per Physical Package. Also, the sysfs interfaces per core are created
> as each core comes online and removed when it goes offline.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> 

After browsing through the code, I concluded that dynamic addition and removal
of CPUs likely won't work.  So I did a quick test.

echo 0 >/sys/devices/system/cpu/cpu2/online
sensors

Result was as suspected:

[   84.518158] IP: [<ffffffffa00332a1>] show_label+0x21/0x70 [coretemp]
[   84.525604] PGD 42b9e0067 PUD 42cc0a067 PMD 0
[   84.530958] Oops: 0000 [#1] SMP
[   84.534870] last sysfs file: /sys/devices/platform/coretemp.0/temp4_label
[   84.542743] CPU 4
...

Oh well. I am quite sure I mentioned it before - _please_ test your code.
I am not your test group (and maybe you start to understand why it takes
so long to review your code).

Other comments below.

> ---
>  drivers/hwmon/coretemp.c |  618 ++++++++++++++++++++++++++++++++++------------
>  1 files changed, 455 insertions(+), 163 deletions(-)
> 
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index 42de98d..2667828 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -39,119 +39,140 @@
> 
>  #define DRVNAME        "coretemp"
> 
> -typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_TTARGET, SHOW_LABEL,
> -               SHOW_NAME } SHOW;
> +#define CORES_PER_CPU          16      /* No of Real Cores per CPU */
> +#define CORETEMP_NAME_LENGTH   17      /* String Length of attrs */
> +#define MAX_ATTRS              5       /* Maximum no of per-core attrs */
> 
>  /*
> - * Functions declaration
> + * Per-Core Temperature Data
> + * @last_updated: The time when the current temperature value was updated
> + *             earlier (in jiffies).
> + * @cpu_core_id: The CPU Core from which temperature values should be read
> + *             This value is passed as "id" field to rdmsr/wrmsr functions.
> + * @status_reg: One of IA32_THERM_STATUS or IA32_PACKAGE_THERM_STATUS,
> + *             from where the temperature values should be read.
> + * @is_pkg_data: If this is 1, the temp_data holds pkgtemp data.
> + *             Otherwise, temp_data holds coretemp data.
>   */
> -
> -static struct coretemp_data *coretemp_update_device(struct device *dev);
> -
> -struct coretemp_data {
> -       struct device *hwmon_dev;
> -       struct mutex update_lock;
> -       const char *name;
> -       u32 id;
> -       u16 core_id;
> -       char valid;             /* zero until following fields are valid */
> -       unsigned long last_updated;     /* in jiffies */
> +struct temp_data {
>         int temp;
> -       int tjmax;
>         int ttarget;
> -       u8 alarm;
> +       int tjmax;
> +       u8 crit_alarm;

crit_alarm is only written to, thus unnecessary.

[ question, though, is why you keep reading it. Is it expected to change ? ]

> +       u8 max_alarm;

max_alarm is not used anywhere, thus unnecessary.

> +       unsigned long last_updated;
> +       unsigned int cpu;
> +       u32 cpu_core_id;
> +       u32 status_reg;
> +       bool is_pkg_data;
> +       struct sensor_device_attribute sd_attrs[MAX_ATTRS];
> +       char attr_name[MAX_ATTRS][CORETEMP_NAME_LENGTH];
> +       struct mutex update_lock;
>  };
> 
>  /*
> - * Sysfs stuff
> + * Platform Data per Physical CPU
> + * @core_count:                Number of real cores(not HT ones) in a CPU
> + * @phys_proc_id:      The physical CPU id
>   */
> +struct platform_data {
> +       struct device *hwmon_dev;
> +       int core_count;
> +       u16 phys_proc_id;
> +       struct temp_data *core_data[CORES_PER_CPU];
> +       struct device_attribute name_attr;
> +};
> +
> +/* Function Declarations */
> +static void remove_core(struct platform_data *data, struct device *dev, int i);
> +
> +static ssize_t show_name(struct device *dev,
> +                               struct device_attribute *devattr, char *buf)
> +{
> +       return sprintf(buf, "%s\n", DRVNAME);
> +}
> 
> -static ssize_t show_name(struct device *dev, struct device_attribute
> -                         *devattr, char *buf)
> +static ssize_t show_label(struct device *dev,
> +                               struct device_attribute *devattr, char *buf)
>  {
> -       int ret;
>         struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -       struct coretemp_data *data = dev_get_drvdata(dev);
> +       struct platform_data *pdata = dev_get_drvdata(dev);
> +       struct temp_data *tdata = pdata->core_data[attr->index];
> 
> -       if (attr->index = SHOW_NAME)
> -               ret = sprintf(buf, "%s\n", data->name);
> -       else    /* show label */
> -               ret = sprintf(buf, "Core %d\n", data->core_id);
> -       return ret;
> +       if (tdata->is_pkg_data)
> +               return sprintf(buf, "Physical id %d\n", pdata->phys_proc_id);
> +
> +       return sprintf(buf, "Core %d\n", tdata->cpu_core_id);

	Both are really unsigned, so use %u.

>  }
> 
> -static ssize_t show_alarm(struct device *dev, struct device_attribute
> -                         *devattr, char *buf)
> +static ssize_t show_crit_alarm(struct device *dev,
> +                               struct device_attribute *devattr, char *buf)
>  {
> -       struct coretemp_data *data = coretemp_update_device(dev);
> -       /* read the Out-of-spec log, never clear */
> -       return sprintf(buf, "%d\n", data->alarm);
> +       u32 eax, edx;
> +       struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +       struct platform_data *pdata = dev_get_drvdata(dev);
> +       struct temp_data *tdata = pdata->core_data[attr->index];
> +
> +       rdmsr_on_cpu(tdata->cpu_core_id, tdata->status_reg, &eax, &edx);
> +       tdata->crit_alarm = (eax >> 5) & 1;
> +

Either read it once, store it, and display the stored value, or read and display it
without storing it. Storing it but never using the stored value does not make much sense.

> +       return sprintf(buf, "%d\n", tdata->crit_alarm);
>  }
> 
> -static ssize_t show_temp(struct device *dev,
> -                        struct device_attribute *devattr, char *buf)
> +static ssize_t show_tjmax(struct device *dev,
> +                               struct device_attribute *devattr, char *buf)
>  {
>         struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -       struct coretemp_data *data = coretemp_update_device(dev);
> -       int err;
> +       struct platform_data *pdata = dev_get_drvdata(dev);
> 
> -       if (attr->index = SHOW_TEMP)
> -               err = data->valid ? sprintf(buf, "%d\n", data->temp) : -EAGAIN;
> -       else if (attr->index = SHOW_TJMAX)
> -               err = sprintf(buf, "%d\n", data->tjmax);
> -       else
> -               err = sprintf(buf, "%d\n", data->ttarget);
> -       return err;
> +       return sprintf(buf, "%d\n", pdata->core_data[attr->index]->tjmax);
>  }
> 
> -static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL,
> -                         SHOW_TEMP);
> -static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO, show_temp, NULL,
> -                         SHOW_TJMAX);
> -static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO, show_temp, NULL,
> -                         SHOW_TTARGET);
> -static DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL);
> -static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_name, NULL, SHOW_LABEL);
> -static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, SHOW_NAME);
> -
> -static struct attribute *coretemp_attributes[] = {
> -       &sensor_dev_attr_name.dev_attr.attr,
> -       &sensor_dev_attr_temp1_label.dev_attr.attr,
> -       &dev_attr_temp1_crit_alarm.attr,
> -       &sensor_dev_attr_temp1_input.dev_attr.attr,
> -       &sensor_dev_attr_temp1_crit.dev_attr.attr,
> -       NULL
> -};
> +static ssize_t show_ttarget(struct device *dev,
> +                               struct device_attribute *devattr, char *buf)
> +{
> +       struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +       struct platform_data *pdata = dev_get_drvdata(dev);
> 
> -static const struct attribute_group coretemp_group = {
> -       .attrs = coretemp_attributes,
> -};
> +       return sprintf(buf, "%d\n", pdata->core_data[attr->index]->ttarget);
> +}
> 
> -static struct coretemp_data *coretemp_update_device(struct device *dev)
> +static int update_curr_temp(struct temp_data *tdata, u32 eax, int tjmax)
>  {
> -       struct coretemp_data *data = dev_get_drvdata(dev);
> +       int err = -EINVAL;
> 
> -       mutex_lock(&data->update_lock);
> +       mutex_lock(&tdata->update_lock);
> +       /*
> +        * Update the current temperature only if:
> +        * 1. The time interval has elapsed _and_
> +        * 2. The data is valid
> +        */
> +       if (time_after(jiffies, tdata->last_updated + HZ) &&
> +                                               (eax & 0x80000000)) {
> +               tdata->temp = tjmax - (((eax >> 16) & 0x7f) * 1000);
> +               tdata->last_updated = jiffies;
> +               err = 0;
> +       }
> 
> -       if (!data->valid || time_after(jiffies, data->last_updated + HZ)) {
> -               u32 eax, edx;
> +       mutex_unlock(&tdata->update_lock);
> +       return err;
> +}
> 
> -               data->valid = 0;
> -               rdmsr_on_cpu(data->id, MSR_IA32_THERM_STATUS, &eax, &edx);
> -               data->alarm = (eax >> 5) & 1;
> -               /* update only if data has been valid */
> -               if (eax & 0x80000000) {
> -                       data->temp = data->tjmax - (((eax >> 16)
> -                                                       & 0x7f) * 1000);
> -                       data->valid = 1;
> -               } else {
> -                       dev_dbg(dev, "Temperature data invalid (0x%x)\n", eax);
> -               }
> -               data->last_updated = jiffies;
> -       }
> +static ssize_t show_temp(struct device *dev,
> +                               struct device_attribute *devattr, char *buf)
> +{
> +       u32 eax, edx;
> +       struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +       struct platform_data *pdata = dev_get_drvdata(dev);
> +       struct temp_data *tdata = pdata->core_data[attr->index];
> +       int err;
> 
> -       mutex_unlock(&data->update_lock);
> -       return data;
> +       rdmsr_on_cpu(tdata->cpu_core_id, tdata->status_reg, &eax, &edx);
> +       err = update_curr_temp(tdata, eax, tdata->tjmax);

This is really odd. You _always_ read the temperature, but then only use it 
conditionally. What is the point of doing that ?

> +       if (err)
> +               return err;
> +
> +       return sprintf(buf, "%d\n", tdata->temp);
>  }
> 
>  static int __devinit adjust_tjmax(struct cpuinfo_x86 *c, u32 id, struct device *dev)
> @@ -298,115 +319,242 @@ static void __devinit get_ucode_rev_on_cpu(void *edx)
>         rdmsr(MSR_IA32_UCODE_REV, eax, *(u32 *)edx);
>  }
> 
> -static int __devinit coretemp_probe(struct platform_device *pdev)
> +static int get_pkg_tjmax(int cpu, struct device *dev)
>  {
> -       struct coretemp_data *data;
> -       struct cpuinfo_x86 *c = &cpu_data(pdev->id);
> +       int default_tjmax = 100000;     /* 100 degree celsius */
>         int err;
> -       u32 eax, edx;
> +       u32 eax, edx, val;
> 
> -       if (!(data = kzalloc(sizeof(struct coretemp_data), GFP_KERNEL))) {
> -               err = -ENOMEM;
> -               dev_err(&pdev->dev, "Out of memory\n");
> -               goto exit;
> +       err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET, &eax, &edx);
> +       if (!err) {
> +               val = (eax >> 16) & 0xff;
> +               if ((val > 80) && (val < 120))
> +                       return val * 1000;
>         }
> +       dev_warn(dev, "Unable to read Pkg-TjMax from CPU:%d\n", cpu);
> +       return default_tjmax;
> +}
> 
> -       data->id = pdev->id;
> -#ifdef CONFIG_SMP
> -       data->core_id = c->cpu_core_id;
> -#endif
> -       data->name = "coretemp";
> -       mutex_init(&data->update_lock);
> +static int create_name_attr(struct platform_data *pdata, struct device *dev)
> +{
> +       pdata->name_attr.attr.name = "name";
> +       pdata->name_attr.attr.mode = S_IRUGO;
> +       pdata->name_attr.show = show_name;
> +       return device_create_file(dev, &pdata->name_attr);
> +}
> 
> -       /* test if we can access the THERM_STATUS MSR */
> -       err = rdmsr_safe_on_cpu(data->id, MSR_IA32_THERM_STATUS, &eax, &edx);
> -       if (err) {
> -               dev_err(&pdev->dev,
> -                       "Unable to access THERM_STATUS MSR, giving up\n");
> -               goto exit_free;
> +static int create_core_attrs(struct temp_data *tdata, struct device *dev,
> +                               int attr_no)
> +{
> +       int err, i;
> +       ssize_t (*rd_ptr[MAX_ATTRS]) (struct device *dev,
> +                       struct device_attribute *devattr, char *buf) = {
> +                       show_label, show_crit_alarm, show_ttarget,
> +                       show_temp, show_tjmax };
> +       const char *names[MAX_ATTRS] = { "temp%d_label", "temp%d_crit_alarm",
> +                                       "temp%d_max", "temp%d_input",
> +                                       "temp%d_crit" };
> +
> +       /* Increment attr_no since the sysfs interfaces start with temp1_* */
> +       int sysfs_attr_no = attr_no + 1;
> +
> +       for (i = 0; i < MAX_ATTRS; i++) {
> +               snprintf(tdata->attr_name[i], CORETEMP_NAME_LENGTH, names[i],
> +                       sysfs_attr_no);
> +               tdata->sd_attrs[i].dev_attr.attr.name = tdata->attr_name[i];
> +               tdata->sd_attrs[i].dev_attr.attr.mode = S_IRUGO;
> +               tdata->sd_attrs[i].dev_attr.show = rd_ptr[i];
> +               tdata->sd_attrs[i].dev_attr.store = NULL;
> +               tdata->sd_attrs[i].index = attr_no;
> +               err = device_create_file(dev, &tdata->sd_attrs[i].dev_attr);
> +               if (err)
> +                       goto exit_free;
>         }
> +       return 0;
> +
> +exit_free:
> +       while (--i >= 0)
> +               device_remove_file(dev, &tdata->sd_attrs[i].dev_attr);
> +       return err;
> +}
> +
> +static void remove_attrs(struct device *dev, struct temp_data *tdata)
> +{
> +       int i;
> 
> -       /* Check if we have problem with errata AE18 of Core processors:
> -          Readings might stop update when processor visited too deep sleep,
> -          fixed for stepping D0 (6EC).
> -       */
> +       /* Remove the sysfs attributes */
> +       for (i = 0; i < MAX_ATTRS; i++)
> +               device_remove_file(dev, &tdata->sd_attrs[i].dev_attr);
> +}
> 
> +static void update_ttarget(__u8 cpu_model, struct temp_data *tdata,
> +                               struct device *dev)
> +{
> +       int err;
> +       u32 eax, edx;
> +
> +       /*
> +        * Read the still undocumented IA32_TEMPERATURE_TARGET. It exists
> +        * on older CPUs but not in this register,
> +        * Atoms don't have it either.
> +        */
> +       if ((cpu_model > 0xe) && (cpu_model != 0x1c)) {
> +               err = rdmsr_safe_on_cpu(tdata->cpu_core_id,
> +                               MSR_IA32_TEMPERATURE_TARGET, &eax, &edx);
> +               if (err) {
> +                       dev_warn(dev,
> +                       "Unable to read IA32_TEMPERATURE_TARGET MSR\n");
> +
Extra blank line

> +               } else {
> +                       tdata->ttarget = tdata->tjmax -
> +                                       (((eax >> 8) & 0xff) * 1000);
> +               }

{ } are unnecessary.

If you can not read ttarget, you still create tempX_max and display 0.
If ttarget is not supported, the attribute should not be created in the first place.
The old code did this; any special reason for removing it ?

> +       }
> +}
> +
> +static int chk_ucode_version(struct cpuinfo_x86 *c,
> +                               struct platform_device *pdev)
> +{
> +       int err;
> +       u32 edx;
> +
> +       /*
> +        * Check if we have problem with errata AE18 of Core processors:
> +        * Readings might stop update when processor visited too deep sleep,
> +        * fixed for stepping D0 (6EC).
> +        */
>         if ((c->x86_model = 0xe) && (c->x86_mask < 0xc)) {
>                 /* check for microcode update */
> -               err = smp_call_function_single(data->id, get_ucode_rev_on_cpu,
> -                                              &edx, 1);
> +               err = smp_call_function_single(pdev->id, get_ucode_rev_on_cpu,
> +                                                               &edx, 1);
>                 if (err) {
>                         dev_err(&pdev->dev,
>                                 "Cannot determine microcode revision of "
> -                               "CPU#%u (%d)!\n", data->id, err);
> -                       err = -ENODEV;
> -                       goto exit_free;
> +                               "CPU#%u (%d)!\n", pdev->id, err);
> +                       return -ENODEV;
>                 } else if (edx < 0x39) {
> -                       err = -ENODEV;
>                         dev_err(&pdev->dev,
>                                 "Errata AE18 not fixed, update BIOS or "
>                                 "microcode of the CPU!\n");
> -                       goto exit_free;
> +                       return -ENODEV;
>                 }
>         }
> +       return 0;
> +}
> 
> -       data->tjmax = get_tjmax(c, data->id, &pdev->dev);
> -       platform_set_drvdata(pdev, data);
> +static struct temp_data *init_temp_data(int cpu, int core_id, int pkg_flag)
> +{
> +       struct temp_data *tdata;
> +
> +       tdata = kzalloc(sizeof(struct temp_data), GFP_KERNEL);
> +       if (!tdata)
> +               return NULL;
> +
> +       tdata->status_reg = pkg_flag ? MSR_IA32_PACKAGE_THERM_STATUS :
> +                                                       MSR_IA32_THERM_STATUS;
> +       tdata->is_pkg_data = pkg_flag;
> +       tdata->cpu_core_id = core_id;
> +       tdata->cpu = cpu;
> +       mutex_init(&tdata->update_lock);
> +       return tdata;
> +}
> 
> -       /*
> -        * read the still undocumented IA32_TEMPERATURE_TARGET. It exists
> -        * on older CPUs but not in this register,
> -        * Atoms don't have it either.
> -        */
> +static int create_core_data(struct platform_data *pdata,
> +                               struct platform_device *pdev,
> +                               unsigned int cpu, int pkg_flag)
> +{
> +       struct temp_data *tdata;
> +       struct cpuinfo_x86 *c = &cpu_data(cpu);
> +       u32 eax, edx;
> +       int err;
> +       int core_id = c->cpu_core_id;
> 
You need to check here if core_count reached the number of entries in core_data[].
Otherwise we'll get a nasty surprise with the first 16-core CPU (since that CPU will
need 17 entries in core_data[], but there are only 16). 
 
Even better would be to come up with a dynamic scheme which does not depend on the number
of cores.


> -       if ((c->x86_model > 0xe) && (c->x86_model != 0x1c)) {
> -               err = rdmsr_safe_on_cpu(data->id, MSR_IA32_TEMPERATURE_TARGET,
> -                   &eax, &edx);
> -               if (err) {
> -                       dev_warn(&pdev->dev, "Unable to read"
> -                                       " IA32_TEMPERATURE_TARGET MSR\n");
> -               } else {
> -                       data->ttarget = data->tjmax -
> -                                       (((eax >> 8) & 0xff) * 1000);
> -                       err = device_create_file(&pdev->dev,
> -                                       &sensor_dev_attr_temp1_max.dev_attr);
> -                       if (err)
> -                               goto exit_free;
> -               }
> +       tdata = init_temp_data(cpu, core_id, pkg_flag);
> +       if (!tdata)
> +               return -ENOMEM;
> +
> +       /* Test if we can access the status register */
> +       err = rdmsr_safe_on_cpu(core_id, tdata->status_reg, &eax, &edx);
> +       if (err)
> +               goto exit_free;
> +
> +       /* Create sysfs interfaces */
> +       err = create_core_attrs(tdata, &pdev->dev, pdata->core_count);
> +       if (err)
> +               goto exit_free;
> +
> +       /* Get Critical Temperature */
> +       if (pkg_flag)
> +               tdata->tjmax = get_pkg_tjmax(pdev->id, &pdev->dev);
> +       else
> +               tdata->tjmax = get_tjmax(c, core_id, &pdev->dev);
> +
> +       update_ttarget(c->x86_model, tdata, &pdev->dev);
> +
> +       pdata->core_data[pdata->core_count] = tdata;
> +       pdata->core_count++;
> +
> +       return 0;
> +exit_free:
> +       kfree(tdata);
> +       return err;
> +}
> +
> +static int __devinit coretemp_probe(struct platform_device *pdev)
> +{
> +       struct platform_data *pdata;
> +       struct cpuinfo_x86 *c = &cpu_data(pdev->id);
> +       int err;
> +
> +       /* Initialize the per-package data structures */
> +       pdata = kzalloc(sizeof(struct platform_data), GFP_KERNEL);
> +       if (!pdata) {
> +               dev_err(&pdev->dev, "Out of memory\n");
> +               return -ENOMEM;
>         }
> 
> -       if ((err = sysfs_create_group(&pdev->dev.kobj, &coretemp_group)))
> -               goto exit_dev;
> +       pdata->core_count = 0;
> +       pdata->phys_proc_id = c->phys_proc_id;
> 
> -       data->hwmon_dev = hwmon_device_register(&pdev->dev);
> -       if (IS_ERR(data->hwmon_dev)) {
> -               err = PTR_ERR(data->hwmon_dev);
> -               dev_err(&pdev->dev, "Class registration failed (%d)\n",
> -                       err);
> -               goto exit_class;
> +       err = create_name_attr(pdata, &pdev->dev);
> +       if (err)
> +               goto exit_free;
> +
> +       /* Check the microcode version of the CPU */
> +       err = chk_ucode_version(c, pdev);
> +       if (err)
> +               goto exit_name;
> +
> +       platform_set_drvdata(pdev, pdata);
> +
> +       pdata->hwmon_dev = hwmon_device_register(&pdev->dev);
> +       if (IS_ERR(pdata->hwmon_dev)) {
> +               err = PTR_ERR(pdata->hwmon_dev);
> +               dev_err(&pdev->dev, "Class registration failed (%d)\n", err);
> +               goto exit_name;
>         }
> 
>         return 0;
> 
> -exit_class:
> -       sysfs_remove_group(&pdev->dev.kobj, &coretemp_group);
> -exit_dev:
> -       device_remove_file(&pdev->dev, &sensor_dev_attr_temp1_max.dev_attr);
> +exit_name:
> +       device_remove_file(&pdev->dev, &pdata->name_attr);
>  exit_free:
> -       kfree(data);
> -exit:
> +       kfree(pdata);
>         return err;
>  }
> 
>  static int __devexit coretemp_remove(struct platform_device *pdev)
>  {
> -       struct coretemp_data *data = platform_get_drvdata(pdev);
> +       struct platform_data *pdata = platform_get_drvdata(pdev);
> +       int i;
> 
> -       hwmon_device_unregister(data->hwmon_dev);
> -       sysfs_remove_group(&pdev->dev.kobj, &coretemp_group);
> -       device_remove_file(&pdev->dev, &sensor_dev_attr_temp1_max.dev_attr);
> +       for (i = pdata->core_count - 1; i >= 0; --i)
> +               remove_core(pdata, &pdev->dev, i);
> +
> +       device_remove_file(&pdev->dev, &pdata->name_attr);
> +       hwmon_device_unregister(pdata->hwmon_dev);
>         platform_set_drvdata(pdev, NULL);
> -       kfree(data);
> +       kfree(pdata);
>         return 0;
>  }
> 
> @@ -432,6 +580,96 @@ struct pdev_entry {
>  static LIST_HEAD(pdev_list);
>  static DEFINE_MUTEX(pdev_list_mutex);
> 
> +static struct platform_device *get_pdev(int phys_proc_id)
> +{

Function names should all start with coretemp_.

> +       struct pdev_entry *p;
> +
> +       mutex_lock(&pdev_list_mutex);
> +
> +       list_for_each_entry(p, &pdev_list, list)
> +               if (p->phys_proc_id = phys_proc_id) {
> +                       mutex_unlock(&pdev_list_mutex);
> +                       return p->pdev;
> +               }
> +
> +       mutex_unlock(&pdev_list_mutex);
> +       return NULL;
> +}
> +
> +static int get_core_indx(struct platform_data *pdata, int core_id)
> +{
> +       int i;
> +
> +       for (i = 0; i < pdata->core_count; i++) {
> +               if (pdata->core_data[i]->cpu_core_id = core_id &&
> +                       !pdata->core_data[i]->is_pkg_data)
> +                       return i;
> +       }
> +       return -ENODEV;
> +}
> +
> +static void add_core(unsigned int cpu, int pkg_flag)
> +{
> +       int indx, err;
> +       struct platform_data *pdata;
> +       struct cpuinfo_x86 *c = &cpu_data(cpu);
> +       struct platform_device *pdev = get_pdev(c->phys_proc_id);
> +
> +       if (!pdev)
> +               return;
> +
> +       pdata = platform_get_drvdata(pdev);
> +
> +       /* If this core is a HT one, do not create any interfaces */
> +       indx = get_core_indx(pdata, c->cpu_core_id);
> +       if (indx >= 0) {
> +               dev_info(&pdev->dev, "A HT core (%u) is onlined\n", cpu);

Is this message really necessary ? It does not provide any real value, or does it ?

> +               return;
> +       }
> +
> +       err = create_core_data(pdata, pdev, cpu, pkg_flag);
> +       if (err) {
> +               dev_err(&pdev->dev, "Onlining Core %u on Pkg %d failed\n",

	"Onlining" and "Offlining" are really odd terms. Can you find something better ?
	Adding / Removing, maybe ?

	%d --> %u. Arguable if you want to use %d everywhere, but mixing it is a bit odd.

> +                       cpu, c->phys_proc_id);
> +       }
> +}
> +
> +static void remove_core(struct platform_data *pdata, struct device *dev,
> +                       int indx)
> +{
> +       int i;
> +       int max = pdata->core_count - 1;
> +
> +       /* Remove all sysfs attrs for this core */
> +       remove_attrs(dev, pdata->core_data[indx]);
> +
> +       /* Shift the core_data elements */
> +       for (i = indx; i < max; i++)
> +               pdata->core_data[i] = pdata->core_data[i + 1];
> +
> +       /* Free the _last_ element */
> +       kfree(pdata->core_data[max]);

	Did you think this through ?
	You removed element [indx], yet you remove pdata->core_data[max]
	which you just moved to pdata->core_data[max - 1].
	Sounds like a recipe for a crash.

> +       pdata->core_data[max] = NULL;
> +
> +       pdata->core_count--;
> +
> +       /*
> +        * If count is _only_ one and the device is pkg temp
> +        * remove those interfaces and get rid off this 'pdev' entry
> +        * in the pdev_entry list.
> +        *
> +        * The pkg temp is alive as long as atleast one of the

	atleast --> at least

> +        * cores inside the pkg is online. And it is removed
> +        * when all cores in a pkg go offline.
> +        */
> +       if (pdata->core_count = 1 && pdata->core_data[0]->is_pkg_data) {
> +               remove_attrs(dev, pdata->core_data[0]);
> +               kfree(pdata->core_data[0]);
> +               pdata->core_data[0] = NULL;
> +               pdata->core_count--;
> +       }
> +}
> +
>  static int __cpuinit coretemp_device_add(unsigned int cpu)
>  {
>         int err;
> @@ -503,28 +741,81 @@ exit:
>         return err;
>  }
> 
> -static void __cpuinit coretemp_device_remove(unsigned int cpu)
> +static void __cpuinit coretemp_device_remove(int phys_proc_id)
>  {
>         struct pdev_entry *p;
> -       unsigned int i;
> 
>         mutex_lock(&pdev_list_mutex);
>         list_for_each_entry(p, &pdev_list, list) {
> -               if (p->cpu != cpu)
> +               if (p->phys_proc_id != phys_proc_id)
>                         continue;
> 
>                 platform_device_unregister(p->pdev);
>                 list_del(&p->list);
>                 mutex_unlock(&pdev_list_mutex);
>                 kfree(p);
> -               for_each_cpu(i, cpu_sibling_mask(cpu))
> -                       if (i != cpu && !coretemp_device_add(i))
> -                               break;
>                 return;
>         }
>         mutex_unlock(&pdev_list_mutex);
>  }
> 
> +static void get_core_online(unsigned int cpu)
> +{
> +       struct cpuinfo_x86 *c = &cpu_data(cpu);
> +       struct platform_device *pdev = get_pdev(c->phys_proc_id);
> +
> +       if (!pdev) {
> +               /*
> +                * We are bringing the _first_ core in this pkg
> +                * online. So initialize per-pkg data structures and
> +                * then bring this core online.
> +                */
> +               coretemp_device_add(cpu);

If the CPU has no thermal sensors, or if coretemp_device_add() fails,
you keep going anyway. Kind of odd to check for the error in add_core()
instead of not calling add_core() in the first place.

Also, even though coretemp_device_add() is now only called once per CPU package,
it still includes code which is only useful if it was called once per core.

> +               if (cpu_has(c, X86_FEATURE_PTS))
> +                       add_core(cpu, 1);
> +       }
> +       /*
> +        * Physical CPU device already exists.
> +        * So, just bring this core online.
> +        */
> +       add_core(cpu, 0);
> +}
> +
> +
> +static void put_core_offline(unsigned int cpu)
> +{
> +       int indx;
> +       struct platform_data *pdata;
> +       struct cpuinfo_x86 *c = &cpu_data(cpu);
> +       struct platform_device *pdev = get_pdev(c->phys_proc_id);
> +
> +       /* If the physical CPU device does not exist, just return */
> +       if (!pdev)
> +               return;
> +
> +       pdata = platform_get_drvdata(pdev);
> +
> +       indx = get_core_indx(pdata, c->cpu_core_id);
> +       if (indx < 0) {
> +               dev_info(&pdev->dev, "Core %d does not exist\n",
> +                       c->cpu_core_id);

I think you'll hit this for the 2nd core of HT CPUs. If so, it is not an error
condition. Also see below.

> +               return;
> +       }
> +
> +       if (pdata->core_data[indx]->cpu = cpu)
> +               remove_core(pdata, &pdev->dev, indx);
> +       else
> +               dev_info(&pdev->dev, "A HT CPU (%u) is offlined\n", cpu);
> +
	Somehow this looks fishy. Since you did not add an entry for the HT core
	in the first place, doesn't that mean that get_core_indx() will fail
	if you try to remove that non-existing entry ? 
	If so, you would get an "Core %d does not exist" error whenever a HT
	CPU is removed. Not desirable.

	The old code would add the 2nd HT core if the 1st one was taken offline.
	I don't see that happen here. Not sure if that was a good idea, so I don't
	really mind. But I don't think the code should complain if that 2nd never
	added HT core is taken offline.

> +       /*
> +        * If all cores in this pkg are offline,
> +        * remove this device from the pdev_entry list and
> +        * do pkg level clean ups
> +        */
> +       if (pdata->core_count = 0)
> +               coretemp_device_remove(c->phys_proc_id);
> +}
> +
>  static int __cpuinit coretemp_cpu_callback(struct notifier_block *nfb,
>                                  unsigned long action, void *hcpu)
>  {
> @@ -533,10 +824,10 @@ static int __cpuinit coretemp_cpu_callback(struct notifier_block *nfb,
>         switch (action) {
>         case CPU_ONLINE:
>         case CPU_DOWN_FAILED:
> -               coretemp_device_add(cpu);
> +               get_core_online(cpu);
>                 break;
>         case CPU_DOWN_PREPARE:
> -               coretemp_device_remove(cpu);
> +               put_core_offline(cpu);
>                 break;
>         }
>         return NOTIFY_OK;
> @@ -546,6 +837,7 @@ static struct notifier_block coretemp_cpu_notifier __refdata = {
>         .notifier_call = coretemp_cpu_callback,
>  };
> 
> +
>  static int __init coretemp_init(void)
>  {
>         int i, err = -ENODEV;
> @@ -559,7 +851,7 @@ static int __init coretemp_init(void)
>                 goto exit;
> 
>         for_each_online_cpu(i)
> -               coretemp_device_add(i);
> +               get_core_online(i);
> 
>  #ifndef CONFIG_HOTPLUG_CPU
>         if (list_empty(&pdev_list)) {
> --
> 1.7.4
> 



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[R, Durgadoss]

Hi Guenter,

Thanks for a detailed review and intuitive comments.

> > From: Durgadoss R <durgadoss.r@intel.com>
> >
> > Date: Thu, 3 Mar 2011 02:37:40 +0530
> > Subject: [PATCH:hwmon:v4:Resend]Merging_pkgtemp_with_coretemp
> >
> > This patch merges the pkgtemp driver with the coretemp driver.
> > This merged driver creates one hwmon device per physical CPU i.e
> > per Physical Package. Also, the sysfs interfaces per core are created
> > as each core comes online and removed when it goes offline.
> >
> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> >
> 
> After browsing through the code, I concluded that dynamic addition and removal
> of CPUs likely won't work.  So I did a quick test.
> 
> echo 0 >/sys/devices/system/cpu/cpu2/online
> sensors
> 
> Result was as suspected:
> 
> [   84.518158] IP: [<ffffffffa00332a1>] show_label+0x21/0x70 [coretemp]
> [   84.525604] PGD 42b9e0067 PUD 42cc0a067 PMD 0
> [   84.530958] Oops: 0000 [#1] SMP
> [   84.534870] last sysfs file: /sys/devices/platform/coretemp.0/temp4_label
> [   84.542743] CPU 4
> ...
> 
> Oh well. I am quite sure I mentioned it before - _please_ test your code.
> I am not your test group (and maybe you start to understand why it takes
> so long to review your code).
> 

I did test it on Corei5 and SNB. Anyway, next time when I submit this patch,
I shall attach the dmesg logs.
 
> > -       struct device *hwmon_dev;
> > -       struct mutex update_lock;
> > -       const char *name;
> > -       u32 id;
> > -       u16 core_id;
> > -       char valid;             /* zero until following fields are valid */
> > -       unsigned long last_updated;     /* in jiffies */
> > +struct temp_data {
> >         int temp;
> > -       int tjmax;
> >         int ttarget;
> > -       u8 alarm;
> > +       int tjmax;
> > +       u8 crit_alarm;
> 
> crit_alarm is only written to, thus unnecessary.
> 
> [ question, though, is why you keep reading it. Is it expected to change ? ]

When the temp1_input goes above temp1_crit, this crit_alarm is set to 1.
When the temp1_input falls below temp1_crit, this crit_alarm is reset to 0.
As you mentioned below, I don't need this variable. I can just read and display.

> 
> > +       u8 max_alarm;
> 
> max_alarm is not used anywhere, thus unnecessary.

We need this when we add the threshold(max and max_hyst) support also.
Hence I added.

> > +       if (tdata->is_pkg_data)
> > +               return sprintf(buf, "Physical id %d\n", pdata->phys_proc_id);
> > +
> > +       return sprintf(buf, "Core %d\n", tdata->cpu_core_id);
> 
> 	Both are really unsigned, so use %u.

Shall Change it to %u.

> > +static ssize_t show_crit_alarm(struct device *dev,
> > +                               struct device_attribute *devattr, char *buf)
> >  {
> > -       struct coretemp_data *data = coretemp_update_device(dev);
> > -       /* read the Out-of-spec log, never clear */
> > -       return sprintf(buf, "%d\n", data->alarm);
> > +       u32 eax, edx;
> > +       struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > +       struct platform_data *pdata = dev_get_drvdata(dev);
> > +       struct temp_data *tdata = pdata->core_data[attr->index];
> > +
> > +       rdmsr_on_cpu(tdata->cpu_core_id, tdata->status_reg, &eax, &edx);
> > +       tdata->crit_alarm = (eax >> 5) & 1;
> > +
> 
> Either read it once, store it, and display the stored value, or read and
> display it
> without storing it. Storing it but never using the stored value does not make
> much sense.

I will just read and display. Shall remove the temp1_crit variable.


> > -static struct coretemp_data *coretemp_update_device(struct device *dev)
> > +static int update_curr_temp(struct temp_data *tdata, u32 eax, int tjmax)
> >  {
> > -       struct coretemp_data *data = dev_get_drvdata(dev);
> > +       int err = -EINVAL;
> >
> > -       mutex_lock(&data->update_lock);
> > +       mutex_lock(&tdata->update_lock);
> > +       /*
> > +        * Update the current temperature only if:
> > +        * 1. The time interval has elapsed _and_
> > +        * 2. The data is valid
> > +        */
> > +       if (time_after(jiffies, tdata->last_updated + HZ) &&
> > +                                               (eax & 0x80000000)) {
> > +               tdata->temp = tjmax - (((eax >> 16) & 0x7f) * 1000);
> > +               tdata->last_updated = jiffies;
> > +               err = 0;
> > +       }
> >
> > -       if (!data->valid || time_after(jiffies, data->last_updated + HZ)) {
> > -               u32 eax, edx;
> > +       mutex_unlock(&tdata->update_lock);
> > +       return err;
> > +}
> >
> > -               data->valid = 0;
> > -               rdmsr_on_cpu(data->id, MSR_IA32_THERM_STATUS, &eax, &edx);
> > -               data->alarm = (eax >> 5) & 1;
> > -               /* update only if data has been valid */
> > -               if (eax & 0x80000000) {
> > -                       data->temp = data->tjmax - (((eax >> 16)
> > -                                                       & 0x7f) * 1000);
> > -                       data->valid = 1;
> > -               } else {
> > -                       dev_dbg(dev, "Temperature data invalid (0x%x)\n",
> eax);
> > -               }
> > -               data->last_updated = jiffies;
> > -       }
> > +static ssize_t show_temp(struct device *dev,
> > +                               struct device_attribute *devattr, char *buf)
> > +{
> > +       u32 eax, edx;
> > +       struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > +       struct platform_data *pdata = dev_get_drvdata(dev);
> > +       struct temp_data *tdata = pdata->core_data[attr->index];
> > +       int err;
> >
> > -       mutex_unlock(&data->update_lock);
> > -       return data;
> > +       rdmsr_on_cpu(tdata->cpu_core_id, tdata->status_reg, &eax, &edx);
> > +       err = update_curr_temp(tdata, eax, tdata->tjmax);
> 
> This is really odd. You _always_ read the temperature, but then only use it
> conditionally. What is the point of doing that ?

Shall Modify the flow of show_temp, such that I do the timer check before
doing/updating anything.


> > +static void update_ttarget(__u8 cpu_model, struct temp_data *tdata,
> > +                               struct device *dev)
> > +{
> > +       int err;
> > +       u32 eax, edx;
> > +
> > +       /*
> > +        * Read the still undocumented IA32_TEMPERATURE_TARGET. It exists
> > +        * on older CPUs but not in this register,
> > +        * Atoms don't have it either.
> > +        */
> > +       if ((cpu_model > 0xe) && (cpu_model != 0x1c)) {
> > +               err = rdmsr_safe_on_cpu(tdata->cpu_core_id,
> > +                               MSR_IA32_TEMPERATURE_TARGET, &eax, &edx);
> > +               if (err) {
> > +                       dev_warn(dev,
> > +                       "Unable to read IA32_TEMPERATURE_TARGET MSR\n");
> > +
> Extra blank line
> 
> > +               } else {
> > +                       tdata->ttarget = tdata->tjmax -
> > +                                       (((eax >> 8) & 0xff) * 1000);
> > +               }
> 
> { } are unnecessary.

In one of the previous patches, I had a single statement split into two lines,
Without Braces. And I got comment asking me to add braces to increase readability.
With that in mind, I added braces.
Shall remove if it is Ok to do so ;-)

> 
> If you can not read ttarget, you still create tempX_max and display 0.
> If ttarget is not supported, the attribute should not be created in the first
> place.
> The old code did this; any special reason for removing it ?

Earlier when I tried the "Adding Threshold Support to Coretemp.patch",
We discussed that temp1_max will have the Threshold1 value if ttarget is not supported.
When we have the Threshold patch all _max things will be fixed.

But if you say that will not work or not the right way to do, I can change
the code accordingly.

> > +static int create_core_data(struct platform_data *pdata,
> > +                               struct platform_device *pdev,
> > +                               unsigned int cpu, int pkg_flag)
> > +{
> > +       struct temp_data *tdata;
> > +       struct cpuinfo_x86 *c = &cpu_data(cpu);
> > +       u32 eax, edx;
> > +       int err;
> > +       int core_id = c->cpu_core_id;
> >
> You need to check here if core_count reached the number of entries in
> core_data[].
> Otherwise we'll get a nasty surprise with the first 16-core CPU (since that CPU
> will
> need 17 entries in core_data[], but there are only 16).

I missed it..Thanks for pointing it out..

> 
> Even better would be to come up with a dynamic scheme which does not depend on
> the number
> of cores.

Thought through this..But the way would be to use a ** and do dynamic mem allocation.
Again, My opinion is that it will make the code complex.
But, if you are Ok with this approach, I can implement this way.

> > +static struct platform_device *get_pdev(int phys_proc_id)
> > +{
> 
> Function names should all start with coretemp_.

Shall Change accordingly..

> > +       /* If this core is a HT one, do not create any interfaces */
> > +       indx = get_core_indx(pdata, c->cpu_core_id);
> > +       if (indx >= 0) {
> > +               dev_info(&pdev->dev, "A HT core (%u) is onlined\n", cpu);
> 
> Is this message really necessary ? It does not provide any real value, or does
> it ?

I will remove this in the next version of the patch..

> 
> > +               return;
> > +       }
> > +
> > +       err = create_core_data(pdata, pdev, cpu, pkg_flag);
> > +       if (err) {
> > +               dev_err(&pdev->dev, "Onlining Core %u on Pkg %d failed\n",
> 
> 	"Onlining" and "Offlining" are really odd terms. Can you find something
> better ?
> 	Adding / Removing, maybe ?

I also had this thought..Shall replace them with Adding / Removing

> 
> 	%d --> %u. Arguable if you want to use %d everywhere, but mixing it is a
> bit odd.

I would rather use %u consistently..

+static void remove_core(struct platform_data *pdata, struct device *dev,
> > +                       int indx)
> > +{
> > +       int i;
> > +       int max = pdata->core_count - 1;
> > +
> > +       /* Remove all sysfs attrs for this core */
> > +       remove_attrs(dev, pdata->core_data[indx]);
> > +
> > +       /* Shift the core_data elements */
> > +       for (i = indx; i < max; i++)
> > +               pdata->core_data[i] = pdata->core_data[i + 1];
> > +
> > +       /* Free the _last_ element */
> > +       kfree(pdata->core_data[max]);
> 
> 	Did you think this through ?
> 	You removed element [indx], yet you remove pdata->core_data[max]
> 	which you just moved to pdata->core_data[max - 1].
> 	Sounds like a recipe for a crash.

Got it..Shall fix this major bug..
I read this core_data as an array and forgot that this is an array
Of _pointers_. Thanks Guenter ;-)

> > +static void get_core_online(unsigned int cpu)
> > +{
> > +       struct cpuinfo_x86 *c = &cpu_data(cpu);
> > +       struct platform_device *pdev = get_pdev(c->phys_proc_id);
> > +
> > +       if (!pdev) {
> > +               /*
> > +                * We are bringing the _first_ core in this pkg
> > +                * online. So initialize per-pkg data structures and
> > +                * then bring this core online.
> > +                */
> > +               coretemp_device_add(cpu);
> 
> If the CPU has no thermal sensors, or if coretemp_device_add() fails,
> you keep going anyway. Kind of odd to check for the error in add_core()
> instead of not calling add_core() in the first place.

Shall add error checking and return if device_add(cpu) fails..

> 
> Also, even though coretemp_device_add() is now only called once per CPU
> package,
> it still includes code which is only useful if it was called once per core.

I did not want to touch that method..
I see that there is a check to skip HT entries.
Shall remove that in the next version of the patch.

> > +
> > +       pdata = platform_get_drvdata(pdev);
> > +
> > +       indx = get_core_indx(pdata, c->cpu_core_id);
> > +       if (indx < 0) {
> > +               dev_info(&pdev->dev, "Core %d does not exist\n",
> > +                       c->cpu_core_id);
> 
> I think you'll hit this for the 2nd core of HT CPUs. If so, it is not an error
> condition. Also see below.
> 
> > +               return;
> > +       }
> > +
> > +       if (pdata->core_data[indx]->cpu = cpu)
> > +               remove_core(pdata, &pdev->dev, indx);
> > +       else
> > +               dev_info(&pdev->dev, "A HT CPU (%u) is offlined\n", cpu);
> > +
> 	Somehow this looks fishy. Since you did not add an entry for the HT core
> 	in the first place, doesn't that mean that get_core_indx() will fail
> 	if you try to remove that non-existing entry ?
> 	If so, you would get an "Core %d does not exist" error whenever a HT
> 	CPU is removed. Not desirable.
> 
> 	The old code would add the 2nd HT core if the 1st one was taken offline.
> 	I don't see that happen here. Not sure if that was a good idea, so I
> don't
> 	really mind. But I don't think the code should complain if that 2nd never
> 	added HT core is taken offline.

Alright. Shall remove the message. Thanks for pointing it out.

> >  static int __init coretemp_init(void)
> >  {
> >         int i, err = -ENODEV;
> > @@ -559,7 +851,7 @@ static int __init coretemp_init(void)
> >                 goto exit;
> >
> >         for_each_online_cpu(i)
> > -               coretemp_device_add(i);
> > +               get_core_online(i);
> >
> >  #ifndef CONFIG_HOTPLUG_CPU
> >         if (list_empty(&pdev_list)) {
> > --
> > 1.7.4
> >
> 

Thanks,
Durga

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Guenter Roeck]

On Wed, Apr 06, 2011 at 02:52:03AM -0400, R, Durgadoss wrote:
> Hi Guenter,
> 
> Thanks for a detailed review and intuitive comments.
> 
> > > From: Durgadoss R <durgadoss.r@intel.com>
> > >
> > > Date: Thu, 3 Mar 2011 02:37:40 +0530
> > > Subject: [PATCH:hwmon:v4:Resend]Merging_pkgtemp_with_coretemp
> > >
> > > This patch merges the pkgtemp driver with the coretemp driver.
> > > This merged driver creates one hwmon device per physical CPU i.e
> > > per Physical Package. Also, the sysfs interfaces per core are created
> > > as each core comes online and removed when it goes offline.
> > >
> > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > >
> >
> > After browsing through the code, I concluded that dynamic addition and removal
> > of CPUs likely won't work.  So I did a quick test.
> >
> > echo 0 >/sys/devices/system/cpu/cpu2/online
> > sensors
> >
> > Result was as suspected:
> >
> > [   84.518158] IP: [<ffffffffa00332a1>] show_label+0x21/0x70 [coretemp]
> > [   84.525604] PGD 42b9e0067 PUD 42cc0a067 PMD 0
> > [   84.530958] Oops: 0000 [#1] SMP
> > [   84.534870] last sysfs file: /sys/devices/platform/coretemp.0/temp4_label
> > [   84.542743] CPU 4
> > ...
> >
> > Oh well. I am quite sure I mentioned it before - _please_ test your code.
> > I am not your test group (and maybe you start to understand why it takes
> > so long to review your code).
> >
> 
> I did test it on Corei5 and SNB. Anyway, next time when I submit this patch,
> I shall attach the dmesg logs.
> 
Yes, but obviously you did not try adding/removing CPU cores....

[ ... ]

> > > +       u8 crit_alarm;
> >
> > crit_alarm is only written to, thus unnecessary.
> >
> > [ question, though, is why you keep reading it. Is it expected to change ? ]
> 
> When the temp1_input goes above temp1_crit, this crit_alarm is set to 1.
> When the temp1_input falls below temp1_crit, this crit_alarm is reset to 0.

Sure, but it is still a write-only variable, so what is the point ?

> As you mentioned below, I don't need this variable. I can just read and display.
> 

Exactly.

> >
> > > +       u8 max_alarm;
> >
> > max_alarm is not used anywhere, thus unnecessary.
> 
> We need this when we add the threshold(max and max_hyst) support also.
> Hence I added.
> 
In this case, you can add the variable when you need it. No need to do it now.

[ ... ]

> > > +               if (err) {
> > > +                       dev_warn(dev,
> > > +                       "Unable to read IA32_TEMPERATURE_TARGET MSR\n");
> > > +
> > Extra blank line
> >
> > > +               } else {
> > > +                       tdata->ttarget = tdata->tjmax -
> > > +                                       (((eax >> 8) & 0xff) * 1000);
> > > +               }
> >
> > { } are unnecessary.
> 
> In one of the previous patches, I had a single statement split into two lines,
> Without Braces. And I got comment asking me to add braces to increase readability.
> With that in mind, I added braces.
> Shall remove if it is Ok to do so ;-)
> 
Just keep it. Maybe it is better since those are multi-line statements.

> >
> > If you can not read ttarget, you still create tempX_max and display 0.
> > If ttarget is not supported, the attribute should not be created in the first
> > place.
> > The old code did this; any special reason for removing it ?
> 
> Earlier when I tried the "Adding Threshold Support to Coretemp.patch",
> We discussed that temp1_max will have the Threshold1 value if ttarget is not supported.
> When we have the Threshold patch all _max things will be fixed.
> 
> But if you say that will not work or not the right way to do, I can change
> the code accordingly.
> 
At least add a comment indicating that this will be fixed in a subsequent patch.

> > need 17 entries in core_data[], but there are only 16).
> 
> I missed it..Thanks for pointing it out..
> 
> >
> > Even better would be to come up with a dynamic scheme which does not depend on
> > the number
> > of cores.
> 
> Thought through this..But the way would be to use a ** and do dynamic mem allocation.
> Again, My opinion is that it will make the code complex.
> But, if you are Ok with this approach, I can implement this way.
> 
I am fine with the static allocation. Just make sure you don't exceed the limits.

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Jay Alexander Fleming]

In addition to this post, here is report from HW monitor in BIOS (v. 1.60 for
this mobo), right after restart my computer:

Voltage---------
 Vcore     1.432
 +3.30V    3.344
 +5.00V    5.088
 +12.00V  12.408

Temp------------
 CPU   43
 MB    38



 


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Valentijn Scholten]

Valentijn Scholten <valentijnscholten <at> hotmail.com> writes:

Intel forum link: http://software.intel.com/en-us/forums/showthread.php?t‚888



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Valentijn Scholten]

Valentijn Scholten <valentijnscholten <at> hotmail.com> writes:

> 
> Valentijn Scholten <valentijnscholten <at> hotmail.com> writes:
> 
> Intel forum link: http://software.intel.com/en-us/forums/showthread.php?t‚888
> 


Just produced the same results using the Intel AMT opensource driver.
The module is now called mei instead of heci.
The sample applications from the Intel QST SDK still work, so need for old and 
patched heci driver from www.openamt.org.


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Valentijn Scholten]

Valentijn Scholten <valentijnscholten <at> hotmail.com> writes:

Made a typo: You do *not* need the heci driver from www.openamt.org anymore!




_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Maryvonne JUDIT]

[-- Attachment #1.1: Type: text/plain, Size: 28 bytes --]

merci de bien me désinscrire

[-- Attachment #1.2: Type: text/html, Size: 344 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[Malika et Christophe CHARBONNIER]

[-- Attachment #1.1: Type: text/plain, Size: 209 bytes --]

Veuillez retirer définitivement les adresses suivantes de vos fichiers.
malikacha@laposte.net
charbonnierc@laposte.net
gaiacha@live.fr
gaiacha@bbox.fr

Cei dans les plus brefs délais.
Merci par avance

[-- Attachment #1.2: Type: text/html, Size: 638 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors]

[serge chartrain]

Merci de faire le nécéssaire pour ma radiation de vos liste, n'ayant jamais 
demandé à y figurer, je suis ulcéré des procédés que vous utilisez.

S.Chartrain 

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors