Hi Guenter,
> On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote:
> > On a system with course grain resolution of energy unit (milli J) the
> > accumulation thread can be executed less frequently than on the system
> > with fine grain resolution(micro J).
> >
> > This patch sets the accumulation thread interval to an optimum value
> > calculated based on the (energy unit) resolution supported by the
> > hardware (assuming a peak wattage of 240W).
> >
> > Signed-off-by: Naveen Krishna Chatradhi <nchatrad@xxxxxxx>
> > ---
> > drivers/hwmon/amd_energy.c | 12 +++++++++++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
> > index 9580a16185b8..f0a13d6cc419 100644
> > --- a/drivers/hwmon/amd_energy.c
> > +++ b/drivers/hwmon/amd_energy.c
> > @@ -48,6 +48,7 @@ struct amd_energy_data {
> > struct sensor_accumulator *accums;
> > /* Energy Status Units */
> > u64 energy_units;
> > + unsigned int timeout;
> > int nr_cpus;
> > int nr_socks;
> > int core_id;
> > @@ -215,6 +216,7 @@ static umode_t amd_energy_is_visible(const void
> > *_data, static int energy_accumulator(void *p) {
> > struct amd_energy_data *data = (struct amd_energy_data *)p;
> > + unsigned int timeout = data->timeout;
> >
> > while (!kthread_should_stop()) {
> > /*
> > @@ -234,7 +236,7 @@ static int energy_accumulator(void *p)
> > *
> > * let us accumulate for every 100secs
> > */
> > - schedule_timeout(msecs_to_jiffies(100000));
> > + schedule_timeout(msecs_to_jiffies(timeout));
>
> Numbers below are in seconds, used as milli-seconds here.
>
> > }
> > return 0;
> > }
> > @@ -331,6 +333,14 @@ static int amd_energy_probe(struct platform_device *pdev)
> > if (IS_ERR(hwmon_dev))
> > return PTR_ERR(hwmon_dev);
> >
> > + /* Once in 3 minutes for a resolution of 1/2*16 */
>
> * 16 or ^ 16 ?
>
> > + if (data->energy_units == 0x10)
> > + data->timeout = 3 * 60;
>
> 180 ms ? I assume this is a bug and meant to be 3 * 60 * 1000.
>
> > +
> > + /* Once in 3 days for a resolution of 1/2^6 */
> > + if (data->energy_units == 0x6)
> > + data->timeout = 3 * 24 * 60 * 60;
> > +
>
> ... and else ?
>
> This needs to cover all cases, including those not currently existing.
> I would suggest to define a formula based on data->energy_units.
> The energy units value can be anything from 0..31. Based on your numbers, something like
> timeout_ms = BIT(34 - data->energy_units); should do. It translates to about 3.1 days for energy_units=6, and
> 4.3 minutes for energy_units=16. If that is too much, you can make it
> timeout_ms = BIT(33 - data->energy_units);
>
> To avoid overflow, it might make sense to max out at BIT(31).
> timeout_ms = BIT((min(31, 33 - data->energy_units));
>
Thank you for the feedback and suggestions. I will implement and
submit the updated version.
> Guenter
>
> > data->wrap_accumulate = kthread_run(energy_accumulator, data,
> > "%s", dev_name(hwmon_dev));
> > if (IS_ERR(data->wrap_accumulate))
> >
Naveen
