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

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

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

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

[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);
 	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

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

[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;
>   	}
>   
>   	/*

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

[Caesar Wang]

在 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;
>>       }
>>         /*
>

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

[Caesar Wang]

在 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

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

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

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

[Caesar Wang]

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

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

[Brian Norris]

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

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

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

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

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

[Caesar Wang]

在 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

[PATCH 3/3] thermal: rockchip: don't pass table structs by value

[Brian Norris]

This driver passes struct chip_tsadc_table by value throughout; this is
inefficient, and AFAICT, there is no reason for it. Let's pass pointers
instead.

Signed-off-by: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 drivers/thermal/rockchip_thermal.c | 80 +++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 40 deletions(-)

diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
index 35554d146b9d..30fb95a0dff0 100644
--- a/drivers/thermal/rockchip_thermal.c
+++ b/drivers/thermal/rockchip_thermal.c
@@ -118,11 +118,11 @@ struct rockchip_tsadc_chip {
 	void (*control)(void __iomem *reg, bool on);
 
 	/* Per-sensor methods */
-	int (*get_temp)(struct chip_tsadc_table table,
+	int (*get_temp)(const struct chip_tsadc_table *table,
 			int chn, void __iomem *reg, int *temp);
-	void (*set_alarm_temp)(struct chip_tsadc_table table,
+	void (*set_alarm_temp)(const struct chip_tsadc_table *table,
 			       int chn, void __iomem *reg, int temp);
-	void (*set_tshut_temp)(struct chip_tsadc_table table,
+	void (*set_tshut_temp)(const struct chip_tsadc_table *table,
 			       int chn, void __iomem *reg, int temp);
 	void (*set_tshut_mode)(int chn, void __iomem *reg, enum tshut_mode m);
 
@@ -397,26 +397,26 @@ static const struct tsadc_table rk3399_code_table[] = {
 	{TSADCV3_DATA_MASK, 125000},
 };
 
-static u32 rk_tsadcv2_temp_to_code(struct chip_tsadc_table table,
+static u32 rk_tsadcv2_temp_to_code(const struct chip_tsadc_table *table,
 				   int temp)
 {
 	int high, low, mid;
 	u32 error = 0;
 
 	low = 0;
-	high = table.length - 1;
+	high = table->length - 1;
 	mid = (high + low) / 2;
 
 	/* Return mask code data when the temp is over table range */
-	if (temp < table.id[low].temp || temp > table.id[high].temp) {
-		error = table.data_mask;
+	if (temp < table->id[low].temp || temp > table->id[high].temp) {
+		error = table->data_mask;
 		goto exit;
 	}
 
 	while (low <= high) {
-		if (temp == table.id[mid].temp)
-			return table.id[mid].code;
-		else if (temp < table.id[mid].temp)
+		if (temp == table->id[mid].temp)
+			return table->id[mid].code;
+		else if (temp < table->id[mid].temp)
 			high = mid - 1;
 		else
 			low = mid + 1;
@@ -429,28 +429,28 @@ static u32 rk_tsadcv2_temp_to_code(struct chip_tsadc_table table,
 	return error;
 }
 
-static int rk_tsadcv2_code_to_temp(struct chip_tsadc_table table, u32 code,
-				   int *temp)
+static int rk_tsadcv2_code_to_temp(const struct chip_tsadc_table *table,
+				   u32 code, int *temp)
 {
 	unsigned int low = 1;
-	unsigned int high = table.length - 1;
+	unsigned int high = table->length - 1;
 	unsigned int mid = (low + high) / 2;
 	unsigned int num;
 	unsigned long denom;
 
-	WARN_ON(table.length < 2);
+	WARN_ON(table->length < 2);
 
-	switch (table.mode) {
+	switch (table->mode) {
 	case ADC_DECREMENT:
-		code &= table.data_mask;
-		if (code < table.id[high].code)
+		code &= table->data_mask;
+		if (code < table->id[high].code)
 			return -EAGAIN;		/* Incorrect reading */
 
 		while (low <= high) {
-			if (code >= table.id[mid].code &&
-			    code < table.id[mid - 1].code)
+			if (code >= table->id[mid].code &&
+			    code < table->id[mid - 1].code)
 				break;
-			else if (code < table.id[mid].code)
+			else if (code < table->id[mid].code)
 				low = mid + 1;
 			else
 				high = mid - 1;
@@ -459,15 +459,15 @@ static int rk_tsadcv2_code_to_temp(struct chip_tsadc_table table, u32 code,
 		}
 		break;
 	case ADC_INCREMENT:
-		code &= table.data_mask;
-		if (code < table.id[low].code)
+		code &= table->data_mask;
+		if (code < table->id[low].code)
 			return -EAGAIN;		/* Incorrect reading */
 
 		while (low <= high) {
-			if (code <= table.id[mid].code &&
-			    code > table.id[mid - 1].code)
+			if (code <= table->id[mid].code &&
+			    code > table->id[mid - 1].code)
 				break;
-			else if (code > table.id[mid].code)
+			else if (code > table->id[mid].code)
 				low = mid + 1;
 			else
 				high = mid - 1;
@@ -477,7 +477,7 @@ static int rk_tsadcv2_code_to_temp(struct chip_tsadc_table table, u32 code,
 		break;
 	default:
 		pr_err("%s: invalid conversion table, mode=%d\n",
-			__func__, table.mode);
+			__func__, table->mode);
 		return -EINVAL;
 	}
 
@@ -487,10 +487,10 @@ static int rk_tsadcv2_code_to_temp(struct chip_tsadc_table table, u32 code,
 	 * temperature between 2 table entries is linear and interpolate
 	 * to produce less granular result.
 	 */
-	num = table.id[mid].temp - table.id[mid - 1].temp;
-	num *= abs(table.id[mid - 1].code - code);
-	denom = abs(table.id[mid - 1].code - table.id[mid].code);
-	*temp = table.id[mid - 1].temp + (num / denom);
+	num = table->id[mid].temp - table->id[mid - 1].temp;
+	num *= abs(table->id[mid - 1].code - code);
+	denom = abs(table->id[mid - 1].code - table->id[mid].code);
+	*temp = table->id[mid - 1].temp + (num / denom);
 
 	return 0;
 }
@@ -646,7 +646,7 @@ static void rk_tsadcv3_control(void __iomem *regs, bool enable)
 	writel_relaxed(val, regs + TSADCV2_AUTO_CON);
 }
 
-static int rk_tsadcv2_get_temp(struct chip_tsadc_table table,
+static int rk_tsadcv2_get_temp(const struct chip_tsadc_table *table,
 			       int chn, void __iomem *regs, int *temp)
 {
 	u32 val;
@@ -656,17 +656,17 @@ static int rk_tsadcv2_get_temp(struct chip_tsadc_table table,
 	return rk_tsadcv2_code_to_temp(table, val, temp);
 }
 
-static void rk_tsadcv2_alarm_temp(struct chip_tsadc_table table,
+static void rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table,
 				  int chn, void __iomem *regs, int temp)
 {
 	u32 alarm_value, int_en;
 
 	/* Make sure the value is valid */
 	alarm_value = rk_tsadcv2_temp_to_code(table, temp);
-	if (alarm_value == table.data_mask)
+	if (alarm_value == table->data_mask)
 		return;
 
-	writel_relaxed(alarm_value & table.data_mask,
+	writel_relaxed(alarm_value & table->data_mask,
 		       regs + TSADCV2_COMP_INT(chn));
 
 	int_en = readl_relaxed(regs + TSADCV2_INT_EN);
@@ -674,14 +674,14 @@ static void rk_tsadcv2_alarm_temp(struct chip_tsadc_table table,
 	writel_relaxed(int_en, regs + TSADCV2_INT_EN);
 }
 
-static void rk_tsadcv2_tshut_temp(struct chip_tsadc_table table,
+static void rk_tsadcv2_tshut_temp(const struct chip_tsadc_table *table,
 				  int chn, void __iomem *regs, int temp)
 {
 	u32 tshut_value, val;
 
 	/* Make sure the value is valid */
 	tshut_value = rk_tsadcv2_temp_to_code(table, temp);
-	if (tshut_value == table.data_mask)
+	if (tshut_value == table->data_mask)
 		return;
 
 	writel_relaxed(tshut_value, regs + TSADCV2_COMP_SHUT(chn));
@@ -891,7 +891,7 @@ static int rockchip_thermal_set_trips(void *_sensor, int low, int high)
 	dev_dbg(&thermal->pdev->dev, "%s: sensor %d: low: %d, high %d\n",
 		__func__, sensor->id, low, high);
 
-	tsadc->set_alarm_temp(tsadc->table,
+	tsadc->set_alarm_temp(&tsadc->table,
 			      sensor->id, thermal->regs, high);
 
 	return 0;
@@ -904,7 +904,7 @@ static int rockchip_thermal_get_temp(void *_sensor, int *out_temp)
 	const struct rockchip_tsadc_chip *tsadc = sensor->thermal->chip;
 	int retval;
 
-	retval = tsadc->get_temp(tsadc->table,
+	retval = tsadc->get_temp(&tsadc->table,
 				 sensor->id, thermal->regs, out_temp);
 	dev_dbg(&thermal->pdev->dev, "sensor %d - temp: %d, retval: %d\n",
 		sensor->id, *out_temp, retval);
@@ -988,7 +988,7 @@ rockchip_thermal_register_sensor(struct platform_device *pdev,
 	int error;
 
 	tsadc->set_tshut_mode(id, thermal->regs, thermal->tshut_mode);
-	tsadc->set_tshut_temp(tsadc->table, id, thermal->regs,
+	tsadc->set_tshut_temp(&tsadc->table, id, thermal->regs,
 			      thermal->tshut_temp);
 
 	sensor->thermal = thermal;
@@ -1202,7 +1202,7 @@ static int __maybe_unused rockchip_thermal_resume(struct device *dev)
 
 		thermal->chip->set_tshut_mode(id, thermal->regs,
 					      thermal->tshut_mode);
-		thermal->chip->set_tshut_temp(thermal->chip->table,
+		thermal->chip->set_tshut_temp(&thermal->chip->table,
 					      id, thermal->regs,
 					      thermal->tshut_temp);
 	}
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 3/3] thermal: rockchip: don't pass table structs by value

[Caesar Wang]

在 2016年11月19日 07:52, Brian Norris 写道:
> This driver passes struct chip_tsadc_table by value throughout; this is
> inefficient, and AFAICT, there is no reason for it. Let's pass pointers
> instead.
>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Caesar Wang <wxt@rock-chips.com>
Tested-by: Caesar Wang <wxt@rock-chips.com>

Yup, that make sense to improve efficiency.
Thanks the fixes.

And tested on rk3399 evb board.

> ---
>   drivers/thermal/rockchip_thermal.c | 80 +++++++++++++++++++-------------------
>   1 file changed, 40 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> index 35554d146b9d..30fb95a0dff0 100644
> --- a/drivers/thermal/rockchip_thermal.c
> +++ b/drivers/thermal/rockchip_thermal.c
> @@ -118,11 +118,11 @@ struct rockchip_tsadc_chip {
>   	void (*control)(void __iomem *reg, bool on);
>   
>   	/* Per-sensor methods */
> -	int (*get_temp)(struct chip_tsadc_table table,
> +	int (*get_temp)(const struct chip_tsadc_table *table,
>   			int chn, void __iomem *reg, int *temp);
> -	void (*set_alarm_temp)(struct chip_tsadc_table table,
> +	void (*set_alarm_temp)(const struct chip_tsadc_table *table,
>   			       int chn, void __iomem *reg, int temp);
> -	void (*set_tshut_temp)(struct chip_tsadc_table table,
> +	void (*set_tshut_temp)(const struct chip_tsadc_table *table,
>   			       int chn, void __iomem *reg, int temp);
>   	void (*set_tshut_mode)(int chn, void __iomem *reg, enum tshut_mode m);
>   
> @@ -397,26 +397,26 @@ static const struct tsadc_table rk3399_code_table[] = {
>   	{TSADCV3_DATA_MASK, 125000},
>   };
>   
> -static u32 rk_tsadcv2_temp_to_code(struct chip_tsadc_table table,
> +static u32 rk_tsadcv2_temp_to_code(const struct chip_tsadc_table *table,
>   				   int temp)
>   {
>   	int high, low, mid;
>   	u32 error = 0;
>   
>   	low = 0;
> -	high = table.length - 1;
> +	high = table->length - 1;
>   	mid = (high + low) / 2;
>   
>   	/* Return mask code data when the temp is over table range */
> -	if (temp < table.id[low].temp || temp > table.id[high].temp) {
> -		error = table.data_mask;
> +	if (temp < table->id[low].temp || temp > table->id[high].temp) {
> +		error = table->data_mask;
>   		goto exit;
>   	}
>   
>   	while (low <= high) {
> -		if (temp == table.id[mid].temp)
> -			return table.id[mid].code;
> -		else if (temp < table.id[mid].temp)
> +		if (temp == table->id[mid].temp)
> +			return table->id[mid].code;
> +		else if (temp < table->id[mid].temp)
>   			high = mid - 1;
>   		else
>   			low = mid + 1;
> @@ -429,28 +429,28 @@ static u32 rk_tsadcv2_temp_to_code(struct chip_tsadc_table table,
>   	return error;
>   }
>   
> -static int rk_tsadcv2_code_to_temp(struct chip_tsadc_table table, u32 code,
> -				   int *temp)
> +static int rk_tsadcv2_code_to_temp(const struct chip_tsadc_table *table,
> +				   u32 code, int *temp)
>   {
>   	unsigned int low = 1;
> -	unsigned int high = table.length - 1;
> +	unsigned int high = table->length - 1;
>   	unsigned int mid = (low + high) / 2;
>   	unsigned int num;
>   	unsigned long denom;
>   
> -	WARN_ON(table.length < 2);
> +	WARN_ON(table->length < 2);
>   
> -	switch (table.mode) {
> +	switch (table->mode) {
>   	case ADC_DECREMENT:
> -		code &= table.data_mask;
> -		if (code < table.id[high].code)
> +		code &= table->data_mask;
> +		if (code < table->id[high].code)
>   			return -EAGAIN;		/* Incorrect reading */
>   
>   		while (low <= high) {
> -			if (code >= table.id[mid].code &&
> -			    code < table.id[mid - 1].code)
> +			if (code >= table->id[mid].code &&
> +			    code < table->id[mid - 1].code)
>   				break;
> -			else if (code < table.id[mid].code)
> +			else if (code < table->id[mid].code)
>   				low = mid + 1;
>   			else
>   				high = mid - 1;
> @@ -459,15 +459,15 @@ static int rk_tsadcv2_code_to_temp(struct chip_tsadc_table table, u32 code,
>   		}
>   		break;
>   	case ADC_INCREMENT:
> -		code &= table.data_mask;
> -		if (code < table.id[low].code)
> +		code &= table->data_mask;
> +		if (code < table->id[low].code)
>   			return -EAGAIN;		/* Incorrect reading */
>   
>   		while (low <= high) {
> -			if (code <= table.id[mid].code &&
> -			    code > table.id[mid - 1].code)
> +			if (code <= table->id[mid].code &&
> +			    code > table->id[mid - 1].code)
>   				break;
> -			else if (code > table.id[mid].code)
> +			else if (code > table->id[mid].code)
>   				low = mid + 1;
>   			else
>   				high = mid - 1;
> @@ -477,7 +477,7 @@ static int rk_tsadcv2_code_to_temp(struct chip_tsadc_table table, u32 code,
>   		break;
>   	default:
>   		pr_err("%s: invalid conversion table, mode=%d\n",
> -			__func__, table.mode);
> +			__func__, table->mode);
>   		return -EINVAL;
>   	}
>   
> @@ -487,10 +487,10 @@ static int rk_tsadcv2_code_to_temp(struct chip_tsadc_table table, u32 code,
>   	 * temperature between 2 table entries is linear and interpolate
>   	 * to produce less granular result.
>   	 */
> -	num = table.id[mid].temp - table.id[mid - 1].temp;
> -	num *= abs(table.id[mid - 1].code - code);
> -	denom = abs(table.id[mid - 1].code - table.id[mid].code);
> -	*temp = table.id[mid - 1].temp + (num / denom);
> +	num = table->id[mid].temp - table->id[mid - 1].temp;
> +	num *= abs(table->id[mid - 1].code - code);
> +	denom = abs(table->id[mid - 1].code - table->id[mid].code);
> +	*temp = table->id[mid - 1].temp + (num / denom);
>   
>   	return 0;
>   }
> @@ -646,7 +646,7 @@ static void rk_tsadcv3_control(void __iomem *regs, bool enable)
>   	writel_relaxed(val, regs + TSADCV2_AUTO_CON);
>   }
>   
> -static int rk_tsadcv2_get_temp(struct chip_tsadc_table table,
> +static int rk_tsadcv2_get_temp(const struct chip_tsadc_table *table,
>   			       int chn, void __iomem *regs, int *temp)
>   {
>   	u32 val;
> @@ -656,17 +656,17 @@ static int rk_tsadcv2_get_temp(struct chip_tsadc_table table,
>   	return rk_tsadcv2_code_to_temp(table, val, temp);
>   }
>   
> -static void rk_tsadcv2_alarm_temp(struct chip_tsadc_table table,
> +static void rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table,
>   				  int chn, void __iomem *regs, int temp)
>   {
>   	u32 alarm_value, int_en;
>   
>   	/* Make sure the value is valid */
>   	alarm_value = rk_tsadcv2_temp_to_code(table, temp);
> -	if (alarm_value == table.data_mask)
> +	if (alarm_value == table->data_mask)
>   		return;
>   
> -	writel_relaxed(alarm_value & table.data_mask,
> +	writel_relaxed(alarm_value & table->data_mask,
>   		       regs + TSADCV2_COMP_INT(chn));
>   
>   	int_en = readl_relaxed(regs + TSADCV2_INT_EN);
> @@ -674,14 +674,14 @@ static void rk_tsadcv2_alarm_temp(struct chip_tsadc_table table,
>   	writel_relaxed(int_en, regs + TSADCV2_INT_EN);
>   }
>   
> -static void rk_tsadcv2_tshut_temp(struct chip_tsadc_table table,
> +static void rk_tsadcv2_tshut_temp(const struct chip_tsadc_table *table,
>   				  int chn, void __iomem *regs, int temp)
>   {
>   	u32 tshut_value, val;
>   
>   	/* Make sure the value is valid */
>   	tshut_value = rk_tsadcv2_temp_to_code(table, temp);
> -	if (tshut_value == table.data_mask)
> +	if (tshut_value == table->data_mask)
>   		return;
>   
>   	writel_relaxed(tshut_value, regs + TSADCV2_COMP_SHUT(chn));
> @@ -891,7 +891,7 @@ static int rockchip_thermal_set_trips(void *_sensor, int low, int high)
>   	dev_dbg(&thermal->pdev->dev, "%s: sensor %d: low: %d, high %d\n",
>   		__func__, sensor->id, low, high);
>   
> -	tsadc->set_alarm_temp(tsadc->table,
> +	tsadc->set_alarm_temp(&tsadc->table,
>   			      sensor->id, thermal->regs, high);
>   
>   	return 0;
> @@ -904,7 +904,7 @@ static int rockchip_thermal_get_temp(void *_sensor, int *out_temp)
>   	const struct rockchip_tsadc_chip *tsadc = sensor->thermal->chip;
>   	int retval;
>   
> -	retval = tsadc->get_temp(tsadc->table,
> +	retval = tsadc->get_temp(&tsadc->table,
>   				 sensor->id, thermal->regs, out_temp);
>   	dev_dbg(&thermal->pdev->dev, "sensor %d - temp: %d, retval: %d\n",
>   		sensor->id, *out_temp, retval);
> @@ -988,7 +988,7 @@ rockchip_thermal_register_sensor(struct platform_device *pdev,
>   	int error;
>   
>   	tsadc->set_tshut_mode(id, thermal->regs, thermal->tshut_mode);
> -	tsadc->set_tshut_temp(tsadc->table, id, thermal->regs,
> +	tsadc->set_tshut_temp(&tsadc->table, id, thermal->regs,
>   			      thermal->tshut_temp);
>   
>   	sensor->thermal = thermal;
> @@ -1202,7 +1202,7 @@ static int __maybe_unused rockchip_thermal_resume(struct device *dev)
>   
>   		thermal->chip->set_tshut_mode(id, thermal->regs,
>   					      thermal->tshut_mode);
> -		thermal->chip->set_tshut_temp(thermal->chip->table,
> +		thermal->chip->set_tshut_temp(&thermal->chip->table,
>   					      id, thermal->regs,
>   					      thermal->tshut_temp);
>   	}

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

[Caesar Wang]

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;

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

[Eduardo Valentin]

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
> 

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

[Brian Norris]

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

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

[Zhang Rui]

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

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

[Eduardo Valentin]

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 

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

[Brian Norris]

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

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

[Dmitry Torokhov]

On Tue, Nov 22, 2016 at 2:27 PM, Brian Norris <briannorris@chromium.org> wrote:
> 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 :)

Not quite a year, but the status is still the same ;)

By the way, I do not quite understand why we want to mess with
emulated temperature when hardware reports errors. I'd say when
get_temp() fails we need to let upper layers know right away. Only
when we read temperature successfully and we are sure that the
temperature is not above critical level we should allow reporting
emulated value.

Can we please apply the patch?

Thanks.

-- 
Dmitry