From: Larry Finger <Larry.Finger@lwfinger.net>
To: Ali Bahar <ali@internetdog.org>
Cc: Greg Kroah-Hartman <gregkh@suse.de>, linux-wireless@vger.kernel.org
Subject: Re: [PATCH 1/2] staging: r8712u: Most return-values changed from -1 to proper errno macros.
Date: Fri, 15 Jul 2011 16:00:10 -0500 [thread overview]
Message-ID: <4E20AA5A.6020101@lwfinger.net> (raw)
In-Reply-To: <1310749876-32632-2-git-send-email-ali@internetDog.org>
On 07/15/2011 12:11 PM, Ali Bahar wrote:
> The ioctl handlers were frequently returning -1 upon failure. Most of
> these have now been changed to proper errno macros.
> The few remaining ones have been left untouched because either the
> handler is not called (and so cannot be tested), or the function never
> fails (and so cannot be system-tested), or requires new code to
> distinguish its failures.
>
> Signed-off-by: Ali Bahar<ali@internetDog.org>
> Cc: Larry Finger<Larry.Finger@lwfinger.net>
> ---
> drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 121 +++++++++++++++++++++----
> 1 files changed, 105 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> index 40e6b5c..bb97d8b 100644
> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> @@ -526,7 +526,7 @@ static int r871x_set_wpa_ie(struct _adapter *padapter, char *pie,
> memcpy(buf, pie , ielen);
> pos = buf;
> if (ielen< RSN_HEADER_LEN) {
> - ret = -1;
> + ret = -EINVAL;
> goto exit;
> }
> if (r8712_parse_wpa_ie(buf, ielen,&group_cipher,
> @@ -689,6 +689,11 @@ static const long frequency_list[] = {
> 5825
> };
>
> +/*
> + * This function intends to handle the Set Freq command.
> + * Currently, the request comes via the Wireless Extensions' SIOCSIWFREQ ioctl.
> + *
> + */
This comment is not needed. The function name is descriptive and it is clear
that it is part of WEXT.
> static int r8711_wx_set_freq(struct net_device *dev,
> struct iw_request_info *info,
> union iwreq_data *wrqu, char *extra)
> @@ -723,6 +728,11 @@ static int r8711_wx_set_freq(struct net_device *dev,
> return rc;
> }
>
> +/*
> + * This function intends to handle the Get Freq command.
> + * Currently, the request comes via the Wireless Extensions' SIOCGIWFREQ ioctl.
> + *
> + */
Ditto.
> static int r8711_wx_get_freq(struct net_device *dev,
> struct iw_request_info *info,
> union iwreq_data *wrqu, char *extra)
> @@ -736,11 +746,21 @@ static int r8711_wx_get_freq(struct net_device *dev,
> pcur_bss->Configuration.DSConfig-1] * 100000;
> wrqu->freq.e = 1;
> wrqu->freq.i = pcur_bss->Configuration.DSConfig;
> - } else
> - return -1;
> + } else {
> + return -ENOLINK;
> + }
> return 0;
> }
>
> +/*
> + * This function intends to handle the Set Mode command.
> + * Currently, the request comes via the Wireless Extensions' SIOCSIWMODE ioctl.
> + *
The part above is not needed.
> + * Note: Currently (July 2011) the call to
> + * r8712_set_802_11_infrastructure_mode() always returns true. Since we have no
> + * specific failure reason, we return EPERM in lieu of general failure.
> + *
> + */
> static int r8711_wx_set_mode(struct net_device *dev,
> struct iw_request_info *a,
> union iwreq_data *wrqu, char *b)
> @@ -769,10 +789,15 @@ static int r8711_wx_set_mode(struct net_device *dev,
> else
> r8712_setopmode_cmd(padapter, Ndis802_11AutoUnknown);
> if (!r8712_set_802_11_infrastructure_mode(padapter, networkType))
> - return -1;
> + return -EPERM; /* Unknown failure. */
> return 0;
As r8712_set_802_11_infrastructure_mode() always returns true. it would be
better to rewrite it as a void function and completely do away with the test here.
> }
>
> +/*
> + * This function intends to handle the Get Mode command.
> + * Currently, the request comes via the Wireless Extensions' SIOCGIWMODE ioctl.
> + *
> + */
Another senseless comment.
> static int r8711_wx_get_mode(struct net_device *dev, struct iw_request_info *a,
> union iwreq_data *wrqu, char *b)
> {
> @@ -975,6 +1000,16 @@ static int r871x_wx_set_priv(struct net_device *dev,
> * s2. set_802_11_authentication_mode()
> * s3. set_802_11_encryption_mode()
> * s4. set_802_11_bssid()
> + *
> + * This function intends to handle the Set AP command, which specifies the
> + * MAC# of a preferred Access Point.
> + * Currently, the request comes via Wireless Extensions' SIOCSIWAP ioctl.
> + *
> + * For this operation to succeed, there is no need for the interface to be Up.
> + *
> + * Note: Currently (July 2011) r8712_set_802_11_bssid() may fail for one of
> + * several reasons. For now, we let -1 be returned as a catch-all failure
> + * indication, until that function's implementation is updated.
> */
Ditto.
> static int r8711_wx_set_wap(struct net_device *dev,
> struct iw_request_info *info,
> @@ -995,7 +1030,7 @@ static int r8711_wx_set_wap(struct net_device *dev,
> if (padapter->bup == false)
> return -1;
> if (check_fwstate(pmlmepriv, _FW_UNDER_SURVEY) == true)
> - return -1;
> + return -EBUSY;
> if (check_fwstate(pmlmepriv, _FW_UNDER_LINKING) == true)
> return ret;
> if (temp->sa_family != ARPHRD_ETHER)
> @@ -1014,14 +1049,14 @@ static int r8711_wx_set_wap(struct net_device *dev,
> if (!memcmp(dst_bssid, temp->sa_data, ETH_ALEN)) {
> if (r8712_set_802_11_infrastructure_mode(padapter,
> pnetwork->network.InfrastructureMode) == false)
> - ret = -1;
> + ret = -EPERM; /* Unknown failure. */
> break;
> }
> }
> spin_unlock_irqrestore(&queue->lock, irqL);
> if (!ret) {
> if (!r8712_set_802_11_authentication_mode(padapter, authmode))
> - ret = -1;
> + ret = -ENOMEM;
> else {
> if (!r8712_set_802_11_bssid(padapter, temp->sa_data))
> ret = -1;
> @@ -1074,6 +1109,19 @@ static int r871x_wx_set_mlme(struct net_device *dev,
> return ret;
> }
>
> +/**
> + *
> + * This function intends to handle the Set Scan command.
> + * Currently, the request comes via Wireless Extensions' SIOCSIWSCAN ioctl.
> + *
> + * For this operation to succeed, the interface is brought Up beforehand.
> + *
> + * Note: It is not clear what value we ought to return when hw_init_completed
> + * is false. If the firmware could not be found/loaded, which errno should we
> + * return?
> + * The same question arises for bDriverStopped being true.
> + *
> + */
The above comment is too long.
> static int r8711_wx_set_scan(struct net_device *dev,
> struct iw_request_info *a,
> union iwreq_data *wrqu, char *extra)
> @@ -1088,7 +1136,7 @@ static int r8711_wx_set_scan(struct net_device *dev,
> return -1;
> }
> if (padapter->bup == false)
> - return -1;
> + return -ENETDOWN;
> if (padapter->hw_init_completed == false)
> return -1;
> if ((check_fwstate(pmlmepriv, _FW_UNDER_SURVEY|_FW_UNDER_LINKING)) ||
> @@ -1122,6 +1170,13 @@ static int r8711_wx_set_scan(struct net_device *dev,
> return 0;
> }
>
> +/**
> + *
> + * This function intends to handle the Get Scan command.
> + * Currently, the request comes via Wireless Extensions' SIOCGIWSCAN ioctl.
> + *
> + *
> + */
Comment not needed.
> static int r8711_wx_get_scan(struct net_device *dev,
> struct iw_request_info *a,
> union iwreq_data *wrqu, char *extra)
> @@ -1169,6 +1224,16 @@ static int r8711_wx_get_scan(struct net_device *dev,
> * s2. set_802_11_authenticaion_mode()
> * s3. set_802_11_encryption_mode()
> * s4. set_802_11_ssid()
> + *
> + * This function intends to handle the Set ESSID command.
> + * Currently, the request comes via the Wireless Extensions' SIOCSIWESSID ioctl.
> + *
> + * For this operation to succeed, there is no need for the interface to be Up.
> + *
> + * Note: Currently (July 2011) the call to
> + * r8712_set_802_11_infrastructure_mode() always returns true. Since we have no
> + * specific failure reason, we return EPERM in lieu of general failure.
> + *
> */
Why is the business about r8712_set_802_11_infrastructure_mode() repeated?
> static int r8711_wx_set_essid(struct net_device *dev,
> struct iw_request_info *a,
> @@ -1187,7 +1252,7 @@ static int r8711_wx_set_essid(struct net_device *dev,
> if (padapter->bup == false)
> return -1;
> if (check_fwstate(pmlmepriv, _FW_UNDER_SURVEY))
> - return -1;
> + return -EBUSY;
> if (check_fwstate(pmlmepriv, _FW_UNDER_LINKING))
> return 0;
> if (wrqu->essid.length> IW_ESSID_MAX_SIZE)
> @@ -1215,7 +1280,7 @@ static int r8711_wx_set_essid(struct net_device *dev,
> if (!r8712_set_802_11_infrastructure_mode(
> padapter,
> pnetwork->network.InfrastructureMode))
> - return -1;
> + return -EPERM; /* Unknown failure. */
> break;
> }
> }
> @@ -1225,6 +1290,12 @@ static int r8711_wx_set_essid(struct net_device *dev,
> return -EINPROGRESS;
> }
>
> +/**
> + *
> + * This function intends to handle the Get ESSID command.
> + * Currently, the request comes via Wireless Extensions' SIOCGIWESSID ioctl.
> + *
> + */
Not needed.
> static int r8711_wx_get_essid(struct net_device *dev,
> struct iw_request_info *a,
> union iwreq_data *wrqu, char *extra)
> @@ -1240,10 +1311,16 @@ static int r8711_wx_get_essid(struct net_device *dev,
> memcpy(extra, pcur_bss->Ssid.Ssid, len);
> wrqu->essid.flags = 1;
> } else
> - ret = -1;
> + ret = -ENOLINK;
> return ret;
> }
>
> +/**
> + *
> + * This function intends to handle the Set Rate command.
> + * Currently, the request comes via the Wireless Extensions' SIOCSIWRATE ioctl.
> + *
> + */
Ditto.
> static int r8711_wx_set_rate(struct net_device *dev,
> struct iw_request_info *a,
> union iwreq_data *wrqu, char *extra)
> @@ -1312,10 +1389,16 @@ set_rate:
> datarates[i] = 0xff;
> }
> if (r8712_setdatarate_cmd(padapter, datarates) != _SUCCESS)
> - ret = -1;
> + ret = -ENOMEM;
> return ret;
> }
>
> +/**
> + *
> + * This function intends to handle the Get Rate command.
> + * Currently, the request comes via the Wireless Extensions' SIOCGIWRATE ioctl.
> + *
> + */
Ditto.
> static int r8711_wx_get_rate(struct net_device *dev,
> struct iw_request_info *info,
> union iwreq_data *wrqu, char *extra)
> @@ -1371,10 +1454,16 @@ static int r8711_wx_get_rate(struct net_device *dev,
> wrqu->bitrate.value = max_rate * 500000;
> }
> } else
> - return -1;
> + return -ENOLINK;
> return 0;
> }
>
> +/**
> + *
> + * This function intends to handle the Get RTS Threshold command.
> + * Currently, the request comes via the Wireless Extensions' SIOCGIWRTS ioctl.
> + *
> + */
Yet again.
> static int r8711_wx_get_rts(struct net_device *dev,
> struct iw_request_info *info,
> union iwreq_data *wrqu, char *extra)
> @@ -1701,7 +1790,7 @@ static int r871x_wx_set_enc_ext(struct net_device *dev,
> param_len = sizeof(struct ieee_param) + pext->key_len;
> param = (struct ieee_param *)_malloc(param_len);
> if (param == NULL)
> - return -1;
> + return -ENOMEM;
> memset(param, 0, param_len);
> param->cmd = IEEE_CMD_SET_ENCRYPTION;
> memset(param->sta_addr, 0xff, ETH_ALEN);
> @@ -1719,7 +1808,7 @@ static int r871x_wx_set_enc_ext(struct net_device *dev,
> alg_name = "CCMP";
> break;
> default:
> - return -1;
> + return -EINVAL;
> }
> strncpy((char *)param->u.crypt.alg, alg_name, IEEE_CRYPT_ALG_NAME_LEN);
> if (pext->ext_flags& IW_ENCODE_EXT_GROUP_KEY)
> @@ -1785,7 +1874,7 @@ static int dummy(struct net_device *dev,
> struct iw_request_info *a,
> union iwreq_data *wrqu, char *b)
> {
> - return -1;
> + return -ENOSYS;
> }
>
> static int r8711_drvext_hdl(struct net_device *dev,
Comments in the Linux kernel are relatively rare. Only things that are not
immediately obvious should receive special annotation.
Larry
next prev parent reply other threads:[~2011-07-15 21:00 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-05-03 14:47 [PATCH 00/12] rtlwifi: rtl8192se: Merge new driver Larry Finger
2011-05-03 15:01 ` Borislav Petkov
2011-05-03 20:03 ` Larry Finger
2011-05-04 17:01 ` Borislav Petkov
2011-05-04 17:20 ` Larry Finger
2011-05-04 18:06 ` Borislav Petkov
2011-05-04 19:15 ` Larry Finger
2011-05-03 15:59 ` Walter Goldens
2011-05-03 16:45 ` Larry Finger
2011-05-03 20:19 ` Gábor Stefanik
2011-05-03 20:33 ` Larry Finger
2011-05-03 20:38 ` Gábor Stefanik
2011-05-03 20:56 ` Larry Finger
2011-05-03 21:09 ` Gábor Stefanik
2011-05-03 21:35 ` Larry Finger
2011-06-21 12:49 ` r8712u driver for the rtl8192su chip Ali Bahar
2011-06-21 22:08 ` Larry Finger
2011-06-22 1:00 ` Ali Bahar
2011-06-22 2:03 ` Julian Calaby
2011-06-22 2:12 ` Gábor Stefanik
2011-06-22 2:46 ` Ali Bahar
2011-06-22 2:39 ` Ali Bahar
2011-06-22 2:34 ` Larry Finger
2011-06-22 2:56 ` Ali Bahar
2011-06-22 3:09 ` Larry Finger
2011-06-22 3:17 ` Ali Bahar
2011-06-27 7:23 ` Ali Bahar
2011-07-02 17:09 ` Larry Finger
2011-07-03 1:06 ` Ali Bahar
2011-07-03 1:24 ` Larry Finger
2011-07-03 1:47 ` Ali Bahar
2011-07-15 17:11 ` [PATCH 0/2] staging: r8712u: Most return-values changed from -1 to Ali Bahar
2011-07-15 17:11 ` [PATCH 1/2] staging: r8712u: Most return-values changed from -1 to proper errno macros Ali Bahar
2011-07-15 21:00 ` Larry Finger [this message]
2011-07-16 3:21 ` Ali Bahar
2011-07-16 9:23 ` Kalle Valo
2011-07-16 10:50 ` Ali Bahar
2011-07-16 11:44 ` Kalle Valo
2011-07-16 11:55 ` Ali Bahar
2011-07-15 17:11 ` [PATCH 2/2] staging: r8712u: checkpatch errors: trailing whitespace on 2 lines Ali Bahar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4E20AA5A.6020101@lwfinger.net \
--to=larry.finger@lwfinger.net \
--cc=ali@internetdog.org \
--cc=gregkh@suse.de \
--cc=linux-wireless@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).