[patch] regulator: max77686: fix a shift wrapping bug

[patch] regulator: max77686: fix a shift wrapping bug

[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 <dan.carpenter@oracle.com>

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index 23b7c06..59b3210 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -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,

Re: [patch] regulator: max77686: fix a shift wrapping bug

[Krzysztof Kozlowski]

2015-05-15 18:25 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> 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 <dan.carpenter@oracle.com>

Could you add "Cc: <stable@vger.kernel.org>" please?

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

Re: [patch] regulator: max77686: fix a shift wrapping bug

[Chanwoo Choi]

Hi Dan,

On 05/15/2015 06:25 PM, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
> index 23b7c06..59b3210 100644
> --- a/drivers/regulator/max77686.c
> +++ b/drivers/regulator/max77686.c
> @@ -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,
> 

Looks good to me.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

Thanks,
Chanwoo Choi

Re: [patch] regulator: max77686: fix a shift wrapping bug

[Joe Perches]

On Fri, 2015-05-15 at 19:19 +0900, Chanwoo Choi wrote:
> On 05/15/2015 06:25 PM, Dan Carpenter wrote:
> > 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 <dan.carpenter@oracle.com>

Alternate suggested patch below:

> > diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
[]
> > @@ -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))
[]
> > @@ -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);
[]
> Looks good to me.
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

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

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

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index 23b7c06..17ccf36 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -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,

Re: [patch] regulator: max77686: fix a shift wrapping bug

[Krzysztof Kozlowski]

2015-05-16 2:10 GMT+09:00 Joe Perches <joe@perches.com>:
> On Fri, 2015-05-15 at 19:19 +0900, Chanwoo Choi wrote:
>> On 05/15/2015 06:25 PM, Dan Carpenter wrote:
>> > 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 <dan.carpenter@oracle.com>
>
> Alternate suggested patch below:
>
>> > diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
> []
>> > @@ -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))
> []
>> > @@ -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);
> []
>> Looks good to me.
>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>
> 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

Re: [patch] regulator: max77686: fix a shift wrapping bug

[Dan Carpenter]

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

regards,
dan carpenter

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

[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 <dan.carpenter@oracle.com>
Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/regulator/max77686.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index 23b7c06..17ccf36 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -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,

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

[Krzysztof Kozlowski]

On 19.05.2015 02:01, Joe Perches wrote:
> 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 <dan.carpenter@oracle.com>
> Signed-off-by: Joe Perches <joe@perches.com>

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 199 bytes --]

On Mon, May 18, 2015 at 10:01:03AM -0700, Joe Perches wrote:
> The code should handle more than 32 bits here because "id"
> can be a value up to MAX77686_REGULATORS (currently 34).

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]