| Message ID | 20200402142116.22869-1-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Zhang Rui |
| Headers | show |
| Series | thermal: core: Send a sysfs notification on trip points | expand |
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.
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");
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.
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]
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.
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] >
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.
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
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(+)