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