Message-ID: <e0c819ce-31f4-cee1-c7cc-7ecb73d374a3@linaro.org>
Date:   Fri, 3 Apr 2020 17:26:39 +0200
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Vincent Whitchurch <vincent.whitchurch@...s.com>
Cc:     "rui.zhang@...el.com" <rui.zhang@...el.com>,
        "amit.kucheria@...durent.com" <amit.kucheria@...durent.com>,
        "open list:THERMAL" <linux-pm@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] thermal: core: Send a sysfs notification on trip points

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

Powered by blists - more mailing lists