linux-acpi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] thermal: core/ACPI: Fix processor cooling device regression
@ 2023-03-13 14:24 Rafael J. Wysocki
  2023-03-13 14:27 ` [PATCH v2 1/4] ACPI: processor: Reorder acpi_processor_driver_init() Rafael J. Wysocki
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Rafael J. Wysocki @ 2023-03-13 14:24 UTC (permalink / raw)
  To: Linux PM
  Cc: Zhang Rui, Linux ACPI, LKML, Daniel Lezcano, Srinivas Pandruvada,
	Viresh Kumar, Quanxian Wang

Hi All,

The first revision of this patch series was posted as

https://lore.kernel.org/linux-pm/2148907.irdbgypaU6@kreacher/

As reported by Rui in this thread:

Link: https://lore.kernel.org/linux-pm/53ec1f06f61c984100868926f282647e57ecfb2d.camel@intel.com/

some recent changes in the thermal core cause the CPU cooling devices
registered by the ACPI processor driver to become unusable in some cases
and somewhat crippled in general.

The problem is that the ACPI processor driver changes its ->get_max_state()
callback return value depending on whether or not cpufreq is available and
there is a cpufreq policy for a given CPU.  However, the thermal core has
always assumed that the return value of that callback will not change, which
in fact is relied on by the cooling device statistics code.  In particular,
when the ->get_max_state() grows, the memory buffer allocated for storing the
statistics will be too small and corruption may ensue as a result.

For this reason, the issue needs to be addressed in the ACPI processor driver
and not in the thermal core, but the core needs to help somewhat too.  Namely,
it needs to provide a helper allowing an interested driver to update the
max_state value for an already registered cooling device in certain situations
which will also cause the statistics to be rebuilt.

This series implements the above and for details please refer to the individual
patch chagelogs.

Thanks!




^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/4] ACPI: processor: Reorder acpi_processor_driver_init()
  2023-03-13 14:24 [PATCH v2 0/4] thermal: core/ACPI: Fix processor cooling device regression Rafael J. Wysocki
@ 2023-03-13 14:27 ` Rafael J. Wysocki
  2023-03-14  2:03   ` Zhang, Rui
  2023-03-13 14:28 ` [PATCH v2 2/4] thermal: core: Introduce thermal_cooling_device_present() Rafael J. Wysocki
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Rafael J. Wysocki @ 2023-03-13 14:27 UTC (permalink / raw)
  To: Linux PM
  Cc: Zhang Rui, Linux ACPI, LKML, Daniel Lezcano, Srinivas Pandruvada,
	Viresh Kumar, Quanxian Wang

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The cpufreq policy notifier in the ACPI processor driver may as
well be registered before the driver itself, which causes
acpi_processor_cpufreq_init to be true (unless the notifier
registration fails, which is unlikely at that point) when the
ACPI CPU thermal cooling devices are registered, so the
processor_get_max_state() result does not change while
acpi_processor_driver_init() is running.

Change the ordering in acpi_processor_driver_init() accordingly
to prevent the max_state value from remaining 0 permanently for all
ACPI CPU cooling devices due to setting acpi_processor_cpufreq_init
too late.  [Note that processor_get_max_state() may still return
different values at different times after this change, depending on
the cpufreq driver registration time, but that issue needs to be
addressed separately.]

Fixes: a365105c685c("thermal: sysfs: Reuse cdev->max_state")
Reported-by: Wang, Quanxian <quanxian.wang@intel.com>
Link: https://lore.kernel.org/linux-pm/53ec1f06f61c984100868926f282647e57ecfb2d.camel@intel.com/
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2: Expand changelog to explain that this particular patch addresses
          part of the issue.

---
 drivers/acpi/processor_driver.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/acpi/processor_driver.c
===================================================================
--- linux-pm.orig/drivers/acpi/processor_driver.c
+++ linux-pm/drivers/acpi/processor_driver.c
@@ -263,6 +263,12 @@ static int __init acpi_processor_driver_
 	if (acpi_disabled)
 		return 0;
 
+	if (!cpufreq_register_notifier(&acpi_processor_notifier_block,
+				       CPUFREQ_POLICY_NOTIFIER)) {
+		acpi_processor_cpufreq_init = true;
+		acpi_processor_ignore_ppc_init();
+	}
+
 	result = driver_register(&acpi_processor_driver);
 	if (result < 0)
 		return result;
@@ -276,12 +282,6 @@ static int __init acpi_processor_driver_
 	cpuhp_setup_state_nocalls(CPUHP_ACPI_CPUDRV_DEAD, "acpi/cpu-drv:dead",
 				  NULL, acpi_soft_cpu_dead);
 
-	if (!cpufreq_register_notifier(&acpi_processor_notifier_block,
-				       CPUFREQ_POLICY_NOTIFIER)) {
-		acpi_processor_cpufreq_init = true;
-		acpi_processor_ignore_ppc_init();
-	}
-
 	acpi_processor_throttling_init();
 	return 0;
 err:




^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 2/4] thermal: core: Introduce thermal_cooling_device_present()
  2023-03-13 14:24 [PATCH v2 0/4] thermal: core/ACPI: Fix processor cooling device regression Rafael J. Wysocki
  2023-03-13 14:27 ` [PATCH v2 1/4] ACPI: processor: Reorder acpi_processor_driver_init() Rafael J. Wysocki
@ 2023-03-13 14:28 ` Rafael J. Wysocki
  2023-03-14  2:03   ` Zhang, Rui
  2023-03-13 14:32 ` [PATCH v2 3/4] thermal: core: Introduce thermal_cooling_device_update() Rafael J. Wysocki
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Rafael J. Wysocki @ 2023-03-13 14:28 UTC (permalink / raw)
  To: Linux PM
  Cc: Zhang Rui, Linux ACPI, LKML, Daniel Lezcano, Srinivas Pandruvada,
	Viresh Kumar, Quanxian Wang

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Introduce a helper function, thermal_cooling_device_present(), for
checking if the given cooling device is in the list of registered
cooling devices to avoid some code duplication in a subsequent
patch.

No expected functional impact.

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

v1 -> v2: No changes

---
 drivers/thermal/thermal_core.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -1045,6 +1045,18 @@ devm_thermal_of_cooling_device_register(
 }
 EXPORT_SYMBOL_GPL(devm_thermal_of_cooling_device_register);
 
+static bool thermal_cooling_device_present(struct thermal_cooling_device *cdev)
+{
+	struct thermal_cooling_device *pos = NULL;
+
+	list_for_each_entry(pos, &thermal_cdev_list, node) {
+		if (pos == cdev)
+			return true;
+	}
+
+	return false;
+}
+
 static void __unbind(struct thermal_zone_device *tz, int mask,
 		     struct thermal_cooling_device *cdev)
 {
@@ -1067,20 +1079,17 @@ void thermal_cooling_device_unregister(s
 	int i;
 	const struct thermal_zone_params *tzp;
 	struct thermal_zone_device *tz;
-	struct thermal_cooling_device *pos = NULL;
 
 	if (!cdev)
 		return;
 
 	mutex_lock(&thermal_list_lock);
-	list_for_each_entry(pos, &thermal_cdev_list, node)
-		if (pos == cdev)
-			break;
-	if (pos != cdev) {
-		/* thermal cooling device not found */
+
+	if (!thermal_cooling_device_present(cdev)) {
 		mutex_unlock(&thermal_list_lock);
 		return;
 	}
+
 	list_del(&cdev->node);
 
 	/* Unbind all thermal zones associated with 'this' cdev */




^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 3/4] thermal: core: Introduce thermal_cooling_device_update()
  2023-03-13 14:24 [PATCH v2 0/4] thermal: core/ACPI: Fix processor cooling device regression Rafael J. Wysocki
  2023-03-13 14:27 ` [PATCH v2 1/4] ACPI: processor: Reorder acpi_processor_driver_init() Rafael J. Wysocki
  2023-03-13 14:28 ` [PATCH v2 2/4] thermal: core: Introduce thermal_cooling_device_present() Rafael J. Wysocki
@ 2023-03-13 14:32 ` Rafael J. Wysocki
  2023-03-14  2:03   ` Zhang, Rui
  2023-03-13 14:34 ` [PATCH v2 4/4] ACPI: processor: thermal: Update CPU cooling devices on cpufreq policy changes Rafael J. Wysocki
  2023-03-13 16:47 ` [PATCH v2 0/4] thermal: core/ACPI: Fix processor cooling device regression Zhang, Rui
  4 siblings, 1 reply; 12+ messages in thread
From: Rafael J. Wysocki @ 2023-03-13 14:32 UTC (permalink / raw)
  To: Linux PM
  Cc: Zhang Rui, Linux ACPI, LKML, Daniel Lezcano, Srinivas Pandruvada,
	Viresh Kumar, Quanxian Wang

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Introduce a core thermal API function, thermal_cooling_device_update(),
for updating the max_state value for a cooling device and rearranging
its statistics in sysfs after a possible change of its ->get_max_state()
callback return value.

That callback is now invoked only once, during cooling device
registration, to populate the max_state field in the cooling device
object, so if its return value changes, it needs to be invoked again
and the new return value needs to be stored as max_state.  Moreover,
the statistics presented in sysfs need to be rearranged in general,
because there may not be enough room in them to store data for all
of the possible states (in the case when max_state grows).

The new function takes care of that (and some other minor things
related to it), but some extra locking and lockdep annotations are
added in several places too to protect against crashes in the cases
when the statistics are not present or when a stale max_state value
might be used by sysfs attributes.

Note that the actual user of the new function will be added separately.

Link: https://lore.kernel.org/linux-pm/53ec1f06f61c984100868926f282647e57ecfb2d.camel@intel.com/
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2:
    * Make thermal_cooling_device_update() do the full IS_ERR_OR_NULL() check
      on cdev, so that its callers don't need to do it.
    * Remove ->set_cur_state() callback invocation from
      thermal_cooling_device_update(), because it is redundant (and it doesn't
      make sense to pass a state value exceeding max_state to it anyway).

---
 drivers/thermal/thermal_core.c  |   40 ++++++++++++++++++++++
 drivers/thermal/thermal_core.h  |    1 
 drivers/thermal/thermal_sysfs.c |   72 +++++++++++++++++++++++++++++++++++-----
 include/linux/thermal.h         |    1 
 4 files changed, 106 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -1057,6 +1057,46 @@ static bool thermal_cooling_device_prese
 	return false;
 }
 
+void thermal_cooling_device_update(struct thermal_cooling_device *cdev)
+{
+	unsigned long state;
+
+	if (IS_ERR_OR_NULL(cdev))
+		return;
+
+	/*
+	 * Hold thermal_list_lock throughout the update to prevent the device
+	 * from going away while being updated.
+	 */
+	mutex_lock(&thermal_list_lock);
+
+	if (!thermal_cooling_device_present(cdev))
+		goto unlock_list;
+
+	/*
+	 * Update under the cdev lock to prevent the state from being set beyond
+	 * the new limit concurrently.
+	 */
+	mutex_lock(&cdev->lock);
+
+	if (cdev->ops->get_max_state(cdev, &cdev->max_state))
+		goto unlock;
+
+	thermal_cooling_device_stats_reinit(cdev);
+
+	if (cdev->ops->get_cur_state(cdev, &state) || state > cdev->max_state)
+		goto unlock;
+
+	thermal_cooling_device_stats_update(cdev, state);
+
+unlock:
+	mutex_unlock(&cdev->lock);
+
+unlock_list:
+	mutex_unlock(&thermal_list_lock);
+}
+EXPORT_SYMBOL_GPL(thermal_cooling_device_update);
+
 static void __unbind(struct thermal_zone_device *tz, int mask,
 		     struct thermal_cooling_device *cdev)
 {
Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_sysfs.c
+++ linux-pm/drivers/thermal/thermal_sysfs.c
@@ -685,6 +685,8 @@ void thermal_cooling_device_stats_update
 {
 	struct cooling_dev_stats *stats = cdev->stats;
 
+	lockdep_assert_held(&cdev->lock);
+
 	if (!stats)
 		return;
 
@@ -706,13 +708,22 @@ static ssize_t total_trans_show(struct d
 				struct device_attribute *attr, char *buf)
 {
 	struct thermal_cooling_device *cdev = to_cooling_device(dev);
-	struct cooling_dev_stats *stats = cdev->stats;
+	struct cooling_dev_stats *stats;
 	int ret;
 
+	mutex_lock(&cdev->lock);
+
+	stats = cdev->stats;
+	if (!stats)
+		goto unlock;
+
 	spin_lock(&stats->lock);
 	ret = sprintf(buf, "%u\n", stats->total_trans);
 	spin_unlock(&stats->lock);
 
+unlock:
+	mutex_unlock(&cdev->lock);
+
 	return ret;
 }
 
@@ -721,11 +732,18 @@ time_in_state_ms_show(struct device *dev
 		      char *buf)
 {
 	struct thermal_cooling_device *cdev = to_cooling_device(dev);
-	struct cooling_dev_stats *stats = cdev->stats;
+	struct cooling_dev_stats *stats;
 	ssize_t len = 0;
 	int i;
 
+	mutex_lock(&cdev->lock);
+
+	stats = cdev->stats;
+	if (!stats)
+		goto unlock;
+
 	spin_lock(&stats->lock);
+
 	update_time_in_state(stats);
 
 	for (i = 0; i <= cdev->max_state; i++) {
@@ -734,6 +752,9 @@ time_in_state_ms_show(struct device *dev
 	}
 	spin_unlock(&stats->lock);
 
+unlock:
+	mutex_unlock(&cdev->lock);
+
 	return len;
 }
 
@@ -742,8 +763,16 @@ reset_store(struct device *dev, struct d
 	    size_t count)
 {
 	struct thermal_cooling_device *cdev = to_cooling_device(dev);
-	struct cooling_dev_stats *stats = cdev->stats;
-	int i, states = cdev->max_state + 1;
+	struct cooling_dev_stats *stats;
+	int i, states;
+
+	mutex_lock(&cdev->lock);
+
+	stats = cdev->stats;
+	if (!stats)
+		goto unlock;
+
+	states = cdev->max_state + 1;
 
 	spin_lock(&stats->lock);
 
@@ -757,6 +786,9 @@ reset_store(struct device *dev, struct d
 
 	spin_unlock(&stats->lock);
 
+unlock:
+	mutex_unlock(&cdev->lock);
+
 	return count;
 }
 
@@ -764,10 +796,18 @@ static ssize_t trans_table_show(struct d
 				struct device_attribute *attr, char *buf)
 {
 	struct thermal_cooling_device *cdev = to_cooling_device(dev);
-	struct cooling_dev_stats *stats = cdev->stats;
+	struct cooling_dev_stats *stats;
 	ssize_t len = 0;
 	int i, j;
 
+	mutex_lock(&cdev->lock);
+
+	stats = cdev->stats;
+	if (!stats) {
+		len = -ENODATA;
+		goto unlock;
+	}
+
 	len += snprintf(buf + len, PAGE_SIZE - len, " From  :    To\n");
 	len += snprintf(buf + len, PAGE_SIZE - len, "       : ");
 	for (i = 0; i <= cdev->max_state; i++) {
@@ -775,8 +815,10 @@ static ssize_t trans_table_show(struct d
 			break;
 		len += snprintf(buf + len, PAGE_SIZE - len, "state%2u  ", i);
 	}
-	if (len >= PAGE_SIZE)
-		return PAGE_SIZE;
+	if (len >= PAGE_SIZE) {
+		len = PAGE_SIZE;
+		goto unlock;
+	}
 
 	len += snprintf(buf + len, PAGE_SIZE - len, "\n");
 
@@ -799,8 +841,12 @@ static ssize_t trans_table_show(struct d
 
 	if (len >= PAGE_SIZE) {
 		pr_warn_once("Thermal transition table exceeds PAGE_SIZE. Disabling\n");
-		return -EFBIG;
+		len = -EFBIG;
 	}
+
+unlock:
+	mutex_unlock(&cdev->lock);
+
 	return len;
 }
 
@@ -830,6 +876,8 @@ static void cooling_device_stats_setup(s
 	unsigned long states = cdev->max_state + 1;
 	int var;
 
+	lockdep_assert_held(&cdev->lock);
+
 	var = sizeof(*stats);
 	var += sizeof(*stats->time_in_state) * states;
 	var += sizeof(*stats->trans_table) * states * states;
@@ -855,6 +903,8 @@ out:
 
 static void cooling_device_stats_destroy(struct thermal_cooling_device *cdev)
 {
+	lockdep_assert_held(&cdev->lock);
+
 	kfree(cdev->stats);
 	cdev->stats = NULL;
 }
@@ -879,6 +929,12 @@ void thermal_cooling_device_destroy_sysf
 	cooling_device_stats_destroy(cdev);
 }
 
+void thermal_cooling_device_stats_reinit(struct thermal_cooling_device *cdev)
+{
+	cooling_device_stats_destroy(cdev);
+	cooling_device_stats_setup(cdev);
+}
+
 /* these helper will be used only at the time of bindig */
 ssize_t
 trip_point_show(struct device *dev, struct device_attribute *attr, char *buf)
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -127,6 +127,7 @@ int thermal_zone_create_device_groups(st
 void thermal_zone_destroy_device_groups(struct thermal_zone_device *);
 void thermal_cooling_device_setup_sysfs(struct thermal_cooling_device *);
 void thermal_cooling_device_destroy_sysfs(struct thermal_cooling_device *cdev);
+void thermal_cooling_device_stats_reinit(struct thermal_cooling_device *cdev);
 /* used only at binding time */
 ssize_t trip_point_show(struct device *, struct device_attribute *, char *);
 ssize_t weight_show(struct device *, struct device_attribute *, char *);
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -388,6 +388,7 @@ devm_thermal_of_cooling_device_register(
 				struct device_node *np,
 				char *type, void *devdata,
 				const struct thermal_cooling_device_ops *ops);
+void thermal_cooling_device_update(struct thermal_cooling_device *);
 void thermal_cooling_device_unregister(struct thermal_cooling_device *);
 struct thermal_zone_device *thermal_zone_get_zone_by_name(const char *name);
 int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);




^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 4/4] ACPI: processor: thermal: Update CPU cooling devices on cpufreq policy changes
  2023-03-13 14:24 [PATCH v2 0/4] thermal: core/ACPI: Fix processor cooling device regression Rafael J. Wysocki
                   ` (2 preceding siblings ...)
  2023-03-13 14:32 ` [PATCH v2 3/4] thermal: core: Introduce thermal_cooling_device_update() Rafael J. Wysocki
@ 2023-03-13 14:34 ` Rafael J. Wysocki
  2023-03-14  2:03   ` Zhang, Rui
  2023-03-13 16:47 ` [PATCH v2 0/4] thermal: core/ACPI: Fix processor cooling device regression Zhang, Rui
  4 siblings, 1 reply; 12+ messages in thread
From: Rafael J. Wysocki @ 2023-03-13 14:34 UTC (permalink / raw)
  To: Linux PM
  Cc: Zhang Rui, Linux ACPI, LKML, Daniel Lezcano, Srinivas Pandruvada,
	Viresh Kumar, Quanxian Wang

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

When a cpufreq policy appears or goes away, the CPU cooling devices for
the CPUs covered by that policy need to be updated so that the new
processor_get_max_state() value is stored as max_state and the
statistics in sysfs are rearranged for each of them.

Do that accordingly in acpi_thermal_cpufreq_init() and
acpi_thermal_cpufreq_exit().

Fixes: a365105c685c("thermal: sysfs: Reuse cdev->max_state")
Reported-by: Wang, Quanxian <quanxian.wang@intel.com>
Link: https://lore.kernel.org/linux-pm/53ec1f06f61c984100868926f282647e57ecfb2d.camel@intel.com/
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2: Remove the now redundant IS_ERR() checks on cdev before calling
          thermal_cooling_device_update().

---
 drivers/acpi/processor_thermal.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/acpi/processor_thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/processor_thermal.c
+++ linux-pm/drivers/acpi/processor_thermal.c
@@ -140,9 +140,13 @@ void acpi_thermal_cpufreq_init(struct cp
 		ret = freq_qos_add_request(&policy->constraints,
 					   &pr->thermal_req,
 					   FREQ_QOS_MAX, INT_MAX);
-		if (ret < 0)
+		if (ret < 0) {
 			pr_err("Failed to add freq constraint for CPU%d (%d)\n",
 			       cpu, ret);
+			continue;
+		}
+
+		thermal_cooling_device_update(pr->cdev);
 	}
 }
 
@@ -153,8 +157,12 @@ void acpi_thermal_cpufreq_exit(struct cp
 	for_each_cpu(cpu, policy->related_cpus) {
 		struct acpi_processor *pr = per_cpu(processors, cpu);
 
-		if (pr)
-			freq_qos_remove_request(&pr->thermal_req);
+		if (!pr)
+			continue;
+
+		freq_qos_remove_request(&pr->thermal_req);
+
+		thermal_cooling_device_update(pr->cdev);
 	}
 }
 #else				/* ! CONFIG_CPU_FREQ */




^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/4] thermal: core/ACPI: Fix processor cooling device regression
  2023-03-13 14:24 [PATCH v2 0/4] thermal: core/ACPI: Fix processor cooling device regression Rafael J. Wysocki
                   ` (3 preceding siblings ...)
  2023-03-13 14:34 ` [PATCH v2 4/4] ACPI: processor: thermal: Update CPU cooling devices on cpufreq policy changes Rafael J. Wysocki
@ 2023-03-13 16:47 ` Zhang, Rui
  2023-03-13 18:02   ` Rafael J. Wysocki
  4 siblings, 1 reply; 12+ messages in thread
From: Zhang, Rui @ 2023-03-13 16:47 UTC (permalink / raw)
  To: linux-pm, rjw
  Cc: srinivas.pandruvada, viresh.kumar, Wang, Quanxian, linux-kernel,
	daniel.lezcano, linux-acpi

Hi, Rafael,

The only concern to me is that, in thermal_cooling_device_update(), we
should handle the cases that the cooling device is current used by
one/more thermal zone. say, something like

list_for_each_entry(pos, &cdev->thermal_instances, cdev_node) {
	/* e.g. what to do if tz1 set it to state 1 previously */
}
I have not got a clear idea what we should do here.

But given that I have confirmed that this patch series fixes the
original problem, and the ACPI passive cooling is unlikely to be
triggered before CPUFREQ_CREATE_POLICY notification, probably we can
address that problem later.

Tested-by: Zhang Rui <rui.zhang@intel.com>
Reviewed-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui

On Mon, 2023-03-13 at 15:24 +0100, Rafael J. Wysocki wrote:
> Hi All,
> 
> The first revision of this patch series was posted as
> 
> https://lore.kernel.org/linux-pm/2148907.irdbgypaU6@kreacher/
> 
> As reported by Rui in this thread:
> 
> Link: 
> https://lore.kernel.org/linux-pm/53ec1f06f61c984100868926f282647e57ecfb2d.camel@intel.com/
> 
> some recent changes in the thermal core cause the CPU cooling devices
> registered by the ACPI processor driver to become unusable in some
> cases
> and somewhat crippled in general.
> 
> The problem is that the ACPI processor driver changes its
> ->get_max_state()
> callback return value depending on whether or not cpufreq is
> available and
> there is a cpufreq policy for a given CPU.  However, the thermal core
> has
> always assumed that the return value of that callback will not
> change, which
> in fact is relied on by the cooling device statistics code.  In
> particular,
> when the ->get_max_state() grows, the memory buffer allocated for
> storing the
> statistics will be too small and corruption may ensue as a result.
> 
> For this reason, the issue needs to be addressed in the ACPI
> processor driver
> and not in the thermal core, but the core needs to help somewhat
> too.  Namely,
> it needs to provide a helper allowing an interested driver to update
> the
> max_state value for an already registered cooling device in certain
> situations
> which will also cause the statistics to be rebuilt.
> 
> This series implements the above and for details please refer to the
> individual
> patch chagelogs.
> 
> Thanks!
> 
> 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/4] thermal: core/ACPI: Fix processor cooling device regression
  2023-03-13 16:47 ` [PATCH v2 0/4] thermal: core/ACPI: Fix processor cooling device regression Zhang, Rui
@ 2023-03-13 18:02   ` Rafael J. Wysocki
  2023-03-14  2:02     ` Zhang, Rui
  0 siblings, 1 reply; 12+ messages in thread
From: Rafael J. Wysocki @ 2023-03-13 18:02 UTC (permalink / raw)
  To: Zhang, Rui
  Cc: linux-pm, rjw, srinivas.pandruvada, viresh.kumar, Wang, Quanxian,
	linux-kernel, daniel.lezcano, linux-acpi

On Mon, Mar 13, 2023 at 5:47 PM Zhang, Rui <rui.zhang@intel.com> wrote:
>
> Hi, Rafael,
>
> The only concern to me is that, in thermal_cooling_device_update(), we
> should handle the cases that the cooling device is current used by
> one/more thermal zone. say, something like
>
> list_for_each_entry(pos, &cdev->thermal_instances, cdev_node) {
>         /* e.g. what to do if tz1 set it to state 1 previously */
> }
> I have not got a clear idea what we should do here.

For each instance, set upper to max_state if above it and set target
to upper if above it I'd say.

I guess otherwise there may be some confusion in principle and I have
missed that piece, so thanks for pointing it out!

> But given that I have confirmed that this patch series fixes the
> original problem, and the ACPI passive cooling is unlikely to be
> triggered before CPUFREQ_CREATE_POLICY notification, probably we can
> address that problem later.
>
> Tested-by: Zhang Rui <rui.zhang@intel.com>
> Reviewed-by: Zhang Rui <rui.zhang@intel.com>

Thank you!

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/4] thermal: core/ACPI: Fix processor cooling device regression
  2023-03-13 18:02   ` Rafael J. Wysocki
@ 2023-03-14  2:02     ` Zhang, Rui
  0 siblings, 0 replies; 12+ messages in thread
From: Zhang, Rui @ 2023-03-14  2:02 UTC (permalink / raw)
  To: rafael
  Cc: viresh.kumar, daniel.lezcano, Wang, Quanxian, rjw,
	srinivas.pandruvada, linux-kernel, linux-acpi, linux-pm

On Mon, 2023-03-13 at 19:02 +0100, Rafael J. Wysocki wrote:
> On Mon, Mar 13, 2023 at 5:47 PM Zhang, Rui <rui.zhang@intel.com>
> wrote:
> > Hi, Rafael,
> > 
> > The only concern to me is that, in thermal_cooling_device_update(),
> > we
> > should handle the cases that the cooling device is current used by
> > one/more thermal zone. say, something like
> > 
> > list_for_each_entry(pos, &cdev->thermal_instances, cdev_node) {
> >         /* e.g. what to do if tz1 set it to state 1 previously */
> > }
> > I have not got a clear idea what we should do here.
> 
> For each instance, set upper to max_state if above it and set target
> to upper if above it I'd say.
> 

Say, before update,
max_state: 3
target: 1
upper is set to 3 because upper == THERMAL_NO_LIMIT during binding

then, after update
max_state: 7
target: ?
upper: ?

Maybe we should do unbind and rebind, and then set target
to THERMAL_NO_TARGET? it is really the governor that should set the
target.

> I guess otherwise there may be some confusion in principle and I have
> missed that piece, so thanks for pointing it out!
> 
> > But given that I have confirmed that this patch series fixes the
> > original problem, and the ACPI passive cooling is unlikely to be
> > triggered before CPUFREQ_CREATE_POLICY notification, probably we
> > can
> > address that problem later.
> > 
> > Tested-by: Zhang Rui <rui.zhang@intel.com>
> > Reviewed-by: Zhang Rui <rui.zhang@intel.com>
> 
> 
I recalled that patchwork used to catch these tags here and apply them
to every patches in the series, so the tags are appended automatically
when applying the patches. But it apparently does not work now.

Let me reply to the patches one by one.

thanks,
rui

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/4] ACPI: processor: Reorder acpi_processor_driver_init()
  2023-03-13 14:27 ` [PATCH v2 1/4] ACPI: processor: Reorder acpi_processor_driver_init() Rafael J. Wysocki
@ 2023-03-14  2:03   ` Zhang, Rui
  0 siblings, 0 replies; 12+ messages in thread
From: Zhang, Rui @ 2023-03-14  2:03 UTC (permalink / raw)
  To: linux-pm, rjw
  Cc: srinivas.pandruvada, viresh.kumar, Wang, Quanxian, linux-kernel,
	daniel.lezcano, linux-acpi

On Mon, 2023-03-13 at 15:27 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The cpufreq policy notifier in the ACPI processor driver may as
> well be registered before the driver itself, which causes
> acpi_processor_cpufreq_init to be true (unless the notifier
> registration fails, which is unlikely at that point) when the
> ACPI CPU thermal cooling devices are registered, so the
> processor_get_max_state() result does not change while
> acpi_processor_driver_init() is running.
> 
> Change the ordering in acpi_processor_driver_init() accordingly
> to prevent the max_state value from remaining 0 permanently for all
> ACPI CPU cooling devices due to setting acpi_processor_cpufreq_init
> too late.  [Note that processor_get_max_state() may still return
> different values at different times after this change, depending on
> the cpufreq driver registration time, but that issue needs to be
> addressed separately.]
> 
> Fixes: a365105c685c("thermal: sysfs: Reuse cdev->max_state")
> Reported-by: Wang, Quanxian <quanxian.wang@intel.com>
> Link: 
> https://lore.kernel.org/linux-pm/53ec1f06f61c984100868926f282647e57ecfb2d.camel@intel.com/
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Tested-by: Zhang Rui <rui.zhang@intel.com>
Reviewed-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui
> ---
> 
> v1 -> v2: Expand changelog to explain that this particular patch
> addresses
>           part of the issue.
> 
> ---
>  drivers/acpi/processor_driver.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> Index: linux-pm/drivers/acpi/processor_driver.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/processor_driver.c
> +++ linux-pm/drivers/acpi/processor_driver.c
> @@ -263,6 +263,12 @@ static int __init acpi_processor_driver_
>  	if (acpi_disabled)
>  		return 0;
>  
> +	if (!cpufreq_register_notifier(&acpi_processor_notifier_block,
> +				       CPUFREQ_POLICY_NOTIFIER)) {
> +		acpi_processor_cpufreq_init = true;
> +		acpi_processor_ignore_ppc_init();
> +	}
> +
>  	result = driver_register(&acpi_processor_driver);
>  	if (result < 0)
>  		return result;
> @@ -276,12 +282,6 @@ static int __init acpi_processor_driver_
>  	cpuhp_setup_state_nocalls(CPUHP_ACPI_CPUDRV_DEAD, "acpi/cpu-
> drv:dead",
>  				  NULL, acpi_soft_cpu_dead);
>  
> -	if (!cpufreq_register_notifier(&acpi_processor_notifier_block,
> -				       CPUFREQ_POLICY_NOTIFIER)) {
> -		acpi_processor_cpufreq_init = true;
> -		acpi_processor_ignore_ppc_init();
> -	}
> -
>  	acpi_processor_throttling_init();
>  	return 0;
>  err:
> 
> 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/4] thermal: core: Introduce thermal_cooling_device_present()
  2023-03-13 14:28 ` [PATCH v2 2/4] thermal: core: Introduce thermal_cooling_device_present() Rafael J. Wysocki
@ 2023-03-14  2:03   ` Zhang, Rui
  0 siblings, 0 replies; 12+ messages in thread
From: Zhang, Rui @ 2023-03-14  2:03 UTC (permalink / raw)
  To: linux-pm, rjw
  Cc: srinivas.pandruvada, viresh.kumar, Wang, Quanxian, linux-kernel,
	daniel.lezcano, linux-acpi

On Mon, 2023-03-13 at 15:28 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Introduce a helper function, thermal_cooling_device_present(), for
> checking if the given cooling device is in the list of registered
> cooling devices to avoid some code duplication in a subsequent
> patch.
> 
> No expected functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Tested-by: Zhang Rui <rui.zhang@intel.com>
Reviewed-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui

> ---
> 
> v1 -> v2: No changes
> 
> ---
>  drivers/thermal/thermal_core.c |   21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -1045,6 +1045,18 @@ devm_thermal_of_cooling_device_register(
>  }
>  EXPORT_SYMBOL_GPL(devm_thermal_of_cooling_device_register);
>  
> +static bool thermal_cooling_device_present(struct
> thermal_cooling_device *cdev)
> +{
> +	struct thermal_cooling_device *pos = NULL;
> +
> +	list_for_each_entry(pos, &thermal_cdev_list, node) {
> +		if (pos == cdev)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  static void __unbind(struct thermal_zone_device *tz, int mask,
>  		     struct thermal_cooling_device *cdev)
>  {
> @@ -1067,20 +1079,17 @@ void thermal_cooling_device_unregister(s
>  	int i;
>  	const struct thermal_zone_params *tzp;
>  	struct thermal_zone_device *tz;
> -	struct thermal_cooling_device *pos = NULL;
>  
>  	if (!cdev)
>  		return;
>  
>  	mutex_lock(&thermal_list_lock);
> -	list_for_each_entry(pos, &thermal_cdev_list, node)
> -		if (pos == cdev)
> -			break;
> -	if (pos != cdev) {
> -		/* thermal cooling device not found */
> +
> +	if (!thermal_cooling_device_present(cdev)) {
>  		mutex_unlock(&thermal_list_lock);
>  		return;
>  	}
> +
>  	list_del(&cdev->node);
>  
>  	/* Unbind all thermal zones associated with 'this' cdev */
> 
> 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 3/4] thermal: core: Introduce thermal_cooling_device_update()
  2023-03-13 14:32 ` [PATCH v2 3/4] thermal: core: Introduce thermal_cooling_device_update() Rafael J. Wysocki
@ 2023-03-14  2:03   ` Zhang, Rui
  0 siblings, 0 replies; 12+ messages in thread
From: Zhang, Rui @ 2023-03-14  2:03 UTC (permalink / raw)
  To: linux-pm, rjw
  Cc: srinivas.pandruvada, viresh.kumar, Wang, Quanxian, linux-kernel,
	daniel.lezcano, linux-acpi

On Mon, 2023-03-13 at 15:32 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Introduce a core thermal API function,
> thermal_cooling_device_update(),
> for updating the max_state value for a cooling device and rearranging
> its statistics in sysfs after a possible change of its
> ->get_max_state()
> callback return value.
> 
> That callback is now invoked only once, during cooling device
> registration, to populate the max_state field in the cooling device
> object, so if its return value changes, it needs to be invoked again
> and the new return value needs to be stored as max_state.  Moreover,
> the statistics presented in sysfs need to be rearranged in general,
> because there may not be enough room in them to store data for all
> of the possible states (in the case when max_state grows).
> 
> The new function takes care of that (and some other minor things
> related to it), but some extra locking and lockdep annotations are
> added in several places too to protect against crashes in the cases
> when the statistics are not present or when a stale max_state value
> might be used by sysfs attributes.
> 
> Note that the actual user of the new function will be added
> separately.
> 
> Link: 
> https://lore.kernel.org/linux-pm/53ec1f06f61c984100868926f282647e57ecfb2d.camel@intel.com/
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Tested-by: Zhang Rui <rui.zhang@intel.com>
Reviewed-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui

> ---
> 
> v1 -> v2:
>     * Make thermal_cooling_device_update() do the full
> IS_ERR_OR_NULL() check
>       on cdev, so that its callers don't need to do it.
>     * Remove ->set_cur_state() callback invocation from
>       thermal_cooling_device_update(), because it is redundant (and
> it doesn't
>       make sense to pass a state value exceeding max_state to it
> anyway).
> 
> ---
>  drivers/thermal/thermal_core.c  |   40 ++++++++++++++++++++++
>  drivers/thermal/thermal_core.h  |    1 
>  drivers/thermal/thermal_sysfs.c |   72
> +++++++++++++++++++++++++++++++++++-----
>  include/linux/thermal.h         |    1 
>  4 files changed, 106 insertions(+), 8 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -1057,6 +1057,46 @@ static bool thermal_cooling_device_prese
>  	return false;
>  }
>  
> +void thermal_cooling_device_update(struct thermal_cooling_device
> *cdev)
> +{
> +	unsigned long state;
> +
> +	if (IS_ERR_OR_NULL(cdev))
> +		return;
> +
> +	/*
> +	 * Hold thermal_list_lock throughout the update to prevent the
> device
> +	 * from going away while being updated.
> +	 */
> +	mutex_lock(&thermal_list_lock);
> +
> +	if (!thermal_cooling_device_present(cdev))
> +		goto unlock_list;
> +
> +	/*
> +	 * Update under the cdev lock to prevent the state from being
> set beyond
> +	 * the new limit concurrently.
> +	 */
> +	mutex_lock(&cdev->lock);
> +
> +	if (cdev->ops->get_max_state(cdev, &cdev->max_state))
> +		goto unlock;
> +
> +	thermal_cooling_device_stats_reinit(cdev);
> +
> +	if (cdev->ops->get_cur_state(cdev, &state) || state > cdev-
> >max_state)
> +		goto unlock;
> +
> +	thermal_cooling_device_stats_update(cdev, state);
> +
> +unlock:
> +	mutex_unlock(&cdev->lock);
> +
> +unlock_list:
> +	mutex_unlock(&thermal_list_lock);
> +}
> +EXPORT_SYMBOL_GPL(thermal_cooling_device_update);
> +
>  static void __unbind(struct thermal_zone_device *tz, int mask,
>  		     struct thermal_cooling_device *cdev)
>  {
> Index: linux-pm/drivers/thermal/thermal_sysfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> +++ linux-pm/drivers/thermal/thermal_sysfs.c
> @@ -685,6 +685,8 @@ void thermal_cooling_device_stats_update
>  {
>  	struct cooling_dev_stats *stats = cdev->stats;
>  
> +	lockdep_assert_held(&cdev->lock);
> +
>  	if (!stats)
>  		return;
>  
> @@ -706,13 +708,22 @@ static ssize_t total_trans_show(struct d
>  				struct device_attribute *attr, char
> *buf)
>  {
>  	struct thermal_cooling_device *cdev = to_cooling_device(dev);
> -	struct cooling_dev_stats *stats = cdev->stats;
> +	struct cooling_dev_stats *stats;
>  	int ret;
>  
> +	mutex_lock(&cdev->lock);
> +
> +	stats = cdev->stats;
> +	if (!stats)
> +		goto unlock;
> +
>  	spin_lock(&stats->lock);
>  	ret = sprintf(buf, "%u\n", stats->total_trans);
>  	spin_unlock(&stats->lock);
>  
> +unlock:
> +	mutex_unlock(&cdev->lock);
> +
>  	return ret;
>  }
>  
> @@ -721,11 +732,18 @@ time_in_state_ms_show(struct device *dev
>  		      char *buf)
>  {
>  	struct thermal_cooling_device *cdev = to_cooling_device(dev);
> -	struct cooling_dev_stats *stats = cdev->stats;
> +	struct cooling_dev_stats *stats;
>  	ssize_t len = 0;
>  	int i;
>  
> +	mutex_lock(&cdev->lock);
> +
> +	stats = cdev->stats;
> +	if (!stats)
> +		goto unlock;
> +
>  	spin_lock(&stats->lock);
> +
>  	update_time_in_state(stats);
>  
>  	for (i = 0; i <= cdev->max_state; i++) {
> @@ -734,6 +752,9 @@ time_in_state_ms_show(struct device *dev
>  	}
>  	spin_unlock(&stats->lock);
>  
> +unlock:
> +	mutex_unlock(&cdev->lock);
> +
>  	return len;
>  }
>  
> @@ -742,8 +763,16 @@ reset_store(struct device *dev, struct d
>  	    size_t count)
>  {
>  	struct thermal_cooling_device *cdev = to_cooling_device(dev);
> -	struct cooling_dev_stats *stats = cdev->stats;
> -	int i, states = cdev->max_state + 1;
> +	struct cooling_dev_stats *stats;
> +	int i, states;
> +
> +	mutex_lock(&cdev->lock);
> +
> +	stats = cdev->stats;
> +	if (!stats)
> +		goto unlock;
> +
> +	states = cdev->max_state + 1;
>  
>  	spin_lock(&stats->lock);
>  
> @@ -757,6 +786,9 @@ reset_store(struct device *dev, struct d
>  
>  	spin_unlock(&stats->lock);
>  
> +unlock:
> +	mutex_unlock(&cdev->lock);
> +
>  	return count;
>  }
>  
> @@ -764,10 +796,18 @@ static ssize_t trans_table_show(struct d
>  				struct device_attribute *attr, char
> *buf)
>  {
>  	struct thermal_cooling_device *cdev = to_cooling_device(dev);
> -	struct cooling_dev_stats *stats = cdev->stats;
> +	struct cooling_dev_stats *stats;
>  	ssize_t len = 0;
>  	int i, j;
>  
> +	mutex_lock(&cdev->lock);
> +
> +	stats = cdev->stats;
> +	if (!stats) {
> +		len = -ENODATA;
> +		goto unlock;
> +	}
> +
>  	len += snprintf(buf + len, PAGE_SIZE - len, "
> From  :    To\n");
>  	len += snprintf(buf + len, PAGE_SIZE - len, "       : ");
>  	for (i = 0; i <= cdev->max_state; i++) {
> @@ -775,8 +815,10 @@ static ssize_t trans_table_show(struct d
>  			break;
>  		len += snprintf(buf + len, PAGE_SIZE - len,
> "state%2u  ", i);
>  	}
> -	if (len >= PAGE_SIZE)
> -		return PAGE_SIZE;
> +	if (len >= PAGE_SIZE) {
> +		len = PAGE_SIZE;
> +		goto unlock;
> +	}
>  
>  	len += snprintf(buf + len, PAGE_SIZE - len, "\n");
>  
> @@ -799,8 +841,12 @@ static ssize_t trans_table_show(struct d
>  
>  	if (len >= PAGE_SIZE) {
>  		pr_warn_once("Thermal transition table exceeds
> PAGE_SIZE. Disabling\n");
> -		return -EFBIG;
> +		len = -EFBIG;
>  	}
> +
> +unlock:
> +	mutex_unlock(&cdev->lock);
> +
>  	return len;
>  }
>  
> @@ -830,6 +876,8 @@ static void cooling_device_stats_setup(s
>  	unsigned long states = cdev->max_state + 1;
>  	int var;
>  
> +	lockdep_assert_held(&cdev->lock);
> +
>  	var = sizeof(*stats);
>  	var += sizeof(*stats->time_in_state) * states;
>  	var += sizeof(*stats->trans_table) * states * states;
> @@ -855,6 +903,8 @@ out:
>  
>  static void cooling_device_stats_destroy(struct
> thermal_cooling_device *cdev)
>  {
> +	lockdep_assert_held(&cdev->lock);
> +
>  	kfree(cdev->stats);
>  	cdev->stats = NULL;
>  }
> @@ -879,6 +929,12 @@ void thermal_cooling_device_destroy_sysf
>  	cooling_device_stats_destroy(cdev);
>  }
>  
> +void thermal_cooling_device_stats_reinit(struct
> thermal_cooling_device *cdev)
> +{
> +	cooling_device_stats_destroy(cdev);
> +	cooling_device_stats_setup(cdev);
> +}
> +
>  /* these helper will be used only at the time of bindig */
>  ssize_t
>  trip_point_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -127,6 +127,7 @@ int thermal_zone_create_device_groups(st
>  void thermal_zone_destroy_device_groups(struct thermal_zone_device
> *);
>  void thermal_cooling_device_setup_sysfs(struct
> thermal_cooling_device *);
>  void thermal_cooling_device_destroy_sysfs(struct
> thermal_cooling_device *cdev);
> +void thermal_cooling_device_stats_reinit(struct
> thermal_cooling_device *cdev);
>  /* used only at binding time */
>  ssize_t trip_point_show(struct device *, struct device_attribute *,
> char *);
>  ssize_t weight_show(struct device *, struct device_attribute *, char
> *);
> Index: linux-pm/include/linux/thermal.h
> ===================================================================
> --- linux-pm.orig/include/linux/thermal.h
> +++ linux-pm/include/linux/thermal.h
> @@ -388,6 +388,7 @@ devm_thermal_of_cooling_device_register(
>  				struct device_node *np,
>  				char *type, void *devdata,
>  				const struct thermal_cooling_device_ops
> *ops);
> +void thermal_cooling_device_update(struct thermal_cooling_device *);
>  void thermal_cooling_device_unregister(struct thermal_cooling_device
> *);
>  struct thermal_zone_device *thermal_zone_get_zone_by_name(const char
> *name);
>  int thermal_zone_get_temp(struct thermal_zone_device *tz, int
> *temp);
> 
> 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 4/4] ACPI: processor: thermal: Update CPU cooling devices on cpufreq policy changes
  2023-03-13 14:34 ` [PATCH v2 4/4] ACPI: processor: thermal: Update CPU cooling devices on cpufreq policy changes Rafael J. Wysocki
@ 2023-03-14  2:03   ` Zhang, Rui
  0 siblings, 0 replies; 12+ messages in thread
From: Zhang, Rui @ 2023-03-14  2:03 UTC (permalink / raw)
  To: linux-pm, rjw
  Cc: srinivas.pandruvada, viresh.kumar, Wang, Quanxian, linux-kernel,
	daniel.lezcano, linux-acpi

On Mon, 2023-03-13 at 15:34 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> When a cpufreq policy appears or goes away, the CPU cooling devices
> for
> the CPUs covered by that policy need to be updated so that the new
> processor_get_max_state() value is stored as max_state and the
> statistics in sysfs are rearranged for each of them.
> 
> Do that accordingly in acpi_thermal_cpufreq_init() and
> acpi_thermal_cpufreq_exit().
> 
> Fixes: a365105c685c("thermal: sysfs: Reuse cdev->max_state")
> Reported-by: Wang, Quanxian <quanxian.wang@intel.com>
> Link: 
> https://lore.kernel.org/linux-pm/53ec1f06f61c984100868926f282647e57ecfb2d.camel@intel.com/
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Tested-by: Zhang Rui <rui.zhang@intel.com>
Reviewed-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui

> ---
> 
> v1 -> v2: Remove the now redundant IS_ERR() checks on cdev before
> calling
>           thermal_cooling_device_update().
> 
> ---
>  drivers/acpi/processor_thermal.c |   14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> Index: linux-pm/drivers/acpi/processor_thermal.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/processor_thermal.c
> +++ linux-pm/drivers/acpi/processor_thermal.c
> @@ -140,9 +140,13 @@ void acpi_thermal_cpufreq_init(struct cp
>  		ret = freq_qos_add_request(&policy->constraints,
>  					   &pr->thermal_req,
>  					   FREQ_QOS_MAX, INT_MAX);
> -		if (ret < 0)
> +		if (ret < 0) {
>  			pr_err("Failed to add freq constraint for CPU%d
> (%d)\n",
>  			       cpu, ret);
> +			continue;
> +		}
> +
> +		thermal_cooling_device_update(pr->cdev);
>  	}
>  }
>  
> @@ -153,8 +157,12 @@ void acpi_thermal_cpufreq_exit(struct cp
>  	for_each_cpu(cpu, policy->related_cpus) {
>  		struct acpi_processor *pr = per_cpu(processors, cpu);
>  
> -		if (pr)
> -			freq_qos_remove_request(&pr->thermal_req);
> +		if (!pr)
> +			continue;
> +
> +		freq_qos_remove_request(&pr->thermal_req);
> +
> +		thermal_cooling_device_update(pr->cdev);
>  	}
>  }
>  #else				/* ! CONFIG_CPU_FREQ */
> 
> 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-03-14  2:03 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-13 14:24 [PATCH v2 0/4] thermal: core/ACPI: Fix processor cooling device regression Rafael J. Wysocki
2023-03-13 14:27 ` [PATCH v2 1/4] ACPI: processor: Reorder acpi_processor_driver_init() Rafael J. Wysocki
2023-03-14  2:03   ` Zhang, Rui
2023-03-13 14:28 ` [PATCH v2 2/4] thermal: core: Introduce thermal_cooling_device_present() Rafael J. Wysocki
2023-03-14  2:03   ` Zhang, Rui
2023-03-13 14:32 ` [PATCH v2 3/4] thermal: core: Introduce thermal_cooling_device_update() Rafael J. Wysocki
2023-03-14  2:03   ` Zhang, Rui
2023-03-13 14:34 ` [PATCH v2 4/4] ACPI: processor: thermal: Update CPU cooling devices on cpufreq policy changes Rafael J. Wysocki
2023-03-14  2:03   ` Zhang, Rui
2023-03-13 16:47 ` [PATCH v2 0/4] thermal: core/ACPI: Fix processor cooling device regression Zhang, Rui
2023-03-13 18:02   ` Rafael J. Wysocki
2023-03-14  2:02     ` Zhang, Rui

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).