[PATCH 2/2] rtw88: Use udelay instead of usleep in atomic context

[PATCH 2/2] rtw88: Use udelay instead of usleep in atomic context

[Kai-Heng Feng]

It's incorrect to use usleep in atomic context.

Switch to a macro which uses udelay instead of usleep to prevent the issue.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/net/wireless/realtek/rtw88/fw.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
index 245da96dfddc..8f998b4a7234 100644
--- a/drivers/net/wireless/realtek/rtw88/fw.c
+++ b/drivers/net/wireless/realtek/rtw88/fw.c
@@ -228,9 +228,9 @@ static void rtw_fw_send_h2c_command(struct rtw_dev *rtwdev,
 		goto out;
 	}
 
-	ret = read_poll_timeout(rtw_read8, box_state,
-				!((box_state >> box) & 0x1), 100, 3000, false,
-				rtwdev, REG_HMETFR);
+	ret = read_poll_timeout_atomic(rtw_read8, box_state,
+				       !((box_state >> box) & 0x1), 100, 3000,
+				       false, rtwdev, REG_HMETFR);
 
 	if (ret) {
 		rtw_err(rtwdev, "failed to send h2c command\n");
-- 
2.17.1

Re: [PATCH 2/2] rtw88: Use udelay instead of usleep in atomic context

[Kalle Valo]

Kai-Heng Feng <kai.heng.feng@canonical.com> writes:

> It's incorrect to use usleep in atomic context.
>
> Switch to a macro which uses udelay instead of usleep to prevent the issue.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

This fixes a regression, right? So there should be a Fixes line.

Also I can't take this until patch 1 is in my tree. And I don't know who
takes iopoll.h patches.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 2/2] rtw88: Use udelay instead of usleep in atomic context

[Kai-Heng Feng]

> On Apr 23, 2020, at 14:49, Kalle Valo <kvalo@codeaurora.org> wrote:
> 
> Kai-Heng Feng <kai.heng.feng@canonical.com> writes:
> 
>> It's incorrect to use usleep in atomic context.
>> 
>> Switch to a macro which uses udelay instead of usleep to prevent the issue.
>> 
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> 
> This fixes a regression, right? So there should be a Fixes line.

Yes, but the regression commit isn't in Linus' tree, so the sha1 may change.

Kai-Heng

> 
> Also I can't take this until patch 1 is in my tree. And I don't know who
> takes iopoll.h patches.
> 
> -- 
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 2/2] rtw88: Use udelay instead of usleep in atomic context

[Kalle Valo]

Kai-Heng Feng <kai.heng.feng@canonical.com> writes:

>> On Apr 23, 2020, at 14:49, Kalle Valo <kvalo@codeaurora.org> wrote:
>> 
>> Kai-Heng Feng <kai.heng.feng@canonical.com> writes:
>> 
>>> It's incorrect to use usleep in atomic context.
>>> 
>>> Switch to a macro which uses udelay instead of usleep to prevent the issue.
>>> 
>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> 
>> This fixes a regression, right? So there should be a Fixes line.
>
> Yes, but the regression commit isn't in Linus' tree, so the sha1 may change.

No, the commit id won't change after I have commited the patch. I don't
rebase my trees.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH v2 2/2] rtw88: Use udelay instead of usleep in atomic context

[Kai-Heng Feng]

It's incorrect to use usleep in atomic context.

Switch to a macro which uses udelay instead of usleep to prevent the issue.

Fixes: 6343a6d4b213 ("rtw88: Add delay on polling h2c command status bit")
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v2:
 - Add Fixes tag.

 drivers/net/wireless/realtek/rtw88/fw.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
index 245da96dfddc..8f998b4a7234 100644
--- a/drivers/net/wireless/realtek/rtw88/fw.c
+++ b/drivers/net/wireless/realtek/rtw88/fw.c
@@ -228,9 +228,9 @@ static void rtw_fw_send_h2c_command(struct rtw_dev *rtwdev,
 		goto out;
 	}
 
-	ret = read_poll_timeout(rtw_read8, box_state,
-				!((box_state >> box) & 0x1), 100, 3000, false,
-				rtwdev, REG_HMETFR);
+	ret = read_poll_timeout_atomic(rtw_read8, box_state,
+				       !((box_state >> box) & 0x1), 100, 3000,
+				       false, rtwdev, REG_HMETFR);
 
 	if (ret) {
 		rtw_err(rtwdev, "failed to send h2c command\n");
-- 
2.17.1

Re: [PATCH v2 2/2] rtw88: Use udelay instead of usleep in atomic context

[Kalle Valo]

Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:

> It's incorrect to use usleep in atomic context.
> 
> Switch to a macro which uses udelay instead of usleep to prevent the issue.
> 
> Fixes: 6343a6d4b213 ("rtw88: Add delay on polling h2c command status bit")
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

For patch 1 please also CC linux-wireless, otherwise patchwork cannot see it.

-- 
https://patchwork.kernel.org/patch/11505147/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2 2/2] rtw88: Use udelay instead of usleep in atomic context

[Kalle Valo]

Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:

> It's incorrect to use usleep in atomic context.
> 
> Switch to a macro which uses udelay instead of usleep to prevent the issue.
> 
> Fixes: 6343a6d4b213 ("rtw88: Add delay on polling h2c command status bit")
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

Patch applied to wireless-drivers-next.git, thanks.

fd5d781964b0 rtw88: Use udelay instead of usleep in atomic context

-- 
https://patchwork.kernel.org/patch/11505147/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH v2 1/2] iopoll: Introduce read_poll_timeout_atomic macro

[Kai-Heng Feng]

Like read_poll_timeout, an atomic variant for multiple parameter read
function can be useful.

Will be used by a later patch.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v2:
 - Cc linux-wireless.

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

diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
index cb20c733b15a..bc89ac625f26 100644
--- a/include/linux/iopoll.h
+++ b/include/linux/iopoll.h
@@ -57,6 +57,48 @@
 	(cond) ? 0 : -ETIMEDOUT; \
 })
 
+/**
+ * read_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).  Should
+ *            be less than ~10us since udelay is used (see
+ *            Documentation/timers/timers-howto.rst).
+ * @timeout_us: Timeout in us, 0 means never timeout
+ * @delay_before_read: if it is true, delay @delay_us before read.
+ *
+ * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
+ * case, the last read value at @args 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 read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, \
+					delay_before_read, args...) \
+({ \
+	u64 __timeout_us = (timeout_us); \
+	unsigned long __delay_us = (delay_us); \
+	ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
+	if (delay_before_read && __delay_us) \
+		udelay(__delay_us); \
+	for (;;) { \
+		(val) = op(args); \
+		if (cond) \
+			break; \
+		if (__timeout_us && \
+		    ktime_compare(ktime_get(), __timeout) > 0) { \
+			(val) = op(args); \
+			break; \
+		} \
+		if (__delay_us) \
+			udelay(__delay_us); \
+	} \
+	(cond) ? 0 : -ETIMEDOUT; \
+})
+
 /**
  * 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)
@@ -96,25 +138,7 @@
  * macros defined below rather than this macro directly.
  */
 #define readx_poll_timeout_atomic(op, addr, val, cond, delay_us, timeout_us) \
-({ \
-	u64 __timeout_us = (timeout_us); \
-	unsigned long __delay_us = (delay_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; \
-})
-
+	read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, false, addr)
 
 #define readb_poll_timeout(addr, val, cond, delay_us, timeout_us) \
 	readx_poll_timeout(readb, addr, val, cond, delay_us, timeout_us)
-- 
2.17.1

Re: [PATCH v2 1/2] iopoll: Introduce read_poll_timeout_atomic macro

[Kalle Valo]

Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:

> Like read_poll_timeout, an atomic variant for multiple parameter read
> function can be useful.
> 
> Will be used by a later patch.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

Patch applied to wireless-drivers-next.git, thanks.

57a29df34146 iopoll: Introduce read_poll_timeout_atomic macro

-- 
https://patchwork.kernel.org/patch/11508809/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches