[PATCH v2 03/12] thermal: core: Represent suspend-related thermal zone flags as bits

Posted by Rafael J. Wysocki 1 month, 3 weeks ago

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

Instead of using two separate fields in struct thermal_zone_device for
representing flags related to thermal zone suspend, represent them
explicitly as bits in one u8 "state" field.

Subsequently, that field will be used for addressing race conditions
related to thermal zone initialization and exit.

No intentional functional impact.

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

This is a new iteration of

https://lore.kernel.org/linux-pm/2215885.irdbgypaU6@rjwysocki.net/

v1 -> v2: The thermal zone guard has not been introduced yet, so adjust for that.

---
 drivers/thermal/thermal_core.c |   11 +++++------
 drivers/thermal/thermal_core.h |   11 +++++++----
 2 files changed, 12 insertions(+), 10 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -547,7 +547,7 @@ void __thermal_zone_device_update(struct
 	int low = -INT_MAX, high = INT_MAX;
 	int temp, ret;
 
-	if (tz->suspended || tz->mode != THERMAL_DEVICE_ENABLED)
+	if (tz->state != TZ_STATE_READY || tz->mode != THERMAL_DEVICE_ENABLED)
 		return;
 
 	ret = __thermal_zone_get_temp(tz, &temp);
@@ -1662,7 +1662,7 @@ static void thermal_zone_device_resume(s
 
 	mutex_lock(&tz->lock);
 
-	tz->suspended = false;
+	tz->state &= ~(TZ_STATE_FLAG_SUSPENDED | TZ_STATE_FLAG_RESUMING);
 
 	thermal_debug_tz_resume(tz);
 	thermal_zone_device_init(tz);
@@ -1670,7 +1670,6 @@ static void thermal_zone_device_resume(s
 	__thermal_zone_device_update(tz, THERMAL_TZ_RESUME);
 
 	complete(&tz->resume);
-	tz->resuming = false;
 
 	mutex_unlock(&tz->lock);
 }
@@ -1679,7 +1678,7 @@ static void thermal_zone_pm_prepare(stru
 {
 	mutex_lock(&tz->lock);
 
-	if (tz->resuming) {
+	if (tz->state & TZ_STATE_FLAG_RESUMING) {
 		/*
 		 * thermal_zone_device_resume() queued up for this zone has not
 		 * acquired the lock yet, so release it to let the function run
@@ -1692,7 +1691,7 @@ static void thermal_zone_pm_prepare(stru
 		mutex_lock(&tz->lock);
 	}
 
-	tz->suspended = true;
+	tz->state |= TZ_STATE_FLAG_SUSPENDED;
 
 	mutex_unlock(&tz->lock);
 }
@@ -1704,7 +1703,7 @@ static void thermal_zone_pm_complete(str
 	cancel_delayed_work(&tz->poll_queue);
 
 	reinit_completion(&tz->resume);
-	tz->resuming = true;
+	tz->state |= TZ_STATE_FLAG_RESUMING;
 
 	/*
 	 * Replace the work function with the resume one, which will restore the
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -61,6 +61,11 @@ struct thermal_governor {
 	struct list_head	governor_list;
 };
 
+#define	TZ_STATE_FLAG_SUSPENDED	BIT(0)
+#define	TZ_STATE_FLAG_RESUMING	BIT(1)
+
+#define TZ_STATE_READY		0
+
 /**
  * struct thermal_zone_device - structure for a thermal zone
  * @id:		unique id number for each thermal zone
@@ -100,8 +105,7 @@ struct thermal_governor {
  * @node:	node in thermal_tz_list (in thermal_core.c)
  * @poll_queue:	delayed work for polling
  * @notify_event: Last notification event
- * @suspended: thermal zone suspend indicator
- * @resuming:	indicates whether or not thermal zone resume is in progress
+ * @state: 	current state of the thermal zone
  * @trips:	array of struct thermal_trip objects
  */
 struct thermal_zone_device {
@@ -134,8 +138,7 @@ struct thermal_zone_device {
 	struct list_head node;
 	struct delayed_work poll_queue;
 	enum thermal_notify_event notify_event;
-	bool suspended;
-	bool resuming;
+	u8 state;
 #ifdef CONFIG_THERMAL_DEBUGFS
 	struct thermal_debugfs *debugfs;
 #endif

Re: [PATCH v2 03/12] thermal: core: Represent suspend-related thermal zone flags as bits

Posted by Lukasz Luba 1 month, 1 week ago


On 10/4/24 20:11, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Instead of using two separate fields in struct thermal_zone_device for
> representing flags related to thermal zone suspend, represent them
> explicitly as bits in one u8 "state" field.
> 
> Subsequently, that field will be used for addressing race conditions
> related to thermal zone initialization and exit.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> This is a new iteration of
> 
> https://lore.kernel.org/linux-pm/2215885.irdbgypaU6@rjwysocki.net/
> 
> v1 -> v2: The thermal zone guard has not been introduced yet, so adjust for that.
> 
> ---
>   drivers/thermal/thermal_core.c |   11 +++++------
>   drivers/thermal/thermal_core.h |   11 +++++++----
>   2 files changed, 12 insertions(+), 10 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -547,7 +547,7 @@ void __thermal_zone_device_update(struct
>   	int low = -INT_MAX, high = INT_MAX;
>   	int temp, ret;
>   
> -	if (tz->suspended || tz->mode != THERMAL_DEVICE_ENABLED)
> +	if (tz->state != TZ_STATE_READY || tz->mode != THERMAL_DEVICE_ENABLED)
>   		return;
>   
>   	ret = __thermal_zone_get_temp(tz, &temp);
> @@ -1662,7 +1662,7 @@ static void thermal_zone_device_resume(s
>   
>   	mutex_lock(&tz->lock);
>   
> -	tz->suspended = false;
> +	tz->state &= ~(TZ_STATE_FLAG_SUSPENDED | TZ_STATE_FLAG_RESUMING);

I see, we now clean both in on go...

>   
>   	thermal_debug_tz_resume(tz);
>   	thermal_zone_device_init(tz);
> @@ -1670,7 +1670,6 @@ static void thermal_zone_device_resume(s
>   	__thermal_zone_device_update(tz, THERMAL_TZ_RESUME);
>   
>   	complete(&tz->resume);
> -	tz->resuming = false;

so handled as well.

>   
>   	mutex_unlock(&tz->lock);
>   }
> @@ -1679,7 +1678,7 @@ static void thermal_zone_pm_prepare(stru
>   {
>   	mutex_lock(&tz->lock);
>   
> -	if (tz->resuming) {
> +	if (tz->state & TZ_STATE_FLAG_RESUMING) {
>   		/*
>   		 * thermal_zone_device_resume() queued up for this zone has not
>   		 * acquired the lock yet, so release it to let the function run
> @@ -1692,7 +1691,7 @@ static void thermal_zone_pm_prepare(stru
>   		mutex_lock(&tz->lock);
>   	}
>   
> -	tz->suspended = true;
> +	tz->state |= TZ_STATE_FLAG_SUSPENDED;
>   
>   	mutex_unlock(&tz->lock);
>   }
> @@ -1704,7 +1703,7 @@ static void thermal_zone_pm_complete(str
>   	cancel_delayed_work(&tz->poll_queue);
>   
>   	reinit_completion(&tz->resume);
> -	tz->resuming = true;
> +	tz->state |= TZ_STATE_FLAG_RESUMING;
>   
>   	/*
>   	 * Replace the work function with the resume one, which will restore the
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -61,6 +61,11 @@ struct thermal_governor {
>   	struct list_head	governor_list;
>   };
>   
> +#define	TZ_STATE_FLAG_SUSPENDED	BIT(0)
> +#define	TZ_STATE_FLAG_RESUMING	BIT(1)
> +
> +#define TZ_STATE_READY		0
> +
>   /**
>    * struct thermal_zone_device - structure for a thermal zone
>    * @id:		unique id number for each thermal zone
> @@ -100,8 +105,7 @@ struct thermal_governor {
>    * @node:	node in thermal_tz_list (in thermal_core.c)
>    * @poll_queue:	delayed work for polling
>    * @notify_event: Last notification event
> - * @suspended: thermal zone suspend indicator
> - * @resuming:	indicates whether or not thermal zone resume is in progress
> + * @state: 	current state of the thermal zone
>    * @trips:	array of struct thermal_trip objects
>    */
>   struct thermal_zone_device {
> @@ -134,8 +138,7 @@ struct thermal_zone_device {
>   	struct list_head node;
>   	struct delayed_work poll_queue;
>   	enum thermal_notify_event notify_event;
> -	bool suspended;
> -	bool resuming;
> +	u8 state;
>   #ifdef CONFIG_THERMAL_DEBUGFS
>   	struct thermal_debugfs *debugfs;
>   #endif
> 
> 
> 

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>