lkml.org 
[lkml]   [2016]   [Nov]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 1/3] thermal: handle get_temp() errors properly
    Hi,

    On Fri, Nov 18, 2016 at 07:41:59PM -0800, Eduardo Valentin wrote:
    > On Fri, Nov 18, 2016 at 03:52:55PM -0800, Brian Norris wrote:
    > > If using CONFIG_THERMAL_EMULATION, there's a corner case where we might
    > > get an error from the zone's get_temp() callback, but we'll ignore that
    > > and keep using its value. Let's just error out properly instead.
    > >
    > > Signed-off-by: Brian Norris <briannorris@chromium.org>
    > > ---
    > > drivers/thermal/thermal_core.c | 3 +++
    > > 1 file changed, 3 insertions(+)
    > >
    > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
    > > index 911fd964c742..0fa497f10d25 100644
    > > --- a/drivers/thermal/thermal_core.c
    > > +++ b/drivers/thermal/thermal_core.c
    > > @@ -494,6 +494,8 @@ int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
    > > mutex_lock(&tz->lock);
    > >
    > > ret = tz->ops->get_temp(tz, temp);
    > > + if (ret)
    > > + goto exit_unlock;
    >
    > Yeah, but the follow through is intentional, if I am not mistaken.

    OK...but it has a bug. It potentially utilizes an uninitialized value
    for *temp.

    > >
    > > if (IS_ENABLED(CONFIG_THERMAL_EMULATION) && tz->emul_temperature) {
    >
    > Even if the driver is not able to read real temperature, but emul temp
    > is configured, then there is still opportunity to report the emulated
    > temperature.

    OK, maybe, but you should avoid doing this comparison then:

    513 if (!ret && *temp < crit_temp)
    514 *temp = tz->emul_temperature;

    Note that 'ret' might be 0 (from the calls to ->get_trip_type()), and then
    you're comparing with the uninitialized value of *temp. So you need some
    solution that accounts for this and decides to ignore the real
    temperature properly.

    > > for (count = 0; count < tz->trips; count++) {
    > > @@ -514,6 +516,7 @@ int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
    > > *temp = tz->emul_temperature;
    >
    > And if you check the lines at the bottom of the loop, you will see that,
    > in the fail case, we will stil compare to what is the content of temp,
    > which might be problematic.

    Yes...are you saying the same thing I am above?

    > I would prefer we consider the patch I sent
    > some time ago:
    > https://patchwork.kernel.org/patch/7876381/

    Honestly I didn't look that deeply into the framework here (and I also
    don't use CONFIG_THERMAL_EMULATION), I was just fixing something that
    was obviously wrong.

    But on first read, that patch looks good to me -- although it'd be good
    to note the uninitialized value fix in the comit log. Any reason that
    didn't end up getting merged? It looks like it got reviewed, and you're
    a thermal subsystem maintainer...

    Brian
    \
     
     \ /
      Last update: 2016-11-19 06:31    [W:2.543 / U:0.048 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site