Re: [PATCH] mwifiex: add device specific ioctl handler

[kbuild test robot]

Hi Xinming,

[auto build test ERROR on wireless-drivers-next/master]
[also build test ERROR on v4.14-rc1 next-20170922]
[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/Xinming-Hu/mwifiex-add-device-specific-ioctl-handler/20170923-094243
base:   
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git 
master
config: tile-allyesconfig (attached as .config)
compiler: tilegx-linux-gcc (GCC) 4.6.2
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=tile 

All errors (new ones prefixed by >>):

   drivers/net//wireless/marvell/mwifiex/main.c: In function 'mwifiex_do_ioctl':
   drivers/net//wireless/marvell/mwifiex/main.c:1276:1: error: pasting 
"MWIFIEX_DBG_" and ""ioctl cmd = 0x%x\n"" does not give a valid preprocessing 
token
>> drivers/net//wireless/marvell/mwifiex/main.c:1276:1: error: 'MWIFIEX_DBG_' 
>> undeclared (first use in this function)
   drivers/net//wireless/marvell/mwifiex/main.c:1276:1: note: each undeclared 
identifier is reported only once for each function it appears in
   drivers/net//wireless/marvell/mwifiex/main.c:1276:2: error: expected ')' 
before string constant
>> drivers/net//wireless/marvell/mwifiex/main.c:1276:2: error: too few 
>> arguments to function '_mwifiex_dbg'
   drivers/net//wireless/marvell/mwifiex/main.h:206:6: note: declared here

vim +1276 drivers/net//wireless/marvell/mwifiex/main.c

  1267  
  1268  static int mwifiex_do_ioctl(struct net_device *dev, struct ifreq *req, 
int cmd)
  1269  {
  1270  struct mwifiex_private *priv = mwifiex_netdev_get_priv(dev);
  1271  int ret;
  1272  
  1273  if (!priv->adapter->mfg_mode)
  1274  return -EINVAL;
  1275  
> 1276  mwifiex_dbg(priv->adapter, "ioctl cmd = 0x%x\n", cmd);
  1277  
  1278  switch (cmd) {
  1279  case MWIFIEX_HOSTCMD_IOCTL:
  1280  ret = mwifiex_process_host_command(priv, req);
  1281  break;
  1282  default:
  1283  ret = -EINVAL;
  1284  break;
  1285  }
  1286  
  1287  return ret;
  1288  }
  1289  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH] mwifiex: add device specific ioctl handler

[kbuild test robot]

Hi Xinming,

[auto build test ERROR on wireless-drivers-next/master]
[also build test ERROR on v4.14-rc1 next-20170922]
[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/Xinming-Hu/mwifiex-add-device-specific-ioctl-handler/20170923-094243
base:   
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git 
master
config: x86_64-randconfig-x000-201738 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

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

   In file included from drivers/net/wireless/marvell/mwifiex/main.c:22:0:
   drivers/net/wireless/marvell/mwifiex/main.c: In function 'mwifiex_do_ioctl':
   drivers/net/wireless/marvell/mwifiex/main.h:209:24: error: pasting 
"MWIFIEX_DBG_" and ""ioctl cmd = 0x%x\n"" does not give a valid preprocessing 
token
 _mwifiex_dbg(adapter, MWIFIEX_DBG_##mask, fmt, ##__VA_ARGS__)
   ^
>> drivers/net/wireless/marvell/mwifiex/main.c:1276:2: note: in expansion of 
>> macro 'mwifiex_dbg'
 mwifiex_dbg(priv->adapter, "ioctl cmd = 0x%x\n", cmd);
 ^~~
>> drivers/net/wireless/marvell/mwifiex/main.h:209:24: error: 'MWIFIEX_DBG_' 
>> undeclared (first use in this function)
 _mwifiex_dbg(adapter, MWIFIEX_DBG_##mask, fmt, ##__VA_ARGS__)
   ^
>> drivers/net/wireless/marvell/mwifiex/main.c:1276:2: note: in expansion of 
>> macro 'mwifiex_dbg'
 mwifiex_dbg(priv->adapter, "ioctl cmd = 0x%x\n", cmd);
 ^~~
   drivers/net/wireless/marvell/mwifiex/main.h:209:24: note: each undeclared 
identifier is reported only once for each function it appears in
 _mwifiex_dbg(adapter, MWIFIEX_DBG_##mask, fmt, ##__VA_ARGS__)
   ^
>> drivers/net/wireless/marvell/mwifiex/main.c:1276:2: note: in expansion of 
>> macro 'mwifiex_dbg'
 mwifiex_dbg(priv->adapter, "ioctl cmd = 0x%x\n", cmd);
 ^~~
>> drivers/net/wireless/marvell/mwifiex/main.c:1276:29: error: expected ')' 
>> before string constant
 mwifiex_dbg(priv->adapter, "ioctl cmd = 0x%x\n", cmd);
^
   drivers/net/wireless/marvell/mwifiex/main.h:209:38: note: in definition of 
macro 'mwifiex_dbg'
 _mwifiex_dbg(adapter, MWIFIEX_DBG_##mask, fmt, ##__VA_ARGS__)
 ^~~~
>> drivers/net/wireless/marvell/mwifiex/main.h:209:2: error: too few arguments 
>> to function '_mwifiex_dbg'
 _mwifiex_dbg(adapter, MWIFIEX_DBG_##mask, fmt, ##__VA_ARGS__)
 ^
>> drivers/net/wireless/marvell/mwifiex/main.c:1276:2: note: in expansion of 
>> macro 'mwifiex_dbg'
 mwifiex_dbg(priv->adapter, "ioctl cmd = 0x%x\n", cmd);
 ^~~
   drivers/net/wireless/marvell/mwifiex/main.h:206:6: note: declared here
void _mwifiex_dbg(const struct mwifiex_adapter *adapter, int mask,
 ^~~~

vim +209 drivers/net/wireless/marvell/mwifiex/main.h

c687a0077 drivers/net/wireless/mwifiex/main.h Zhaoyang Liu 2015-05-12  200  
c687a0077 drivers/net/wireless/mwifiex/main.h Zhaoyang Liu 2015-05-12  201  
#define MWIFIEX_DEFAULT_DEBUG_MASK  (MWIFIEX_DBG_MSG | \
c687a0077 drivers/net/wireless/mwifiex/main.h Zhaoyang Liu 2015-05-12  202  
MWIFIEX_DBG_FATAL | \
c687a0077 drivers/net/wireless/mwifiex/main.h Zhaoyang Liu 2015-05-12  203  
MWIFIEX_DBG_ERROR)
c687a0077 drivers/net/wireless/mwifiex/main.h Zhaoyang Liu 2015-05-12  204  
36925e52c drivers/net/wireless/mwifiex/main.h Joe Perches  2015-08-31  205  
__printf(3, 4)
36925e52c drivers/net/wireless/mwifiex/main.h Joe Perches  2015-08-31  206  
void _mwifiex_dbg(const struct mwifiex_adapter *adapter, int mask,
36925e52c drivers/net/wireless/mwifiex/main.h Joe Perches  2015-08-31  207  
  const char *fmt, ...);
36925e52c drivers/net/wireless/mwifiex/main.h Joe Perches  2015-08-31  208  
#define mwifiex_dbg(adapter, mask, fmt, ...)\
36925e52c drivers/net/wireless/mwifiex/main.h Joe Perches  2015-08-31 @209  
_mwifiex_dbg(adapter, MWIFIEX_DBG_##mask, fmt, ##__VA_ARGS__)
c687a0077 drivers/net/wireless/mwifiex/main.h Zhaoyang Liu 2015-05-12  210  

:: The code at line 209 was first introduced by commit
:: 36925e52c5ac9d10a553d1339e13a61bfbdd4ba4 mwifiex: Make mwifiex_dbg a 
function, reduce object size

:: TO: Joe Perches 
:: CC: Kalle Valo 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH] mwifiex: add device specific ioctl handler

[Kalle Valo]

Xinming Hu  writes:

>> You could look at
>> nl80211 vendor commands, but that is also under scrutiny if common wifi
>> functionality is provided by it. I am pretty sure Kalle has his ideas about 
>> this :-)
>> 
>
> Yes, the new utility should use nl80211 interface.
> But we have lots of legacy utility based on ioctl, it will be helpful
> to reduce massive work on refactoring.

That's not an excuse to implement bad interfaces. It's not really that
hard to convert ioctl based tools to use nl80211 testmode command, I
even have myself done that.

-- 
Kalle Valo

Re: [PATCH] mwifiex: add device specific ioctl handler

[Kalle Valo]

Arend van Spriel  writes:

> On 9/20/2017 8:44 AM, Xinming Hu wrote:
>> From: Xinming Hu 
>>
>> Add net_dev ndo_do_ioctl handler, which could be used for
>> downloading host command by utility in manufactory test
>
> Not sure if we want ioctls in upstream wireless drivers.

Yeah, patches adding ioctls to wireless drivers are automatically
rejected.

> You could look at nl80211 vendor commands, but that is also under
> scrutiny if common wifi functionality is provided by it. I am pretty
> sure Kalle has his ideas about this :-)

For manufacturing tests we have nl80211 testmode command and event.
There are upstream drivers already using it so you even have examples.

-- 
Kalle Valo

Re: Re: [PATCH] mwifiex: add device specific ioctl handler

[Xinming Hu]

Hi,

> -Original Message-
> From: Arend van Spriel [mailto:arend.vanspr...@broadcom.com]
> Sent: 2017年9月20日 16:40
> To: Xinming Hu <huxinming...@gmail.com>; Linux Wireless
> <linux-wireless@vger.kernel.org>
> Cc: Kalle Valo <kv...@codeaurora.org>; Brian Norris
> <briannor...@google.com>; Dmitry Torokhov <d...@google.com>;
> raja...@google.com; Zhiyuan Yang <yan...@marvell.com>; Tim Song
> <song...@marvell.com>; Cathy Luo <c...@marvell.com>; Ganapathi Bhat
> <gb...@marvell.com>; Xinming Hu <h...@marvell.com>
> Subject: [EXT] Re: [PATCH] mwifiex: add device specific ioctl handler
> 
> External Email
> 
> --
> On 9/20/2017 8:44 AM, Xinming Hu wrote:
> > From: Xinming Hu <h...@marvell.com>
> >
> > Add net_dev ndo_do_ioctl handler, which could be used for downloading
> > host command by utility in manufactory test
> 
> Not sure if we want ioctls in upstream wireless drivers.

Yes, wext ioctl could be discard and replaced by well defined nl80211/cfg80211.
But per my understand, the net device ioctl will still works normally. Right ?

> You could look at
> nl80211 vendor commands, but that is also under scrutiny if common wifi
> functionality is provided by it. I am pretty sure Kalle has his ideas about 
> this :-)
> 

Yes, the new utility should use nl80211 interface.
But we have lots of legacy utility based on ioctl, it will be helpful to reduce 
massive work on refactoring.

> Regards,
> Arend

RE: [EXT] Re: [PATCH] mwifiex: add device specific ioctl handler

[Xinming Hu]

Hi,

> -Original Message-
> From: Souptick Joarder [mailto:jrdr.li...@gmail.com]
> Sent: 2017年9月20日 16:07
> To: Xinming Hu <huxinming...@gmail.com>
> Cc: Linux Wireless <linux-wireless@vger.kernel.org>; Kalle Valo
> <kv...@codeaurora.org>; Brian Norris <briannor...@google.com>; Dmitry
> Torokhov <d...@google.com>; raja...@google.com; Zhiyuan Yang
> <yan...@marvell.com>; Tim Song <song...@marvell.com>; Cathy Luo
> <c...@marvell.com>; Ganapathi Bhat <gb...@marvell.com>; Xinming Hu
> <h...@marvell.com>
> Subject: [EXT] Re: [PATCH] mwifiex: add device specific ioctl handler
> 
> External Email
> 
> --
> On Wed, Sep 20, 2017 at 12:14 PM, Xinming Hu <huxinming...@gmail.com>
> wrote:
> > From: Xinming Hu <h...@marvell.com>
> >
> > Add net_dev ndo_do_ioctl handler, which could be used for downloading
> > host command by utility in manufactory test
> >
> > Signed-off-by: Xinming Hu <h...@marvell.com>
> > Signed-off-by: Cathy Luo <c...@marvell.com>
> > Signed-off-by: Ganapathi Bhat <gb...@marvell.com>
> > ---
> >  drivers/net/wireless/marvell/mwifiex/cmdevt.c | 59
> +++
> >  drivers/net/wireless/marvell/mwifiex/main.c   | 23 +++
> >  drivers/net/wireless/marvell/mwifiex/main.h   |  7 +++-
> >  3 files changed, 88 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> > b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> > index 0edc5d6..86ee399 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> > @@ -839,6 +839,8 @@ int mwifiex_process_cmdresp(struct
> mwifiex_adapter *adapter)
> > hostcmd = adapter->curr_cmd->data_buf;
> > hostcmd->len = size;
> > memcpy(hostcmd->cmd, resp, size);
> > +   adapter->hostcmd_resp_data.len = size;
> > +   memcpy(adapter->hostcmd_resp_data.cmd,
> resp,
> > + size);
> > }
> > }
> > orig_cmdresp_no = le16_to_cpu(resp->command); @@ -1221,6
> > +1223,63 @@ int mwifiex_ret_802_11_hs_cfg(struct mwifiex_private
> > *priv,  }  EXPORT_SYMBOL_GPL(mwifiex_process_hs_config);
> >
> > +/* This function get hostcmd data from userspace and construct a cmd
> > + * to be download to FW.
> > + */
> > +int mwifiex_process_host_command(struct mwifiex_private *priv,
> > +struct ifreq *req) {
> > +   struct mwifiex_ds_misc_cmd *hostcmd_buf;
> > +   struct host_cmd_ds_command *cmd;
> > +   struct mwifiex_adapter *adapter = priv->adapter;
> > +   int ret;
> > +
> > +   hostcmd_buf = kzalloc(sizeof(*hostcmd_buf), GFP_KERNEL);
> 
> will it be sizeof(*hostcmd_buf) or sizeof( struct mwifiex_ds_misc_cmd *) ?

It should be sizeof(*hostcmd_buf), as we are trying to allocate a struct, not a 
pointer.

> 
> > +   if (!hostcmd_buf)
> > +   return -ENOMEM;
> > +
> > +   cmd = (void *)hostcmd_buf->cmd;
> > +
> > +   if (copy_from_user(cmd, req->ifr_data,
> > +  sizeof(cmd->command) + sizeof(cmd->size)))
> {
> > +   ret = -EFAULT;
> > +   goto done;
> > +   }
> > +
> > +   hostcmd_buf->len = le16_to_cpu(cmd->size);
> > +   if (hostcmd_buf->len > MWIFIEX_SIZE_OF_CMD_BUFFER) {
> > +   ret = -EINVAL;
> > +   goto done;
> > +   }
> > +
> > +   if (copy_from_user(cmd, req->ifr_data, hostcmd_buf->len)) {
> > +   ret = -EFAULT;
> > +   goto done;
> > +   }
> > +
> > +   if (mwifiex_send_cmd(priv, 0, 0, 0, hostcmd_buf, true)) {
> > +   dev_err(priv->adapter->dev, "Failed to process
> hostcmd\n");
> > +   ret = -EFAULT;
> > +   goto done;
> > +   }
> > +
> > +   if (adapter->hostcmd_resp_data.len > hostcmd_buf->len) {
> > +   ret = -EFBIG;
> > +   goto done;
> > +   }
> > +
> > +   if (copy_to_user(req->ifr_data, adapter->hostcmd_resp_data.cmd,
> > +adapter->hostcmd_resp_data.len)) {
> > +   ret = -EFAULT;
> > +   goto done;

Re: [PATCH] mwifiex: add device specific ioctl handler

[Arend van Spriel]

On 9/20/2017 8:44 AM, Xinming Hu wrote:

From: Xinming Hu 

Add net_dev ndo_do_ioctl handler, which could be used for
downloading host command by utility in manufactory test


Not sure if we want ioctls in upstream wireless drivers. You could look 
at nl80211 vendor commands, but that is also under scrutiny if common 
wifi functionality is provided by it. I am pretty sure Kalle has his 
ideas about this :-)


Regards,
Arend

Re: [PATCH] mwifiex: add device specific ioctl handler

[Souptick Joarder]

On Wed, Sep 20, 2017 at 12:14 PM, Xinming Hu  wrote:
> From: Xinming Hu 
>
> Add net_dev ndo_do_ioctl handler, which could be used for
> downloading host command by utility in manufactory test
>
> Signed-off-by: Xinming Hu 
> Signed-off-by: Cathy Luo 
> Signed-off-by: Ganapathi Bhat 
> ---
>  drivers/net/wireless/marvell/mwifiex/cmdevt.c | 59 
> +++
>  drivers/net/wireless/marvell/mwifiex/main.c   | 23 +++
>  drivers/net/wireless/marvell/mwifiex/main.h   |  7 +++-
>  3 files changed, 88 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c 
> b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> index 0edc5d6..86ee399 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> @@ -839,6 +839,8 @@ int mwifiex_process_cmdresp(struct mwifiex_adapter 
> *adapter)
> hostcmd = adapter->curr_cmd->data_buf;
> hostcmd->len = size;
> memcpy(hostcmd->cmd, resp, size);
> +   adapter->hostcmd_resp_data.len = size;
> +   memcpy(adapter->hostcmd_resp_data.cmd, resp, size);
> }
> }
> orig_cmdresp_no = le16_to_cpu(resp->command);
> @@ -1221,6 +1223,63 @@ int mwifiex_ret_802_11_hs_cfg(struct mwifiex_private 
> *priv,
>  }
>  EXPORT_SYMBOL_GPL(mwifiex_process_hs_config);
>
> +/* This function get hostcmd data from userspace and construct a cmd
> + * to be download to FW.
> + */
> +int mwifiex_process_host_command(struct mwifiex_private *priv,
> +struct ifreq *req)
> +{
> +   struct mwifiex_ds_misc_cmd *hostcmd_buf;
> +   struct host_cmd_ds_command *cmd;
> +   struct mwifiex_adapter *adapter = priv->adapter;
> +   int ret;
> +
> +   hostcmd_buf = kzalloc(sizeof(*hostcmd_buf), GFP_KERNEL);

will it be sizeof(*hostcmd_buf) or sizeof( struct mwifiex_ds_misc_cmd *) ?

> +   if (!hostcmd_buf)
> +   return -ENOMEM;
> +
> +   cmd = (void *)hostcmd_buf->cmd;
> +
> +   if (copy_from_user(cmd, req->ifr_data,
> +  sizeof(cmd->command) + sizeof(cmd->size))) {
> +   ret = -EFAULT;
> +   goto done;
> +   }
> +
> +   hostcmd_buf->len = le16_to_cpu(cmd->size);
> +   if (hostcmd_buf->len > MWIFIEX_SIZE_OF_CMD_BUFFER) {
> +   ret = -EINVAL;
> +   goto done;
> +   }
> +
> +   if (copy_from_user(cmd, req->ifr_data, hostcmd_buf->len)) {
> +   ret = -EFAULT;
> +   goto done;
> +   }
> +
> +   if (mwifiex_send_cmd(priv, 0, 0, 0, hostcmd_buf, true)) {
> +   dev_err(priv->adapter->dev, "Failed to process hostcmd\n");
> +   ret = -EFAULT;
> +   goto done;
> +   }
> +
> +   if (adapter->hostcmd_resp_data.len > hostcmd_buf->len) {
> +   ret = -EFBIG;
> +   goto done;
> +   }
> +
> +   if (copy_to_user(req->ifr_data, adapter->hostcmd_resp_data.cmd,
> +adapter->hostcmd_resp_data.len)) {
> +   ret = -EFAULT;
> +   goto done;
> +   }
> +
> +   ret = 0;
> +done:
> +   kfree(hostcmd_buf);
> +   return ret;
> +}
> +
>  /*
>   * This function handles the command response of a sleep confirm command.
>   *
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
> b/drivers/net/wireless/marvell/mwifiex/main.c
> index ee40b73..3e7700f 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -1265,12 +1265,35 @@ static struct net_device_stats 
> *mwifiex_get_stats(struct net_device *dev)
> return mwifiex_1d_to_wmm_queue[skb->priority];
>  }
>
> +static int mwifiex_do_ioctl(struct net_device *dev, struct ifreq *req, int 
> cmd)
> +{
> +   struct mwifiex_private *priv = mwifiex_netdev_get_priv(dev);
> +   int ret;
> +
> +   if (!priv->adapter->mfg_mode)
> +   return -EINVAL;

ret can be used instead of returning -EINVAL.

> +
> +   mwifiex_dbg(priv->adapter, "ioctl cmd = 0x%x\n", cmd);
> +
> +   switch (cmd) {
> +   case MWIFIEX_HOSTCMD_IOCTL:
> +   ret = mwifiex_process_host_command(priv, req);
> +   break;
> +   default:
> +   ret = -EINVAL;
> +   break;
> +   }
> +
> +   return ret;
> +}
> +
>  /* Network device handlers */
>  static const struct net_device_ops mwifiex_netdev_ops = {
> .ndo_open = mwifiex_open,
> .ndo_stop = mwifiex_close,
> .ndo_start_xmit = mwifiex_hard_start_xmit,
> .ndo_set_mac_address = mwifiex_ndo_set_mac_address,
> +   .ndo_do_ioctl = mwifiex_do_ioctl,
> .ndo_validate_addr = eth_validate_addr,
> .ndo_tx_timeout = mwifiex_tx_timeout,
>