[PATCH 0/6] thermal/rapl: fixes, improvements and cleanups

[PATCH 0/6] thermal/rapl: fixes, improvements and cleanups

[Zhang Rui]

The patches in this series are independent of each other. Keep them in
one series just for easy review and tracking.

Patch 1/6 fixes a bug introduced recently. In my test, reading the
domain info at a bogus address returns all 'f'. So the psys domain is
always probed both before and after the broke commit and there should be
no visible impact.

Patch 2/6 fixes a WARNING since 6.11-rc1. But the underlying problem is
there since the proc thermal driver was introduced. Lastest code still
handles it but with an extra WARNING. So I'm not sure if this should be
considered as -stable material or not.

Patch 3 - 6 are general improvements and cleanups.

thanks,
rui

Sumeet Pawnikar (1):
  powercap: intel_rapl_msr: Add PL4 support for Arrowlake-U

Zhang Rui (5):
  powercap: intel_rapl_tpmi: Fix bogus register reading
  thermal: proc_thermal: Fix WARNING during module unload
  powercap: intel_rapl_tpmi: Ignore minor version change
  thermal: proc_thermal: Remove MMIO RAPL cpu hotplug support
  thermal: proc_thermal: Add MMIO RAPL PL4 support

 drivers/powercap/intel_rapl_msr.c             |  1 +
 drivers/powercap/intel_rapl_tpmi.c            | 19 +++--
 .../processor_thermal_device_pci.c            |  2 -
 .../int340x_thermal/processor_thermal_rapl.c  | 70 +++++++------------
 4 files changed, 40 insertions(+), 52 deletions(-)

-- 
2.34.1

[PATCH 1/6] powercap: intel_rapl_tpmi: Fix bogus register reading

[Zhang Rui]

Fix bogus reading of the TPMI_RAPL_REG_DOMAIN_INFO register.

Cc: stable@vger.kernel.org
Fixes: 903eb9fb85e3 ("powercap: intel_rapl_tpmi: Fix System Domain probing")
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/powercap/intel_rapl_tpmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/powercap/intel_rapl_tpmi.c b/drivers/powercap/intel_rapl_tpmi.c
index 947544e4d229..7435df083ea4 100644
--- a/drivers/powercap/intel_rapl_tpmi.c
+++ b/drivers/powercap/intel_rapl_tpmi.c
@@ -181,7 +181,7 @@ static int parse_one_domain(struct tpmi_rapl_package *trp, u32 offset)
 			pr_warn(FW_BUG "System domain must support Domain Info register\n");
 			return -ENODEV;
 		}
-		tpmi_domain_info = readq(trp->base + offset + TPMI_RAPL_REG_DOMAIN_INFO);
+		tpmi_domain_info = readq(trp->base + offset + TPMI_RAPL_REG_DOMAIN_INFO * 8);
 		if (!(tpmi_domain_info & TPMI_RAPL_DOMAIN_ROOT))
 			return 0;
 		domain_type = RAPL_DOMAIN_PLATFORM;
-- 
2.34.1

[PATCH 2/6] thermal: proc_thermal: Fix WARNING during module unload

[Zhang Rui]

proc_thermal driver uses pcim_device_enable() to enable the pci device,
which means the device will be automatically disabled on driver detach.
Thus there is no need to call pci_disable_device() again.

With recent PCI device resource management improvements, e.g. commit
f748a07a0b64 ("PCI: Remove legacy pcim_release()"), this problem is
exposed and triggers below WARNING,

 [  224.010735] proc_thermal_pci 0000:00:04.0: disabling already-disabled device
 [  224.010747] WARNING: CPU: 8 PID: 4442 at drivers/pci/pci.c:2250 pci_disable_device+0xe5/0x100
 ...
 [  224.010844] Call Trace:
 [  224.010845]  <TASK>
 [  224.010847]  ? show_regs+0x6d/0x80
 [  224.010851]  ? __warn+0x8c/0x140
 [  224.010854]  ? pci_disable_device+0xe5/0x100
 [  224.010856]  ? report_bug+0x1c9/0x1e0
 [  224.010859]  ? handle_bug+0x46/0x80
 [  224.010862]  ? exc_invalid_op+0x1d/0x80
 [  224.010863]  ? asm_exc_invalid_op+0x1f/0x30
 [  224.010867]  ? pci_disable_device+0xe5/0x100
 [  224.010869]  ? pci_disable_device+0xe5/0x100
 [  224.010871]  ? kfree+0x21a/0x2b0
 [  224.010873]  pcim_disable_device+0x20/0x30
 [  224.010875]  devm_action_release+0x16/0x20
 [  224.010878]  release_nodes+0x47/0xc0
 [  224.010880]  devres_release_all+0x9f/0xe0
 [  224.010883]  device_unbind_cleanup+0x12/0x80
 [  224.010885]  device_release_driver_internal+0x1ca/0x210
 [  224.010887]  driver_detach+0x4e/0xa0
 [  224.010889]  bus_remove_driver+0x6f/0xf0
 [  224.010890]  driver_unregister+0x35/0x60
 [  224.010892]  pci_unregister_driver+0x44/0x90
 [  224.010894]  proc_thermal_pci_driver_exit+0x14/0x5f0 [processor_thermal_device_pci]
 ...
 [  224.010921] ---[ end trace 0000000000000000 ]---

Remove the pci_disable_device() calls.

Fixes: acd65d5d1cf4 ("thermal/drivers/int340x/processor_thermal: Add PCI MMIO based thermal driver")
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../intel/int340x_thermal/processor_thermal_device_pci.c        | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
index 006614921870..ba5d36d36fc4 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
@@ -416,7 +416,6 @@ static int proc_thermal_pci_probe(struct pci_dev *pdev, const struct pci_device_
 	if (!pci_info->no_legacy)
 		proc_thermal_remove(proc_priv);
 	proc_thermal_mmio_remove(pdev, proc_priv);
-	pci_disable_device(pdev);
 
 	return ret;
 }
@@ -438,7 +437,6 @@ static void proc_thermal_pci_remove(struct pci_dev *pdev)
 	proc_thermal_mmio_remove(pdev, pci_info->proc_priv);
 	if (!pci_info->no_legacy)
 		proc_thermal_remove(proc_priv);
-	pci_disable_device(pdev);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.34.1

[PATCH 3/6] powercap: intel_rapl_tpmi: Ignore minor version change

[Zhang Rui]

The hardware definition of every TPMI feature contains a major and minor
version. When there is a change in the MMIO offset or change in the
definition of a field, hardware will change major version. For addition
of new fields without modifying existing MMIO offsets or fields, only the
minor version is changed.

Driver is developed to support RAPL functionality for a major and minor
version. If the hardware changes major version, since offsets and
definitions are changed, driver cannot continue to provide RAPL interface
to users. Driver can still function with a minor version change as it will
just miss the new functionality added by the hardware. The current
implementation doesn't ignore any version change.

If there is mismatch with the minor version, continue with an information
log message. If there is mismatch with the major version, log error and
exit.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/powercap/intel_rapl_tpmi.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/powercap/intel_rapl_tpmi.c b/drivers/powercap/intel_rapl_tpmi.c
index 7435df083ea4..645fd1dc51a9 100644
--- a/drivers/powercap/intel_rapl_tpmi.c
+++ b/drivers/powercap/intel_rapl_tpmi.c
@@ -15,7 +15,8 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 
-#define TPMI_RAPL_VERSION 1
+#define TPMI_RAPL_MAJOR_VERSION 0
+#define TPMI_RAPL_MINOR_VERSION 1
 
 /* 1 header + 10 registers + 5 reserved. 8 bytes for each. */
 #define TPMI_RAPL_DOMAIN_SIZE 128
@@ -154,11 +155,21 @@ static int parse_one_domain(struct tpmi_rapl_package *trp, u32 offset)
 	tpmi_domain_size = tpmi_domain_header >> 16 & 0xff;
 	tpmi_domain_flags = tpmi_domain_header >> 32 & 0xffff;
 
-	if (tpmi_domain_version != TPMI_RAPL_VERSION) {
-		pr_warn(FW_BUG "Unsupported version:%d\n", tpmi_domain_version);
+	if (tpmi_domain_version == TPMI_VERSION_INVALID) {
+		pr_warn(FW_BUG "Invalid version\n");
 		return -ENODEV;
 	}
 
+	if (TPMI_MAJOR_VERSION(tpmi_domain_version) != TPMI_RAPL_MAJOR_VERSION) {
+		pr_warn(FW_BUG "Unsupported major version:%ld\n",
+			TPMI_MAJOR_VERSION(tpmi_domain_version));
+		return -ENODEV;
+	}
+
+	if (TPMI_MINOR_VERSION(tpmi_domain_version) > TPMI_RAPL_MINOR_VERSION)
+		pr_info("Ignore: Unsupported minor version:%ld\n",
+			TPMI_MINOR_VERSION(tpmi_domain_version));
+
 	/* Domain size: in unit of 128 Bytes */
 	if (tpmi_domain_size != 1) {
 		pr_warn(FW_BUG "Invalid Domain size %d\n", tpmi_domain_size);
-- 
2.34.1

[PATCH 4/6] powercap: intel_rapl_msr: Add PL4 support for Arrowlake-U

[Zhang Rui]

From: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>

Add PL4 support for ArrowLake-U platform.

Signed-off-by: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/powercap/intel_rapl_msr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/powercap/intel_rapl_msr.c b/drivers/powercap/intel_rapl_msr.c
index 733a36f67fbc..1f4c5389676a 100644
--- a/drivers/powercap/intel_rapl_msr.c
+++ b/drivers/powercap/intel_rapl_msr.c
@@ -147,6 +147,7 @@ static const struct x86_cpu_id pl4_support_ids[] = {
 	X86_MATCH_VFM(INTEL_RAPTORLAKE_P, NULL),
 	X86_MATCH_VFM(INTEL_METEORLAKE, NULL),
 	X86_MATCH_VFM(INTEL_METEORLAKE_L, NULL),
+	X86_MATCH_VFM(INTEL_ARROWLAKE_U, NULL),
 	{}
 };
 
-- 
2.34.1

[PATCH 5/6] thermal: proc_thermal: Remove MMIO RAPL cpu hotplug support

[Zhang Rui]

CPU0/package0 is always online and the MMIO RAPL driver runs on single
package systems only, so there is no need to handle CPU hotplug.

Always register a RAPL package device for package 0 and remove the
unnecessary cpu hotplug support.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../int340x_thermal/processor_thermal_rapl.c  | 66 +++++++------------
 1 file changed, 22 insertions(+), 44 deletions(-)

diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_rapl.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_rapl.c
index e9aa9e23aab9..769510e748c0 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_rapl.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_rapl.c
@@ -19,42 +19,6 @@ static const struct rapl_mmio_regs rapl_mmio_default = {
 	.limits[RAPL_DOMAIN_DRAM] = BIT(POWER_LIMIT2),
 };
 
-static int rapl_mmio_cpu_online(unsigned int cpu)
-{
-	struct rapl_package *rp;
-
-	/* mmio rapl supports package 0 only for now */
-	if (topology_physical_package_id(cpu))
-		return 0;
-
-	rp = rapl_find_package_domain_cpuslocked(cpu, &rapl_mmio_priv, true);
-	if (!rp) {
-		rp = rapl_add_package_cpuslocked(cpu, &rapl_mmio_priv, true);
-		if (IS_ERR(rp))
-			return PTR_ERR(rp);
-	}
-	cpumask_set_cpu(cpu, &rp->cpumask);
-	return 0;
-}
-
-static int rapl_mmio_cpu_down_prep(unsigned int cpu)
-{
-	struct rapl_package *rp;
-	int lead_cpu;
-
-	rp = rapl_find_package_domain_cpuslocked(cpu, &rapl_mmio_priv, true);
-	if (!rp)
-		return 0;
-
-	cpumask_clear_cpu(cpu, &rp->cpumask);
-	lead_cpu = cpumask_first(&rp->cpumask);
-	if (lead_cpu >= nr_cpu_ids)
-		rapl_remove_package_cpuslocked(rp);
-	else if (rp->lead_cpu == cpu)
-		rp->lead_cpu = lead_cpu;
-	return 0;
-}
-
 static int rapl_mmio_read_raw(int cpu, struct reg_action *ra)
 {
 	if (!ra->reg.mmio)
@@ -82,6 +46,7 @@ static int rapl_mmio_write_raw(int cpu, struct reg_action *ra)
 int proc_thermal_rapl_add(struct pci_dev *pdev, struct proc_thermal_device *proc_priv)
 {
 	const struct rapl_mmio_regs *rapl_regs = &rapl_mmio_default;
+	struct rapl_package *rp;
 	enum rapl_domain_reg_id reg;
 	enum rapl_domain_type domain;
 	int ret;
@@ -109,25 +74,38 @@ int proc_thermal_rapl_add(struct pci_dev *pdev, struct proc_thermal_device *proc
 		return PTR_ERR(rapl_mmio_priv.control_type);
 	}
 
-	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "powercap/rapl:online",
-				rapl_mmio_cpu_online, rapl_mmio_cpu_down_prep);
-	if (ret < 0) {
-		powercap_unregister_control_type(rapl_mmio_priv.control_type);
-		rapl_mmio_priv.control_type = NULL;
-		return ret;
+	/* Register a RAPL package device for package 0 which is always online */
+	rp = rapl_find_package_domain(0, &rapl_mmio_priv, false);
+	if (rp) {
+		ret = -EEXIST;
+		goto err;
+	}
+
+	rp = rapl_add_package(0, &rapl_mmio_priv, false);
+	if (IS_ERR(rp)) {
+		ret = PTR_ERR(rp);
+		goto err;
 	}
-	rapl_mmio_priv.pcap_rapl_online = ret;
 
 	return 0;
+
+err:
+	powercap_unregister_control_type(rapl_mmio_priv.control_type);
+	rapl_mmio_priv.control_type = NULL;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(proc_thermal_rapl_add);
 
 void proc_thermal_rapl_remove(void)
 {
+	struct rapl_package *rp;
+
 	if (IS_ERR_OR_NULL(rapl_mmio_priv.control_type))
 		return;
 
-	cpuhp_remove_state(rapl_mmio_priv.pcap_rapl_online);
+	rp = rapl_find_package_domain(0, &rapl_mmio_priv, false);
+	if (rp)
+		rapl_remove_package(rp);
 	powercap_unregister_control_type(rapl_mmio_priv.control_type);
 }
 EXPORT_SYMBOL_GPL(proc_thermal_rapl_remove);
-- 
2.34.1

[PATCH 6/6] thermal: proc_thermal: Add MMIO RAPL PL4 support

[Zhang Rui]

Similar to MSR RAPL interface, MMIO RAPL also has PL4 support.

Add PL4 support for MMIO RAPL. As a result, the powercap sysfs for MMIO
RAPL will show a new "peak power" constraint.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../thermal/intel/int340x_thermal/processor_thermal_rapl.c    | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_rapl.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_rapl.c
index 769510e748c0..bde2cc386afd 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_rapl.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_rapl.c
@@ -13,9 +13,9 @@ static struct rapl_if_priv rapl_mmio_priv;
 
 static const struct rapl_mmio_regs rapl_mmio_default = {
 	.reg_unit = 0x5938,
-	.regs[RAPL_DOMAIN_PACKAGE] = { 0x59a0, 0x593c, 0x58f0, 0, 0x5930},
+	.regs[RAPL_DOMAIN_PACKAGE] = { 0x59a0, 0x593c, 0x58f0, 0, 0x5930, 0x59b0},
 	.regs[RAPL_DOMAIN_DRAM] = { 0x58e0, 0x58e8, 0x58ec, 0, 0},
-	.limits[RAPL_DOMAIN_PACKAGE] = BIT(POWER_LIMIT2),
+	.limits[RAPL_DOMAIN_PACKAGE] = BIT(POWER_LIMIT2) | BIT(POWER_LIMIT4),
 	.limits[RAPL_DOMAIN_DRAM] = BIT(POWER_LIMIT2),
 };
 
-- 
2.34.1

Re: [PATCH 0/6] thermal/rapl: fixes, improvements and cleanups

[Rafael J. Wysocki]

On Mon, Sep 30, 2024 at 10:19 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> The patches in this series are independent of each other. Keep them in
> one series just for easy review and tracking.
>
> Patch 1/6 fixes a bug introduced recently. In my test, reading the
> domain info at a bogus address returns all 'f'. So the psys domain is
> always probed both before and after the broke commit and there should be
> no visible impact.
>
> Patch 2/6 fixes a WARNING since 6.11-rc1. But the underlying problem is
> there since the proc thermal driver was introduced. Lastest code still
> handles it but with an extra WARNING. So I'm not sure if this should be
> considered as -stable material or not.
>
> Patch 3 - 6 are general improvements and cleanups.
>
> thanks,
> rui
>
> Sumeet Pawnikar (1):
>   powercap: intel_rapl_msr: Add PL4 support for Arrowlake-U
>
> Zhang Rui (5):
>   powercap: intel_rapl_tpmi: Fix bogus register reading
>   thermal: proc_thermal: Fix WARNING during module unload
>   powercap: intel_rapl_tpmi: Ignore minor version change
>   thermal: proc_thermal: Remove MMIO RAPL cpu hotplug support
>   thermal: proc_thermal: Add MMIO RAPL PL4 support
>
>  drivers/powercap/intel_rapl_msr.c             |  1 +
>  drivers/powercap/intel_rapl_tpmi.c            | 19 +++--
>  .../processor_thermal_device_pci.c            |  2 -
>  .../int340x_thermal/processor_thermal_rapl.c  | 70 +++++++------------
>  4 files changed, 40 insertions(+), 52 deletions(-)
>
> --

All applied as 6.12-rc material, thanks!