On 4/12/21 5:14 AM, Jean Delvare wrote:
> Hi Guenter,
>
> On Fri, 9 Apr 2021 10:48:51 -0700, Guenter Roeck wrote:
>> The driver implements two separate functions to read and update
>> energy values. One function is called periodically and updates
>> cached enery information to avoid loss of data, the other reads
>> energy data on demand to report it to userspace. The second
>> function reads current energy data, adds the difference to the
>> data previously obtained by the first function, and then discards
>> the updated data.
>>
>> Simplify the code and always call the first function, then report
>> the energy data obtained by this function to userspace.
>
> I like the idea.
>
>> Cc: Naveen Krishna Chatradhi <nchatrad@xxxxxxx>
>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
>> ---
>> v2: Added patch, simplification
>>
>> drivers/hwmon/amd_energy.c | 31 ++++++-------------------------
>> 1 file changed, 6 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
>> index a86cc8d6d93d..93bad64039f1 100644
>> --- a/drivers/hwmon/amd_energy.c
>> +++ b/drivers/hwmon/amd_energy.c
>> @@ -118,35 +118,12 @@ static void read_accumulate(struct amd_energy_data *data)
>> data->core_id++;
>> }
>>
>> -static void amd_add_delta(struct amd_energy_data *data, int ch,
>> - int cpu, long *val, u32 reg)
>> -{
>> - struct sensor_accumulator *accum;
>> - u64 input;
>> -
>> - mutex_lock(&data->lock);
>> - rdmsrl_safe_on_cpu(cpu, reg, &input);
>> - input &= AMD_ENERGY_MASK;
>> -
>> - accum = &data->accums[ch];
>> - if (input >= accum->prev_value)
>> - input += accum->energy_ctr -
>> - accum->prev_value;
>> - else
>> - input += UINT_MAX - accum->prev_value +
>> - accum->energy_ctr;
>> -
>> - /* Energy consumed = (1/(2^ESU) * RAW * 1000000UL) μJoules */
>> - *val = div64_ul(input * 1000000UL, BIT(data->energy_units));
>> -
>> - mutex_unlock(&data->lock);
>> -}
>> -
>> static int amd_energy_read(struct device *dev,
>> enum hwmon_sensor_types type,
>> u32 attr, int channel, long *val)
>> {
>> struct amd_energy_data *data = dev_get_drvdata(dev);
>> + struct sensor_accumulator *accum;
>> u32 reg;
>> int cpu;
>>
>> @@ -162,7 +139,11 @@ static int amd_energy_read(struct device *dev,
>>
>> reg = ENERGY_CORE_MSR;
>> }
>> - amd_add_delta(data, channel, cpu, val, reg);
>> +
>> + accumulate_delta(data, channel, cpu, reg);
>> + accum = &data->accums[channel];
>> +
>> + *val = div64_ul(accum->energy_ctr * 1000000UL, BIT(data->energy_units));
>
> Is it considered safe to read accum->energy_ctr while not holding
> data->lock?
>
You mean because it is a 64-bit value ? Good question; it might not be if compiled
on a 32-bit system. Good news is that I moved reading accum->energy_ctr under the lock
in the next patch, so we should be good.
>>
>> return 0;
>> }
>
> If the answer to the question above is "yes" then:
>
> Reviewed-by: Jean Delvare <jdelvare@xxxxxxx>
>
Thanks!
Guenter
