DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 66505213A2
Message-ID: <926a1d1e65f089767b9316fecc619ef4c70fa008.camel@linux.intel.com>
Subject: Re: [PATCH] mac80211_hwsim: allow setting custom features/iftypes
From:   James Prestwood <james.prestwood@linux.intel.com>
To:     Johannes Berg <johannes@sipsolutions.net>,
        linux-wireless@vger.kernel.org
Date:   Tue, 09 Oct 2018 13:42:16 -0700
In-Reply-To: <1539116037.3687.143.camel@sipsolutions.net>
References: <20181009174829.15163-1-james.prestwood@linux.intel.com>
         (sfid-20181009_194449_357742_49BD7697) <1539114071.3687.134.camel@sipsolutions.net>
         <0b8356b90053c16e674905ca78707d6a19de0701.camel@linux.intel.com>
         <1539116037.3687.143.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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