Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211008081137.1948848-1-michal.vokac@ysoft.com> <20211018112820.qkebjt2gk2w53lp5@pengutronix.de>
In-Reply-To: <20211018112820.qkebjt2gk2w53lp5@pengutronix.de>
From:   Petr Benes <petrben@gmail.com>
Date:   Mon, 18 Oct 2021 13:41:48 +0200
Message-ID: <CAPwXO5Y8e8zpo_8zrfM=JFNkhKehE0mnpe8wzUzOQiifEZnx9Q@mail.gmail.com>
Subject: Re: [PATCH] thermal: imx: Fix temperature measurements on i.MX6 after alarm
To:     Oleksij Rempel <o.rempel@pengutronix.de>
Cc:     =?UTF-8?B?TWljaGFsIFZva8OhxI0=?= <michal.vokac@ysoft.com>,
        Andrzej Pietrasiewicz <andrzej.p@collabora.com>,
        linux-pm@vger.kernel.org,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Shawn Guo <shawnguo@kernel.org>,
        Amit Kucheria <amitk@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Fabio Estevam <festevam@gmail.com>,
        NXP Linux Team <linux-imx@nxp.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        =?UTF-8?Q?Petr_Bene=C5=A1?= <petr.benes@ysoft.com>,
        stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, 18 Oct 2021 at 13:28, Oleksij Rempel <o.rempel@pengutronix.de> wrot=
e:
>
> Hi Michal,
>
> I hope you have seen this patch:
> https://lore.kernel.org/all/20210924115032.29684-1-o.rempel@pengutronix.d=
e/
>
> Are there any reason why this was ignored?

Missed completely. It looks it addresses the problem more systematically.

>
> On Fri, Oct 08, 2021 at 10:11:37AM +0200, Michal Vok=C3=A1=C4=8D wrote:
> > From: Petr Bene=C5=A1 <petr.benes@ysoft.com>
> >
> > SoC temperature readout may not work after thermal alarm fires interrup=
t.
> > This harms userspace as well as CPU cooling device.
> >
> > Two issues with the logic involved. First, there is no protection again=
st
> > concurent measurements, hence one can switch the sensor off while
> > the other one tries to read temperature later. Second, the interrupt pa=
th
> > 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")
> >
> > 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 w=
hether to run a measurement")
> > Cc: petrben@gmail.com
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Petr Bene=C5=A1 <petr.benes@ysoft.com>
> > Signed-off-by: Michal Vok=C3=A1=C4=8D <michal.vokac@ysoft.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_therma=
l.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_devic=
e *tz, int *temp)
> >       const struct thermal_soc_data *soc_data =3D data->socdata;
> >       struct regmap *map =3D data->tempmon;
> >       unsigned int n_meas;
> > -     bool wait, run_measurement;
> > +     bool wait;
> >       u32 val;
> >
> > -     run_measurement =3D !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 =3D !(val & soc_data->temp_valid_mask);
> > @@ -283,13 +286,15 @@ static int imx_get_temp(struct thermal_zone_devic=
e *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) =3D=3D 0) {
> >               dev_dbg(&tz->device, "temp measurement never finished\n")=
;
> >               return -EAGAIN;
> > @@ -339,20 +344,26 @@ static int imx_change_mode(struct thermal_zone_de=
vice *tz,
> >       const struct thermal_soc_data *soc_data =3D data->socdata;
> >
> >       if (mode =3D=3D 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 =3D true;
> > +             mutex_unlock(&data->sensor_lock);
> >
> >               if (!data->irq_enabled) {
> >                       data->irq_enabled =3D 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 =3D 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 =3D false;
> > +     mutex_unlock(&data->sensor_lock);
> >
> >       ret =3D imx_thermal_register_legacy_cooling(data);
> >       if (ret)
> > @@ -796,10 +811,13 @@ static int imx_thermal_probe(struct platform_devi=
ce *pdev)
> >       if (data->socdata->version =3D=3D 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 =3D true;
> > +     mutex_unlock(&data->sensor_lock);
> >
> >       data->irq_enabled =3D true;
> >       ret =3D thermal_zone_device_enable(data->tz);
> > @@ -832,8 +850,12 @@ static int imx_thermal_remove(struct platform_devi=
ce *pdev)
> >       struct regmap *map =3D 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 =3D false;
> > +     mutex_unlock(&data->sensor_lock);
> > +
> >       if (!IS_ERR(data->thermal_clk))
> >               clk_disable_unprepare(data->thermal_clk);
> >
> > --
> > 2.25.1
> >