Re: [PATCH] staging: rtl8188eu: Fix private WEXT IOCTL calls

[kbuild test robot]

Hi Ishraq,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on staging/staging-testing]
[also build test ERROR on v4.14 next-20171124]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/ishraq-i-ashraf-gmail-com/staging-rtl8188eu-Fix-private-WEXT-IOCTL-calls/20171126-052554
config: x86_64-randconfig-u0-11270543 (attached as .config)
compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c: In function 
'rtw_hostapd_sta_flush_pvt':
>> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3101:9: error: implicit 
>> declaration of function 'rtw_sta_flush' 
>> [-Werror=implicit-function-declaration]
 return rtw_sta_flush(padapter);
^
   In file included from include/linux/kernel.h:14:0,
from include/linux/skbuff.h:17,
from include/linux/if_ether.h:23,
from include/linux/ieee80211.h:21,
from drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:17:
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c: In function 
'rtw_add_sta_pvt':
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3133:49: error: 'union 
' has no member named 'add_sta'
 DBG_88E("rtw_add_sta(aid =%d) =%pM\n", param->u.add_sta.aid, 
(param->sta_addr));
^
   include/linux/printk.h:310:34: note: in definition of macro 'pr_info'
 printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
 ^
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3133:2: note: in expansion of 
macro 'DBG_88E'
 DBG_88E("rtw_add_sta(aid =%d) =%pM\n", param->u.add_sta.aid, 
(param->sta_addr));
 ^
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3145:23: error: 'union 
' has no member named 'add_sta'
  int flags = param->u.add_sta.flags;
  ^
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3146:23: error: 'union 
' has no member named 'add_sta'
  psta->aid = param->u.add_sta.aid; // aid = 1~2007.
  ^
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3148:36: error: 'union 
' has no member named 'add_sta'
  memcpy(psta->bssrateset, param->u.add_sta.tx_supp_rates, 16);
   ^
>> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3151:7: error: 'WLAN_STA_WME' 
>> undeclared (first use in this function)
  if (WLAN_STA_WME)
  ^
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3151:7: note: each undeclared 
identifier is reported only once for each function it appears in
>> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3160:7: error: 'WLAN_STA_HT' 
>> undeclared (first use in this function)
  if (WLAN_STA_HT) {
  ^
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3164:20: error: 'union 
' has no member named 'add_sta'
  >u.add_sta.ht_cap,
   ^
>> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3173:3: error: implicit 
>> declaration of function 'update_sta_info_apmode' 
>> [-Werror=implicit-function-declaration]
  update_sta_info_apmode(padapter, psta);
  ^
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c: In function 
'rtw_del_sta_pvt':
>> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3229:12: error: 'struct 
>> sta_priv' has no member named 'asoc_list_cnt'
   pstapriv->asoc_list_cnt--;
   ^
>> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3230:14: error: implicit 
>> declaration of function 'ap_free_sta' [-Werror=implicit-function-declaration]
   updated = ap_free_sta(padapter, psta, true, WLAN_REASON_DEAUTH_LEAVING);
 ^
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3233:3: error: implicit 
declaration of function 'associated_clients_update' 
[-Werror=implicit-function-declaration]
  associated_clients_update(padapter, updated);
  ^
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c: In function 
'rtw_set_beacon_pvt':
>> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3276:17: error: 'union 
>> ' has no member named 'bcn_ie'
 pbuf = param->u.bcn_ie.buf;
^
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3283:18: error: 'struct 
sta_priv' has no member named 'max_num_sta'
 memcpy(>max_num_sta, param->u.bcn_ie.reserved, 2);
 ^
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3283:41: error: 'union 
' has no member named 'bcn_ie'
 memcpy(>max_num_sta, param->u.bcn_ie.reserved, 2);
^
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3285:15: error: 'struct 
sta_priv' has no member named 'max_num_sta'
 if ((pstapriv->max_num_sta > NUM_STA) || (pstapriv->max_num_sta <= 0))
  ^
   

Re: [PATCH] staging: rtl8188eu: Fix private WEXT IOCTL calls

[kbuild test robot]

Hi Ishraq,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on staging/staging-testing]
[also build test ERROR on v4.14 next-20171124]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/ishraq-i-ashraf-gmail-com/staging-rtl8188eu-Fix-private-WEXT-IOCTL-calls/20171126-052554
config: x86_64-randconfig-n0-11270329 (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c: In function 
'rtw_hostapd_sta_flush_pvt':
>> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3101:9: error: implicit 
>> declaration of function 'rtw_sta_flush'; did you mean 'rtw_set_auth'? 
>> [-Werror=implicit-function-declaration]
 return rtw_sta_flush(padapter);
^
rtw_set_auth
   In file included from include/linux/kernel.h:14:0,
from include/linux/skbuff.h:17,
from include/linux/if_ether.h:23,
from include/linux/ieee80211.h:21,
from drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:17:
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c: In function 
'rtw_add_sta_pvt':
>> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3133:49: error: 'union 
>> ' has no member named 'add_sta'
 DBG_88E("rtw_add_sta(aid =%d) =%pM\n", param->u.add_sta.aid, 
(param->sta_addr));
^
   include/linux/printk.h:310:34: note: in definition of macro 'pr_info'
 printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
 ^~~
>> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3133:2: note: in expansion of 
>> macro 'DBG_88E'
 DBG_88E("rtw_add_sta(aid =%d) =%pM\n", param->u.add_sta.aid, 
(param->sta_addr));
 ^~~
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3145:23: error: 'union 
' has no member named 'add_sta'
  int flags = param->u.add_sta.flags;
  ^
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3146:23: error: 'union 
' has no member named 'add_sta'
  psta->aid = param->u.add_sta.aid; // aid = 1~2007.
  ^
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3148:36: error: 'union 
' has no member named 'add_sta'
  memcpy(psta->bssrateset, param->u.add_sta.tx_supp_rates, 16);
   ^
>> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3151:7: error: 'WLAN_STA_WME' 
>> undeclared (first use in this function); did you mean 'SCAN_STATE'?
  if (WLAN_STA_WME)
  ^~~~
  SCAN_STATE
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3151:7: note: each undeclared 
identifier is reported only once for each function it appears in
>> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3160:7: error: 'WLAN_STA_HT' 
>> undeclared (first use in this function); did you mean 'WLAN_STA_WME'?
  if (WLAN_STA_HT) {
  ^~~
  WLAN_STA_WME
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3164:20: error: 'union 
' has no member named 'add_sta'
  >u.add_sta.ht_cap,
   ^
>> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3173:3: error: implicit 
>> declaration of function 'update_sta_info_apmode'; did you mean 
>> 'update_sta_info'? [-Werror=implicit-function-declaration]
  update_sta_info_apmode(padapter, psta);
  ^~
  update_sta_info
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c: In function 
'rtw_del_sta_pvt':
>> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3229:14: error: 'struct 
>> sta_priv' has no member named 'asoc_list_cnt'; did you mean 'asoc_list_lock'?
   pstapriv->asoc_list_cnt--;
 ^
 asoc_list_lock
>> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3230:14: error: implicit 
>> declaration of function 'ap_free_sta'; did you mean '__kfree_skb'? 
>> [-Werror=implicit-function-declaration]
   updated = ap_free_sta(padapter, psta, true, WLAN_REASON_DEAUTH_LEAVING);
 ^~~
 __kfree_skb
>> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3233:3: error: implicit 
>> declaration of function 'associated_clients_update' 
>> [-Werror=implicit-function-declaration]
  associated_clients_update(padapter, updated);
  ^
   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c: In function 
'rtw_set_beacon_pvt':
>> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3276:18: error: 'union 
>> ' has no member named 'bcn_ie'; did you mean 'wpa_ie'?
 pbuf = param->u.bcn_ie.buf;
 ^~
 wpa_ie
>> drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:3283:18: error: 'struct 
>> sta_priv' has no member named 

Re: [PATCH] staging: rtl8188eu: Fix private WEXT IOCTL calls

[Dan Carpenter]

On Thu, Nov 23, 2017 at 02:29:06AM +0100, ishraq.i.ash...@gmail.com wrote:
> From: Ishraq Ibne Ashraf 
> 
> Commit 8bfb36766064 ("wireless: wext: remove ndo_do_ioctl fallback") breaks 
> private WEXT
> IOCTL calls of this driver as these are not invoked through ndo_do_ioctl
> interface anymore. As a result hostapd stops working with this driver. In
> this patch this problem is solved by implementing equivalent private IOCTL
> functions of the existing ones which are accessed via iw_handler_def
> interface.
> 
> Signed-off-by: Ishraq Ibne Ashraf 


It's great to fix this, but new code should be at normal kernel quality.

> ---
>  drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 1042 
> 
>  1 file changed, 1042 insertions(+)
> 
> diff --git a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c 
> b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
> index c0664dc..7503751 100644
> --- a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
> +++ b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
> @@ -3061,6 +3061,1046 @@ static iw_handler rtw_handlers[] = {
>   NULL,   /*---hole---*/
>  };
>  
> +static int get_private_handler_ieee_param(struct adapter *padapter,
> + union iwreq_data *wrqu,
> + void *param)

Indent these more.

> +{
> + /*
> +  * This function is expected to be called in master mode, which allows 
> no
> +  * power saving. So we just check hw_init_completed.
> +  */
> +
> + if (!padapter->hw_init_completed)
> + return -EPERM;
> +
> + if (!wrqu->data.pointer)
> + return -EINVAL;

You could leave this out and it will return -EFAULT when copy_from_user()
fails.  That's probably the right error code.

> +
> + /*
> +  * Since we don't allocate memory for param in this function, we assume
> +  * the caller of this function will properly allocate and deallocate 
> memory
> +  * for param.
> +  */

This is an obvious comment.  Remove it.

> + if (copy_from_user(param, wrqu->data.pointer, wrqu->data.length))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static int rtw_hostapd_sta_flush_pvt(struct net_device *dev,
> + struct iw_request_info *info,
> + union iwreq_data *wrqu,
> + char *extra)
> +{
> + struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
> +
> + DBG_88E("%s\n", __func__);

Remove this line.

> +
> + flush_all_cam_entry(padapter); // Clear CAM.

Comment style.

> +
> + return rtw_sta_flush(padapter);
> +}
> +
> +static int rtw_add_sta_pvt(struct net_device *dev,
> + struct iw_request_info *info,
> + union iwreq_data *wrqu,
> + char *extra)
> +{
> + int ret = 0;
> + struct sta_info *psta = NULL;
> + struct ieee_param *param = NULL;

Don't initialize these to useless values.  It turns off the static
checker for finding uninitialized variables.

> + struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
> + struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
> + struct sta_priv *pstapriv = >stapriv;
> +
> + param = (struct ieee_param *)rtw_malloc(wrqu->data.length);

rtw_malloc() is buggy.  It ignores locking.  Let's not use it in new
code.

> +
> + if (!param) {
> + DBG_88E(" rtw_add_sta: ieee_param allocate fail !!!\n");

No need to print this debug message.

> +
> + return -ENOMEM;
> + }
> +
> + ret = get_private_handler_ieee_param(padapter, wrqu, param);
> +
> + if (ret != 0) {

if (ret).  "ret" isn't a number zero which can be used for math like
"ret + 2", it's an error code.  The != 0 is a double negative which
hurts readability.  != 0 is appropriate for numbers and strcmp()
functions.

> + kfree(param);

Free this at the end of the function.

> + DBG_88E(" rtw_add_sta: ieee_param get fail !!!\n");

These messages are so ugly !!!

> +
> + return ret;
> + }
> +
> + DBG_88E("rtw_add_sta(aid =%d) =%pM\n", param->u.add_sta.aid, 
> (param->sta_addr));
> +
> + if (!check_fwstate(pmlmepriv, (_FW_LINKED|WIFI_AP_STATE)))
> + return -EINVAL;

ret = -EINVAL;
goto err_free_param;

> +
> + if (param->sta_addr[0] == 0xff && param->sta_addr[1] == 0xff &&
> + param->sta_addr[2] == 0xff && param->sta_addr[3] == 0xff &&
> + param->sta_addr[4] == 0xff && param->sta_addr[5] == 0xff)
> +   return -EINVAL;

ret = -EINVAL;
goto err_free_param;


> +
> + psta = rtw_get_stainfo(pstapriv, param->sta_addr);
> + if (psta) {

Always do failure handling.  Never do success handling.  So this becomes:

if (!psta)
goto err_free_param;



> + int flags = param->u.add_sta.flags;
> + psta->aid = param->u.add_sta.aid; // aid = 1~2007.
> +
> + memcpy(psta->bssrateset, 

[PATCH] staging: rtl8188eu: Fix private WEXT IOCTL calls

[ishraq . i . ashraf]

From: Ishraq Ibne Ashraf 

Commit 8bfb36766064 ("wireless: wext: remove ndo_do_ioctl fallback") breaks 
private WEXT
IOCTL calls of this driver as these are not invoked through ndo_do_ioctl
interface anymore. As a result hostapd stops working with this driver. In
this patch this problem is solved by implementing equivalent private IOCTL
functions of the existing ones which are accessed via iw_handler_def
interface.

Signed-off-by: Ishraq Ibne Ashraf 
---
 drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 1042 
 1 file changed, 1042 insertions(+)

diff --git a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c 
b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
index c0664dc..7503751 100644
--- a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
+++ b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
@@ -3061,6 +3061,1046 @@ static iw_handler rtw_handlers[] = {
NULL,   /*---hole---*/
 };
 
+static int get_private_handler_ieee_param(struct adapter *padapter,
+   union iwreq_data *wrqu,
+   void *param)
+{
+   /*
+* This function is expected to be called in master mode, which allows 
no
+* power saving. So we just check hw_init_completed.
+*/
+
+   if (!padapter->hw_init_completed)
+   return -EPERM;
+
+   if (!wrqu->data.pointer)
+   return -EINVAL;
+
+   /*
+* Since we don't allocate memory for param in this function, we assume
+* the caller of this function will properly allocate and deallocate 
memory
+* for param.
+*/
+   if (copy_from_user(param, wrqu->data.pointer, wrqu->data.length))
+   return -EFAULT;
+
+   return 0;
+}
+
+static int rtw_hostapd_sta_flush_pvt(struct net_device *dev,
+   struct iw_request_info *info,
+   union iwreq_data *wrqu,
+   char *extra)
+{
+   struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
+
+   DBG_88E("%s\n", __func__);
+
+   flush_all_cam_entry(padapter); // Clear CAM.
+
+   return rtw_sta_flush(padapter);
+}
+
+static int rtw_add_sta_pvt(struct net_device *dev,
+   struct iw_request_info *info,
+   union iwreq_data *wrqu,
+   char *extra)
+{
+   int ret = 0;
+   struct sta_info *psta = NULL;
+   struct ieee_param *param = NULL;
+   struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
+   struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
+   struct sta_priv *pstapriv = >stapriv;
+
+   param = (struct ieee_param *)rtw_malloc(wrqu->data.length);
+
+   if (!param) {
+   DBG_88E(" rtw_add_sta: ieee_param allocate fail !!!\n");
+
+   return -ENOMEM;
+   }
+
+   ret = get_private_handler_ieee_param(padapter, wrqu, param);
+
+   if (ret != 0) {
+   kfree(param);
+   DBG_88E(" rtw_add_sta: ieee_param get fail !!!\n");
+
+   return ret;
+   }
+
+   DBG_88E("rtw_add_sta(aid =%d) =%pM\n", param->u.add_sta.aid, 
(param->sta_addr));
+
+   if (!check_fwstate(pmlmepriv, (_FW_LINKED|WIFI_AP_STATE)))
+   return -EINVAL;
+
+   if (param->sta_addr[0] == 0xff && param->sta_addr[1] == 0xff &&
+   param->sta_addr[2] == 0xff && param->sta_addr[3] == 0xff &&
+   param->sta_addr[4] == 0xff && param->sta_addr[5] == 0xff)
+ return -EINVAL;
+
+   psta = rtw_get_stainfo(pstapriv, param->sta_addr);
+   if (psta) {
+   int flags = param->u.add_sta.flags;
+   psta->aid = param->u.add_sta.aid; // aid = 1~2007.
+
+   memcpy(psta->bssrateset, param->u.add_sta.tx_supp_rates, 16);
+
+   // Check WMM cap.
+   if (WLAN_STA_WME)
+   psta->qos_option = 1;
+   else
+   psta->qos_option = 0;
+
+   if (pmlmepriv->qospriv.qos_option == 0)
+   psta->qos_option = 0;
+
+   // Check 802.11n HT cap.
+   if (WLAN_STA_HT) {
+   psta->htpriv.ht_option = true;
+   psta->qos_option = 1;
+   memcpy(>htpriv.ht_cap,
+  >u.add_sta.ht_cap,
+  sizeof(struct ieee80211_ht_cap));
+   } else {
+   psta->htpriv.ht_option = false;
+   }
+
+   if (pmlmepriv->htpriv.ht_option == false)
+   psta->htpriv.ht_option = false;
+
+   update_sta_info_apmode(padapter, psta);
+   } else {
+   ret = -ENOMEM;
+   }
+
+   if (ret == 0 && (copy_to_user(wrqu->data.pointer, param, 
wrqu->data.length)))
+   ret = -EFAULT;
+
+   return ret;
+}
+
+static int rtw_del_sta_pvt(struct net_device *dev,
+   struct iw_request_info *info,
+   union iwreq_data *wrqu,
+   char *extra)
+{