Re: [lm-sensors] [PATCH-v2:RFC][hwmon]:Merging_Pkgtemp_with_Coretemp

[Guenter Roeck <guenter.roeck@ericsson.com>]

On Mon, Feb 21, 2011 at 04:33:49AM -0500, R, Durgadoss wrote:
> Hi Guenter,
> 
> Sorry for the delayed response due to the weekend.
> 
> > I tried to install your patch, but must have done something wrong - it crashes
> > for
> > me with a null pointer access. System is running 2.6.35, so it might not be the
> > driver's fault but something I missed while backporting it.
> > 
> 
> Yes. This patch will apply cleanly on any kernel version above 2.6.36-stable.
> I prefer creating these patches on 2.6.38-rc4.
> I should have specified it while sending the patch..Sorry that I missed it.
> 
> 
I applied the patch as-is to the latest kernel as of two hours ago and loaded it.
This resulted in a null pointer dereference crash - exactly the same as I got
when trying with 2.6.35. CPU is i3-540.

> > > +static DEFINE_MUTEX(update_lock);
> > >
> > Not a good idea. You don't want any global variables.
> 
> Ok. Shall move it back into the structure.
> 
> > > +/* Attributes per physical CPU */
> > > +static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, DRV_NAME);
> > > +static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO, show_tjmax, NULL, 0);
> > > +/* Coretemp attributes */
> > > +static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_name, NULL, COREID);
> > > +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL, 0);
> > > +static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO, show_ttarget, NULL, 0);
> > > +static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_crit_alarm, NULL,
> > 0);
> > > +/* Packagetemp attributes */
> > > +static SENSOR_DEVICE_ATTR(temp2_label, S_IRUGO, show_name, NULL,
> > PHYS_PROCID);
> > > +static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp, NULL, 1);
> > > +static SENSOR_DEVICE_ATTR(temp2_max, S_IRUGO, show_ttarget, NULL, 1);
> > > +static SENSOR_DEVICE_ATTR(temp2_crit_alarm, S_IRUGO, show_crit_alarm, NULL,
> > 1);
> > >
> > Still not the right approach here. I could not test the driver, but since you
> > only have two sets of sensors it can not do what it is supposed to do if there
> > are multiple cores. Again, we would expect a single instance of the driver
> > to handle all cores plus the package sensor. Looks like you merged core temp
> > with pkg temp, but you still create an instance of the driver per core.
> > And I am not sure if the package temp now shows up on each core.
> 
> I think we are missing something here.
> 
> In 2.6.35 version of the coretemp, there is no #ifdef CONFIG_SMP
> in the probe function. This creates a hwmon device for every core
> Of the CPU. (In fact I verified this in one board that was running
> 2.6.35 kernel. Though the board had only One physical CPU, there were
> two instances of the hwmon device, Created by Coretemp. And both of them
> showed exactly same temperature always.)
> The same board running a 2.6.37 showed only one hwmon device.
> Then I figured out that this is due to the SMP check in the probe
> function. This check eliminates the creation of redundant coretemp attributes.
> (These were created for all cores in a Physical CPU in older kernels)
> Hence, in newer kernels, there will be one set of attributes(temp1*) for all cores
> in a Physical CPU created by Coretemp and other set (temp2*) of attributes for PKG temp.
> 
> Kindly correct me if I am wrong.
> 
Yes, you are. With a 4-core CPU, the assumption is that there would be either four 
or five tempX_input attributes, one per core plus optionally another one for the package.
Your code only has temp1_input and temp2_input, thus it can not do what we expected to see
from the merged driver. This has nothing to do with CONFIG_SMP.

There is supposed to be one temperature reading per physical core.
On 2.6.38-r55+, output with i3-540 is

coretemp-isa-0000
Adapter: ISA adapter
Core 0:       +16.0°C  (high = +89.0°C, crit = +105.0°C)

coretemp-isa-0002
Adapter: ISA adapter
Core 2:       +23.0°C  (high = +89.0°C, crit = +105.0°C)

Again, expected output would be along the line of 

coretemp-isa-0000
Adapter: ISA adapter
Core 0:       +16.0°C  (high = +89.0°C, crit = +105.0°C)
Core 2:       +23.0°C  (high = +89.0°C, crit = +105.0°C)

ie there should be only one instance of the driver instead of two for the two cores.
You don't see Core 1 and Core 3 because those are virtual (hyperthreading) cores,
not physical cores. This is the effect you are talking about above
(ie the change affects physical vs. virtual cores, not physical CPUs).

Guenter

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