The use_count of a regulator should only be incremented when the
enable_count changes from 0 to 1. Similarly, the use_count should
only be decremented when the enable_count changes from 1 to 0.

In the previous implementation, use_count was sometimes decremented
to 0 when some consumer called unbalanced disable,
leading to unexpected disable even the regulator is enabled by
other consumers. With this change, the use_count accurately reflects
the number of users which the regulator is enabled.

Signed-off-by: Rui Zhang <zr.zhang@vivo.com>
---
 drivers/regulator/core.c | 56 +++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 26 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 3137e40fcd3e..a7b3e548ea5a 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2918,7 +2918,8 @@ static int _regulator_enable(struct regulator *regulator)
 		/* Fallthrough on positive return values - already enabled */
 	}
 
-	rdev->use_count++;
+	if (regulator->enable_count == 1)
+		rdev->use_count++;
 
 	return 0;
 
@@ -2993,37 +2994,40 @@ static int _regulator_disable(struct regulator *regulator)
 
 	lockdep_assert_held_once(&rdev->mutex.base);
 
-	if (WARN(rdev->use_count <= 0,
+	if (WARN(regulator->enable_count == 0,
 		 "unbalanced disables for %s\n", rdev_get_name(rdev)))
 		return -EIO;
 
-	/* are we the last user and permitted to disable ? */
-	if (rdev->use_count == 1 &&
-	    (rdev->constraints && !rdev->constraints->always_on)) {
-
-		/* we are last user */
-		if (regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS)) {
-			ret = _notifier_call_chain(rdev,
-						   REGULATOR_EVENT_PRE_DISABLE,
-						   NULL);
-			if (ret & NOTIFY_STOP_MASK)
-				return -EINVAL;
-
-			ret = _regulator_do_disable(rdev);
-			if (ret < 0) {
-				rdev_err(rdev, "failed to disable: %pe\n", ERR_PTR(ret));
-				_notifier_call_chain(rdev,
-						REGULATOR_EVENT_ABORT_DISABLE,
+	if (regulator->enable_count == 1) {
+	/* disabling last enable_count from this regulator */
+		/* are we the last user and permitted to disable ? */
+		if (rdev->use_count == 1 &&
+		    (rdev->constraints && !rdev->constraints->always_on)) {
+
+			/* we are last user */
+			if (regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS)) {
+				ret = _notifier_call_chain(rdev,
+							   REGULATOR_EVENT_PRE_DISABLE,
+							   NULL);
+				if (ret & NOTIFY_STOP_MASK)
+					return -EINVAL;
+
+				ret = _regulator_do_disable(rdev);
+				if (ret < 0) {
+					rdev_err(rdev, "failed to disable: %pe\n", ERR_PTR(ret));
+					_notifier_call_chain(rdev,
+							REGULATOR_EVENT_ABORT_DISABLE,
+							NULL);
+					return ret;
+				}
+				_notifier_call_chain(rdev, REGULATOR_EVENT_DISABLE,
 						NULL);
-				return ret;
 			}
-			_notifier_call_chain(rdev, REGULATOR_EVENT_DISABLE,
-					NULL);
-		}
 
-		rdev->use_count = 0;
-	} else if (rdev->use_count > 1) {
-		rdev->use_count--;
+			rdev->use_count = 0;
+		} else if (rdev->use_count > 1) {
+			rdev->use_count--;
+		}
 	}
 
 	if (ret == 0)

base-commit: 8af4b4efdac959c02e2a82b4e2fbb8f2c0b57c1e
-- 
2.30.2

On Fri, 03 Nov 2023 15:42:31 +0800, Rui Zhang wrote:
> The use_count of a regulator should only be incremented when the
> enable_count changes from 0 to 1. Similarly, the use_count should
> only be decremented when the enable_count changes from 1 to 0.
> 
> In the previous implementation, use_count was sometimes decremented
> to 0 when some consumer called unbalanced disable,
> leading to unexpected disable even the regulator is enabled by
> other consumers. With this change, the use_count accurately reflects
> the number of users which the regulator is enabled.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/1] regulator: core: Only increment use_count when enable_count changes
      commit: 7993d3a9c34f609c02171e115fd12c10e2105ff4

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

On Fri, Nov 03, 2023 at 03:42:31PM +0800, Rui Zhang wrote:

> The use_count of a regulator should only be incremented when the
> enable_count changes from 0 to 1. Similarly, the use_count should
> only be decremented when the enable_count changes from 1 to 0.

Why?

> In the previous implementation, use_count was sometimes decremented
> to 0 when some consumer called unbalanced disable,
> leading to unexpected disable even the regulator is enabled by
> other consumers. With this change, the use_count accurately reflects
> the number of users which the regulator is enabled.

If a consumer does an unbalanced disable the consumer is buggy and the
reference counting is wrong overall.  The bug is in the consumer driver
doing the unbalanced disable.

It was very great to get feedback from you.

On 2023/11/3 20:51, Mark Brown wrote:
> On Fri, Nov 03, 2023 at 03:42:31PM +0800, Rui Zhang wrote:
>
>> The use_count of a regulator should only be incremented when the
>> enable_count changes from 0 to 1. Similarly, the use_count should
>> only be decremented when the enable_count changes from 1 to 0.
> Why?


Sorry. It might be an inappropriate expression.
I think my tone should be softer.

I believe that tracking active consumers would be an useful approach.
In case of enable/disable interfaces, the framework could benefit from
considering whether the resource should be provided particularly when
there are active consumers involved. Enabling the resource when
there is an active consumer might be generally preferable.

That's why recording the number of active consumers throught use_count
could prvide an accurate reflection of the the resource needs at that time.


>
>> In the previous implementation, use_count was sometimes decremented
>> to 0 when some consumer called unbalanced disable,
>> leading to unexpected disable even the regulator is enabled by
>> other consumers. With this change, the use_count accurately reflects
>> the number of users which the regulator is enabled.
> If a consumer does an unbalanced disable the consumer is buggy and the
> reference counting is wrong overall.  The bug is in the consumer driver
> doing the unbalanced disable.

I completely agree with you that the buggy consumer is the root cause of
the problem.

It would be better if the regulator core can prevent the spreading of the
malfunction. We all hope that failures could be limited to a minimum range
and not result in a system-wide disaster.

Addtionally, this change could make it easier to identify the buggy consumer.

For example, there are Foo_A and Foo_B as consumers, both of whom have been
enabled at some point. If Foo_B disables twice reducing the use_count to 0.
(Regulator core will complain about underflow of enable_count.)
Then after a long time, Foo_A disables, the regulator core will complain
about the unbalanced disable. It seems that Foo_A might be the buggy consumer
even it is actually the victim.
However, we would need to ensure that logs from prior to this event from
Foo_B are available to accurately determine the root cause.