thermal: rockchip: fix handling of invalid readings

PATCH In review Dmitry Torokhov 2 participants 5 messages Aug 7, 2015 – Sep 14, 2015

Subsystems (2)

Arm Rockchip Thermal

lore

Share & tools

lore

Dmitry Torokhov

author

2015-08-07 20:59 UTC #0 lore raw

PATCH 1/1 thermal: rockchip: fix handling of invalid readings

We attempted to signal invalid code by returning -EAGAIN from rk_tsadcv2_code_to_temp(), unfortunately the return value was stuffed directly into the temperature pointer, potentially confusing upper layers with temperature of -EINVAL.

Let's split temperature from error/success indicator to avoid such confusion.

Also change the way we scan the temperature table to start with the 2nd element so that we do not need to worry that we may reference out of bounds element while doing binary search and keep checking that we end up with 'mid' equal to 0 (since we are looking for the temperature that would fall into interval between the 'mid' and 'mid - 1') .

Signed-off-by Dmitry Torokhov <[email protected]>

---
 drivers/thermal/rockchip_thermal.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

drivers/thermal/rockchip_thermal.c +13 ~~-15~~

index c89ffb2..93ee307 100644
@@ -124,7 +124,7 @@ struct rockchip_thermal_data {
 #define TSADCV2_AUTO_PERIOD_HT_TIME		50  /* msec */
 
 struct tsadc_table {
-	unsigned long code;
+	u32 code;
 	long temp;
 };

@@ -164,7 +164,6 @@ static const struct tsadc_table v2_code_table[] = {
 	{3452, 115000},
 	{3437, 120000},
 	{3421, 125000},
-	{0, 125000},
 };
 
 static u32 rk_tsadcv2_temp_to_code(long temp)

@@ -191,19 +190,21 @@ static u32 rk_tsadcv2_temp_to_code(long temp)
 	return 0;
 }
 
-static int rk_tsadcv2_code_to_temp(u32 code)
+static int rk_tsadcv2_code_to_temp(u32 code, int *temp)
 {
-	unsigned int low = 0;
+	unsigned int low = 1;
 	unsigned int high = ARRAY_SIZE(v2_code_table) - 1;
 	unsigned int mid = (low + high) / 2;
 	unsigned int num;
 	unsigned long denom;
 
-	/* Invalid code, return -EAGAIN */
-	if (code > TSADCV2_DATA_MASK)
-		return -EAGAIN;
+	BUILD_BUG_ON(ARRAY_SIZE(v2_code_table) < 2);
 
-	while (low <= high && mid) {
+	code &= TSADCV2_DATA_MASK;
+	if (code < v2_code_table[high].code)
+		return -EAGAIN;		/* Incorrect reading */
+
+	while (low <= high) {
 		if (code >= v2_code_table[mid].code &&
 		    code < v2_code_table[mid - 1].code)
 			break;

@@ -223,7 +224,9 @@ static int rk_tsadcv2_code_to_temp(u32 code)
 	num = v2_code_table[mid].temp - v2_code_table[mid - 1].temp;
 	num *= v2_code_table[mid - 1].code - code;
 	denom = v2_code_table[mid - 1].code - v2_code_table[mid].code;
-	return v2_code_table[mid - 1].temp + (num / denom);
+	*temp = v2_code_table[mid - 1].temp + (num / denom);
+
+	return 0;
 }
 
 /**

@@ -281,14 +284,9 @@ static int rk_tsadcv2_get_temp(int chn, void __iomem *regs, int *temp)
 {
 	u32 val;
 
-	/* the A/D value of the channel last conversion need some time */
 	val = readl_relaxed(regs + TSADCV2_DATA(chn));
-	if (val == 0)
-		return -EAGAIN;
-
-	*temp = rk_tsadcv2_code_to_temp(val);
 
-	return 0;
+	return rk_tsadcv2_code_to_temp(val, temp);
 }
 
 static void rk_tsadcv2_tshut_temp(int chn, void __iomem *regs, long temp)

Signature · 5 lines

2.5.0.rc2.392.g76e840b


-- 
Dmitry

Caesar Wang

2015-08-10 06:30 UTC #1 lore raw

Dear Dmitry,

Thanks your patch.

The code looks like fine,but I don't think the TS-ADC will work on rk3288 SoC.

Quoted 80 lines · Dmitry Torokhov

Let's split temperature from error/success indicator to avoid such confusion.

Signed-off-by Dmitry Torokhov <[email protected]>

---
  drivers/thermal/rockchip_thermal.c | 28 +++++++++++++---------------
  1 file changed, 13 insertions(+), 15 deletions(-)

drivers/thermal/rockchip_thermal.c +12 -9

index c89ffb2..93ee307 100644
@@ -124,7 +124,7 @@ struct rockchip_thermal_data {
  #define TSADCV2_AUTO_PERIOD_HT_TIME		50  /* msec */
 
  struct tsadc_table {
-	unsigned long code;
+	u32 code;
  	long temp;
  };

@@ -164,7 +164,6 @@ static const struct tsadc_table v2_code_table[] = {
  	{3452, 115000},
  	{3437, 120000},
  	{3421, 125000},
-	{0, 125000},
  };
 
  static u32 rk_tsadcv2_temp_to_code(long temp)

@@ -191,19 +190,21 @@ static u32 rk_tsadcv2_temp_to_code(long temp)
  	return 0;
  }
 
-static int rk_tsadcv2_code_to_temp(u32 code)
+static int rk_tsadcv2_code_to_temp(u32 code, int *temp)
  {
-	unsigned int low = 0;
+	unsigned int low = 1;
  	unsigned int high = ARRAY_SIZE(v2_code_table) - 1;
  	unsigned int mid = (low + high) / 2;
  	unsigned int num;
  	unsigned long denom;
 
-	/* Invalid code, return -EAGAIN */
-	if (code > TSADCV2_DATA_MASK)
-		return -EAGAIN;
+	BUILD_BUG_ON(ARRAY_SIZE(v2_code_table) < 2);
 
-	while (low <= high && mid) {
+	code &= TSADCV2_DATA_MASK;
+	if (code < v2_code_table[high].code)
+		return -EAGAIN;		/* Incorrect reading */
+
+	while (low <= high) {
  		if (code >= v2_code_table[mid].code &&
  		    code < v2_code_table[mid - 1].code)
  			break;

@@ -223,7 +224,9 @@ static int rk_tsadcv2_code_to_temp(u32 code)
  	num = v2_code_table[mid].temp - v2_code_table[mid - 1].temp;
  	num *= v2_code_table[mid - 1].code - code;
  	denom = v2_code_table[mid - 1].code - v2_code_table[mid].code;
-	return v2_code_table[mid - 1].temp + (num / denom);
+	*temp = v2_code_table[mid - 1].temp + (num / denom);
+
+	return 0;
  }
 
  /**

drivers/thermal/rockchip_thermal.c static int rk_tsadcv2_get_temp(int chn, void __iomem *regs, int *temp) · line 288

Quoted 8 lines

drivers/thermal/rockchip_thermal.c +0 -4

@@ -281,14 +284,9 @@ static int rk_tsadcv2_get_temp(int chn, void __iomem *regs, int *temp)
  {
  	u32 val;
 
-	/* the A/D value of the channel last conversion need some time */
  	val = readl_relaxed(regs + TSADCV2_DATA(chn));
-	if (val == 0)
-		return -EAGAIN;
-

if we reserve the above code, that will get the ADC value.

Quoted 7 lines

drivers/thermal/rockchip_thermal.c +1 -2

-	*temp = rk_tsadcv2_code_to_temp(val);
 
-	return 0;
+	return rk_tsadcv2_code_to_temp(val, temp);
  }
 
  static void rk_tsadcv2_tshut_temp(int chn, void __iomem *regs, long temp)

Dmitry Torokhov

author

2015-08-10 07:59 UTC #2 lore raw

Hi Caesar,

Quoted 6 lines · Caesar Wang

Dear Dmitry,
Thanks your patch.
The code looks like fine,but I don't think the TS-ADC will work on rk3288 SoC.

What do you mean? It seems to work when I ran it...

Quoted 81 lines

? 2015?08?08? 04:59, Dmitry Torokhov ??:

Quoted 78 lines

Let's split temperature from error/success indicator to avoid such confusion.

Signed-off-by Dmitry Torokhov <[email protected]>

---
  drivers/thermal/rockchip_thermal.c | 28 +++++++++++++---------------
  1 file changed, 13 insertions(+), 15 deletions(-)

drivers/thermal/rockchip_thermal.c +1 -1

index c89ffb2..93ee307 100644
@@ -124,7 +124,7 @@ struct rockchip_thermal_data {
  #define TSADCV2_AUTO_PERIOD_HT_TIME           50  /* msec */
    struct tsadc_table {
-       unsigned long code;
+       u32 code;
        long temp;
  };
  @@ -164,7 +164,6 @@ static const struct tsadc_table v2_code_table[] = {
        {3452, 115000},

        {3437, 120000},
        {3421, 125000},

drivers/thermal/rockchip_thermal.c +11 -5

-       {0, 125000},
  };
    static u32 rk_tsadcv2_temp_to_code(long temp)

@@ -191,19 +190,21 @@ static u32 rk_tsadcv2_temp_to_code(long temp)
        return 0;
  }
  -static int rk_tsadcv2_code_to_temp(u32 code)
+static int rk_tsadcv2_code_to_temp(u32 code, int *temp)
  {
-       unsigned int low = 0;
+       unsigned int low = 1;
        unsigned int high = ARRAY_SIZE(v2_code_table) - 1;
        unsigned int mid = (low + high) / 2;
        unsigned int num;
        unsigned long denom;
  -     /* Invalid code, return -EAGAIN */
-       if (code > TSADCV2_DATA_MASK)
-               return -EAGAIN;
+       BUILD_BUG_ON(ARRAY_SIZE(v2_code_table) < 2);
  -     while (low <= high && mid) {
+       code &= TSADCV2_DATA_MASK;
+       if (code < v2_code_table[high].code)
+               return -EAGAIN;         /* Incorrect reading */
+
+       while (low <= high) {
                if (code >= v2_code_table[mid].code &&
                    code < v2_code_table[mid - 1].code)
                        break;

@@ -223,7 +224,9 @@ static int rk_tsadcv2_code_to_temp(u32 code)
        num = v2_code_table[mid].temp - v2_code_table[mid - 1].temp;
        num *= v2_code_table[mid - 1].code - code;
        denom = v2_code_table[mid - 1].code - v2_code_table[mid].code;
-       return v2_code_table[mid - 1].temp + (num / denom);
+       *temp = v2_code_table[mid - 1].temp + (num / denom);
+
+       return 0;
  }
    /**

@@ -281,14 +284,9 @@ static int rk_tsadcv2_get_temp(int chn, void __iomem
 *regs, int *temp)
  {
        u32 val;

Quoted 8 lines

drivers/thermal/rockchip_thermal.c static int rk_tsadcv2_get_temp(int chn, void __iomem *regs, int *temp) · line 288
Quoted 5 lines
drivers/thermal/rockchip_thermal.c +0 -3
  -     /* the A/D value of the channel last conversion need some time */
        val = readl_relaxed(regs + TSADCV2_DATA(chn));
-       if (val == 0)
-               return -EAGAIN;
-
if we reserve the above code, that will get the ADC value.

But if we pass 0 into rk_tsadcv2_code_to_temp() it will trigger:

Quoted 2 lines

drivers/thermal/rockchip_thermal.c static u32 rk_tsadcv2_temp_to_code(long temp) · line 205
Quoted 2 lines
drivers/thermal/rockchip_thermal.c +2 -0
+       if (code < v2_code_table[high].code)
+               return -EAGAIN;         /* Incorrect reading */

and still return -EAGAIN, as it was. There is no behavior change as far as I can see.

Quoted 10 lines

Quoted 8 lines

drivers/thermal/rockchip_thermal.c +1 -1

-       *temp = rk_tsadcv2_code_to_temp(val);
  -     return 0;
+       return rk_tsadcv2_code_to_temp(val, temp);
  }
    static void rk_tsadcv2_tshut_temp(int chn, void __iomem *regs, long

temp)

Thanks,
Dmitry

Caesar Wang

Tested-by

2015-08-10 10:22 UTC #3 lore raw

? 2015?08?10? 15:59, Dmitry Torokhov ??:

Quoted 10 lines

Hi Caesar,
Quoted 6 lines · Caesar Wang
Dear Dmitry,
Thanks your patch.
The code looks like fine,but I don't think the TS-ADC will work on rk3288 SoC.
What do you mean? It seems to work when I ran it...

Making a mistake.:-(

Quoted 37 lines

Quoted 37 lines
? 2015?08?08? 04:59, Dmitry Torokhov ??:
Quoted 35 lines
We attempted to signal invalid code by returning -EAGAIN from rk_tsadcv2_code_to_temp(), unfortunately the return value was stuffed directly into the temperature pointer, potentially confusing upper layers with temperature of -EINVAL.
Let's split temperature from error/success indicator to avoid such confusion.
Also change the way we scan the temperature table to start with the 2nd element so that we do not need to worry that we may reference out of bounds element while doing binary search and keep checking that we end up with 'mid' equal to 0 (since we are looking for the temperature that would fall into interval between the 'mid' and 'mid - 1') .
Signed-off-by Dmitry Torokhov <[email protected]>
---
   drivers/thermal/rockchip_thermal.c | 28 +++++++++++++---------------
   1 file changed, 13 insertions(+), 15 deletions(-)
drivers/thermal/rockchip_thermal.c +1 -1
index c89ffb2..93ee307 100644
@@ -124,7 +124,7 @@ struct rockchip_thermal_data {
   #define TSADCV2_AUTO_PERIOD_HT_TIME           50  /* msec */
     struct tsadc_table {
-       unsigned long code;
+       u32 code;
         long temp;
   };
   @@ -164,7 +164,6 @@ static const struct tsadc_table v2_code_table[] = {
         {3452, 115000},
         {3437, 120000},
         {3421, 125000},

Quoted 8 lines

Quoted 8 lines
drivers/thermal/rockchip_thermal.c static u32 rk_tsadcv2_temp_to_code(long temp) · line 193
Quoted 8 lines
drivers/thermal/rockchip_thermal.c +1 -1
-       {0, 125000},
   };
     static u32 rk_tsadcv2_temp_to_code(long temp)
@@ -191,19 +190,21 @@ static u32 rk_tsadcv2_temp_to_code(long temp)
         return 0;
   }
   -static int rk_tsadcv2_code_to_temp(u32 code)
+static int rk_tsadcv2_code_to_temp(u32 code, int *temp)

It's wired that can't git-am this patch.

I search the Eduardo'branch[0] for rockchip-thermal.c driver. The function is "static long rk_tsadcv2_code_to_temp(u32 code)"

Maybe i'm missing something, I verified this patch on my board but this fixed.

So you can free add it:

Tested-by Caesar Wang <[email protected]>

[0]:
url =
git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-thermal.git

---
Thanks,
Caesar

Quoted 41 lines

Quoted 33 lines

drivers/thermal/rockchip_thermal.c +10 -4

   {
-       unsigned int low = 0;
+       unsigned int low = 1;
         unsigned int high = ARRAY_SIZE(v2_code_table) - 1;
         unsigned int mid = (low + high) / 2;
         unsigned int num;
         unsigned long denom;
   -     /* Invalid code, return -EAGAIN */
-       if (code > TSADCV2_DATA_MASK)
-               return -EAGAIN;
+       BUILD_BUG_ON(ARRAY_SIZE(v2_code_table) < 2);
   -     while (low <= high && mid) {
+       code &= TSADCV2_DATA_MASK;
+       if (code < v2_code_table[high].code)
+               return -EAGAIN;         /* Incorrect reading */
+
+       while (low <= high) {
                 if (code >= v2_code_table[mid].code &&
                     code < v2_code_table[mid - 1].code)
                         break;

@@ -223,7 +224,9 @@ static int rk_tsadcv2_code_to_temp(u32 code)
         num = v2_code_table[mid].temp - v2_code_table[mid - 1].temp;
         num *= v2_code_table[mid - 1].code - code;
         denom = v2_code_table[mid - 1].code - v2_code_table[mid].code;
-       return v2_code_table[mid - 1].temp + (num / denom);
+       *temp = v2_code_table[mid - 1].temp + (num / denom);
+
+       return 0;
   }
     /**

@@ -281,14 +284,9 @@ static int rk_tsadcv2_get_temp(int chn, void __iomem
 *regs, int *temp)
   {

Quoted 8 lines

drivers/thermal/rockchip_thermal.c static int rk_tsadcv2_get_temp(int chn, void __iomem *regs, int *temp) · line 288
Quoted 6 lines
drivers/thermal/rockchip_thermal.c +0 -3
         u32 val;
   -     /* the A/D value of the channel last conversion need some time */
         val = readl_relaxed(regs + TSADCV2_DATA(chn));
-       if (val == 0)
-               return -EAGAIN;
-
if we reserve the above code, that will get the ADC value.

Quoted 6 lines

But if we pass 0 into rk_tsadcv2_code_to_temp() it will trigger:
Quoted 2 lines
drivers/thermal/rockchip_thermal.c static u32 rk_tsadcv2_temp_to_code(long temp) · line 205
Quoted 2 lines
drivers/thermal/rockchip_thermal.c +2 -0
+       if (code < v2_code_table[high].code)
+               return -EAGAIN;         /* Incorrect reading */
and still return -EAGAIN, as it was. There is no behavior change as far as I can see.

Yup, that's ok for me.

Quoted 13 lines

Quoted 8 lines

Quoted 7 lines

drivers/thermal/rockchip_thermal.c +1 -1

-       *temp = rk_tsadcv2_code_to_temp(val);
   -     return 0;
+       return rk_tsadcv2_code_to_temp(val, temp);
   }
     static void rk_tsadcv2_tshut_temp(int chn, void __iomem *regs, long

temp)

Thanks,
Dmitry

Caesar Wang

2015-09-14 11:46 UTC #4 lore raw

Hello Eduardo,

I guess you forgot to review/pick up this series patchs.

Another patch as follows:

[PATCH] thermal: rockhip: fix setting thermal shutdown polarity
https://patchwork.kernel.org/patch/6973131/

----
Thanks,
Caesar

? 2015?08?10? 18:22, Caesar Wang ??:

Quoted 157 lines

? 2015?08?10? 15:59, Dmitry Torokhov ??:
Quoted 11 lines
Hi Caesar,
Quoted 7 lines · Caesar Wang
Dear Dmitry,
Thanks your patch.
The code looks like fine,but I don't think the TS-ADC will work on rk3288
SoC.
What do you mean? It seems to work when I ran it...
Making a mistake.:-(
Quoted 41 lines
Quoted 41 lines
? 2015?08?08? 04:59, Dmitry Torokhov ??:
Quoted 39 lines
We attempted to signal invalid code by returning -EAGAIN from rk_tsadcv2_code_to_temp(), unfortunately the return value was stuffed directly into the temperature pointer, potentially confusing upper layers with temperature of -EINVAL.
Let's split temperature from error/success indicator to avoid such confusion.
Also change the way we scan the temperature table to start with the 2nd
element so that we do not need to worry that we may reference out of bounds element while doing binary search and keep checking that we end up with 'mid' equal to 0 (since we are looking for the temperature that
would fall into interval between the 'mid' and 'mid - 1') .
Signed-off-by Dmitry Torokhov <[email protected]>
---
   drivers/thermal/rockchip_thermal.c | 28 
+++++++++++++---------------
   1 file changed, 13 insertions(+), 15 deletions(-)
drivers/thermal/rockchip_thermal.c +1 -1
index c89ffb2..93ee307 100644
@@ -124,7 +124,7 @@ struct rockchip_thermal_data {
   #define TSADCV2_AUTO_PERIOD_HT_TIME           50  /* msec */
     struct tsadc_table {
-       unsigned long code;
+       u32 code;
         long temp;
   };
   @@ -164,7 +164,6 @@ static const struct tsadc_table 
 v2_code_table[] = {
         {3452, 115000},
         {3437, 120000},
         {3421, 125000},
Quoted 8 lines
Quoted 8 lines
drivers/thermal/rockchip_thermal.c static u32 rk_tsadcv2_temp_to_code(long temp) · line 193
Quoted 8 lines
drivers/thermal/rockchip_thermal.c +1 -1
-       {0, 125000},
   };
     static u32 rk_tsadcv2_temp_to_code(long temp)
@@ -191,19 +190,21 @@ static u32 rk_tsadcv2_temp_to_code(long temp)
         return 0;
   }
   -static int rk_tsadcv2_code_to_temp(u32 code)
+static int rk_tsadcv2_code_to_temp(u32 code, int *temp)
It's wired that can't git-am this patch.
I search the Eduardo'branch[0] for rockchip-thermal.c driver. The function is "static long rk_tsadcv2_code_to_temp(u32 code)"
Maybe i'm missing something, I verified this patch on my board but this fixed.
So you can free add it:
Tested-by Caesar Wang <[email protected]>
[0]:
url =
git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-thermal.git
---
Thanks,
Caesar
Quoted 44 lines
Quoted 36 lines
Quoted 36 lines
drivers/thermal/rockchip_thermal.c +10 -4
   {
-       unsigned int low = 0;
+       unsigned int low = 1;
         unsigned int high = ARRAY_SIZE(v2_code_table) - 1;
         unsigned int mid = (low + high) / 2;
         unsigned int num;
         unsigned long denom;
   -     /* Invalid code, return -EAGAIN */
-       if (code > TSADCV2_DATA_MASK)
-               return -EAGAIN;
+       BUILD_BUG_ON(ARRAY_SIZE(v2_code_table) < 2);
   -     while (low <= high && mid) {
+       code &= TSADCV2_DATA_MASK;
+       if (code < v2_code_table[high].code)
+               return -EAGAIN;         /* Incorrect reading */
+
+       while (low <= high) {
                 if (code >= v2_code_table[mid].code &&
                     code < v2_code_table[mid - 1].code)
                         break;
@@ -223,7 +224,9 @@ static int rk_tsadcv2_code_to_temp(u32 code)
         num = v2_code_table[mid].temp - v2_code_table[mid - 1].temp;
         num *= v2_code_table[mid - 1].code - code;
         denom = v2_code_table[mid - 1].code - 
 v2_code_table[mid].code;
-       return v2_code_table[mid - 1].temp + (num / denom);
+       *temp = v2_code_table[mid - 1].temp + (num / denom);
+
+       return 0;
   }
     /**
@@ -281,14 +284,9 @@ static int rk_tsadcv2_get_temp(int chn, void 
__iomem
*regs, int *temp)
   {
         u32 val;
Quoted 8 lines
drivers/thermal/rockchip_thermal.c static int rk_tsadcv2_get_temp(int chn, void __iomem *regs, int *temp) · line 288
Quoted 6 lines
- /* the A/D value of the channel last conversion need some
time */
         val = readl_relaxed(regs + TSADCV2_DATA(chn));
drivers/thermal/rockchip_thermal.c +0 -3
-       if (val == 0)
-               return -EAGAIN;
-
if we reserve the above code, that will get the ADC value.
Quoted 6 lines
But if we pass 0 into rk_tsadcv2_code_to_temp() it will trigger:
Quoted 2 lines
drivers/thermal/rockchip_thermal.c static u32 rk_tsadcv2_temp_to_code(long temp) · line 205
Quoted 2 lines
drivers/thermal/rockchip_thermal.c +2 -0
+       if (code < v2_code_table[high].code)
+               return -EAGAIN;         /* Incorrect reading */
and still return -EAGAIN, as it was. There is no behavior change as far as I can see.
Yup, that's ok for me.
Quoted 14 lines
Quoted 9 lines
Quoted 8 lines
drivers/thermal/rockchip_thermal.c +1 -1
-       *temp = rk_tsadcv2_code_to_temp(val);
   -     return 0;
+       return rk_tsadcv2_code_to_temp(val, temp);
   }
     static void rk_tsadcv2_tshut_temp(int chn, void __iomem *regs, 
long
temp)
Thanks,
Dmitry