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"? 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. > > > > 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? > > 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. > 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
