[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

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

[Kalle Valo]

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

> 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>
> ---
>  include/linux/iopoll.h | 62 +++++++++++++++++++++++++++++-------------
>  1 file changed, 43 insertions(+), 19 deletions(-)

I don't know who maintains iopoll.h, at least MAINTAINERS file doesn't
have an entry, so not sure how to handle this patch.

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 380 bytes --]

On Thu, Apr 23, 2020 at 09:47:23AM +0300, Kalle Valo wrote:

> I don't know who maintains iopoll.h, at least MAINTAINERS file doesn't
> have an entry, so not sure how to handle this patch.

Andrew Moton often picks up things like that, or if it's used by some
other patch as the original message indicated then often whoever picks
up the user can pick up the core change as well.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Kalle Valo]

Mark Brown <broonie@kernel.org> writes:

> On Thu, Apr 23, 2020 at 09:47:23AM +0300, Kalle Valo wrote:
>
>> I don't know who maintains iopoll.h, at least MAINTAINERS file doesn't
>> have an entry, so not sure how to handle this patch.
>
> Andrew Moton often picks up things like that, or if it's used by some
> other patch as the original message indicated then often whoever picks
> up the user can pick up the core change as well.

Oh, ok. As patch 2 goes to my wireless-drivers-next tree would it be ok
for everyone if I take this patch as well?

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 294 bytes --]

On Thu, Apr 23, 2020 at 02:27:02PM +0300, Kalle Valo wrote:

> Oh, ok. As patch 2 goes to my wireless-drivers-next tree would it be ok
> for everyone if I take this patch as well?

I'd give it a bit longer than usual for people to review but yeah, if
nobody complains it's probably reasonable.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Kalle Valo]

Mark Brown <broonie@kernel.org> writes:

> On Thu, Apr 23, 2020 at 02:27:02PM +0300, Kalle Valo wrote:
>
>> Oh, ok. As patch 2 goes to my wireless-drivers-next tree would it be ok
>> for everyone if I take this patch as well?
>
> I'd give it a bit longer than usual for people to review but yeah, if
> nobody complains it's probably reasonable.

Yeah, I'll wait for comments. Thanks!

-- 
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