[PATCH] regulator: s2mps11: Fix GPIO suspend enable shift wrapping bug

[PATCH] regulator: s2mps11: Fix GPIO suspend enable shift wrapping bug

[Krzysztof Kozlowski]

Status of enabling suspend mode for regulator was stored in bitmap-like
long integer.

However since adding support for S2MPU02 the number of regulators
exceeded 32 so on devices with more than 32 regulators (S2MPU02 and
S2MPS13) overflow happens when shifting the bit. This could lead to
enabling suspend mode for completely different regulator than intended
or to switching different regulator to other mode (e.g. from always
enabled to controlled by PWRHOLD pin). Both cases could result in larger
energy usage and issues when suspending to RAM.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: <stable@vger.kernel.org>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Fixes: 00e2573d2c10 ("regulator: s2mps11: Add support S2MPU02 regulator device")
---
 drivers/regulator/s2mps11.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
index 326ffb553371..72fc3c32db49 100644
--- a/drivers/regulator/s2mps11.c
+++ b/drivers/regulator/s2mps11.c
@@ -34,6 +34,8 @@
 #include <linux/mfd/samsung/s2mps14.h>
 #include <linux/mfd/samsung/s2mpu02.h>
 
+/* The highest number of possible regulators for supported devices. */
+#define S2MPS_REGULATOR_MAX		S2MPS13_REGULATOR_MAX
 struct s2mps11_info {
 	unsigned int rdev_num;
 	int ramp_delay2;
@@ -49,7 +51,7 @@ struct s2mps11_info {
 	 * One bit for each S2MPS13/S2MPS14/S2MPU02 regulator whether
 	 * the suspend mode was enabled.
 	 */
-	unsigned long long s2mps14_suspend_state:50;
+	DECLARE_BITMAP(suspend_state, S2MPS_REGULATOR_MAX);
 
 	/* Array of size rdev_num with GPIO-s for external sleep control */
 	int *ext_control_gpio;
@@ -500,7 +502,7 @@ static int s2mps14_regulator_enable(struct regulator_dev *rdev)
 	switch (s2mps11->dev_type) {
 	case S2MPS13X:
 	case S2MPS14X:
-		if (s2mps11->s2mps14_suspend_state & (1 << rdev_get_id(rdev)))
+		if (test_bit(rdev_get_id(rdev), s2mps11->suspend_state))
 			val = S2MPS14_ENABLE_SUSPEND;
 		else if (gpio_is_valid(s2mps11->ext_control_gpio[rdev_get_id(rdev)]))
 			val = S2MPS14_ENABLE_EXT_CONTROL;
@@ -508,7 +510,7 @@ static int s2mps14_regulator_enable(struct regulator_dev *rdev)
 			val = rdev->desc->enable_mask;
 		break;
 	case S2MPU02:
-		if (s2mps11->s2mps14_suspend_state & (1 << rdev_get_id(rdev)))
+		if (test_bit(rdev_get_id(rdev), s2mps11->suspend_state))
 			val = S2MPU02_ENABLE_SUSPEND;
 		else
 			val = rdev->desc->enable_mask;
@@ -562,7 +564,7 @@ static int s2mps14_regulator_set_suspend_disable(struct regulator_dev *rdev)
 	if (ret < 0)
 		return ret;
 
-	s2mps11->s2mps14_suspend_state |= (1 << rdev_get_id(rdev));
+	set_bit(rdev_get_id(rdev), s2mps11->suspend_state);
 	/*
 	 * Don't enable suspend mode if regulator is already disabled because
 	 * this would effectively for a short time turn on the regulator after
@@ -960,18 +962,22 @@ static int s2mps11_pmic_probe(struct platform_device *pdev)
 	case S2MPS11X:
 		s2mps11->rdev_num = ARRAY_SIZE(s2mps11_regulators);
 		regulators = s2mps11_regulators;
+		BUILD_BUG_ON(S2MPS_REGULATOR_MAX < s2mps11->rdev_num);
 		break;
 	case S2MPS13X:
 		s2mps11->rdev_num = ARRAY_SIZE(s2mps13_regulators);
 		regulators = s2mps13_regulators;
+		BUILD_BUG_ON(S2MPS_REGULATOR_MAX < s2mps11->rdev_num);
 		break;
 	case S2MPS14X:
 		s2mps11->rdev_num = ARRAY_SIZE(s2mps14_regulators);
 		regulators = s2mps14_regulators;
+		BUILD_BUG_ON(S2MPS_REGULATOR_MAX < s2mps11->rdev_num);
 		break;
 	case S2MPU02:
 		s2mps11->rdev_num = ARRAY_SIZE(s2mpu02_regulators);
 		regulators = s2mpu02_regulators;
+		BUILD_BUG_ON(S2MPS_REGULATOR_MAX < s2mps11->rdev_num);
 		break;
 	default:
 		dev_err(&pdev->dev, "Invalid device type: %u\n",
-- 
1.9.1

Re: [PATCH] regulator: s2mps11: Fix GPIO suspend enable shift wrapping bug

[Mark Brown]

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

On Wed, May 27, 2015 at 12:22:08PM +0900, Krzysztof Kozlowski wrote:
> Status of enabling suspend mode for regulator was stored in bitmap-like
> long integer.

Applied, thanks.

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

Re: [PATCH] regulator: s2mps11: Fix GPIO suspend enable shift wrapping bug

[Krzysztof Kozlowski]

2015-05-28 23:44 GMT+09:00 Mark Brown <broonie@kernel.org>:
> On Wed, May 27, 2015 at 12:22:08PM +0900, Krzysztof Kozlowski wrote:
>> Status of enabling suspend mode for regulator was stored in bitmap-like
>> long integer.
>
> Applied, thanks.

Mark, what happened with this patch? I can't find it in your tree.

Re: [PATCH] regulator: s2mps11: Fix GPIO suspend enable shift wrapping bug

[Mark Brown]

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

On Wed, Jun 24, 2015 at 01:02:28PM +0900, Krzysztof Kozlowski wrote:
> 2015-05-28 23:44 GMT+09:00 Mark Brown <broonie@kernel.org>:
> > On Wed, May 27, 2015 at 12:22:08PM +0900, Krzysztof Kozlowski wrote:
> >> Status of enabling suspend mode for regulator was stored in bitmap-like
> >> long integer.

> > Applied, thanks.

> Mark, what happened with this patch? I can't find it in your tree.

I don't know.  If it's not there it's not there and you'll need to
resend, sorry.  There were some problems with error handling for bad
networks when pushing things but it looks like this was after I fixed
those.

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

Re: [PATCH] regulator: s2mps11: Fix GPIO suspend enable shift wrapping bug

[Krzysztof Kozlowski]

2015-06-24 18:31 GMT+09:00 Mark Brown <broonie@kernel.org>:
> On Wed, Jun 24, 2015 at 01:02:28PM +0900, Krzysztof Kozlowski wrote:
>> 2015-05-28 23:44 GMT+09:00 Mark Brown <broonie@kernel.org>:
>> > On Wed, May 27, 2015 at 12:22:08PM +0900, Krzysztof Kozlowski wrote:
>> >> Status of enabling suspend mode for regulator was stored in bitmap-like
>> >> long integer.
>
>> > Applied, thanks.
>
>> Mark, what happened with this patch? I can't find it in your tree.
>
> I don't know.  If it's not there it's not there and you'll need to
> resend, sorry.  There were some problems with error handling for bad
> networks when pushing things but it looks like this was after I fixed
> those.

Okay, it happens. If you don't mind, I'll resend now, during merge
window. If it is bad timing, then let me know or postpone the patch.
This is a fix so in my humble opinion it's a candidate for 4.2-rc
(although it would be nice if this could sit in linux-next for a
while).

Best regards,
Krzysztof

Applied "regulator: s2mps11: Fix GPIO suspend enable shift wrapping bug" to the regulator tree

[Mark Brown]

The patch

   regulator: s2mps11: Fix GPIO suspend enable shift wrapping bug

has been applied to the regulator tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 32c848e33ace75fce388cceff76223d12b46eaa3 Mon Sep 17 00:00:00 2001
From: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Date: Wed, 24 Jun 2015 19:48:43 +0900
Subject: [PATCH] regulator: s2mps11: Fix GPIO suspend enable shift wrapping
 bug

Status of enabling suspend mode for regulator was stored in bitmap-like
long integer.

However since adding support for S2MPU02 the number of regulators
exceeded 32 so on devices with more than 32 regulators (S2MPU02 and
S2MPS13) overflow happens when shifting the bit. This could lead to
enabling suspend mode for completely different regulator than intended
or to switching different regulator to other mode (e.g. from always
enabled to controlled by PWRHOLD pin). Both cases could result in larger
energy usage and issues when suspending to RAM.

Fixes: 00e2573d2c10 ("regulator: s2mps11: Add support S2MPU02 regulator device")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
Cc: <stable@vger.kernel.org>
---
 drivers/regulator/s2mps11.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
index 326ffb553371..72fc3c32db49 100644
--- a/drivers/regulator/s2mps11.c
+++ b/drivers/regulator/s2mps11.c
@@ -34,6 +34,8 @@
 #include <linux/mfd/samsung/s2mps14.h>
 #include <linux/mfd/samsung/s2mpu02.h>
 
+/* The highest number of possible regulators for supported devices. */
+#define S2MPS_REGULATOR_MAX		S2MPS13_REGULATOR_MAX
 struct s2mps11_info {
 	unsigned int rdev_num;
 	int ramp_delay2;
@@ -49,7 +51,7 @@ struct s2mps11_info {
 	 * One bit for each S2MPS13/S2MPS14/S2MPU02 regulator whether
 	 * the suspend mode was enabled.
 	 */
-	unsigned long long s2mps14_suspend_state:50;
+	DECLARE_BITMAP(suspend_state, S2MPS_REGULATOR_MAX);
 
 	/* Array of size rdev_num with GPIO-s for external sleep control */
 	int *ext_control_gpio;
@@ -500,7 +502,7 @@ static int s2mps14_regulator_enable(struct regulator_dev *rdev)
 	switch (s2mps11->dev_type) {
 	case S2MPS13X:
 	case S2MPS14X:
-		if (s2mps11->s2mps14_suspend_state & (1 << rdev_get_id(rdev)))
+		if (test_bit(rdev_get_id(rdev), s2mps11->suspend_state))
 			val = S2MPS14_ENABLE_SUSPEND;
 		else if (gpio_is_valid(s2mps11->ext_control_gpio[rdev_get_id(rdev)]))
 			val = S2MPS14_ENABLE_EXT_CONTROL;
@@ -508,7 +510,7 @@ static int s2mps14_regulator_enable(struct regulator_dev *rdev)
 			val = rdev->desc->enable_mask;
 		break;
 	case S2MPU02:
-		if (s2mps11->s2mps14_suspend_state & (1 << rdev_get_id(rdev)))
+		if (test_bit(rdev_get_id(rdev), s2mps11->suspend_state))
 			val = S2MPU02_ENABLE_SUSPEND;
 		else
 			val = rdev->desc->enable_mask;
@@ -562,7 +564,7 @@ static int s2mps14_regulator_set_suspend_disable(struct regulator_dev *rdev)
 	if (ret < 0)
 		return ret;
 
-	s2mps11->s2mps14_suspend_state |= (1 << rdev_get_id(rdev));
+	set_bit(rdev_get_id(rdev), s2mps11->suspend_state);
 	/*
 	 * Don't enable suspend mode if regulator is already disabled because
 	 * this would effectively for a short time turn on the regulator after
@@ -960,18 +962,22 @@ static int s2mps11_pmic_probe(struct platform_device *pdev)
 	case S2MPS11X:
 		s2mps11->rdev_num = ARRAY_SIZE(s2mps11_regulators);
 		regulators = s2mps11_regulators;
+		BUILD_BUG_ON(S2MPS_REGULATOR_MAX < s2mps11->rdev_num);
 		break;
 	case S2MPS13X:
 		s2mps11->rdev_num = ARRAY_SIZE(s2mps13_regulators);
 		regulators = s2mps13_regulators;
+		BUILD_BUG_ON(S2MPS_REGULATOR_MAX < s2mps11->rdev_num);
 		break;
 	case S2MPS14X:
 		s2mps11->rdev_num = ARRAY_SIZE(s2mps14_regulators);
 		regulators = s2mps14_regulators;
+		BUILD_BUG_ON(S2MPS_REGULATOR_MAX < s2mps11->rdev_num);
 		break;
 	case S2MPU02:
 		s2mps11->rdev_num = ARRAY_SIZE(s2mpu02_regulators);
 		regulators = s2mpu02_regulators;
+		BUILD_BUG_ON(S2MPS_REGULATOR_MAX < s2mps11->rdev_num);
 		break;
 	default:
 		dev_err(&pdev->dev, "Invalid device type: %u\n",
-- 
2.1.4