2014-11-24 20:15:16

by Mitchel Humpherys

[permalink] [raw]
Subject: [PATCH RESEND v8] iopoll: Introduce memory-mapped IO polling macros

From: Matt Wagantall <[email protected]>

It is sometimes necessary to poll a memory-mapped register until its value
satisfies some condition. Introduce a family of convenience macros that do
this. Tight-looping, sleeping, and timing out can all be accomplished using
these macros.

Cc: Thierry Reding <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Andrew Morton <[email protected]>
Signed-off-by: Matt Wagantall <[email protected]>
Signed-off-by: Mitchel Humpherys <[email protected]>
---
This patch was originally part of a series [1] for adding support for IOMMU
address translations through an ARM SMMU hardware register. The other
patch in the series (the one that actually uses these macros and implements
said hardware address translations) was Ack'd by the driver maintainer
there (Will Deacon) so I've pulled this patch out to avoid resending an
already Ack'd patch over and over again.

In short, please see [1] for previous discussion and the first user of
these macros.

This patch has been resent previously here [2], here [3], and here [4] on
2014-10-30, 2014-11-06, and 2014-11-17, respectively. It has not changed
since [2] and has not received any comments since [1] on 2014-10-19.
Thanks to everyone who has taken a look at this.

[1] http://thread.gmane.org/gmane.linux.kernel.iommu/7140
[2] http://thread.gmane.org/gmane.linux.kernel/1818213
[3] http://thread.gmane.org/gmane.linux.kernel/1823422
[4] http://thread.gmane.org/gmane.linux.kernel.iommu/7394


Changes since v7:
- sorted helper macros by size (b, w, l, q)
- removed some of the more esoteric (or flat-out bogus) helper macros
---
include/linux/iopoll.h | 140 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 140 insertions(+)
create mode 100644 include/linux/iopoll.h

diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
new file mode 100644
index 0000000000..bd161dae2d
--- /dev/null
+++ b/include/linux/iopoll.h
@@ -0,0 +1,140 @@
+/*
+ * Copyright (c) 2012-2014 The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef _LINUX_IOPOLL_H
+#define _LINUX_IOPOLL_H
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/hrtimer.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/io.h>
+
+/**
+ * readx_poll_timeout - Periodically poll an address until a condition is met or a timeout occurs
+ * @op: accessor function (takes @addr as its only argument)
+ * @addr: Address to poll
+ * @val: Variable to read the value into
+ * @cond: Break condition (usually involving @val)
+ * @sleep_us: Maximum time to sleep between reads in us (0 tight-loops)
+ * @timeout_us: Timeout in us, 0 means never timeout
+ *
+ * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
+ * case, the last read value at @addr is stored in @val. Must not
+ * be called from atomic context if sleep_us or timeout_us are used.
+ *
+ * When available, you'll probably want to use one of the specialized
+ * macros defined below rather than this macro directly.
+ */
+#define readx_poll_timeout(op, addr, val, cond, sleep_us, timeout_us) \
+({ \
+ ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
+ might_sleep_if(sleep_us); \
+ for (;;) { \
+ (val) = op(addr); \
+ if (cond) \
+ break; \
+ if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \
+ (val) = op(addr); \
+ break; \
+ } \
+ if (sleep_us) \
+ usleep_range((sleep_us >> 2) + 1, sleep_us); \
+ } \
+ (cond) ? 0 : -ETIMEDOUT; \
+})
+
+/**
+ * readx_poll_timeout_atomic - Periodically poll an address until a condition is met or a timeout occurs
+ * @op: accessor function (takes @addr as its only argument)
+ * @addr: Address to poll
+ * @val: Variable to read the value into
+ * @cond: Break condition (usually involving @val)
+ * @delay_us: Time to udelay between reads in us (0 tight-loops)
+ * @timeout_us: Timeout in us, 0 means never timeout
+ *
+ * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
+ * case, the last read value at @addr is stored in @val.
+ *
+ * When available, you'll probably want to use one of the specialized
+ * macros defined below rather than this macro directly.
+ */
+#define readx_poll_timeout_atomic(op, addr, val, cond, delay_us, timeout_us) \
+({ \
+ ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
+ for (;;) { \
+ (val) = op(addr); \
+ if (cond) \
+ break; \
+ if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \
+ (val) = op(addr); \
+ break; \
+ } \
+ if (delay_us) \
+ udelay(delay_us); \
+ } \
+ (cond) ? 0 : -ETIMEDOUT; \
+})
+
+
+#define readb_poll_timeout(addr, val, cond, delay_us, timeout_us) \
+ readx_poll_timeout(readb, addr, val, cond, delay_us, timeout_us)
+
+#define readb_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
+ readx_poll_timeout_atomic(readb, addr, val, cond, delay_us, timeout_us)
+
+#define readw_poll_timeout(addr, val, cond, delay_us, timeout_us) \
+ readx_poll_timeout(readw, addr, val, cond, delay_us, timeout_us)
+
+#define readw_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
+ readx_poll_timeout_atomic(readw, addr, val, cond, delay_us, timeout_us)
+
+#define readl_poll_timeout(addr, val, cond, delay_us, timeout_us) \
+ readx_poll_timeout(readl, addr, val, cond, delay_us, timeout_us)
+
+#define readl_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
+ readx_poll_timeout_atomic(readl, addr, val, cond, delay_us, timeout_us)
+
+#define readq_poll_timeout(addr, val, cond, delay_us, timeout_us) \
+ readx_poll_timeout(readq, addr, val, cond, delay_us, timeout_us)
+
+#define readq_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
+ readx_poll_timeout_atomic(readq, addr, val, cond, delay_us, timeout_us)
+
+#define readb_relaxed_poll_timeout(addr, val, cond, delay_us, timeout_us) \
+ readx_poll_timeout(readb_relaxed, addr, val, cond, delay_us, timeout_us)
+
+#define readb_relaxed_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
+ readx_poll_timeout_atomic(readb_relaxed, addr, val, cond, delay_us, timeout_us)
+
+#define readw_relaxed_poll_timeout(addr, val, cond, delay_us, timeout_us) \
+ readx_poll_timeout(readw_relaxed, addr, val, cond, delay_us, timeout_us)
+
+#define readw_relaxed_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
+ readx_poll_timeout_atomic(readw_relaxed, addr, val, cond, delay_us, timeout_us)
+
+#define readl_relaxed_poll_timeout(addr, val, cond, delay_us, timeout_us) \
+ readx_poll_timeout(readl_relaxed, addr, val, cond, delay_us, timeout_us)
+
+#define readl_relaxed_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
+ readx_poll_timeout_atomic(readl_relaxed, addr, val, cond, delay_us, timeout_us)
+
+#define readq_relaxed_poll_timeout(addr, val, cond, delay_us, timeout_us) \
+ readx_poll_timeout(readq_relaxed, addr, val, cond, delay_us, timeout_us)
+
+#define readq_relaxed_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
+ readx_poll_timeout_atomic(readq_relaxed, addr, val, cond, delay_us, timeout_us)
+
+#endif /* _LINUX_IOPOLL_H */
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


Subject: RE: [PATCH RESEND v8] iopoll: Introduce memory-mapped IO polling macros



> -----Original Message-----
> From: [email protected] [mailto:linux-kernel-
> [email protected]] On Behalf Of Mitchel Humpherys
> Sent: Monday, 24 November, 2014 2:15 PM
...
> From: Matt Wagantall <[email protected]>
>
> It is sometimes necessary to poll a memory-mapped register until its value
> satisfies some condition. Introduce a family of convenience macros that do
> this. Tight-looping, sleeping, and timing out can all be accomplished
> using these macros.
>
...
> +#define readx_poll_timeout(op, addr, val, cond, sleep_us, timeout_us) \
> +({ \
> + ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
> + might_sleep_if(sleep_us); \
> + for (;;) { \
> + (val) = op(addr); \
> + if (cond) \
> + break; \
> + if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) {
> \
> + (val) = op(addr); \
> + break; \
> + } \
> + if (sleep_us) \
> + usleep_range((sleep_us >> 2) + 1, sleep_us); \

The hpsa SCSI driver used to use usleep_range in a loop like
that, but we found that it caused scheduler problems during
boots because it uses TASK_UNINTERRUPTIBLE:
[ 9.260668] [sched_delayed] sched: RT throttling activated

msleep() worked much better.

---
Rob Elliott HP Server Storage


2014-11-25 01:21:13

by Mitchel Humpherys

[permalink] [raw]
Subject: Re: [PATCH RESEND v8] iopoll: Introduce memory-mapped IO polling macros

On Mon, Nov 24 2014 at 04:53:19 PM, "Elliott, Robert (Server Storage)" <[email protected]> wrote:
>> -----Original Message-----
>> From: [email protected] [mailto:linux-kernel-
>> [email protected]] On Behalf Of Mitchel Humpherys
>> Sent: Monday, 24 November, 2014 2:15 PM
> ...
>> From: Matt Wagantall <[email protected]>
>>
>> It is sometimes necessary to poll a memory-mapped register until its value
>> satisfies some condition. Introduce a family of convenience macros that do
>> this. Tight-looping, sleeping, and timing out can all be accomplished
>> using these macros.
>>
> ...
>> +#define readx_poll_timeout(op, addr, val, cond, sleep_us, timeout_us) \
>> +({ \
>> + ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
>> + might_sleep_if(sleep_us); \
>> + for (;;) { \
>> + (val) = op(addr); \
>> + if (cond) \
>> + break; \
>> + if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) {
>> \
>> + (val) = op(addr); \
>> + break; \
>> + } \
>> + if (sleep_us) \
>> + usleep_range((sleep_us >> 2) + 1, sleep_us); \
>
> The hpsa SCSI driver used to use usleep_range in a loop like
> that, but we found that it caused scheduler problems during
> boots because it uses TASK_UNINTERRUPTIBLE:
> [ 9.260668] [sched_delayed] sched: RT throttling activated
>
> msleep() worked much better.

Hmm, maybe you were just sleeping for too long? According to
Documentation/timers/timers-howto.txt, usleep_range is what should be
used for non-atomic sleeps in the range [10us, 20ms]. Plus we need
microsecond granularity anyways, so msleep wouldn't cut it.

If there are any potential users of these macros that would want to
sleep for more than 20ms I guess we could add a special case here to use
msleep when sleep_us exceeds 20,000 or so.


-Mitch

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Subject: RE: [PATCH RESEND v8] iopoll: Introduce memory-mapped IO polling macros



> -----Original Message-----
> From: Mitchel Humpherys [mailto:[email protected]]
> Sent: Monday, November 24, 2014 7:21 PM
> To: Elliott, Robert (Server Storage)
...
> > The hpsa SCSI driver used to use usleep_range in a loop like
> > that, but we found that it caused scheduler problems during
> > boots because it uses TASK_UNINTERRUPTIBLE:
> > [ 9.260668] [sched_delayed] sched: RT throttling activated
> >
> > msleep() worked much better.

Sorry, that might have been msleep_interruptible() - the fixes
are still not upstream, so I'll have to doublecheck tomorrow.

> Hmm, maybe you were just sleeping for too long? According to
> Documentation/timers/timers-howto.txt, usleep_range is what should be
> used for non-atomic sleeps in the range [10us, 20ms]. Plus we need
> microsecond granularity anyways, so msleep wouldn't cut it.

The intervals and the overall number of loops were indeed long. I
think the corrected code waits a total of 1 second; before the fix,
600 seconds.

> If there are any potential users of these macros that would want to
> sleep for more than 20ms I guess we could add a special case here to use
> msleep when sleep_us exceeds 20,000 or so.
...

Maybe just a comment in the documentation for the macro would suffice,
possibly with a separate macro for longer interval sleeps. I don't
want to force an extra msleep() call to be compiled into a bunch
of places where it's not needed.


---
Rob Elliott HP Server Storage

2014-12-02 16:55:59

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH RESEND v8] iopoll: Introduce memory-mapped IO polling macros

On Mon, Nov 24, 2014 at 08:14:52PM +0000, Mitchel Humpherys wrote:
> From: Matt Wagantall <[email protected]>
>
> It is sometimes necessary to poll a memory-mapped register until its value
> satisfies some condition. Introduce a family of convenience macros that do
> this. Tight-looping, sleeping, and timing out can all be accomplished using
> these macros.
>
> Cc: Thierry Reding <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Signed-off-by: Matt Wagantall <[email protected]>
> Signed-off-by: Mitchel Humpherys <[email protected]>
> ---
> This patch was originally part of a series [1] for adding support for IOMMU
> address translations through an ARM SMMU hardware register. The other
> patch in the series (the one that actually uses these macros and implements
> said hardware address translations) was Ack'd by the driver maintainer
> there (Will Deacon) so I've pulled this patch out to avoid resending an
> already Ack'd patch over and over again.
>
> In short, please see [1] for previous discussion and the first user of
> these macros.
>
> This patch has been resent previously here [2], here [3], and here [4] on
> 2014-10-30, 2014-11-06, and 2014-11-17, respectively. It has not changed
> since [2] and has not received any comments since [1] on 2014-10-19.
> Thanks to everyone who has taken a look at this.
>
> [1] http://thread.gmane.org/gmane.linux.kernel.iommu/7140
> [2] http://thread.gmane.org/gmane.linux.kernel/1818213
> [3] http://thread.gmane.org/gmane.linux.kernel/1823422
> [4] http://thread.gmane.org/gmane.linux.kernel.iommu/7394

Did this ever get queued? It would be pretty useful for a bunch of drivers
that seem to copy-paste this sort of code.

If it helps:

Acked-by: Will Deacon <[email protected]>

Will

> Changes since v7:
> - sorted helper macros by size (b, w, l, q)
> - removed some of the more esoteric (or flat-out bogus) helper macros
> ---
> include/linux/iopoll.h | 140 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 140 insertions(+)
> create mode 100644 include/linux/iopoll.h
>
> diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
> new file mode 100644
> index 0000000000..bd161dae2d
> --- /dev/null
> +++ b/include/linux/iopoll.h
> @@ -0,0 +1,140 @@
> +/*
> + * Copyright (c) 2012-2014 The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef _LINUX_IOPOLL_H
> +#define _LINUX_IOPOLL_H
> +
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/hrtimer.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/io.h>
> +
> +/**
> + * readx_poll_timeout - Periodically poll an address until a condition is met or a timeout occurs
> + * @op: accessor function (takes @addr as its only argument)
> + * @addr: Address to poll
> + * @val: Variable to read the value into
> + * @cond: Break condition (usually involving @val)
> + * @sleep_us: Maximum time to sleep between reads in us (0 tight-loops)
> + * @timeout_us: Timeout in us, 0 means never timeout
> + *
> + * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
> + * case, the last read value at @addr is stored in @val. Must not
> + * be called from atomic context if sleep_us or timeout_us are used.
> + *
> + * When available, you'll probably want to use one of the specialized
> + * macros defined below rather than this macro directly.
> + */
> +#define readx_poll_timeout(op, addr, val, cond, sleep_us, timeout_us) \
> +({ \
> + ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
> + might_sleep_if(sleep_us); \
> + for (;;) { \
> + (val) = op(addr); \
> + if (cond) \
> + break; \
> + if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \
> + (val) = op(addr); \
> + break; \
> + } \
> + if (sleep_us) \
> + usleep_range((sleep_us >> 2) + 1, sleep_us); \
> + } \
> + (cond) ? 0 : -ETIMEDOUT; \
> +})
> +
> +/**
> + * readx_poll_timeout_atomic - Periodically poll an address until a condition is met or a timeout occurs
> + * @op: accessor function (takes @addr as its only argument)
> + * @addr: Address to poll
> + * @val: Variable to read the value into
> + * @cond: Break condition (usually involving @val)
> + * @delay_us: Time to udelay between reads in us (0 tight-loops)
> + * @timeout_us: Timeout in us, 0 means never timeout
> + *
> + * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
> + * case, the last read value at @addr is stored in @val.
> + *
> + * When available, you'll probably want to use one of the specialized
> + * macros defined below rather than this macro directly.
> + */
> +#define readx_poll_timeout_atomic(op, addr, val, cond, delay_us, timeout_us) \
> +({ \
> + ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
> + for (;;) { \
> + (val) = op(addr); \
> + if (cond) \
> + break; \
> + if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \
> + (val) = op(addr); \
> + break; \
> + } \
> + if (delay_us) \
> + udelay(delay_us); \
> + } \
> + (cond) ? 0 : -ETIMEDOUT; \
> +})
> +
> +
> +#define readb_poll_timeout(addr, val, cond, delay_us, timeout_us) \
> + readx_poll_timeout(readb, addr, val, cond, delay_us, timeout_us)
> +
> +#define readb_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
> + readx_poll_timeout_atomic(readb, addr, val, cond, delay_us, timeout_us)
> +
> +#define readw_poll_timeout(addr, val, cond, delay_us, timeout_us) \
> + readx_poll_timeout(readw, addr, val, cond, delay_us, timeout_us)
> +
> +#define readw_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
> + readx_poll_timeout_atomic(readw, addr, val, cond, delay_us, timeout_us)
> +
> +#define readl_poll_timeout(addr, val, cond, delay_us, timeout_us) \
> + readx_poll_timeout(readl, addr, val, cond, delay_us, timeout_us)
> +
> +#define readl_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
> + readx_poll_timeout_atomic(readl, addr, val, cond, delay_us, timeout_us)
> +
> +#define readq_poll_timeout(addr, val, cond, delay_us, timeout_us) \
> + readx_poll_timeout(readq, addr, val, cond, delay_us, timeout_us)
> +
> +#define readq_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
> + readx_poll_timeout_atomic(readq, addr, val, cond, delay_us, timeout_us)
> +
> +#define readb_relaxed_poll_timeout(addr, val, cond, delay_us, timeout_us) \
> + readx_poll_timeout(readb_relaxed, addr, val, cond, delay_us, timeout_us)
> +
> +#define readb_relaxed_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
> + readx_poll_timeout_atomic(readb_relaxed, addr, val, cond, delay_us, timeout_us)
> +
> +#define readw_relaxed_poll_timeout(addr, val, cond, delay_us, timeout_us) \
> + readx_poll_timeout(readw_relaxed, addr, val, cond, delay_us, timeout_us)
> +
> +#define readw_relaxed_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
> + readx_poll_timeout_atomic(readw_relaxed, addr, val, cond, delay_us, timeout_us)
> +
> +#define readl_relaxed_poll_timeout(addr, val, cond, delay_us, timeout_us) \
> + readx_poll_timeout(readl_relaxed, addr, val, cond, delay_us, timeout_us)
> +
> +#define readl_relaxed_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
> + readx_poll_timeout_atomic(readl_relaxed, addr, val, cond, delay_us, timeout_us)
> +
> +#define readq_relaxed_poll_timeout(addr, val, cond, delay_us, timeout_us) \
> + readx_poll_timeout(readq_relaxed, addr, val, cond, delay_us, timeout_us)
> +
> +#define readq_relaxed_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
> + readx_poll_timeout_atomic(readq_relaxed, addr, val, cond, delay_us, timeout_us)
> +
> +#endif /* _LINUX_IOPOLL_H */
> --
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
>