On 18/10/2021 10:36, Petr Benes wrote: > On Sun, 17 Oct 2021 at 00:23, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote: >> >> On 08/10/2021 10:11, Michal Vokáč wrote: >>> From: Petr Beneš <petr.benes@xxxxxxxxx> >>> >>> SoC temperature readout may not work after thermal alarm fires interrupt. >>> This harms userspace as well as CPU cooling device. >>> >>> Two issues with the logic involved. First, there is no protection against >>> concurent measurements, hence one can switch the sensor off while >>> the other one tries to read temperature later. Second, the interrupt path >>> usually fails. At the end the sensor is powered off and thermal IRQ is >>> disabled. One has to reenable the thermal zone by the sysfs interface. >>> >>> Most of troubles come from commit d92ed2c9d3ff ("thermal: imx: Use >>> driver's local data to decide whether to run a measurement") >> >> Are these troubles observed and reproduced ? Or is it your understanding >> from reading the code ? > > Yes, it is observed. We are using: > CONFIG_CPU_THERMAL=y > CONFIG_CPU_FREQ_THERMAL=y > If the SoC temperature oscillates around the passive trip point, it is not > possible to read out /sys/class/thermal/thermal_zone0/temp after > a while (minutes). For reproduction either heat the device up a bit or set > the passive trip point lower. Ok, I have an i.MX7 here to resurrect. I'll give a try. >> get_temp() and tz enable/disable are protected against races in the core >> code via the tz mutex > > imx_get_temp() itself doesn't handle concurrent invocations properly, despite > it may seem it does. The sensor is switched on/off without control. > That is a flaw in imx_get_temp(). > No evidence the core locking is wrong. > > data->irq_enabled is set to false in imx_thermal_alarm_irq() each time > the interrupt arrives. imx_get_temp() does wrong decision based > on the value later. > >> >>> It uses data->irq_enabled as the "local data". Indeed, its value is >>> related to the state of the sensor loosely under normal operation and, >>> frankly, gets unleashed when the thermal interrupt arrives. >>> >>> Current patch adds the "local data" (new member sensor_on in >>> imx_thermal_data) and sets its value in controlled manner.> >>> Fixes: d92ed2c9d3ff ("thermal: imx: Use driver's local data to decide whether to run a measurement") >>> Cc: petrben@xxxxxxxxx >>> Cc: stable@xxxxxxxxxxxxxxx >>> Signed-off-by: Petr Beneš <petr.benes@xxxxxxxxx> >>> Signed-off-by: Michal Vokáč <michal.vokac@xxxxxxxxx> >>> --- >>> drivers/thermal/imx_thermal.c | 30 ++++++++++++++++++++++++++---- >>> 1 file changed, 26 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c >>> index 2c7473d86a59..df5658e21828 100644 >>> --- a/drivers/thermal/imx_thermal.c >>> +++ b/drivers/thermal/imx_thermal.c >>> @@ -209,6 +209,8 @@ struct imx_thermal_data { >>> struct clk *thermal_clk; >>> const struct thermal_soc_data *socdata; >>> const char *temp_grade; >>> + struct mutex sensor_lock; >>> + bool sensor_on; >>> }; >>> >>> static void imx_set_panic_temp(struct imx_thermal_data *data, >>> @@ -252,11 +254,12 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp) >>> const struct thermal_soc_data *soc_data = data->socdata; >>> struct regmap *map = data->tempmon; >>> unsigned int n_meas; >>> - bool wait, run_measurement; >>> + bool wait; >>> u32 val; >>> >>> - run_measurement = !data->irq_enabled; >>> - if (!run_measurement) { >>> + mutex_lock(&data->sensor_lock); >>> + >>> + if (data->sensor_on) { >>> /* Check if a measurement is currently in progress */ >>> regmap_read(map, soc_data->temp_data, &val); >>> wait = !(val & soc_data->temp_valid_mask); >>> @@ -283,13 +286,15 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp) >>> >>> regmap_read(map, soc_data->temp_data, &val); >>> >>> - if (run_measurement) { >>> + if (!data->sensor_on) { >>> regmap_write(map, soc_data->sensor_ctrl + REG_CLR, >>> soc_data->measure_temp_mask); >>> regmap_write(map, soc_data->sensor_ctrl + REG_SET, >>> soc_data->power_down_mask); >>> } >>> >>> + mutex_unlock(&data->sensor_lock); >>> + >>> if ((val & soc_data->temp_valid_mask) == 0) { >>> dev_dbg(&tz->device, "temp measurement never finished\n"); >>> return -EAGAIN; >>> @@ -339,20 +344,26 @@ static int imx_change_mode(struct thermal_zone_device *tz, >>> const struct thermal_soc_data *soc_data = data->socdata; >>> >>> if (mode == THERMAL_DEVICE_ENABLED) { >>> + mutex_lock(&data->sensor_lock); >>> regmap_write(map, soc_data->sensor_ctrl + REG_CLR, >>> soc_data->power_down_mask); >>> regmap_write(map, soc_data->sensor_ctrl + REG_SET, >>> soc_data->measure_temp_mask); >>> + data->sensor_on = true; >>> + mutex_unlock(&data->sensor_lock); >>> >>> if (!data->irq_enabled) { >>> data->irq_enabled = true; >>> enable_irq(data->irq); >>> } >>> } else { >>> + mutex_lock(&data->sensor_lock); >>> regmap_write(map, soc_data->sensor_ctrl + REG_CLR, >>> soc_data->measure_temp_mask); >>> regmap_write(map, soc_data->sensor_ctrl + REG_SET, >>> soc_data->power_down_mask); >>> + data->sensor_on = false; >>> + mutex_unlock(&data->sensor_lock); >>> >>> if (data->irq_enabled) { >>> disable_irq(data->irq); >>> @@ -728,6 +739,8 @@ static int imx_thermal_probe(struct platform_device *pdev) >>> } >>> >>> /* Make sure sensor is in known good state for measurements */ >>> + mutex_init(&data->sensor_lock); >>> + mutex_lock(&data->sensor_lock); >>> regmap_write(map, data->socdata->sensor_ctrl + REG_CLR, >>> data->socdata->power_down_mask); >>> regmap_write(map, data->socdata->sensor_ctrl + REG_CLR, >>> @@ -739,6 +752,8 @@ static int imx_thermal_probe(struct platform_device *pdev) >>> IMX6_MISC0_REFTOP_SELBIASOFF); >>> regmap_write(map, data->socdata->sensor_ctrl + REG_SET, >>> data->socdata->power_down_mask); >>> + data->sensor_on = false; >>> + mutex_unlock(&data->sensor_lock); >>> >>> ret = imx_thermal_register_legacy_cooling(data); >>> if (ret) >>> @@ -796,10 +811,13 @@ static int imx_thermal_probe(struct platform_device *pdev) >>> if (data->socdata->version == TEMPMON_IMX6SX) >>> imx_set_panic_temp(data, data->temp_critical); >>> >>> + mutex_lock(&data->sensor_lock); >>> regmap_write(map, data->socdata->sensor_ctrl + REG_CLR, >>> data->socdata->power_down_mask); >>> regmap_write(map, data->socdata->sensor_ctrl + REG_SET, >>> data->socdata->measure_temp_mask); >>> + data->sensor_on = true; >>> + mutex_unlock(&data->sensor_lock); >>> >>> data->irq_enabled = true; >>> ret = thermal_zone_device_enable(data->tz); >>> @@ -832,8 +850,12 @@ static int imx_thermal_remove(struct platform_device *pdev) >>> struct regmap *map = data->tempmon; >>> >>> /* Disable measurements */ >>> + mutex_lock(&data->sensor_lock); >>> regmap_write(map, data->socdata->sensor_ctrl + REG_SET, >>> data->socdata->power_down_mask); >>> + data->sensor_on = false; >>> + mutex_unlock(&data->sensor_lock); >>> + >>> if (!IS_ERR(data->thermal_clk)) >>> clk_disable_unprepare(data->thermal_clk); >>> >>> >> >> >> -- >> <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 -- <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