| Message ID | 1466756034-5485-1-git-send-email-erik.stromdahl@gmail.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Jun 24, 2016 at 10:13:54AM +0200, Erik Stromdahl wrote: > The purpose of the echo command is to provide a test > facility for user space programs. > diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c > @@ -332,14 +332,16 @@ static const struct ieee80211_rate hwsim_rates[] = { > #define QCA_NL80211_SUBCMD_TEST 1 > +#define QCA_NL80211_SUBCMD_ECHO 2 NAK. That QCA vendor specific value has not been assigned nor have I even seen a request to assign such a value. > enum qca_nl80211_vendor_subcmds { > QCA_WLAN_VENDOR_ATTR_TEST = 8, > - QCA_WLAN_VENDOR_ATTR_MAX = QCA_WLAN_VENDOR_ATTR_TEST > + QCA_WLAN_VENDOR_ATTR_ECHO, And this vendor attribute value has already been assigned for another purpose. It is no acceptable to pick arbitrary values of identifiers without proper request to the owner of the OUI that is used to assign the values. As far as the QCA vendor specific identifiers are concerned, their assignment process is described here: http://w1.fi/cgit/hostap/plain/src/common/qca-vendor.h Please do not submit kernel changes that use any unassigned vendor identifier or even worse, a value that have already been assigned for a completely different purpose.
The mac80211_hwsim driver is currently defining the QCA vendor ID 0x001374 for some reason. Also, all current vendor commands are using the "QCA_" prefix, hence the reason why I decided to add the new vendor command with the "QCA_" prefix as well. Anyway, I understand that you don't want the hwsim driver bloated with definitions that does not belong there. Is there any reason why hwsim uses the QCA vendor id? Perhaps it would be best if the hwsim driver would define its own vendor id? On 06/24/2016 10:47 PM, Jouni Malinen wrote: > On Fri, Jun 24, 2016 at 10:13:54AM +0200, Erik Stromdahl wrote: >> The purpose of the echo command is to provide a test >> facility for user space programs. > >> diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c >> @@ -332,14 +332,16 @@ static const struct ieee80211_rate hwsim_rates[] = { >> #define QCA_NL80211_SUBCMD_TEST 1 >> +#define QCA_NL80211_SUBCMD_ECHO 2 > > NAK. That QCA vendor specific value has not been assigned nor have I > even seen a request to assign such a value. > >> enum qca_nl80211_vendor_subcmds { >> QCA_WLAN_VENDOR_ATTR_TEST = 8, >> - QCA_WLAN_VENDOR_ATTR_MAX = QCA_WLAN_VENDOR_ATTR_TEST >> + QCA_WLAN_VENDOR_ATTR_ECHO, > > And this vendor attribute value has already been assigned for another > purpose. > > It is no acceptable to pick arbitrary values of identifiers without > proper request to the owner of the OUI that is used to assign the > values. > > As far as the QCA vendor specific identifiers are concerned, their > assignment process is described here: > http://w1.fi/cgit/hostap/plain/src/common/qca-vendor.h > > Please do not submit kernel changes that use any unassigned vendor > identifier or even worse, a value that have already been assigned for a > completely different purpose. > >
On Sat, Jun 25, 2016 at 06:08:01PM +0200, Erik Stromdahl wrote: > The mac80211_hwsim driver is currently defining the QCA vendor ID > 0x001374 for some reason. That is used for allowing nl80211 vendor specific commands and events to be tested. The particular vendor command was properly assigned from the QCA 00:13:74 OUI. > Also, all current vendor commands are using the "QCA_" prefix, hence > the reason why I decided to add the new vendor command with the > "QCA_" prefix as well. > > Anyway, I understand that you don't want the hwsim driver bloated > with definitions that does not belong there. That's not the issue here; the issue is in getting vendor subcommands assigned by the entity who controls the OUI that is used. > Is there any reason why hwsim uses the QCA vendor id? Yes, that's an OUI for which there is a process that allows new values to be assigned.. I happen to be the one who is in control of assigning such values. > Perhaps it would be best if the hwsim driver would define its own vendor id? That would need an OUI which are a bit expensive to acquire. I don't see any point for mac80211_hwsim to need a different OUI since there is already a friendly vendor who has provided an OUI that can be used for this type of purposes. In other words, you just need to follow the process defined for assigning unique vendor subcommands/attribute identifiers for this OUI 00:13:74 and that process is what I pointed out in the previous email: > >As far as the QCA vendor specific identifiers are concerned, their > >assignment process is described here: > >http://w1.fi/cgit/hostap/plain/src/common/qca-vendor.h All you need to do is to prepare a hostap.git contribution that requests a new subcommand/attribute to be assigned and once that gets applied to the hostap.git master branch, the values have been assigned and can be used for whatever purpose they were assigned, e.g., in mac80211_hwsim. More details on how to make such a hostap.git contribution: http://w1.fi/cgit/hostap/plain/CONTRIBUTIONS
On Sat, 2016-06-25 at 21:32 +0300, Jouni Malinen wrote: > > All you need to do is to prepare a hostap.git contribution that > requests a new subcommand/attribute to be assigned and once that gets > applied to the hostap.git master branch, the values have been > assigned and can be used for whatever purpose they were assigned, > e.g., in mac80211_hwsim. > Assuming, of course, that there's actually a point in the original patch. I haven't quite figured out any reason to actually have this facility. johannes
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c index 4dd5adc..2e17bf1 100644 --- a/drivers/net/wireless/mac80211_hwsim.c +++ b/drivers/net/wireless/mac80211_hwsim.c @@ -332,14 +332,16 @@ static const struct ieee80211_rate hwsim_rates[] = { #define OUI_QCA 0x001374 #define QCA_NL80211_SUBCMD_TEST 1 +#define QCA_NL80211_SUBCMD_ECHO 2 enum qca_nl80211_vendor_subcmds { QCA_WLAN_VENDOR_ATTR_TEST = 8, - QCA_WLAN_VENDOR_ATTR_MAX = QCA_WLAN_VENDOR_ATTR_TEST + QCA_WLAN_VENDOR_ATTR_ECHO, + QCA_WLAN_VENDOR_ATTR_MAX = QCA_WLAN_VENDOR_ATTR_ECHO }; static const struct nla_policy hwsim_vendor_test_policy[QCA_WLAN_VENDOR_ATTR_MAX + 1] = { - [QCA_WLAN_VENDOR_ATTR_MAX] = { .type = NLA_U32 }, + [QCA_WLAN_VENDOR_ATTR_TEST] = { .type = NLA_U32 }, }; static int mac80211_hwsim_vendor_cmd_test(struct wiphy *wiphy, @@ -393,12 +395,33 @@ static int mac80211_hwsim_vendor_cmd_test(struct wiphy *wiphy, return cfg80211_vendor_cmd_reply(skb); } +static int mac80211_hwsim_vendor_cmd_echo(struct wiphy *wiphy, + struct wireless_dev *wdev, + const void *data, int data_len) +{ + struct sk_buff *skb; + + skb = cfg80211_vendor_cmd_alloc_reply_skb(wiphy, data_len); + if (!skb) + return -ENOMEM; + + nla_put(skb, QCA_WLAN_VENDOR_ATTR_ECHO, data_len, data); + + return cfg80211_vendor_cmd_reply(skb); +} + static struct wiphy_vendor_command mac80211_hwsim_vendor_commands[] = { { .info = { .vendor_id = OUI_QCA, .subcmd = QCA_NL80211_SUBCMD_TEST }, .flags = WIPHY_VENDOR_CMD_NEED_NETDEV, .doit = mac80211_hwsim_vendor_cmd_test, + }, + { + .info = { .vendor_id = OUI_QCA, + .subcmd = QCA_NL80211_SUBCMD_ECHO }, + .flags = WIPHY_VENDOR_CMD_NEED_NETDEV, + .doit = mac80211_hwsim_vendor_cmd_echo, } };
The purpose of the echo command is to provide a test facility for user space programs. Signed-off-by: Erik Stromdahl <erik.stromdahl@gmail.com> --- drivers/net/wireless/mac80211_hwsim.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)