| Message ID | 20181009174829.15163-1-james.prestwood@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | mac80211_hwsim: allow setting custom features/iftypes | expand |
On Tue, 2018-10-09 at 10:48 -0700, James Prestwood wrote: > The current driver hard codes various features, ext features and supported > interface types when a new radio is created. This patch adds several new > hwsim attributes so user space can specify specific features the radio should > have: > > - HWSIM_ATTR_FEATURE_SUPPORT > Bit field of nl80211_feature_flags flags > - HWSIM_ATTR_EXT_FEATURE_SUPPORT > Variable length bit field where bits map to nl80211_ext_feature_index > values. > - HWSIM_ATTR_IFTYPE_SUPPORT > Nested attribute of flags containing all supported interface types. > Each flag attribute sets support for an interface type. This seems rather wrong, most (extended) features require the driver/device to actually *do* something. Let's say you enable NL80211_EXT_FEATURE_TXQS - but then nothing actually happens when you try to configure those. Or let's say you enable NL80211_FEATURE_TX_POWER_INSERTION but then nothing actually happens when sending the frame... johannes
On Tue, 2018-10-09 at 21:41 +0200, Johannes Berg wrote: > On Tue, 2018-10-09 at 10:48 -0700, James Prestwood wrote: > > The current driver hard codes various features, ext features and > > supported > > interface types when a new radio is created. This patch adds > > several new > > hwsim attributes so user space can specify specific features the > > radio should > > have: > > > > - HWSIM_ATTR_FEATURE_SUPPORT > > Bit field of nl80211_feature_flags flags > > - HWSIM_ATTR_EXT_FEATURE_SUPPORT > > Variable length bit field where bits map to > > nl80211_ext_feature_index > > values. > > - HWSIM_ATTR_IFTYPE_SUPPORT > > Nested attribute of flags containing all supported interface > > types. > > Each flag attribute sets support for an interface type. > > This seems rather wrong, most (extended) features require the > driver/device to actually *do* something. > > Let's say you enable NL80211_EXT_FEATURE_TXQS - but then nothing > actually happens when you try to configure those. Or let's say you > enable NL80211_FEATURE_TX_POWER_INSERTION but then nothing actually > happens when sending the frame... Ok, that makes sense. The intent here was to test logic for detecting and handling supported driver features/iftypes, rather than actually using the features. But I do understand it would be strange for anyone trying to enable a feature, to find that all it does it set a feature bit (although this is exactly what we want :)). Would it be acceptable (for now at least) if we just included the iftype piece? I can pull that out into a new patch if so. > > johannes
On Tue, 2018-10-09 at 13:12 -0700, James Prestwood wrote: > Ok, that makes sense. The intent here was to test logic for detecting > and handling supported driver features/iftypes, rather than actually > using the features. But I do understand it would be strange for anyone > trying to enable a feature, to find that all it does it set a feature > bit (although this is exactly what we want :)). :-) In general, I guess what would work is to be able to *restrict* the advertised features over what's currently the case, but I suspect that's not something that would be so very much useful for you (unless we also add more features to hwsim). > Would it be acceptable > (for now at least) if we just included the iftype piece? I can pull > that out into a new patch if so. As written, it doesn't make much sense, but again you could restrict the feature set? There are also the pure software feature types etc., and mac80211 will add those in some cases. Similar to the features though, you wouldn't want to advertise e.g. NAN over hwsim, since that requires special operations to be implemented. I guess here also I could see perhaps a way to restrict the interface types could be worthwhile? Though I'd do the implementation with a single u32 attribute with a bitmap, rather than the massive nested flags. Which, btw, you should've used the nested policy support for that I added recently in commit 9a659a35ba17 ("netlink: allow NLA_NESTED to specify nested policy to validate"), but that point is of course moot if we don't want to do a nested attribute :-) johannes
On Tue, 2018-10-09 at 22:13 +0200, Johannes Berg wrote: > On Tue, 2018-10-09 at 13:12 -0700, James Prestwood wrote: > > > Ok, that makes sense. The intent here was to test logic for > > detecting > > and handling supported driver features/iftypes, rather than > > actually > > using the features. But I do understand it would be strange for > > anyone > > trying to enable a feature, to find that all it does it set a > > feature > > bit (although this is exactly what we want :)). > > :-) > > In general, I guess what would work is to be able to *restrict* the > advertised features over what's currently the case, but I suspect > that's > not something that would be so very much useful for you (unless we > also > add more features to hwsim). Actually this would work perfectly. Just being able to disable/restrict features will work fine. I can change the attribute to take a mask of disabled/enabled features, but only features that hwsim actually supports. > > > Would it be acceptable > > (for now at least) if we just included the iftype piece? I can pull > > that out into a new patch if so. > > As written, it doesn't make much sense, but again you could restrict > the > feature set? There are also the pure software feature types etc., and > mac80211 will add those in some cases. > > Similar to the features though, you wouldn't want to advertise e.g. > NAN > over hwsim, since that requires special operations to be implemented. > > I guess here also I could see perhaps a way to restrict the interface > types could be worthwhile? > > Though I'd do the implementation with a single u32 attribute with a > bitmap, rather than the massive nested flags. Which, btw, you > should've > used the nested policy support for that I added recently in commit > 9a659a35ba17 ("netlink: allow NLA_NESTED to specify nested policy to > validate"), but that point is of course moot if we don't want to do a > nested attribute :-) The reason I did it this way was to follow how NL80211 packaged up iftype support (NL80211_ATTR_SUPPORTED_IFTYPES). We could model this after how we want the feature support, where we only allow disabling/enabling features/iftypes that are already supported in the default configuration. So, for iftypes, my code could remain the same where I build up a mask of iftypes, but then AND that with a the default configuration. Thanks, James > > johannes
On Tue, 2018-10-09 at 13:42 -0700, James Prestwood wrote: > > In general, I guess what would work is to be able to *restrict* the > > advertised features over what's currently the case, but I suspect > > that's not something that would be so very much useful for you (unless we > > also add more features to hwsim). > > Actually this would work perfectly. Just being able to disable/restrict > features will work fine. I can change the attribute to take a mask of > disabled/enabled features, but only features that hwsim actually > supports. Well I guess you really only need *disabled* ones since the default would remain to have all enabled, and you can't change them on the fly, only set them at radio creation, right? > The reason I did it this way was to follow how NL80211 packaged up > iftype support (NL80211_ATTR_SUPPORTED_IFTYPES). Ok, but I still think a bitmap would make more sense, back then for some reason I/we didn't (like to) do bitmaps, but these days I think it's the much better representation. > We could model this > after how we want the feature support, where we only allow > disabling/enabling features/iftypes that are already supported in the > default configuration. So, for iftypes, my code could remain the same > where I build up a mask of iftypes, but then AND that with a the > default configuration. Sort of. Yes, I think we would want to have a list/mask of *enabled* (extended) features/interface types, unlike what I said above to just have a mask of disabled features; if we do a mask of disabled ones then one basically has to list all current and *future ones* to have predictable output, that's not going to be feasible. So yes, we want a list of *enabled* ones. However, I think we want to reject configurations that list more enabled features than we currently support, because otherwise again you end up with unpredictability if the hwsim version changes underneath your tool or test case. So I think the algorithm should be: valid_features = <current list>; enabled_features = nla_get_whatever(...); if (enabled_features & ~valid_features) { NL_SET_ERR_ATTR_MSG(...); return -EINVAL; } // use enabled_features instead of the current list later johannes
On Tue, 2018-10-09 at 22:47 +0200, Johannes Berg wrote: > On Tue, 2018-10-09 at 13:42 -0700, James Prestwood wrote: > > > > In general, I guess what would work is to be able to *restrict* > > > the > > > advertised features over what's currently the case, but I suspect > > > that's not something that would be so very much useful for you > > > (unless we > > > also add more features to hwsim). > > > > Actually this would work perfectly. Just being able to > > disable/restrict > > features will work fine. I can change the attribute to take a mask > > of > > disabled/enabled features, but only features that hwsim actually > > supports. > > Well I guess you really only need *disabled* ones since the default > would remain to have all enabled, and you can't change them on the > fly, > only set them at radio creation, right? Yes, disabling features is really what we needed ultimately. And changing them on the fly is not needed. > > > The reason I did it this way was to follow how NL80211 packaged up > > iftype support (NL80211_ATTR_SUPPORTED_IFTYPES). > > Ok, but I still think a bitmap would make more sense, back then for > some > reason I/we didn't (like to) do bitmaps, but these days I think it's > the > much better representation. > > > We could model this > > after how we want the feature support, where we only allow > > disabling/enabling features/iftypes that are already supported in > > the > > default configuration. So, for iftypes, my code could remain the > > same > > where I build up a mask of iftypes, but then AND that with a the > > default configuration. > > Sort of. > > Yes, I think we would want to have a list/mask of *enabled* > (extended) > features/interface types, unlike what I said above to just have a > mask > of disabled features; if we do a mask of disabled ones then one > basically has to list all current and *future ones* to have > predictable > output, that's not going to be feasible. > > So yes, we want a list of *enabled* ones. > > However, I think we want to reject configurations that list more > enabled > features than we currently support, because otherwise again you end > up > with unpredictability if the hwsim version changes underneath your > tool > or test case. Ok, yeah this makes sense. > > So I think the algorithm should be: > > valid_features = <current list>; > enabled_features = nla_get_whatever(...); > > if (enabled_features & ~valid_features) { > NL_SET_ERR_ATTR_MSG(...); > return -EINVAL; > } > > // use enabled_features instead of the current list later Ok, thanks for your help/suggestions. Ill get a patch together with the talked about changes. > > johannes
James Prestwood <james.prestwood@linux.intel.com> writes: > The current driver hard codes various features, ext features and supported > interface types when a new radio is created. This patch adds several new > hwsim attributes so user space can specify specific features the radio should > have: > > - HWSIM_ATTR_FEATURE_SUPPORT > Bit field of nl80211_feature_flags flags > - HWSIM_ATTR_EXT_FEATURE_SUPPORT > Variable length bit field where bits map to nl80211_ext_feature_index > values. > - HWSIM_ATTR_IFTYPE_SUPPORT > Nested attribute of flags containing all supported interface types. > Each flag attribute sets support for an interface type. > > Each attribute data format matches a corresponding NL80211 attribute: > > HWSIM_ATTR_FEATURE_SUPPORT --> NL80211_ATTR_FEATURE_FLAGS > HWSIM_ATTR_EXT_FEATURE_SUPPORT --> NL80211_ATTR_EXT_FEATURES > HWSIM_ATTR_IFTYPE_SUPPORT --> NL80211_ATTR_SUPPORTED_IFTYPES Signed-off-by is missing. https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#signed-off-by_missing
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c index 18e819d964f1..727e61cf154c 100644 --- a/drivers/net/wireless/mac80211_hwsim.c +++ b/drivers/net/wireless/mac80211_hwsim.c @@ -641,8 +641,30 @@ static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = { [HWSIM_ATTR_NO_VIF] = { .type = NLA_FLAG }, [HWSIM_ATTR_FREQ] = { .type = NLA_U32 }, [HWSIM_ATTR_PERM_ADDR] = { .type = NLA_UNSPEC, .len = ETH_ALEN }, + [HWSIM_ATTR_FEATURE_SUPPORT] = { .type = NLA_U32 }, + [HWSIM_ATTR_EXT_FEATURE_SUPPORT] = { .type = NLA_BINARY }, + [HWSIM_ATTR_IFTYPE_SUPPORT] = { .type = NLA_NESTED }, }; +/* policy for iftype support flags */ +static const struct nla_policy +hwsim_iftype_support_policy[NUM_NL80211_IFTYPES] = { + [NL80211_IFTYPE_UNSPECIFIED] = { .type = NLA_FLAG }, + [NL80211_IFTYPE_ADHOC] = { .type = NLA_FLAG }, + [NL80211_IFTYPE_STATION] = { .type = NLA_FLAG }, + [NL80211_IFTYPE_AP] = { .type = NLA_FLAG }, + [NL80211_IFTYPE_AP_VLAN] = { .type = NLA_FLAG }, + [NL80211_IFTYPE_WDS] = { .type = NLA_FLAG }, + [NL80211_IFTYPE_MONITOR] = { .type = NLA_FLAG }, + [NL80211_IFTYPE_MESH_POINT] = { .type = NLA_FLAG }, + [NL80211_IFTYPE_P2P_CLIENT] = { .type = NLA_FLAG }, + [NL80211_IFTYPE_P2P_GO] = { .type = NLA_FLAG }, + [NL80211_IFTYPE_P2P_DEVICE] = { .type = NLA_FLAG }, + [NL80211_IFTYPE_OCB] = { .type = NLA_FLAG }, + [NL80211_IFTYPE_NAN] = { .type = NLA_FLAG }, +}; + + static void mac80211_hwsim_tx_frame(struct ieee80211_hw *hw, struct sk_buff *skb, struct ieee80211_channel *chan); @@ -2413,6 +2435,9 @@ struct hwsim_new_radio_params { const char *hwname; bool no_vif; const u8 *perm_addr; + u32 features; + u8 ext_features[DIV_ROUND_UP(NUM_NL80211_EXT_FEATURES, 8)]; + u32 iftypes; }; static void hwsim_mcast_config_msg(struct sk_buff *mcast_skb, @@ -2631,12 +2656,8 @@ static int mac80211_hwsim_new_radio(struct genl_info *info, hw->queues = 5; hw->offchannel_tx_hw_queue = 4; - hw->wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION) | - BIT(NL80211_IFTYPE_AP) | - BIT(NL80211_IFTYPE_P2P_CLIENT) | - BIT(NL80211_IFTYPE_P2P_GO) | - BIT(NL80211_IFTYPE_ADHOC) | - BIT(NL80211_IFTYPE_MESH_POINT); + + hw->wiphy->interface_modes = param->iftypes; if (param->p2p_device) hw->wiphy->interface_modes |= BIT(NL80211_IFTYPE_P2P_DEVICE); @@ -2658,12 +2679,10 @@ static int mac80211_hwsim_new_radio(struct genl_info *info, WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL | WIPHY_FLAG_AP_UAPSD | WIPHY_FLAG_HAS_CHANNEL_SWITCH; - hw->wiphy->features |= NL80211_FEATURE_ACTIVE_MONITOR | - NL80211_FEATURE_AP_MODE_CHAN_WIDTH_CHANGE | - NL80211_FEATURE_STATIC_SMPS | - NL80211_FEATURE_DYNAMIC_SMPS | - NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR; - wiphy_ext_feature_set(hw->wiphy, NL80211_EXT_FEATURE_VHT_IBSS); + + hw->wiphy->features |= param->features; + memcpy(hw->wiphy->ext_features, param->ext_features, + DIV_ROUND_UP(NUM_NL80211_EXT_FEATURES, 8)); /* ask mac80211 to reserve space for magic */ hw->vif_data_size = sizeof(struct hwsim_vif_priv); @@ -2767,8 +2786,6 @@ static int mac80211_hwsim_new_radio(struct genl_info *info, if (param->no_vif) ieee80211_hw_set(hw, NO_AUTO_VIF); - wiphy_ext_feature_set(hw->wiphy, NL80211_EXT_FEATURE_CQM_RSSI_LIST); - err = ieee80211_register_hw(hw); if (err < 0) { pr_debug("mac80211_hwsim: ieee80211_register_hw failed (%d)\n", @@ -3244,6 +3261,56 @@ static int hwsim_new_radio_nl(struct sk_buff *msg, struct genl_info *info) param.perm_addr = nla_data(info->attrs[HWSIM_ATTR_PERM_ADDR]); } + if (info->attrs[HWSIM_ATTR_FEATURE_SUPPORT]) + param.features = nla_get_u32( + info->attrs[HWSIM_ATTR_FEATURE_SUPPORT]); + else + param.features = NL80211_FEATURE_ACTIVE_MONITOR | + NL80211_FEATURE_AP_MODE_CHAN_WIDTH_CHANGE | + NL80211_FEATURE_STATIC_SMPS | + NL80211_FEATURE_DYNAMIC_SMPS | + NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR; + + if (info->attrs[HWSIM_ATTR_EXT_FEATURE_SUPPORT]) { + if (nla_len(info->attrs[HWSIM_ATTR_EXT_FEATURE_SUPPORT]) != + DIV_ROUND_UP(NUM_NL80211_EXT_FEATURES, 8)) + return -EINVAL; + + memcpy(param.ext_features, + nla_data(info->attrs[HWSIM_ATTR_EXT_FEATURE_SUPPORT]), + DIV_ROUND_UP(NUM_NL80211_EXT_FEATURES, 8)); + } else { + param.ext_features[NL80211_EXT_FEATURE_VHT_IBSS / 8] |= + BIT(NL80211_EXT_FEATURE_VHT_IBSS % 8); + param.ext_features[NL80211_EXT_FEATURE_CQM_RSSI_LIST / 8] |= + BIT(NL80211_EXT_FEATURE_CQM_RSSI_LIST % 8); + } + + if (info->attrs[HWSIM_ATTR_IFTYPE_SUPPORT]) { + int i; + struct nlattr *types[NUM_NL80211_IFTYPES]; + int err = nla_parse_nested(types, + NUM_NL80211_IFTYPES - 1, + info->attrs[HWSIM_ATTR_IFTYPE_SUPPORT], + hwsim_iftype_support_policy, + info->extack); + if (err) { + kfree(hwname); + return err; + } + + for (i = 0; i < NUM_NL80211_IFTYPES; i++) { + if (nla_get_flag(types[i])) + param.iftypes |= BIT(i); + } + } else + param.iftypes = BIT(NL80211_IFTYPE_STATION) | + BIT(NL80211_IFTYPE_AP) | + BIT(NL80211_IFTYPE_P2P_CLIENT) | + BIT(NL80211_IFTYPE_P2P_GO) | + BIT(NL80211_IFTYPE_ADHOC) | + BIT(NL80211_IFTYPE_MESH_POINT); + ret = mac80211_hwsim_new_radio(info, ¶m); kfree(hwname); return ret; diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h index 0fe3199f8c72..aea242eb4746 100644 --- a/drivers/net/wireless/mac80211_hwsim.h +++ b/drivers/net/wireless/mac80211_hwsim.h @@ -132,6 +132,13 @@ enum { * @HWSIM_ATTR_TX_INFO_FLAGS: additional flags for corresponding * rates of %HWSIM_ATTR_TX_INFO * @HWSIM_ATTR_PERM_ADDR: permanent mac address of new radio + * @HWSIM_ATTR_FEATURE_SUPPORT: bit mask of supported features (u32 attribute) + * Matches data format of %NL80211_ATTR_FEATURE_FLAGS + * @HWSIM_ATTR_EXT_FEATURE_SUPPORT: variable length bit field. Matches data + * format of %NL80211_ATTR_EXT_FEATURES + * @HWSIM_ATTR_IFTYPE_SUPPORT: nested attribute containing flag attributes. + * Each flag either sets the support for each iftype. Matches format of + * %NL80211_ATTR_SUPPORTED_IFTYPES * @__HWSIM_ATTR_MAX: enum limit */ @@ -160,6 +167,9 @@ enum { HWSIM_ATTR_PAD, HWSIM_ATTR_TX_INFO_FLAGS, HWSIM_ATTR_PERM_ADDR, + HWSIM_ATTR_FEATURE_SUPPORT, + HWSIM_ATTR_EXT_FEATURE_SUPPORT, + HWSIM_ATTR_IFTYPE_SUPPORT, __HWSIM_ATTR_MAX, }; #define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1)