regulator: max77686: fix a shift wrapping bug

PATCH In review Dan Carpenter 5 participants 9 messages May 15, 2015 – May 20, 2015

Subsystems (3)

Kernel janitors Voltage and current regulator framework Maxim PMIC and MUIC drivers for Exynos based boards

lore

Share & tools

lore

Dan Carpenter

author

2015-05-15 09:25 UTC #0 lore raw

PATCH 1/1 regulator: max77686: fix a shift wrapping bug

We need to be able to handle more than 32 bits here because "id" can go up to MAX77686_BUCK9 (34). ->gpio_enabled is a u64 so that's fine already.

Fixes 3307e9025d29 ('regulator: max77686: Add GPIO control')

Signed-off-by Dan Carpenter <[email protected]>

drivers/regulator/max77686.c +2 -2

index 23b7c06..59b3210 100644
@@ -121,7 +121,7 @@ static unsigned int max77686_map_normal_mode(struct max77686_data *max77686,
 	case MAX77686_BUCK8:
 	case MAX77686_BUCK9:
 	case MAX77686_LDO20 ... MAX77686_LDO22:
-		if (max77686->gpio_enabled & (1 << id))
+		if (max77686->gpio_enabled & (1ULL << id))
 			return MAX77686_GPIO_CONTROL;
 	}

@@ -277,7 +277,7 @@ static int max77686_of_parse_cb(struct device_node *np,
 	}
 
 	if (gpio_is_valid(config->ena_gpio)) {
-		max77686->gpio_enabled |= (1 << desc->id);
+		max77686->gpio_enabled |= (1ULL << desc->id);
 
 		return regmap_update_bits(config->regmap, desc->enable_reg,
 					  desc->enable_mask,

Krzysztof Kozlowski

Reviewed-by on v1 1/1

2015-05-15 10:12 UTC #1 lore raw

2015-05-15 18:25 GMT+09:00 Dan Carpenter <[email protected]>:

Quoted 6 lines

We need to be able to handle more than 32 bits here because "id" can go up to MAX77686_BUCK9 (34). ->gpio_enabled is a u64 so that's fine already.
Fixes 3307e9025d29 ('regulator: max77686: Add GPIO control')
Signed-off-by Dan Carpenter <[email protected]>

Could you add "Cc: <[email protected]>" please?

Reviewed-by Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

Chanwoo Choi

maintainer Reviewed-by on v1 1/1

2015-05-15 10:19 UTC #2 lore raw

In reply to Dan Carpenter

Hi Dan,

Quoted 22 lines · Dan Carpenter

We need to be able to handle more than 32 bits here because "id" can go up to MAX77686_BUCK9 (34). ->gpio_enabled is a u64 so that's fine already.
Fixes 3307e9025d29 ('regulator: max77686: Add GPIO control')
Signed-off-by Dan Carpenter <[email protected]>
drivers/regulator/max77686.c +1 -1
index 23b7c06..59b3210 100644
@@ -121,7 +121,7 @@ static unsigned int max77686_map_normal_mode(struct max77686_data *max77686,
 	case MAX77686_BUCK8:
 	case MAX77686_BUCK9:
 	case MAX77686_LDO20 ... MAX77686_LDO22:
-		if (max77686->gpio_enabled & (1 << id))
+		if (max77686->gpio_enabled & (1ULL << id))
 			return MAX77686_GPIO_CONTROL;
 	}
 

drivers/regulator/max77686.c static int max77686_of_parse_cb(struct device_node *np, · line 283

Quoted 8 lines

drivers/regulator/max77686.c +1 -1

@@ -277,7 +277,7 @@ static int max77686_of_parse_cb(struct device_node *np,
 	}
 
 	if (gpio_is_valid(config->ena_gpio)) {
-		max77686->gpio_enabled |= (1 << desc->id);
+		max77686->gpio_enabled |= (1ULL << desc->id);
 
 		return regmap_update_bits(config->regmap, desc->enable_reg,
 					  desc->enable_mask,

Looks good to me.

Reviewed-by Chanwoo Choi <[email protected]>

Thanks,
Chanwoo Choi

Joe Perches

2015-05-15 17:10 UTC #3 lore raw

Quoted 7 lines · Chanwoo Choi

Quoted 6 lines · Dan Carpenter
We need to be able to handle more than 32 bits here because "id" can go up to MAX77686_BUCK9 (34). ->gpio_enabled is a u64 so that's fine already.
Fixes 3307e9025d29 ('regulator: max77686: Add GPIO control')
Signed-off-by Dan Carpenter <[email protected]>

Alternate suggested patch below:

Quoted 1 line

drivers/regulator/max77686.c
Quoted 1 line
drivers/regulator/max77686.c +0 -0

[]

Quoted 3 lines

drivers/regulator/max77686.c static unsigned int max77686_map_normal_mode(struct max77686_data *max77686, · line 121
Quoted 3 lines
drivers/regulator/max77686.c +1 -1
@@ -121,7 +121,7 @@ static unsigned int max77686_map_normal_mode(struct max77686_data *max77686,
-		if (max77686->gpio_enabled & (1 << id))
+		if (max77686->gpio_enabled & (1ULL << id))

[]

Quoted 4 lines

drivers/regulator/max77686.c static int max77686_of_parse_cb(struct device_node *np, · line 278
Quoted 4 lines
drivers/regulator/max77686.c +1 -1
@@ -277,7 +277,7 @@ static int max77686_of_parse_cb(struct device_node *np,
 	if (gpio_is_valid(config->ena_gpio)) {
-		max77686->gpio_enabled |= (1 << desc->id);
+		max77686->gpio_enabled |= (1ULL << desc->id);

[]

Quoted 2 lines

Looks good to me.
Reviewed-by Chanwoo Choi <[email protected]>

This could be better with DECLARE_BITMAP and test_bit/set_bit
---

 drivers/regulator/max77686.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/regulator/max77686.c +3 -3

index 23b7c06..17ccf36 100644
@@ -88,7 +88,7 @@ enum max77686_ramp_rate {
 };
 
 struct max77686_data {
-	u64 gpio_enabled:MAX77686_REGULATORS;
+	DECLARE_BITMAP(gpio_enabled, MAX77686_REGULATORS);
 
 	/* Array indexed by regulator id */
 	unsigned int opmode[MAX77686_REGULATORS];

@@ -121,7 +121,7 @@ static unsigned int max77686_map_normal_mode(struct max77686_data *max77686,
 	case MAX77686_BUCK8:
 	case MAX77686_BUCK9:
 	case MAX77686_LDO20 ... MAX77686_LDO22:
-		if (max77686->gpio_enabled & (1 << id))
+		if (test_bit(id, max77686->gpio_enabled))
 			return MAX77686_GPIO_CONTROL;
 	}

@@ -277,7 +277,7 @@ static int max77686_of_parse_cb(struct device_node *np,
 	}
 
 	if (gpio_is_valid(config->ena_gpio)) {
-		max77686->gpio_enabled |= (1 << desc->id);
+		set_bit(desc->id, max77686->gpio_enabled);
 
 		return regmap_update_bits(config->regmap, desc->enable_reg,
 					  desc->enable_mask,

Krzysztof Kozlowski

2015-05-16 00:27 UTC #4 lore raw

2015-05-16 2:10 GMT+09:00 Joe Perches <[email protected]>:

Quoted 22 lines · Chanwoo Choi

Quoted 7 lines · Dan Carpenter
Quoted 6 lines
We need to be able to handle more than 32 bits here because "id" can go up to MAX77686_BUCK9 (34). ->gpio_enabled is a u64 so that's fine already.
Fixes 3307e9025d29 ('regulator: max77686: Add GPIO control')
Signed-off-by Dan Carpenter <[email protected]>
Alternate suggested patch below:
Quoted 1 line
drivers/regulator/max77686.c
Quoted 1 line
drivers/regulator/max77686.c +0 -0
[]
Quoted 3 lines
drivers/regulator/max77686.c static unsigned int max77686_map_normal_mode(struct max77686_data *max77686, · line 121
Quoted 3 lines
drivers/regulator/max77686.c +1 -1
@@ -121,7 +121,7 @@ static unsigned int max77686_map_normal_mode(struct max77686_data *max77686,
-           if (max77686->gpio_enabled & (1 << id))
+           if (max77686->gpio_enabled & (1ULL << id))
[]
Quoted 4 lines
drivers/regulator/max77686.c static int max77686_of_parse_cb(struct device_node *np, · line 278
Quoted 4 lines
drivers/regulator/max77686.c +1 -1
@@ -277,7 +277,7 @@ static int max77686_of_parse_cb(struct device_node *np,
    if (gpio_is_valid(config->ena_gpio)) {
-           max77686->gpio_enabled |= (1 << desc->id);
+           max77686->gpio_enabled |= (1ULL << desc->id);
[]

Quoted 4 lines

Quoted 2 lines
Looks good to me.
Reviewed-by Chanwoo Choi <[email protected]>
This could be better with DECLARE_BITMAP and test_bit/set_bit

Yes, this looks better - it clearly shows the purpose of "gpio_enabled" member. Joe or Dan, can you resend with new solution and respective tags? (Cc stable, reported-by Dan if patch comes from Joe)

Anyway my reviewed-by may stay on for both solutions.

Thanks,
Krzysztof

Dan Carpenter

author

2015-05-18 09:22 UTC #5 lore raw

In reply to Joe Perches

That looks ok to me. Please add my Reported-by tag and resend.

regards,
dan carpenter

Joe Perches

2015-05-18 17:01 UTC #6 lore raw

PATCH 1/1 regulator: max77686: fix gpio_enabled shift wrapping bug

In reply to Chanwoo Choi

The code should handle more than 32 bits here because "id" can be a value up to MAX77686_REGULATORS (currently 34).

Convert the gpio_enabled type to DECLARE_BITMAP and use test_bit/set_bit.

Fixes 3307e9025d29 ("regulator: max77686: Add GPIO control")

Reported-by Dan Carpenter <[email protected]>

Signed-off-by Joe Perches <[email protected]>

---
 drivers/regulator/max77686.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/regulator/max77686.c +3 -3

index 23b7c06..17ccf36 100644
@@ -88,7 +88,7 @@ enum max77686_ramp_rate {
 };
 
 struct max77686_data {
-	u64 gpio_enabled:MAX77686_REGULATORS;
+	DECLARE_BITMAP(gpio_enabled, MAX77686_REGULATORS);
 
 	/* Array indexed by regulator id */
 	unsigned int opmode[MAX77686_REGULATORS];

@@ -121,7 +121,7 @@ static unsigned int max77686_map_normal_mode(struct max77686_data *max77686,
 	case MAX77686_BUCK8:
 	case MAX77686_BUCK9:
 	case MAX77686_LDO20 ... MAX77686_LDO22:
-		if (max77686->gpio_enabled & (1 << id))
+		if (test_bit(id, max77686->gpio_enabled))
 			return MAX77686_GPIO_CONTROL;
 	}

@@ -277,7 +277,7 @@ static int max77686_of_parse_cb(struct device_node *np,
 	}
 
 	if (gpio_is_valid(config->ena_gpio)) {
-		max77686->gpio_enabled |= (1 << desc->id);
+		set_bit(desc->id, max77686->gpio_enabled);
 
 		return regmap_update_bits(config->regmap, desc->enable_reg,
 					  desc->enable_mask,

Krzysztof Kozlowski

Reviewed-by Tested-by on v1 1/1

2015-05-19 00:16 UTC #7 lore raw

Re: [PATCH] regulator: max77686: fix gpio_enabled shift wrapping bug

Quoted 9 lines · Joe Perches

The code should handle more than 32 bits here because "id" can be a value up to MAX77686_REGULATORS (currently 34).
Convert the gpio_enabled type to DECLARE_BITMAP and use test_bit/set_bit.
Fixes 3307e9025d29 ("regulator: max77686: Add GPIO control")
Reported-by Dan Carpenter <[email protected]>
Signed-off-by Joe Perches <[email protected]>

Reviewed-by Krzysztof Kozlowski <[email protected]>

Tested-by Krzysztof Kozlowski <[email protected]>

Again - why you do not add CC-stable? If you need observational bug effect to the commit message you can add:

<<When max77686 regulator was configured to GPIO controlled mode, the overflow of 32 bit regulator ID could switch off completely different regulators. This could happen because for other regulators the value of "GPIO controlled mode" means "disabled".>>

Although I did not saw such effect of overflow in testing, it actually should happen looking at the code.

Best regards,
Krzysztof

Mark Brown

maintainer Applied

2015-05-20 18:10 UTC #8 lore raw

Re: [PATCH] regulator: max77686: fix gpio_enabled shift wrapping bug

In reply to Joe Perches

Quoted 2 lines · Joe Perches

The code should handle more than 32 bits here because "id" can be a value up to MAX77686_REGULATORS (currently 34).

Applied, thanks.