* [PATCH v3]hostap:hostap_ioctl.c Fix variable 'ret' set but not used @ 2010-08-03 1:46 Justin P. Mattock 2010-08-03 1:58 ` Julian Calaby 0 siblings, 1 reply; 7+ messages in thread From: Justin P. Mattock @ 2010-08-03 1:46 UTC (permalink / raw) To: linux-wireless; +Cc: j, linville, mcgrof, julian.calaby, Justin P. Mattock Resend version 3 The below patch fixes a warning message generated by GCC: CC [M] drivers/net/wireless/hostap/hostap_ioctl.o drivers/net/wireless/hostap/hostap_ioctl.c: In function 'prism2_request_scan': drivers/net/wireless/hostap/hostap_ioctl.c:1666:6: warning: variable 'ret' set but not used Signed-off-by: Justin P. Mattock <justinmattock@gmail.com> --- drivers/net/wireless/hostap/hostap_ioctl.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/net/wireless/hostap/hostap_ioctl.c b/drivers/net/wireless/hostap/hostap_ioctl.c index a85e43a..2cf6532 100644 --- a/drivers/net/wireless/hostap/hostap_ioctl.c +++ b/drivers/net/wireless/hostap/hostap_ioctl.c @@ -1689,7 +1689,7 @@ static int prism2_request_scan(struct net_device *dev) if (local->func->set_rid(dev, HFA384X_RID_SCANREQUEST, &scan_req, sizeof(scan_req))) { printk(KERN_DEBUG "SCANREQUEST failed\n"); - ret = -EINVAL; + return ret; } if (!local->host_roaming) -- 1.7.1.rc1.21.gf3bd6 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3]hostap:hostap_ioctl.c Fix variable 'ret' set but not used 2010-08-03 1:46 [PATCH v3]hostap:hostap_ioctl.c Fix variable 'ret' set but not used Justin P. Mattock @ 2010-08-03 1:58 ` Julian Calaby 2010-08-03 2:23 ` Justin P. Mattock 0 siblings, 1 reply; 7+ messages in thread From: Julian Calaby @ 2010-08-03 1:58 UTC (permalink / raw) To: Justin P. Mattock; +Cc: linux-wireless, j, linville, mcgrof [-- Attachment #1: Type: text/plain, Size: 1581 bytes --] On Tue, Aug 3, 2010 at 11:46, Justin P. Mattock <justinmattock@gmail.com> wrote: > Resend version 3 > > The below patch fixes a warning message generated by GCC: > CC [M] drivers/net/wireless/hostap/hostap_ioctl.o > drivers/net/wireless/hostap/hostap_ioctl.c: In function 'prism2_request_scan': > drivers/net/wireless/hostap/hostap_ioctl.c:1666:6: warning: variable 'ret' set but not used > > Signed-off-by: Justin P. Mattock <justinmattock@gmail.com> > > --- > drivers/net/wireless/hostap/hostap_ioctl.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/net/wireless/hostap/hostap_ioctl.c b/drivers/net/wireless/hostap/hostap_ioctl.c > index a85e43a..2cf6532 100644 > --- a/drivers/net/wireless/hostap/hostap_ioctl.c > +++ b/drivers/net/wireless/hostap/hostap_ioctl.c > @@ -1689,7 +1689,7 @@ static int prism2_request_scan(struct net_device *dev) > if (local->func->set_rid(dev, HFA384X_RID_SCANREQUEST, &scan_req, > sizeof(scan_req))) { > printk(KERN_DEBUG "SCANREQUEST failed\n"); > - ret = -EINVAL; > + return ret; > } > > if (!local->host_roaming) NACK: This doesn't actually fix the error mentioned. Attached is a more correct patch. Apologies for the attachment, I'm building the patch by hand at work and only have gMail to send it with. This should apply to wireless-testing. Thanks, -- Julian Calaby Email: julian.calaby@gmail.com .Plan: http://sites.google.com/site/juliancalaby/ [-- Attachment #2: fix-ret-not-used-hostap.diff --] [-- Type: text/x-patch, Size: 633 bytes --] Hostap: Fix "'ret' set but not used" warning message from GCC in hostap CC [M] drivers/net/wireless/hostap/hostap_ioctl.o drivers/net/wireless/hostap/hostap_ioctl.c: In function 'prism2_request_scan': drivers/net/wireless/hostap/hostap_ioctl.c:1666:6: warning: variable 'ret' set but not used Signed-off-by: Julian Calaby <julian.calaby@gmail.com> --- --- a/drivers/net/wireless/hostap/hostap_ioctl.c +++ b/drivers/net/wireless/hostap/hostap_ioctl.c @@ -1696,7 +1696,7 @@ hostap_set_word(dev, HFA384X_RID_CNFROAMINGMODE, HFA384X_ROAMING_FIRMWARE); - return 0; + return ret; } #else /* !PRISM2_NO_STATION_MODES */ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3]hostap:hostap_ioctl.c Fix variable 'ret' set but not used 2010-08-03 1:58 ` Julian Calaby @ 2010-08-03 2:23 ` Justin P. Mattock 2010-08-03 2:31 ` Julian Calaby 0 siblings, 1 reply; 7+ messages in thread From: Justin P. Mattock @ 2010-08-03 2:23 UTC (permalink / raw) To: Julian Calaby; +Cc: linux-wireless, j, linville, mcgrof On 08/02/2010 06:58 PM, Julian Calaby wrote: > On Tue, Aug 3, 2010 at 11:46, Justin P. Mattock<justinmattock@gmail.com> wrote: >> Resend version 3 >> >> The below patch fixes a warning message generated by GCC: >> CC [M] drivers/net/wireless/hostap/hostap_ioctl.o >> drivers/net/wireless/hostap/hostap_ioctl.c: In function 'prism2_request_scan': >> drivers/net/wireless/hostap/hostap_ioctl.c:1666:6: warning: variable 'ret' set but not used >> >> Signed-off-by: Justin P. Mattock<justinmattock@gmail.com> >> >> --- >> drivers/net/wireless/hostap/hostap_ioctl.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/net/wireless/hostap/hostap_ioctl.c b/drivers/net/wireless/hostap/hostap_ioctl.c >> index a85e43a..2cf6532 100644 >> --- a/drivers/net/wireless/hostap/hostap_ioctl.c >> +++ b/drivers/net/wireless/hostap/hostap_ioctl.c >> @@ -1689,7 +1689,7 @@ static int prism2_request_scan(struct net_device *dev) >> if (local->func->set_rid(dev, HFA384X_RID_SCANREQUEST,&scan_req, >> sizeof(scan_req))) { >> printk(KERN_DEBUG "SCANREQUEST failed\n"); >> - ret = -EINVAL; >> + return ret; >> } >> >> if (!local->host_roaming) > > NACK: This doesn't actually fix the error mentioned. > > Attached is a more correct patch. > > Apologies for the attachment, I'm building the patch by hand at work > and only have gMail to send it with. > > This should apply to wireless-testing. > > Thanks, > wait im lost.. looking at your patch looks the same(number wise) except I see different parts of code in there: hostap_set_word(dev, HFA384X_RID_CNFROAMINGMODE, HFA384X_ROAMING_FIRMWARE); - return 0; + return ret; mine is: printk(KERN_DEBUG "SCANREQUEST failed\n"); - ret = -EINVAL; + return ret; The tree Im using is Linus's tree. any ideas? Justin P. Mattock ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3]hostap:hostap_ioctl.c Fix variable 'ret' set but not used 2010-08-03 2:23 ` Justin P. Mattock @ 2010-08-03 2:31 ` Julian Calaby 2010-08-03 2:49 ` Justin P. Mattock 0 siblings, 1 reply; 7+ messages in thread From: Julian Calaby @ 2010-08-03 2:31 UTC (permalink / raw) To: Justin P. Mattock; +Cc: linux-wireless, j, linville, mcgrof On Tue, Aug 3, 2010 at 12:23, Justin P. Mattock <justinmattock@gmail.com> wrote: > wait im lost.. looking at your patch looks the same(number wise) except I > see different parts of code in there: > > hostap_set_word(dev, HFA384X_RID_CNFROAMINGMODE, > HFA384X_ROAMING_FIRMWARE); > > - return 0; > + return ret; > > > > mine is: > > printk(KERN_DEBUG "SCANREQUEST failed\n"); > - ret = -EINVAL; > + return ret; > > > The tree Im using is Linus's tree. I'm using wireless-testing, and the line you're changing is #1692, I'm changing #1699. Thanks, -- Julian Calaby Email: julian.calaby@gmail.com .Plan: http://sites.google.com/site/juliancalaby/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3]hostap:hostap_ioctl.c Fix variable 'ret' set but not used 2010-08-03 2:31 ` Julian Calaby @ 2010-08-03 2:49 ` Justin P. Mattock 2010-08-03 3:31 ` Julian Calaby 0 siblings, 1 reply; 7+ messages in thread From: Justin P. Mattock @ 2010-08-03 2:49 UTC (permalink / raw) To: Julian Calaby; +Cc: linux-wireless, j, linville, mcgrof On 08/02/2010 07:31 PM, Julian Calaby wrote: > On Tue, Aug 3, 2010 at 12:23, Justin P. Mattock<justinmattock@gmail.com> wrote: >> wait im lost.. looking at your patch looks the same(number wise) except I >> see different parts of code in there: >> >> hostap_set_word(dev, HFA384X_RID_CNFROAMINGMODE, >> HFA384X_ROAMING_FIRMWARE); >> >> - return 0; >> + return ret; >> >> >> >> mine is: >> >> printk(KERN_DEBUG "SCANREQUEST failed\n"); >> - ret = -EINVAL; >> + return ret; >> >> >> The tree Im using is Linus's tree. > > I'm using wireless-testing, and the line you're changing is #1692, I'm > changing #1699. > > Thanks, > ahh.. so there's two return ret's that need to be added then.. (if so then go ahead and take my patch and add it to yours.. I'm off to other things) cheers, Justin P. Mattock ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3]hostap:hostap_ioctl.c Fix variable 'ret' set but not used 2010-08-03 2:49 ` Justin P. Mattock @ 2010-08-03 3:31 ` Julian Calaby 2010-08-03 3:44 ` Justin P. Mattock 0 siblings, 1 reply; 7+ messages in thread From: Julian Calaby @ 2010-08-03 3:31 UTC (permalink / raw) To: Justin P. Mattock; +Cc: linux-wireless, j, linville, mcgrof On Tue, Aug 3, 2010 at 12:49, Justin P. Mattock <justinmattock@gmail.com> wrote: > On 08/02/2010 07:31 PM, Julian Calaby wrote: >> >> On Tue, Aug 3, 2010 at 12:23, Justin P. Mattock<justinmattock@gmail.com> >> wrote: >>> >>> wait im lost.. looking at your patch looks the same(number wise) except I >>> see different parts of code in there: >>> >>> hostap_set_word(dev, HFA384X_RID_CNFROAMINGMODE, >>> HFA384X_ROAMING_FIRMWARE); >>> >>> - return 0; >>> + return ret; >>> >>> >>> >>> mine is: >>> >>> printk(KERN_DEBUG "SCANREQUEST failed\n"); >>> - ret = -EINVAL; >>> + return ret; >>> >>> >>> The tree Im using is Linus's tree. >> >> I'm using wireless-testing, and the line you're changing is #1692, I'm >> changing #1699. >> >> Thanks, >> > > > ahh.. so there's two return ret's that need to be added then.. > (if so then go ahead and take my patch and add it to yours.. I'm off to > other things) No. The function works like this: 1. set up scan_req 2. if local->host_roaming is not set, set host roaming on the hardware 3. set scan_req on the hardware 4. if local->host_roaming is not set, set the hardware to do firmware roaming 5. If set_req could not be set on the hardware, return -EINVAL, otherwise return 0. The significant bits are steps 2 and 4. These must *both* be executed, regardless of whether setting scan_req on the hardware succeeds or not. As such, the return value is stored in the ret variable, which is initialised to 0 initially, then set to -EINVAL if setting scan_req on the hardware fails. Hence, step 5 can be performed by returning ret. As such, all of your patches are wrong as they are either returning the wrong value when setting scan_req on the hardware fails, or failing to execute step 4. The *only* correct place to return a value is once the roaming status of the hardware has been set properly - i.e. at the end of the function. I hope this has cleared up your confusion. Thanks, -- Julian Calaby Email: julian.calaby@gmail.com .Plan: http://sites.google.com/site/juliancalaby/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3]hostap:hostap_ioctl.c Fix variable 'ret' set but not used 2010-08-03 3:31 ` Julian Calaby @ 2010-08-03 3:44 ` Justin P. Mattock 0 siblings, 0 replies; 7+ messages in thread From: Justin P. Mattock @ 2010-08-03 3:44 UTC (permalink / raw) To: Julian Calaby; +Cc: linux-wireless, j, linville, mcgrof On 08/02/2010 08:31 PM, Julian Calaby wrote: > On Tue, Aug 3, 2010 at 12:49, Justin P. Mattock<justinmattock@gmail.com> wrote: >> On 08/02/2010 07:31 PM, Julian Calaby wrote: >>> >>> On Tue, Aug 3, 2010 at 12:23, Justin P. Mattock<justinmattock@gmail.com> >>> wrote: >>>> >>>> wait im lost.. looking at your patch looks the same(number wise) except I >>>> see different parts of code in there: >>>> >>>> hostap_set_word(dev, HFA384X_RID_CNFROAMINGMODE, >>>> HFA384X_ROAMING_FIRMWARE); >>>> >>>> - return 0; >>>> + return ret; >>>> >>>> >>>> >>>> mine is: >>>> >>>> printk(KERN_DEBUG "SCANREQUEST failed\n"); >>>> - ret = -EINVAL; >>>> + return ret; >>>> >>>> >>>> The tree Im using is Linus's tree. >>> >>> I'm using wireless-testing, and the line you're changing is #1692, I'm >>> changing #1699. >>> >>> Thanks, >>> >> >> >> ahh.. so there's two return ret's that need to be added then.. >> (if so then go ahead and take my patch and add it to yours.. I'm off to >> other things) > > No. > > The function works like this: > > 1. set up scan_req > 2. if local->host_roaming is not set, set host roaming on the hardware > 3. set scan_req on the hardware > 4. if local->host_roaming is not set, set the hardware to do firmware roaming > 5. If set_req could not be set on the hardware, return -EINVAL, > otherwise return 0. > > The significant bits are steps 2 and 4. These must *both* be executed, > regardless of whether setting scan_req on the hardware succeeds or > not. > > As such, the return value is stored in the ret variable, which is > initialised to 0 initially, then set to -EINVAL if setting scan_req on > the hardware fails. Hence, step 5 can be performed by returning ret. > > As such, all of your patches are wrong as they are either returning > the wrong value when setting scan_req on the hardware fails, or > failing to execute step 4. The *only* correct place to return a value > is once the roaming status of the hardware has been set properly - > i.e. at the end of the function. > > I hope this has cleared up your confusion. > > Thanks, > cleared it up perfectly... didnt think that using ret down more would work..(was stuck a few lines up) but it does.. just tested your version out and no warning messages.. cheers, Justin P. Mattock ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-08-03 3:43 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-08-03 1:46 [PATCH v3]hostap:hostap_ioctl.c Fix variable 'ret' set but not used Justin P. Mattock 2010-08-03 1:58 ` Julian Calaby 2010-08-03 2:23 ` Justin P. Mattock 2010-08-03 2:31 ` Julian Calaby 2010-08-03 2:49 ` Justin P. Mattock 2010-08-03 3:31 ` Julian Calaby 2010-08-03 3:44 ` Justin P. Mattock
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).