Linux-Hwmon Archive on lore.kernel.org
 help / color / Atom feed
From: Eddie James <eajames@linux.vnet.ibm.com>
To: Guenter Roeck <linux@roeck-us.net>, Eddie James <eajames@linux.ibm.com>
Cc: linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org,
	jdelvare@suse.com
Subject: Re: [PATCH] hwmon (occ): Switch power average to between poll responses
Date: Thu, 9 May 2019 10:38:38 -0500
Message-ID: <97d20bd2-8060-41a9-f8ee-c33bf7e1079f@linux.vnet.ibm.com> (raw)
In-Reply-To: <20190508210336.GA29619@roeck-us.net>


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@linux.ibm.com>
>> ---
>>   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
>>


      reply index

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-07 19:35 Eddie James
2019-05-08 21:03 ` Guenter Roeck
2019-05-09 15:38   ` Eddie James [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=97d20bd2-8060-41a9-f8ee-c33bf7e1079f@linux.vnet.ibm.com \
    --to=eajames@linux.vnet.ibm.com \
    --cc=eajames@linux.ibm.com \
    --cc=jdelvare@suse.com \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Hwmon Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hwmon/0 linux-hwmon/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hwmon linux-hwmon/ https://lore.kernel.org/linux-hwmon \
		linux-hwmon@vger.kernel.org linux-hwmon@archiver.kernel.org
	public-inbox-index linux-hwmon


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hwmon


AGPL code for this site: git clone https://public-inbox.org/ public-inbox