[PATCH v3] staging: wlan-ng: prism2mgmt.c Fix break not useful

[Tobias Klauser]

On 2014-08-13 at 21:11:19 +0200, Jeshwanth Kumar N K <jeshkumar555 at gmail.com> wrote:
> On Wed, Aug 13, 2014 at 12:06 PM, Tobias Klauser <tklauser at distanz.ch>
> wrote:
> 
> > On 2014-08-12 at 20:54:48 +0200, Jeshwanth Kumar N K <
> > jeshkumar555 at gmail.com> wrote:
> > > removed goto label, and directly returning 0 not through goto.
> > >
> > > Signed-off-by: Jeshwanth Kumar N K <jeshkumar555 at gmail.com>
> > > ---
> > >  drivers/staging/wlan-ng/prism2mgmt.c | 14 ++++----------
> > >  1 file changed, 4 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/staging/wlan-ng/prism2mgmt.c
> > b/drivers/staging/wlan-ng/prism2mgmt.c
> > > index 5837b0e..9218399 100644
> > > --- a/drivers/staging/wlan-ng/prism2mgmt.c
> > > +++ b/drivers/staging/wlan-ng/prism2mgmt.c
> > > @@ -1107,8 +1107,7 @@ int prism2mgmt_wlansniff(wlandevice_t *wlandev,
> > void *msgp)
> > >               if (wlandev->netdev->type == ARPHRD_ETHER) {
> > >                       msg->resultcode.data =
> > >                           P80211ENUM_resultcode_invalid_parameters;
> > > -                     result = 0;
> > > -                     goto exit;
> > > +                     return 0;
> > >               }
> > >               /* Disable monitor mode */
> > >               result = hfa384x_cmd_monitor(hw, HFA384x_MONITOR_DISABLE);
> > > @@ -1166,8 +1165,7 @@ int prism2mgmt_wlansniff(wlandevice_t *wlandev,
> > void *msgp)
> > >
> > >               netdev_info(wlandev->netdev, "monitor mode disabled\n");
> > >               msg->resultcode.data = P80211ENUM_resultcode_success;
> > > -             result = 0;
> > > -             goto exit;
> > > +             return 0;
> > >       case P80211ENUM_truth_true:
> > >               /* Disable the port (if enabled), only check Port 0 */
> > >               if (hw->port_enabled[0]) {
> > > @@ -1312,17 +1310,13 @@ int prism2mgmt_wlansniff(wlandevice_t *wlandev,
> > void *msgp)
> > >               }
> > >
> > >               msg->resultcode.data = P80211ENUM_resultcode_success;
> > > -             result = 0;
> > > -             goto exit;
> > > +             return 0;
> > >       default:
> > >               msg->resultcode.data =
> > P80211ENUM_resultcode_invalid_parameters;
> > > -             result = 0;
> > > -             goto exit;
> > > +             return 0;
> > >       }
> > >
> > >  failed:
> > >       msg->resultcode.data = P80211ENUM_resultcode_refused;
> > >       result = 0;
> > > -exit:
> > > -     return result;
> >
> > You will still need the return here for the cases where the 'failed'
> > label is the jump target. Also, this change leads to a compiler warning:
> >
> > drivers/staging/wlan-ng/prism2usb.c: In function ‘prism2mgmt_wlansniff’:
> > drivers/staging/wlan-ng/prism2mgmt.c:1322:1: warning: control reaches end
> > of non-void function [-Wreturn-type]
> >
> > In any case, resetting the return value to 0 makes the function always
> > return
> > successfully  and seems to signal the error condition by setting
> > msg->resultcode.data. This looks a bit odd to me and also doesn't
> > correspond to
> > the documentation of the function's return value. It might be worthwile to
> > take
> > a closer look there as well.
> >
> > Thanks
> > Tobias
> >

Hi Jeshwanth

> Thanks, I have changed it in local now (not yet submitted). But yes as you
> said it's not returning 0 every time.
> 
> In comment it's mentioned that "<0      success, but we're waiting for
> something to finish.". This means this function implementation is not yet
> finished?

I assume so, but I'm not really familiar with the workings of this
driver.

> If there is no 2nd value of return, this can be made as void function?  or
> need to understand the logic behind?

I would leave it as is, unless you clearly understand the logic behind
it. I'd suggest for your current patch to just change the
return/goto/break part and then take care about the return value in a
separate patch once you figured out how the function should really
behave. You'll most probably also need the hardware in question then to
test your changes on.

> And I didn't find it is checking P80211ENUM_resultcode_refused anywhere in
> the code.

Hm yes, strage. So it seems the error conditions in this functions are
not handled at all except for the pr_debug messages.

Thanks
Tobias