Skip to contentSkip to conversation

mfd: tps65911-comparator: Fix an off by one bug

PATCH Superseded Dan Carpenter 3 participants 15 messages Apr 19, 2018 – Apr 24, 2018

Subsystems (2)
Kernel janitors Multifunction devices (MFD)
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 mfd: tps65911-comparator: Fix an off by one bug

The tps_comparators[] array is used in two places. We only access the COMP1 (1) and COMP2 (2) elements. Unfortunately, we're accessing the wrong elements and also one element beyond the end of the array. There was supposed to be a zero element at the start of the array which is isn't accessed but makes the math work out nicely.

Fixes 6851ad3ab346 ("TPS65911: Comparator: Add comparator driver")
Signed-off-by Dan Carpenter <[email protected]>

---
I can't actually compile this code...

drivers/mfd/tps65911-comparator.c +1 -0
index c0789f81a1c5..35d7380f6fad 100644
@@ -42,6 +42,7 @@ struct comparator {
 };
 
 static struct comparator tps_comparators[] = {
+	{	.name = "COMP", },
 	{
 		.name = "COMP1",
 		.reg = TPS65911_VMBCH,
#1 lore raw
Quoted 5 lines · Dan Carpenter

The tps_comparators[] array is used in two places. We only access the COMP1 (1) and COMP2 (2) elements. Unfortunately, we're accessing the wrong elements and also one element beyond the end of the array. There was supposed to be a zero element at the start of the array which is isn't accessed but makes the math work out nicely.

I normally just apply patches from you, but this is a hack, right?

Quoted 17 lines
Fixes 6851ad3ab346 ("TPS65911: Comparator: Add comparator driver")
Signed-off-by Dan Carpenter <[email protected]>

---
I can't actually compile this code...

drivers/mfd/tps65911-comparator.c +1 -0
index c0789f81a1c5..35d7380f6fad 100644
@@ -42,6 +42,7 @@ struct comparator {
 };
 
 static struct comparator tps_comparators[] = {
+	{	.name = "COMP", },
 	{
 		.name = "COMP1",
 		.reg = TPS65911_VMBCH,
Signature · 4 lines
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
author
#2 lore raw
Quoted 10 lines · Lee Jones
Quoted 5 lines · Dan Carpenter

The tps_comparators[] array is used in two places. We only access the COMP1 (1) and COMP2 (2) elements. Unfortunately, we're accessing the wrong elements and also one element beyond the end of the array. There was supposed to be a zero element at the start of the array which is isn't accessed but makes the math work out nicely.

I normally just apply patches from you, but this is a hack, right?

I liked it, I thought it was nice. It uses 32 bytes but any fix was going to use *some* memory. I don't have strong feelings about it though, if you want to write a different patch I can do that instead.

regards,
dan carpenter

author
#3 lore raw
Quoted 9 lines · Dan Carpenter
Quoted 8 lines · Lee Jones
Quoted 5 lines · Dan Carpenter

The tps_comparators[] array is used in two places. We only access the COMP1 (1) and COMP2 (2) elements. Unfortunately, we're accessing the wrong elements and also one element beyond the end of the array. There was supposed to be a zero element at the start of the array which is isn't accessed but makes the math work out nicely.

Quoted 6 lines
Quoted 2 lines

I normally just apply patches from you, but this is a hack, right?

I liked it, I thought it was nice. It uses 32 bytes but any fix was going to use *some* memory. I don't have strong feelings about it though, if you want to write a different patch I can do that instead.

"If you want *me* to write" I meant...

regards,
dan carpenter

#4 lore raw
In reply to Dan Carpenter
Quoted 9 lines · Dan Carpenter
Quoted 8 lines · Lee Jones
Quoted 5 lines · Dan Carpenter

The tps_comparators[] array is used in two places. We only access the COMP1 (1) and COMP2 (2) elements. Unfortunately, we're accessing the wrong elements and also one element beyond the end of the array. There was supposed to be a zero element at the start of the array which is isn't accessed but makes the math work out nicely.

Quoted 5 lines
Quoted 1 line

I normally just apply patches from you, but this is a hack, right?

I liked it, I thought it was nice. It uses 32 bytes but any fix was going to use *some* memory. I don't have strong feelings about it though, if you want to write a different patch I can do that instead.

#define COMP                                    0
#define COMP1                                   1
#define COMP2                                   2

It's unclear what the defines mean, but if COMP really does exist (is there a datasheet for this device?) then your solution is a suitable one. However, if there is a COMP, then why isn't it used?

If it doesn't actually exist then this would be more appropriate change I think:

#define COMP1 0
#define COMP2 1

Signature · 4 lines
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
author
#5 lore raw
Quoted 25 lines · Lee Jones
Quoted 9 lines · Dan Carpenter
Quoted 8 lines · Lee Jones
Quoted 5 lines · Dan Carpenter

The tps_comparators[] array is used in two places. We only access the COMP1 (1) and COMP2 (2) elements. Unfortunately, we're accessing the wrong elements and also one element beyond the end of the array. There was supposed to be a zero element at the start of the array which is isn't accessed but makes the math work out nicely.

Quoted 5 lines
Quoted 1 line

I normally just apply patches from you, but this is a hack, right?

I liked it, I thought it was nice. It uses 32 bytes but any fix was going to use *some* memory. I don't have strong feelings about it though, if you want to write a different patch I can do that instead.

#define COMP                                    0
#define COMP1                                   1
#define COMP2                                   2

It's unclear what the defines mean, but if COMP really does exist (is there a datasheet for this device?) then your solution is a suitable one. However, if there is a COMP, then why isn't it used?

Quoted 6 lines

If it doesn't actually exist then this would be more appropriate change I think:

#define COMP1 0
#define COMP2 1

I hate to define something_one as zero... Let me send a different patch for this and see what you think.

regards,
dan carpenter

#6 lore raw
Quoted 29 lines · Dan Carpenter
Quoted 28 lines · Lee Jones
Quoted 9 lines · Dan Carpenter
Quoted 8 lines · Lee Jones
Quoted 5 lines · Dan Carpenter

The tps_comparators[] array is used in two places. We only access the COMP1 (1) and COMP2 (2) elements. Unfortunately, we're accessing the wrong elements and also one element beyond the end of the array. There was supposed to be a zero element at the start of the array which is isn't accessed but makes the math work out nicely.

Quoted 5 lines
Quoted 1 line

I normally just apply patches from you, but this is a hack, right?

I liked it, I thought it was nice. It uses 32 bytes but any fix was going to use *some* memory. I don't have strong feelings about it though, if you want to write a different patch I can do that instead.

#define COMP                                    0
#define COMP1                                   1
#define COMP2                                   2

It's unclear what the defines mean, but if COMP really does exist (is there a datasheet for this device?) then your solution is a suitable one. However, if there is a COMP, then why isn't it used?

If it doesn't actually exist then this would be more appropriate change I think:

Quoted 6 lines
Quoted 3 lines

#define COMP1 0
#define COMP2 1

I hate to define something_one as zero... Let me send a different patch for this and see what you think.

I guess that's the call of the hardware engineer/datasheet author?

Signature · 4 lines
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
author
#7 lore raw

PATCH v2 1/1 mfd: tps65911-comparator: Fix an off by one bug

In reply to Lee Jones

The COMP1 and COMP2 elements are in 0 and 1 respectively so this code is accessing the wrong elements and one space beyond the end of the array. We should be using "id - 1" instead.

The "id" variable is never COMP (0) so that code can be removed.

Fixes 6851ad3ab346 ("TPS65911: Comparator: Add comparator driver")
Signed-off-by Dan Carpenter <[email protected]>

---
v2: we can fix the bug and save memory.

drivers/mfd/tps65911-comparator.c +2 -9
index c0789f81a1c5..887409c3938d 100644
@@ -22,7 +22,6 @@
 #include <linux/gpio.h>
 #include <linux/mfd/tps65910.h>
 
-#define COMP					0
 #define COMP1					1
 #define COMP2					2
 

@@ -58,14 +57,11 @@ static struct comparator tps_comparators[] = {
 
 static int comp_threshold_set(struct tps65910 *tps65910, int id, int voltage)
 {
-	struct comparator tps_comp = tps_comparators[id];
+	struct comparator tps_comp = tps_comparators[id - 1];
 	int curr_voltage = 0;
 	int ret;
 	u8 index = 0, val;
 
-	if (id = COMP)
-		return 0;
-
 	while (curr_voltage < tps_comp.uV_max) {
 		curr_voltage = tps_comp.vsel_table[index];
 		if (curr_voltage >= voltage)

@@ -85,13 +81,10 @@ static int comp_threshold_set(struct tps65910 *tps65910, int id, int voltage)
 
 static int comp_threshold_get(struct tps65910 *tps65910, int id)
 {
-	struct comparator tps_comp = tps_comparators[id];
+	struct comparator tps_comp = tps_comparators[id - 1];
 	int ret;
 	u8 val;
 
-	if (id = COMP)
-		return 0;
-
 	ret = tps65910->read(tps65910, tps_comp.reg, 1, &val);
 	if (ret < 0)
 		return ret;
#8 lore raw
Quoted 10 lines · Dan Carpenter

The COMP1 and COMP2 elements are in 0 and 1 respectively so this code is accessing the wrong elements and one space beyond the end of the array. We should be using "id - 1" instead.

The "id" variable is never COMP (0) so that code can be removed.

Fixes 6851ad3ab346 ("TPS65911: Comparator: Add comparator driver")
Signed-off-by Dan Carpenter <[email protected]>

---
v2: we can fix the bug and save memory.

Looks like it's as we feared:

http://www.ti.com/lit/ds/symlink/tps65911.pdf (page 52)

There are only 2 comparators 1 and 2.

drivers/mfd/tps65911-comparator.c line 26

Quoted 11 lines
drivers/mfd/tps65911-comparator.c +0 -1
index c0789f81a1c5..887409c3938d 100644
@@ -22,7 +22,6 @@
 #include <linux/gpio.h>
 #include <linux/mfd/tps65910.h>
 
-#define COMP					0
 #define COMP1					1
 #define COMP2					2

Sorry, but I think these defines should describe the indexes into the supplied struct directly, especially as in this case it is their only reason for being.

I completely agree with you that defining something that is named '1' as '0' is not ideal, but IMHO it's better than jumping through hoops using arithmetic in array indexes.

Quoted 31 lines
drivers/mfd/tps65911-comparator.c +2 -8
@@ -58,14 +57,11 @@ static struct comparator tps_comparators[] = {
 
 static int comp_threshold_set(struct tps65910 *tps65910, int id, int voltage)
 {
-	struct comparator tps_comp = tps_comparators[id];
+	struct comparator tps_comp = tps_comparators[id - 1];
 	int curr_voltage = 0;
 	int ret;
 	u8 index = 0, val;
 
-	if (id = COMP)
-		return 0;
-
 	while (curr_voltage < tps_comp.uV_max) {
 		curr_voltage = tps_comp.vsel_table[index];
 		if (curr_voltage >= voltage)

@@ -85,13 +81,10 @@ static int comp_threshold_set(struct tps65910 *tps65910, int id, int voltage)
 
 static int comp_threshold_get(struct tps65910 *tps65910, int id)
 {
-	struct comparator tps_comp = tps_comparators[id];
+	struct comparator tps_comp = tps_comparators[id - 1];
 	int ret;
 	u8 val;
 
-	if (id = COMP)
-		return 0;
-
 	ret = tps65910->read(tps65910, tps_comp.reg, 1, &val);
 	if (ret < 0)
 		return ret;
Signature · 4 lines
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
author
#9 lore raw

I really don't want authorship credit for a patch that does:

#define COMP1 0
#define COMP2 1

It just makes me itch to look at it...

regards,
dan carpenter

#10 lore raw
Quoted 6 lines · Dan Carpenter

I really don't want authorship credit for a patch that does:

#define COMP1 0
#define COMP2 1

It just makes me itch to look at it...

As I say, I get where you're coming from, but it was the decision made by the H/W engineers. Doesn't mean it's not the correct thing to do from a S/W POV.

If you're not happy putting your SOB on the patch, I'll draft it and sign it off. /me has no shame!

Signature · 4 lines
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
author
#11 lore raw
In reply to Dan Carpenter
Quoted 10 lines · Dan Carpenter

The tps_comparators[] array is used in two places. We only access the COMP1 (1) and COMP2 (2) elements. Unfortunately, we're accessing the wrong elements and also one element beyond the end of the array. There was supposed to be a zero element at the start of the array which is isn't accessed but makes the math work out nicely.

Fixes 6851ad3ab346 ("TPS65911: Comparator: Add comparator driver")
Signed-off-by Dan Carpenter <[email protected]>

---
I can't actually compile this code...

I assumed that I couldn't compile this code because it was non-x86 or something but actually it's just really out of date. We broke it in 2012 when we removed the ->write() function pointer.

commit 3f7e82759c692df473675ed06fb90b20f1f225c3

Author: Rhyland Klein <[email protected]>
Date:   Tue May 8 11:42:38 2012 -0700

mfd: Commonize tps65910 regmap access through header

regards,
dan carpenter

author
#12 lore raw

PATCH 1/1 mfd: tps65911-comparator: Fix a build error

In 2012, we changed the tps65910 API and fixed most drivers but forgot to update this one.

Fixes 3f7e82759c69 ("mfd: Commonize tps65910 regmap access through header")
Signed-off-by Dan Carpenter <[email protected]>
drivers/mfd/tps65911-comparator.c +3 -3
index 887409c3938d..90cfdd1f88c6 100644
@@ -74,7 +74,7 @@ static int comp_threshold_set(struct tps65910 *tps65910, int id, int voltage)
 		return -EINVAL;
 
 	val = index << 1;
-	ret = tps65910->write(tps65910, tps_comp.reg, 1, &val);
+	ret = tps65910_reg_write(tps65910, tps_comp.reg, val);
 
 	return ret;
 }

@@ -82,10 +82,10 @@ static int comp_threshold_set(struct tps65910 *tps65910, int id, int voltage)
 static int comp_threshold_get(struct tps65910 *tps65910, int id)
 {
 	struct comparator tps_comp = tps_comparators[id - 1];
+	unsigned int val;
 	int ret;
-	u8 val;
 
-	ret = tps65910->read(tps65910, tps_comp.reg, 1, &val);
+	ret = tps65910_reg_read(tps65910, tps_comp.reg, &val);
 	if (ret < 0)
 		return ret;
Applied
#14 lore raw

Re: [PATCH] mfd: tps65911-comparator: Fix a build error

In reply to Dan Carpenter
Quoted 5 lines · Dan Carpenter

In 2012, we changed the tps65910 API and fixed most drivers but forgot to update this one.

Fixes 3f7e82759c69 ("mfd: Commonize tps65910 regmap access through header")
Signed-off-by Dan Carpenter <[email protected]>

Applied, thanks.

Signature · 4 lines
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog