On 26/01/2023 13:55, Thierry Reding wrote:
On Tue, Jan 24, 2023 at 08:57:23PM +0100, Daniel Lezcano wrote:
Hi,
does anyone know what is the purpose of the get_thermal_instance() usage in
this code:
https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/thermal/tegra/soctherm.c?h=thermal/linux-next#n623
The driver is using a function which is reserved for the thermal core. It
should not.
Is the following change ok ?
diff --git a/drivers/thermal/tegra/soctherm.c
b/drivers/thermal/tegra/soctherm.c
index 220873298d77..5f552402d987 100644
--- a/drivers/thermal/tegra/soctherm.c
+++ b/drivers/thermal/tegra/soctherm.c
@@ -620,9 +620,8 @@ static int tegra_thermctl_set_trip_temp(struct
thermal_zone_device *tz, int trip
continue;
cdev = ts->throt_cfgs[i].cdev;
- if (get_thermal_instance(tz, cdev, trip_id))
- stc = find_throttle_cfg_by_name(ts, cdev->type);
- else
+ stc = find_throttle_cfg_by_name(ts, cdev->type);
+ if (!stc)
continue;
return throttrip_program(dev, sg, stc, temp);
@@ -768,9 +767,9 @@ static int tegra_soctherm_set_hwtrips(struct device
*dev,
continue;
cdev = ts->throt_cfgs[i].cdev;
- if (get_thermal_instance(tz, cdev, trip))
- stc = find_throttle_cfg_by_name(ts, cdev->type);
- else
+
+ stc = find_throttle_cfg_by_name(ts, cdev->type);
+ if (!stc)
continue;
ret = throttrip_program(dev, sg, stc, temperature);
There's a small difference in behavior after applying this patch. Prior
to this I get (on Tegra210):
[ 12.354091] tegra_soctherm 700e2000.thermal-sensor: missing thermtrips, will use critical trips as shut down temp
[ 12.379009] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when cpu reaches 102500 mC
[ 12.388882] tegra_soctherm 700e2000.thermal-sensor: programming throttle for cpu to 102500
[ 12.401007] tegra_soctherm 700e2000.thermal-sensor: throttrip: will throttle when cpu reaches 102500 mC
[ 12.471041] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when gpu reaches 103000 mC
[ 12.482852] tegra_soctherm 700e2000.thermal-sensor: programming throttle for gpu to 103000
[ 12.482860] tegra_soctherm 700e2000.thermal-sensor: throttrip: will throttle when gpu reaches 103000 mC
[ 12.485357] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when pll reaches 103000 mC
[ 12.501774] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when mem reaches 103000 mC
and after these changes, it turns into:
[ 12.447113] tegra_soctherm 700e2000.thermal-sensor: missing thermtrips, will use critical trips as shut down temp
[ 12.472300] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when cpu reaches 102500 mC
[ 12.481789] tegra_soctherm 700e2000.thermal-sensor: programming throttle for cpu to 102500
[ 12.495447] tegra_soctherm 700e2000.thermal-sensor: throttrip: will throttle when cpu reaches 102500 mC
[ 12.496514] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when gpu reaches 103000 mC
[ 12.510353] tegra_soctherm 700e2000.thermal-sensor: programming throttle for gpu to 103000
[ 12.526856] tegra_soctherm 700e2000.thermal-sensor: throttrip: will throttle when gpu reaches 103000 mC
[ 12.528774] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when pll reaches 103000 mC
[ 12.569352] tegra_soctherm 700e2000.thermal-sensor: programming throttle for pll to 103000
[ 12.577635] tegra_soctherm 700e2000.thermal-sensor: throttrip: will throttle when pll reaches 103000 mC
[ 12.590952] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when mem reaches 103000 mC
[ 12.600783] tegra_soctherm 700e2000.thermal-sensor: programming throttle for mem to 103000
[ 12.609204] tegra_soctherm 700e2000.thermal-sensor: throttrip: will throttle when mem reaches 103000 mC
The "programming throttle ..." messages are something I've added locally
to trace what gets called. So it looks like for "pll" and "mem" thermal
zones, we now program trip points whereas we previously didn't.
Hmm, yeah. I did go into the details of the driver but if there is no
cooling device associated with a trip point it will result in a noop
from the thermal framework POV. The check is done in the governors by
going through the thermal zone device list and cdev.
I'll take a closer look to see if we can replace the calls to
get_thermal_instance() by something else.
That is great, thanks !
--
<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
