| Message ID | 20210409174852.4585-1-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2,1/2] hwmon: (amd_energy) Use unified function to read energy data | expand |
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@amd.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > 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? > > return 0; > } If the answer to the question above is "yes" then: Reviewed-by: Jean Delvare <jdelvare@suse.de>
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@amd.com> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> 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@suse.de> > Thanks! Guenter
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)); return 0; }
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. Cc: Naveen Krishna Chatradhi <nchatrad@amd.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- v2: Added patch, simplification drivers/hwmon/amd_energy.c | 31 ++++++------------------------- 1 file changed, 6 insertions(+), 25 deletions(-)