[PATCH] thermal: Add ratelimited print for HOT trip point

[PATCH] thermal: Add ratelimited print for HOT trip point

[Vincent Whitchurch]

Currently, HOT trips don't do much in the core except for emitting an
ftrace event.  Unlike for ACTIVE/PASSIVE, cooling/throttling is not
activated.

The ACPI driver sends a netlink notification to userspace, but the other
drivers either don't implement ->notify() (all but rcar_thermal), or
don't do anything for HOT in their ->notify() implementation
(rcar_thermal).  (tegra appears to use the HOT trip device tree data for
other purposes.)

To make this trip point a bit more useful, add a warning print in the
core when this temperature is reached, so that this information is
available in the kernel log for diagnostic purposes.  The warning is
ratelimited by default to once an hour and can be adjusted with module
parameters.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
 drivers/thermal/thermal_core.c | 12 ++++++++++++
 include/linux/thermal.h        |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 9a321dc548c8..06f1dfae0310 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -47,6 +47,12 @@ static bool power_off_triggered;
 
 static struct thermal_governor *def_governor;
 
+static unsigned int thermal_ratelimit_interval = 60 * 60 * HZ;
+static unsigned int thermal_ratelimit_burst = 1;
+
+module_param_named(ratelimit_interval, thermal_ratelimit_interval, uint, 0);
+module_param_named(ratelimit_burst, thermal_ratelimit_burst, uint, 0);
+
 /*
  * Governor section: set of functions to handle thermal governors
  *
@@ -404,6 +410,9 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
 			power_off_triggered = true;
 		}
 		mutex_unlock(&poweroff_lock);
+	} else if (__ratelimit(&tz->ratelimit)) {
+		dev_warn(&tz->device, "hot temperature reached (%d C)\n",
+			  tz->temperature / 1000);
 	}
 }
 
@@ -1290,6 +1299,9 @@ thermal_zone_device_register(const char *type, int trips, int mask,
 	tz->passive_delay = passive_delay;
 	tz->polling_delay = polling_delay;
 
+	ratelimit_state_init(&tz->ratelimit, thermal_ratelimit_interval,
+			     thermal_ratelimit_burst);
+
 	/* sys I/F */
 	/* Add nodes that are always present via .groups */
 	result = thermal_zone_create_device_groups(tz, mask);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 126913c6a53b..d3d0d6c7c4a1 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -196,6 +196,7 @@ struct thermal_zone_device {
 	int passive;
 	int prev_low_trip;
 	int prev_high_trip;
+	struct ratelimit_state ratelimit;
 	unsigned int forced_passive;
 	atomic_t need_update;
 	struct thermal_zone_device_ops *ops;
-- 
2.25.1

Re: [PATCH] thermal: Add ratelimited print for HOT trip point

[Daniel Lezcano]

On 02/04/2020 15:52, Vincent Whitchurch wrote:
> Currently, HOT trips don't do much in the core except for emitting
> an ftrace event.  Unlike for ACTIVE/PASSIVE, cooling/throttling is
> not activated.
>
> The ACPI driver sends a netlink notification to userspace, but the
> other drivers either don't implement ->notify() (all but
> rcar_thermal), or don't do anything for HOT in their ->notify()
> implementation (rcar_thermal).  (tegra appears to use the HOT trip
> device tree data for other purposes.)
>
> To make this trip point a bit more useful, add a warning print in
> the core when this temperature is reached, so that this information
> is available in the kernel log for diagnostic purposes.  The
> warning is ratelimited by default to once an hour and can be
> adjusted with module parameters.
>
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>

Let me propose an alternative to solve your problem.




-- 
<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

[PATCH] thermal: core: Send a sysfs notification on trip points

[Daniel Lezcano]

Currently the userspace has no easy way to get notified when a
specific trip point was crossed. There are a couple of approaches:

- the userspace polls the sysfs temperature with usually an
  unacceptable delay between the trip temperature point crossing and
  the moment it is detected, or a high polling rate with an
  unacceptable number of wakeup events.

- the thermal zone is set to be managed by an userspace governor in
  order to receive the uevent even if the thermal zone needs to be
  managed by another governor.

These changes allow to send a sysfs notification on the
trip_point_*_temp when the temperature is getting higher than the trip
point temperature. By this way, the userspace can be notified
everytime when the trip point is crossed, this is useful for the
thermal Android HAL or for notification to be sent via d-bus.

That allows the userspace to manage the applications based on specific
alerts on different thermal zones to mitigate the skin temperature,
letting the kernel governors handle the high temperature for hardware
like the CPU, the GPU or the modem.

The temperature can be oscillating around a trip point and the event
will be sent multiple times. It is up to the userspace to deal with
this situation.

The following userspace program allows to monitor those events:

struct trip_data {
       int fd;
       int temperature;
       const char *path;
};

int main(int argc, char *argv[])
{
	int efd, i;
	struct trip_data *td;
	struct epoll_event epe;

	if (argc < 2) {
		fprintf(stderr, "%s <trip1> ... <tripn>\n", argv[0]);
		return 1;
	}

	if (argc > MAX_TRIPS) {
		fprintf(stderr, "Max trip points supported: %d\n", MAX_TRIPS);
		return 1;
	}

	efd = epoll_create(MAX_TRIPS);
	if (efd <  0) {
		fprintf(stderr, "epoll_create failed: %d\n", errno);
		return 1;
	}

	for (i = 0; i < argc - 1; i++) {

		FILE *f;
		int temperature;

		f = fopen(argv[i + 1], "r");
		if (!f) {
			fprintf(stderr, "Failed to open '%s': %d\n", argv[i + 1], errno);
			return 1;
		}

		td = malloc(sizeof(*td));
		if (!td) {
			fprintf(stderr, "Failed to allocate trip_data\n");
			return 1;
		}

		fscanf(f, "%d\n", &temperature);
		rewind(f);

		td->fd = fileno(f);
		td->path = argv[i + 1];
		td->temperature = temperature;

		epe.events = EPOLLIN | EPOLLET;
		epe.data.ptr = td;

		if (epoll_ctl(efd, EPOLL_CTL_ADD, td->fd, &epe)) {
			fprintf(stderr, "Failed to epoll_ctl: %d\n", errno);
			return 1;
		}

		printf("Set '%s' for temperature '%d'\n",
		       td->path, td->temperature);
	}

	while(1) {

		if (epoll_wait(efd, &epe, 1, -1) < 0) {
			fprintf(stderr, "Failed to epoll_wait: %d\n", errno);
			return 1;
		}

		td = epe.data.ptr;

		printf("The trip point '%s' crossed the temperature '%d'\n",
		       td->path, td->temperature);
	}

	return 0;
}

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index c06550930979..3cbdd20252ab 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -407,6 +407,19 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
 	}
 }
 
+static int thermal_trip_crossed(struct thermal_zone_device *tz, int trip)
+{
+	int trip_temp;
+
+	tz->ops->get_trip_temp(tz, trip, &trip_temp);
+
+	if (tz->last_temperature == THERMAL_TEMP_INVALID)
+		return 0;
+
+	return ((tz->last_temperature < trip_temp)) &&
+		(tz->temperature >= trip_temp));
+}
+
 static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
 {
 	enum thermal_trip_type type;
@@ -417,6 +430,16 @@ static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
 
 	tz->ops->get_trip_type(tz, trip, &type);
 
+	/*
+	 * This condition will be true everytime the temperature is
+	 * greater than the trip point and the previous temperature
+	 * was below. In this case notify the userspace via a sysfs
+	 * event on the trip point.
+	 */
+	if (thermal_trip_crossed(tz, trip))
+		sysfs_notify(&tz->device.kobj, NULL,
+			     tz->trip_temp_attrs[trip].attr.attr.name);
+
 	if (type == THERMAL_TRIP_CRITICAL || type == THERMAL_TRIP_HOT)
 		handle_critical_trips(tz, trip, type);
 	else
-- 
2.17.1

Re: [PATCH] thermal: core: Send a sysfs notification on trip points

[Vincent Whitchurch]

On Thu, Apr 02, 2020 at 04:21:15PM +0200, Daniel Lezcano wrote:
> Currently the userspace has no easy way to get notified when a
> specific trip point was crossed. There are a couple of approaches:
> 
> - the userspace polls the sysfs temperature with usually an
>   unacceptable delay between the trip temperature point crossing and
>   the moment it is detected, or a high polling rate with an
>   unacceptable number of wakeup events.
> 
> - the thermal zone is set to be managed by an userspace governor in
>   order to receive the uevent even if the thermal zone needs to be
>   managed by another governor.
> 
> These changes allow to send a sysfs notification on the
> trip_point_*_temp when the temperature is getting higher than the trip
> point temperature. By this way, the userspace can be notified
> everytime when the trip point is crossed, this is useful for the
> thermal Android HAL or for notification to be sent via d-bus.
> 
> That allows the userspace to manage the applications based on specific
> alerts on different thermal zones to mitigate the skin temperature,
> letting the kernel governors handle the high temperature for hardware
> like the CPU, the GPU or the modem.
> 
> The temperature can be oscillating around a trip point and the event
> will be sent multiple times. It is up to the userspace to deal with
> this situation.

The actual temperature value would also be interesting.  Is there a way
for userspace to obtain it in a race-free manner when it is notified
that the trip point has been crossed?

> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index c06550930979..3cbdd20252ab 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -407,6 +407,19 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
>  	}
>  }
>  
> +static int thermal_trip_crossed(struct thermal_zone_device *tz, int trip)
> +{
> +	int trip_temp;
> +
> +	tz->ops->get_trip_temp(tz, trip, &trip_temp);
> +
> +	if (tz->last_temperature == THERMAL_TEMP_INVALID)
> +		return 0;
> +
> +	return ((tz->last_temperature < trip_temp)) &&
> +		(tz->temperature >= trip_temp));

 drivers/thermal/thermal_core.c: In function ‘thermal_trip_crossed’:
 drivers/thermal/thermal_core.c:425:33: error: expected ‘;’ before ‘)’ token
    (tz->temperature >= trip_temp));
                                  ^
 drivers/thermal/thermal_core.c:425:33: error: expected statement before ‘)’ token

> +}
> +
>  static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
>  {
>  	enum thermal_trip_type type;
> @@ -417,6 +430,16 @@ static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
>  
>  	tz->ops->get_trip_type(tz, trip, &type);
>  
> +	/*
> +	 * This condition will be true everytime the temperature is
> +	 * greater than the trip point and the previous temperature
> +	 * was below. In this case notify the userspace via a sysfs
> +	 * event on the trip point.
> +	 */
> +	if (thermal_trip_crossed(tz, trip))
> +		sysfs_notify(&tz->device.kobj, NULL,
> +			     tz->trip_temp_attrs[trip].attr.attr.name);

Normally sysfs_notify() is used to notify userspace that the value of
the sysfs file has changed, but in this case it's being used on a sysfs
file whose value never changes.  I don't know if there are other drivers
that do something similar.

Re: [PATCH] thermal: core: Send a sysfs notification on trip points

[Daniel Lezcano]

On 03/04/2020 16:40, Vincent Whitchurch wrote:
> On Thu, Apr 02, 2020 at 04:21:15PM +0200, Daniel Lezcano wrote:
>> Currently the userspace has no easy way to get notified when a
>> specific trip point was crossed. There are a couple of
>> approaches:
>>
>> - the userspace polls the sysfs temperature with usually an
>> unacceptable delay between the trip temperature point crossing
>> and the moment it is detected, or a high polling rate with an
>> unacceptable number of wakeup events.
>>
>> - the thermal zone is set to be managed by an userspace governor
>> in order to receive the uevent even if the thermal zone needs to
>> be managed by another governor.
>>
>> These changes allow to send a sysfs notification on the
>> trip_point_*_temp when the temperature is getting higher than the
>> trip point temperature. By this way, the userspace can be
>> notified everytime when the trip point is crossed, this is useful
>> for the thermal Android HAL or for notification to be sent via
>> d-bus.
>>
>> That allows the userspace to manage the applications based on
>> specific alerts on different thermal zones to mitigate the skin
>> temperature, letting the kernel governors handle the high
>> temperature for hardware like the CPU, the GPU or the modem.
>>
>> The temperature can be oscillating around a trip point and the
>> event will be sent multiple times. It is up to the userspace to
>> deal with this situation.
>
> The actual temperature value would also be interesting.  Is there a
> way for userspace to obtain it in a race-free manner when it is
> notified that the trip point has been crossed?

No and IMO that would not make sense because even if there is a
guarantee you have the temperature with the notification, one micro
second later it could be less than the trip point. The content of the
trip point file is the temperature you can rely on as a start of the
sampling.

The trip point notification must be the trigger to start reading the
temperatures and the polling sampling gives the accuracy of the results.

The hysteresis value can reduce the oscillation with the notifications
but the userspace must ensure in any case it is able to deal with
multiple notifications.

There are some plans to create a new mechanism to notify and pass data
from kernel space to user space via a kfifo but that is fairly new
framework with a lot of cleanup before in the thermal core.


>> diff --git a/drivers/thermal/thermal_core.c
>> b/drivers/thermal/thermal_core.c index c06550930979..3cbdd20252ab
>> 100644 --- a/drivers/thermal/thermal_core.c +++
>> b/drivers/thermal/thermal_core.c @@ -407,6 +407,19 @@ static void
>> handle_critical_trips(struct thermal_zone_device *tz, } }
>>
>> +static int thermal_trip_crossed(struct thermal_zone_device *tz,
>> int trip) +{ +	int trip_temp; + +	tz->ops->get_trip_temp(tz,
>> trip, &trip_temp); + +	if (tz->last_temperature ==
>> THERMAL_TEMP_INVALID) +		return 0; + +	return
>> ((tz->last_temperature < trip_temp)) && +		(tz->temperature >=
>> trip_temp));
>
> drivers/thermal/thermal_core.c: In function
> ‘thermal_trip_crossed’: drivers/thermal/thermal_core.c:425:33:
> error: expected ‘;’ before ‘)’ token (tz->temperature >=
> trip_temp)); ^ drivers/thermal/thermal_core.c:425:33: error:
> expected statement before ‘)’ token

Yep, I will fix that.

Thanks for reporting.

>> +} + static void handle_thermal_trip(struct thermal_zone_device
>> *tz, int trip) { enum thermal_trip_type type; @@ -417,6 +430,16
>> @@ static void handle_thermal_trip(struct thermal_zone_device
>> *tz, int trip)
>>
>> tz->ops->get_trip_type(tz, trip, &type);
>>
>> +	/* +	 * This condition will be true everytime the temperature
>> is +	 * greater than the trip point and the previous temperature
>> +	 * was below. In this case notify the userspace via a sysfs +
>> * event on the trip point. +	 */ +	if (thermal_trip_crossed(tz,
>> trip)) +		sysfs_notify(&tz->device.kobj, NULL, +
>> tz->trip_temp_attrs[trip].attr.attr.name);
>
> Normally sysfs_notify() is used to notify userspace that the value
> of the sysfs file has changed, but in this case it's being used on
> a sysfs file whose value never changes.  I don't know if there are
> other drivers that do something similar.

I think so:

eg.

drivers/hwmon/adt7x10.c:
	sysfs_notify(&dev->kobj, NULL, "temp1_max_alarm");
drivers/hwmon/adt7x10.c:
	sysfs_notify(&dev->kobj, NULL, "temp1_min_alarm");
drivers/hwmon/adt7x10.c:
	sysfs_notify(&dev->kobj, NULL, "temp1_crit_alarm");

drivers/hwmon/abx500.c:
	sysfs_notify(&data->pdev->dev.kobj, NULL, alarm_node);
drivers/hwmon/abx500.c:
	sysfs_notify(&data->pdev->dev.kobj, NULL, alarm_node);

drivers/hwmon/stts751.c:
	sysfs_notify(&priv->dev->kobj, NULL, "temp1_max_alarm");
drivers/hwmon/stts751.c:
	sysfs_notify(&priv->dev->kobj, NULL, "temp1_min_alarm");

There are also some other places I believe they are doing the same like:

drivers/md/md.c:
	sysfs_notify(&mddev->kobj, NULL, "sync_completed");
drivers/md/md.c:
	sysfs_notify(&mddev->kobj, NULL, "degraded");


-- 
<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

Re: [PATCH] thermal: core: Send a sysfs notification on trip points

[Vincent Whitchurch]

On Fri, Apr 03, 2020 at 05:26:39PM +0200, Daniel Lezcano wrote:
> On 03/04/2020 16:40, Vincent Whitchurch wrote:
> > Normally sysfs_notify() is used to notify userspace that the value
> > of the sysfs file has changed, but in this case it's being used on
> > a sysfs file whose value never changes.  I don't know if there are
> > other drivers that do something similar.
> 
> I think so:
> 
> eg.
> 
> drivers/hwmon/adt7x10.c:
> 	sysfs_notify(&dev->kobj, NULL, "temp1_max_alarm");
> drivers/hwmon/adt7x10.c:
> 	sysfs_notify(&dev->kobj, NULL, "temp1_min_alarm");
> drivers/hwmon/adt7x10.c:
> 	sysfs_notify(&dev->kobj, NULL, "temp1_crit_alarm");
> 
> drivers/hwmon/abx500.c:
> 	sysfs_notify(&data->pdev->dev.kobj, NULL, alarm_node);
> drivers/hwmon/abx500.c:
> 	sysfs_notify(&data->pdev->dev.kobj, NULL, alarm_node);
> 
> drivers/hwmon/stts751.c:
> 	sysfs_notify(&priv->dev->kobj, NULL, "temp1_max_alarm");
> drivers/hwmon/stts751.c:
> 	sysfs_notify(&priv->dev->kobj, NULL, "temp1_min_alarm");
> 
> There are also some other places I believe they are doing the same like:
> 
> drivers/md/md.c:
> 	sysfs_notify(&mddev->kobj, NULL, "sync_completed");
> drivers/md/md.c:
> 	sysfs_notify(&mddev->kobj, NULL, "degraded");

AFAICS all these drivers (including the hwmon ones) use sysfs_notify()
to notify that the value of the sysfs file has changed, unlike your
proposed patch.

Re: [PATCH] thermal: core: Send a sysfs notification on trip points

[Daniel Lezcano]

On 06/04/2020 09:45, Vincent Whitchurch wrote:
> On Fri, Apr 03, 2020 at 05:26:39PM +0200, Daniel Lezcano wrote:
>> On 03/04/2020 16:40, Vincent Whitchurch wrote:
>>> Normally sysfs_notify() is used to notify userspace that the
>>> value of the sysfs file has changed, but in this case it's
>>> being used on a sysfs file whose value never changes.  I don't
>>> know if there are other drivers that do something similar.
>>
>> I think so:
>>
>> eg.
>>
>> drivers/hwmon/adt7x10.c: sysfs_notify(&dev->kobj, NULL,
>> "temp1_max_alarm"); drivers/hwmon/adt7x10.c:
>> sysfs_notify(&dev->kobj, NULL, "temp1_min_alarm");
>> drivers/hwmon/adt7x10.c: sysfs_notify(&dev->kobj, NULL,
>> "temp1_crit_alarm");
>>
>> drivers/hwmon/abx500.c: sysfs_notify(&data->pdev->dev.kobj, NULL,
>> alarm_node); drivers/hwmon/abx500.c:
>> sysfs_notify(&data->pdev->dev.kobj, NULL, alarm_node);
>>
>> drivers/hwmon/stts751.c: sysfs_notify(&priv->dev->kobj, NULL,
>> "temp1_max_alarm"); drivers/hwmon/stts751.c:
>> sysfs_notify(&priv->dev->kobj, NULL, "temp1_min_alarm");
>>
>> There are also some other places I believe they are doing the
>> same like:
>>
>> drivers/md/md.c: sysfs_notify(&mddev->kobj, NULL,
>> "sync_completed"); drivers/md/md.c: sysfs_notify(&mddev->kobj,
>> NULL, "degraded");
>
> AFAICS all these drivers (including the hwmon ones) use
> sysfs_notify() to notify that the value of the sysfs file has
> changed, unlike your proposed patch.

Sorry, I don't have the same understanding:

drivers/hwmon/adt7x10.c:

 - receives an interrupt because one of the programmed temperature is
reached
 - reads the status to know which one and sends a sysfs notification

drivers/hwmon/stts751.c:

 - receives an I2C alert message, checks if it is a temperature alert
and then sends a sysfs notification

drivers/hwmon/abx500:

 - This one is probably sending a notification on a change

The documentation also is giving the semantic for sysfs_notify for
certain sysfs nodes:

Documentation/misc-devices/apds990x.txt:             sysfs_notify
called when threshold interrupt occurs
Documentation/misc-devices/bh1770glc.txt:            sysfs_notify
called when threshold interrupt occurs

AFAICT, it is a matter of documenting the notification for
trip_point_*_temp.


-- 
<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

Re: [PATCH] thermal: core: Send a sysfs notification on trip points

[Vincent Whitchurch]

On Mon, Apr 06, 2020 at 11:45:10AM +0200, Daniel Lezcano wrote:
> On 06/04/2020 09:45, Vincent Whitchurch wrote:
> > On Fri, Apr 03, 2020 at 05:26:39PM +0200, Daniel Lezcano wrote:
> >> On 03/04/2020 16:40, Vincent Whitchurch wrote:
> >>> Normally sysfs_notify() is used to notify userspace that the
> >>> value of the sysfs file has changed, but in this case it's
> >>> being used on a sysfs file whose value never changes.  I don't
> >>> know if there are other drivers that do something similar.
> >>
> >> I think so:
> >>
> >> eg.
> >>
> >> drivers/hwmon/adt7x10.c: sysfs_notify(&dev->kobj, NULL,
> >> "temp1_max_alarm"); drivers/hwmon/adt7x10.c:
> >> sysfs_notify(&dev->kobj, NULL, "temp1_min_alarm");
> >> drivers/hwmon/adt7x10.c: sysfs_notify(&dev->kobj, NULL,
> >> "temp1_crit_alarm");
> >>
> >> drivers/hwmon/abx500.c: sysfs_notify(&data->pdev->dev.kobj, NULL,
> >> alarm_node); drivers/hwmon/abx500.c:
> >> sysfs_notify(&data->pdev->dev.kobj, NULL, alarm_node);
> >>
> >> drivers/hwmon/stts751.c: sysfs_notify(&priv->dev->kobj, NULL,
> >> "temp1_max_alarm"); drivers/hwmon/stts751.c:
> >> sysfs_notify(&priv->dev->kobj, NULL, "temp1_min_alarm");
> >>
> >> There are also some other places I believe they are doing the
> >> same like:
> >>
> >> drivers/md/md.c: sysfs_notify(&mddev->kobj, NULL,
> >> "sync_completed"); drivers/md/md.c: sysfs_notify(&mddev->kobj,
> >> NULL, "degraded");
> >
> > AFAICS all these drivers (including the hwmon ones) use
> > sysfs_notify() to notify that the value of the sysfs file has
> > changed, unlike your proposed patch.
> 
> Sorry, I don't have the same understanding:
> 
> drivers/hwmon/adt7x10.c:
> 
>  - receives an interrupt because one of the programmed temperature is
> reached
>  - reads the status to know which one and sends a sysfs notification

In the sysfs file implementation, you can see that the value in the
sysfs file changes based on the same condition:

static ssize_t adt7x10_alarm_show(struct device *dev,
                                  struct device_attribute *da, char *buf)
{
        struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
        int ret;

        ret = adt7x10_read_byte(dev, ADT7X10_STATUS);
        if (ret < 0)
                return ret;

        return sprintf(buf, "%d\n", !!(ret & attr->index));
}

static SENSOR_DEVICE_ATTR_RO(temp1_max_alarm, adt7x10_alarm,
                             ADT7X10_STAT_T_HIGH);

It's the same case with the other examples: the sysfs file's value
changes.

Anyway, as you say it probably doesn't matter as long as it is
documented.

Re: [PATCH] thermal: core: Send a sysfs notification on trip points

[Amit Kucheria]

On Thu, Apr 2, 2020 at 7:53 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> Currently the userspace has no easy way to get notified when a
> specific trip point was crossed. There are a couple of approaches:
>
> - the userspace polls the sysfs temperature with usually an
>   unacceptable delay between the trip temperature point crossing and
>   the moment it is detected, or a high polling rate with an
>   unacceptable number of wakeup events.
>
> - the thermal zone is set to be managed by an userspace governor in
>   order to receive the uevent even if the thermal zone needs to be
>   managed by another governor.
>
> These changes allow to send a sysfs notification on the
> trip_point_*_temp when the temperature is getting higher than the trip
> point temperature. By this way, the userspace can be notified
> everytime when the trip point is crossed, this is useful for the
> thermal Android HAL or for notification to be sent via d-bus.
>
> That allows the userspace to manage the applications based on specific
> alerts on different thermal zones to mitigate the skin temperature,
> letting the kernel governors handle the high temperature for hardware
> like the CPU, the GPU or the modem.
>
> The temperature can be oscillating around a trip point and the event
> will be sent multiple times. It is up to the userspace to deal with
> this situation.

Thinking about this a bit more, userspace might want a notification
when the temperature reduces and crosses the threshold on its way down
too.

Currently, we're only sending the notification on the way up. How
should userspace know when to stop mitigation activity other than
constantly polling the TZ temperature?

> The following userspace program allows to monitor those events:
>
> struct trip_data {
>        int fd;
>        int temperature;
>        const char *path;
> };
>

[snip]

Re: [PATCH] thermal: core: Send a sysfs notification on trip points

[Daniel Lezcano]

On 06/04/2020 11:25, Amit Kucheria wrote:
> On Thu, Apr 2, 2020 at 7:53 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> Currently the userspace has no easy way to get notified when a
>> specific trip point was crossed. There are a couple of
>> approaches:
>>
>> - the userspace polls the sysfs temperature with usually an
>> unacceptable delay between the trip temperature point crossing
>> and the moment it is detected, or a high polling rate with an
>> unacceptable number of wakeup events.
>>
>> - the thermal zone is set to be managed by an userspace governor
>> in order to receive the uevent even if the thermal zone needs to
>> be managed by another governor.
>>
>> These changes allow to send a sysfs notification on the
>> trip_point_*_temp when the temperature is getting higher than the
>> trip point temperature. By this way, the userspace can be
>> notified everytime when the trip point is crossed, this is useful
>> for the thermal Android HAL or for notification to be sent via
>> d-bus.
>>
>> That allows the userspace to manage the applications based on
>> specific alerts on different thermal zones to mitigate the skin
>> temperature, letting the kernel governors handle the high
>> temperature for hardware like the CPU, the GPU or the modem.
>>
>> The temperature can be oscillating around a trip point and the
>> event will be sent multiple times. It is up to the userspace to
>> deal with this situation.
>
> Thinking about this a bit more, userspace might want a
> notification when the temperature reduces and crosses the threshold
> on its way down too.
>
> Currently, we're only sending the notification on the way up. How
> should userspace know when to stop mitigation activity other than
> constantly polling the TZ temperature?

Assuming you want to monitor the temperature after a specific trip
point is reached:

 - the notification is sent way up

 - user space starts reading the temperature

 - the temperature goes below the trip point temperature

 - user space stops reading the temperature

Actually, I don't see the point of being notified and not read the
temperature after.

That is how is working the kernel side, especially with the interrupt
mode. The sensor fires an interrupt -> thermal zone update ->
temperature read -> trip point reached -> passive mode on ->
temperature polling -> temperature below trip point -> passive mode off.



>> The following userspace program allows to monitor those events:
>>
>> struct trip_data { int fd; int temperature; const char *path; };
>>
>
> [snip]
>


-- 
<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