Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1525800 > unrolled thread

[PATCH 1/3] thermal: handle get_temp() errors properly

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/3] thermal: handle get_temp() errors properly Brian Norris <briannorris@chromium.org> - 2016-11-19 01:00 +0100
    [PATCH 2/3] thermal: rockchip: improve conversion error messages Brian Norris <briannorris@chromium.org> - 2016-11-19 01:00 +0100
      Re: [PATCH 2/3] thermal: rockchip: improve conversion error messages Caesar Wang <wxt@rock-chips.com> - 2016-11-19 05:00 +0100
      Re: [PATCH 2/3] thermal: rockchip: improve conversion error messages Caesar Wang <wxt@rock-chips.com> - 2016-11-19 05:00 +0100
        Re: [PATCH 2/3] thermal: rockchip: improve conversion error messages Caesar Wang <caesar.upstream@gmail.com> - 2016-11-22 03:00 +0100
          Re: [PATCH 2/3] thermal: rockchip: improve conversion error messages Brian Norris <briannorris@chromium.org> - 2016-11-22 03:20 +0100
            Re: [PATCH 2/3] thermal: rockchip: improve conversion error messages Caesar Wang <caesar.upstream@gmail.com> - 2016-11-22 03:40 +0100
              Re: [PATCH 2/3] thermal: rockchip: improve conversion error messages Brian Norris <briannorris@chromium.org> - 2016-11-22 04:50 +0100
          Re: [PATCH 2/3] thermal: rockchip: improve conversion error messages Zhang Rui <rui.zhang@intel.com> - 2016-11-22 09:00 +0100
            Re: [PATCH 2/3] thermal: rockchip: improve conversion error messages Caesar Wang <caesar.upstream@gmail.com> - 2016-11-22 13:50 +0100
    Re: [PATCH 1/3] thermal: handle get_temp() errors properly Caesar Wang <sasukewxt@163.com> - 2016-11-19 04:30 +0100
    Re: [PATCH 1/3] thermal: handle get_temp() errors properly Eduardo Valentin <edubezval@gmail.com> - 2016-11-19 04:50 +0100
      Re: [PATCH 1/3] thermal: handle get_temp() errors properly Brian Norris <briannorris@chromium.org> - 2016-11-19 06:40 +0100
        Re: [PATCH 1/3] thermal: handle get_temp() errors properly Zhang Rui <rui.zhang@intel.com> - 2016-11-22 09:00 +0100
          Re: [PATCH 1/3] thermal: handle get_temp() errors properly Eduardo Valentin <edubezval@gmail.com> - 2016-11-22 12:10 +0100
            Re: [PATCH 1/3] thermal: handle get_temp() errors properly Brian Norris <briannorris@chromium.org> - 2016-11-22 23:40 +0100

#1525800 — [PATCH 1/3] thermal: handle get_temp() errors properly

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;
 
 	if (IS_ENABLED(CONFIG_THERMAL_EMULATION) && tz->emul_temperature) {
 		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;
 	}
  
+exit_unlock:
 	mutex_unlock(&tz->lock);
 exit:
 	return ret;
-- 
2.8.0.rc3.226.g39d4020

[toc] | [next] | [standalone]

#1525801 — [PATCH 2/3] thermal: rockchip: improve conversion error messages

These error messages don't give much information about what went wrong.
It would be nice, for one, to see what invalid temperature was being
requested when conversion fails. It's also good to return an error when
we can't handle a conversion properly.

While we're at it, fix the grammar too.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
Note: it'd probably be even nicer to know which sensor this was, but we've
kinda abstracted that one away by this point...

 drivers/thermal/rockchip_thermal.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
index e227a9f0acf7..35554d146b9d 100644
--- a/drivers/thermal/rockchip_thermal.c
+++ b/drivers/thermal/rockchip_thermal.c
@@ -424,7 +424,8 @@ static u32 rk_tsadcv2_temp_to_code(struct chip_tsadc_table table,
 	}
 
 exit:
-	pr_err("Invalid the conversion, error=%d\n", error);
+	pr_err("%s: invalid temperature, temp=%d error=%d\n",
+		__func__, temp, error);
 	return error;
 }
 
@@ -475,7 +476,9 @@ static int rk_tsadcv2_code_to_temp(struct chip_tsadc_table table, u32 code,
 		}
 		break;
 	default:
-		pr_err("Invalid the conversion table\n");
+		pr_err("%s: invalid conversion table, mode=%d\n",
+			__func__, table.mode);
+		return -EINVAL;
 	}
 
 	/*
-- 
2.8.0.rc3.226.g39d4020

[toc] | [prev] | [next] | [standalone]

#1525842 — Re: [PATCH 2/3] thermal: rockchip: improve conversion error messages

在 2016年11月19日 11:31, Caesar Wang 写道:
> Brian,
>
> 在 2016年11月19日 07:52, Brian Norris 写道:
>> These error messages don't give much information about what went wrong.
>> It would be nice, for one, to see what invalid temperature was being
>> requested when conversion fails. It's also good to return an error when
>> we can't handle a conversion properly.
>>
>> While we're at it, fix the grammar too.
>>
>> Signed-off-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Caesar Wang@rock-chips.com

Thanks the fixes.

-Caesar
>> ---
>> Note: it'd probably be even nicer to know which sensor this was, but 
>> we've
>> kinda abstracted that one away by this point...
>>
>>   drivers/thermal/rockchip_thermal.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/thermal/rockchip_thermal.c 
>> b/drivers/thermal/rockchip_thermal.c
>> index e227a9f0acf7..35554d146b9d 100644
>> --- a/drivers/thermal/rockchip_thermal.c
>> +++ b/drivers/thermal/rockchip_thermal.c
>> @@ -424,7 +424,8 @@ static u32 rk_tsadcv2_temp_to_code(struct 
>> chip_tsadc_table table,
>>       }
>>     exit:
>> -    pr_err("Invalid the conversion, error=%d\n", error);
>> +    pr_err("%s: invalid temperature, temp=%d error=%d\n",
>> +        __func__, temp, error);
>
> I have do some similar for rockchip inside thermal driver.  Forget to 
> send for upstream. :(
> exit:
>     pr_err("%s: Invalid conversion table: code=%d, temperature=%d\n",
>            __func__, error, temp);
>
>>       return error;
>>   }
>>   @@ -475,7 +476,9 @@ static int rk_tsadcv2_code_to_temp(struct 
>> chip_tsadc_table table, u32 code,
>>           }
>>           break;
>>       default:
>> -        pr_err("Invalid the conversion table\n");
>> +        pr_err("%s: invalid conversion table, mode=%d\n",
>> +            __func__, table.mode);
>> +        return -EINVAL;
>>       }
>>         /*
>

[toc] | [prev] | [next] | [standalone]

#1525843 — Re: [PATCH 2/3] thermal: rockchip: improve conversion error messages

Brian,

在 2016年11月19日 07:52, Brian Norris 写道:
> These error messages don't give much information about what went wrong.
> It would be nice, for one, to see what invalid temperature was being
> requested when conversion fails. It's also good to return an error when
> we can't handle a conversion properly.
>
> While we're at it, fix the grammar too.
>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> Note: it'd probably be even nicer to know which sensor this was, but we've
> kinda abstracted that one away by this point...
>
>   drivers/thermal/rockchip_thermal.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> index e227a9f0acf7..35554d146b9d 100644
> --- a/drivers/thermal/rockchip_thermal.c
> +++ b/drivers/thermal/rockchip_thermal.c
> @@ -424,7 +424,8 @@ static u32 rk_tsadcv2_temp_to_code(struct chip_tsadc_table table,
>   	}
>   
>   exit:
> -	pr_err("Invalid the conversion, error=%d\n", error);
> +	pr_err("%s: invalid temperature, temp=%d error=%d\n",
> +		__func__, temp, error);

I have do some similar for rockchip inside thermal driver.  Forget to 
send for upstream. :(
exit:
     pr_err("%s: Invalid conversion table: code=%d, temperature=%d\n",
            __func__, error, temp);

>   	return error;
>   }
>   
> @@ -475,7 +476,9 @@ static int rk_tsadcv2_code_to_temp(struct chip_tsadc_table table, u32 code,
>   		}
>   		break;
>   	default:
> -		pr_err("Invalid the conversion table\n");
> +		pr_err("%s: invalid conversion table, mode=%d\n",
> +			__func__, table.mode);
> +		return -EINVAL;
>   	}
>   
>   	/*

[toc] | [prev] | [next] | [standalone]

#1527185 — Re: [PATCH 2/3] thermal: rockchip: improve conversion error messages

在 2016年11月19日 11:31, Caesar Wang 写道:
> Brian,
>
> 在 2016年11月19日 07:52, Brian Norris 写道:
>> These error messages don't give much information about what went wrong.
>> It would be nice, for one, to see what invalid temperature was being
>> requested when conversion fails. It's also good to return an error when
>> we can't handle a conversion properly.
>>
>> While we're at it, fix the grammar too.
>>
>> Signed-off-by: Brian Norris <briannorris@chromium.org>
>> ---
>> Note: it'd probably be even nicer to know which sensor this was, but 
>> we've
>> kinda abstracted that one away by this point...
>>
>>   drivers/thermal/rockchip_thermal.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/thermal/rockchip_thermal.c 
>> b/drivers/thermal/rockchip_thermal.c
>> index e227a9f0acf7..35554d146b9d 100644
>> --- a/drivers/thermal/rockchip_thermal.c
>> +++ b/drivers/thermal/rockchip_thermal.c
>> @@ -424,7 +424,8 @@ static u32 rk_tsadcv2_temp_to_code(struct 
>> chip_tsadc_table table,
>>       }
>>     exit:
>> -    pr_err("Invalid the conversion, error=%d\n", error);
>> +    pr_err("%s: invalid temperature, temp=%d error=%d\n",
>> +        __func__, temp, error);
>
> I have do some similar for rockchip inside thermal driver.  Forget to 
> send for upstream. :(
> exit:
>     pr_err("%s: Invalid conversion table: code=%d, temperature=%d\n",
>            __func__, error, temp);
>
>>       return error;
>>   }
>>   @@ -475,7 +476,9 @@ static int rk_tsadcv2_code_to_temp(struct 
>> chip_tsadc_table table, u32 code,
>>           }
>>           break;
>>       default:
>> -        pr_err("Invalid the conversion table\n");
>> +        pr_err("%s: invalid conversion table, mode=%d\n",
>> +            __func__, table.mode);
>> +        return -EINVAL;

CHECK: Alignment should match open parenthesis
#428: FILE: drivers/thermal/rockchip_thermal.c:428:
+    pr_err("%s: invalid temperature, temp=%d error=%d\n",
+        __func__, temp, error);

CHECK: Alignment should match open parenthesis
#480: FILE: drivers/thermal/rockchip_thermal.c:480:
+        pr_err("%s: invalid conversion table, mode=%d\n",
+            __func__, table->mode);


I'm ready to resend all rockchip thermal patches. (contain them)

>>       }
>>         /*
>
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

[toc] | [prev] | [next] | [standalone]

#1527186 — Re: [PATCH 2/3] thermal: rockchip: improve conversion error messages

On Tue, Nov 22, 2016 at 09:51:23AM +0800, Caesar Wang wrote:
> CHECK: Alignment should match open parenthesis
> #428: FILE: drivers/thermal/rockchip_thermal.c:428:
> +    pr_err("%s: invalid temperature, temp=%d error=%d\n",
> +        __func__, temp, error);
> 
> CHECK: Alignment should match open parenthesis
> #480: FILE: drivers/thermal/rockchip_thermal.c:480:
> +        pr_err("%s: invalid conversion table, mode=%d\n",
> +            __func__, table->mode);

What patch are you checking? I ran mine through checkpatch, and there
are no problems. Did you perhaps mangle the tabs into spaces when you
saved the patch?

> I'm ready to resend all rockchip thermal patches. (contain them)

I see no reason to resend so far; the only criticism was on the 1st
patch (a non-critical patch to the core thermal code; the others are
relatively independent, as long as you don't care that I'm adding
another error return without fixing up the broken
CONFIG_THERMAL_EMULATION support).

Brian

[toc] | [prev] | [next] | [standalone]

#1527189 — Re: [PATCH 2/3] thermal: rockchip: improve conversion error messages

在 2016年11月22日 10:15, Brian Norris 写道:
> On Tue, Nov 22, 2016 at 09:51:23AM +0800, Caesar Wang wrote:
>> CHECK: Alignment should match open parenthesis
>> #428: FILE: drivers/thermal/rockchip_thermal.c:428:
>> +    pr_err("%s: invalid temperature, temp=%d error=%d\n",
>> +        __func__, temp, error);
>>
>> CHECK: Alignment should match open parenthesis
>> #480: FILE: drivers/thermal/rockchip_thermal.c:480:
>> +        pr_err("%s: invalid conversion table, mode=%d\n",
>> +            __func__, table->mode);
> What patch are you checking? I ran mine through checkpatch, and there
> are no problems.

That  just checkcode on Chromeos kernelv4.4,  that trivial things :)
$chromiumos/src/third_party/kernel/v4.4$ checkcode 
drivers/thermal/rockchip_thermal.c
CHECK: Alignment should match open parenthesis
#428: FILE: drivers/thermal/rockchip_thermal.c:428:
+    pr_err("%s: invalid temperature, temp=%d error=%d\n",
+        __func__, temp, error);
...

vi drivers/thermal/rockchip_thermal.c +428 or  vi 
drivers/thermal/rockchip_thermal.c +480,

> Did you perhaps mangle the tabs into spaces when you
> saved the patch?
>
>> I'm ready to resend all rockchip thermal patches. (contain them)
> I see no reason to resend so far; the only criticism was on the 1st
> patch (a non-critical patch to the core thermal code; the others are
> relatively independent, as long as you don't care that I'm adding
> another error return without fixing up the broken
> CONFIG_THERMAL_EMULATION support).
>
> Brian
>

[toc] | [prev] | [next] | [standalone]

#1527202 — Re: [PATCH 2/3] thermal: rockchip: improve conversion error messages

On Tue, Nov 22, 2016 at 10:33:27AM +0800, Caesar Wang wrote:
> 在 2016年11月22日 10:15, Brian Norris 写道:
> >On Tue, Nov 22, 2016 at 09:51:23AM +0800, Caesar Wang wrote:
> >>CHECK: Alignment should match open parenthesis
> >>#428: FILE: drivers/thermal/rockchip_thermal.c:428:
> >>+    pr_err("%s: invalid temperature, temp=%d error=%d\n",
> >>+        __func__, temp, error);
> >>
> >>CHECK: Alignment should match open parenthesis
> >>#480: FILE: drivers/thermal/rockchip_thermal.c:480:
> >>+        pr_err("%s: invalid conversion table, mode=%d\n",
> >>+            __func__, table->mode);
> >What patch are you checking? I ran mine through checkpatch, and there
> >are no problems.
> 
> That  just checkcode on Chromeos kernelv4.4,  that trivial things :)
> $chromiumos/src/third_party/kernel/v4.4$ checkcode

I'm not familiar with that tool, and that repository isn't upstream
but...

> drivers/thermal/rockchip_thermal.c
> CHECK: Alignment should match open parenthesis
> #428: FILE: drivers/thermal/rockchip_thermal.c:428:
> +    pr_err("%s: invalid temperature, temp=%d error=%d\n",
> +        __func__, temp, error);

...on approximately my 10th read of this...I guess maybe this tool has
determined that 2 tabs is 1 character too much, because the second line
lines up with the '%' instead of the '"'. If this is so important to
you, you can of course edit my patch and include it with yours.

Regards,
Brian

[toc] | [prev] | [next] | [standalone]

#1527282 — Re: [PATCH 2/3] thermal: rockchip: improve conversion error messages

On Tue, 2016-11-22 at 09:51 +0800, Caesar Wang wrote:
> 在 2016年11月19日 11:31, Caesar Wang 写道:
> > 
> > Brian,
> > 
> > 在 2016年11月19日 07:52, Brian Norris 写道:
> > > 
> > > These error messages don't give much information about what went
> > > wrong.
> > > It would be nice, for one, to see what invalid temperature was
> > > being
> > > requested when conversion fails. It's also good to return an
> > > error when
> > > we can't handle a conversion properly.
> > > 
> > > While we're at it, fix the grammar too.
> > > 
> > > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > > ---
> > > Note: it'd probably be even nicer to know which sensor this was,
> > > but 
> > > we've
> > > kinda abstracted that one away by this point...
> > > 
> > >   drivers/thermal/rockchip_thermal.c | 7 +++++--
> > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/thermal/rockchip_thermal.c 
> > > b/drivers/thermal/rockchip_thermal.c
> > > index e227a9f0acf7..35554d146b9d 100644
> > > --- a/drivers/thermal/rockchip_thermal.c
> > > +++ b/drivers/thermal/rockchip_thermal.c
> > > @@ -424,7 +424,8 @@ static u32 rk_tsadcv2_temp_to_code(struct 
> > > chip_tsadc_table table,
> > >       }
> > >     exit:
> > > -    pr_err("Invalid the conversion, error=%d\n", error);
> > > +    pr_err("%s: invalid temperature, temp=%d error=%d\n",
> > > +        __func__, temp, error);
> > I have do some similar for rockchip inside thermal driver.  Forget
> > to 
> > send for upstream. :(
> > exit:
> >     pr_err("%s: Invalid conversion table: code=%d,
> > temperature=%d\n",
> >            __func__, error, temp);
> > 
> > > 
> > >       return error;
> > >   }
> > >   @@ -475,7 +476,9 @@ static int rk_tsadcv2_code_to_temp(struct 
> > > chip_tsadc_table table, u32 code,
> > >           }
> > >           break;
> > >       default:
> > > -        pr_err("Invalid the conversion table\n");
> > > +        pr_err("%s: invalid conversion table, mode=%d\n",
> > > +            __func__, table.mode);
> > > +        return -EINVAL;
> CHECK: Alignment should match open parenthesis
> #428: FILE: drivers/thermal/rockchip_thermal.c:428:
> +    pr_err("%s: invalid temperature, temp=%d error=%d\n",
> +        __func__, temp, error);
> 
> CHECK: Alignment should match open parenthesis
> #480: FILE: drivers/thermal/rockchip_thermal.c:480:
> +        pr_err("%s: invalid conversion table, mode=%d\n",
> +            __func__, table->mode);
> 
> 
> I'm ready to resend all rockchip thermal patches. (contain them)
> 
so I will ignore patch 2/3 and 3/3 for now and wait for your new patch
set.

thanks,
rui
> > 
> > > 
> > >       }
> > >         /*
> > 
> > 
> > _______________________________________________
> > Linux-rockchip mailing list
> > Linux-rockchip@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-rockchip

[toc] | [prev] | [next] | [standalone]

#1527483 — Re: [PATCH 2/3] thermal: rockchip: improve conversion error messages

在 2016年11月22日 15:57, Zhang Rui 写道:
> On Tue, 2016-11-22 at 09:51 +0800, Caesar Wang wrote:
>> 在 2016年11月19日 11:31, Caesar Wang 写道:
>>> Brian,
>>>
>>> 在 2016年11月19日 07:52, Brian Norris 写道:
>>>> These error messages don't give much information about what went
>>>> wrong.
>>>> It would be nice, for one, to see what invalid temperature was
>>>> being
>>>> requested when conversion fails. It's also good to return an
>>>> error when
>>>> we can't handle a conversion properly.
>>>>
>>>> While we're at it, fix the grammar too.
>>>>
>>>> Signed-off-by: Brian Norris <briannorris@chromium.org>
>>>> ---
>>>> Note: it'd probably be even nicer to know which sensor this was,
>>>> but
>>>> we've
>>>> kinda abstracted that one away by this point...
>>>>
>>>>    drivers/thermal/rockchip_thermal.c | 7 +++++--
>>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/thermal/rockchip_thermal.c
>>>> b/drivers/thermal/rockchip_thermal.c
>>>> index e227a9f0acf7..35554d146b9d 100644
>>>> --- a/drivers/thermal/rockchip_thermal.c
>>>> +++ b/drivers/thermal/rockchip_thermal.c
>>>> @@ -424,7 +424,8 @@ static u32 rk_tsadcv2_temp_to_code(struct
>>>> chip_tsadc_table table,
>>>>        }
>>>>      exit:
>>>> -    pr_err("Invalid the conversion, error=%d\n", error);
>>>> +    pr_err("%s: invalid temperature, temp=%d error=%d\n",
>>>> +        __func__, temp, error);
>>> I have do some similar for rockchip inside thermal driver.  Forget
>>> to
>>> send for upstream. :(
>>> exit:
>>>      pr_err("%s: Invalid conversion table: code=%d,
>>> temperature=%d\n",
>>>             __func__, error, temp);
>>>
>>>>        return error;
>>>>    }
>>>>    @@ -475,7 +476,9 @@ static int rk_tsadcv2_code_to_temp(struct
>>>> chip_tsadc_table table, u32 code,
>>>>            }
>>>>            break;
>>>>        default:
>>>> -        pr_err("Invalid the conversion table\n");
>>>> +        pr_err("%s: invalid conversion table, mode=%d\n",
>>>> +            __func__, table.mode);
>>>> +        return -EINVAL;
>> CHECK: Alignment should match open parenthesis
>> #428: FILE: drivers/thermal/rockchip_thermal.c:428:
>> +    pr_err("%s: invalid temperature, temp=%d error=%d\n",
>> +        __func__, temp, error);
>>
>> CHECK: Alignment should match open parenthesis
>> #480: FILE: drivers/thermal/rockchip_thermal.c:480:
>> +        pr_err("%s: invalid conversion table, mode=%d\n",
>> +            __func__, table->mode);
>>
>>
>> I'm ready to resend all rockchip thermal patches. (contain them)
>>
> so I will ignore patch 2/3 and 3/3 for now and wait for your new patch
> set.

Posted the patch set on https://lkml.org/lkml/2016/11/22/250

>
> thanks,
> rui
>>>>        }
>>>>          /*
>>>
>>> _______________________________________________
>>> Linux-rockchip mailing list
>>> Linux-rockchip@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

[toc] | [prev] | [next] | [standalone]

#1525838

Brian,
在 2016年11月19日 07:52, Brian Norris 写道:
> 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>
Tested-by: Caesar Wang <wxt@rock-chips.com>

[    8.111296] thermal thermal_zone4: power_allocator: sustainable_power 
will be estimated
[    8.119420] thermal_zone_get_temp:537 the ret=-19, no such device, 
look like the A/D value had no ready yet.
..
Anyway, this patch is useful for improving thermal.

-Caesar
> ---
>   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;
>   
>   	if (IS_ENABLED(CONFIG_THERMAL_EMULATION) && tz->emul_temperature) {
>   		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;
>   	}
>    
> +exit_unlock:
>   	mutex_unlock(&tz->lock);
>   exit:
>   	return ret;

[toc] | [prev] | [next] | [standalone]

#1525840

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.


>  
>  	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.

>  		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.

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

>  	}
>   
> +exit_unlock:
>  	mutex_unlock(&tz->lock);
>  exit:
>  	return ret;
> -- 
> 2.8.0.rc3.226.g39d4020
> 

[toc] | [prev] | [next] | [standalone]

#1525857

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

[toc] | [prev] | [next] | [standalone]

#1527283

Hi, Brian,

On Fri, 2016-11-18 at 21:30 -0800, Brian Norris wrote:
> 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.
> 
Agreed.
> > 
> > > 
> > >  
> > >  	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.
> 
right.
> > 
> > > 
> > >  		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...
> 
hmmm, I forgot why I missed this one in the end.
Eduardo,
would you mind refresh and resend the patch?

thanks,
rui
> Brian
> --
> 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

[toc] | [prev] | [next] | [standalone]

#1527417

On Tue, Nov 22, 2016 at 03:52:25PM +0800, Zhang Rui wrote:
> Hi, Brian,
> 
> On Fri, 2016-11-18 at 21:30 -0800, Brian Norris wrote:
> > 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.
> > 
> Agreed.

I also agree that this section of current get_temp is buggy. That is why 
I sent the patch some time ago.

> > > 
> > > > 
> > > >  
> > > >  	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.
> > 
> right.
> > > 
> > > > 
> > > >  		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?

Yes, Brian, we are concerned about the same bug.


> > 
> > > 
> > > 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.

Yeah, but that is why we need people to look the code considering all
features. :-)


> > 
> > 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...
> > 

I do not remember why Rui postponed it. A note of clarification, for
things that touch thermal core, I agree with Rui that they go through
his tree. Besides, I tend to avoid acking and sending my own patches
without proper review, which was not the case of that patch, that was
just postponed and fell into the cracks somehow.

> hmmm, I forgot why I missed this one in the end.
> Eduardo,
> would you mind refresh and resend the patch?

Yeah sure. I have at least three extra patch sets on thermal core on
my queue. But I would like to get first the thermal sysfs reorg in
first. This fix is one of the changes that will go on top of the thermal
sysfs reorg.

BR,

Eduardo 

[toc] | [prev] | [next] | [standalone]

#1527995

On Tue, Nov 22, 2016 at 03:00:47AM -0800, Eduardo Valentin wrote:
> On Tue, Nov 22, 2016 at 03:52:25PM +0800, Zhang Rui wrote:
> > On Fri, 2016-11-18 at 21:30 -0800, Brian Norris wrote:
> > > On Fri, Nov 18, 2016 at 07:41:59PM -0800, Eduardo Valentin wrote:
> > > > 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.
> 
> Yeah, but that is why we need people to look the code considering all
> features. :-)

Well, there are bugfixes and there are features. My patch fixed the bug
in the simplest way possible; it didn't break CONFIG_THERMAL_EMULATION
any further than it already was, and it'll still work if get_temp()
doesn't return an error.

I'd say your patch is essentially adding a feature, and IMO that's not
the best way to fix a bug. You can fix the bug and *then* add the
feature.

Anyway, I'm not going to tell you how to run your subsystem. If your
patch goes through, that's probably just as well.

[...]

> > hmmm, I forgot why I missed this one in the end.
> > Eduardo,
> > would you mind refresh and resend the patch?
> 
> Yeah sure. I have at least three extra patch sets on thermal core on
> my queue. But I would like to get first the thermal sysfs reorg in
> first. This fix is one of the changes that will go on top of the thermal
> sysfs reorg.

So, the bugfix depends on feature work? I guess I'll check back in
another year to see what the status of the bugfix is :)

Brian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web