[PATCH v3 0/7] iopoll: Busy loop and timeout improvements + conversions

[PATCH v3 0/7] iopoll: Busy loop and timeout improvements + conversions

[Geert Uytterhoeven]

	Hi all,

When implementing a polling loop, a common review comment is to use one
of the read*_poll_timeout*() helpers.  Unfortunately this is not always
possible, or might introduce subtle bugs.  This patch series aims to
improve these helpers, so they can gain wider use.

  1. The first patch improves busy-looping behavior of both the atomic
     and non-atomic read*_poll_timeout*() helpers.
     The issue addressed by this patch was discussed before[1-2], but I
     am not aware of any patches moving forward.

  2. The second patch fixes timeout handling of the atomic variants.
     Some of the issues addressed by this patch were mitigated in
     various places[3-5], and some of these findings may be of interest
     to the authors of [6-8].

The first two patches were sent before, and already received some acks
and reviews.  I plan to queue these in an immutable and tagged branch
after the weekend, for consumption by myself, and by other interested
parties.

The last five patches are new, and convert several Renesas-specific
drivers from open-coded loops to the fixed read*_poll_timeout_atomic()
helpers:
  - I plan to queue the clk and soc patches in renesas-devel resp.
    renesas-clk for v6.5,
  - The IOMMU patch can wait for v6.6, unless the IOMMU maintainers
    already want to merge the immutable branch, too.

Thanks for your comments!

Changes compared to v2[9]:
  - Add Acked-by, Reviewed-by,
  - Add comment about not using timekeeping, and its impact,
  - Add conversion patches 3-7.

Changes compared to v1[10]:
  - Add Acked-by,
  - Add compiler barrier and inlining explanation (thanks, Peter!),
  - Add patch [2/2].

Thanks for your comments!

[1] "Re: [PATCH 6/7] clk: renesas: rcar-gen3: Add custom clock for PLLs"
    https://lore.kernel.org/all/CAMuHMdWUEhs=nwP+a0vO2jOzkq-7FEOqcJ+SsxAGNXX1PQ2KMA@mail.gmail.com
[2] "Re: [PATCH v2] clk: samsung: Prevent potential endless loop in the
    PLL set_rate ops"
    https://lore.kernel.org/all/20200811164628.GA7958@kozik-lap
[3] b3e9431854e8f305 ("bus: ti-sysc: Fix timekeeping_suspended warning
		       on resume")
[4] 44a9e78f9242872c ("clk: samsung: Prevent potential endless loop in
		       the PLL ops")
[5] 3d8598fb9c5a7783 ("clk: ti: clkctrl: use fallback udelay approach if
		      timekeeping is suspended")
[6] bd40cbb0e3b37a4d ("PM: domains: Move genpd's time-accounting to
		       ktime_get_mono_fast_ns()")
[7] c155f6499f9797f2 ("PM-runtime: Switch accounting over to
		       ktime_get_mono_fast_ns()")
[8] 15efb47dc560849d ("PM-runtime: Fix deadlock with ktime_get()")

[9] https://lore.kernel.org/all/cover.1683722688.git.geert+renesas@glider.be

[10] https://lore.kernel.org/r/8d492ee4a391bd089a01c218b0b4e05cf8ea593c.1674729407.git.geert+renesas@glider.be/

Geert Uytterhoeven (7):
  iopoll: Call cpu_relax() in busy loops
  iopoll: Do not use timekeeping in read_poll_timeout_atomic()
  clk: renesas: cpg-mssr: Convert to readl_poll_timeout_atomic()
  clk: renesas: mstp: Convert to readl_poll_timeout_atomic()
  clk: renesas: rzg2l: Convert to readl_poll_timeout_atomic()
  soc: renesas: rmobile-sysc: Convert to readl_poll_timeout_atomic()
  iommu/ipmmu-vmsa: Convert to read_poll_timeout_atomic()

 drivers/clk/renesas/clk-mstp.c         | 18 ++++++---------
 drivers/clk/renesas/renesas-cpg-mssr.c | 31 +++++++++-----------------
 drivers/clk/renesas/rzg2l-cpg.c        | 16 +++++--------
 drivers/iommu/ipmmu-vmsa.c             | 15 +++++--------
 drivers/soc/renesas/rmobile-sysc.c     | 31 +++++++++-----------------
 include/linux/iopoll.h                 | 24 +++++++++++++++-----
 6 files changed, 58 insertions(+), 77 deletions(-)

-- 
2.34.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH v3 1/7] iopoll: Call cpu_relax() in busy loops

[Geert Uytterhoeven]

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 <geert+renesas@glider.be>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Tony Lindgren <tony@atomide.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
v3:
  - Add Reviewed-by,

v2:
  - Add Acked-by,
  - Add compiler barrier and inlining explanation (thanks, Peter!).
---
 include/linux/iopoll.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
index 2c8860e406bd8cae..0417360a6db9b0d6 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; \
 })
-- 
2.34.1

[PATCH v3 2/7] iopoll: Do not use timekeeping in read_poll_timeout_atomic()

[Geert Uytterhoeven]

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 <geert+renesas@glider.be>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Tony Lindgren <tony@atomide.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
The first issue was seen with the rcar-sysc driver in the BSP, as the
BSP contains modifications to the resume sequence of various PM Domains.

v3:
  - Add Acked-by, Reviewed-by,
  - Add comment about not using timekeeping, and its impact,

v2:
  - New.
---
 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 0417360a6db9b0d6..19a7b00baff43595 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; \
 })
-- 
2.34.1

[PATCH v3 3/7] clk: renesas: cpg-mssr: Convert to readl_poll_timeout_atomic()

[Geert Uytterhoeven]

Use readl_poll_timeout_atomic() instead of open-coding the same
operation.

As typically no retries are needed, 10 µs is a suitable timeout value.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Polling measurements done on R-Car M2-W, H3 ES2.0, M3-W, M3-N, E3, and
V4H.

v3:
  - New.
---
 drivers/clk/renesas/renesas-cpg-mssr.c | 31 +++++++++-----------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
index e9c0e341380e8f55..2772499d20165127 100644
--- a/drivers/clk/renesas/renesas-cpg-mssr.c
+++ b/drivers/clk/renesas/renesas-cpg-mssr.c
@@ -17,6 +17,7 @@
 #include <linux/device.h>
 #include <linux/init.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
@@ -196,8 +197,8 @@ static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable)
 	struct device *dev = priv->dev;
 	u32 bitmask = BIT(bit);
 	unsigned long flags;
-	unsigned int i;
 	u32 value;
+	int error;
 
 	dev_dbg(dev, "MSTP %u%02u/%pC %s\n", reg, bit, hw->clk,
 		enable ? "ON" : "OFF");
@@ -228,19 +229,13 @@ static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable)
 	if (!enable || priv->reg_layout == CLK_REG_LAYOUT_RZ_A)
 		return 0;
 
-	for (i = 1000; i > 0; --i) {
-		if (!(readl(priv->base + priv->status_regs[reg]) & bitmask))
-			break;
-		cpu_relax();
-	}
-
-	if (!i) {
+	error = readl_poll_timeout_atomic(priv->base + priv->status_regs[reg],
+					  value, !(value & bitmask), 0, 10);
+	if (error)
 		dev_err(dev, "Failed to enable SMSTP %p[%d]\n",
 			priv->base + priv->control_regs[reg], bit);
-		return -ETIMEDOUT;
-	}
 
-	return 0;
+	return error;
 }
 
 static int cpg_mstp_clock_enable(struct clk_hw *hw)
@@ -896,8 +891,9 @@ static int cpg_mssr_suspend_noirq(struct device *dev)
 static int cpg_mssr_resume_noirq(struct device *dev)
 {
 	struct cpg_mssr_priv *priv = dev_get_drvdata(dev);
-	unsigned int reg, i;
+	unsigned int reg;
 	u32 mask, oldval, newval;
+	int error;
 
 	/* This is the best we can do to check for the presence of PSCI */
 	if (!psci_ops.cpu_suspend)
@@ -935,14 +931,9 @@ static int cpg_mssr_resume_noirq(struct device *dev)
 		if (!mask)
 			continue;
 
-		for (i = 1000; i > 0; --i) {
-			oldval = readl(priv->base + priv->status_regs[reg]);
-			if (!(oldval & mask))
-				break;
-			cpu_relax();
-		}
-
-		if (!i)
+		error = readl_poll_timeout_atomic(priv->base + priv->status_regs[reg],
+						oldval, !(oldval & mask), 0, 10);
+		if (error)
 			dev_warn(dev, "Failed to enable SMSTP%u[0x%x]\n", reg,
 				 oldval & mask);
 	}
-- 
2.34.1

[PATCH v3 4/7] clk: renesas: mstp: Convert to readl_poll_timeout_atomic()

[Geert Uytterhoeven]

Use readl_poll_timeout_atomic() instead of open-coding the same
operation.

As typically no retries are needed, 10 µs is a suitable timeout value.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Polling measurements done on R-Mobile APE6 and A1, R-Car H1, and
SH-Mobile AG5.

v3:
  - New.
---
 drivers/clk/renesas/clk-mstp.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
index 6e3c4a9c16b07ae9..e96457371b4cce88 100644
--- a/drivers/clk/renesas/clk-mstp.c
+++ b/drivers/clk/renesas/clk-mstp.c
@@ -14,6 +14,7 @@
 #include <linux/clk/renesas.h>
 #include <linux/device.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/pm_clock.h>
@@ -78,8 +79,8 @@ static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable)
 	struct mstp_clock_group *group = clock->group;
 	u32 bitmask = BIT(clock->bit_index);
 	unsigned long flags;
-	unsigned int i;
 	u32 value;
+	int ret;
 
 	spin_lock_irqsave(&group->lock, flags);
 
@@ -102,19 +103,14 @@ static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable)
 	if (!enable || !group->mstpsr)
 		return 0;
 
-	for (i = 1000; i > 0; --i) {
-		if (!(cpg_mstp_read(group, group->mstpsr) & bitmask))
-			break;
-		cpu_relax();
-	}
-
-	if (!i) {
+	/* group->width_8bit is always false if group->mstpsr is present */
+	ret = readl_poll_timeout_atomic(group->mstpsr, value,
+					!(value & bitmask), 0, 10);
+	if (ret)
 		pr_err("%s: failed to enable %p[%d]\n", __func__,
 		       group->smstpcr, clock->bit_index);
-		return -ETIMEDOUT;
-	}
 
-	return 0;
+	return ret;
 }
 
 static int cpg_mstp_clock_enable(struct clk_hw *hw)
-- 
2.34.1

[PATCH v3 5/7] clk: renesas: rzg2l: Convert to readl_poll_timeout_atomic()

[Geert Uytterhoeven]

Use readl_poll_timeout_atomic() instead of open-coding the same
operation.

As typically no retries are needed, 10 µs is a suitable timeout value.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Polling measurements done on RZ/Five.

v3:
  - New.
---
 drivers/clk/renesas/rzg2l-cpg.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index ca8b921c77625317..bc623515ad843cf5 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -903,9 +903,9 @@ static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
 	unsigned int reg = clock->off;
 	struct device *dev = priv->dev;
 	unsigned long flags;
-	unsigned int i;
 	u32 bitmask = BIT(clock->bit);
 	u32 value;
+	int error;
 
 	if (!clock->off) {
 		dev_dbg(dev, "%pC does not support ON/OFF\n",  hw->clk);
@@ -930,19 +930,13 @@ static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
 	if (!priv->info->has_clk_mon_regs)
 		return 0;
 
-	for (i = 1000; i > 0; --i) {
-		if (((readl(priv->base + CLK_MON_R(reg))) & bitmask))
-			break;
-		cpu_relax();
-	}
-
-	if (!i) {
+	error = readl_poll_timeout_atomic(priv->base + CLK_MON_R(reg), value,
+					  value & bitmask, 0, 10);
+	if (error)
 		dev_err(dev, "Failed to enable CLK_ON %p\n",
 			priv->base + CLK_ON_R(reg));
-		return -ETIMEDOUT;
-	}
 
-	return 0;
+	return error;
 }
 
 static int rzg2l_mod_clock_enable(struct clk_hw *hw)
-- 
2.34.1

[PATCH v3 6/7] soc: renesas: rmobile-sysc: Convert to readl_poll_timeout_atomic()

[Geert Uytterhoeven]

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 <geert+renesas@glider.be>
---
Polling measurements done on R-Mobile APE6 and A1, and SH-Mobile AG5.

v3:
  - New.
---
 drivers/soc/renesas/rmobile-sysc.c | 31 ++++++++++--------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

diff --git a/drivers/soc/renesas/rmobile-sysc.c b/drivers/soc/renesas/rmobile-sysc.c
index 728ebac98e14a5cc..5d621c35fba1116a 100644
--- a/drivers/soc/renesas/rmobile-sysc.c
+++ b/drivers/soc/renesas/rmobile-sysc.c
@@ -12,6 +12,8 @@
 #include <linux/clk/renesas.h>
 #include <linux/console.h>
 #include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/pm.h>
@@ -19,8 +21,6 @@
 #include <linux/pm_domain.h>
 #include <linux/slab.h>
 
-#include <asm/io.h>
-
 /* 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,25 +71,17 @@ 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;
-	int ret = 0;
+	unsigned int val, mask = BIT(rmobile_pd->bit_shift);
+	int ret;
 
 	if (readl(rmobile_pd->base + PSTR) & mask)
 		return ret;
 
 	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,
-- 
2.34.1

[PATCH v3 7/7] iommu/ipmmu-vmsa: Convert to read_poll_timeout_atomic()

[Geert Uytterhoeven]

Use read_poll_timeout_atomic() instead of open-coding the same
operation.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v3:
  - New.
---
 drivers/iommu/ipmmu-vmsa.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 9f64c5c9f5b90ace..3b58a8ea3bdef190 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -14,6 +14,7 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/io-pgtable.h>
 #include <linux/iommu.h>
 #include <linux/of.h>
@@ -253,17 +254,13 @@ static void ipmmu_imuctr_write(struct ipmmu_vmsa_device *mmu,
 /* Wait for any pending TLB invalidations to complete */
 static void ipmmu_tlb_sync(struct ipmmu_vmsa_domain *domain)
 {
-	unsigned int count = 0;
+	u32 val;
 
-	while (ipmmu_ctx_read_root(domain, IMCTR) & IMCTR_FLUSH) {
-		cpu_relax();
-		if (++count == TLB_LOOP_TIMEOUT) {
-			dev_err_ratelimited(domain->mmu->dev,
+	if (read_poll_timeout_atomic(ipmmu_ctx_read_root, val,
+				     !(val & IMCTR_FLUSH), 1, TLB_LOOP_TIMEOUT,
+				     false, domain, IMCTR))
+		dev_err_ratelimited(domain->mmu->dev,
 			"TLB sync timed out -- MMU may be deadlocked\n");
-			return;
-		}
-		udelay(1);
-	}
 }
 
 static void ipmmu_tlb_invalidate(struct ipmmu_vmsa_domain *domain)
-- 
2.34.1

Re: [PATCH v3 6/7] soc: renesas: rmobile-sysc: Convert to readl_poll_timeout_atomic()

[kernel test robot]

Hi Geert,

kernel test robot noticed the following build warnings:

[auto build test WARNING on geert-renesas-drivers/renesas-clk]
[also build test WARNING on joro-iommu/next clk/clk-next linus/master v6.4-rc4 next-20230602]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Geert-Uytterhoeven/iopoll-Call-cpu_relax-in-busy-loops/20230602-165454
base:   https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git renesas-clk
patch link:    https://lore.kernel.org/r/ae4bf03ab8fd5a557c683086958d6764babc0723.1685692810.git.geert%2Brenesas%40glider.be
patch subject: [PATCH v3 6/7] soc: renesas: rmobile-sysc: Convert to readl_poll_timeout_atomic()
config: hexagon-randconfig-r031-20230531 (https://download.01.org/0day-ci/archive/20230602/202306022137.eAP4Q3hL-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 4faf3aaf28226a4e950c103a14f6fc1d1fdabb1b)
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/2198e2f3fd21eb62861b6ada08eeec7d6f420ee3
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Geert-Uytterhoeven/iopoll-Call-cpu_relax-in-busy-loops/20230602-165454
        git checkout 2198e2f3fd21eb62861b6ada08eeec7d6f420ee3
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/soc/renesas/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306022137.eAP4Q3hL-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/soc/renesas/rmobile-sysc.c:15:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/soc/renesas/rmobile-sysc.c:15:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/soc/renesas/rmobile-sysc.c:15:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
>> drivers/soc/renesas/rmobile-sysc.c:78:10: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
                   return ret;
                          ^~~
   drivers/soc/renesas/rmobile-sysc.c:75:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   7 warnings generated.


vim +/ret +78 drivers/soc/renesas/rmobile-sysc.c

8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  71  
445aeb081bc713 arch/arm/mach-shmobile/pm-rmobile.c Geert Uytterhoeven 2018-11-29  72  static int __rmobile_pd_power_up(struct rmobile_pm_domain *rmobile_pd)
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  73  {
2198e2f3fd21eb drivers/soc/renesas/rmobile-sysc.c  Geert Uytterhoeven 2023-06-02  74  	unsigned int val, mask = BIT(rmobile_pd->bit_shift);
2198e2f3fd21eb drivers/soc/renesas/rmobile-sysc.c  Geert Uytterhoeven 2023-06-02  75  	int ret;
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  76  
8b6bed678428b6 drivers/soc/renesas/rmobile-sysc.c  Geert Uytterhoeven 2020-11-19  77  	if (readl(rmobile_pd->base + PSTR) & mask)
445aeb081bc713 arch/arm/mach-shmobile/pm-rmobile.c Geert Uytterhoeven 2018-11-29 @78  		return ret;
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  79  
8b6bed678428b6 drivers/soc/renesas/rmobile-sysc.c  Geert Uytterhoeven 2020-11-19  80  	writel(mask, rmobile_pd->base + SWUCR);
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  81  
2198e2f3fd21eb drivers/soc/renesas/rmobile-sysc.c  Geert Uytterhoeven 2023-06-02  82  	ret = readl_poll_timeout_atomic(rmobile_pd->base + SWUCR, val,
2198e2f3fd21eb drivers/soc/renesas/rmobile-sysc.c  Geert Uytterhoeven 2023-06-02  83  					(val & mask), PSTR_DELAY_US,
2198e2f3fd21eb drivers/soc/renesas/rmobile-sysc.c  Geert Uytterhoeven 2023-06-02  84  					PSTR_RETRIES * PSTR_DELAY_US);
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  85  
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  86  	pr_debug("%s: Power on, 0x%08x -> PSTR = 0x%08x\n",
25717b85736076 arch/arm/mach-shmobile/pm-rmobile.c Geert Uytterhoeven 2014-12-03  87  		 rmobile_pd->genpd.name, mask,
8b6bed678428b6 drivers/soc/renesas/rmobile-sysc.c  Geert Uytterhoeven 2020-11-19  88  		 readl(rmobile_pd->base + PSTR));
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  89  
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  90  	return ret;
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  91  }
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  92  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 6/7] soc: renesas: rmobile-sysc: Convert to readl_poll_timeout_atomic()

[Geert Uytterhoeven]

On Fri, Jun 2, 2023 at 10:51 AM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> 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 <geert+renesas@glider.be>

> diff --git a/drivers/soc/renesas/rmobile-sysc.c b/drivers/soc/renesas/rmobile-sysc.c
> index 728ebac98e14a5cc..5d621c35fba1116a 100644

> @@ -74,25 +71,17 @@ 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;
> -       int ret = 0;
> +       unsigned int val, mask = BIT(rmobile_pd->bit_shift);
> +       int ret;

Oops, "ret" should still be initialized to zero.

>
>         if (readl(rmobile_pd->base + PSTR) & mask)
>                 return ret;
>
>         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,

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 0/7] iopoll: Busy loop and timeout improvements + conversions

[Geert Uytterhoeven]

On Fri, Jun 2, 2023 at 10:51 AM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> When implementing a polling loop, a common review comment is to use one
> of the read*_poll_timeout*() helpers.  Unfortunately this is not always
> possible, or might introduce subtle bugs.  This patch series aims to
> improve these helpers, so they can gain wider use.
>
>   1. The first patch improves busy-looping behavior of both the atomic
>      and non-atomic read*_poll_timeout*() helpers.
>      The issue addressed by this patch was discussed before[1-2], but I
>      am not aware of any patches moving forward.
>
>   2. The second patch fixes timeout handling of the atomic variants.
>      Some of the issues addressed by this patch were mitigated in
>      various places[3-5], and some of these findings may be of interest
>      to the authors of [6-8].
>
> The first two patches were sent before, and already received some acks
> and reviews.  I plan to queue these in an immutable and tagged branch
> after the weekend, for consumption by myself, and by other interested
> parties.

FTR...
The following changes since commit ac9a78681b921877518763ba0e89202254349d1b:

  Linux 6.4-rc1 (2023-05-07 13:34:35 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git
tags/iopoll-busy-loop-timeout-tag

for you to fetch changes up to 7349a69cf3125e92d48e442d9f400ba446fa314f:

  iopoll: Do not use timekeeping in read_poll_timeout_atomic()
(2023-06-05 15:35:27 +0200)

----------------------------------------------------------------
iopoll: Busy loop and timeout improvements

----------------------------------------------------------------
Geert Uytterhoeven (2):
      iopoll: Call cpu_relax() in busy loops
      iopoll: Do not use timekeeping in read_poll_timeout_atomic()

 include/linux/iopoll.h | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 2/7] iopoll: Do not use timekeeping in read_poll_timeout_atomic()

[Zong Li]

On Fri, Jun 02, 2023 at 10:50:37AM +0200, Geert Uytterhoeven wrote:
> 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.
>

Hi Geert,
I tested this patch on the FPGA, and I noticed the timeout duration
was much longer than expected. I tested it by removing the op operation
and break condition for avoiding the influence of other factors.
The code would look like as follows:

for (;;) {
        if (__timeout_us && __left_ns < 0)
                break;
        if (__delay_us) {
                udelay(__delay_us);
                if (__timeout_us)
                        __left_ns -= __delay_ns;;
	cpu_relex();
        if (__timeout_us)
                __left_ns--;
        }
}

Despite setting the timeout to 1 second, it actually takes 25 seconds
to reach the specified timeout value. I displayed the value of
__left_ns when a timeout occurred. As follows: __delay_us is 1, when
__left_ns counts down to -1, the system has run for 25 seconds.

[   26.016213] __timeout_us: 1000000 __left_ns: -1
[   50.818585] __timeout_us: 1000000  __left_ns: -1
[   75.620467] __timeout_us: 1000000  __left_ns: -1
[  100.422664] __timeout_us: 1000000  __left_ns: -1
[  125.224775] __timeout_us: 1000000  __left_ns: -1
...

I attempted to blend the two versions (e.g., ktime version and the
current version) for discarding the value of __left_ns. The resulting
output is as follows: __delay_us is 1, when it exceeds 1 second
according to ktime, __left_ns only counts around 40 ms.

[    6.734482] __timeout_us: 1000000  __left_ns: 961699000
[    7.738485] __timeout_us: 1000000  __left_ns: 961228000
[    8.812797] __timeout_us: 1000000  __left_ns: 961755000
[    9.814021] __timeout_us: 1000000  __left_ns: 961542000
[   10.815373] __timeout_us: 1000000 __left_ns: 962464000
[   11.816184] __timeout_us: 1000000 __left_ns: 961536000
[   12.817137] __timeout_us: 1000000 __left_ns: 961121000
...

Per your suggestion, I attempted to increase delay_us to 10 us,
it really helps to eliminate the underestimation. The actual
timeout became 3 secs on the FPGA.

I moved on my host x86 machine, the timeout has been reduced to
2 seconds even if the delay_us is 1. And the timeout can be
precise 1 seconds when delay_us is 10. I'm not sure if the clock
frequency or RTC frequency might also determine the underestimation
of wall clock time? Is there a suggested value of delay_us for a
driver that runs on various platforms?
What is your perspective for those situation?

Thanks.

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Reviewed-by: Tony Lindgren <tony@atomide.com>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> The first issue was seen with the rcar-sysc driver in the BSP, as the
> BSP contains modifications to the resume sequence of various PM Domains.
> 
> v3:
>   - Add Acked-by, Reviewed-by,
>   - Add comment about not using timekeeping, and its impact,
> 
> v2:
>   - New.
> ---
>  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 0417360a6db9b0d6..19a7b00baff43595 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; \
>  })
> -- 
> 2.34.1
> 

Re: [PATCH v3 2/7] iopoll: Do not use timekeeping in read_poll_timeout_atomic()

[Geert Uytterhoeven]

Hi Zong,

On Tue, Mar 26, 2024 at 2:31 AM Zong Li <zong.li@sifive.com> wrote:
> On Fri, Jun 02, 2023 at 10:50:37AM +0200, Geert Uytterhoeven wrote:
> > 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.
> >
>
> Hi Geert,
> I tested this patch on the FPGA, and I noticed the timeout duration
> was much longer than expected. I tested it by removing the op operation
> and break condition for avoiding the influence of other factors.
> The code would look like as follows:
>
> for (;;) {
>         if (__timeout_us && __left_ns < 0)
>                 break;
>         if (__delay_us) {
>                 udelay(__delay_us);
>                 if (__timeout_us)
>                         __left_ns -= __delay_ns;;
>         cpu_relex();
>         if (__timeout_us)
>                 __left_ns--;
>         }
> }
>
> Despite setting the timeout to 1 second, it actually takes 25 seconds
> to reach the specified timeout value. I displayed the value of
> __left_ns when a timeout occurred. As follows: __delay_us is 1, when
> __left_ns counts down to -1, the system has run for 25 seconds.
>
> [   26.016213] __timeout_us: 1000000 __left_ns: -1
> [   50.818585] __timeout_us: 1000000  __left_ns: -1
> [   75.620467] __timeout_us: 1000000  __left_ns: -1
> [  100.422664] __timeout_us: 1000000  __left_ns: -1
> [  125.224775] __timeout_us: 1000000  __left_ns: -1
> ...
>
> I attempted to blend the two versions (e.g., ktime version and the
> current version) for discarding the value of __left_ns. The resulting
> output is as follows: __delay_us is 1, when it exceeds 1 second
> according to ktime, __left_ns only counts around 40 ms.
>
> [    6.734482] __timeout_us: 1000000  __left_ns: 961699000
> [    7.738485] __timeout_us: 1000000  __left_ns: 961228000
> [    8.812797] __timeout_us: 1000000  __left_ns: 961755000
> [    9.814021] __timeout_us: 1000000  __left_ns: 961542000
> [   10.815373] __timeout_us: 1000000 __left_ns: 962464000
> [   11.816184] __timeout_us: 1000000 __left_ns: 961536000
> [   12.817137] __timeout_us: 1000000 __left_ns: 961121000
> ...
>
> Per your suggestion, I attempted to increase delay_us to 10 us,
> it really helps to eliminate the underestimation. The actual
> timeout became 3 secs on the FPGA.
>
> I moved on my host x86 machine, the timeout has been reduced to
> 2 seconds even if the delay_us is 1. And the timeout can be
> precise 1 seconds when delay_us is 10. I'm not sure if the clock
> frequency or RTC frequency might also determine the underestimation
> of wall clock time? Is there a suggested value of delay_us for a
> driver that runs on various platforms?
> What is your perspective for those situation?

RTC frequency does not impact the timeout, as the macro no longer
relies on timekeeping.
CPU clock frequency does impact the timeout, especially when op()
executes lots of instructions.  The code assumes op() takes 1 ns,
which is a conservative value to prevent overestimation of wall clock
time.  This assumes that such an overestimation (i.e. timing out too
early) is much worse than an underestimation (i.e. timing out too late).
If op() takes much more time than 1 ns, or even more time than delay_us,
the effective timeout will be much larger than expected.

I can only suggest to use a reasonable value for delay_us, i.e. a value
that is (sufficiently) larger than the expected execution time of op().

I hope this helps.

> > --- 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; \
> >  })

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds