[PATCH 1/2] regulator: core: Sleep (not delay) in set_voltage()

[PATCH 1/2] regulator: core: Sleep (not delay) in set_voltage()

[Brian Norris]

These delays can be relatively large (e.g., hundreds of microseconds on
RK3399 Gru systems). Per Documentation/timers/timers-howto.rst, that
should usually use a sleeping delay. Let's use fsleep() to handle both
large and small delays appropriately. This avoids burning a bunch of CPU
time and hurting scheduling latencies when hitting regulators a lot
(e.g., during cpufreq).

The sleep vs. delay issue choice has been made differently over time --
early versions of RK3399 Gru PWM-regulator support used usleep_range()
in pwm-regulator.c. More of this got moved into the regulator core,
in commits like:

73e705bf81ce regulator: core: Add set_voltage_time op

At the same time, the sleep turned into a delay.

It's OK to sleep here, as we aren't in an atomic contexts. (All our
callers grab various mutexes already.)

Signed-off-by: Brian Norris <briannorris@chromium.org>
---

 drivers/regulator/core.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d2553970a67b..223c6d71a2b2 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3548,12 +3548,7 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 	}
 
 	/* Insert any necessary delays */
-	if (delay >= 1000) {
-		mdelay(delay / 1000);
-		udelay(delay % 1000);
-	} else if (delay) {
-		udelay(delay);
-	}
+	fsleep(delay);
 
 	if (best_val >= 0) {
 		unsigned long data = best_val;
-- 
2.36.0.rc0.470.gd361397f0d-goog

[PATCH 2/2] regulator: core: Replace _regulator_enable_delay() with fsleep()

[Brian Norris]

fsleep() was designed to handle exactly the same thing as
_regulator_enable_delay(): flexible sleep lengths, according to the
guidelines at Documentation/timers/timers-howto.rst. Let's use it,
instead of duplicating it.

One notable difference: fsleep() allows a usleep range of twice the
requested amount instead of a fixed +100us.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---

 drivers/regulator/core.c | 45 +++-------------------------------------
 1 file changed, 3 insertions(+), 42 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 223c6d71a2b2..d0bac80206d8 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2510,45 +2510,6 @@ static int regulator_ena_gpio_ctrl(struct regulator_dev *rdev, bool enable)
 	return 0;
 }
 
-/**
- * _regulator_enable_delay - a delay helper function
- * @delay: time to delay in microseconds
- *
- * Delay for the requested amount of time as per the guidelines in:
- *
- *     Documentation/timers/timers-howto.rst
- *
- * The assumption here is that regulators will never be enabled in
- * atomic context and therefore sleeping functions can be used.
- */
-static void _regulator_enable_delay(unsigned int delay)
-{
-	unsigned int ms = delay / 1000;
-	unsigned int us = delay % 1000;
-
-	if (ms > 0) {
-		/*
-		 * For small enough values, handle super-millisecond
-		 * delays in the usleep_range() call below.
-		 */
-		if (ms < 20)
-			us += ms * 1000;
-		else
-			msleep(ms);
-	}
-
-	/*
-	 * Give the scheduler some room to coalesce with any other
-	 * wakeup sources. For delays shorter than 10 us, don't even
-	 * bother setting up high-resolution timers and just busy-
-	 * loop.
-	 */
-	if (us >= 10)
-		usleep_range(us, us + 100);
-	else
-		udelay(us);
-}
-
 /**
  * _regulator_check_status_enabled
  *
@@ -2603,7 +2564,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 		s64 remaining = ktime_us_delta(end, ktime_get());
 
 		if (remaining > 0)
-			_regulator_enable_delay(remaining);
+			fsleep(remaining);
 	}
 
 	if (rdev->ena_pin) {
@@ -2637,7 +2598,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 		unsigned int time_remaining = delay;
 
 		while (time_remaining > 0) {
-			_regulator_enable_delay(rdev->desc->poll_enabled_time);
+			fsleep(rdev->desc->poll_enabled_time);
 
 			if (rdev->desc->ops->get_status) {
 				ret = _regulator_check_status_enabled(rdev);
@@ -2656,7 +2617,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 			return -ETIMEDOUT;
 		}
 	} else {
-		_regulator_enable_delay(delay);
+		fsleep(delay);
 	}
 
 	trace_regulator_enable_complete(rdev_get_name(rdev));
-- 
2.36.0.rc0.470.gd361397f0d-goog

Re: [PATCH 2/2] regulator: core: Replace _regulator_enable_delay() with fsleep()

[Matthias Kaehlcke]

On Mon, Apr 18, 2022 at 02:12:40PM -0700, Brian Norris wrote:
> fsleep() was designed to handle exactly the same thing as
> _regulator_enable_delay(): flexible sleep lengths, according to the
> guidelines at Documentation/timers/timers-howto.rst. Let's use it,
> instead of duplicating it.
> 
> One notable difference: fsleep() allows a usleep range of twice the
> requested amount instead of a fixed +100us.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH 2/2] regulator: core: Replace _regulator_enable_delay() with fsleep()

[Mark Brown]

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

On Mon, Apr 18, 2022 at 02:12:40PM -0700, Brian Norris wrote:

> One notable difference: fsleep() allows a usleep range of twice the
> requested amount instead of a fixed +100us.

Did the issue with the delay functions preferring delays on the higher
end of the allowed range get fixed?  That might be an issue for larger
usleep() values.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] regulator: core: Replace _regulator_enable_delay() with fsleep()

[Brian Norris]

Hi Mark,

On Wed, Apr 20, 2022 at 05:28:46PM +0100, Mark Brown wrote:
> Did the issue with the delay functions preferring delays on the higher
> end of the allowed range get fixed?  That might be an issue for larger
> usleep() values.

Hmm, good question. I had a faint memory of this problem, and searching
around, I couldn't find that anybody *thought* they fixed it, and I
found evidence to the contrary (some reports complaining about, e.g.,
boot-time performance issues in drivers/usb due to the same, with no
indication that anybody truly fixed the problem).

And measurement on my systems (with expected usecs between 322 and 390)
show that we overshoot by the following stats (on >3000 samples, with
moderate load):

 minimum overshoot: ~3%
 maximum overshoot: ~150%
 median overshoot: ~104%
 mean overshoot: ~98%
 stddev: 0.207

I guess this is one aspect that Documentation/timers/timers-howto.rst is
referring to, when it says: "Exact tolerances here are very situation
specific, thus it is left to the caller to determine a reasonable
range." It feels like fsleep()'s "x2" is pretty arbitrary and often not
what people want, but maybe it's good enough for non-sensitive cases.

So maybe it's better to retain the regulator core helper
(_regulator_enable_delay()) and rename/repurpose it for my patch 1?

I feel like there's some room for improvement in either fsleep() or
usleep_range() or both, but I'm not sure exactly how to go about that
right now.

Brian

Re: [PATCH 2/2] regulator: core: Replace _regulator_enable_delay() with fsleep()

[Mark Brown]

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

On Wed, Apr 20, 2022 at 11:24:08AM -0700, Brian Norris wrote:
> On Wed, Apr 20, 2022 at 05:28:46PM +0100, Mark Brown wrote:

> > Did the issue with the delay functions preferring delays on the higher
> > end of the allowed range get fixed?  That might be an issue for larger
> > usleep() values.

> Hmm, good question. I had a faint memory of this problem, and searching
> around, I couldn't find that anybody *thought* they fixed it, and I
> found evidence to the contrary (some reports complaining about, e.g.,
> boot-time performance issues in drivers/usb due to the same, with no
> indication that anybody truly fixed the problem).

That's what I feared :/

> So maybe it's better to retain the regulator core helper
> (_regulator_enable_delay()) and rename/repurpose it for my patch 1?

Sounds like a plan.

> I feel like there's some room for improvement in either fsleep() or
> usleep_range() or both, but I'm not sure exactly how to go about that
> right now.

It's really difficult to design an API that's both tasteful and clear
about intent - it's relatively easy for something like a driver that
just has a single hard coded value so you can just use usleep_range()
directly but it gets awkward once you start being generic and the code
really has no idea what the actual delay it's dealing with is.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] regulator: core: Sleep (not delay) in set_voltage()

[Matthias Kaehlcke]

On Mon, Apr 18, 2022 at 02:12:39PM -0700, Brian Norris wrote:
> These delays can be relatively large (e.g., hundreds of microseconds on
> RK3399 Gru systems). Per Documentation/timers/timers-howto.rst, that
> should usually use a sleeping delay. Let's use fsleep() to handle both
> large and small delays appropriately. This avoids burning a bunch of CPU
> time and hurting scheduling latencies when hitting regulators a lot
> (e.g., during cpufreq).
> 
> The sleep vs. delay issue choice has been made differently over time --
> early versions of RK3399 Gru PWM-regulator support used usleep_range()
> in pwm-regulator.c. More of this got moved into the regulator core,
> in commits like:
> 
> 73e705bf81ce regulator: core: Add set_voltage_time op
> 
> At the same time, the sleep turned into a delay.
> 
> It's OK to sleep here, as we aren't in an atomic contexts. (All our
> callers grab various mutexes already.)
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>