Skip to contentSkip to conversation

iopoll: Introduce ioreadXX_poll_timeout() macros

PATCH In review Andy Shevchenko 3 participants 7 messages Mar 30, 2023 – Apr 5, 2023

Subsystems (2)
USB Thunderbolt driver
lore
Share & tools
lore

Andy Shevchenko

author
#0 lore raw

PATCH 1/2 iopoll: Introduce ioreadXX_poll_timeout() macros

From: Olliver Schinagl <[email protected]>

There are users in the Linux kernel that would benefit from using ioreadXX_poll_timeout() macros, such as ioread32_poll_timeout(). Introduce those macros.

Signed-off-by Olliver Schinagl <[email protected]>
Co-developed-by Andy Shevchenko <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
---
 include/linux/iopoll.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
include/linux/iopoll.h +27 -0
index 2c8860e406bd..30ba609175a7 100644
@@ -140,6 +140,7 @@
 #define readx_poll_timeout_atomic(op, addr, val, cond, delay_us, timeout_us) \
 	read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, false, addr)
 
+/* readX() */
 #define readb_poll_timeout(addr, val, cond, delay_us, timeout_us) \
 	readx_poll_timeout(readb, addr, val, cond, delay_us, timeout_us)
 

@@ -164,6 +165,7 @@
 #define readq_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
 	readx_poll_timeout_atomic(readq, addr, val, cond, delay_us, timeout_us)
 
+/* readX_relaxed() */
 #define readb_relaxed_poll_timeout(addr, val, cond, delay_us, timeout_us) \
 	readx_poll_timeout(readb_relaxed, addr, val, cond, delay_us, timeout_us)
 

@@ -188,4 +190,29 @@
 #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)
 
+/* ioreadXX() */
+#define ioread8_poll_timeout(addr, val, cond, delay_us, timeout_us) \
+	readx_poll_timeout(ioread8, addr, val, cond, delay_us, timeout_us)
+
+#define ioread8_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
+	readx_poll_timeout_atomic(ioread8, addr, val, cond, delay_us, timeout_us)
+
+#define ioread16_poll_timeout(addr, val, cond, delay_us, timeout_us) \
+	readx_poll_timeout(ioread16, addr, val, cond, delay_us, timeout_us)
+
+#define ioread16_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
+	readx_poll_timeout_atomic(ioread16, addr, val, cond, delay_us, timeout_us)
+
+#define ioread32_poll_timeout(addr, val, cond, delay_us, timeout_us) \
+	readx_poll_timeout(ioread32, addr, val, cond, delay_us, timeout_us)
+
+#define ioread32_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
+	readx_poll_timeout_atomic(ioread32, addr, val, cond, delay_us, timeout_us)
+
+#define ioread64_poll_timeout(addr, val, cond, delay_us, timeout_us) \
+	readx_poll_timeout(ioread64, addr, val, cond, delay_us, timeout_us)
+
+#define ioread64_poll_timeout_atomic(addr, val, cond, delay_us, timeout_us) \
+	readx_poll_timeout_atomic(ioread64, addr, val, cond, delay_us, timeout_us)
+
 #endif /* _LINUX_IOPOLL_H */
Signature · 1 line
2.40.0.1.gaa8946217a0b
author
#1 lore raw

PATCH 2/2 thunderbolt: Make use of ioread32_poll_timeout()

Instead of open coding the same routine switch the code to use ioread32_poll_timeout().

Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/thunderbolt/nhi.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)
drivers/thunderbolt/nhi.c +8 -12
index d76e923fbc6a..d0bf11b40131 100644
@@ -16,8 +16,8 @@
 #include <linux/dma-mapping.h>
 #include <linux/interrupt.h>
 #include <linux/iommu.h>
+#include <linux/iopoll.h>
 #include <linux/module.h>
-#include <linux/delay.h>
 #include <linux/property.h>
 #include <linux/string_helpers.h>
 

@@ -40,7 +40,7 @@
 #define MSIX_MIN_VECS		6
 #define MSIX_MAX_VECS		16
 
-#define NHI_MAILBOX_TIMEOUT	500 /* ms */
+#define NHI_MAILBOX_TIMEOUT_US	500000
 
 /* Host interface quirks */
 #define QUIRK_AUTO_CLEAR_INT	BIT(0)

@@ -830,8 +830,8 @@ EXPORT_SYMBOL_GPL(tb_ring_free);
  */
 int nhi_mailbox_cmd(struct tb_nhi *nhi, enum nhi_mailbox_cmd cmd, u32 data)
 {
-	ktime_t timeout;
 	u32 val;
+	int ret;
 
 	iowrite32(data, nhi->iobase + REG_INMAIL_DATA);
 

@@ -840,16 +840,12 @@ int nhi_mailbox_cmd(struct tb_nhi *nhi, enum nhi_mailbox_cmd cmd, u32 data)
 	val |= REG_INMAIL_OP_REQUEST | cmd;
 	iowrite32(val, nhi->iobase + REG_INMAIL_CMD);
 
-	timeout = ktime_add_ms(ktime_get(), NHI_MAILBOX_TIMEOUT);
-	do {
-		val = ioread32(nhi->iobase + REG_INMAIL_CMD);
-		if (!(val & REG_INMAIL_OP_REQUEST))
-			break;
-		usleep_range(10, 20);
-	} while (ktime_before(ktime_get(), timeout));
+	ret = ioread32_poll_timeout(nhi->iobase + REG_INMAIL_CMD,
+				    val, !(val & REG_INMAIL_OP_REQUEST),
+				    40, NHI_MAILBOX_TIMEOUT_US);
+	if (ret)
+		return ret;
 
-	if (val & REG_INMAIL_OP_REQUEST)
-		return -ETIMEDOUT;
 	if (val & REG_INMAIL_ERROR)
 		return -EIO;
Signature · 1 line
2.40.0.1.gaa8946217a0b
#2 lore raw
In reply to Andy Shevchenko

Hey Andy,

Quoted 18 lines · Andy Shevchenko

From: Olliver Schinagl <[email protected]>

There are users in the Linux kernel that would benefit from using ioreadXX_poll_timeout() macros, such as ioread32_poll_timeout(). Introduce those macros.

Signed-off-by Olliver Schinagl <[email protected]>
Co-developed-by Andy Shevchenko <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
---
  include/linux/iopoll.h | 27 +++++++++++++++++++++++++++
  1 file changed, 27 insertions(+)
include/linux/iopoll.h +0 -0
index 2c8860e406bd..30ba609175a7 100644

include/linux/iopoll.h

Quoted 9 lines
include/linux/iopoll.h +1 -0
@@ -140,6 +140,7 @@
  #define readx_poll_timeout_atomic(op, addr, val, cond, delay_us, timeout_us) \
  	read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, false, addr)
 
+/* readX() */
  #define readb_poll_timeout(addr, val, cond, delay_us, timeout_us) \
  	readx_poll_timeout(readb, addr, val, cond, delay_us, timeout_us)
 
<snip>
  #endif /* _LINUX_IOPOLL_H */

Thanks for re-posting this? (I don't recall If I actually posted the first time around).

But I actually have a small addendum for this series, as it will break drivers/net/wwan/t7xx/t7xx_dpmaif.c due to a redefinition (yeah I know).

So I have:
https://gitlab.com/olliver/linux/-/commit/c9e591f2dabb2dbaeceebee61fa70b70fdbffc2a
https://gitlab.com/olliver/linux/-/commit/41e0f8c08a1c55940813a240215910336ad7bec2
https://gitlab.com/olliver/linux/-/commit/f36562f09b0185d403415864ef7218b46a742cdc
https://gitlab.com/olliver/linux/-/commit/66237fd97bc42d272602b01dc0cca541c619b2be

Which actually replaces silly calls like readx_poll_timeout(ioread32, ... .

Do you want me to (re-?)post this as a series? I can put your thunderbolt (and your changes here) and post as one big series (or you can do the same obviously).

Olliver

author
#3 lore raw
Quoted 1 line · Olliver Schinagl

On 30-03-2023 16:14, Andy Shevchenko wrote:

...

Quoted 9 lines

But I actually have a small addendum for this series, as it will break drivers/net/wwan/t7xx/t7xx_dpmaif.c due to a redefinition (yeah I know).

So I have:
https://gitlab.com/olliver/linux/-/commit/c9e591f2dabb2dbaeceebee61fa70b70fdbffc2a
https://gitlab.com/olliver/linux/-/commit/41e0f8c08a1c55940813a240215910336ad7bec2
https://gitlab.com/olliver/linux/-/commit/f36562f09b0185d403415864ef7218b46a742cdc
https://gitlab.com/olliver/linux/-/commit/66237fd97bc42d272602b01dc0cca541c619b2be

Quoted 5 lines

Which actually replaces silly calls like readx_poll_timeout(ioread32, ... .

Do you want me to (re-?)post this as a series? I can put your thunderbolt (and your changes here) and post as one big series (or you can do the same obviously).

I believe the best course of action is to collect this series and your fix in your hands and send it as a material for Linux WWAN subsystem with Mika's Ack (if he has no objections). That said, let's wait for Mika's answer on this.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#4 lore raw

Hi,

Quoted 16 lines · Andy Shevchenko
Quoted 1 line · Olliver Schinagl

On 30-03-2023 16:14, Andy Shevchenko wrote:

...

Quoted 11 lines

But I actually have a small addendum for this series, as it will break drivers/net/wwan/t7xx/t7xx_dpmaif.c due to a redefinition (yeah I know).

So I have:
https://gitlab.com/olliver/linux/-/commit/c9e591f2dabb2dbaeceebee61fa70b70fdbffc2a
https://gitlab.com/olliver/linux/-/commit/41e0f8c08a1c55940813a240215910336ad7bec2
https://gitlab.com/olliver/linux/-/commit/f36562f09b0185d403415864ef7218b46a742cdc
https://gitlab.com/olliver/linux/-/commit/66237fd97bc42d272602b01dc0cca541c619b2be

Which actually replaces silly calls like readx_poll_timeout(ioread32, ... .

Quoted 8 lines
Quoted 3 lines

Do you want me to (re-?)post this as a series? I can put your thunderbolt (and your changes here) and post as one big series (or you can do the same obviously).

I believe the best course of action is to collect this series and your fix in your hands and send it as a material for Linux WWAN subsystem with Mika's Ack (if he has no objections). That said, let's wait for Mika's answer on this.

No objections from my side.

author
#5 lore raw
Quoted 3 lines · Mika Westerberg
Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Olliver Schinagl

On 30-03-2023 16:14, Andy Shevchenko wrote:

...

Quoted 15 lines
Quoted 15 lines
Quoted 14 lines

But I actually have a small addendum for this series, as it will break drivers/net/wwan/t7xx/t7xx_dpmaif.c due to a redefinition (yeah I know).

So I have:
https://gitlab.com/olliver/linux/-/commit/c9e591f2dabb2dbaeceebee61fa70b70fdbffc2a
https://gitlab.com/olliver/linux/-/commit/41e0f8c08a1c55940813a240215910336ad7bec2
https://gitlab.com/olliver/linux/-/commit/f36562f09b0185d403415864ef7218b46a742cdc
https://gitlab.com/olliver/linux/-/commit/66237fd97bc42d272602b01dc0cca541c619b2be

Which actually replaces silly calls like readx_poll_timeout(ioread32, ... .

Do you want me to (re-?)post this as a series? I can put your thunderbolt (and your changes here) and post as one big series (or you can do the same obviously).

Quoted 6 lines
Quoted 4 lines

I believe the best course of action is to collect this series and your fix in your hands and send it as a material for Linux WWAN subsystem with Mika's Ack (if he has no objections). That said, let's wait for Mika's answer on this.

No objections from my side.

Thank you, Mika!

This implies that I stop issuing a new version and Olliver takes over.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#6 lore raw
Quoted 4 lines · Andy Shevchenko
Quoted 3 lines · Mika Westerberg
Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Olliver Schinagl

On 30-03-2023 16:14, Andy Shevchenko wrote:

...

Quoted 20 lines
Quoted 20 lines
Quoted 19 lines
Quoted 9 lines

But I actually have a small addendum for this series, as it will break drivers/net/wwan/t7xx/t7xx_dpmaif.c due to a redefinition (yeah I know).

So I have:
https://gitlab.com/olliver/linux/-/commit/c9e591f2dabb2dbaeceebee61fa70b70fdbffc2a
https://gitlab.com/olliver/linux/-/commit/41e0f8c08a1c55940813a240215910336ad7bec2
https://gitlab.com/olliver/linux/-/commit/f36562f09b0185d403415864ef7218b46a742cdc
https://gitlab.com/olliver/linux/-/commit/66237fd97bc42d272602b01dc0cca541c619b2be

Quoted 5 lines

Which actually replaces silly calls like readx_poll_timeout(ioread32, ... .

Do you want me to (re-?)post this as a series? I can put your thunderbolt (and your changes here) and post as one big series (or you can do the same obviously).

I believe the best course of action is to collect this series and your fix in your hands and send it as a material for Linux WWAN subsystem with Mika's Ack (if he has no objections). That said, let's wait for Mika's answer on this.

Quoted 5 lines
Quoted 1 line

No objections from my side.

Thank you, Mika!

This implies that I stop issuing a new version and Olliver takes over.

Olliver, just note that I reduced a lot your initial patch in a sense to add only what is close to the existing users, i.e. ioreadXX() and their atomic versions (no be(), no _rep() variants). If you are objecting that, you need to have yet another patch(es) to prove that there are users.

Also note, that I reordered macro appearance to be consistent with existing schema.

Signature · 2 lines
With Best Regards,
Andy Shevchenko