From b407460ee99033503993ac7437d593451fcdfe44 Mon Sep 17 00:00:00 2001 From: Geert Uytterhoeven Date: Fri, 2 Jun 2023 10:50:36 +0200 Subject: [PATCH 1/3] iopoll: Call cpu_relax() in busy loops It is considered good practice to call cpu_relax() in busy loops, see Documentation/process/volatile-considered-harmful.rst. This can not only lower CPU power consumption or yield to a hyperthreaded twin processor, but also allows an architecture to mitigate hardware issues (e.g. ARM Erratum 754327 for Cortex-A9 prior to r2p0) in the architecture-specific cpu_relax() implementation. In addition, cpu_relax() is also a compiler barrier. It is not immediately obvious that the @op argument "function" will result in an actual function call (e.g. in case of inlining). Where a function call is a C sequence point, this is lost on inlining. Therefore, with agressive enough optimization it might be possible for the compiler to hoist the: (val) = op(args); "load" out of the loop because it doesn't see the value changing. The addition of cpu_relax() would inhibit this. As the iopoll helpers lack calls to cpu_relax(), people are sometimes reluctant to use them, and may fall back to open-coded polling loops (including cpu_relax() calls) instead. Fix this by adding calls to cpu_relax() to the iopoll helpers: - For the non-atomic case, it is sufficient to call cpu_relax() in case of a zero sleep-between-reads value, as a call to usleep_range() is a safe barrier otherwise. However, it doesn't hurt to add the call regardless, for simplicity, and for similarity with the atomic case below. - For the atomic case, cpu_relax() must be called regardless of the sleep-between-reads value, as there is no guarantee all architecture-specific implementations of udelay() handle this. Signed-off-by: Geert Uytterhoeven Acked-by: Peter Zijlstra (Intel) Acked-by: Arnd Bergmann Reviewed-by: Tony Lindgren Reviewed-by: Ulf Hansson Link: https://lore.kernel.org/r/45c87bec3397fdd704376807f0eec5cc71be440f.1685692810.git.geert+renesas@glider.be --- include/linux/iopoll.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h index 2c8860e406bd..0417360a6db9 100644 --- a/include/linux/iopoll.h +++ b/include/linux/iopoll.h @@ -53,6 +53,7 @@ } \ if (__sleep_us) \ usleep_range((__sleep_us >> 2) + 1, __sleep_us); \ + cpu_relax(); \ } \ (cond) ? 0 : -ETIMEDOUT; \ }) @@ -95,6 +96,7 @@ } \ if (__delay_us) \ udelay(__delay_us); \ + cpu_relax(); \ } \ (cond) ? 0 : -ETIMEDOUT; \ }) From 7349a69cf3125e92d48e442d9f400ba446fa314f Mon Sep 17 00:00:00 2001 From: Geert Uytterhoeven Date: Fri, 2 Jun 2023 10:50:37 +0200 Subject: [PATCH 2/3] iopoll: Do not use timekeeping in read_poll_timeout_atomic() read_poll_timeout_atomic() uses ktime_get() to implement the timeout feature, just like its non-atomic counterpart. However, there are several issues with this, due to its use in atomic contexts: 1. When called in the s2ram path (as typically done by clock or PM domain drivers), timekeeping may be suspended, triggering the WARN_ON(timekeeping_suspended) in ktime_get(): WARNING: CPU: 0 PID: 654 at kernel/time/timekeeping.c:843 ktime_get+0x28/0x78 Calling ktime_get_mono_fast_ns() instead of ktime_get() would get rid of that warning. However, that would break timeout handling, as (at least on systems with an ARM architectured timer), the time returned by ktime_get_mono_fast_ns() does not advance while timekeeping is suspended. Interestingly, (on the same ARM systems) the time returned by ktime_get() does advance while timekeeping is suspended, despite the warning. 2. Depending on the actual clock source, and especially before a high-resolution clocksource (e.g. the ARM architectured timer) becomes available, time may not advance in atomic contexts, thus breaking timeout handling. Fix this by abandoning the idea that one can rely on timekeeping to implement timeout handling in all atomic contexts, and switch from a global time-based to a locally-estimated timeout handling. In most (all?) cases the timeout condition is exceptional and an error condition, hence any additional delays due to underestimating wall clock time are irrelevant. Signed-off-by: Geert Uytterhoeven Acked-by: Arnd Bergmann Reviewed-by: Tony Lindgren Reviewed-by: Ulf Hansson Link: https://lore.kernel.org/r/3d2a2f4e553489392d871108797c3be08f88300b.1685692810.git.geert+renesas@glider.be --- include/linux/iopoll.h | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h index 0417360a6db9..19a7b00baff4 100644 --- a/include/linux/iopoll.h +++ b/include/linux/iopoll.h @@ -74,6 +74,10 @@ * Returns 0 on success and -ETIMEDOUT upon a timeout. In either * case, the last read value at @args is stored in @val. * + * This macro does not rely on timekeeping. Hence it is safe to call even when + * timekeeping is suspended, at the expense of an underestimation of wall clock + * time, which is rather minimal with a non-zero delay_us. + * * When available, you'll probably want to use one of the specialized * macros defined below rather than this macro directly. */ @@ -81,22 +85,30 @@ delay_before_read, args...) \ ({ \ u64 __timeout_us = (timeout_us); \ + s64 __left_ns = __timeout_us * NSEC_PER_USEC; \ unsigned long __delay_us = (delay_us); \ - ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \ - if (delay_before_read && __delay_us) \ + u64 __delay_ns = __delay_us * NSEC_PER_USEC; \ + if (delay_before_read && __delay_us) { \ udelay(__delay_us); \ + if (__timeout_us) \ + __left_ns -= __delay_ns; \ + } \ for (;;) { \ (val) = op(args); \ if (cond) \ break; \ - if (__timeout_us && \ - ktime_compare(ktime_get(), __timeout) > 0) { \ + if (__timeout_us && __left_ns < 0) { \ (val) = op(args); \ break; \ } \ - if (__delay_us) \ + if (__delay_us) { \ udelay(__delay_us); \ + if (__timeout_us) \ + __left_ns -= __delay_ns; \ + } \ cpu_relax(); \ + if (__timeout_us) \ + __left_ns--; \ } \ (cond) ? 0 : -ETIMEDOUT; \ }) From a00d47f7645d6e840a38a62fd961c8aa2c8fed6c Mon Sep 17 00:00:00 2001 From: Geert Uytterhoeven Date: Fri, 2 Jun 2023 10:50:41 +0200 Subject: [PATCH 3/3] soc: renesas: rmobile-sysc: Convert to readl_poll_timeout_atomic() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use readl_poll_timeout_atomic() instead of open-coding the same operation. 1. rmobile_pd_power_down(): as typically less than 20 retries are needed, PSTR_RETRIES (100) µs is a suitable timeout value. 2. __rmobile_pd_power_up(): the old method of first polling some cycles with a 1 µs delay, followed by more polling cycles without any delay didn't make much sense, as the latter was insignificant compared to the former. Furthermore, typically no retries are needed. Hence just retain the polling with delay. Signed-off-by: Geert Uytterhoeven Link: https://lore.kernel.org/r/ae4bf03ab8fd5a557c683086958d6764babc0723.1685692810.git.geert+renesas@glider.be --- drivers/soc/renesas/rmobile-sysc.c | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/drivers/soc/renesas/rmobile-sysc.c b/drivers/soc/renesas/rmobile-sysc.c index 728ebac98e14..912daadaa10d 100644 --- a/drivers/soc/renesas/rmobile-sysc.c +++ b/drivers/soc/renesas/rmobile-sysc.c @@ -12,6 +12,8 @@ #include #include #include +#include +#include #include #include #include @@ -19,8 +21,6 @@ #include #include -#include - /* SYSC */ #define SPDCR 0x08 /* SYS Power Down Control Register */ #define SWUCR 0x14 /* SYS Wakeup Control Register */ @@ -47,6 +47,7 @@ static int rmobile_pd_power_down(struct generic_pm_domain *genpd) { struct rmobile_pm_domain *rmobile_pd = to_rmobile_pd(genpd); unsigned int mask = BIT(rmobile_pd->bit_shift); + u32 val; if (rmobile_pd->suspend) { int ret = rmobile_pd->suspend(); @@ -56,14 +57,10 @@ static int rmobile_pd_power_down(struct generic_pm_domain *genpd) } if (readl(rmobile_pd->base + PSTR) & mask) { - unsigned int retry_count; writel(mask, rmobile_pd->base + SPDCR); - for (retry_count = PSTR_RETRIES; retry_count; retry_count--) { - if (!(readl(rmobile_pd->base + SPDCR) & mask)) - break; - cpu_relax(); - } + readl_poll_timeout_atomic(rmobile_pd->base + SPDCR, val, + !(val & mask), 0, PSTR_RETRIES); } pr_debug("%s: Power off, 0x%08x -> PSTR = 0x%08x\n", genpd->name, mask, @@ -74,8 +71,7 @@ static int rmobile_pd_power_down(struct generic_pm_domain *genpd) static int __rmobile_pd_power_up(struct rmobile_pm_domain *rmobile_pd) { - unsigned int mask = BIT(rmobile_pd->bit_shift); - unsigned int retry_count; + unsigned int val, mask = BIT(rmobile_pd->bit_shift); int ret = 0; if (readl(rmobile_pd->base + PSTR) & mask) @@ -83,16 +79,9 @@ static int __rmobile_pd_power_up(struct rmobile_pm_domain *rmobile_pd) writel(mask, rmobile_pd->base + SWUCR); - for (retry_count = 2 * PSTR_RETRIES; retry_count; retry_count--) { - if (!(readl(rmobile_pd->base + SWUCR) & mask)) - break; - if (retry_count > PSTR_RETRIES) - udelay(PSTR_DELAY_US); - else - cpu_relax(); - } - if (!retry_count) - ret = -EIO; + ret = readl_poll_timeout_atomic(rmobile_pd->base + SWUCR, val, + (val & mask), PSTR_DELAY_US, + PSTR_RETRIES * PSTR_DELAY_US); pr_debug("%s: Power on, 0x%08x -> PSTR = 0x%08x\n", rmobile_pd->genpd.name, mask,