On Mon, Jul 19, 2010 at 07:24:13AM -0400, Jean Delvare wrote: > Hi all, > > Random comments: > > On Mon, 19 Jul 2010 00:44:02 -0300, Henrique de Moraes Holschuh wrote: > > On Sun, 18 Jul 2010, Guenter Roeck wrote: > > > On Sun, Jul 18, 2010 at 10:22:14PM -0400, Chen Gong wrote: > > > > 7/18/2010 10:10 PM, Henrique de Moraes Holschuh > > > > > On Sun, 18 Jul 2010, Guenter Roeck wrote: > > > > >> All correct. I have a second concern after looking through the IA-32 > > > > >> documentation. > > > > >> > > > > >> The package sensor is not supported for all chips, and there is only > > > > >> one sensor per package (not per core). Yet, the code as written adds the > > > > >> new sensor unconditionally. I think this needs to change to only create > > > > >> the sysfs file if the sensor is supported by the CPU, and only once per core. > > I guess you really meant: "only once per CPU"? > yes. > This will be difficult and inconvenient to implement with the current > driver's strategy of having a separate hwmon entry for each core > temperature value. > Yes, but better than showing something that isn't there. > > > > > It is a really good idea to never expose through sysfs something that > > > > > isn't there. Alternatively, you can return errors like ENXIO, but you > > > > > should only do that when you have no other choice (because, e.g. you > > > > > cannot know beforehand whether a sensor will be available or not). > > +1 > > > > > > If one can detect when processor packages and cores come and go, and one > > > > > can ask the package and cores about the sensors that are really > > > > > available, one can expose to sysfs just the sensors that are > > > > > operational, and remove or add sensors when needed. > > I think we only see cores come and go, not packages = (physical CPUs). > The driver would have to track the cores. When the last core from a > given package goes away, we claim the package to be gone. Likewise, > when a core shows up with a physical id we didn't have yet, we claim > the package just came in. > > > > > Grrr, It looks like a separate driver for package thermal sensor is a > > > > good idea ? > > No. This would only make things even more confusing and difficult to > handle. If anything, I'd rather consolidate the temperature values into > fewer hwmon entries, not the other way around. But even if we go for a > separate entry for the package temperature, it can be done in the > coretemp driver. No need for a separate driver. > > > > Why ? Just make the sysfs entry conditional. All you need to do is to detect > > > in the probe function if there is a package sensor, > > How? > One method would be to use rdmsr_safe_on_cpu() and try to read the register. If the read fails, the register isn't there, and the sensor is not supported. > > > and only create the sysfs > > > entry if there is. Plus, make sure that it is only created once. > > As said above, this isn't trivial. If you just add the sysfs attribute > to existing hwmon devices, you'll have duplicates. If you don't want > duplicates, you'll get an asymmetry (an arbitrary hwmon entry will get > the extra temperature, the user will get inevitably confused by this) > and the hotplug code will be difficult to deal with. > > The most direct approach is probably to have a separate hwmon entry for > the package temperature. But I start feeling dizzy when looking at > these 8 coretemp hwmon entries I already have on my system, and adding > a couple more doesn't exactly please me. So it may be the right time to > start thinking of a different strategy - even though this comes with > backwards compatibility pain. > Personally I would prefer to have one entry per CPU, with one sensor per core. In other words, something like 1st CPU: coretemp-xxx-0000 temp1_input core 0 temp2_input core 1 .. tempN_input core N-1 tempN+1_input package temperature, if the sensor exists 2nd CPU: coretemp-xxx-0001 and so on. > > And make sure to recompute it when CPUs are hotadded or hotremoved, I > > suppose. Maybe it doesn't need to do it right away (I don't even know if > > coretemp already does it), but it will have to sooner or later... > > The coretemp driver deals with hotplug fine (minus the bug Chen > apparently spotted), although I never tried it myself. > > But please keep in mind that libsensors applications are not notified > when sysfs attributes come and go at runtime, nor when hwmon entries > come and go. So in the end, hotplug needs a manual restart of all > monitoring applications for now anyway. > > -- > Jean Delvare _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
