[PATCH 07/11] powercap/intel_rapl: simplify rapl_find_package()

[PATCH 07/11] powercap/intel_rapl: simplify rapl_find_package()

[Len Brown]

From: Zhang Rui <rui.zhang@intel.com>

Simplify how the code to discover a package is called.
Rename find_package_by_id() to rapl_find_package()

No functional change.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/powercap/intel_rapl.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c
index 6cdb2c14eee4..6057d9695fed 100644
--- a/drivers/powercap/intel_rapl.c
+++ b/drivers/powercap/intel_rapl.c
@@ -264,8 +264,9 @@ static struct powercap_control_type *control_type; /* PowerCap Controller */
 static struct rapl_domain *platform_rapl_domain; /* Platform (PSys) domain */
 
 /* caller to ensure CPU hotplug lock is held */
-static struct rapl_package *find_package_by_id(int id)
+static struct rapl_package *rapl_find_package(int cpu)
 {
+	int id = topology_physical_package_id(cpu);
 	struct rapl_package *rp;
 
 	list_for_each_entry(rp, &rapl_packages, plist) {
@@ -1298,7 +1299,7 @@ static int __init rapl_register_psys(void)
 	rd->rpl[0].name = pl1_name;
 	rd->rpl[1].prim_id = PL2_ENABLE;
 	rd->rpl[1].name = pl2_name;
-	rd->rp = find_package_by_id(0);
+	rd->rp = rapl_find_package(0);
 
 	power_zone = powercap_register_zone(&rd->power_zone, control_type,
 					    "psys", NULL,
@@ -1454,8 +1455,9 @@ static void rapl_remove_package(struct rapl_package *rp)
 }
 
 /* called from CPU hotplug notifier, hotplug lock held */
-static struct rapl_package *rapl_add_package(int cpu, int pkgid)
+static struct rapl_package *rapl_add_package(int cpu)
 {
+	int id = topology_physical_package_id(cpu);
 	struct rapl_package *rp;
 	int ret;
 
@@ -1464,7 +1466,7 @@ static struct rapl_package *rapl_add_package(int cpu, int pkgid)
 		return ERR_PTR(-ENOMEM);
 
 	/* add the new package to the list */
-	rp->id = pkgid;
+	rp->id = id;
 	rp->lead_cpu = cpu;
 
 	/* check if the package contains valid domains */
@@ -1495,12 +1497,11 @@ static struct rapl_package *rapl_add_package(int cpu, int pkgid)
  */
 static int rapl_cpu_online(unsigned int cpu)
 {
-	int pkgid = topology_physical_package_id(cpu);
 	struct rapl_package *rp;
 
-	rp = find_package_by_id(pkgid);
+	rp = rapl_find_package(cpu);
 	if (!rp) {
-		rp = rapl_add_package(cpu, pkgid);
+		rp = rapl_add_package(cpu);
 		if (IS_ERR(rp))
 			return PTR_ERR(rp);
 	}
@@ -1510,11 +1511,10 @@ static int rapl_cpu_online(unsigned int cpu)
 
 static int rapl_cpu_down_prep(unsigned int cpu)
 {
-	int pkgid = topology_physical_package_id(cpu);
 	struct rapl_package *rp;
 	int lead_cpu;
 
-	rp = find_package_by_id(pkgid);
+	rp = rapl_find_package(cpu);
 	if (!rp)
 		return 0;
 
-- 
2.18.0-rc0

Re: [PATCH 07/11] powercap/intel_rapl: simplify rapl_find_package()

[Rafael J. Wysocki]

On Tue, Feb 19, 2019 at 4:41 AM Len Brown <lenb@kernel.org> wrote:
>
> From: Zhang Rui <rui.zhang@intel.com>
>
> Simplify how the code to discover a package is called.
> Rename find_package_by_id() to rapl_find_package()
>
> No functional change.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> Signed-off-by: Len Brown <len.brown@intel.com>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Len Brown <len.brown@intel.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/powercap/intel_rapl.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c
> index 6cdb2c14eee4..6057d9695fed 100644
> --- a/drivers/powercap/intel_rapl.c
> +++ b/drivers/powercap/intel_rapl.c
> @@ -264,8 +264,9 @@ static struct powercap_control_type *control_type; /* PowerCap Controller */
>  static struct rapl_domain *platform_rapl_domain; /* Platform (PSys) domain */
>
>  /* caller to ensure CPU hotplug lock is held */
> -static struct rapl_package *find_package_by_id(int id)
> +static struct rapl_package *rapl_find_package(int cpu)
>  {
> +       int id = topology_physical_package_id(cpu);
>         struct rapl_package *rp;
>
>         list_for_each_entry(rp, &rapl_packages, plist) {
> @@ -1298,7 +1299,7 @@ static int __init rapl_register_psys(void)
>         rd->rpl[0].name = pl1_name;
>         rd->rpl[1].prim_id = PL2_ENABLE;
>         rd->rpl[1].name = pl2_name;
> -       rd->rp = find_package_by_id(0);
> +       rd->rp = rapl_find_package(0);
>
>         power_zone = powercap_register_zone(&rd->power_zone, control_type,
>                                             "psys", NULL,
> @@ -1454,8 +1455,9 @@ static void rapl_remove_package(struct rapl_package *rp)
>  }
>
>  /* called from CPU hotplug notifier, hotplug lock held */
> -static struct rapl_package *rapl_add_package(int cpu, int pkgid)
> +static struct rapl_package *rapl_add_package(int cpu)
>  {
> +       int id = topology_physical_package_id(cpu);
>         struct rapl_package *rp;
>         int ret;
>
> @@ -1464,7 +1466,7 @@ static struct rapl_package *rapl_add_package(int cpu, int pkgid)
>                 return ERR_PTR(-ENOMEM);
>
>         /* add the new package to the list */
> -       rp->id = pkgid;
> +       rp->id = id;
>         rp->lead_cpu = cpu;
>
>         /* check if the package contains valid domains */
> @@ -1495,12 +1497,11 @@ static struct rapl_package *rapl_add_package(int cpu, int pkgid)
>   */
>  static int rapl_cpu_online(unsigned int cpu)
>  {
> -       int pkgid = topology_physical_package_id(cpu);
>         struct rapl_package *rp;
>
> -       rp = find_package_by_id(pkgid);
> +       rp = rapl_find_package(cpu);
>         if (!rp) {
> -               rp = rapl_add_package(cpu, pkgid);
> +               rp = rapl_add_package(cpu);
>                 if (IS_ERR(rp))
>                         return PTR_ERR(rp);
>         }
> @@ -1510,11 +1511,10 @@ static int rapl_cpu_online(unsigned int cpu)
>
>  static int rapl_cpu_down_prep(unsigned int cpu)
>  {
> -       int pkgid = topology_physical_package_id(cpu);
>         struct rapl_package *rp;
>         int lead_cpu;
>
> -       rp = find_package_by_id(pkgid);
> +       rp = rapl_find_package(cpu);
>         if (!rp)
>                 return 0;
>
> --
> 2.18.0-rc0
>

[PATCH 08/11] powercap/intel_rapl: Support multi-die/package

[Len Brown]

From: Zhang Rui <rui.zhang@intel.com>

On the new dual-die/package systems, the RAPL MSR becomes die-scope.
Thus instead of one powercap device per physical package, now there
should be one powercap device for each unique die on these systems.

This patch introduces intel_rapl driver support for new
dual-die/package systems.

On the hardwares that do not have multi-die, topology_unique_die_id()
equals topology_physical_package_id(), thus there is no functional change.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/powercap/intel_rapl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c
index 6057d9695fed..e004707283da 100644
--- a/drivers/powercap/intel_rapl.c
+++ b/drivers/powercap/intel_rapl.c
@@ -266,7 +266,7 @@ static struct rapl_domain *platform_rapl_domain; /* Platform (PSys) domain */
 /* caller to ensure CPU hotplug lock is held */
 static struct rapl_package *rapl_find_package(int cpu)
 {
-	int id = topology_physical_package_id(cpu);
+	int id = topology_unique_die_id(cpu);
 	struct rapl_package *rp;
 
 	list_for_each_entry(rp, &rapl_packages, plist) {
@@ -1457,7 +1457,7 @@ static void rapl_remove_package(struct rapl_package *rp)
 /* called from CPU hotplug notifier, hotplug lock held */
 static struct rapl_package *rapl_add_package(int cpu)
 {
-	int id = topology_physical_package_id(cpu);
+	int id = topology_unique_die_id(cpu);
 	struct rapl_package *rp;
 	int ret;
 
-- 
2.18.0-rc0

Re: [PATCH 08/11] powercap/intel_rapl: Support multi-die/package

[Rafael J. Wysocki]

On Tue, Feb 19, 2019 at 4:40 AM Len Brown <lenb@kernel.org> wrote:
>
> From: Zhang Rui <rui.zhang@intel.com>
>
> On the new dual-die/package systems, the RAPL MSR becomes die-scope.
> Thus instead of one powercap device per physical package, now there
> should be one powercap device for each unique die on these systems.
>
> This patch introduces intel_rapl driver support for new
> dual-die/package systems.
>
> On the hardwares that do not have multi-die, topology_unique_die_id()
> equals topology_physical_package_id(), thus there is no functional change.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> Signed-off-by: Len Brown <len.brown@intel.com>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Len Brown <len.brown@intel.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/powercap/intel_rapl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c
> index 6057d9695fed..e004707283da 100644
> --- a/drivers/powercap/intel_rapl.c
> +++ b/drivers/powercap/intel_rapl.c
> @@ -266,7 +266,7 @@ static struct rapl_domain *platform_rapl_domain; /* Platform (PSys) domain */
>  /* caller to ensure CPU hotplug lock is held */
>  static struct rapl_package *rapl_find_package(int cpu)
>  {
> -       int id = topology_physical_package_id(cpu);
> +       int id = topology_unique_die_id(cpu);
>         struct rapl_package *rp;
>
>         list_for_each_entry(rp, &rapl_packages, plist) {
> @@ -1457,7 +1457,7 @@ static void rapl_remove_package(struct rapl_package *rp)
>  /* called from CPU hotplug notifier, hotplug lock held */
>  static struct rapl_package *rapl_add_package(int cpu)
>  {
> -       int id = topology_physical_package_id(cpu);
> +       int id = topology_unique_die_id(cpu);
>         struct rapl_package *rp;
>         int ret;
>
> --
> 2.18.0-rc0
>

Re: [PATCH 08/11] powercap/intel_rapl: Support multi-die/package

[Peter Zijlstra]

On Mon, Feb 18, 2019 at 10:40:10PM -0500, Len Brown wrote:
> From: Zhang Rui <rui.zhang@intel.com>
> 
> On the new dual-die/package systems, the RAPL MSR becomes die-scope.
> Thus instead of one powercap device per physical package, now there
> should be one powercap device for each unique die on these systems.
> 
> This patch introduces intel_rapl driver support for new
> dual-die/package systems.
> 
> On the hardwares that do not have multi-die, topology_unique_die_id()
> equals topology_physical_package_id(), thus there is no functional change.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> Signed-off-by: Len Brown <len.brown@intel.com>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
>  drivers/powercap/intel_rapl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c
> index 6057d9695fed..e004707283da 100644
> --- a/drivers/powercap/intel_rapl.c
> +++ b/drivers/powercap/intel_rapl.c
> @@ -266,7 +266,7 @@ static struct rapl_domain *platform_rapl_domain; /* Platform (PSys) domain */
>  /* caller to ensure CPU hotplug lock is held */
>  static struct rapl_package *rapl_find_package(int cpu)
>  {
> -	int id = topology_physical_package_id(cpu);
> +	int id = topology_unique_die_id(cpu);
>  	struct rapl_package *rp;
>  
>  	list_for_each_entry(rp, &rapl_packages, plist) {
> @@ -1457,7 +1457,7 @@ static void rapl_remove_package(struct rapl_package *rp)
>  /* called from CPU hotplug notifier, hotplug lock held */
>  static struct rapl_package *rapl_add_package(int cpu)
>  {
> -	int id = topology_physical_package_id(cpu);
> +	int id = topology_unique_die_id(cpu);
>  	struct rapl_package *rp;
>  	int ret;

And now your new function names are misnomers.

Re: [PATCH 08/11] powercap/intel_rapl: Support multi-die/package

[Len Brown]

On Wed, Feb 20, 2019 at 6:02 AM Peter Zijlstra <peterz@infradead.org> wrote:

> >       list_for_each_entry(rp, &rapl_packages, plist) {
> > @@ -1457,7 +1457,7 @@ static void rapl_remove_package(struct rapl_package *rp)
> >  /* called from CPU hotplug notifier, hotplug lock held */
> >  static struct rapl_package *rapl_add_package(int cpu)
> >  {
> > -     int id = topology_physical_package_id(cpu);
> > +     int id = topology_unique_die_id(cpu);
> >       struct rapl_package *rp;
> >       int ret;
>
> And now your new function names are misnomers.

That is fair.

Seems that a subsequent re-name-only patch is appropriate.

Len Brown, Intel Open Source Technology Center

Re: [PATCH 08/11] powercap/intel_rapl: Support multi-die/package

[Len Brown]

On Thu, Feb 21, 2019 at 12:44 AM Len Brown <lenb@kernel.org> wrote:
>
> On Wed, Feb 20, 2019 at 6:02 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> > >       list_for_each_entry(rp, &rapl_packages, plist) {
> > > @@ -1457,7 +1457,7 @@ static void rapl_remove_package(struct rapl_package *rp)
> > >  /* called from CPU hotplug notifier, hotplug lock held */
> > >  static struct rapl_package *rapl_add_package(int cpu)
> > >  {
> > > -     int id = topology_physical_package_id(cpu);
> > > +     int id = topology_unique_die_id(cpu);
> > >       struct rapl_package *rp;
> > >       int ret;
> >
> > And now your new function names are misnomers.
>
> That is fair.
>
> Seems that a subsequent re-name-only patch is appropriate.

I'm not sure that re-naming these functions is a good idea.

Fundamentally, the reason stems from the SDM being in-consistent.
And the reason that the SDM is inconsistent is for compatibility.

ie. the PACKAGE MSRs in the SDM are still called PACKAGE MSRs,
even though on a multi-die system, they are DIE scoped.
There is no plan to re-name all of those MSRs.

And so what do you call a routine that parses a PACKAGE_RAPL domain?
Well, it is still called PACKAGE MSR, even though the code is smart enough
to know that on a multi-die system, its scope is die-scoped, not package-scoped.

And yes, just to confuse things, there WILL be PACKAGE scope MSRs
in the future that span multiple die on multi-die systems.  No, it will not
be a surprise when they appear -- by definition, they will be different
and incompatible with previous PACKAGE MSRs.  We will need to update
some software to be smart about handling them -- no blind assumptions
on using the word "package" in this context.

So unless Rui disagrees, I'm inclined to leave these routine names alone.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 08/11] powercap/intel_rapl: Support multi-die/package

[Zhang Rui]

On 一, 2019-02-25 at 23:41 -0500, Len Brown wrote:
> On Thu, Feb 21, 2019 at 12:44 AM Len Brown <lenb@kernel.org> wrote:
> > 
> > 
> > On Wed, Feb 20, 2019 at 6:02 AM Peter Zijlstra <peterz@infradead.or
> > g> wrote:
> > 
> > > 
> > > > 
> > > >       list_for_each_entry(rp, &rapl_packages, plist) {
> > > > @@ -1457,7 +1457,7 @@ static void rapl_remove_package(struct
> > > > rapl_package *rp)
> > > >  /* called from CPU hotplug notifier, hotplug lock held */
> > > >  static struct rapl_package *rapl_add_package(int cpu)
> > > >  {
> > > > -     int id = topology_physical_package_id(cpu);
> > > > +     int id = topology_unique_die_id(cpu);
> > > >       struct rapl_package *rp;
> > > >       int ret;
> > > And now your new function names are misnomers.
> > That is fair.
> > 
> > Seems that a subsequent re-name-only patch is appropriate.
> I'm not sure that re-naming these functions is a good idea.
> 
> Fundamentally, the reason stems from the SDM being in-consistent.
> And the reason that the SDM is inconsistent is for compatibility.
> 
> ie. the PACKAGE MSRs in the SDM are still called PACKAGE MSRs,
> even though on a multi-die system, they are DIE scoped.
> There is no plan to re-name all of those MSRs.
> 
> And so what do you call a routine that parses a PACKAGE_RAPL domain?
> Well, it is still called PACKAGE MSR, even though the code is smart
> enough
> to know that on a multi-die system, its scope is die-scoped, not
> package-scoped.
> 
Agreed.

rapl_add_package() actually adds a package RAPL domain, and "package
RAPL domain" comes from SDM, which is used to describe the RAPL domain
that uses the package MSRs.

IMO, we can keep using "package RAPL domain" as the name of this
certain kind of RAPL domains, but just stop aligning it with the cpu
physical package.
Actually, my next patch fixes the places that had this assumption.
In short, "package domain foo" is okay, but "domain for package X"
should be avoided.

thanks,
rui

> And yes, just to confuse things, there WILL be PACKAGE scope MSRs
> in the future that span multiple die on multi-die systems.  No, it
> will not
> be a surprise when they appear -- by definition, they will be
> different
> and incompatible with previous PACKAGE MSRs.  We will need to update
> some software to be smart about handling them -- no blind assumptions
> on using the word "package" in this context.
> 
> So unless Rui disagrees, I'm inclined to leave these routine names
> alone.
> 
> thanks,
> Len Brown, Intel Open Source Technology Center

[PATCH 09/11] powercap/intel_rapl: update rapl domain name and debug messages

[Len Brown]

From: Zhang Rui <rui.zhang@intel.com>

The RAPL domain "name" attribute contains "Package-N",
which is ambiguous on multi-die per-package systems.

Update the name to "package-X-die-Y" on those systems.

No change on systems without multi-die.

Driver debug messages are also updated.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/powercap/intel_rapl.c | 57 ++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 25 deletions(-)

diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c
index e004707283da..c990eee45239 100644
--- a/drivers/powercap/intel_rapl.c
+++ b/drivers/powercap/intel_rapl.c
@@ -178,12 +178,15 @@ struct rapl_domain {
 #define power_zone_to_rapl_domain(_zone) \
 	container_of(_zone, struct rapl_domain, power_zone)
 
+/* maximum rapl package domain name: package-%d-die-%d */
+#define PACKAGE_DOMAIN_NAME_LENGTH 30
 
-/* Each physical package contains multiple domains, these are the common
+
+/* Each rapl package contains multiple domains, these are the common
  * data across RAPL domains within a package.
  */
 struct rapl_package {
-	unsigned int id; /* physical package/socket id */
+	unsigned int id; /* logical die id, equals physical 1-die systems */
 	unsigned int nr_domains;
 	unsigned long domain_map; /* bit map of active domains */
 	unsigned int power_unit;
@@ -198,6 +201,7 @@ struct rapl_package {
 	int lead_cpu; /* one active cpu per package for access */
 	/* Track active cpus */
 	struct cpumask cpumask;
+	char name[PACKAGE_DOMAIN_NAME_LENGTH];
 };
 
 struct rapl_defaults {
@@ -926,8 +930,8 @@ static int rapl_check_unit_core(struct rapl_package *rp, int cpu)
 	value = (msr_val & TIME_UNIT_MASK) >> TIME_UNIT_OFFSET;
 	rp->time_unit = 1000000 / (1 << value);
 
-	pr_debug("Core CPU package %d energy=%dpJ, time=%dus, power=%duW\n",
-		rp->id, rp->energy_unit, rp->time_unit, rp->power_unit);
+	pr_debug("Core CPU %s energy=%dpJ, time=%dus, power=%duW\n",
+		rp->name, rp->energy_unit, rp->time_unit, rp->power_unit);
 
 	return 0;
 }
@@ -951,8 +955,8 @@ static int rapl_check_unit_atom(struct rapl_package *rp, int cpu)
 	value = (msr_val & TIME_UNIT_MASK) >> TIME_UNIT_OFFSET;
 	rp->time_unit = 1000000 / (1 << value);
 
-	pr_debug("Atom package %d energy=%dpJ, time=%dus, power=%duW\n",
-		rp->id, rp->energy_unit, rp->time_unit, rp->power_unit);
+	pr_debug("Atom %s energy=%dpJ, time=%dus, power=%duW\n",
+		rp->name, rp->energy_unit, rp->time_unit, rp->power_unit);
 
 	return 0;
 }
@@ -1179,7 +1183,7 @@ static void rapl_update_domain_data(struct rapl_package *rp)
 	u64 val;
 
 	for (dmn = 0; dmn < rp->nr_domains; dmn++) {
-		pr_debug("update package %d domain %s data\n", rp->id,
+		pr_debug("update %s domain %s data\n", rp->name,
 			 rp->domains[dmn].name);
 		/* exclude non-raw primitives */
 		for (prim = 0; prim < NR_RAW_PRIMITIVES; prim++) {
@@ -1204,7 +1208,6 @@ static void rapl_unregister_powercap(void)
 static int rapl_package_register_powercap(struct rapl_package *rp)
 {
 	struct rapl_domain *rd;
-	char dev_name[17]; /* max domain name = 7 + 1 + 8 for int + 1 for null*/
 	struct powercap_zone *power_zone = NULL;
 	int nr_pl, ret;
 
@@ -1215,20 +1218,16 @@ static int rapl_package_register_powercap(struct rapl_package *rp)
 	for (rd = rp->domains; rd < rp->domains + rp->nr_domains; rd++) {
 		if (rd->id == RAPL_DOMAIN_PACKAGE) {
 			nr_pl = find_nr_power_limit(rd);
-			pr_debug("register socket %d package domain %s\n",
-				rp->id, rd->name);
-			memset(dev_name, 0, sizeof(dev_name));
-			snprintf(dev_name, sizeof(dev_name), "%s-%d",
-				rd->name, rp->id);
+			pr_debug("register package domain %s\n", rp->name);
 			power_zone = powercap_register_zone(&rd->power_zone,
 							control_type,
-							dev_name, NULL,
+							rp->name, NULL,
 							&zone_ops[rd->id],
 							nr_pl,
 							&constraint_ops);
 			if (IS_ERR(power_zone)) {
-				pr_debug("failed to register package, %d\n",
-					rp->id);
+				pr_debug("failed to register power zone %s\n",
+					rp->name);
 				return PTR_ERR(power_zone);
 			}
 			/* track parent zone in per package/socket data */
@@ -1254,8 +1253,8 @@ static int rapl_package_register_powercap(struct rapl_package *rp)
 						&constraint_ops);
 
 		if (IS_ERR(power_zone)) {
-			pr_debug("failed to register power_zone, %d:%s:%s\n",
-				rp->id, rd->name, dev_name);
+			pr_debug("failed to register power_zone, %s:%s\n",
+				rp->name, rd->name);
 			ret = PTR_ERR(power_zone);
 			goto err_cleanup;
 		}
@@ -1268,7 +1267,7 @@ static int rapl_package_register_powercap(struct rapl_package *rp)
 	 * failed after the first domain setup.
 	 */
 	while (--rd >= rp->domains) {
-		pr_debug("unregister package %d domain %s\n", rp->id, rd->name);
+		pr_debug("unregister %s domain %s\n", rp->name, rd->name);
 		powercap_unregister_zone(control_type, &rd->power_zone);
 	}
 
@@ -1378,8 +1377,8 @@ static void rapl_detect_powerlimit(struct rapl_domain *rd)
 	/* check if the domain is locked by BIOS, ignore if MSR doesn't exist */
 	if (!rapl_read_data_raw(rd, FW_LOCK, false, &val64)) {
 		if (val64) {
-			pr_info("RAPL package %d domain %s locked by BIOS\n",
-				rd->rp->id, rd->name);
+			pr_info("RAPL %s domain %s locked by BIOS\n",
+				rd->rp->name, rd->name);
 			rd->state |= DOMAIN_STATE_BIOS_LOCKED;
 		}
 	}
@@ -1408,10 +1407,10 @@ static int rapl_detect_domains(struct rapl_package *rp, int cpu)
 	}
 	rp->nr_domains = bitmap_weight(&rp->domain_map,	RAPL_DOMAIN_MAX);
 	if (!rp->nr_domains) {
-		pr_debug("no valid rapl domains found in package %d\n", rp->id);
+		pr_debug("no valid rapl domains found in %s\n", rp->name);
 		return -ENODEV;
 	}
-	pr_debug("found %d domains on package %d\n", rp->nr_domains, rp->id);
+	pr_debug("found %d domains on %s\n", rp->nr_domains, rp->name);
 
 	rp->domains = kcalloc(rp->nr_domains + 1, sizeof(struct rapl_domain),
 			GFP_KERNEL);
@@ -1444,8 +1443,8 @@ static void rapl_remove_package(struct rapl_package *rp)
 			rd_package = rd;
 			continue;
 		}
-		pr_debug("remove package, undo power limit on %d: %s\n",
-			 rp->id, rd->name);
+		pr_debug("remove package, undo power limit on %s: %s\n",
+			 rp->name, rd->name);
 		powercap_unregister_zone(control_type, &rd->power_zone);
 	}
 	/* do parent zone last */
@@ -1459,6 +1458,7 @@ static struct rapl_package *rapl_add_package(int cpu)
 {
 	int id = topology_unique_die_id(cpu);
 	struct rapl_package *rp;
+	struct cpuinfo_x86 *c = &cpu_data(cpu);
 	int ret;
 
 	rp = kzalloc(sizeof(struct rapl_package), GFP_KERNEL);
@@ -1469,6 +1469,13 @@ static struct rapl_package *rapl_add_package(int cpu)
 	rp->id = id;
 	rp->lead_cpu = cpu;
 
+	if (c->x86_max_dies > 1)
+		snprintf(rp->name, PACKAGE_DOMAIN_NAME_LENGTH,
+			"package-%d-die-%d", c->phys_proc_id, c->cpu_die_id);
+	else
+		snprintf(rp->name, PACKAGE_DOMAIN_NAME_LENGTH, "package-%d",
+			c->phys_proc_id);
+
 	/* check if the package contains valid domains */
 	if (rapl_detect_domains(rp, cpu) ||
 		rapl_defaults->check_unit(rp, cpu)) {
-- 
2.18.0-rc0

Re: [PATCH 09/11] powercap/intel_rapl: update rapl domain name and debug messages

[Rafael J. Wysocki]

On Tue, Feb 19, 2019 at 4:40 AM Len Brown <lenb@kernel.org> wrote:
>
> From: Zhang Rui <rui.zhang@intel.com>
>
> The RAPL domain "name" attribute contains "Package-N",
> which is ambiguous on multi-die per-package systems.
>
> Update the name to "package-X-die-Y" on those systems.
>
> No change on systems without multi-die.
>
> Driver debug messages are also updated.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> Signed-off-by: Len Brown <len.brown@intel.com>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Len Brown <len.brown@intel.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/powercap/intel_rapl.c | 57 ++++++++++++++++++++---------------
>  1 file changed, 32 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c
> index e004707283da..c990eee45239 100644
> --- a/drivers/powercap/intel_rapl.c
> +++ b/drivers/powercap/intel_rapl.c
> @@ -178,12 +178,15 @@ struct rapl_domain {
>  #define power_zone_to_rapl_domain(_zone) \
>         container_of(_zone, struct rapl_domain, power_zone)
>
> +/* maximum rapl package domain name: package-%d-die-%d */
> +#define PACKAGE_DOMAIN_NAME_LENGTH 30
>
> -/* Each physical package contains multiple domains, these are the common
> +
> +/* Each rapl package contains multiple domains, these are the common
>   * data across RAPL domains within a package.
>   */
>  struct rapl_package {
> -       unsigned int id; /* physical package/socket id */
> +       unsigned int id; /* logical die id, equals physical 1-die systems */
>         unsigned int nr_domains;
>         unsigned long domain_map; /* bit map of active domains */
>         unsigned int power_unit;
> @@ -198,6 +201,7 @@ struct rapl_package {
>         int lead_cpu; /* one active cpu per package for access */
>         /* Track active cpus */
>         struct cpumask cpumask;
> +       char name[PACKAGE_DOMAIN_NAME_LENGTH];
>  };
>
>  struct rapl_defaults {
> @@ -926,8 +930,8 @@ static int rapl_check_unit_core(struct rapl_package *rp, int cpu)
>         value = (msr_val & TIME_UNIT_MASK) >> TIME_UNIT_OFFSET;
>         rp->time_unit = 1000000 / (1 << value);
>
> -       pr_debug("Core CPU package %d energy=%dpJ, time=%dus, power=%duW\n",
> -               rp->id, rp->energy_unit, rp->time_unit, rp->power_unit);
> +       pr_debug("Core CPU %s energy=%dpJ, time=%dus, power=%duW\n",
> +               rp->name, rp->energy_unit, rp->time_unit, rp->power_unit);
>
>         return 0;
>  }
> @@ -951,8 +955,8 @@ static int rapl_check_unit_atom(struct rapl_package *rp, int cpu)
>         value = (msr_val & TIME_UNIT_MASK) >> TIME_UNIT_OFFSET;
>         rp->time_unit = 1000000 / (1 << value);
>
> -       pr_debug("Atom package %d energy=%dpJ, time=%dus, power=%duW\n",
> -               rp->id, rp->energy_unit, rp->time_unit, rp->power_unit);
> +       pr_debug("Atom %s energy=%dpJ, time=%dus, power=%duW\n",
> +               rp->name, rp->energy_unit, rp->time_unit, rp->power_unit);
>
>         return 0;
>  }
> @@ -1179,7 +1183,7 @@ static void rapl_update_domain_data(struct rapl_package *rp)
>         u64 val;
>
>         for (dmn = 0; dmn < rp->nr_domains; dmn++) {
> -               pr_debug("update package %d domain %s data\n", rp->id,
> +               pr_debug("update %s domain %s data\n", rp->name,
>                          rp->domains[dmn].name);
>                 /* exclude non-raw primitives */
>                 for (prim = 0; prim < NR_RAW_PRIMITIVES; prim++) {
> @@ -1204,7 +1208,6 @@ static void rapl_unregister_powercap(void)
>  static int rapl_package_register_powercap(struct rapl_package *rp)
>  {
>         struct rapl_domain *rd;
> -       char dev_name[17]; /* max domain name = 7 + 1 + 8 for int + 1 for null*/
>         struct powercap_zone *power_zone = NULL;
>         int nr_pl, ret;
>
> @@ -1215,20 +1218,16 @@ static int rapl_package_register_powercap(struct rapl_package *rp)
>         for (rd = rp->domains; rd < rp->domains + rp->nr_domains; rd++) {
>                 if (rd->id == RAPL_DOMAIN_PACKAGE) {
>                         nr_pl = find_nr_power_limit(rd);
> -                       pr_debug("register socket %d package domain %s\n",
> -                               rp->id, rd->name);
> -                       memset(dev_name, 0, sizeof(dev_name));
> -                       snprintf(dev_name, sizeof(dev_name), "%s-%d",
> -                               rd->name, rp->id);
> +                       pr_debug("register package domain %s\n", rp->name);
>                         power_zone = powercap_register_zone(&rd->power_zone,
>                                                         control_type,
> -                                                       dev_name, NULL,
> +                                                       rp->name, NULL,
>                                                         &zone_ops[rd->id],
>                                                         nr_pl,
>                                                         &constraint_ops);
>                         if (IS_ERR(power_zone)) {
> -                               pr_debug("failed to register package, %d\n",
> -                                       rp->id);
> +                               pr_debug("failed to register power zone %s\n",
> +                                       rp->name);
>                                 return PTR_ERR(power_zone);
>                         }
>                         /* track parent zone in per package/socket data */
> @@ -1254,8 +1253,8 @@ static int rapl_package_register_powercap(struct rapl_package *rp)
>                                                 &constraint_ops);
>
>                 if (IS_ERR(power_zone)) {
> -                       pr_debug("failed to register power_zone, %d:%s:%s\n",
> -                               rp->id, rd->name, dev_name);
> +                       pr_debug("failed to register power_zone, %s:%s\n",
> +                               rp->name, rd->name);
>                         ret = PTR_ERR(power_zone);
>                         goto err_cleanup;
>                 }
> @@ -1268,7 +1267,7 @@ static int rapl_package_register_powercap(struct rapl_package *rp)
>          * failed after the first domain setup.
>          */
>         while (--rd >= rp->domains) {
> -               pr_debug("unregister package %d domain %s\n", rp->id, rd->name);
> +               pr_debug("unregister %s domain %s\n", rp->name, rd->name);
>                 powercap_unregister_zone(control_type, &rd->power_zone);
>         }
>
> @@ -1378,8 +1377,8 @@ static void rapl_detect_powerlimit(struct rapl_domain *rd)
>         /* check if the domain is locked by BIOS, ignore if MSR doesn't exist */
>         if (!rapl_read_data_raw(rd, FW_LOCK, false, &val64)) {
>                 if (val64) {
> -                       pr_info("RAPL package %d domain %s locked by BIOS\n",
> -                               rd->rp->id, rd->name);
> +                       pr_info("RAPL %s domain %s locked by BIOS\n",
> +                               rd->rp->name, rd->name);
>                         rd->state |= DOMAIN_STATE_BIOS_LOCKED;
>                 }
>         }
> @@ -1408,10 +1407,10 @@ static int rapl_detect_domains(struct rapl_package *rp, int cpu)
>         }
>         rp->nr_domains = bitmap_weight(&rp->domain_map, RAPL_DOMAIN_MAX);
>         if (!rp->nr_domains) {
> -               pr_debug("no valid rapl domains found in package %d\n", rp->id);
> +               pr_debug("no valid rapl domains found in %s\n", rp->name);
>                 return -ENODEV;
>         }
> -       pr_debug("found %d domains on package %d\n", rp->nr_domains, rp->id);
> +       pr_debug("found %d domains on %s\n", rp->nr_domains, rp->name);
>
>         rp->domains = kcalloc(rp->nr_domains + 1, sizeof(struct rapl_domain),
>                         GFP_KERNEL);
> @@ -1444,8 +1443,8 @@ static void rapl_remove_package(struct rapl_package *rp)
>                         rd_package = rd;
>                         continue;
>                 }
> -               pr_debug("remove package, undo power limit on %d: %s\n",
> -                        rp->id, rd->name);
> +               pr_debug("remove package, undo power limit on %s: %s\n",
> +                        rp->name, rd->name);
>                 powercap_unregister_zone(control_type, &rd->power_zone);
>         }
>         /* do parent zone last */
> @@ -1459,6 +1458,7 @@ static struct rapl_package *rapl_add_package(int cpu)
>  {
>         int id = topology_unique_die_id(cpu);
>         struct rapl_package *rp;
> +       struct cpuinfo_x86 *c = &cpu_data(cpu);
>         int ret;
>
>         rp = kzalloc(sizeof(struct rapl_package), GFP_KERNEL);
> @@ -1469,6 +1469,13 @@ static struct rapl_package *rapl_add_package(int cpu)
>         rp->id = id;
>         rp->lead_cpu = cpu;
>
> +       if (c->x86_max_dies > 1)
> +               snprintf(rp->name, PACKAGE_DOMAIN_NAME_LENGTH,
> +                       "package-%d-die-%d", c->phys_proc_id, c->cpu_die_id);
> +       else
> +               snprintf(rp->name, PACKAGE_DOMAIN_NAME_LENGTH, "package-%d",
> +                       c->phys_proc_id);
> +
>         /* check if the package contains valid domains */
>         if (rapl_detect_domains(rp, cpu) ||
>                 rapl_defaults->check_unit(rp, cpu)) {
> --
> 2.18.0-rc0
>

[PATCH 11/11] hwmon/coretemp: Support multi-die/package

[Len Brown]

From: Zhang Rui <rui.zhang@intel.com>

This patch introduces coretemp driver support
for new dual-die/package systems.

On the new dual-die/package systems, the package temperature MSRs becomes
die-scope. Thus instead of one hwmon device per physical package, now
there should be one hwmon device for each die on these systems.

On the hardwares that do not have multi-die support,
topology_unique_die_id() equals topology_physical_package_id(), thus the
only difference is that physical package id is used as the coretemp
platform device id, instead of logical package id on these systems.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-hwmon@vger.kernel.org
Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/hwmon/coretemp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 5d34f7271e67..a0b6b2247c3a 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -435,7 +435,7 @@ static int chk_ucode_version(unsigned int cpu)
 
 static struct platform_device *coretemp_get_pdev(unsigned int cpu)
 {
-	int pkgid = topology_logical_package_id(cpu);
+	int pkgid = topology_unique_die_id(cpu);
 
 	if (pkgid >= 0 && pkgid < max_packages)
 		return pkg_devices[pkgid];
@@ -579,7 +579,7 @@ static struct platform_driver coretemp_driver = {
 
 static struct platform_device *coretemp_device_add(unsigned int cpu)
 {
-	int err, pkgid = topology_logical_package_id(cpu);
+	int err, pkgid = topology_unique_die_id(cpu);
 	struct platform_device *pdev;
 
 	if (pkgid < 0)
@@ -703,7 +703,7 @@ static int coretemp_cpu_offline(unsigned int cpu)
 	 * the rest.
 	 */
 	if (cpumask_empty(&pd->cpumask)) {
-		pkg_devices[topology_logical_package_id(cpu)] = NULL;
+		pkg_devices[topology_unique_die_id(cpu)] = NULL;
 		platform_device_unregister(pdev);
 		return 0;
 	}
@@ -732,6 +732,7 @@ static enum cpuhp_state coretemp_hp_online;
 static int __init coretemp_init(void)
 {
 	int err;
+	struct cpuinfo_x86 *c = &cpu_data(0);
 
 	/*
 	 * CPUID.06H.EAX[0] indicates whether the CPU has thermal
@@ -741,7 +742,7 @@ static int __init coretemp_init(void)
 	if (!x86_match_cpu(coretemp_ids))
 		return -ENODEV;
 
-	max_packages = topology_max_packages();
+	max_packages = topology_max_packages() * c->x86_max_dies;
 	pkg_devices = kcalloc(max_packages, sizeof(struct platform_device *),
 			      GFP_KERNEL);
 	if (!pkg_devices)
-- 
2.18.0-rc0

Re: [PATCH 11/11] hwmon/coretemp: Support multi-die/package

[Guenter Roeck]

On 2/18/19 7:40 PM, Len Brown wrote:
> From: Zhang Rui <rui.zhang@intel.com>
> 
> This patch introduces coretemp driver support
> for new dual-die/package systems.
> 
> On the new dual-die/package systems, the package temperature MSRs becomes
> die-scope. Thus instead of one hwmon device per physical package, now
> there should be one hwmon device for each die on these systems.
> 
> On the hardwares that do not have multi-die support,
> topology_unique_die_id() equals topology_physical_package_id(), thus the
> only difference is that physical package id is used as the coretemp
> platform device id, instead of logical package id on these systems.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> Signed-off-by: Len Brown <len.brown@intel.com>
> Cc: linux-pm@vger.kernel.org
> Cc: linux-hwmon@vger.kernel.org
> Signed-off-by: Len Brown <len.brown@intel.com>

Acked-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/hwmon/coretemp.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index 5d34f7271e67..a0b6b2247c3a 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -435,7 +435,7 @@ static int chk_ucode_version(unsigned int cpu)
>   
>   static struct platform_device *coretemp_get_pdev(unsigned int cpu)
>   {
> -	int pkgid = topology_logical_package_id(cpu);
> +	int pkgid = topology_unique_die_id(cpu);
>   
>   	if (pkgid >= 0 && pkgid < max_packages)
>   		return pkg_devices[pkgid];
> @@ -579,7 +579,7 @@ static struct platform_driver coretemp_driver = {
>   
>   static struct platform_device *coretemp_device_add(unsigned int cpu)
>   {
> -	int err, pkgid = topology_logical_package_id(cpu);
> +	int err, pkgid = topology_unique_die_id(cpu);
>   	struct platform_device *pdev;
>   
>   	if (pkgid < 0)
> @@ -703,7 +703,7 @@ static int coretemp_cpu_offline(unsigned int cpu)
>   	 * the rest.
>   	 */
>   	if (cpumask_empty(&pd->cpumask)) {
> -		pkg_devices[topology_logical_package_id(cpu)] = NULL;
> +		pkg_devices[topology_unique_die_id(cpu)] = NULL;
>   		platform_device_unregister(pdev);
>   		return 0;
>   	}
> @@ -732,6 +732,7 @@ static enum cpuhp_state coretemp_hp_online;
>   static int __init coretemp_init(void)
>   {
>   	int err;
> +	struct cpuinfo_x86 *c = &cpu_data(0);
>   
>   	/*
>   	 * CPUID.06H.EAX[0] indicates whether the CPU has thermal
> @@ -741,7 +742,7 @@ static int __init coretemp_init(void)
>   	if (!x86_match_cpu(coretemp_ids))
>   		return -ENODEV;
>   
> -	max_packages = topology_max_packages();
> +	max_packages = topology_max_packages() * c->x86_max_dies;
>   	pkg_devices = kcalloc(max_packages, sizeof(struct platform_device *),
>   			      GFP_KERNEL);
>   	if (!pkg_devices)
>