| Message ID | 1426175362-8640-1-git-send-email-hofrat@osadl.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
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? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)
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(-)