[PATCH] thermal: core: ignore invalid trip temperature

[PATCH] thermal: core: ignore invalid trip temperature

[Srinivas Pandruvada]

Ignore invalid trip temperature less or equal to zero. Some
buggy systems have invalid trips, causing system shutdown.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Acked-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/thermal_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 9bf10aa..fbf301a 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -368,7 +368,7 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
 	tz->ops->get_trip_temp(tz, trip, &trip_temp);
 
 	/* If we have not crossed the trip_temp, we do not care. */
-	if (tz->temperature < trip_temp)
+	if (trip_temp <= 0 || tz->temperature < trip_temp)
 		return;
 
 	trace_thermal_zone_trip(tz, trip, trip_type);
-- 
1.9.1

Re: [PATCH] thermal: core: ignore invalid trip temperature

[Lukasz Majewski]

Hi Srinivas,

> Ignore invalid trip temperature less or equal to zero. Some
> buggy systems have invalid trips, causing system shutdown.
> 
> Signed-off-by: Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com> Acked-by: Zhang Rui
> <rui.zhang@intel.com> ---
>  drivers/thermal/thermal_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/thermal_core.c
> b/drivers/thermal/thermal_core.c index 9bf10aa..fbf301a 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -368,7 +368,7 @@ static void handle_critical_trips(struct
> thermal_zone_device *tz, tz->ops->get_trip_temp(tz, trip, &trip_temp);
>  
>  	/* If we have not crossed the trip_temp, we do not care. */
> -	if (tz->temperature < trip_temp)
> +	if (trip_temp <= 0 || tz->temperature < trip_temp)

To be honest, I regard this as a feature :-), not bug.

In this way I know that on some systems the regulator for thermal unit
is not enabled (which results in read temp of 0xFF).

I'd prefer to keep this as is.

>  		return;
>  
>  	trace_thermal_zone_trip(tz, trip, trip_type);



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH] thermal: core: ignore invalid trip temperature

[Srinivas Pandruvada]

On Fri, 2014-11-14 at 12:50 +0100, Lukasz Majewski wrote:
> Hi Srinivas,
> 
> > Hi Lukasz,
> > 
> > Thanks for your review and comment.
> > 
> > On Thu, 2014-11-13 at 10:23 +0100, Lukasz Majewski wrote:
> > > Hi Srinivas,
> > > 
> > > > Ignore invalid trip temperature less or equal to zero. Some
> > > > buggy systems have invalid trips, causing system shutdown.
> > > > 
> > > > Signed-off-by: Srinivas Pandruvada
> > > > <srinivas.pandruvada@linux.intel.com> Acked-by: Zhang Rui
> > > > <rui.zhang@intel.com> ---
> > > >  drivers/thermal/thermal_core.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/thermal/thermal_core.c
> > > > b/drivers/thermal/thermal_core.c index 9bf10aa..fbf301a 100644
> > > > --- a/drivers/thermal/thermal_core.c
> > > > +++ b/drivers/thermal/thermal_core.c
> > > > @@ -368,7 +368,7 @@ static void handle_critical_trips(struct
> > > > thermal_zone_device *tz, tz->ops->get_trip_temp(tz, trip,
> > > > &trip_temp); 
> > > >  	/* If we have not crossed the trip_temp, we do not care.
> > > > */
> > > > -	if (tz->temperature < trip_temp)
> > > > +	if (trip_temp <= 0 || tz->temperature < trip_temp)
> > > 
> > > To be honest, I regard this as a feature :-), not bug.
> > 
> > I am not saying bug is in the code, saying buggy system with invalid
> > configuration of critical trip.
> > 
> > > In this way I know that on some systems the regulator for thermal
> > > unit is not enabled (which results in read temp of 0xFF).
> > > 
> > So are you saying that trip temp of <= 0 is a valid trip in your
> > system?
> 
> No. Please find below explanation.
> 
> I have 4 available trip points in my system (e.g. Eynos4/5). The last
> trip point is critical (e.g. 120 C degrees).
So this change will still work for you, since your trip > 0.
> 
> When everything is properly configured (i.e. power supply is provided
> to thermal unit, clocks are setup) and we pass the critical trip point
> then interrupt is triggered and my board is shut down unconditionally by
> internal SoC's power management IP block.
> 
> However, when something is misconfigured, wrong value of temperature is
> read (max possible value - 0xFF) and passed to the driver.
> 
> In such a situation, I see that something is wrong, since I cannot fully
> boot up my system.
> 
> I don't mind the patch, but please pay a note that on some systems
> value of -1 is also expected.
If critical trip is -1, then this change will simply return as no need
to evaluate temp for trip for a given temp change.

I am forwarding this to linux-pm list, let me see if others have a
problem.

Thanks,
Srinivas

> 
> > 
> > Thanks,
> > Srinivas
> > 
> > > I'd prefer to keep this as is.
> > > 
> > > >  		return;
> > > >  
> > > >  	trace_thermal_zone_trip(tz, trip, trip_type);
> > > 
> > > 
> > > 
> > 
> > 
> 
> 
> 

Re: [PATCH] thermal: core: ignore invalid trip temperature

[Zhang Rui]

For some reason, I did not see the discussion between Lukasz and you via
email. I can only see it via patchwork.

Lukasz,

if the regulator for thermal unit is not enabled, what will you get?
temperature 0xFF + trip point -1? or Just temperature 0xFF?
I don't think this patch makes any difference in the second case.

BTW, if you expect some indicator when the thermal unit is not enabled,
system critical shutdown is not a proper one, we can either check the
sysfs I/F, and we can add a warning message here, telling that invalid
trip point is found.

thanks,
rui

On Wed, 2014-11-12 at 15:43 -0800, Srinivas Pandruvada wrote:
> Ignore invalid trip temperature less or equal to zero. Some
> buggy systems have invalid trips, causing system shutdown.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Acked-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/thermal/thermal_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 9bf10aa..fbf301a 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -368,7 +368,7 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
>  	tz->ops->get_trip_temp(tz, trip, &trip_temp);
>  
>  	/* If we have not crossed the trip_temp, we do not care. */
> -	if (tz->temperature < trip_temp)
> +	if (trip_temp <= 0 || tz->temperature < trip_temp)
>  		return;
>  
>  	trace_thermal_zone_trip(tz, trip, trip_type);

Re: [PATCH] thermal: core: ignore invalid trip temperature

[Lukasz Majewski]

Hi Zhang,

> 
> For some reason, I did not see the discussion between Lukasz and you
> via email. I can only see it via patchwork.

It is strange. However I'd appreciate to be in CC of this e-mail :-)

> 
> Lukasz,
> 
> if the regulator for thermal unit is not enabled, what will you get?
> temperature 0xFF + trip point -1? or Just temperature 0xFF?

Just 0xFF temperature.

Since 0xFF is larger than SW_TRIP point (mapped to
THERMAL_TRIP_CRITICAL), the code at handle_critical_trips() is executed.

>From my standpoint 0xFF is a possible and valid temperature in Exynos.

Srinivas, what is your error/use case that you need this check?

> I don't think this patch makes any difference in the second case.
> 
> BTW, if you expect some indicator when the thermal unit is not
> enabled, 

Actually, the TMU is enabled and configured, 
Lack of proper regulator (vtmu) for TMU is the culprit of this
situation.

> system critical shutdown is not a proper one, we can either
> check the sysfs I/F, and we can add a warning message here, telling
> that invalid trip point is found.

I think that, it would be a good idea to abort Exynos TMU probe when
"vtmu" regulator is not found.

> 
> thanks,
> rui
> 
> On Wed, 2014-11-12 at 15:43 -0800, Srinivas Pandruvada wrote:
> > Ignore invalid trip temperature less or equal to zero. Some
> > buggy systems have invalid trips, causing system shutdown.
> > 
> > Signed-off-by: Srinivas Pandruvada
> > <srinivas.pandruvada@linux.intel.com> Acked-by: Zhang Rui
> > <rui.zhang@intel.com> ---
> >  drivers/thermal/thermal_core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/thermal/thermal_core.c
> > b/drivers/thermal/thermal_core.c index 9bf10aa..fbf301a 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -368,7 +368,7 @@ static void handle_critical_trips(struct
> > thermal_zone_device *tz, tz->ops->get_trip_temp(tz, trip,
> > &trip_temp); 
> >  	/* If we have not crossed the trip_temp, we do not care. */
> > -	if (tz->temperature < trip_temp)
> > +	if (trip_temp <= 0 || tz->temperature < trip_temp)
> >  		return;
> >  
> >  	trace_thermal_zone_trip(tz, trip, trip_type);
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH] thermal: core: ignore invalid trip temperature

[Srinivas Pandruvada]

On Thu, 2014-11-20 at 11:25 +0100, Lukasz Majewski wrote: 
> Hi Zhang,
> 
> > 
> > For some reason, I did not see the discussion between Lukasz and you
> > via email. I can only see it via patchwork.
> 
> It is strange. However I'd appreciate to be in CC of this e-mail :-)
> 
> > 
> > Lukasz,
> > 
> > if the regulator for thermal unit is not enabled, what will you get?
> > temperature 0xFF + trip point -1? or Just temperature 0xFF?
> 
> Just 0xFF temperature.
> 
> Since 0xFF is larger than SW_TRIP point (mapped to
> THERMAL_TRIP_CRITICAL), the code at handle_critical_trips() is executed.
> 
> From my standpoint 0xFF is a possible and valid temperature in Exynos.
> 
> Srinivas, what is your error/use case that you need this check?
critical trip point < 0 because buggy FW and temp read is more than trip
temp.

Thanks,
Srinivas 
> 
> > I don't think this patch makes any difference in the second case.
> > 
> > BTW, if you expect some indicator when the thermal unit is not
> > enabled, 
> 
> Actually, the TMU is enabled and configured, 
> Lack of proper regulator (vtmu) for TMU is the culprit of this
> situation.
> 
> > system critical shutdown is not a proper one, we can either
> > check the sysfs I/F, and we can add a warning message here, telling
> > that invalid trip point is found.
> 
> I think that, it would be a good idea to abort Exynos TMU probe when
> "vtmu" regulator is not found.
> 
> > 
> > thanks,
> > rui
> > 
> > On Wed, 2014-11-12 at 15:43 -0800, Srinivas Pandruvada wrote:
> > > Ignore invalid trip temperature less or equal to zero. Some
> > > buggy systems have invalid trips, causing system shutdown.
> > > 
> > > Signed-off-by: Srinivas Pandruvada
> > > <srinivas.pandruvada@linux.intel.com> Acked-by: Zhang Rui
> > > <rui.zhang@intel.com> ---
> > >  drivers/thermal/thermal_core.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/thermal/thermal_core.c
> > > b/drivers/thermal/thermal_core.c index 9bf10aa..fbf301a 100644
> > > --- a/drivers/thermal/thermal_core.c
> > > +++ b/drivers/thermal/thermal_core.c
> > > @@ -368,7 +368,7 @@ static void handle_critical_trips(struct
> > > thermal_zone_device *tz, tz->ops->get_trip_temp(tz, trip,
> > > &trip_temp); 
> > >  	/* If we have not crossed the trip_temp, we do not care. */
> > > -	if (tz->temperature < trip_temp)
> > > +	if (trip_temp <= 0 || tz->temperature < trip_temp)
> > >  		return;
> > >  
> > >  	trace_thermal_zone_trip(tz, trip, trip_type);
> > 
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> 
> 

Re: [PATCH] thermal: core: ignore invalid trip temperature

[Zhang Rui]

On Thu, 2014-11-20 at 11:25 +0100, Lukasz Majewski wrote:
> Hi Zhang,
> 
> > 
> > For some reason, I did not see the discussion between Lukasz and you
> > via email. I can only see it via patchwork.
> 
> It is strange. However I'd appreciate to be in CC of this e-mail :-)
> 
> > 
> > Lukasz,
> > 
> > if the regulator for thermal unit is not enabled, what will you get?
> > temperature 0xFF + trip point -1? or Just temperature 0xFF?
> 
> Just 0xFF temperature.
> 
> Since 0xFF is larger than SW_TRIP point (mapped to
> THERMAL_TRIP_CRITICAL), the code at handle_critical_trips() is executed.
> 
> From my standpoint 0xFF is a possible and valid temperature in Exynos.
> 
sorry, but I still don't see why this patch breaks that.
The patch doesn't make any difference, as long as the trip_temp is a
positive value, no matter what the temperature is.

thanks,
rui

> Srinivas, what is your error/use case that you need this check?
> 
> > I don't think this patch makes any difference in the second case.
> > 
> > BTW, if you expect some indicator when the thermal unit is not
> > enabled, 
> 
> Actually, the TMU is enabled and configured, 
> Lack of proper regulator (vtmu) for TMU is the culprit of this
> situation.
> 
> > system critical shutdown is not a proper one, we can either
> > check the sysfs I/F, and we can add a warning message here, telling
> > that invalid trip point is found.
> 
> I think that, it would be a good idea to abort Exynos TMU probe when
> "vtmu" regulator is not found.
> 
> > 
> > thanks,
> > rui
> > 
> > On Wed, 2014-11-12 at 15:43 -0800, Srinivas Pandruvada wrote:
> > > Ignore invalid trip temperature less or equal to zero. Some
> > > buggy systems have invalid trips, causing system shutdown.
> > > 
> > > Signed-off-by: Srinivas Pandruvada
> > > <srinivas.pandruvada@linux.intel.com> Acked-by: Zhang Rui
> > > <rui.zhang@intel.com> ---
> > >  drivers/thermal/thermal_core.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/thermal/thermal_core.c
> > > b/drivers/thermal/thermal_core.c index 9bf10aa..fbf301a 100644
> > > --- a/drivers/thermal/thermal_core.c
> > > +++ b/drivers/thermal/thermal_core.c
> > > @@ -368,7 +368,7 @@ static void handle_critical_trips(struct
> > > thermal_zone_device *tz, tz->ops->get_trip_temp(tz, trip,
> > > &trip_temp); 
> > >  	/* If we have not crossed the trip_temp, we do not care. */
> > > -	if (tz->temperature < trip_temp)
> > > +	if (trip_temp <= 0 || tz->temperature < trip_temp)
> > >  		return;
> > >  
> > >  	trace_thermal_zone_trip(tz, trip, trip_type);
> > 
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> 
>