[PATCH 0/4] ath10k: a few incorrect return handling fix-up

[PATCH 0/4] ath10k: a few incorrect return handling fix-up

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

[PATCH 1/4] ath10k: fixup wait_for_completion_timeout return handling

[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

[PATCH 2/4] ath10k: fixup wait_for_completion_timeout return handling

[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

[PATCH 3/4] ath10k: fixup wait_for_completion_timeout return handling

[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

[PATCH 4/4] ath10k: fixup wait_for_completion_timeout return handling

[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

Re: [PATCH 0/4] ath10k: a few incorrect return handling fix-up

[Sergei Shtylyov]

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

Re: [PATCH 0/4] ath10k: a few incorrect return handling fix-up

[Nicholas Mc Guire]

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

Re: [PATCH 0/4] ath10k: a few incorrect return handling fix-up

[Sergei Shtylyov]

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

Re: [PATCH 0/4] ath10k: a few incorrect return handling fix-up

[Nicholas Mc Guire]

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