[PATCH] thermal: imx: Fix race condition in imx_thermal_probe()

[PATCH] thermal: imx: Fix race condition in imx_thermal_probe()

[Fabio Estevam]

From: Mikhail Lappo <mikhail.lappo@esrlabs.com>

When device boots with T > T_trip_1 and requests interrupt,
the race condition takes place. The interrupt comes before
THERMAL_DEVICE_ENABLED is set. This leads to an attempt to
reading sensor value from irq and disabling the sensor, based on
the data->mode field, which expected to be THERMAL_DEVICE_ENABLED,
but still stays as THERMAL_DEVICE_DISABLED. Afher this issue
sensor is never re-enabled, as the driver state is wrong.

Fix this problem by setting the 'data' members prior to
requesting the interrupts.

Fixes: 37713a1e8e4c ("thermal: imx: implement thermal alarm interrupt handling")
Cc: <stable@vger.kernel.org>
Signed-off-by: Mikhail Lappo <mikhail.lappo@esrlabs.com>
Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
 drivers/thermal/imx_thermal.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index a67781b..ee3a215 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -637,6 +637,9 @@ static int imx_thermal_probe(struct platform_device *pdev)
 	regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_POWER_DOWN);
 	regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_MEASURE_TEMP);
 
+	data->irq_enabled = true;
+	data->mode = THERMAL_DEVICE_ENABLED;
+
 	ret = devm_request_threaded_irq(&pdev->dev, data->irq,
 			imx_thermal_alarm_irq, imx_thermal_alarm_irq_thread,
 			0, "imx_thermal", data);
@@ -649,9 +652,6 @@ static int imx_thermal_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	data->irq_enabled = true;
-	data->mode = THERMAL_DEVICE_ENABLED;
-
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH] thermal: imx: Fix race condition in imx_thermal_probe()

[Fabio Estevam]

Hi Eduardo and Rui,

On Fri, Feb 2, 2018 at 4:17 PM, Fabio Estevam <festevam@gmail.com> wrote:
> From: Mikhail Lappo <mikhail.lappo@esrlabs.com>
>
> When device boots with T > T_trip_1 and requests interrupt,
> the race condition takes place. The interrupt comes before
> THERMAL_DEVICE_ENABLED is set. This leads to an attempt to
> reading sensor value from irq and disabling the sensor, based on
> the data->mode field, which expected to be THERMAL_DEVICE_ENABLED,
> but still stays as THERMAL_DEVICE_DISABLED. Afher this issue
> sensor is never re-enabled, as the driver state is wrong.
>
> Fix this problem by setting the 'data' members prior to
> requesting the interrupts.
>
> Fixes: 37713a1e8e4c ("thermal: imx: implement thermal alarm interrupt handling")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Mikhail Lappo <mikhail.lappo@esrlabs.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>

A gentle ping on this patch.

> ---
>  drivers/thermal/imx_thermal.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index a67781b..ee3a215 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -637,6 +637,9 @@ static int imx_thermal_probe(struct platform_device *pdev)
>         regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_POWER_DOWN);
>         regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_MEASURE_TEMP);
>
> +       data->irq_enabled = true;
> +       data->mode = THERMAL_DEVICE_ENABLED;
> +
>         ret = devm_request_threaded_irq(&pdev->dev, data->irq,
>                         imx_thermal_alarm_irq, imx_thermal_alarm_irq_thread,
>                         0, "imx_thermal", data);
> @@ -649,9 +652,6 @@ static int imx_thermal_probe(struct platform_device *pdev)
>                 return ret;
>         }
>
> -       data->irq_enabled = true;
> -       data->mode = THERMAL_DEVICE_ENABLED;
> -
>         return 0;
>  }
>
> --
> 2.7.4
>

Re: [PATCH] thermal: imx: Fix race condition in imx_thermal_probe()

[Fabio Estevam]

Rui,

Any comments, please?

On Wed, Feb 14, 2018 at 1:23 AM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Eduardo and Rui,
>
> On Fri, Feb 2, 2018 at 4:17 PM, Fabio Estevam <festevam@gmail.com> wrote:
>> From: Mikhail Lappo <mikhail.lappo@esrlabs.com>
>>
>> When device boots with T > T_trip_1 and requests interrupt,
>> the race condition takes place. The interrupt comes before
>> THERMAL_DEVICE_ENABLED is set. This leads to an attempt to
>> reading sensor value from irq and disabling the sensor, based on
>> the data->mode field, which expected to be THERMAL_DEVICE_ENABLED,
>> but still stays as THERMAL_DEVICE_DISABLED. Afher this issue
>> sensor is never re-enabled, as the driver state is wrong.
>>
>> Fix this problem by setting the 'data' members prior to
>> requesting the interrupts.
>>
>> Fixes: 37713a1e8e4c ("thermal: imx: implement thermal alarm interrupt handling")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Mikhail Lappo <mikhail.lappo@esrlabs.com>
>> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
>
> A gentle ping on this patch.
>
>> ---
>>  drivers/thermal/imx_thermal.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
>> index a67781b..ee3a215 100644
>> --- a/drivers/thermal/imx_thermal.c
>> +++ b/drivers/thermal/imx_thermal.c
>> @@ -637,6 +637,9 @@ static int imx_thermal_probe(struct platform_device *pdev)
>>         regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_POWER_DOWN);
>>         regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_MEASURE_TEMP);
>>
>> +       data->irq_enabled = true;
>> +       data->mode = THERMAL_DEVICE_ENABLED;
>> +
>>         ret = devm_request_threaded_irq(&pdev->dev, data->irq,
>>                         imx_thermal_alarm_irq, imx_thermal_alarm_irq_thread,
>>                         0, "imx_thermal", data);
>> @@ -649,9 +652,6 @@ static int imx_thermal_probe(struct platform_device *pdev)
>>                 return ret;
>>         }
>>
>> -       data->irq_enabled = true;
>> -       data->mode = THERMAL_DEVICE_ENABLED;
>> -
>>         return 0;
>>  }
>>
>> --
>> 2.7.4
>>

Re: [PATCH] thermal: imx: Fix race condition in imx_thermal_probe()

[Zhang Rui]

On Mon, 2018-02-26 at 15:56 -0300, Fabio Estevam wrote:
> Rui,
> 
> Any comments, please?
> 
interesting, this problem should be covered by this patch https://patch
work.kernel.org/patch/10242483/, together with some driver code change.

anyway, as that is not a complete solution yet, I will apply this patch
as a quick solution.

thanks,
rui
> On Wed, Feb 14, 2018 at 1:23 AM, Fabio Estevam <festevam@gmail.com>
> wrote:
> > 
> > Hi Eduardo and Rui,
> > 
> > On Fri, Feb 2, 2018 at 4:17 PM, Fabio Estevam <festevam@gmail.com>
> > wrote:
> > > 
> > > From: Mikhail Lappo <mikhail.lappo@esrlabs.com>
> > > 
> > > When device boots with T > T_trip_1 and requests interrupt,
> > > the race condition takes place. The interrupt comes before
> > > THERMAL_DEVICE_ENABLED is set. This leads to an attempt to
> > > reading sensor value from irq and disabling the sensor, based on
> > > the data->mode field, which expected to be
> > > THERMAL_DEVICE_ENABLED,
> > > but still stays as THERMAL_DEVICE_DISABLED. Afher this issue
> > > sensor is never re-enabled, as the driver state is wrong.
> > > 
> > > Fix this problem by setting the 'data' members prior to
> > > requesting the interrupts.
> > > 
> > > Fixes: 37713a1e8e4c ("thermal: imx: implement thermal alarm
> > > interrupt handling")
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Mikhail Lappo <mikhail.lappo@esrlabs.com>
> > > Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> > A gentle ping on this patch.
> > 
> > > 
> > > ---
> > >  drivers/thermal/imx_thermal.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/thermal/imx_thermal.c
> > > b/drivers/thermal/imx_thermal.c
> > > index a67781b..ee3a215 100644
> > > --- a/drivers/thermal/imx_thermal.c
> > > +++ b/drivers/thermal/imx_thermal.c
> > > @@ -637,6 +637,9 @@ static int imx_thermal_probe(struct
> > > platform_device *pdev)
> > >         regmap_write(map, TEMPSENSE0 + REG_CLR,
> > > TEMPSENSE0_POWER_DOWN);
> > >         regmap_write(map, TEMPSENSE0 + REG_SET,
> > > TEMPSENSE0_MEASURE_TEMP);
> > > 
> > > +       data->irq_enabled = true;
> > > +       data->mode = THERMAL_DEVICE_ENABLED;
> > > +
> > >         ret = devm_request_threaded_irq(&pdev->dev, data->irq,
> > >                         imx_thermal_alarm_irq,
> > > imx_thermal_alarm_irq_thread,
> > >                         0, "imx_thermal", data);
> > > @@ -649,9 +652,6 @@ static int imx_thermal_probe(struct
> > > platform_device *pdev)
> > >                 return ret;
> > >         }
> > > 
> > > -       data->irq_enabled = true;
> > > -       data->mode = THERMAL_DEVICE_ENABLED;
> > > -
> > >         return 0;
> > >  }
> > > 
> > > --
> > > 2.7.4
> > > 

Re: [PATCH] thermal: imx: Fix race condition in imx_thermal_probe()

[Fabio Estevam]

Hi Rui,

On Wed, Feb 28, 2018 at 1:42 AM, Zhang Rui <rui.zhang@intel.com> wrote:

> interesting, this problem should be covered by this patch https://patch
> work.kernel.org/patch/10242483/, together with some driver code change.
>
> anyway, as that is not a complete solution yet, I will apply this patch
> as a quick solution.

Thanks, but I still don't see this one in your tree.

Re: [PATCH] thermal: imx: Fix race condition in imx_thermal_probe()

[Fabio Estevam]

Hi Rui,

On Wed, Mar 7, 2018 at 4:55 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Rui,
>
> On Wed, Feb 28, 2018 at 1:42 AM, Zhang Rui <rui.zhang@intel.com> wrote:
>
>> interesting, this problem should be covered by this patch https://patch
>> work.kernel.org/patch/10242483/, together with some driver code change.
>>
>> anyway, as that is not a complete solution yet, I will apply this patch
>> as a quick solution.
>
> Thanks, but I still don't see this one in your tree.

Sorry to bother you, but is it possible to get this one to 4.17, please?

Re: [PATCH] thermal: imx: Fix race condition in imx_thermal_probe()

[Zhang Rui]

On 二, 2018-03-20 at 10:13 -0300, Fabio Estevam wrote:
> Hi Rui,
> 
> On Wed, Mar 7, 2018 at 4:55 PM, Fabio Estevam <festevam@gmail.com>
> wrote:
> > 
> > Hi Rui,
> > 
> > On Wed, Feb 28, 2018 at 1:42 AM, Zhang Rui <rui.zhang@intel.com>
> > wrote:
> > 
> > > 
> > > interesting, this problem should be covered by this patch https:/
> > > /patch
> > > work.kernel.org/patch/10242483/, together with some driver code
> > > change.
> > > 
> > > anyway, as that is not a complete solution yet, I will apply this
> > > patch
> > > as a quick solution.
> > Thanks, but I still don't see this one in your tree.
> Sorry to bother you, but is it possible to get this one to 4.17,
> please?

yes, it's in my tree now.

thanks,
rui

Re: [PATCH] thermal: imx: Fix race condition in imx_thermal_probe()

[Philipp Zabel]

On Fri, 2018-02-02 at 16:17 -0200, Fabio Estevam wrote:
> From: Mikhail Lappo <mikhail.lappo@esrlabs.com>
> 
> When device boots with T > T_trip_1 and requests interrupt,
> the race condition takes place. The interrupt comes before
> THERMAL_DEVICE_ENABLED is set. This leads to an attempt to
> reading sensor value from irq and disabling the sensor, based on
> the data->mode field, which expected to be THERMAL_DEVICE_ENABLED,
> but still stays as THERMAL_DEVICE_DISABLED. Afher this issue
> sensor is never re-enabled, as the driver state is wrong.
> 
> Fix this problem by setting the 'data' members prior to
> requesting the interrupts.
> 
> Fixes: 37713a1e8e4c ("thermal: imx: implement thermal alarm interrupt handling")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Mikhail Lappo <mikhail.lappo@esrlabs.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

RE: [PATCH] thermal: imx: Fix race condition in imx_thermal_probe()

[A.s. Dong]

> -----Original Message-----
> From: Fabio Estevam [mailto:festevam@gmail.com]
> Sent: Saturday, February 3, 2018 2:18 AM
> To: rui.zhang@intel.com
> Cc: edubezval@gmail.com; mikhail.lappo@esrlabs.com; linux-
> pm@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> p.zabel@pengutronix.de; shawnguo@kernel.org; stable@vger.kernel.org;
> Fabio Estevam <fabio.estevam@nxp.com>
> Subject: [PATCH] thermal: imx: Fix race condition in imx_thermal_probe()
> 
> From: Mikhail Lappo <mikhail.lappo@esrlabs.com>
> 
> When device boots with T > T_trip_1 and requests interrupt, the race
> condition takes place. The interrupt comes before
> THERMAL_DEVICE_ENABLED is set. This leads to an attempt to reading
> sensor value from irq and disabling the sensor, based on the data->mode
> field, which expected to be THERMAL_DEVICE_ENABLED, but still stays as
> THERMAL_DEVICE_DISABLED. Afher this issue sensor is never re-enabled, as
> the driver state is wrong.
> 
> Fix this problem by setting the 'data' members prior to requesting the
> interrupts.
> 
> Fixes: 37713a1e8e4c ("thermal: imx: implement thermal alarm interrupt
> handling")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Mikhail Lappo <mikhail.lappo@esrlabs.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>

Looks reasonable to me.
Acked-by: Dong Aisheng <aisheng.dong@nxp.com>

Regards
Dong Aisheng

> ---
>  drivers/thermal/imx_thermal.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index a67781b..ee3a215 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -637,6 +637,9 @@ static int imx_thermal_probe(struct platform_device
> *pdev)
>  	regmap_write(map, TEMPSENSE0 + REG_CLR,
> TEMPSENSE0_POWER_DOWN);
>  	regmap_write(map, TEMPSENSE0 + REG_SET,
> TEMPSENSE0_MEASURE_TEMP);
> 
> +	data->irq_enabled = true;
> +	data->mode = THERMAL_DEVICE_ENABLED;
> +
>  	ret = devm_request_threaded_irq(&pdev->dev, data->irq,
>  			imx_thermal_alarm_irq,
> imx_thermal_alarm_irq_thread,
>  			0, "imx_thermal", data);
> @@ -649,9 +652,6 @@ static int imx_thermal_probe(struct platform_device
> *pdev)
>  		return ret;
>  	}
> 
> -	data->irq_enabled = true;
> -	data->mode = THERMAL_DEVICE_ENABLED;
> -
>  	return 0;
>  }
> 
> --
> 2.7.4