* [patch] regulator: max77686: fix a shift wrapping bug
@ 2015-05-15 9:25 Dan Carpenter
2015-05-15 10:12 ` Krzysztof Kozlowski
2015-05-15 10:19 ` Chanwoo Choi
0 siblings, 2 replies; 9+ messages in thread
From: Dan Carpenter @ 2015-05-15 9:25 UTC (permalink / raw)
To: Chanwoo Choi, Krzysztof Kozlowski
Cc: Liam Girdwood, Mark Brown, linux-kernel, kernel-janitors
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,
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [patch] regulator: max77686: fix a shift wrapping bug 2015-05-15 9:25 [patch] regulator: max77686: fix a shift wrapping bug Dan Carpenter @ 2015-05-15 10:12 ` Krzysztof Kozlowski 2015-05-15 10:19 ` Chanwoo Choi 1 sibling, 0 replies; 9+ messages in thread From: Krzysztof Kozlowski @ 2015-05-15 10:12 UTC (permalink / raw) To: Dan Carpenter Cc: Chanwoo Choi, Krzysztof Kozlowski, Liam Girdwood, Mark Brown, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch] regulator: max77686: fix a shift wrapping bug 2015-05-15 9:25 [patch] regulator: max77686: fix a shift wrapping bug Dan Carpenter 2015-05-15 10:12 ` Krzysztof Kozlowski @ 2015-05-15 10:19 ` Chanwoo Choi 2015-05-15 17:10 ` Joe Perches 2015-05-18 17:01 ` [PATCH] regulator: max77686: fix gpio_enabled " Joe Perches 1 sibling, 2 replies; 9+ messages in thread From: Chanwoo Choi @ 2015-05-15 10:19 UTC (permalink / raw) To: Dan Carpenter Cc: Krzysztof Kozlowski, Liam Girdwood, Mark Brown, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch] regulator: max77686: fix a shift wrapping bug 2015-05-15 10:19 ` Chanwoo Choi @ 2015-05-15 17:10 ` Joe Perches 2015-05-16 0:27 ` Krzysztof Kozlowski 2015-05-18 9:22 ` Dan Carpenter 2015-05-18 17:01 ` [PATCH] regulator: max77686: fix gpio_enabled " Joe Perches 1 sibling, 2 replies; 9+ messages in thread From: Joe Perches @ 2015-05-15 17:10 UTC (permalink / raw) To: Chanwoo Choi Cc: Dan Carpenter, Krzysztof Kozlowski, Liam Girdwood, Mark Brown, linux-kernel, kernel-janitors 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, ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [patch] regulator: max77686: fix a shift wrapping bug 2015-05-15 17:10 ` Joe Perches @ 2015-05-16 0:27 ` Krzysztof Kozlowski 2015-05-18 9:22 ` Dan Carpenter 1 sibling, 0 replies; 9+ messages in thread From: Krzysztof Kozlowski @ 2015-05-16 0:27 UTC (permalink / raw) To: Joe Perches, Dan Carpenter Cc: Chanwoo Choi, Krzysztof Kozlowski, Liam Girdwood, Mark Brown, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch] regulator: max77686: fix a shift wrapping bug 2015-05-15 17:10 ` Joe Perches 2015-05-16 0:27 ` Krzysztof Kozlowski @ 2015-05-18 9:22 ` Dan Carpenter 1 sibling, 0 replies; 9+ messages in thread From: Dan Carpenter @ 2015-05-18 9:22 UTC (permalink / raw) To: Joe Perches Cc: Chanwoo Choi, Krzysztof Kozlowski, Liam Girdwood, Mark Brown, linux-kernel, kernel-janitors That looks ok to me. Please add my Reported-by tag and resend. regards, dan carpenter ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] regulator: max77686: fix gpio_enabled shift wrapping bug 2015-05-15 10:19 ` Chanwoo Choi 2015-05-15 17:10 ` Joe Perches @ 2015-05-18 17:01 ` Joe Perches 2015-05-19 0:16 ` Krzysztof Kozlowski 2015-05-20 18:10 ` Mark Brown 1 sibling, 2 replies; 9+ messages in thread From: Joe Perches @ 2015-05-18 17:01 UTC (permalink / raw) To: Chanwoo Choi Cc: Dan Carpenter, Krzysztof Kozlowski, Liam Girdwood, Mark Brown, linux-kernel, kernel-janitors 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, ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] regulator: max77686: fix gpio_enabled shift wrapping bug 2015-05-18 17:01 ` [PATCH] regulator: max77686: fix gpio_enabled " Joe Perches @ 2015-05-19 0:16 ` Krzysztof Kozlowski 2015-05-20 18:10 ` Mark Brown 1 sibling, 0 replies; 9+ messages in thread From: Krzysztof Kozlowski @ 2015-05-19 0:16 UTC (permalink / raw) To: Joe Perches, Chanwoo Choi Cc: Dan Carpenter, Liam Girdwood, Mark Brown, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] regulator: max77686: fix gpio_enabled shift wrapping bug 2015-05-18 17:01 ` [PATCH] regulator: max77686: fix gpio_enabled " Joe Perches 2015-05-19 0:16 ` Krzysztof Kozlowski @ 2015-05-20 18:10 ` Mark Brown 1 sibling, 0 replies; 9+ messages in thread From: Mark Brown @ 2015-05-20 18:10 UTC (permalink / raw) To: Joe Perches Cc: Chanwoo Choi, Dan Carpenter, Krzysztof Kozlowski, Liam Girdwood, linux-kernel, kernel-janitors [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-05-20 18:10 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-05-15 9:25 [patch] regulator: max77686: fix a shift wrapping bug Dan Carpenter 2015-05-15 10:12 ` Krzysztof Kozlowski 2015-05-15 10:19 ` Chanwoo Choi 2015-05-15 17:10 ` Joe Perches 2015-05-16 0:27 ` Krzysztof Kozlowski 2015-05-18 9:22 ` Dan Carpenter 2015-05-18 17:01 ` [PATCH] regulator: max77686: fix gpio_enabled " Joe Perches 2015-05-19 0:16 ` Krzysztof Kozlowski 2015-05-20 18:10 ` Mark Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).