Re: [PATCH] hwmon: (fam15h_power) Disable preemption when reading registers

From: Huang, Ray
Date: Wed Jun 01 2016 - 11:41:51 EST




> On Jun 1, 2016, at 9:23 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
>> On 06/01/2016 03:04 AM, Borislav Petkov wrote:
>> From: Borislav Petkov <bp@xxxxxxx>
>>
>> We need to read a bunch of registers on each compute unit and possibly
>> on the current CPU too. Disable preemption around it.
>
> An explanation would be helpful. Is this a bug fix ? I would like to get
> a confirmation from someone at AMD that this is really necessary.
>

This change looks good for me. But I am in office this week, I will test it on CZ platform next week. :-)

Thanks,
Rui

> Thanks,
> Guenter
>
>> Signed-off-by: Borislav Petkov <bp@xxxxxxx>
>> Cc: Rui Huang <ray.huang@xxxxxxx>
>> Cc: Sherry Hurwitz <sherry.hurwitz@xxxxxxx>
>> Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
>> ---
>> drivers/hwmon/fam15h_power.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hwmon/fam15h_power.c b/drivers/hwmon/fam15h_power.c
>> index eb97a9241d17..69bb810f528b 100644
>> --- a/drivers/hwmon/fam15h_power.c
>> +++ b/drivers/hwmon/fam15h_power.c
>> @@ -172,9 +172,9 @@ static void do_read_registers_on_cu(void *_data)
>> */
>> static int read_registers(struct fam15h_power_data *data)
>> {
>> - int this_cpu, ret, cpu;
>> int core, this_core;
>> cpumask_var_t mask;
>> + int ret, cpu;
>>
>> ret = zalloc_cpumask_var(&mask, GFP_KERNEL);
>> if (!ret)
>> @@ -183,7 +183,6 @@ static int read_registers(struct fam15h_power_data *data)
>> memset(data->cu_on, 0, sizeof(int) * MAX_CUS);
>>
>> get_online_cpus();
>> - this_cpu = smp_processor_id();
>>
>> /*
>> * Choose the first online core of each compute unit, and then
>> @@ -205,10 +204,13 @@ static int read_registers(struct fam15h_power_data *data)
>> cpumask_set_cpu(cpumask_any(topology_sibling_cpumask(cpu)), mask);
>> }
>>
>> - if (cpumask_test_cpu(this_cpu, mask))
>> + preempt_disable();
>> + smp_call_function_many(mask, do_read_registers_on_cu, data, true);
>> +
>> + if (cpumask_test_cpu(smp_processor_id(), mask))
>> do_read_registers_on_cu(data);
>>
>> - smp_call_function_many(mask, do_read_registers_on_cu, data, true);
>> + preempt_enable();
>> put_online_cpus();
>>
>> free_cpumask_var(mask);
>