mwifiex: handle edmac vendor command

Commit Message

Amitkumar Karwar April 29, 2016, 3:58 p.m. UTC

From: chunfan chen <[e-mail]>

Userspace can configure edmac values through a custom vendor
command. They will be used by hardware for adaptivity.

Signed-off-by: chunfan chen <[e-mail]>
Signed-off-by: Amitkumar Karwar <[e-mail]>
---
 drivers/net/wireless/marvell/mwifiex/Makefile   |  1 +
 drivers/net/wireless/marvell/mwifiex/cfg80211.c |  2 +
 drivers/net/wireless/marvell/mwifiex/main.h     |  4 ++
 drivers/net/wireless/marvell/mwifiex/sta_cmd.c  | 20 ++++++---
 drivers/net/wireless/marvell/mwifiex/vendor.c   | 59 +++++++++++++++++++++++++
 drivers/net/wireless/marvell/mwifiex/vendor.h   | 27 +++++++++++
 6 files changed, 108 insertions(+), 5 deletions(-)
 create mode 100644 drivers/net/wireless/marvell/mwifiex/vendor.c
 create mode 100644 drivers/net/wireless/marvell/mwifiex/vendor.h

Comments

Amitkumar Karwar May 12, 2016, 3:39 p.m. UTC | #1

Hi Kalle,
	
> From: Amitkumar Karwar [mailto:[e-mail]]
> Sent: Friday, April 29, 2016 9:28 PM
> To: [e-mail]
> Cc: Jeff CF Chen; Amitkumar Karwar
> Subject: [PATCH] mwifiex: handle edmac vendor command
> 
> From: chunfan chen <[e-mail]>
> 
> Userspace can configure edmac values through a custom vendor command.
> They will be used by hardware for adaptivity.
> 
> Signed-off-by: chunfan chen <[e-mail]>
> Signed-off-by: Amitkumar Karwar <[e-mail]>
> ---
>  drivers/net/wireless/marvell/mwifiex/Makefile   |  1 +
>  drivers/net/wireless/marvell/mwifiex/cfg80211.c |  2 +
>  drivers/net/wireless/marvell/mwifiex/main.h     |  4 ++
>  drivers/net/wireless/marvell/mwifiex/sta_cmd.c  | 20 ++++++---
>  drivers/net/wireless/marvell/mwifiex/vendor.c   | 59
> +++++++++++++++++++++++++
>  drivers/net/wireless/marvell/mwifiex/vendor.h   | 27 +++++++++++
>  6 files changed, 108 insertions(+), 5 deletions(-)  create mode 100644
> drivers/net/wireless/marvell/mwifiex/vendor.c
>  create mode 100644 drivers/net/wireless/marvell/mwifiex/vendor.h
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/Makefile
> b/drivers/net/wireless/marvell/mwifiex/Makefile
> index fdfd9bf..8b34ce9 100644
> --- a/drivers/net/wireless/marvell/mwifiex/Makefile
> +++ b/drivers/net/wireless/marvell/mwifiex/Makefile
> @@ -42,6 +42,7 @@ mwifiex-y += cfg80211.o  mwifiex-y += ethtool.o
> mwifiex-y += 11h.o  mwifiex-y += tdls.o
> +mwifiex-y += vendor.o
>  mwifiex-$(CONFIG_DEBUG_FS) += debugfs.o
>  obj-$(CONFIG_MWIFIEX) += mwifiex.o
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> index ff948a9..00aca7e 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> @@ -4118,6 +4118,8 @@ int mwifiex_register_cfg80211(struct
> mwifiex_adapter *adapter)
>  	if (adapter->fw_api_ver == MWIFIEX_FW_V15)
>  		wiphy->features |= NL80211_FEATURE_SK_TX_STATUS;
> 
> +	marvell_set_vendor_commands(wiphy);
> +
>  	/* Reserve space for mwifiex specific private data for BSS */
>  	wiphy->bss_priv_size = sizeof(struct mwifiex_bss_priv);
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h
> b/drivers/net/wireless/marvell/mwifiex/main.h
> index 0207af0..66ba5c0 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -1002,6 +1002,8 @@ struct mwifiex_adapter {
>  	bool usb_mc_status;
>  	bool usb_mc_setup;
>  	struct cfg80211_wowlan_nd_info *nd_info;
> +	u8 *cfg_data;
> +	int cfg_len;
>  };
> 
>  void mwifiex_process_tx_queue(struct mwifiex_adapter *adapter); @@ -
> 1614,6 +1616,8 @@ void mwifiex_process_multi_chan_event(struct
> mwifiex_private *priv,
>  				      struct sk_buff *event_skb);
>  void mwifiex_multi_chan_resync(struct mwifiex_adapter *adapter);
> 
> +void marvell_set_vendor_commands(struct wiphy *wiphy);
> +
>  #ifdef CONFIG_DEBUG_FS
>  void mwifiex_debugfs_init(void);
>  void mwifiex_debugfs_remove(void);
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
> b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
> index e436574..6b8cc39 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
> @@ -1487,9 +1487,10 @@ static int mwifiex_cmd_cfg_data(struct
> mwifiex_private *priv,  {
>  	struct mwifiex_adapter *adapter = priv->adapter;
>  	struct property *prop = data_buf;
> -	u32 len;
> +	u32 len = 0;
>  	u8 *data = (u8 *)cmd + S_DS_GEN;
>  	int ret;
> +	const struct firmware *cal_data = adapter->cal_data;
> 
>  	if (prop) {
>  		len = prop->length;
> @@ -1500,11 +1501,20 @@ static int mwifiex_cmd_cfg_data(struct
> mwifiex_private *priv,
>  		mwifiex_dbg(adapter, INFO,
>  			    "download cfg_data from device tree: %s\n",
>  			    prop->name);
> -	} else if (adapter->cal_data->data && adapter->cal_data->size > 0)
> {
> -		len = mwifiex_parse_cal_cfg((u8 *)adapter->cal_data->data,
> -					    adapter->cal_data->size, data);
> +	} else if (cal_data) {
> +		if (cal_data->data && cal_data->size > 0) {
> +			len = mwifiex_parse_cal_cfg((u8 *)cal_data->data,
> +						    cal_data->size, data);
> +			mwifiex_dbg(adapter, INFO,
> +				    "download cfg_data from config file\n");
> +		} else {
> +			return -1;
> +		}
> +	} else if (adapter->cfg_data && adapter->cfg_len > 0) {
> +		len = mwifiex_parse_cal_cfg(adapter->cfg_data,
> +					    adapter->cfg_len, data);
>  		mwifiex_dbg(adapter, INFO,
> -			    "download cfg_data from config file\n");
> +			    "download cfg_data from iw vendor command\n");
>  	} else {
>  		return -1;
>  	}
> diff --git a/drivers/net/wireless/marvell/mwifiex/vendor.c
> b/drivers/net/wireless/marvell/mwifiex/vendor.c
> new file mode 100644
> index 0000000..ecdc10c
> --- /dev/null
> +++ b/drivers/net/wireless/marvell/mwifiex/vendor.c
> @@ -0,0 +1,59 @@
> +/* Marvell Wireless LAN device driver: TDLS handling
> + *
> + * Copyright (C) 2014, Marvell International Ltd.
> + *
> + * This software file (the "File") is distributed by Marvell
> +International
> + * Ltd. under the terms of the GNU General Public License Version 2,
> +June 1991
> + * (the "License").  You may use, redistribute and/or modify this File
> +in
> + * accordance with the terms and conditions of the License, a copy of
> +which
> + * is available on the worldwide web at
> + * http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt.
> + *
> + * THE FILE IS DISTRIBUTED AS-IS, WITHOUT WARRANTY OF ANY KIND, AND THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY OR FITNESS FOR A PARTICULAR
> +PURPOSE
> + * ARE EXPRESSLY DISCLAIMED.  The License provides additional details
> +about
> + * this warranty disclaimer.
> + */
> +
> +#include <net/mac80211.h>
> +#include <net/netlink.h>
> +#include "vendor.h"
> +#include "main.h"
> +
> +static int
> +mwifiex_vendor_cmd_set_edmac_data(struct wiphy *wiphy,
> +				  struct wireless_dev *wdev,
> +				  const void *data, int data_len)
> +{
> +	struct mwifiex_private *priv = mwifiex_netdev_get_priv(wdev-
> >netdev);
> +	int ret;
> +
> +	priv->adapter->cfg_data = (u8 *)data;
> +	priv->adapter->cfg_len = data_len;
> +
> +	ret = mwifiex_send_cmd(priv, HostCmd_CMD_CFG_DATA,
> +			       HostCmd_ACT_GEN_SET, 0, NULL, true);
> +
> +	priv->adapter->cfg_data = NULL;
> +	priv->adapter->cfg_len = 0;
> +
> +	return 0;
> +}
> +
> +static const struct wiphy_vendor_command marvell_vendor_commands[] = {
> +	{
> +		.info = {
> +			.vendor_id = MARVELL_OUI,
> +			.subcmd = MARVELL_VENDOR_CMD_SET_EDMAC_DATA,
> +		},
> +		.flags = WIPHY_VENDOR_CMD_NEED_NETDEV |
> +			 WIPHY_VENDOR_CMD_NEED_RUNNING,
> +		.doit = mwifiex_vendor_cmd_set_edmac_data,
> +	},
> +};
> +
> +void marvell_set_vendor_commands(struct wiphy *wiphy) {
> +	wiphy->vendor_commands = marvell_vendor_commands;
> +	wiphy->n_vendor_commands = ARRAY_SIZE(marvell_vendor_commands);
> +}
> diff --git a/drivers/net/wireless/marvell/mwifiex/vendor.h
> b/drivers/net/wireless/marvell/mwifiex/vendor.h
> new file mode 100644
> index 0000000..e5d6fe8
> --- /dev/null
> +++ b/drivers/net/wireless/marvell/mwifiex/vendor.h
> @@ -0,0 +1,27 @@
> +/* Marvell Wireless LAN device driver: TDLS handling
> + *
> + * Copyright (C) 2014, Marvell International Ltd.
> + *
> + * This software file (the "File") is distributed by Marvell
> +International
> + * Ltd. under the terms of the GNU General Public License Version 2,
> +June 1991
> + * (the "License").  You may use, redistribute and/or modify this File
> +in
> + * accordance with the terms and conditions of the License, a copy of
> +which
> + * is available on the worldwide web at
> + * http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt.
> + *
> + * THE FILE IS DISTRIBUTED AS-IS, WITHOUT WARRANTY OF ANY KIND, AND THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY OR FITNESS FOR A PARTICULAR
> +PURPOSE
> + * ARE EXPRESSLY DISCLAIMED.  The License provides additional details
> +about
> + * this warranty disclaimer.
> + */
> +
> +#ifndef __MARVELL_VENDOR_H__
> +#define __MARVELL_VENDOR_H__
> +
> +#define MARVELL_OUI	0x005043
> +
> +enum marvell_vendor_commands {
> +	MARVELL_VENDOR_CMD_SET_EDMAC_DATA,
> +};
> +
> +#endif /* __MARVELL_VENDOR_H__ */
> --

This patch seems to have deferred.
We basically want a way to download a vendor specific configuration to our firmware.
Do you have any suggestions on how can achieve this in better way?

I can see below iw command suits our requirement.

iw dev <devname> vendor send <oui> <subcmd> <filename|-|hex data>

Please guide.

Regards,
Amitkumar
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kalle Valo May 23, 2016, 8:22 p.m. UTC | #2

Amitkumar Karwar <[e-mail]> writes:

> Hi Kalle,
> 	
>> From: Amitkumar Karwar [mailto:[e-mail]]
>> Sent: Friday, April 29, 2016 9:28 PM
>> To: [e-mail]
>> Cc: Jeff CF Chen; Amitkumar Karwar
>> Subject: [PATCH] mwifiex: handle edmac vendor command
>> 
>> From: chunfan chen <[e-mail]>
>> 
>> Userspace can configure edmac values through a custom vendor command.
>> They will be used by hardware for adaptivity.
>> 
>> Signed-off-by: chunfan chen <[e-mail]>
>> Signed-off-by: Amitkumar Karwar <[e-mail]>

[deleted over 200 lines of unnecessary quotes]

> This patch seems to have deferred. We basically want a way to download
> a vendor specific configuration to our firmware. Do you have any
> suggestions on how can achieve this in better way?
>
> I can see below iw command suits our requirement.
>
> iw dev <devname> vendor send <oui> <subcmd> <filename|-|hex data>
>
> Please guide.

It was deferred because use of the nl80211 vendor interface (which I
don't think belong to upstream drivers). I'll take a look at this patch
in detail after the merge window.

Amitkumar Karwar May 24, 2016, 7:18 a.m. UTC | #3

> From: Kalle Valo [mailto:[e-mail]]
> Sent: Tuesday, May 24, 2016 1:53 AM
> To: Amitkumar Karwar
> Cc: [e-mail]
> Subject: Re: [PATCH] mwifiex: handle edmac vendor command
> 
> Amitkumar Karwar <[e-mail]> writes:
> 
> > Hi Kalle,
> >
> >> From: Amitkumar Karwar [mailto:[e-mail]]
> >> Sent: Friday, April 29, 2016 9:28 PM
> >> To: [e-mail]
> >> Cc: Jeff CF Chen; Amitkumar Karwar
> >> Subject: [PATCH] mwifiex: handle edmac vendor command
> >>
> >> From: chunfan chen <[e-mail]>
> >>
> >> Userspace can configure edmac values through a custom vendor command.
> >> They will be used by hardware for adaptivity.
> >>
> >> Signed-off-by: chunfan chen <[e-mail]>
> >> Signed-off-by: Amitkumar Karwar <[e-mail]>
> 
> [deleted over 200 lines of unnecessary quotes]
> 
> > This patch seems to have deferred. We basically want a way to download
> > a vendor specific configuration to our firmware. Do you have any
> > suggestions on how can achieve this in better way?
> >
> > I can see below iw command suits our requirement.
> >
> > iw dev <devname> vendor send <oui> <subcmd> <filename|-|hex data>
> >
> > Please guide.
> 
> It was deferred because use of the nl80211 vendor interface (which I
> don't think belong to upstream drivers). I'll take a look at this patch
> in detail after the merge window.

Ok. Thanks for your reply.
We will work to improve the patch if you have any suggestions.

Regards,
Amitkumar
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Amitkumar Karwar Aug. 3, 2016, 11:20 a.m. UTC | #4

Hi Kalle,

> From: Kalle Valo [mailto:[e-mail]]
> Sent: Tuesday, May 24, 2016 1:53 AM
> To: Amitkumar Karwar
> Cc: [e-mail]
> Subject: Re: [PATCH] mwifiex: handle edmac vendor command
> 
> Amitkumar Karwar <[e-mail]> writes:
> 
> > Hi Kalle,
> >
> >> From: Amitkumar Karwar [mailto:[e-mail]]
> >> Sent: Friday, April 29, 2016 9:28 PM
> >> To: [e-mail]
> >> Cc: Jeff CF Chen; Amitkumar Karwar
> >> Subject: [PATCH] mwifiex: handle edmac vendor command
> >>
> >> From: chunfan chen <[e-mail]>
> >>
> >> Userspace can configure edmac values through a custom vendor command.
> >> They will be used by hardware for adaptivity.
> >>
> >> Signed-off-by: chunfan chen <[e-mail]>
> >> Signed-off-by: Amitkumar Karwar <[e-mail]>
> 
> [deleted over 200 lines of unnecessary quotes]
> 
> > This patch seems to have deferred. We basically want a way to download
> > a vendor specific configuration to our firmware. Do you have any
> > suggestions on how can achieve this in better way?
> >
> > I can see below iw command suits our requirement.
> >
> > iw dev <devname> vendor send <oui> <subcmd> <filename|-|hex data>
> >
> > Please guide.
> 
> It was deferred because use of the nl80211 vendor interface (which I
> don't think belong to upstream drivers). I'll take a look at this patch
> in detail after the merge window.
> 

Did you get a chance to check this patch?
Please let me know if you have any suggestions.

Regards,
Amitkumar
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Amitkumar Karwar Aug. 31, 2016, 11:13 a.m. UTC | #5

> From: [e-mail] [mailto:linux-wireless-
> [e-mail]] On Behalf Of Amitkumar Karwar
> Sent: Wednesday, August 03, 2016 4:51 PM
> To: Kalle Valo
> Cc: [e-mail]
> Subject: RE: [PATCH] mwifiex: handle edmac vendor command
> 
> Hi Kalle,
> 
> > From: Kalle Valo [mailto:[e-mail]]
> > Sent: Tuesday, May 24, 2016 1:53 AM
> > To: Amitkumar Karwar
> > Cc: [e-mail]
> > Subject: Re: [PATCH] mwifiex: handle edmac vendor command
> >
> > Amitkumar Karwar <[e-mail]> writes:
> >
> > > Hi Kalle,
> > >
> > >> From: Amitkumar Karwar [mailto:[e-mail]]
> > >> Sent: Friday, April 29, 2016 9:28 PM
> > >> To: [e-mail]
> > >> Cc: Jeff CF Chen; Amitkumar Karwar
> > >> Subject: [PATCH] mwifiex: handle edmac vendor command
> > >>
> > >> From: chunfan chen <[e-mail]>
> > >>
> > >> Userspace can configure edmac values through a custom vendor
> command.
> > >> They will be used by hardware for adaptivity.
> > >>
> > >> Signed-off-by: chunfan chen <[e-mail]>
> > >> Signed-off-by: Amitkumar Karwar <[e-mail]>
> >
> > [deleted over 200 lines of unnecessary quotes]
> >
> > > This patch seems to have deferred. We basically want a way to
> > > download a vendor specific configuration to our firmware. Do you
> > > have any suggestions on how can achieve this in better way?
> > >
> > > I can see below iw command suits our requirement.
> > >
> > > iw dev <devname> vendor send <oui> <subcmd> <filename|-|hex data>
> > >
> > > Please guide.
> >
> > It was deferred because use of the nl80211 vendor interface (which I
> > don't think belong to upstream drivers). I'll take a look at this
> > patch in detail after the merge window.
> >
> 
> Did you get a chance to check this patch?
> Please let me know if you have any suggestions.
> 

Any suggestions for a mechanism to download vendor specific command to firmware?

Regards,
Amitkumar Karwar

Kalle Valo Sept. 1, 2016, 2:53 p.m. UTC | #6

Amitkumar Karwar <[e-mail]> writes:

>> From: Kalle Valo [mailto:[e-mail]]
>> Sent: Tuesday, May 24, 2016 1:53 AM
>> To: Amitkumar Karwar
>> Cc: [e-mail]
>> Subject: Re: [PATCH] mwifiex: handle edmac vendor command
>> 
>> Amitkumar Karwar <[e-mail]> writes:
>> 
>> > Hi Kalle,
>> >
>> >> From: Amitkumar Karwar [mailto:[e-mail]]
>> >> Sent: Friday, April 29, 2016 9:28 PM
>> >> To: [e-mail]
>> >> Cc: Jeff CF Chen; Amitkumar Karwar
>> >> Subject: [PATCH] mwifiex: handle edmac vendor command
>> >>
>> >> From: chunfan chen <[e-mail]>
>> >>
>> >> Userspace can configure edmac values through a custom vendor command.
>> >> They will be used by hardware for adaptivity.
>> >>
>> >> Signed-off-by: chunfan chen <[e-mail]>
>> >> Signed-off-by: Amitkumar Karwar <[e-mail]>
>> 
>> [deleted over 200 lines of unnecessary quotes]
>> 
>> > This patch seems to have deferred. We basically want a way to download
>> > a vendor specific configuration to our firmware. Do you have any
>> > suggestions on how can achieve this in better way?
>> >
>> > I can see below iw command suits our requirement.
>> >
>> > iw dev <devname> vendor send <oui> <subcmd> <filename|-|hex data>
>> >
>> > Please guide.
>> 
>> It was deferred because use of the nl80211 vendor interface (which I
>> don't think belong to upstream drivers). I'll take a look at this patch
>> in detail after the merge window.
>> 
>
> Did you get a chance to check this patch?
> Please let me know if you have any suggestions.

Sorry for the delay. I have been thinking about vendor commands quite a
lot and I don't think they belong to upstream drivers. For regular use
cases (used by normal users) we have nl80211, for developer and testing
purposes we have debugfs and for manufacturing type of tests we have
nl80211 testmode interface. The focus of development should be adding
new functionality to nl80211 (and other generic interfaces), not to
driver specific vendor commands.

I know brcm80211 and ti's wlcore have few vendor commands but I'm hoping
to remove them sometime soon.

Thoughts?

Amitkumar Karwar Sept. 1, 2016, 4:34 p.m. UTC | #7

Hi Kalle,

> From: Kalle Valo [mailto:[e-mail]]
> Sent: Thursday, September 01, 2016 8:23 PM
> To: Amitkumar Karwar
> Cc: [e-mail]
> Subject: Re: [PATCH] mwifiex: handle edmac vendor command
> 
> Amitkumar Karwar <[e-mail]> writes:
> 
> >> From: Kalle Valo [mailto:[e-mail]]
> >> Sent: Tuesday, May 24, 2016 1:53 AM
> >> To: Amitkumar Karwar
> >> Cc: [e-mail]
> >> Subject: Re: [PATCH] mwifiex: handle edmac vendor command
> >>
> >> Amitkumar Karwar <[e-mail]> writes:
> >>
> >> > Hi Kalle,
> >> >
> >> >> From: Amitkumar Karwar [mailto:[e-mail]]
> >> >> Sent: Friday, April 29, 2016 9:28 PM
> >> >> To: [e-mail]
> >> >> Cc: Jeff CF Chen; Amitkumar Karwar
> >> >> Subject: [PATCH] mwifiex: handle edmac vendor command
> >> >>
> >> >> From: chunfan chen <[e-mail]>
> >> >>
> >> >> Userspace can configure edmac values through a custom vendor
> command.
> >> >> They will be used by hardware for adaptivity.
> >> >>
> >> >> Signed-off-by: chunfan chen <[e-mail]>
> >> >> Signed-off-by: Amitkumar Karwar <[e-mail]>
> >>
> >> [deleted over 200 lines of unnecessary quotes]
> >>
> >> > This patch seems to have deferred. We basically want a way to
> >> > download a vendor specific configuration to our firmware. Do you
> >> > have any suggestions on how can achieve this in better way?
> >> >
> >> > I can see below iw command suits our requirement.
> >> >
> >> > iw dev <devname> vendor send <oui> <subcmd> <filename|-|hex data>
> >> >
> >> > Please guide.
> >>
> >> It was deferred because use of the nl80211 vendor interface (which I
> >> don't think belong to upstream drivers). I'll take a look at this
> >> patch in detail after the merge window.
> >>
> >
> > Did you get a chance to check this patch?
> > Please let me know if you have any suggestions.
> 
> Sorry for the delay. I have been thinking about vendor commands quite a
> lot and I don't think they belong to upstream drivers. For regular use
> cases (used by normal users) we have nl80211, for developer and testing
> purposes we have debugfs and for manufacturing type of tests we have
> nl80211 testmode interface. The focus of development should be adding
> new functionality to nl80211 (and other generic interfaces), not to
> driver specific vendor commands.
> 
> I know brcm80211 and ti's wlcore have few vendor commands but I'm hoping
> to remove them sometime soon.
> 
> Thoughts?
> 

Thanks for your reply.

There is something called energy detect mode. Chip can detect non-WiFi radio signal also and monitor it for specified time before transmitting frames.
As per ETSI specification, enabling this mode is mandatory for some countries for certain frequencies.

The purpose of this patch is to configure the chip for working in this mode.
I can see cfg80211 has a framework to handle vendor specific commands and events. I think having vendor commands would be helpful to support vendor specific functionalities which can't be generalized.

I suppose debugfs interface is only for collecting stats and info which can be used for debugging purpose.

Regards,
Amitkumar

Kalle Valo Sept. 2, 2016, 2:56 p.m. UTC | #8

Amitkumar Karwar <[e-mail]> writes:

>> Sorry for the delay. I have been thinking about vendor commands quite a
>> lot and I don't think they belong to upstream drivers. For regular use
>> cases (used by normal users) we have nl80211, for developer and testing
>> purposes we have debugfs and for manufacturing type of tests we have
>> nl80211 testmode interface. The focus of development should be adding
>> new functionality to nl80211 (and other generic interfaces), not to
>> driver specific vendor commands.
>> 
>> I know brcm80211 and ti's wlcore have few vendor commands but I'm hoping
>> to remove them sometime soon.
>> 
>> Thoughts?
>> 
>
> Thanks for your reply.
>
> There is something called energy detect mode. Chip can detect non-WiFi
> radio signal also and monitor it for specified time before
> transmitting frames. As per ETSI specification, enabling this mode is
> mandatory for some countries for certain frequencies.

To me this looks this is something which can be generic, not a driver
specific interface. And why can't regulatory code enable this
automatically, without any user involvement? It already knows what
country we are in.

> The purpose of this patch is to configure the chip for working in this
> mode. I can see cfg80211 has a framework to handle vendor specific
> commands and events. I think having vendor commands would be helpful
> to support vendor specific functionalities which can't be generalized.

First of all, I don't see how the interface is mwifiex specific. If I
could trust that using vendor commands will not slow down the
development of nl80211, and other generic interfaces, I might think
otherwise. But at the moment I am nowhere near that.

> I suppose debugfs interface is only for collecting stats and info
> which can be used for debugging purpose.

Correct. I consider debugfs as a development and debugging interface for
developers and advanced users.

Arend van Spriel Sept. 5, 2016, 8:33 a.m. UTC | #9

On 1-9-2016 16:53, Kalle Valo wrote:
> Amitkumar Karwar <[e-mail]> writes:
> 
>>> From: Kalle Valo [mailto:[e-mail]]
>>> Sent: Tuesday, May 24, 2016 1:53 AM
>>> To: Amitkumar Karwar
>>> Cc: [e-mail]
>>> Subject: Re: [PATCH] mwifiex: handle edmac vendor command
>>>
>>> Amitkumar Karwar <[e-mail]> writes:
>>>
>>>> Hi Kalle,
>>>>
>>>>> From: Amitkumar Karwar [mailto:[e-mail]]
>>>>> Sent: Friday, April 29, 2016 9:28 PM
>>>>> To: [e-mail]
>>>>> Cc: Jeff CF Chen; Amitkumar Karwar
>>>>> Subject: [PATCH] mwifiex: handle edmac vendor command
>>>>>
>>>>> From: chunfan chen <[e-mail]>
>>>>>
>>>>> Userspace can configure edmac values through a custom vendor command.
>>>>> They will be used by hardware for adaptivity.
>>>>>
>>>>> Signed-off-by: chunfan chen <[e-mail]>
>>>>> Signed-off-by: Amitkumar Karwar <[e-mail]>
>>>
>>> [deleted over 200 lines of unnecessary quotes]
>>>
>>>> This patch seems to have deferred. We basically want a way to download
>>>> a vendor specific configuration to our firmware. Do you have any
>>>> suggestions on how can achieve this in better way?
>>>>
>>>> I can see below iw command suits our requirement.
>>>>
>>>> iw dev <devname> vendor send <oui> <subcmd> <filename|-|hex data>
>>>>
>>>> Please guide.
>>>
>>> It was deferred because use of the nl80211 vendor interface (which I
>>> don't think belong to upstream drivers). I'll take a look at this patch
>>> in detail after the merge window.
>>>
>>
>> Did you get a chance to check this patch?
>> Please let me know if you have any suggestions.
> 
> Sorry for the delay. I have been thinking about vendor commands quite a
> lot and I don't think they belong to upstream drivers. For regular use
> cases (used by normal users) we have nl80211, for developer and testing
> purposes we have debugfs and for manufacturing type of tests we have
> nl80211 testmode interface. The focus of development should be adding
> new functionality to nl80211 (and other generic interfaces), not to
> driver specific vendor commands.
> 
> I know brcm80211 and ti's wlcore have few vendor commands but I'm hoping
> to remove them sometime soon.
> 
> Thoughts?

Grabbing the original commit message for vendor commands:

commit ad7e718c9b4f717823fd920a0103f7b0fb06183f
Author: Johannes Berg <[e-mail]>
Date:   Wed Nov 13 13:37:47 2013 +0100

    nl80211: vendor command support

    Add support for vendor-specific commands to nl80211. This is
    intended to be used for really vendor-specific functionality
    that can't be implemented in a generic fashion for any reason.
    It's *NOT* intended to be used for any normal/generic feature
    or any optimisations that could be implemented across drivers.

I agree that the effort needs to be made to come up with a solution that
is usable by (most of) all drivers in the upstream wireless subsystem.

The thing with the test mode is that you need to enable it in Kconfig.
We have 1 vendor command in brcmfmac and it used to be under testmode
api. However, because of the Kconfig thing it would mean we need a
different kernel to do manufacturing testing. Especially when the
modules are built-in. The OEMs typically don't like that as they want to
do the testing using the same kernel as what ends up in the product.
This was our motivation to use the vendor command when that was added.

It does have a nasty side-effect that some will see it as a drop-in
replacement of the driver private ioctl-s. To some extent (as mentioned
in the commit message above) that should be fine, but we need to make a
good effort to move things to nl80211.

Regards,
Arend

Amitkumar Karwar Sept. 7, 2016, 10:12 a.m. UTC | #10

Hi Kalle,

> From: Kalle Valo [mailto:[e-mail]]
> Sent: Friday, September 02, 2016 8:26 PM
> To: Amitkumar Karwar
> Cc: [e-mail]
> Subject: Re: [PATCH] mwifiex: handle edmac vendor command
> 
> Amitkumar Karwar <[e-mail]> writes:
> 
> >> Sorry for the delay. I have been thinking about vendor commands quite
> >> a lot and I don't think they belong to upstream drivers. For regular
> >> use cases (used by normal users) we have nl80211, for developer and
> >> testing purposes we have debugfs and for manufacturing type of tests
> >> we have
> >> nl80211 testmode interface. The focus of development should be adding
> >> new functionality to nl80211 (and other generic interfaces), not to
> >> driver specific vendor commands.
> >>
> >> I know brcm80211 and ti's wlcore have few vendor commands but I'm
> >> hoping to remove them sometime soon.
> >>
> >> Thoughts?
> >>
> >
> > Thanks for your reply.
> >
> > There is something called energy detect mode. Chip can detect non-WiFi
> > radio signal also and monitor it for specified time before
> > transmitting frames. As per ETSI specification, enabling this mode is
> > mandatory for some countries for certain frequencies.
> 
> To me this looks this is something which can be generic, not a driver
> specific interface. And why can't regulatory code enable this
> automatically, without any user involvement? It already knows what
> country we are in.
> 

Got it. I will check if we can make this as generic as you mentioned and explore the option.

Regards,
Amitkumar

Kalle Valo Sept. 7, 2016, 10:59 a.m. UTC | #11

Amitkumar Karwar <[e-mail]> writes:

>> > There is something called energy detect mode. Chip can detect non-WiFi
>> > radio signal also and monitor it for specified time before
>> > transmitting frames. As per ETSI specification, enabling this mode is
>> > mandatory for some countries for certain frequencies.
>> 
>> To me this looks this is something which can be generic, not a driver
>> specific interface. And why can't regulatory code enable this
>> automatically, without any user involvement? It already knows what
>> country we are in.
>
> Got it. I will check if we can make this as generic as you mentioned
> and explore the option.

Thanks. Please let us know how it goes.

Kalle Valo Sept. 7, 2016, 11:27 a.m. UTC | #12

Arend Van Spriel <[e-mail]> writes:

> On 1-9-2016 16:53, Kalle Valo wrote:
>
>> Sorry for the delay. I have been thinking about vendor commands quite a
>> lot and I don't think they belong to upstream drivers. For regular use
>> cases (used by normal users) we have nl80211, for developer and testing
>> purposes we have debugfs and for manufacturing type of tests we have
>> nl80211 testmode interface. The focus of development should be adding
>> new functionality to nl80211 (and other generic interfaces), not to
>> driver specific vendor commands.
>> 
>> I know brcm80211 and ti's wlcore have few vendor commands but I'm hoping
>> to remove them sometime soon.
>> 
>> Thoughts?
>
> Grabbing the original commit message for vendor commands:
>
> commit ad7e718c9b4f717823fd920a0103f7b0fb06183f
> Author: Johannes Berg <[e-mail]>
> Date:   Wed Nov 13 13:37:47 2013 +0100
>
>     nl80211: vendor command support
>
>     Add support for vendor-specific commands to nl80211. This is
>     intended to be used for really vendor-specific functionality
>     that can't be implemented in a generic fashion for any reason.
>     It's *NOT* intended to be used for any normal/generic feature
>     or any optimisations that could be implemented across drivers.
>
> I agree that the effort needs to be made to come up with a solution that
> is usable by (most of) all drivers in the upstream wireless subsystem.
>
> The thing with the test mode is that you need to enable it in Kconfig.

And that's on purpose. We did not want testmode to be enabled on distro
kernels, for example.

> We have 1 vendor command in brcmfmac and it used to be under testmode
> api. However, because of the Kconfig thing it would mean we need a
> different kernel to do manufacturing testing. Especially when the
> modules are built-in. The OEMs typically don't like that as they want to
> do the testing using the same kernel as what ends up in the product.
> This was our motivation to use the vendor command when that was added.

Is there a specific reason why you can't keep the testmode always
enabled? Then you would have the same kernel both in manufacturing and
in "normal mode".

> It does have a nasty side-effect that some will see it as a drop-in
> replacement of the driver private ioctl-s. To some extent (as mentioned
> in the commit message above) that should be fine, but we need to make a
> good effort to move things to nl80211.

Sorry, but I'm way too cynical to buy that :) A "good effort" means that
in most of cases it will never happen. Once the vendor command is
accepted into upsteam trees the developer will lose any interest on
trying to make it a generic interface. That's just how humans work,
including me.

I do see your and Amitkumar's point how much easier the vendor commands
make it to implement new driver features and part of me also wants that.
I just think that in the long run that's detrimental for Linux wireless
and we end up into having multiple drivers having same features enabled
via different vendor commands.

Patch

diff --git a/drivers/net/wireless/marvell/mwifiex/Makefile b/drivers/net/wireless/marvell/mwifiex/Makefile
index fdfd9bf..8b34ce9 100644
--- a/drivers/net/wireless/marvell/mwifiex/Makefile
+++ b/drivers/net/wireless/marvell/mwifiex/Makefile
@@ -42,6 +42,7 @@  mwifiex-y += cfg80211.o
 mwifiex-y += ethtool.o
 mwifiex-y += 11h.o
 mwifiex-y += tdls.o
+mwifiex-y += vendor.o
 mwifiex-$(CONFIG_DEBUG_FS) += debugfs.o
 obj-$(CONFIG_MWIFIEX) += mwifiex.o
 
diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index ff948a9..00aca7e 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -4118,6 +4118,8 @@  int mwifiex_register_cfg80211(struct mwifiex_adapter *adapter)
 	if (adapter->fw_api_ver == MWIFIEX_FW_V15)
 		wiphy->features |= NL80211_FEATURE_SK_TX_STATUS;
 
+	marvell_set_vendor_commands(wiphy);
+
 	/* Reserve space for mwifiex specific private data for BSS */
 	wiphy->bss_priv_size = sizeof(struct mwifiex_bss_priv);
 
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 0207af0..66ba5c0 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -1002,6 +1002,8 @@  struct mwifiex_adapter {
 	bool usb_mc_status;
 	bool usb_mc_setup;
 	struct cfg80211_wowlan_nd_info *nd_info;
+	u8 *cfg_data;
+	int cfg_len;
 };
 
 void mwifiex_process_tx_queue(struct mwifiex_adapter *adapter);
@@ -1614,6 +1616,8 @@  void mwifiex_process_multi_chan_event(struct mwifiex_private *priv,
 				      struct sk_buff *event_skb);
 void mwifiex_multi_chan_resync(struct mwifiex_adapter *adapter);
 
+void marvell_set_vendor_commands(struct wiphy *wiphy);
+
 #ifdef CONFIG_DEBUG_FS
 void mwifiex_debugfs_init(void);
 void mwifiex_debugfs_remove(void);
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
index e436574..6b8cc39 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
@@ -1487,9 +1487,10 @@  static int mwifiex_cmd_cfg_data(struct mwifiex_private *priv,
 {
 	struct mwifiex_adapter *adapter = priv->adapter;
 	struct property *prop = data_buf;
-	u32 len;
+	u32 len = 0;
 	u8 *data = (u8 *)cmd + S_DS_GEN;
 	int ret;
+	const struct firmware *cal_data = adapter->cal_data;
 
 	if (prop) {
 		len = prop->length;
@@ -1500,11 +1501,20 @@  static int mwifiex_cmd_cfg_data(struct mwifiex_private *priv,
 		mwifiex_dbg(adapter, INFO,
 			    "download cfg_data from device tree: %s\n",
 			    prop->name);
-	} else if (adapter->cal_data->data && adapter->cal_data->size > 0) {
-		len = mwifiex_parse_cal_cfg((u8 *)adapter->cal_data->data,
-					    adapter->cal_data->size, data);
+	} else if (cal_data) {
+		if (cal_data->data && cal_data->size > 0) {
+			len = mwifiex_parse_cal_cfg((u8 *)cal_data->data,
+						    cal_data->size, data);
+			mwifiex_dbg(adapter, INFO,
+				    "download cfg_data from config file\n");
+		} else {
+			return -1;
+		}
+	} else if (adapter->cfg_data && adapter->cfg_len > 0) {
+		len = mwifiex_parse_cal_cfg(adapter->cfg_data,
+					    adapter->cfg_len, data);
 		mwifiex_dbg(adapter, INFO,
-			    "download cfg_data from config file\n");
+			    "download cfg_data from iw vendor command\n");
 	} else {
 		return -1;
 	}
diff --git a/drivers/net/wireless/marvell/mwifiex/vendor.c b/drivers/net/wireless/marvell/mwifiex/vendor.c
new file mode 100644
index 0000000..ecdc10c
--- /dev/null
+++ b/drivers/net/wireless/marvell/mwifiex/vendor.c
@@ -0,0 +1,59 @@ 
+/* Marvell Wireless LAN device driver: TDLS handling
+ *
+ * Copyright (C) 2014, Marvell International Ltd.
+ *
+ * This software file (the "File") is distributed by Marvell International
+ * Ltd. under the terms of the GNU General Public License Version 2, June 1991
+ * (the "License").  You may use, redistribute and/or modify this File in
+ * accordance with the terms and conditions of the License, a copy of which
+ * is available on the worldwide web at
+ * http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt.
+ *
+ * THE FILE IS DISTRIBUTED AS-IS, WITHOUT WARRANTY OF ANY KIND, AND THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE
+ * ARE EXPRESSLY DISCLAIMED.  The License provides additional details about
+ * this warranty disclaimer.
+ */
+
+#include <net/mac80211.h>
+#include <net/netlink.h>
+#include "vendor.h"
+#include "main.h"
+
+static int
+mwifiex_vendor_cmd_set_edmac_data(struct wiphy *wiphy,
+				  struct wireless_dev *wdev,
+				  const void *data, int data_len)
+{
+	struct mwifiex_private *priv = mwifiex_netdev_get_priv(wdev->netdev);
+	int ret;
+
+	priv->adapter->cfg_data = (u8 *)data;
+	priv->adapter->cfg_len = data_len;
+
+	ret = mwifiex_send_cmd(priv, HostCmd_CMD_CFG_DATA,
+			       HostCmd_ACT_GEN_SET, 0, NULL, true);
+
+	priv->adapter->cfg_data = NULL;
+	priv->adapter->cfg_len = 0;
+
+	return 0;
+}
+
+static const struct wiphy_vendor_command marvell_vendor_commands[] = {
+	{
+		.info = {
+			.vendor_id = MARVELL_OUI,
+			.subcmd = MARVELL_VENDOR_CMD_SET_EDMAC_DATA,
+		},
+		.flags = WIPHY_VENDOR_CMD_NEED_NETDEV |
+			 WIPHY_VENDOR_CMD_NEED_RUNNING,
+		.doit = mwifiex_vendor_cmd_set_edmac_data,
+	},
+};
+
+void marvell_set_vendor_commands(struct wiphy *wiphy)
+{
+	wiphy->vendor_commands = marvell_vendor_commands;
+	wiphy->n_vendor_commands = ARRAY_SIZE(marvell_vendor_commands);
+}
diff --git a/drivers/net/wireless/marvell/mwifiex/vendor.h b/drivers/net/wireless/marvell/mwifiex/vendor.h
new file mode 100644
index 0000000..e5d6fe8
--- /dev/null
+++ b/drivers/net/wireless/marvell/mwifiex/vendor.h
@@ -0,0 +1,27 @@ 
+/* Marvell Wireless LAN device driver: TDLS handling
+ *
+ * Copyright (C) 2014, Marvell International Ltd.
+ *
+ * This software file (the "File") is distributed by Marvell International
+ * Ltd. under the terms of the GNU General Public License Version 2, June 1991
+ * (the "License").  You may use, redistribute and/or modify this File in
+ * accordance with the terms and conditions of the License, a copy of which
+ * is available on the worldwide web at
+ * http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt.
+ *
+ * THE FILE IS DISTRIBUTED AS-IS, WITHOUT WARRANTY OF ANY KIND, AND THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE
+ * ARE EXPRESSLY DISCLAIMED.  The License provides additional details about
+ * this warranty disclaimer.
+ */
+
+#ifndef __MARVELL_VENDOR_H__
+#define __MARVELL_VENDOR_H__
+
+#define MARVELL_OUI	0x005043
+
+enum marvell_vendor_commands {
+	MARVELL_VENDOR_CMD_SET_EDMAC_DATA,
+};
+
+#endif /* __MARVELL_VENDOR_H__ */