* [PATCH 0/4] ath10k: a few incorrect return handling fix-up
@ 2014-12-30 12:20 ` Nicholas Mc Guire
0 siblings, 0 replies; 19+ messages in thread
From: Nicholas Mc Guire @ 2014-12-30 12:20 UTC (permalink / raw)
To: Kalle Valo
Cc: Michal Kazior, Ben Greear, Chun-Yeow Yeoh, Yanbo Li, ath10k,
linux-wireless, netdev, linux-kernel, Nicholas Mc Guire
wait_for_completion_timeout does not return negative values so the tests
for <= 0 are not needed and the case differentiation in the error handling
path unnecessary.
patch was only compile tested x86_64_defconfig + CONFIG_ATH_CARDS=m
CONFIG_ATH10K=m
patch is against linux-next 3.19.0-rc1 -next-20141226
None of the proposed cleanups are critical.
All changes should only be removing unreachable cases.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 0/4] ath10k: a few incorrect return handling fix-up
@ 2014-12-30 12:20 ` Nicholas Mc Guire
0 siblings, 0 replies; 19+ messages in thread
From: Nicholas Mc Guire @ 2014-12-30 12:20 UTC (permalink / raw)
To: Kalle Valo
Cc: Chun-Yeow Yeoh, netdev, linux-wireless, linux-kernel, ath10k,
Nicholas Mc Guire, Michal Kazior, Yanbo Li, Ben Greear
wait_for_completion_timeout does not return negative values so the tests
for <= 0 are not needed and the case differentiation in the error handling
path unnecessary.
patch was only compile tested x86_64_defconfig + CONFIG_ATH_CARDS=m
CONFIG_ATH10K=m
patch is against linux-next 3.19.0-rc1 -next-20141226
None of the proposed cleanups are critical.
All changes should only be removing unreachable cases.
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/4] ath10k: fixup wait_for_completion_timeout return handling
2014-12-30 12:20 ` Nicholas Mc Guire
(?)
@ 2014-12-30 12:20 ` Nicholas Mc Guire
-1 siblings, 0 replies; 19+ messages in thread
From: Nicholas Mc Guire @ 2014-12-30 12:20 UTC (permalink / raw)
To: Kalle Valo
Cc: Michal Kazior, Ben Greear, Chun-Yeow Yeoh, Yanbo Li, ath10k,
linux-wireless, netdev, linux-kernel, Nicholas Mc Guire
wait_for_completion_timeout does not return negative values so
"ret" handling here should check for timeout only.
patch was only compile tested for x86_64_defconfig + CONFIG_ATH_DEBUG=y
patch is against linux-next 3.19.0-rc1 -next-20141226
Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
---
drivers/net/wireless/ath/ath10k/debug.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index a716758..7e1fe93 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -373,7 +373,7 @@ static int ath10k_debug_fw_stats_request(struct ath10k *ar)
ret = wait_for_completion_timeout(&ar->debug.fw_stats_complete,
1*HZ);
- if (ret <= 0)
+ if (ret == 0)
return -ETIMEDOUT;
spin_lock_bh(&ar->data_lock);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 1/4] ath10k: fixup wait_for_completion_timeout return handling
@ 2014-12-30 12:20 ` Nicholas Mc Guire
0 siblings, 0 replies; 19+ messages in thread
From: Nicholas Mc Guire @ 2014-12-30 12:20 UTC (permalink / raw)
To: Kalle Valo
Cc: Chun-Yeow Yeoh, netdev, linux-wireless, linux-kernel, ath10k,
Nicholas Mc Guire, Michal Kazior, Yanbo Li, Ben Greear
wait_for_completion_timeout does not return negative values so
"ret" handling here should check for timeout only.
patch was only compile tested for x86_64_defconfig + CONFIG_ATH_DEBUG=y
patch is against linux-next 3.19.0-rc1 -next-20141226
Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
---
drivers/net/wireless/ath/ath10k/debug.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index a716758..7e1fe93 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -373,7 +373,7 @@ static int ath10k_debug_fw_stats_request(struct ath10k *ar)
ret = wait_for_completion_timeout(&ar->debug.fw_stats_complete,
1*HZ);
- if (ret <= 0)
+ if (ret == 0)
return -ETIMEDOUT;
spin_lock_bh(&ar->data_lock);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 1/4] ath10k: fixup wait_for_completion_timeout return handling
@ 2014-12-30 12:20 ` Nicholas Mc Guire
0 siblings, 0 replies; 19+ messages in thread
From: Nicholas Mc Guire @ 2014-12-30 12:20 UTC (permalink / raw)
To: Kalle Valo
Cc: Chun-Yeow Yeoh, netdev, linux-wireless, linux-kernel, ath10k,
Nicholas Mc Guire, Michal Kazior, Yanbo Li, Ben Greear
wait_for_completion_timeout does not return negative values so
"ret" handling here should check for timeout only.
patch was only compile tested for x86_64_defconfig + CONFIG_ATH_DEBUG=y
patch is against linux-next 3.19.0-rc1 -next-20141226
Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
---
drivers/net/wireless/ath/ath10k/debug.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index a716758..7e1fe93 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -373,7 +373,7 @@ static int ath10k_debug_fw_stats_request(struct ath10k *ar)
ret = wait_for_completion_timeout(&ar->debug.fw_stats_complete,
1*HZ);
- if (ret <= 0)
+ if (ret == 0)
return -ETIMEDOUT;
spin_lock_bh(&ar->data_lock);
--
1.7.10.4
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/4] ath10k: fixup wait_for_completion_timeout return handling
2014-12-30 12:20 ` Nicholas Mc Guire
@ 2014-12-30 12:20 ` Nicholas Mc Guire
-1 siblings, 0 replies; 19+ messages in thread
From: Nicholas Mc Guire @ 2014-12-30 12:20 UTC (permalink / raw)
To: Kalle Valo
Cc: Michal Kazior, Ben Greear, Chun-Yeow Yeoh, Yanbo Li, ath10k,
linux-wireless, netdev, linux-kernel, Nicholas Mc Guire
wait_for_completion_timeout does not return negative values so
"ret" handling here should check for timeout only.
patch was only compile tested for x86_64_defconfig + CONFIG_ATH10K=m
patch is against linux-next 3.19.0-rc1 -next-20141226
Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
---
drivers/net/wireless/ath/ath10k/mac.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index c400567..f9d7dbb 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2151,7 +2151,7 @@ void ath10k_offchan_tx_work(struct work_struct *work)
ret = wait_for_completion_timeout(&ar->offchan_tx_completed,
3 * HZ);
- if (ret <= 0)
+ if (ret == 0)
ath10k_warn(ar, "timed out waiting for offchannel skb %p\n",
skb);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/4] ath10k: fixup wait_for_completion_timeout return handling
@ 2014-12-30 12:20 ` Nicholas Mc Guire
0 siblings, 0 replies; 19+ messages in thread
From: Nicholas Mc Guire @ 2014-12-30 12:20 UTC (permalink / raw)
To: Kalle Valo
Cc: Chun-Yeow Yeoh, netdev, linux-wireless, linux-kernel, ath10k,
Nicholas Mc Guire, Michal Kazior, Yanbo Li, Ben Greear
wait_for_completion_timeout does not return negative values so
"ret" handling here should check for timeout only.
patch was only compile tested for x86_64_defconfig + CONFIG_ATH10K=m
patch is against linux-next 3.19.0-rc1 -next-20141226
Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
---
drivers/net/wireless/ath/ath10k/mac.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index c400567..f9d7dbb 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2151,7 +2151,7 @@ void ath10k_offchan_tx_work(struct work_struct *work)
ret = wait_for_completion_timeout(&ar->offchan_tx_completed,
3 * HZ);
- if (ret <= 0)
+ if (ret == 0)
ath10k_warn(ar, "timed out waiting for offchannel skb %p\n",
skb);
--
1.7.10.4
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/4] ath10k: fixup wait_for_completion_timeout return handling
2014-12-30 12:20 ` Nicholas Mc Guire
@ 2014-12-30 12:20 ` Nicholas Mc Guire
-1 siblings, 0 replies; 19+ messages in thread
From: Nicholas Mc Guire @ 2014-12-30 12:20 UTC (permalink / raw)
To: Kalle Valo
Cc: Michal Kazior, Ben Greear, Chun-Yeow Yeoh, Yanbo Li, ath10k,
linux-wireless, netdev, linux-kernel, Nicholas Mc Guire
wait_for_completion_timeout does not return negative values so
"ret" handling here should check for timeout only.
patch was only compile tested for x86_64_defconfig + CONFIG_ATH10K=m
patch is against linux-next 3.19.0-rc1 -next-20141226
Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
---
drivers/net/wireless/ath/ath10k/htc.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index f1946a6..2fd9e18 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -703,11 +703,9 @@ int ath10k_htc_connect_service(struct ath10k_htc *htc,
/* wait for response */
status = wait_for_completion_timeout(&htc->ctl_resp,
ATH10K_HTC_CONN_SVC_TIMEOUT_HZ);
- if (status <= 0) {
- if (status == 0)
- status = -ETIMEDOUT;
+ if (status == 0) {
ath10k_err(ar, "Service connect timeout: %d\n", status);
- return status;
+ return -ETIMEDOUT;
}
/* we controlled the buffer creation, it's aligned */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/4] ath10k: fixup wait_for_completion_timeout return handling
@ 2014-12-30 12:20 ` Nicholas Mc Guire
0 siblings, 0 replies; 19+ messages in thread
From: Nicholas Mc Guire @ 2014-12-30 12:20 UTC (permalink / raw)
To: Kalle Valo
Cc: Chun-Yeow Yeoh, netdev, linux-wireless, linux-kernel, ath10k,
Nicholas Mc Guire, Michal Kazior, Yanbo Li, Ben Greear
wait_for_completion_timeout does not return negative values so
"ret" handling here should check for timeout only.
patch was only compile tested for x86_64_defconfig + CONFIG_ATH10K=m
patch is against linux-next 3.19.0-rc1 -next-20141226
Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
---
drivers/net/wireless/ath/ath10k/htc.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index f1946a6..2fd9e18 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -703,11 +703,9 @@ int ath10k_htc_connect_service(struct ath10k_htc *htc,
/* wait for response */
status = wait_for_completion_timeout(&htc->ctl_resp,
ATH10K_HTC_CONN_SVC_TIMEOUT_HZ);
- if (status <= 0) {
- if (status == 0)
- status = -ETIMEDOUT;
+ if (status == 0) {
ath10k_err(ar, "Service connect timeout: %d\n", status);
- return status;
+ return -ETIMEDOUT;
}
/* we controlled the buffer creation, it's aligned */
--
1.7.10.4
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 4/4] ath10k: fixup wait_for_completion_timeout return handling
2014-12-30 12:20 ` Nicholas Mc Guire
@ 2014-12-30 12:20 ` Nicholas Mc Guire
-1 siblings, 0 replies; 19+ messages in thread
From: Nicholas Mc Guire @ 2014-12-30 12:20 UTC (permalink / raw)
To: Kalle Valo
Cc: Michal Kazior, Ben Greear, Chun-Yeow Yeoh, Yanbo Li, ath10k,
linux-wireless, netdev, linux-kernel, Nicholas Mc Guire
wait_for_completion_timeout does not return negative values so
"ret" handling here should check for timeout only.
patch was only compile tested for x86_64_defconfig + CONFIG_ATH10K=m
patch is against linux-next 3.19.0-rc1 -next-20141226
Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
---
drivers/net/wireless/ath/ath10k/htt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/htt.c b/drivers/net/wireless/ath/ath10k/htt.c
index 56cb4ac..58b6fc1 100644
--- a/drivers/net/wireless/ath/ath10k/htt.c
+++ b/drivers/net/wireless/ath/ath10k/htt.c
@@ -102,7 +102,7 @@ int ath10k_htt_setup(struct ath10k_htt *htt)
status = wait_for_completion_timeout(&htt->target_version_received,
HTT_TARGET_VERSION_TIMEOUT_HZ);
- if (status <= 0) {
+ if (status == 0) {
ath10k_warn(ar, "htt version request timed out\n");
return -ETIMEDOUT;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 4/4] ath10k: fixup wait_for_completion_timeout return handling
@ 2014-12-30 12:20 ` Nicholas Mc Guire
0 siblings, 0 replies; 19+ messages in thread
From: Nicholas Mc Guire @ 2014-12-30 12:20 UTC (permalink / raw)
To: Kalle Valo
Cc: Chun-Yeow Yeoh, netdev, linux-wireless, linux-kernel, ath10k,
Nicholas Mc Guire, Michal Kazior, Yanbo Li, Ben Greear
wait_for_completion_timeout does not return negative values so
"ret" handling here should check for timeout only.
patch was only compile tested for x86_64_defconfig + CONFIG_ATH10K=m
patch is against linux-next 3.19.0-rc1 -next-20141226
Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
---
drivers/net/wireless/ath/ath10k/htt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/htt.c b/drivers/net/wireless/ath/ath10k/htt.c
index 56cb4ac..58b6fc1 100644
--- a/drivers/net/wireless/ath/ath10k/htt.c
+++ b/drivers/net/wireless/ath/ath10k/htt.c
@@ -102,7 +102,7 @@ int ath10k_htt_setup(struct ath10k_htt *htt)
status = wait_for_completion_timeout(&htt->target_version_received,
HTT_TARGET_VERSION_TIMEOUT_HZ);
- if (status <= 0) {
+ if (status == 0) {
ath10k_warn(ar, "htt version request timed out\n");
return -ETIMEDOUT;
}
--
1.7.10.4
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 0/4] ath10k: a few incorrect return handling fix-up
2014-12-30 12:20 ` Nicholas Mc Guire
@ 2014-12-30 17:18 ` Sergei Shtylyov
-1 siblings, 0 replies; 19+ messages in thread
From: Sergei Shtylyov @ 2014-12-30 17:18 UTC (permalink / raw)
To: Nicholas Mc Guire, Kalle Valo
Cc: Michal Kazior, Ben Greear, Chun-Yeow Yeoh, Yanbo Li, ath10k,
linux-wireless, netdev, linux-kernel
Hello.
On 12/30/2014 03:20 PM, Nicholas Mc Guire wrote:
> wait_for_completion_timeout does not return negative values so the tests
> for <= 0 are not needed and the case differentiation in the error handling
> path unnecessary.
I decided to verify your statement and I saw that it seems wrong.
do_wait_for_common() can return -ERESTARTSYS and the return value gets
returned by its callers unchanged.
> patch was only compile tested x86_64_defconfig + CONFIG_ATH_CARDS=m
> CONFIG_ATH10K=m
> patch is against linux-next 3.19.0-rc1 -next-20141226
Rather patches. It would have been better to send one patch instead of 4
patches with the same name.
WBR, Sergei
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/4] ath10k: a few incorrect return handling fix-up
@ 2014-12-30 17:18 ` Sergei Shtylyov
0 siblings, 0 replies; 19+ messages in thread
From: Sergei Shtylyov @ 2014-12-30 17:18 UTC (permalink / raw)
To: Nicholas Mc Guire, Kalle Valo
Cc: Chun-Yeow Yeoh, netdev, linux-wireless, linux-kernel, ath10k,
Michal Kazior, Yanbo Li, Ben Greear
Hello.
On 12/30/2014 03:20 PM, Nicholas Mc Guire wrote:
> wait_for_completion_timeout does not return negative values so the tests
> for <= 0 are not needed and the case differentiation in the error handling
> path unnecessary.
I decided to verify your statement and I saw that it seems wrong.
do_wait_for_common() can return -ERESTARTSYS and the return value gets
returned by its callers unchanged.
> patch was only compile tested x86_64_defconfig + CONFIG_ATH_CARDS=m
> CONFIG_ATH10K=m
> patch is against linux-next 3.19.0-rc1 -next-20141226
Rather patches. It would have been better to send one patch instead of 4
patches with the same name.
WBR, Sergei
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/4] ath10k: a few incorrect return handling fix-up
2014-12-30 17:18 ` Sergei Shtylyov
@ 2014-12-30 18:28 ` Nicholas Mc Guire
-1 siblings, 0 replies; 19+ messages in thread
From: Nicholas Mc Guire @ 2014-12-30 18:28 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Kalle Valo, Michal Kazior, Ben Greear, Chun-Yeow Yeoh, Yanbo Li,
ath10k, linux-wireless, netdev, linux-kernel
On Tue, 30 Dec 2014, Sergei Shtylyov wrote:
> Hello.
>
> On 12/30/2014 03:20 PM, Nicholas Mc Guire wrote:
>
>> wait_for_completion_timeout does not return negative values so the tests
>> for <= 0 are not needed and the case differentiation in the error handling
>> path unnecessary.
>
> I decided to verify your statement and I saw that it seems wrong.
> do_wait_for_common() can return -ERESTARTSYS and the return value gets
> returned by its callers unchanged.
the -ERESTARTSYS only can be returned if state matches but
wait_for_completion_timemout passes TASK_UNINTERRUPTIBLE
so signal_pending_state will return 0 and never negativ
my understanding of the callchain is:
wait_for_completion_timemout with TASK_UNINTERRUPTIBLE
-> wait_for_common(...TASK_UNINTERRUPTIBLE)
-> __wait_for_common(...TASK_UNINTERRUPTIBLE)
-> do_wait_for_common(...TASK_UNINTERRUPTIBLE)
-> signal_pending_state(TASK_UNINTERRUPTIBLE...)
static inline int signal_pending_state(long state, struct task_struct *p)
{
if (!(state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL)))
return 0;
so wait_for_completion_timemout should return 0 or 1 only
>
>> patch was only compile tested x86_64_defconfig + CONFIG_ATH_CARDS=m
>> CONFIG_ATH10K=m
>
>> patch is against linux-next 3.19.0-rc1 -next-20141226
>
> Rather patches. It would have been better to send one patch instead of
> 4 patches with the same name.
>
sorry for that - I had split it into separate patches as it was
in different files - giving them the same name of course was a bit
brain-dead.
please do give it one more look - if the above argument is invalid
I apologize for the noise.
thx!
hofrat
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/4] ath10k: a few incorrect return handling fix-up
@ 2014-12-30 18:28 ` Nicholas Mc Guire
0 siblings, 0 replies; 19+ messages in thread
From: Nicholas Mc Guire @ 2014-12-30 18:28 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Chun-Yeow Yeoh, netdev, linux-wireless, linux-kernel, ath10k,
Kalle Valo, Michal Kazior, Yanbo Li, Ben Greear
On Tue, 30 Dec 2014, Sergei Shtylyov wrote:
> Hello.
>
> On 12/30/2014 03:20 PM, Nicholas Mc Guire wrote:
>
>> wait_for_completion_timeout does not return negative values so the tests
>> for <= 0 are not needed and the case differentiation in the error handling
>> path unnecessary.
>
> I decided to verify your statement and I saw that it seems wrong.
> do_wait_for_common() can return -ERESTARTSYS and the return value gets
> returned by its callers unchanged.
the -ERESTARTSYS only can be returned if state matches but
wait_for_completion_timemout passes TASK_UNINTERRUPTIBLE
so signal_pending_state will return 0 and never negativ
my understanding of the callchain is:
wait_for_completion_timemout with TASK_UNINTERRUPTIBLE
-> wait_for_common(...TASK_UNINTERRUPTIBLE)
-> __wait_for_common(...TASK_UNINTERRUPTIBLE)
-> do_wait_for_common(...TASK_UNINTERRUPTIBLE)
-> signal_pending_state(TASK_UNINTERRUPTIBLE...)
static inline int signal_pending_state(long state, struct task_struct *p)
{
if (!(state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL)))
return 0;
so wait_for_completion_timemout should return 0 or 1 only
>
>> patch was only compile tested x86_64_defconfig + CONFIG_ATH_CARDS=m
>> CONFIG_ATH10K=m
>
>> patch is against linux-next 3.19.0-rc1 -next-20141226
>
> Rather patches. It would have been better to send one patch instead of
> 4 patches with the same name.
>
sorry for that - I had split it into separate patches as it was
in different files - giving them the same name of course was a bit
brain-dead.
please do give it one more look - if the above argument is invalid
I apologize for the noise.
thx!
hofrat
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/4] ath10k: a few incorrect return handling fix-up
2014-12-30 18:28 ` Nicholas Mc Guire
@ 2014-12-30 18:39 ` Sergei Shtylyov
-1 siblings, 0 replies; 19+ messages in thread
From: Sergei Shtylyov @ 2014-12-30 18:39 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: Kalle Valo, Michal Kazior, Ben Greear, Chun-Yeow Yeoh, Yanbo Li,
ath10k, linux-wireless, netdev, linux-kernel
On 12/30/2014 09:28 PM, Nicholas Mc Guire wrote:
>>> wait_for_completion_timeout does not return negative values so the tests
>>> for <= 0 are not needed and the case differentiation in the error handling
>>> path unnecessary.
>> I decided to verify your statement and I saw that it seems wrong.
>> do_wait_for_common() can return -ERESTARTSYS and the return value gets
>> returned by its callers unchanged.
> the -ERESTARTSYS only can be returned if state matches but
> wait_for_completion_timemout passes TASK_UNINTERRUPTIBLE
> so signal_pending_state will return 0 and never negativ
> my understanding of the callchain is:
> wait_for_completion_timemout with TASK_UNINTERRUPTIBLE
> -> wait_for_common(...TASK_UNINTERRUPTIBLE)
> -> __wait_for_common(...TASK_UNINTERRUPTIBLE)
> -> do_wait_for_common(...TASK_UNINTERRUPTIBLE)
> -> signal_pending_state(TASK_UNINTERRUPTIBLE...)
> static inline int signal_pending_state(long state, struct task_struct *p)
> {
> if (!(state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL)))
> return 0;
Right. I didn't look into TASK_UNINTERRUPTIBLE thing before sending my mail.
> so wait_for_completion_timemout should return 0 or 1 only
0 or the remaining time, to be precise.
>>> patch was only compile tested x86_64_defconfig + CONFIG_ATH_CARDS=m
>>> CONFIG_ATH10K=m
>>> patch is against linux-next 3.19.0-rc1 -next-20141226
>> Rather patches. It would have been better to send one patch instead of
>> 4 patches with the same name.
> sorry for that - I had split it into separate patches as it was
> in different files - giving them the same name of course was a bit
> brain-dead.
You should have mentioned the modified files in the subject. But IMHO it
would be better to have just one patch.
> please do give it one more look - if the above argument is invalid
> I apologize for the noise.
It's me who should apologize. :-<
> thx!
> hofrat
WBR, Sergei
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/4] ath10k: a few incorrect return handling fix-up
@ 2014-12-30 18:39 ` Sergei Shtylyov
0 siblings, 0 replies; 19+ messages in thread
From: Sergei Shtylyov @ 2014-12-30 18:39 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: Chun-Yeow Yeoh, netdev, linux-wireless, linux-kernel, ath10k,
Kalle Valo, Michal Kazior, Yanbo Li, Ben Greear
On 12/30/2014 09:28 PM, Nicholas Mc Guire wrote:
>>> wait_for_completion_timeout does not return negative values so the tests
>>> for <= 0 are not needed and the case differentiation in the error handling
>>> path unnecessary.
>> I decided to verify your statement and I saw that it seems wrong.
>> do_wait_for_common() can return -ERESTARTSYS and the return value gets
>> returned by its callers unchanged.
> the -ERESTARTSYS only can be returned if state matches but
> wait_for_completion_timemout passes TASK_UNINTERRUPTIBLE
> so signal_pending_state will return 0 and never negativ
> my understanding of the callchain is:
> wait_for_completion_timemout with TASK_UNINTERRUPTIBLE
> -> wait_for_common(...TASK_UNINTERRUPTIBLE)
> -> __wait_for_common(...TASK_UNINTERRUPTIBLE)
> -> do_wait_for_common(...TASK_UNINTERRUPTIBLE)
> -> signal_pending_state(TASK_UNINTERRUPTIBLE...)
> static inline int signal_pending_state(long state, struct task_struct *p)
> {
> if (!(state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL)))
> return 0;
Right. I didn't look into TASK_UNINTERRUPTIBLE thing before sending my mail.
> so wait_for_completion_timemout should return 0 or 1 only
0 or the remaining time, to be precise.
>>> patch was only compile tested x86_64_defconfig + CONFIG_ATH_CARDS=m
>>> CONFIG_ATH10K=m
>>> patch is against linux-next 3.19.0-rc1 -next-20141226
>> Rather patches. It would have been better to send one patch instead of
>> 4 patches with the same name.
> sorry for that - I had split it into separate patches as it was
> in different files - giving them the same name of course was a bit
> brain-dead.
You should have mentioned the modified files in the subject. But IMHO it
would be better to have just one patch.
> please do give it one more look - if the above argument is invalid
> I apologize for the noise.
It's me who should apologize. :-<
> thx!
> hofrat
WBR, Sergei
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/4] ath10k: a few incorrect return handling fix-up
2014-12-30 18:39 ` Sergei Shtylyov
@ 2014-12-30 19:42 ` Nicholas Mc Guire
-1 siblings, 0 replies; 19+ messages in thread
From: Nicholas Mc Guire @ 2014-12-30 19:42 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Kalle Valo, Michal Kazior, Ben Greear, Chun-Yeow Yeoh, Yanbo Li,
ath10k, linux-wireless, netdev, linux-kernel
On Tue, 30 Dec 2014, Sergei Shtylyov wrote:
> On 12/30/2014 09:28 PM, Nicholas Mc Guire wrote:
>
>>>> wait_for_completion_timeout does not return negative values so the tests
>>>> for <= 0 are not needed and the case differentiation in the error handling
>>>> path unnecessary.
>
>>> I decided to verify your statement and I saw that it seems wrong.
>>> do_wait_for_common() can return -ERESTARTSYS and the return value gets
>>> returned by its callers unchanged.
>
>> the -ERESTARTSYS only can be returned if state matches but
>> wait_for_completion_timemout passes TASK_UNINTERRUPTIBLE
>> so signal_pending_state will return 0 and never negativ
>
>> my understanding of the callchain is:
>> wait_for_completion_timemout with TASK_UNINTERRUPTIBLE
>> -> wait_for_common(...TASK_UNINTERRUPTIBLE)
>> -> __wait_for_common(...TASK_UNINTERRUPTIBLE)
>> -> do_wait_for_common(...TASK_UNINTERRUPTIBLE)
>> -> signal_pending_state(TASK_UNINTERRUPTIBLE...)
>
>> static inline int signal_pending_state(long state, struct task_struct *p)
>> {
>> if (!(state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL)))
>> return 0;
>
> Right. I didn't look into TASK_UNINTERRUPTIBLE thing before sending my mail.
>
>> so wait_for_completion_timemout should return 0 or 1 only
>
> 0 or the remaining time, to be precise.
>
yup - thanks for the confirmation!
>>>> patch was only compile tested x86_64_defconfig + CONFIG_ATH_CARDS=m
>>>> CONFIG_ATH10K=m
>
>>>> patch is against linux-next 3.19.0-rc1 -next-20141226
>
>>> Rather patches. It would have been better to send one patch instead of
>>> 4 patches with the same name.
>
>> sorry for that - I had split it into separate patches as it was
>> in different files - giving them the same name of course was a bit
>> brain-dead.
>
> You should have mentioned the modified files in the subject. But IMHO
> it would be better to have just one patch.
>
resent as a single patch as v2
thx!
hofrat
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/4] ath10k: a few incorrect return handling fix-up
@ 2014-12-30 19:42 ` Nicholas Mc Guire
0 siblings, 0 replies; 19+ messages in thread
From: Nicholas Mc Guire @ 2014-12-30 19:42 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Chun-Yeow Yeoh, netdev, linux-wireless, linux-kernel, ath10k,
Kalle Valo, Michal Kazior, Yanbo Li, Ben Greear
On Tue, 30 Dec 2014, Sergei Shtylyov wrote:
> On 12/30/2014 09:28 PM, Nicholas Mc Guire wrote:
>
>>>> wait_for_completion_timeout does not return negative values so the tests
>>>> for <= 0 are not needed and the case differentiation in the error handling
>>>> path unnecessary.
>
>>> I decided to verify your statement and I saw that it seems wrong.
>>> do_wait_for_common() can return -ERESTARTSYS and the return value gets
>>> returned by its callers unchanged.
>
>> the -ERESTARTSYS only can be returned if state matches but
>> wait_for_completion_timemout passes TASK_UNINTERRUPTIBLE
>> so signal_pending_state will return 0 and never negativ
>
>> my understanding of the callchain is:
>> wait_for_completion_timemout with TASK_UNINTERRUPTIBLE
>> -> wait_for_common(...TASK_UNINTERRUPTIBLE)
>> -> __wait_for_common(...TASK_UNINTERRUPTIBLE)
>> -> do_wait_for_common(...TASK_UNINTERRUPTIBLE)
>> -> signal_pending_state(TASK_UNINTERRUPTIBLE...)
>
>> static inline int signal_pending_state(long state, struct task_struct *p)
>> {
>> if (!(state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL)))
>> return 0;
>
> Right. I didn't look into TASK_UNINTERRUPTIBLE thing before sending my mail.
>
>> so wait_for_completion_timemout should return 0 or 1 only
>
> 0 or the remaining time, to be precise.
>
yup - thanks for the confirmation!
>>>> patch was only compile tested x86_64_defconfig + CONFIG_ATH_CARDS=m
>>>> CONFIG_ATH10K=m
>
>>>> patch is against linux-next 3.19.0-rc1 -next-20141226
>
>>> Rather patches. It would have been better to send one patch instead of
>>> 4 patches with the same name.
>
>> sorry for that - I had split it into separate patches as it was
>> in different files - giving them the same name of course was a bit
>> brain-dead.
>
> You should have mentioned the modified files in the subject. But IMHO
> it would be better to have just one patch.
>
resent as a single patch as v2
thx!
hofrat
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2014-12-30 19:43 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-30 12:20 [PATCH 0/4] ath10k: a few incorrect return handling fix-up Nicholas Mc Guire
2014-12-30 12:20 ` Nicholas Mc Guire
2014-12-30 12:20 ` [PATCH 1/4] ath10k: fixup wait_for_completion_timeout return handling Nicholas Mc Guire
2014-12-30 12:20 ` Nicholas Mc Guire
2014-12-30 12:20 ` Nicholas Mc Guire
2014-12-30 12:20 ` [PATCH 2/4] " Nicholas Mc Guire
2014-12-30 12:20 ` Nicholas Mc Guire
2014-12-30 12:20 ` [PATCH 3/4] " Nicholas Mc Guire
2014-12-30 12:20 ` Nicholas Mc Guire
2014-12-30 12:20 ` [PATCH 4/4] " Nicholas Mc Guire
2014-12-30 12:20 ` Nicholas Mc Guire
2014-12-30 17:18 ` [PATCH 0/4] ath10k: a few incorrect return handling fix-up Sergei Shtylyov
2014-12-30 17:18 ` Sergei Shtylyov
2014-12-30 18:28 ` Nicholas Mc Guire
2014-12-30 18:28 ` Nicholas Mc Guire
2014-12-30 18:39 ` Sergei Shtylyov
2014-12-30 18:39 ` Sergei Shtylyov
2014-12-30 19:42 ` Nicholas Mc Guire
2014-12-30 19:42 ` Nicholas Mc Guire
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.