diff mbox

[2/3,RFC] ath10k: wmi: match wait_for_completion_timeout return type

Message ID 1426175362-8640-1-git-send-email-hofrat@osadl.org (mailing list archive)
State RFC
Headers show

Commit Message

Nicholas Mc Guire March 12, 2015, 3:49 p.m. UTC
Return type of wait_for_completion_timeout is unsigned long not int.
An appropriately named unsigned long is added and the assignments fixed up.
Rather than returning 0 (timeout) or a more or less random remaining time
(completion success) this return 0 or 1 which also resolves the type of the
functions being int.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Checking the call-sites of ath10k_wmi_wait_for_unified_ready and 
ath10k_wmi_wait_for_service_ready the positive return value (remaining
time in jiffies) is never passed up the call-chain nor used so it is 
cleaner to treat this like a boolean success/fail only (actually the two
functions should probably be of type bool - but that does not seem to be
common practice in the ath10k code base)

Patch was only compile tested with x86_64_defconfig + CONFIG_ATH_CARDS=m,
CONFIG_ATH10K=m

Patch is against 4.0-rc3 (localversion-next is -next-20150312)
:
 drivers/net/wireless/ath/ath10k/wmi.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

Comments

Michal Kazior March 13, 2015, 6:47 a.m. UTC | #1
On 12 March 2015 at 16:49, Nicholas Mc Guire <hofrat@osadl.org> wrote:
> Return type of wait_for_completion_timeout is unsigned long not int.
> An appropriately named unsigned long is added and the assignments fixed up.
> Rather than returning 0 (timeout) or a more or less random remaining time
> (completion success) this return 0 or 1 which also resolves the type of the
> functions being int.
>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
>
> Checking the call-sites of ath10k_wmi_wait_for_unified_ready and
> ath10k_wmi_wait_for_service_ready the positive return value (remaining
> time in jiffies) is never passed up the call-chain nor used so it is
> cleaner to treat this like a boolean success/fail only (actually the two
> functions should probably be of type bool - but that does not seem to be
> common practice in the ath10k code base)

It'd make sense to have these functions return 0 or -ETIMEDOUT. In
that case both call sites would need to be adjusted to treat "< 0" or
"!x" as an error (instead of the current "<= 0") condition and not set
-ETIMEDOUT themselves.


Micha?
Kalle Valo March 13, 2015, 4:20 p.m. UTC | #2
Nicholas Mc Guire <hofrat@osadl.org> writes:

> Return type of wait_for_completion_timeout is unsigned long not int.
> An appropriately named unsigned long is added and the assignments fixed up.
> Rather than returning 0 (timeout) or a more or less random remaining time
> (completion success) this return 0 or 1 which also resolves the type of the
> functions being int.
>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>

Why does patch 2 in this patchset have RFC in the title but patches 1
and 3 not? That just makes me confused, I can't tell what you want me to
do with the patches. Normally I just drop all patches (or patchsets)
which have RFC, and that's what I'm going to do now.

To save everyone's time, when submitting something please state clearly
what's your intention.
Nicholas Mc Guire March 13, 2015, 4:24 p.m. UTC | #3
On Fri, 13 Mar 2015, Kalle Valo wrote:

> Nicholas Mc Guire <hofrat@osadl.org> writes:
> 
> > Return type of wait_for_completion_timeout is unsigned long not int.
> > An appropriately named unsigned long is added and the assignments fixed up.
> > Rather than returning 0 (timeout) or a more or less random remaining time
> > (completion success) this return 0 or 1 which also resolves the type of the
> > functions being int.
> >
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> 
> Why does patch 2 in this patchset have RFC in the title but patches 1
> and 3 not? That just makes me confused, I can't tell what you want me to
> do with the patches. Normally I just drop all patches (or patchsets)
> which have RFC, and that's what I'm going to do now.
> 
> To save everyone's time, when submitting something please state clearly
> what's your intention.
>
ok - was simply unsure about the proposed change 
and 1 was a trivial cleanup (which should have been 
sent out as a seperate patch and not part of a series - my mistake)

Will fix this up and repost it.

sorry for the screwup - no intent to wast anybodies time.

thx!
hofrat
Nicholas Mc Guire March 14, 2015, 8:59 a.m. UTC | #4
On Fri, 13 Mar 2015, Michal Kazior wrote:

> On 12 March 2015 at 16:49, Nicholas Mc Guire <hofrat@osadl.org> wrote:
> > Return type of wait_for_completion_timeout is unsigned long not int.
> > An appropriately named unsigned long is added and the assignments fixed up.
> > Rather than returning 0 (timeout) or a more or less random remaining time
> > (completion success) this return 0 or 1 which also resolves the type of the
> > functions being int.
> >
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > ---
> >
> > Checking the call-sites of ath10k_wmi_wait_for_unified_ready and
> > ath10k_wmi_wait_for_service_ready the positive return value (remaining
> > time in jiffies) is never passed up the call-chain nor used so it is
> > cleaner to treat this like a boolean success/fail only (actually the two
> > functions should probably be of type bool - but that does not seem to be
> > common practice in the ath10k code base)
> 
> It'd make sense to have these functions return 0 or -ETIMEDOUT. In
> that case both call sites would need to be adjusted to treat "< 0" or
> "!x" as an error (instead of the current "<= 0") condition and not set
> -ETIMEDOUT themselves.
>
looking at the call sites in ath10k_core_start more or less 
all other initialization calls will treate 0 as success and
!=0 as failure so this is the cleaner solution. as its all
now

status = call()
if(status)
 	error

patch just posted.

thx!
hofrat
diff mbox

Patch

diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index c7ea77e..a1cdcba 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -884,20 +884,24 @@  void ath10k_wmi_put_wmi_channel(struct wmi_channel *ch,
 
 int ath10k_wmi_wait_for_service_ready(struct ath10k *ar)
 {
-	int ret;
+	unsigned long time_left;
 
-	ret = wait_for_completion_timeout(&ar->wmi.service_ready,
-					  WMI_SERVICE_READY_TIMEOUT_HZ);
-	return ret;
+	time_left = wait_for_completion_timeout(&ar->wmi.service_ready,
+						WMI_SERVICE_READY_TIMEOUT_HZ);
+	if(!time_left)
+		return 0;
+	return 1;
 }
 
 int ath10k_wmi_wait_for_unified_ready(struct ath10k *ar)
 {
-	int ret;
+	unsigned long time_left;
 
-	ret = wait_for_completion_timeout(&ar->wmi.unified_ready,
-					  WMI_UNIFIED_READY_TIMEOUT_HZ);
-	return ret;
+	time_left = wait_for_completion_timeout(&ar->wmi.unified_ready,
+						WMI_UNIFIED_READY_TIMEOUT_HZ);
+	if(!time_left)
+		return 0;
+	return 1;
 }
 
 struct sk_buff *ath10k_wmi_alloc_skb(struct ath10k *ar, u32 len)