Hello Guenter,
On 04/07/2015 06:44 PM, Guenter Roeck wrote:
> On Tue, Apr 07, 2015 at 04:45:56PM +0200, Cédric Le Goater wrote:
>> The new OPAL device tree adds a few properties which can be used to add
>> extra information on the sensor label.
>>
>> In the case of a cpu core sensor, the firmware exposes the physical
>> identifier of the core in the "ibm,pir" property. The driver
>> translates this identifier in a linux cpu number and prints out a
>> range corresponding to the hardware threads of the core (as they
>> share the same sensor).
>>
>> The numbering gives a hint on the localization of the core in the
>> system (which socket, which chip).
>>
>> Signed-off-by: Cédric Le Goater <clg@xxxxxxxxxx>
>
> Hi Cedric,
>
> Please do not send follow-up patches as reply, but as separate e-mail.
Ok. I will start a new thread when I resend this patch.
> Further comments below.
>
>> ---
>>
>> Changes since v1:
>>
>> - check cpu validity before printing out the attribute label.
>> if invalid, use a "phy" prefix to distinguish a linux cpu
>> number from a physical cpu number.
>>
>> drivers/hwmon/ibmpowernv.c | 34 ++++++++++++++++++++++++++++++++++
>> 1 file changed, 34 insertions(+)
>>
>> Index: linux.git/drivers/hwmon/ibmpowernv.c
>> ===================================================================
>> --- linux.git.orig/drivers/hwmon/ibmpowernv.c
>> +++ linux.git/drivers/hwmon/ibmpowernv.c
>> @@ -113,9 +113,43 @@ static ssize_t show_label(struct device
>> static void __init make_sensor_label(struct device_node *np,
>> struct sensor_data *sdata, const char *label)
>> {
>> + u32 id;
>> size_t n;
>>
>> n = snprintf(sdata->label, sizeof(sdata->label), "%s", label);
>> +
>> + /*
>> + * Core temp pretty print
>> + */
>> + if (!of_property_read_u32(np, "ibm,pir", &id)) {
>> + int i = -1;
>> +
>> + for_each_possible_cpu(i)
>> + if (paca[i].hw_cpu_id == id)
>
> I think you should consider using get_hard_smp_processor_id() and avoid
> direct access to the paca array.
Yes. It will look better. Thanks.
>> + break;
>> +
>> + if (i != -1)
>
> I don't think that works. i is initialized by for_each_possible_cpu(),
> either to -1 or 0 depending on the state of CONFIG_SMP. So pre-initializing
> it won't make a difference. Its last value is NR_CPUS (SMP) or 1 (non-SMP).
>
> You would need a second variable (which you could pre-initialize)
> to determine if the code found a matching ID or not.
gloups. I did that patch waaaay too quickly, it is completely bogus ...
I will cook a new version. sorry for the noise.
>> + /*
>> + * The digital thermal sensors are associated
>> + * with a core. Let's print out the range of
>> + * cpu ids corresponding to the hardware
>> + * threads of the core.
>> + */
>> + n += snprintf(sdata->label + n,
>> + sizeof(sdata->label) - n,
>> + " %d-%d", i, i+7);
>
> I would really like to see how this looks like in practice.
>
> The id in the devicetree entry is the physical CPU. The resulting index
> is the logical CPU number. So let's assume that the logical CPU number
> for the first physical CPU is 0. Output would be "0-7". If the second
> physical CPU matches logical CPU 1, output would be "1-8".
> How does that make any sense ?
The logical CPU numbers on PowerPC are initialized looping on the cores
found in the device tree and then on the threads of the core. Something
like :
logical_cpu = core_index * max_threads_per_core + thread_index
which gives on a P8 :
# ppc64_cpu --info
Core 0: 0* 1* 2* 3* 4* 5* 6* 7*
Core 1: 8* 9* 10* 11* 12* 13* 14* 15*
Core 2: 16* 17* 18* 19* 20* 21* 22* 23*
Core 3: 24* 25* 26* 27* 28* 29* 30* 31*
Core 4: 32* 33* 34* 35* 36* 37* 38* 39*
Core 5: 40* 41* 42* 43* 44* 45* 46* 47*
Core 6: 48* 49* 50* 51* 52* 53* 54* 55*
Core 7: 56* 57* 58* 59* 60* 61* 62* 63*
Core 8: 64* 65* 66* 67* 68* 69* 70* 71*
Core 9: 72* 73* 74* 75* 76* 77* 78* 79*
Core 10: 80* 81* 82* 83* 84* 85* 86* 87*
Core 11: 88* 89* 90* 91* 92* 93* 94* 95*
Core 12: 96* 97* 98* 99* 100* 101* 102* 103*
Core 13: 104* 105* 106* 107* 108* 109* 110* 111*
Core 14: 112* 113* 114* 115* 116* 117* 118* 119*
Core 15: 120* 121* 122* 123* 124* 125* 126* 127*
Core 16: 128* 129* 130* 131* 132* 133* 134* 135*
Core 17: 136* 137* 138* 139* 140* 141* 142* 143*
Core 18: 144* 145* 146* 147* 148* 149* 150* 151*
Core 19: 152* 153* 154* 155* 156* 157* 158* 159*
on a P7 :
# ppc64_cpu --info
Core 0: 0* 1* 2* 3*
Core 1: 4* 5* 6* 7*
Core 2: 8* 9* 10* 11*
Core 3: 12* 13* 14* 15*
Core 4: 16* 17* 18* 19*
Core 5: 20* 21* 22* 23*
> Also, how do you know that the range of CPU IDs is always 8 ?
This is a shortcut. The code is for the ibmpowernv platform and assumes
that we are running on a P8 (8 hardware threads). It would be better to
use a "maximum threads per core" variable but I am not sure this is
available, as it is a tunable. I will look into it.
> Can you provide some resulting outputs ?
sure.
On an Open Power system :
# sensors
ibmpowernv-isa-0000
Adapter: ISA adapter
Core 8-15: +29.0°C
Core 16-23: +29.0°C
Core 24-31: +30.0°C
Core 32-39: +30.0°C
Core 40-47: +32.0°C
Core 48-55: +29.0°C
Core 56-63: +29.0°C
Centaur 0: +28.0°C
Centaur 1: +32.0°C
Centaur 4: +28.0°C
Centaur 5: +27.0°C
Core 0-7: +29.0°C
The Centaur numbering does not look as good as for the core.
On an IBM Power system :
# sensors
ibmpowernv-isa-0000
Adapter: ISA adapter
fan1: 5960 RPM (min = 0 RPM)
fan2: 5252 RPM (min = 0 RPM)
fan3: 5960 RPM (min = 0 RPM)
fan4: 5242 RPM (min = 0 RPM)
fan5: 5008 RPM (min = 0 RPM)
fan6: 5000 RPM (min = 0 RPM)
fan7: 5232 RPM (min = 0 RPM)
fan8: 5986 RPM (min = 0 RPM)
fan9: 5232 RPM (min = 0 RPM)
fan10: 6094 RPM (min = 0 RPM)
fan11: 5242 RPM (min = 0 RPM)
fan12: 5882 RPM (min = 0 RPM)
fan13: 5212 RPM (min = 0 RPM)
fan14: 5973 RPM (min = 0 RPM)
Ambient: +18.0°C (high = +0.0°C)
Core 0-7: +40.0°C
Core 8-15: +39.0°C
Core 16-23: +39.0°C
Core 24-31: +38.0°C
Core 32-39: +38.0°C
Core 40-47: +37.0°C
Core 48-55: +37.0°C
Core 56-63: +38.0°C
Core 64-71: +38.0°C
Core 72-79: +39.0°C
Core 80-87: +35.0°C
Core 88-95: +34.0°C
Core 96-103: +33.0°C
Core 104-111: +34.0°C
Core 112-119: +33.0°C
Core 120-127: +31.0°C
Core 128-135: +31.0°C
Core 136-143: +39.0°C
Core 144-151: +39.0°C
Core 152-159: +40.0°C
power1: 425.00 W
The Centaur are not exposed yet.
Thanks,
C.
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
