| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <df545947-2f5b-a355-859d-7f61eab14dcb@linaro.org>
Date: Sun, 17 Oct 2021 00:23:56 +0200
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Michal Vokáč <michal.vokac@...ft.com>,
Andrzej Pietrasiewicz <andrzej.p@...labora.com>,
linux-pm@...r.kernel.org, Shawn Guo <shawnguo@...nel.org>
Cc: Amit Kucheria <amitk@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>,
NXP Linux Team <linux-imx@....com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Petr Beneš <petr.benes@...ft.com>,
petrben@...il.com, stable@...r.kernel.org
Subject: Re: [PATCH] thermal: imx: Fix temperature measurements on i.MX6 after
alarm
On 08/10/2021 10:11, Michal Vokáč wrote:
> From: Petr Beneš <petr.benes@...ft.com>
>
> 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 ?
get_temp() and tz enable/disable are protected against races in the core
code via the tz mutex
> 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@...il.com
> Cc: stable@...r.kernel.org
> Signed-off-by: Petr Beneš <petr.benes@...ft.com>
> Signed-off-by: Michal Vokáč <michal.vokac@...ft.com>
> ---
> 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
Powered by blists - more mailing lists