| Message ID | 1517595466-23465-1-git-send-email-festevam@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Zhang Rui |
| Headers | show |
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 >
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 >>
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
> -----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
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 > > >
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.
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?
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
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; }