Re: [PATCH] hwmon (occ): Switch power average to between poll responses

[Eddie James <eajames@xxxxxxxxxxxxxxxxxx>]

On 5/8/19 4:03 PM, Guenter Roeck wrote:
> On Tue, May 07, 2019 at 02:35:51PM -0500, Eddie James wrote:
> > The average power reported in the hwmon OCC driver is not useful
> > because the time it represents is too short. Instead, store the
> > previous power accumulator reported by the OCC and average it with the
> > latest accumulator to obtain an average between poll responses. This
> > does operate under the assumption that the user polls regularly.
> That looks really bad. Effectively it means that the number reported
> as average power is more or less useless/random. On top of that, the code
> gets so complicated that it is all but impossible to understand.
>
> Does it really make sense to report an average that has effectively
> no useful meaning (and is, for example, influenced just by reading it) ?


Yea... that's a good point. Basically our userspace environment has no 
good way to do this either, so I tried to shoe-horn this into the 
driver, but this approach is indeed bad. I'll abandon this patch.


Thanks!

Eddie


> Guenter
>
> > Signed-off-by: Eddie James <eajames@xxxxxxxxxxxxx>
> > ---
> >   drivers/hwmon/occ/common.c | 133 ++++++++++++++++++++++++++++++++-------------
> >   drivers/hwmon/occ/common.h |   7 +++
> >   2 files changed, 103 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
> > index e6d3fb5..6ffcee7 100644
> > --- a/drivers/hwmon/occ/common.c
> > +++ b/drivers/hwmon/occ/common.c
> > @@ -118,6 +118,53 @@ struct extended_sensor {
> >   	u8 data[6];
> >   } __packed;
> >   
> > +static void occ_update_prev_power_avgs(struct occ *occ)
> > +{
> > +	u8 i;
> > +	struct power_sensor_1 *ps1;
> > +	struct power_sensor_2 *ps2;
> > +	struct power_sensor_a0 *psa0;
> > +	struct occ_sensor *power = &occ->sensors.power;
> > +	struct occ_power_avg *prevs = occ->prev_power_avgs;
> > +
> > +	switch (power->version) {
> > +	case 1:
> > +		for (i = 0; i < power->num_sensors; ++i) {
> > +			ps1 = ((struct power_sensor_1 *)power->data) + i;
> > +
> > +			prevs[i].accumulator =
> > +				get_unaligned_be32(&ps1->accumulator);
> > +			prevs[i].update_tag =
> > +				get_unaligned_be32(&ps1->update_tag);
> > +		}
> > +		break;
> > +	case 2:
> > +		for (i = 0; i < power->num_sensors; ++i) {
> > +			ps2 = ((struct power_sensor_2 *)power->data) + i;
> > +
> > +			prevs[i].accumulator =
> > +				get_unaligned_be64(&ps2->accumulator);
> > +			prevs[i].update_tag =
> > +				get_unaligned_be32(&ps2->update_tag);
> > +		}
> > +		break;
> > +	case 0xA0:
> > +		for (i = 0; i < power->num_sensors; ++i) {
> > +			psa0 = ((struct power_sensor_a0 *)power->data) + i;
> > +
> > +			prevs[i].accumulator = psa0->system.accumulator;
> > +			prevs[i].update_tag = psa0->system.update_tag;
> > +			prevs[i + 1].accumulator = psa0->proc.accumulator;
> > +			prevs[i + 1].update_tag = psa0->proc.update_tag;
> > +			prevs[i + 2].accumulator = psa0->vdd.accumulator;
> > +			prevs[i + 2].update_tag = psa0->vdd.update_tag;
> > +			prevs[i + 3].accumulator = psa0->vdn.accumulator;
> > +			prevs[i + 3].update_tag = psa0->vdn.update_tag;
> > +		}
> > +		break;
> > +	}
> > +}
> > +
> >   static int occ_poll(struct occ *occ)
> >   {
> >   	int rc;
> > @@ -135,6 +182,8 @@ static int occ_poll(struct occ *occ)
> >   	cmd[6] = checksum & 0xFF;	/* checksum lsb */
> >   	cmd[7] = 0;
> >   
> > +	occ_update_prev_power_avgs(occ);
> > +
> >   	/* mutex should already be locked if necessary */
> >   	rc = occ->send_cmd(occ, cmd);
> >   	if (rc) {
> > @@ -208,6 +257,7 @@ int occ_update_response(struct occ *occ)
> >   	/* limit the maximum rate of polling the OCC */
> >   	if (time_after(jiffies, occ->last_update + OCC_UPDATE_FREQUENCY)) {
> >   		rc = occ_poll(occ);
> > +		occ->prev_update = occ->last_update;
> >   		occ->last_update = jiffies;
> >   	} else {
> >   		rc = occ->last_error;
> > @@ -364,6 +414,14 @@ static ssize_t occ_show_freq_2(struct device *dev,
> >   	return snprintf(buf, PAGE_SIZE - 1, "%u\n", val);
> >   }
> >   
> > +static u64 occ_power_avg(struct occ *occ, u8 idx, u64 accum, u32 samples)
> > +{
> > +	struct occ_power_avg *avg = &occ->prev_power_avgs[idx];
> > +
> > +	return div_u64((accum - avg->accumulator) * 1000000ULL,
> > +		       samples - avg->update_tag);
> > +}
> > +
> >   static ssize_t occ_show_power_1(struct device *dev,
> >   				struct device_attribute *attr, char *buf)
> >   {
> > @@ -385,13 +443,12 @@ static ssize_t occ_show_power_1(struct device *dev,
> >   		val = get_unaligned_be16(&power->sensor_id);
> >   		break;
> >   	case 1:
> > -		val = get_unaligned_be32(&power->accumulator) /
> > -			get_unaligned_be32(&power->update_tag);
> > -		val *= 1000000ULL;
> > +		val = occ_power_avg(occ, sattr->index,
> > +				    get_unaligned_be32(&power->accumulator),
> > +				    get_unaligned_be32(&power->update_tag));
> >   		break;
> >   	case 2:
> > -		val = (u64)get_unaligned_be32(&power->update_tag) *
> > -			   occ->powr_sample_time_us;
> > +		val = jiffies_to_usecs(occ->last_update - occ->prev_update);
> >   		break;
> >   	case 3:
> >   		val = get_unaligned_be16(&power->value) * 1000000ULL;
> > @@ -403,12 +460,6 @@ static ssize_t occ_show_power_1(struct device *dev,
> >   	return snprintf(buf, PAGE_SIZE - 1, "%llu\n", val);
> >   }
> >   
> > -static u64 occ_get_powr_avg(u64 *accum, u32 *samples)
> > -{
> > -	return div64_u64(get_unaligned_be64(accum) * 1000000ULL,
> > -			 get_unaligned_be32(samples));
> > -}
> > -
> >   static ssize_t occ_show_power_2(struct device *dev,
> >   				struct device_attribute *attr, char *buf)
> >   {
> > @@ -431,12 +482,12 @@ static ssize_t occ_show_power_2(struct device *dev,
> >   				get_unaligned_be32(&power->sensor_id),
> >   				power->function_id, power->apss_channel);
> >   	case 1:
> > -		val = occ_get_powr_avg(&power->accumulator,
> > -				       &power->update_tag);
> > +		val = occ_power_avg(occ, sattr->index,
> > +				    get_unaligned_be64(&power->accumulator),
> > +				    get_unaligned_be32(&power->update_tag));
> >   		break;
> >   	case 2:
> > -		val = (u64)get_unaligned_be32(&power->update_tag) *
> > -			   occ->powr_sample_time_us;
> > +		val = jiffies_to_usecs(occ->last_update - occ->prev_update);
> >   		break;
> >   	case 3:
> >   		val = get_unaligned_be16(&power->value) * 1000000ULL;
> > @@ -452,6 +503,8 @@ static ssize_t occ_show_power_a0(struct device *dev,
> >   				 struct device_attribute *attr, char *buf)
> >   {
> >   	int rc;
> > +	u32 samples;
> > +	u64 accum;
> >   	u64 val = 0;
> >   	struct power_sensor_a0 *power;
> >   	struct occ *occ = dev_get_drvdata(dev);
> > @@ -469,12 +522,15 @@ static ssize_t occ_show_power_a0(struct device *dev,
> >   		return snprintf(buf, PAGE_SIZE - 1, "%u_system\n",
> >   				get_unaligned_be32(&power->sensor_id));
> >   	case 1:
> > -		val = occ_get_powr_avg(&power->system.accumulator,
> > -				       &power->system.update_tag);
> > +		accum = get_unaligned_be64(&power->system.accumulator);
> > +		samples = get_unaligned_be32(&power->system.update_tag);
> > +		val = occ_power_avg(occ, sattr->index, accum, samples);
> >   		break;
> >   	case 2:
> > -		val = (u64)get_unaligned_be32(&power->system.update_tag) *
> > -			   occ->powr_sample_time_us;
> > +	case 6:
> > +	case 10:
> > +	case 14:
> > +		val = jiffies_to_usecs(occ->last_update - occ->prev_update);
> >   		break;
> >   	case 3:
> >   		val = get_unaligned_be16(&power->system.value) * 1000000ULL;
> > @@ -483,12 +539,9 @@ static ssize_t occ_show_power_a0(struct device *dev,
> >   		return snprintf(buf, PAGE_SIZE - 1, "%u_proc\n",
> >   				get_unaligned_be32(&power->sensor_id));
> >   	case 5:
> > -		val = occ_get_powr_avg(&power->proc.accumulator,
> > -				       &power->proc.update_tag);
> > -		break;
> > -	case 6:
> > -		val = (u64)get_unaligned_be32(&power->proc.update_tag) *
> > -			   occ->powr_sample_time_us;
> > +		accum = get_unaligned_be64(&power->proc.accumulator);
> > +		samples = get_unaligned_be32(&power->proc.update_tag);
> > +		val = occ_power_avg(occ, sattr->index + 1, accum, samples);
> >   		break;
> >   	case 7:
> >   		val = get_unaligned_be16(&power->proc.value) * 1000000ULL;
> > @@ -497,12 +550,9 @@ static ssize_t occ_show_power_a0(struct device *dev,
> >   		return snprintf(buf, PAGE_SIZE - 1, "%u_vdd\n",
> >   				get_unaligned_be32(&power->sensor_id));
> >   	case 9:
> > -		val = occ_get_powr_avg(&power->vdd.accumulator,
> > -				       &power->vdd.update_tag);
> > -		break;
> > -	case 10:
> > -		val = (u64)get_unaligned_be32(&power->vdd.update_tag) *
> > -			   occ->powr_sample_time_us;
> > +		accum = get_unaligned_be64(&power->vdd.accumulator);
> > +		samples = get_unaligned_be32(&power->vdd.update_tag);
> > +		val = occ_power_avg(occ, sattr->index + 2, accum, samples);
> >   		break;
> >   	case 11:
> >   		val = get_unaligned_be16(&power->vdd.value) * 1000000ULL;
> > @@ -511,12 +561,9 @@ static ssize_t occ_show_power_a0(struct device *dev,
> >   		return snprintf(buf, PAGE_SIZE - 1, "%u_vdn\n",
> >   				get_unaligned_be32(&power->sensor_id));
> >   	case 13:
> > -		val = occ_get_powr_avg(&power->vdn.accumulator,
> > -				       &power->vdn.update_tag);
> > -		break;
> > -	case 14:
> > -		val = (u64)get_unaligned_be32(&power->vdn.update_tag) *
> > -			   occ->powr_sample_time_us;
> > +		accum = get_unaligned_be64(&power->vdn.accumulator);
> > +		samples = get_unaligned_be32(&power->vdn.update_tag);
> > +		val = occ_power_avg(occ, sattr->index + 3, accum, samples);
> >   		break;
> >   	case 15:
> >   		val = get_unaligned_be16(&power->vdn.value) * 1000000ULL;
> > @@ -719,6 +766,7 @@ static ssize_t occ_show_extended(struct device *dev,
> >   static int occ_setup_sensor_attrs(struct occ *occ)
> >   {
> >   	unsigned int i, s, num_attrs = 0;
> > +	unsigned int power_avgs_size = 0;
> >   	struct device *dev = occ->bus_dev;
> >   	struct occ_sensors *sensors = &occ->sensors;
> >   	struct occ_attribute *attr;
> > @@ -761,9 +809,13 @@ static int occ_setup_sensor_attrs(struct occ *occ)
> >   		/* fall through */
> >   	case 1:
> >   		num_attrs += (sensors->power.num_sensors * 4);
> > +		power_avgs_size = sizeof(struct occ_power_avg) *
> > +			sensors->power.num_sensors;
> >   		break;
> >   	case 0xA0:
> >   		num_attrs += (sensors->power.num_sensors * 16);
> > +		power_avgs_size = sizeof(struct occ_power_avg) *
> > +			sensors->power.num_sensors * 4;
> >   		show_power = occ_show_power_a0;
> >   		break;
> >   	default:
> > @@ -792,6 +844,13 @@ static int occ_setup_sensor_attrs(struct occ *occ)
> >   		sensors->extended.num_sensors = 0;
> >   	}
> >   
> > +	if (power_avgs_size) {
> > +		occ->prev_power_avgs = devm_kzalloc(dev, power_avgs_size,
> > +						    GFP_KERNEL);
> > +		if (!occ->prev_power_avgs)
> > +			return -ENOMEM;
> > +	}
> > +
> >   	occ->attrs = devm_kzalloc(dev, sizeof(*occ->attrs) * num_attrs,
> >   				  GFP_KERNEL);
> >   	if (!occ->attrs)
> > diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
> > index c676e48..08970f8 100644
> > --- a/drivers/hwmon/occ/common.h
> > +++ b/drivers/hwmon/occ/common.h
> > @@ -87,17 +87,24 @@ struct occ_attribute {
> >   	struct sensor_device_attribute_2 sensor;
> >   };
> >   
> > +struct occ_power_avg {
> > +	u64 accumulator;
> > +	u32 update_tag;
> > +};
> > +
> >   struct occ {
> >   	struct device *bus_dev;
> >   
> >   	struct occ_response resp;
> >   	struct occ_sensors sensors;
> > +	struct occ_power_avg *prev_power_avgs;
> >   
> >   	int powr_sample_time_us;	/* average power sample time */
> >   	u8 poll_cmd_data;		/* to perform OCC poll command */
> >   	int (*send_cmd)(struct occ *occ, u8 *cmd);
> >   
> >   	unsigned long last_update;
> > +	unsigned long prev_update;
> >   	struct mutex lock;		/* lock OCC access */
> >   
> >   	struct device *hwmon;
> > --
> > 1.8.3.1