Re: [PATCH] mac80211_hwsim: allow setting custom features/iftypes
James Prestwood 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 -- Kalle Valo
Re: [PATCH] mac80211_hwsim: allow setting custom features/iftypes
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 = ;
> 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
Re: [PATCH] mac80211_hwsim: allow setting custom features/iftypes
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 = ;
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
Re: [PATCH] mac80211_hwsim: allow setting custom features/iftypes
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
Re: [PATCH] mac80211_hwsim: allow setting custom features/iftypes
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
Re: [PATCH] mac80211_hwsim: allow setting custom features/iftypes
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
Re: [PATCH] mac80211_hwsim: allow setting custom features/iftypes
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
