Re: [PATCH v4 2/6] hwmon: (fam15h_power) Add compute unit accumulated power

[Huang Rui]

On Fri, Mar 11, 2016 at 07:33:55PM -0800, Guenter Roeck wrote:
> On 03/10/2016 06:17 PM, Huang Rui wrote:
> >This patch adds a member in fam15h_power_data which specifies the
> >compute unit accumulated power. It adds do_read_registers_on_cu to do
> >all the read to all MSRs and run it on one of the online cores on each
> >compute unit with smp_call_function_many(). This behavior can decrease
> >IPI numbers.
> >
> >Suggested-by: Borislav Petkov <bp@xxxxxxxxx>
> >Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
> >---
> >  drivers/hwmon/fam15h_power.c | 61 +++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 60 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/hwmon/fam15h_power.c b/drivers/hwmon/fam15h_power.c
> >index 4f695d8..c5e2297 100644
> >--- a/drivers/hwmon/fam15h_power.c
> >+++ b/drivers/hwmon/fam15h_power.c
> >@@ -25,6 +25,8 @@
> >  #include <linux/module.h>
> >  #include <linux/pci.h>
> >  #include <linux/bitops.h>
> >+#include <linux/cpu.h>
> >+#include <linux/cpumask.h>
> >  #include <asm/processor.h>
> >  #include <asm/msr.h>
> >
> >@@ -44,7 +46,9 @@ MODULE_LICENSE("GPL");
> >
> >  #define FAM15H_MIN_NUM_ATTRS		2
> >  #define FAM15H_NUM_GROUPS		2
> >+#define MAX_CUS				8
> >
> >+#define MSR_F15H_CU_PWR_ACCUMULATOR	0xc001007a
> >  #define MSR_F15H_CU_MAX_PWR_ACCUMULATOR	0xc001007b
> >
> >  #define PCI_DEVICE_ID_AMD_15H_M70H_NB_F4 0x15b4
> >@@ -59,6 +63,8 @@ struct fam15h_power_data {
> >  	struct attribute_group group;
> >  	/* maximum accumulated power of a compute unit */
> >  	u64 max_cu_acc_power;
> >+	/* accumulated power of the compute units */
> >+	u64 cu_acc_power[MAX_CUS];
> >  };
> >
> >  static ssize_t show_power(struct device *dev,
> >@@ -125,6 +131,59 @@ static ssize_t show_power_crit(struct device *dev,
> >  }
> >  static DEVICE_ATTR(power1_crit, S_IRUGO, show_power_crit, NULL);
> >
> >+static void do_read_registers_on_cu(void *_data)
> >+{
> >+	struct fam15h_power_data *data = _data;
> >+	int cpu, cu;
> >+
> >+	cpu = smp_processor_id();
> >+
> >+	cu = cpu / smp_num_siblings;
> >+
> 
> If smp is not configured:
> 
> drivers/hwmon/fam15h_power.c: In function ?do_read_registers_on_cu?:
> drivers/hwmon/fam15h_power.c:144:13: error: ?smp_num_siblings? undeclared (first use in this function)
> 

Nice catch, how about define a static variable like below:

#ifdef CONFIG_SMP
static int cores_per_cu = smp_num_siblings;
#else
static int cores_per_cu = 1;
#endif

Thanks,
Rui