In the work of the thermal zone device self-encapsulation, let's pass
the field value instead of dereferencing them in the traces which
force us to export publicly the thermal_zone_device structure.

No fonctionnal change intended.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/gov_fair_share.c              |  4 +++-
 drivers/thermal/gov_power_allocator.c         |  6 +++--
 drivers/thermal/gov_step_wise.c               |  4 +++-
 drivers/thermal/thermal_core.c                |  8 +++++--
 include/trace/events/thermal.h                | 24 +++++++++----------
 .../trace/events/thermal_power_allocator.h    | 12 +++++-----
 6 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/drivers/thermal/gov_fair_share.c b/drivers/thermal/gov_fair_share.c
index aad7d5fe3a14..cdddd593021d 100644
--- a/drivers/thermal/gov_fair_share.c
+++ b/drivers/thermal/gov_fair_share.c
@@ -35,7 +35,9 @@ static int get_trip_level(struct thermal_zone_device *tz)
 	 * point, in which case, trip_point = count - 1
 	 */
 	if (count > 0)
-		trace_thermal_zone_trip(tz, count - 1, trip.type);
+		trace_thermal_zone_trip(thermal_zone_device_type(tz),
+					thermal_zone_device_id(tz),
+					count - 1, trip.type);
 
 	return count;
 }
diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 0eaf1527d3e3..bebab8b8694b 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -266,7 +266,8 @@ static u32 pid_controller(struct thermal_zone_device *tz,
 
 	power_range = clamp(power_range, (s64)0, (s64)max_allocatable_power);
 
-	trace_thermal_power_allocator_pid(tz, frac_to_int(err),
+	trace_thermal_power_allocator_pid(thermal_zone_device_id(tz),
+					  frac_to_int(err),
 					  frac_to_int(params->err_integral),
 					  frac_to_int(p), frac_to_int(i),
 					  frac_to_int(d), power_range);
@@ -481,7 +482,8 @@ static int allocate_power(struct thermal_zone_device *tz,
 		i++;
 	}
 
-	trace_thermal_power_allocator(tz, req_power, total_req_power,
+	trace_thermal_power_allocator(thermal_zone_device_id(tz),
+				      req_power, total_req_power,
 				      granted_power, total_granted_power,
 				      num_actors, power_range,
 				      max_allocatable_power, tz->temperature,
diff --git a/drivers/thermal/gov_step_wise.c b/drivers/thermal/gov_step_wise.c
index 31235e169c5a..81ef43a40f98 100644
--- a/drivers/thermal/gov_step_wise.c
+++ b/drivers/thermal/gov_step_wise.c
@@ -109,7 +109,9 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip_id
 
 	if (tz->temperature >= trip.temperature) {
 		throttle = true;
-		trace_thermal_zone_trip(tz, trip_id, trip.type);
+		trace_thermal_zone_trip(thermal_zone_device_type(tz),
+					thermal_zone_device_id(tz),
+					trip_id, trip.type);
 	}
 
 	dev_dbg(&tz->device, "Trip%d[type=%d,temp=%d]:trend=%d,throttle=%d\n",
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 46dedfe061df..c856ab911149 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -336,7 +336,9 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
 	if (trip_temp <= 0 || tz->temperature < trip_temp)
 		return;
 
-	trace_thermal_zone_trip(tz, trip, trip_type);
+	trace_thermal_zone_trip(thermal_zone_device_type(tz),
+				thermal_zone_device_id(tz),
+				trip, trip_type);
 
 	if (trip_type == THERMAL_TRIP_HOT && tz->ops->hot)
 		tz->ops->hot(tz);
@@ -387,7 +389,9 @@ static void update_temperature(struct thermal_zone_device *tz)
 	tz->last_temperature = tz->temperature;
 	tz->temperature = temp;
 
-	trace_thermal_temperature(tz);
+	trace_thermal_temperature(thermal_zone_device_type(tz),
+				  thermal_zone_device_id(tz),
+				  tz->last_temperature, tz->temperature);
 
 	thermal_genl_sampling_temp(tz->id, temp);
 }
diff --git a/include/trace/events/thermal.h b/include/trace/events/thermal.h
index e58bf3072f32..50c7d2e1526d 100644
--- a/include/trace/events/thermal.h
+++ b/include/trace/events/thermal.h
@@ -23,22 +23,22 @@ TRACE_DEFINE_ENUM(THERMAL_TRIP_ACTIVE);
 
 TRACE_EVENT(thermal_temperature,
 
-	TP_PROTO(struct thermal_zone_device *tz),
+	TP_PROTO(const char *type, int id, int temp_prev, int temp),
 
-	TP_ARGS(tz),
+	TP_ARGS(type, id, temp_prev, temp),
 
 	TP_STRUCT__entry(
-		__string(thermal_zone, tz->type)
+		__string(thermal_zone, type)
 		__field(int, id)
 		__field(int, temp_prev)
 		__field(int, temp)
 	),
 
 	TP_fast_assign(
-		__assign_str(thermal_zone, tz->type);
-		__entry->id = tz->id;
-		__entry->temp_prev = tz->last_temperature;
-		__entry->temp = tz->temperature;
+		__assign_str(thermal_zone, type);
+		__entry->id = id;
+		__entry->temp_prev = temp_prev;
+		__entry->temp = temp;
 	),
 
 	TP_printk("thermal_zone=%s id=%d temp_prev=%d temp=%d",
@@ -67,21 +67,21 @@ TRACE_EVENT(cdev_update,
 
 TRACE_EVENT(thermal_zone_trip,
 
-	TP_PROTO(struct thermal_zone_device *tz, int trip,
+	TP_PROTO(const char *type, int id, int trip,
 		enum thermal_trip_type trip_type),
 
-	TP_ARGS(tz, trip, trip_type),
+	TP_ARGS(type, id, trip, trip_type),
 
 	TP_STRUCT__entry(
-		__string(thermal_zone, tz->type)
+		__string(thermal_zone, type)
 		__field(int, id)
 		__field(int, trip)
 		__field(enum thermal_trip_type, trip_type)
 	),
 
 	TP_fast_assign(
-		__assign_str(thermal_zone, tz->type);
-		__entry->id = tz->id;
+		__assign_str(thermal_zone, type);
+		__entry->id = id;
 		__entry->trip = trip;
 		__entry->trip_type = trip_type;
 	),
diff --git a/include/trace/events/thermal_power_allocator.h b/include/trace/events/thermal_power_allocator.h
index 1c8fb95544f9..7ac049e7e3cf 100644
--- a/include/trace/events/thermal_power_allocator.h
+++ b/include/trace/events/thermal_power_allocator.h
@@ -8,12 +8,12 @@
 #include <linux/tracepoint.h>
 
 TRACE_EVENT(thermal_power_allocator,
-	TP_PROTO(struct thermal_zone_device *tz, u32 *req_power,
+	TP_PROTO(int id, u32 *req_power,
 		 u32 total_req_power, u32 *granted_power,
 		 u32 total_granted_power, size_t num_actors,
 		 u32 power_range, u32 max_allocatable_power,
 		 int current_temp, s32 delta_temp),
-	TP_ARGS(tz, req_power, total_req_power, granted_power,
+	TP_ARGS(id, req_power, total_req_power, granted_power,
 		total_granted_power, num_actors, power_range,
 		max_allocatable_power, current_temp, delta_temp),
 	TP_STRUCT__entry(
@@ -29,7 +29,7 @@ TRACE_EVENT(thermal_power_allocator,
 		__field(s32,           delta_temp               )
 	),
 	TP_fast_assign(
-		__entry->tz_id = tz->id;
+		__entry->tz_id = id;
 		memcpy(__get_dynamic_array(req_power), req_power,
 			num_actors * sizeof(*req_power));
 		__entry->total_req_power = total_req_power;
@@ -56,9 +56,9 @@ TRACE_EVENT(thermal_power_allocator,
 );
 
 TRACE_EVENT(thermal_power_allocator_pid,
-	TP_PROTO(struct thermal_zone_device *tz, s32 err, s32 err_integral,
+	TP_PROTO(int id, s32 err, s32 err_integral,
 		 s64 p, s64 i, s64 d, s32 output),
-	TP_ARGS(tz, err, err_integral, p, i, d, output),
+	TP_ARGS(id, err, err_integral, p, i, d, output),
 	TP_STRUCT__entry(
 		__field(int, tz_id       )
 		__field(s32, err         )
@@ -69,7 +69,7 @@ TRACE_EVENT(thermal_power_allocator_pid,
 		__field(s32, output      )
 	),
 	TP_fast_assign(
-		__entry->tz_id = tz->id;
+		__entry->tz_id = id;
 		__entry->err = err;
 		__entry->err_integral = err_integral;
 		__entry->p = p;
-- 
2.34.1

On Sun, 26 Feb 2023 23:54:06 +0100
Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

> In the work of the thermal zone device self-encapsulation, let's pass
> the field value instead of dereferencing them in the traces which
> force us to export publicly the thermal_zone_device structure.
> 
> No fonctionnal change intended.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/gov_fair_share.c              |  4 +++-
>  drivers/thermal/gov_power_allocator.c         |  6 +++--
>  drivers/thermal/gov_step_wise.c               |  4 +++-
>  drivers/thermal/thermal_core.c                |  8 +++++--
>  include/trace/events/thermal.h                | 24 +++++++++----------
>  .../trace/events/thermal_power_allocator.h    | 12 +++++-----
>  6 files changed, 34 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/thermal/gov_fair_share.c b/drivers/thermal/gov_fair_share.c
> index aad7d5fe3a14..cdddd593021d 100644
> --- a/drivers/thermal/gov_fair_share.c
> +++ b/drivers/thermal/gov_fair_share.c
> @@ -35,7 +35,9 @@ static int get_trip_level(struct thermal_zone_device *tz)
>  	 * point, in which case, trip_point = count - 1
>  	 */
>  	if (count > 0)
> -		trace_thermal_zone_trip(tz, count - 1, trip.type);
> +		trace_thermal_zone_trip(thermal_zone_device_type(tz),
> +					thermal_zone_device_id(tz),
> +					count - 1, trip.type);

The problem with this approach is that you are moving all the work to
dereference the pointers into the hot paths (the code execution), instead
of doing it in the slow path (where the tracepoint work is done).

If you are concerned with exporting a structure then move the trace file
from:

  include/trace/events/thermal.h to drivers/thermal/trace.h

like drivers/vfio/pci/trace.h and many other drivers do.

Read
   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/trace_events/Makefile
to see how to use a trace header outside the include/trace/events directory.

also, by removing the pointer, you lose out on BPF and kprobe traces that
could dereference the pointer if you needed to trace something that was not
exported by the trace point itself.

-- Steve


>  
>  	return count;
>  }

Hi Steven,


On 27/02/2023 16:07, Steven Rostedt wrote:
> On Sun, 26 Feb 2023 23:54:06 +0100
> Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> 
>> In the work of the thermal zone device self-encapsulation, let's pass
>> the field value instead of dereferencing them in the traces which
>> force us to export publicly the thermal_zone_device structure.
>>
>> No fonctionnal change intended.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>   drivers/thermal/gov_fair_share.c              |  4 +++-
>>   drivers/thermal/gov_power_allocator.c         |  6 +++--
>>   drivers/thermal/gov_step_wise.c               |  4 +++-
>>   drivers/thermal/thermal_core.c                |  8 +++++--
>>   include/trace/events/thermal.h                | 24 +++++++++----------
>>   .../trace/events/thermal_power_allocator.h    | 12 +++++-----
>>   6 files changed, 34 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/thermal/gov_fair_share.c b/drivers/thermal/gov_fair_share.c
>> index aad7d5fe3a14..cdddd593021d 100644
>> --- a/drivers/thermal/gov_fair_share.c
>> +++ b/drivers/thermal/gov_fair_share.c
>> @@ -35,7 +35,9 @@ static int get_trip_level(struct thermal_zone_device *tz)
>>   	 * point, in which case, trip_point = count - 1
>>   	 */
>>   	if (count > 0)
>> -		trace_thermal_zone_trip(tz, count - 1, trip.type);
>> +		trace_thermal_zone_trip(thermal_zone_device_type(tz),
>> +					thermal_zone_device_id(tz),
>> +					count - 1, trip.type);
> 
> The problem with this approach is that you are moving all the work to
> dereference the pointers into the hot paths (the code execution), instead
> of doing it in the slow path (where the tracepoint work is done).

Good point, that is something I did not realize, thanks for pointing it out.

> If you are concerned with exporting a structure then move the trace file
> from:
>
>    include/trace/events/thermal.h to drivers/thermal/trace.h
> 
> like drivers/vfio/pci/trace.h and many other drivers do.

Good idea, I'll do that

> Read
>     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/trace_events/Makefile
> to see how to use a trace header outside the include/trace/events directory.
> 
> also, by removing the pointer, you lose out on BPF and kprobe traces that
> could dereference the pointer if you needed to trace something that was not
> exported by the trace point itself.

As the trace will be in the drivers/thermal/trace.h, it will be able to 
use the thermal_core.h private file and no longer 
include/linux/thermal.h, so preventing to unexport the thermal zone 
structure from thermal.h. Consequently, we no longer have to change the 
prototype of the traces and the pointer will stay in place.

Thanks for your suggestions


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog