Re: [RFC PATCH v2 2/2] mac80211: Add support for Extended Key ID
> > > - /* PTK only using key ID 0 needs special handling on rekey */
> > > - if (new_key && sta && ptk0rekey) {
> > > + /* PTK rekey without Extended Key ID needs special handling */
> > > + if (new_key && pairwise && sta &&
> > > + !test_sta_flag(sta, WLAN_STA_EXT_KEY_ID)) {
> > > local = old_key->local;
> > > sdata = old_key->sdata;
> >
> > This seems wrong, even if you have ext key ID support and everything,
> > but you do 0 -> 0 rekeying, then you still need all the special handling
> > (in fact also then if you go 1->1!). So it seems you'd instead want to
> > see if you're going from a TX key to a TX key with the same key ID, and
> > then you don't need this flag at all.
> >
>
> The intention for Extended Key ID is, to have a comparable short time
> frame where both key IDs can be used. When replacing e.g. key ID 0 again
> it should be idle for a long time. I guess if someone starts re-keying
> in 1s intervals it may become an issue, but then anyone re-keying that
> often can't be helped...
Sure. But ... not sure how that's related?
> With Extended Key IDs it's impossible to directly switch from a TX key
> with one key ID to another one with the same id.
>
> 1) Association
> 2) key ID 0 installed RX only
> 3) key Id 0 set_tx
> 4) rekey timeout passes
> 5) key ID 1 installed RX only
> 6) key ID 1 set_tx (also making key ID 0 RX only)
> 7) rekey timeout passes
> 8) key ID 0 replaced with new RX only key
> 9) key ID 0 set_tx
> 10) rekey timeout passes
> ...
>
> So nobody will use the key being replaced, we don't have to protect
> against PN poisoning.
Exactly.
> And when a driver supports Extended Key ID we
> don't care about if the driver is able to rekey PTK0 correctly.
Strictly speaking, that's false, since you don't know if wpa_s actually
used it, and the peer STA allowed it.
It's also not what you implemented, you implemented checking if
NL80211_KEY_RX_ONLY was ever used.
However, what I'm trying to say is that I'm not sure this makes sense?
It seems to me it would be safer, and easier (no station flag), to just
check
if ("we're replacing the current TX key")
and trigger the workarounds in that case. No?
Yes, parts of the issue also manifest themselves on the RX side, but if
you're not replacing the current key then you were using extended key ID
support?
> > > +++ b/net/mac80211/sta_info.c
> > > @@ -350,6 +350,7 @@ struct sta_info *sta_info_alloc(struct
> > > ieee80211_sub_if_data *sdata,
> > > sta->sta.max_rx_aggregation_subframes =
> > > local->hw.max_rx_aggregation_subframes;
> > >
> > > + sta->ptk_idx = NUM_DEFAULT_KEYS - 1;
> >
> > That makes no sense? Why should it be 3? That's invalid anyway?
>
> Yes, that's the whole reason for that change:-) Setting it to 2 would
> also be fine, as long as it's not 0 or 1.
Hmm, ok. So that probably wants a big comment saying that it relies on
key idx 2/3 being invalid. I'm not sure I like the NUM_DEFAULT_KEYS-1,
better perhaps to do something like
/* comment saying why */
BUILD_BUG_ON(ARRAY_SIZE(sta->ptks) > 2);
sta->ptk_idx = 2;
or so?
> ieee80211_tx_h_select_key starts encrypting packets as soon as
> sta->ptk[tx->sta->ptk_idx] is not null.
Right, so I guess this makes sense.
johannes
Re: [RFC PATCH v2 1/2] nl80211/cfg80211: Add support for Extended Key ID
On Wed, 2018-12-05 at 21:54 +0100, Alexander Wetzel wrote: > > It started out as a flag and I switched to enum later without updating > it. I'll chnage that to nl80211_key_install_mode, much much better... > No, all stations using Extended Key ID will always use RX_ONLY and > SET_TX for pairwise key installs. The AP will install the Key Rx only > prior to sending eapol #3, the sta prior to sending eapol #4. Actually ... let's see all the operations at nl80211 level. We have NEW_KEY and SET_KEY, right? SET_KEY basically already says to use a given key for TX from now on, IIRC? So realistically, don't we only need NEW_KEY (RX-only) as a new variant of NEW_KEY? > The PTK0 rekey patch added a new line in front of the description. The > next author did not notice that and added the description for > NL80211_EXT_FEATURE_ENABLE_FTM_RESPONDER at the wrong place, probably > assuming it was the end of the list. I've noticed that and fixed the > documentation order and the misleading empty line. > I'll break that out as a separate cleanup patch if nobody beats me to it:-) Oh. OK. It also doesn't really matter ;-) > > > k->def_multi = true; > > > > > > + k->rx_only = !!info->attrs[NL80211_ATTR_KEY_RXONLY]; > > > + k->set_tx = !!info->attrs[NL80211_ATTR_KEY_SETTX]; > > > > shouldn't this already be install_mode? > > > > Looks like switching to NL80211_ATTR_INSTALL_MODE will throw out that, > but with the code from this patch that's an interim layer for checks and > mapping it. So I'm not sure I understand that comment. Well, me neither. Sounds almost like I got ahead of myself. > > You need to check if extended key ID is supported > > Yes. I have added checks in cfg80211_validate_key_settings current > development version already, making sure only valid combinations can be > called and reach this section. Ok, great. > NL80211_CMD_SET_KEY is normally only used to set default keys for wep or > managment keys. That changes here. Right. > In this API draft NL80211_CMD_NEW_KEY is only used when installing a > Extended Key ID key RX only. The switch to TX is added to > NL80211_CMD_SET_KEY. The code has some sanity checks and then tells the > driver to switch the key to tx. Makes sense. > But that has been changed quite a bit, the procedure in this patch > turned out to be not so good and even had a locking issue, so it has > changed a bit. I guess we should shelf that till I get the new variant > working and then look at it again. Fair enough. > > > +++ b/net/wireless/util.c > > > @@ -236,13 +236,14 @@ int cfg80211_validate_key_settings(struct > > > cfg80211_registered_device *rdev, > > > case WLAN_CIPHER_SUITE_CCMP_256: > > > case WLAN_CIPHER_SUITE_GCMP: > > > case WLAN_CIPHER_SUITE_GCMP_256: > > > - /* Disallow pairwise keys with non-zero index unless it's WEP > > > + /* IEEE802.11-2016 allows only 0 and 1 for pairwise keys. > > > + * Disallow pairwise keys with index above 1 unless it's WEP > > >* or a vendor specific cipher (because current > > > deployments use > > > - * pairwise WEP keys with non-zero indices and for vendor > > > + * pairwise WEP keys with higher indices and for vendor > > >* specific ciphers this should be validated in the > > > driver or > > > - * hardware level - but 802.11i clearly specifies to use zero) > > > + * hardware level. > > >*/ > > > - if (pairwise && key_idx) > > > + if (pairwise && key_idx > 1) > > > return -EINVAL; > > > break; > > > > Again, only if driver support is advertised, and the comment should > > probably reference the feature bit from the spec. > > That is where I added most of the sanity checks in the meantime. > But what feature bit from the spec are you referring to? The RSN > Capability one? Well, I wasn't thinking that precisely. I just thought it should mention that it now says "IEEE802.11-2016 allows only 0 and 1 for pairwise keys" but doesn't clarify that this is only for stations that actually want to support it, so it could be read as being always that way. johannes
Re: [RFC PATCH v2 2/2] mac80211: Add support for Extended Key ID
> + * @IEEE80211_KEY_FLAG_RX_ONLY: Set by mac80211 to indicate that the key
> + * must not be used for TX (yet).
I'm not sure that's relevant, since you have one key pointer for TX?
> + * @IEEE80211_KEY_FLAG_SET_TX: Set by mac80211 to indicate that a previously
> + * installed key with IEEE80211_KEY_FLAG_RX_ONLY should take over TX
> also.
That also doesn't seem relevant ...
Oh, all of this is for HW offloads?
I _think_ I would prefer to have new key ops instead. Now you'd have
SET_KEY /
SET_KEY / RX_ONLY
SET_KEY / SET_TX
but I think maybe
SET_KEY
SET_KEY_RX_ONLY
KEY_ENABLE_TX
would make more sense?
> + if (pairwise && params->flag == NL80211_KEY_SET_TX) {
> + mutex_lock(>sta_mtx);
> + sta = sta_info_get_bss(sdata, mac_addr);
> +
> + if (!sta ||
> +!(key = rcu_dereference(sta->ptk[key_idx])) ||
indentation here is off by one
> +!(key->conf.flags | IEEE80211_KEY_FLAG_RX_ONLY)) {
that makes no sense, should be & I guess
> - /* PTK only using key ID 0 needs special handling on rekey */
> - if (new_key && sta && ptk0rekey) {
> + /* PTK rekey without Extended Key ID needs special handling */
> + if (new_key && pairwise && sta &&
> + !test_sta_flag(sta, WLAN_STA_EXT_KEY_ID)) {
> local = old_key->local;
> sdata = old_key->sdata;
This seems wrong, even if you have ext key ID support and everything,
but you do 0 -> 0 rekeying, then you still need all the special handling
(in fact also then if you go 1->1!). So it seems you'd instead want to
see if you're going from a TX key to a TX key with the same key ID, and
then you don't need this flag at all.
> +++ b/net/mac80211/sta_info.c
> @@ -350,6 +350,7 @@ struct sta_info *sta_info_alloc(struct
> ieee80211_sub_if_data *sdata,
> sta->sta.max_rx_aggregation_subframes =
> local->hw.max_rx_aggregation_subframes;
>
> + sta->ptk_idx = NUM_DEFAULT_KEYS - 1;
That makes no sense? Why should it be 3? That's invalid anyway?
johannes
Re: [RFC PATCH v2 1/2] nl80211/cfg80211: Add support for Extended Key ID
On Sun, 2018-11-11 at 12:02 +0100, Alexander Wetzel wrote:
> Extend cfg80211 and nl80211 to allow pairwise keys to be installed for
> RX only, allowing to switching over TX independently, as required by
> IEEE-802.11-2016 to support "Extended Key ID for Individually Addressed
> Frames"
>
> PTK and STK keys are now also allowed to use Key ID 1.
>
> Signed-off-by: Alexander Wetzel
> ---
> include/net/cfg80211.h | 2 ++
> include/uapi/linux/nl80211.h | 41 ++---
> net/wireless/nl80211.c | 51
> net/wireless/rdev-ops.h | 3 ++-
> net/wireless/trace.h | 31 ++
> net/wireless/util.c | 9 ---
> 6 files changed, 119 insertions(+), 18 deletions(-)
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 1fa41b7a1be3..0d59340563e0 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -485,6 +485,7 @@ struct vif_params {
> * with the get_key() callback, must be in little endian,
> * length given by @seq_len.
> * @seq_len: length of @seq.
> + * @flag: One flag from key_params_flag
That should be nl80211_key_params_flag.
But if only one flag can be set, then maybe this should instead be
enum nl80211_key_install_mode install_mode;
or so?
> +/**
> + * enum key_params_flag - additional key flag for drivers
> + *
> + * Actions other than @NL80211_KEY_DEFAULT_RX_TX are only required from
> drivers
> + * supporting Extended Key ID for pairwise keys using keyid 0 or 1.
> + *
> + * @NL80211_KEY_DEFAULT_RX_TX: key can be immediately used for Rx and Tx
> + * @NL80211_KEY_RX_ONLY: key must be installed for Rx only
> + * @NL80211_KEY_SET_TX: switch Tx for sta to specified keyid
> + */
> +enum key_params_flag {
> + NL80211_KEY_DEFAULT_RX_TX,
> + NL80211_KEY_RX_ONLY,
> + NL80211_KEY_SET_TX
> +};
Clearly those aren't flags anyway.
I guess RX_ONLY and SET_TX are mostly needed AP-side?
> + * @NL80211_ATTR_KEY_RXONLY: Flag attribute to request RX key install only
> for
> + * a pairwise key. Only supported for keyid's 0 and 1 when driver is
> + * supporting Extended Key ID.
> + *
> + * @NL80211_ATTR_KEY_SETTX: Flag attribute to switch TX to a specified keyid.
> + * Only supported for keyid's 0 and 1 when driver is supporting Extended
> + * Key ID.
Ok, so you have these as separate netlink flags, but then you really
shouldn't also have the "install mode" in nl80211.h, that's not related
to userspace API then.
We might discuss instead having an NL80211_ATTR_INSTALL_MODE attribute,
and that takes the values from the enum, and then you do need the enum -
but if you don't need the enum then don't define it in nl80211.h but
keep it kernel-internal in cfg80211.h (and name it appropriately).
> @@ -4312,6 +4343,8 @@ enum nl80211_key_attributes {
> NL80211_KEY_DEFAULT_MGMT,
> NL80211_KEY_TYPE,
> NL80211_KEY_DEFAULT_TYPES,
> + NL80211_KEY_RXONLY,
> + NL80211_KEY_SETTX,
Indeed if you have this, you don't need the corresponding top-level
NL80211_ATTR_*?
We went through a few iterations with the API, so a lot of this is
backward compatibility stuff, you should update the latest version only.
I believe it's this one.
> - * @NL80211_EXT_FEATURE_ENABLE_FTM_RESPONDER: Driver supports enabling fine
> - * timing measurement responder role.
> - *
> * @NL80211_EXT_FEATURE_CAN_REPLACE_PTK0: Driver/device confirm that they are
> * able to rekey an in-use key correctly. Userspace must not rekey PTK
> keys
> * if this flag is not set. Ignoring this can leak clear text packets
> and/or
> * freeze the connection.
> + * @NL80211_EXT_FEATURE_ENABLE_FTM_RESPONDER: Driver supports enabling fine
> + * timing measurement responder role.
What's going on here?
> @@ -945,6 +960,16 @@ static int nl80211_parse_key_old(struct genl_info *info,
> struct key_parse *k)
> if (k->defmgmt)
Yeah, just don't change parse_key_old, whoever wants to use this stuff
should upgrade to the new API. wpa_s has both IIRC, but of course the
old-side is ignored on newer kernels (and the new on older) so the older
stuff never needs new features.
> k->def_multi = true;
>
> + k->rx_only = !!info->attrs[NL80211_ATTR_KEY_RXONLY];
> + k->set_tx = !!info->attrs[NL80211_ATTR_KEY_SETTX];
shouldn't this already be install_mode?
> + /* only support setting default key and
> + * Extended Key ID action NL80211_KEY_SET_TX */
> + if (!key.def && !key.defmgmt && !key.set_tx)
> return -EINVAL;
You need to check if extended key ID is supported
> - }
> + } else if (wiphy_ext_feature_isset(>wiphy,
> +NL80211_EXT_FEATURE_EXT_KEY_ID)) {
> + if (info->attrs[NL80211_ATTR_MAC])
> + mac_addr = nla_data(info->attrs[NL80211_ATTR_MAC]);
>
> + if (!mac_addr || key.idx < 0 || key.idx > 1) {
> +
Re: [RFC PATCH v2 0/2] Extended Key ID support for linux
Hi, Sorry for the delay. On Sun, 2018-11-11 at 12:02 +0100, Alexander Wetzel wrote: > IEEE 802.11-2012 added support for Extended Key ID, allowing pairwise > keys to also use keyID 1 and moving group keys to IDs 2 and 3. Where do you read this? I've always been under the impression that individually and group addressed frames use key IDs from different "namespaces", so to speak, where PTK/STK can use 0 (0 or 1 with "Extended Key ID" support) and GTK can use 0-3. In fact, the per-frame pseudocode in 802.11-2016 12.9.2.6 clearly states: if MPDU has individual RA then lookup pairwise key using Key ID from MPDU else lookup group key using Key ID from MPDU endif If it weren't different namespaces, you'd not have to differentiate here. > Support for Extended Key ID is basically completed and confirmed working > with both hwsim and "on the air" with ath9k/iwldvm using software > encryption and those patches here. :) > Prior to propose this patch for merging I would like to get Extended > Key ID working with HW encryption for at least some devices, but after > experimenting with ath9k and to a lesser extend with ath10k it's now > clear that this will be an per-driver effort and it may well turn out to > be impossible without firmware updates. Indeed. I think there might be some support with iwlwifi firmware, at least newer versions? I can check later. > So I've decided to continue working on the HW support for now but also > ask you for feedback for what I got so far. Sounds good. > Any feedback is welcome and I especially like to learn what you think of > the API extensions and what has to be changed to get it merged. I'll look over the individual patches. johannes
Re: [RFC 0/5] add XDP support to mt76x2e/mt76x0e drivers
On Mon, 2018-12-03 at 21:36 +0200, Toke Høiland-Jørgensen wrote: > Hi Johannes > > I think your email can be basically summed up to: > > > [ ... ] but really I think it's a can of worms. > > ...right? :) Heh, yeah :) > I sort of had a feeling it would be, but thank you for spelling out in > excruciating detail why that is so. :-) > Given this, I think I agree that it's not worth it for now, and we > should hold off on adding XDP support until we have 802.3/.11 conversion > offload working... Which I think is also where you ended up? :) That case is at least easy, yeah. And it seems kinda likely that we'll end up with that in all well-maintained drivers in the relatively near future anyway? BTW, in a sense I still kind of want to add eBPF to the mac80211 ingress path, just not in the XDP sense. For example, I had a proposal a while ago to add a filter to the monitor mode RX path(s) in eBPF; I still think that's useful. I also think it may be useful to put eBPF programs into per-netdev ingress path, in order to e.g. collect statistics, rather than hard- coding all kinds of statistics into mac80211. All of these things I consider absolutely useful and helpful. I like eBPF and the flexibility it affords. I just really don't think we should call it XDP or let it do similar things to XDP like dropping or redirecting frames. johannes
Re: [RFC 0/5] add XDP support to mt76x2e/mt76x0e drivers
Hi Toke, all, Sorry I wasn't around for the discussion, I've been travelling and sick. I'm picking this as an arbitrary point in the discussion to reply to, hope you don't feel bad about that. >> [A-MPDU reordering] > In principle, all of this could be done. But we need to think carefully > about what things it really makes sense to offload to XDP. In the > general case, we will end up re-implementing all of mac80211 in eBPF, > which is obviously not ideal. Agree with this, completely. Mind you, it's not even simple to do that, because each kind of hardware behaves differently. See below. > For crypto in particular, I wonder if there is much of a speedup to be > had if the crypto is in software anyway. Wouldn't that dominate the > processing time? Yeah, I'd also think the cost of skb allocation is dwarfed by the cost of doing the crypto, but not really sure, maybe you have a fast special AES instruction? :) Anyway, here are my thoughts after reading most of the thread and having discussed it with Lorenzo a while back and today: 1) I'm going to restrict most of the discussion to mac80211, but there are interesting cases outside of it as well. Much of the same arguments apply, though it means *more* feature combinatorial explosion to be able to handle everything. 2) We don't yet have it (but it's been suggested for years and we might also get it on Intel hardware eventually), but if we have TX and/or RX 802.3/802.11 frame conversion offload then I think we have no argument - XDP applies pure and simple as is in each direction you have offloads for, unless you do something really stupid in HW like still having to do reorder buffer or PN checking based on RX frame metadata. I hope nobody does that, but then I suppose you could do it before the XDP hooks w/o allocating SKBs. 3) TX from XDP (i.e. X -> wifi) could be implemented in mac80211, but is subject to some rather iffy details, for example: a) mac80211 may need more headroom than the input has, do we have to copy the whole data? depending on xdp_mem_type I think we may need that, rather than being able to make an skb with frag pages and extra headroom? We don't know it was just a single page and I guess we can't ref that page anyway, but maybe we could meddle with the SKB frag data somehow to avoid that. b) mac80211 may need to software encrypt - copy the whole data c) lifetime issues - we need to call xdp_return_frame(), which I guess we can tie to the skb we'd create anyway [2] (and IMHO we need to allocate an skb anyway, due to TXQs like Toke mentioned and much other possible software handling) 4) XDP during RX - well, that's _mostly_ what this thread has been about, but really I think it's a can of worms. Consider: a) drivers may or may not do SW decryption b) drivers may or may not do PN checking - so your XDP program might end up having to do that or risk security/replay bugs! (which, btw, you can only do after reordering so see below) c) drivers may or may not do A-MSDU deaggregation, do you[1] really want to handle that in XDP? You could argue outer code should walk over the subframes, but then dropping becomes hard - and this really should only happen after a) and b) Oh, and IIRC there are cases that use A-MSDUs for single frames just to encapsulate different MAC addresses. Speaking of which, you have to validate the inner MAC addresses (but it's not a trivial comparison) or risk security bugs again. d) drivers may or may not do A-MPDU reordering (this was discussed earlier in the thread), which means even if you drop a frame you haven't just affected that connection but potentially stalled everything until the reorder buffer times out (~100ms), or we need a way for mac80211 to insert a fake skb into the reorder buffer at that spot (but then you allocate an skb again and high-speed traffic where you'd care most is always in an A-MPDU session) e) speaking of which, Intel's device has hardware assist things for reordering, but not fully offloaded reordering, so you might end up with hardware-specific ways of doing this in XDP? f) Some data frames are used internally in the stack, e.g. for TDLS. g) data frames may also affect the powersave state of a station, so if you have e.g. ath9k and the station sends a frame to wake up, but you decide you XDP_DROP it, the powersave state will get messed up, unless ... special handling (either mac80211 or the bpf program) again. Also, U-APSD. h) There are also simple things like QoS/non-QoS that add to the combinatorial explosion of things you have to handle i) data frames could be sent by anyone, not just STAs connected to your network, so I guess you'd have to filter that and mac80211 would have to expose a station table lookup helper or
Re: mac80211_hwsim : unsigned int for signal in netlink info frames
On Mon, 2018-11-26 at 17:07 +0100, Benjamin Beichler wrote: > I'm working on my code base and I was surprised by the fact, that the > type of the received signal strength for frames received via Netlink is > u32, but the actual struct ieee80211_rx_status.signal uses an s8 for signal. > > I actually refer to this line: > https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/net/wireless/mac80211_hwsim.c#L3250 I guess this should use nla_get_s32 now, but I think that didn't exist before and it also doesn't really matter since if you have a negative value in a u32 it still works just fine as long as userspace and kernelspace agree on the 2's complement representation :-) > As we use the signal measurement in dBm (see > https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/net/wireless/mac80211_hwsim.c#L2764), > negative dBm values are totally reasonable as received signal strength. > Moreover, I don't really know, whether mac80211 or respectively minstrel > can do reasonable work with positive values. Indeed. Make it negative. > On the other hand this line > https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/net/wireless/mac80211_hwsim.c#L1276 > inconsistently uses a negative value in the case of not using > netlink/wmediumd, which is a decent value. Sure, it should be negative. > I think it is not possible to easily switch to another type (e.g. s32 or > even s8) for the netlink attribute without breaking things, but somebody > might correct me. Well, u32 and s32 are really identical in netlink anyway, they're just 4 bytes long integers. So there's no "switching" if we now write "nla_get_s32" in the code. What we might want to do is use a policy now that says it must be a sensible value like -200 to -10 or something, but it doesn't really matter. > Any suggestions? Just put a negative integer there in your userspace. (u32)-50 == 0xffce signal = nla_get_u32(...) = 0xffce -> signal will end up with -50 Nothing to see here, move along ;-) johannes
Re: Issue with STBC capability and forcing radio to 1x1 mode.
On Wed, 2018-11-28 at 14:13 -0800, Ben Greear wrote: > Hello, > > I notice some weird things while debugging an issue on a 4.16+ kernel with > ath10k > radio. > > It seems that the 'iw phy info' does not remove the TX-STBC info > when changing the antenna mask _until_ some vdev is brought up. > > This makes it difficult to properly create hostapd config files. > That's not anything iw can do, must be the driver. johannes
Re: [PATCH] {nl,mac}80211: allow 4addr AP operation on crypto controlled devices
This looks good to me, just a few nits below
On Wed, 2018-11-21 at 16:33 +0530, Manikanta Pubbisetty wrote:
> As per the current design, for sw crypto controlled devices, it is
> the device which has to advertise the support for AP/VLAN iftype
> based on it's capability to tranmsit packets encrypted in software
> (In VLAN functionality, group traffic generated for a specific
> VLAN group is always encrypted in software). Commit db3bdcb9c3ff
> ("mac80211: allow AP_VLAN operation on crypto controlled devices")
> has introduced this change.
>
> Since 4addr AP operation also uses AP/VLAN iftype, this conditional
> way of advertising AP/VLAN support has broken 4addr AP mode operation on
> crypto controlled devices which do not support VLAN functionality.
>
> For example:
> In the case of ath10k driver, not all firmwares have support for VLAN
> functionality but all can support 4addr AP operation. Because AP/VLAN
> support is not advertised for these devices, 4addr AP operations are
> also blocked.
>
> Fix this by allowing 4addr opertion on devices which do not advertise
operation
> AP/VLAN iftype but which can support 4addr operation (the desicion is
decision
> taken based on the wiphy flag WIPHY_FLAG_4ADDR_AP).
> Fixes: Commit db3bdcb9c3ff ("mac80211: allow AP_VLAN operation on
> crypto controlled devices")
I think it'd be better not to wrap this
> --- a/net/wireless/core.c
> +++ b/net/wireless/core.c
> @@ -1394,8 +1394,13 @@ static int cfg80211_netdev_notifier_call(struct
> notifier_block *nb,
> }
> break;
> case NETDEV_PRE_UP:
> - if (!(wdev->wiphy->interface_modes & BIT(wdev->iftype)))
> - return notifier_from_errno(-EOPNOTSUPP);
> + if (!(wdev->wiphy->interface_modes & BIT(wdev->iftype))) {
> + if (!(wdev->iftype == NL80211_IFTYPE_AP_VLAN &&
> + rdev->wiphy.flags & WIPHY_FLAG_4ADDR_AP &&
> + wdev->use_4addr))
> + return notifier_from_errno(-EOPNOTSUPP);
> + }
Maybe that could be combined into one condition? It's kinda hard to read
either way though. I was thinking of making it positive, but then the
ERFKILL
> if (rfkill_blocked(rdev->rfkill))
> return notifier_from_errno(-ERFKILL);
would have to be _before_ it, and I'm not sure we want to change that.
So maybe make it
if (!(interface_modes & BIT() ||
(iftype == AP_VLAN && use_4addr && wiphy.flags & 4ADDR_AP)))
return ...(-EOPNOTSUPP);
instead?
I feel that's still easier to read than the double conditional with both
things being negated.
> +++ b/net/wireless/nl80211.c
> @@ -3383,8 +3383,7 @@ static int nl80211_new_interface(struct sk_buff *skb,
> struct genl_info *info)
> if (info->attrs[NL80211_ATTR_IFTYPE])
> type = nla_get_u32(info->attrs[NL80211_ATTR_IFTYPE]);
>
> - if (!rdev->ops->add_virtual_intf ||
> - !(rdev->wiphy.interface_modes & (1 << type)))
> + if (!rdev->ops->add_virtual_intf)
> return -EOPNOTSUPP;
>
> if ((type == NL80211_IFTYPE_P2P_DEVICE || type == NL80211_IFTYPE_NAN ||
> @@ -3403,6 +3402,13 @@ static int nl80211_new_interface(struct sk_buff *skb,
> struct genl_info *info)
> return err;
> }
>
> + if (!(rdev->wiphy.interface_modes & (1 << type))) {
> + if (!(type == NL80211_IFTYPE_AP_VLAN &&
> + rdev->wiphy.flags & WIPHY_FLAG_4ADDR_AP &&
> + params.use_4addr))
> + return -EOPNOTSUPP;
> + }
I'm not sure you should insert this further down, rather tahn at the
place where the check was before? Why not just insert this part directly
after the piece you modified above?
johannes
Re: rt2800 tx frame dropping issue.
On Tue, 2018-11-20 at 15:20 -0600, Daniel Santos wrote: > I believe I have the answer as to why we're getting frames after we stop > the queue. I had a little chat about this in #kernelnewbies and some > other devs believe it is intentional. > > There is a race in ieee80211_tx_frags (net/mac80211/tx.c) between > releasing local->queue_stop_reason_lock and calling the driver's tx > until we lock queue->tx_lock in rt2x00queue_write_tx_frame -- in between > these two points the thread can be preempted. So while we stop the > queue in one thread, there can be 20 other threads in between that have > already checked that the queue was running and have a frame buffer > sitting on their stacks. Not 20, only 1 per netdev queue. I suspect that means just 1 per hardware queue, but I don't know how rt2x00 maps netdev queues to hardware queues. If you have lots of netdevs, that might actually be 20, but I suspect that's not what's going on here. > I think it could be fixed with the below > patch, but I'm being told that it doesn't need it and that the driver > should just *quietly* drop the frame: [snip patch] > Could anybody illuminate for me why this isn't done? I know that there > has to be a point where we realize there are too many items in the queue > and we can't keep up, but this would seem to be a terrible way to do > that. Is it one of those things where it isn't worth the performance > degradation? Any insights would be most appreciated!! :) There's just not much point, and doing the locking here will mean it's across _all_ queues, whereas if you have multiple hardware queues you wouldn't really need it. Note that with internal TXQs with fq/codel support etc. we actually have the fq->lock and it's global, and it turns out that's still a better deal than actually doing parallel TX. So this may not matter so much. In any case, while this might solve the problem for the specific case you're thinking about, as soon as you have something else starting or stopping queues from outside the TX path it still wouldn't actually help. By the way, one thing that can likely exacerbate the problem is fragmentation, once you have that you're more likely to trigger lots of frames in close succession. What I would suggest doing in the driver is actually stop the queues once a *threshold* is reached, rather than being full. Say if you have 128 entries in the HW queue, you can stop once you reach 120 (you probably don't really need the other 8 places). If you get a 121st frame, then you can still put it on the queue (until you filled 128), but you can also stop the queue again (stopping queues is idempotent). johannes
Re: mac80211_hwsim: multiple antennas
On Wed, 2018-11-21 at 10:09 -0300, Ramon Fontes wrote: > Yes. I thought that it could be related to the number of antennas (or > something else) after seeing this result > (https://bobcopeland.com/blog/2014/09/wmediumd-speed-test/) provided > by wmediumd. I guess you should instead look at the code referenced by Bob in the next post? https://bobcopeland.com/blog/2014/11/functional-bitrate-sim/ johannes
Re: mac80211_hwsim: multiple antennas
On Thu, 2018-11-22 at 09:18 -0300, Ramon Fontes wrote: > > Then I found the Beacon frame TX rate configuration from hostapd.conf. That's really not relevant for the issue at hand though? johannes
Re: mac80211_hwsim: multiple antennas
On Wed, 2018-11-21 at 09:47 -0300, Ramon Fontes wrote: > Okay! Thanks a lot. > > Indeed, the maximum throughput when using only hwsim is in the order of Gbp/s. So I guess the question is where it's going wrong - is it the wmediumd overhead (netlink), some air simulation in wmediumd, etc. johannes
Re: mac80211_hwsim: multiple antennas
On Wed, 2018-11-21 at 09:26 -0300, Ramon Fontes wrote: > Sorry. I meant throughput. Yeah, well. There's no actual medium (access) simulation in hwsim, so the actual throughput you get only depends on the performance of your CPU. 33Mbps seems quite low, but of course I don't know where you're running this. Then again, you say you're using wmediumd, so perhaps that *does* have some sort of medium simulation these days? I didn't think it has it but I may very well be wrong on that. Try pure hwsim (no wmediumd) and see if you get gigabits, if not then your system is probably just too slow. And then I guess you'd have to look at wmediumd in more detail to see if it has some simulations here? I don't know. johannes
Re: wiki: tree labels in patches
On Fri, 2018-11-16 at 10:46 +0200, Kalle Valo wrote: > Yes, I do see your FIX tag in patchwork: > > [ 31] [FIX] brcmfmac: fix reporting support for 160 MHz channels > 2018-11-08 > > But "FIX" is a bit ambigous as not all fixes not go to wireless-drivers, > they can also go to wireless-drivers-next. So I prefer using the release > number (or name of the tree) like this: > > [PATCH 4.20] brcmfmac: fix reporting support for 160 MHz channels FWIW, davem/networking just use [PATCH net] [PATCH net-next] which puts a bit more effort on the submitter but is a bit easier on the maintainer I suppose. Also, not really a problem here, but it would help disambiguate different trees on the same mailing list. I don't really mind either way. johannes
Re: [PATCH v5] iw: ack signal support for tx ack packets
On Mon, 2018-11-12 at 21:56 +0530, Balaji Pothunoori wrote: > Hi Johannes, > > Now that corresponding driver patches are merged, could you please let > me know if anything is left from our end for merging this patch? Oh, I marked this as "changes requested" since I thought the updates we discussed on the kernel side would require changes here (IIRC we talked about changing some names?) If it applies and compiles as is, I guess best if you just resend, since I no longer have it in patchwork and am on vacation this week. johannes
Re: [iw] [patch] Please support CPPFLAGS in Makefile
> > So I guess attachment is fine, but you need to add the s-o-b and a > > commit message in the patch itself. So, actually, that didn't work. I've applied the patch anyway, but in the future better submit patches inline in the email so patchwork picks them up and I can track them (and don't lose them). johannes
Re: [PATCH] mac80211: Improve connection-loss probing.
Hi Ben, Apologies for taking so long to look at this. This looks good to me, but for some reason it doesn't apply on my (mac80211-next) tree, and basically all of the patch fails. I'm not sure I dare try to fix that up, could you resend? johannes
Re: [PATCH] nl80211: Add support to notify radar event info received from STA
On Fri, 2018-10-19 at 14:42 +0530, Sriram R wrote: [...] This looks fine, but I think it would be nice to have some extended netlink error reporting for at least some of these errors: > + dfs_region = reg_get_dfs_region(wiphy); > + if (dfs_region == NL80211_DFS_UNSET) > + return -EINVAL; > + > + err = nl80211_parse_chandef(rdev, info, ); > + if (err) > + return err; > + > + err = cfg80211_chandef_dfs_required(wiphy, , wdev->iftype); > + if (err < 0) > + return err; > + > + if (err == 0) > + return -EINVAL; > + > + /* Do not process this notification if radar is already detected > + * by kernel on this channel > + */ > + if (chandef.chan->dfs_state == NL80211_DFS_UNAVAILABLE) > + return -EINVAL; And maybe that last one should just return 0? johannes
Re: [PATCH 3/3] mac80211: Implement functionality to monitor station's signal stregnth
Oh, umm, that patch is still here ...
I guess we can combine 2 and 3 too.
> + if (sta->rssi_low && bss_conf->enable_beacon) {
> + int last_event =
> + sta->last_rssi_event_value;
> + int sig = -ewma_signal_read(>rx_stats_avg.signal);
> + int low = sta->rssi_low;
> + int high = sta->rssi_high;
This doesn't really support a list, like in patch 2 where you store
sta_info::rssi_tholds?
> + if (sig < low &&
> + (last_event == 0 || last_event >= low)) {
> + sta->last_rssi_event_value = sig;
> + cfg80211_sta_mon_rssi_notify(
> + rx->sdata->dev, sta->addr,
> + NL80211_CQM_RSSI_THRESHOLD_EVENT_LOW,
> + sig, GFP_ATOMIC);
> + rssi_cross = true;
> + } else if (sig > high &&
> +(last_event == 0 || last_event <= high)) {
> + sta->last_rssi_event_value = sig;
> + cfg80211_sta_mon_rssi_notify(
> + rx->sdata->dev, sta->addr,
> + NL80211_CQM_RSSI_THRESHOLD_EVENT_HIGH,
> + sig, GFP_ATOMIC);
> + rssi_cross = true;
> + }
> + }
> +
> + if (rssi_cross) {
> + ieee80211_update_rssi_config(sta);
> + rssi_cross = false;
> + }
> +}
Ah, but it does, just hard to understand.
However, this does mean what I suspected on the other patch is true -
you're calling ieee80211_update_rssi_config() here, and that uses the
sta->rssi_tholds array without any protection, while a concurrent change
of configuration can free the data.
You need to sort that out. I would suggest to stick all the sta->rssi_*
fields you add into a new struct (you even had an empty one), and
protect that struct using RCU. That also saves the memory in case it's
not assigned at all. Something like
struct sta_mon_rssi_config {
struct rcu_head rcu_head;
int n_thresholds;
s32 low, high;
u32 hyst;
s32 last_value;
s32 thresholds[];
};
then you can kfree_rcu() it, and just do a single allocation using
struct_size() for however many entries you need.
> + * @count_rx_signal: Number of data frames used in averaging station signal.
> + * This can be used to avoid generating less reliable station rssi cross
> + * events that would be based only on couple of received frames.
> */
> struct sta_info {
> /* General information, mostly static */
> @@ -600,6 +603,7 @@ struct sta_info {
> s32 rssi_high;
> u32 rssi_hyst;
> s32 last_rssi_event_value;
> + unsigned int count_rx_signal;
I guess that would also move into the new struct.
johannes
Re: [PATCH 2/3] mac80211: Implement API to configure station specific rssi threshold
On Mon, 2018-10-15 at 23:27 +0530, Tamizh chelvam wrote:
>
> + sta_mon_rssi_config_free(sta);
> + sta->rssi_hyst = rssi_hyst;
> + if (fixed_thold) {
> + if (n_rssi_tholds > 2) {
> + ret = -EINVAL;
> + goto out;
> + }
This might be slightly wrong, you free and then can still return an
error.
> + if (n_rssi_tholds == 1) {
> + sta->rssi_low = rssi_tholds[0];
> + sta->rssi_high = rssi_tholds[0];
> + } else {
> + sta->rssi_low = rssi_tholds[0];
> + sta->rssi_high = rssi_tholds[1];
> + }
> + } else {
> + const s32 *rssi_tholds;
> +
> + rssi_tholds = kmemdup(rssi_tholds,
> + n_rssi_tholds * sizeof(s32),
> + GFP_KERNEL);
> + if (!rssi_tholds) {
> + ret = -ENOMEM;
> + goto out;
> + }
Similarly here, I guess you should do the allocation (and other error
checking) before freeing.
> + sta->rssi_tholds = rssi_tholds;
> + sta->n_rssi_tholds = n_rssi_tholds;
> + ieee80211_update_rssi_config(sta);
> +struct sta_mon_rssi_config {
> +};
Huh?
The commit log also makes it sounds like mac80211 actually *supports*
this, but clearly that's not the case. However you don't give any data
to the driver either, did you lose a patch along the way? Previously you
had patch 5 ("mac80211: Implement functionality to monitor station's
rssi cross event") and if I remember correctly I said you should squash
some, but now that's not here?
johannes
Re: [PATCH 1/3] cfg80211: Add support to configure station specific RSSI threshold for AP mode
Hi,
Sorry for the delay in reviewing this.
> + int (*set_sta_mon_rssi_config)(struct wiphy *wiphy,
> + struct net_device *dev,
> + const u8 *addr,
> + const s32 *rssi_tholds,
> + u32 rssi_hyst, int rssi_n_tholds,
> + bool fixed_thold);
> };
I think it might be better to pass all the last 4 arguments (rssi
related ones) as a struct? That's a pattern we typically have elsewhere
too, and it makes things easier to extend and also easier to pass
around.
> + * @NL80211_CMD_SET_STA_MON: This command is used to configure station's
> + * connection monitoring notification trigger levels.
> + * @NL80211_CMD_NOTIFY_STA_MON: This is used as an event to notify
> + * the user space that a trigger level was reached for a station.
Please describe the attributes to use with this.
> + * @NL80211_ATTR_STA_MON_FIXED_THOLD: Flag attribute is used with
> + * %NL80211_CMD_SET_STA_MON to indicate driver that the monitoring
> + * configuration is fixed limit or a moving range threshold.
This isn't really clear to me, what does it mean?
> + if (!sta_mon || !(info->attrs[NL80211_ATTR_MAC]))
Don't really need the parentheses in !(info->...)
> + err = nla_parse_nested(attrs, NL80211_ATTR_CQM_MAX, sta_mon,
> +nl80211_attr_cqm_policy, info->extack);
I *think* I made that a proper nested policy, check and then you can
remove passing it here.
> +void
> +cfg80211_sta_mon_rssi_notify(struct net_device *dev, const u8 *peer,
> + enum nl80211_cqm_rssi_threshold_event rssi_event,
> + s32 rssi_level, gfp_t gfp)
> +{
> + struct sk_buff *msg;
> +
> + if (WARN_ON(!peer))
> + return;
Tracing for this might be nice too?
johannes
Re: [PATCH] mac80211: allow hardware scan to fall back to software
On Fri, 2018-11-09 at 11:48 +0530, Siva Rebbagondla wrote: > Hi, > Gentle Remainder..!!!. > Any update required for this patch?. If not, When can i expect this > patch to be available in wireless-next?. Sorry, I hadn't been merging things for a while due to the merge window. I just put it into mac80211-next, so I guess the answer should be "soon". johannes
Re: [iw] [patch] Please support CPPFLAGS in Makefile
On Thu, 2018-11-08 at 21:26 +0100, Johannes Berg wrote: > Hi, > > > The attached patch adds support for CPPFLAGS to iw's Makefile. > > Please send the patch as plain text (not attachment), and with signed- > off-by per the CONTRIBUTING file. Actually, it looks like patchwork knows how to read attachments? :) So I guess attachment is fine, but you need to add the s-o-b and a commit message in the patch itself. johannes
Re: [iw] [patch] Please support CPPFLAGS in Makefile
Hi, > The attached patch adds support for CPPFLAGS to iw's Makefile. Please send the patch as plain text (not attachment), and with signed- off-by per the CONTRIBUTING file. Thanks, johannes
Re: [RFC 0/3] cfg80211/nl80211/iw: add basic AMPDU/AMSDU controls
Hi, > Here are several RFC patches providing simple high-level controls of > AMSDU/AMPDU > aggregation. The primary purpose of this functionality is an attempt to fill > missing gaps in nl80211 interface for basic WFA certification tests. I see you don't implement it this way in the driver, but wouldn't it make more sense to have this as a per-STA (RA) setting? That's really the granularity it can be done on, I think? Arguably even per-RA/TID, though that seems a little excessive? > We experimented with QCA sigma-dut tool: https://github.com/qca/sigma-dut. > The purpose was to cover basic HT/VHT WFA STA tests for cfg80211 driver > controlled by wpa_supplicant w/o adding any vendor specific commands. > Multiple WFA test parameters (e.g. STBC, NSS, SGI, LDPC) can be configured > by overriding HT/VHT capabilities in wpa_supplicant and applying them on > connect in cfg80211_connect callback. Others (e.g. RTS params) can be > configured using iw tool or NL80211_CMD_SET_WIPHY directly. These patches > implement simpe high-level switches for AMSDU/AMPDU aggregation. > > It would be interesting to collect comments/concerns regarding this approach. > Does it make sense to enhance nl80211 in order to cover all the missing pieces > required for WFA certification tests ? Or maybe it makes sense to use > NL80211_TESTMODE subcommands for this kind of testing. Honestly, I don't really know. I think other tests, e.g. noack, we used to just have debugfs files for, and then we got NL80211_CMD_SET_NOACK_MAP (which *is* per TID, btw) Perhaps if we really don't see any value beyond the testing, keeping it in debugfs would make sense, until we have more use cases and understand the granularity we should support? Clearly this is enough for the testing you refer to, but other use cases might actually need more fine-grained control (in the future), possibly down to the RA/TID level? johannes
[PATCH 1/2] cfg80211: add peer measurement with FTM initiator API
From: Johannes Berg
Add a new "peer measurement" API, that can be used to measure
certain things related to a peer. Right now, only implement
FTM (flight time measurement) over it, but the idea is that
it'll be extensible to also support measuring the necessary
things to calculate e.g. angle-of-arrival for WiGig.
The API is structured to have a generic list of peers and
channels to measure with/on, and then for each of those a
set of measurements (again, only FTM right now) to perform.
Results are sent to the requesting socket, including a final
complete message.
Closing the controlling netlink socket will abort a running
measurement.
Signed-off-by: Johannes Berg
---
v3:
- add a bit to report "final" for partial results
- remove list keeping etc. and just unicast out the results
to the requester (big code reduction ...)
- also send complete message unicast, and as a result
remove the multicast group
- separate out struct cfg80211_pmsr_ftm_request_peer
from struct cfg80211_pmsr_request_peer
- document timeout == 0 if no timeout
- disallow setting timeout nl80211 attribute to 0,
must not include attribute for no timeout
- make MAC address randomization optional
- change num bursts exponent default to 0 (1 burst, rather
rather than the old default of 15==don't care)
v4:
- clarify NL80211_ATTR_TIMEOUT documentation
v5:
- remove unnecessary nl80211 multicast/family changes
- remove partial results bit/flag, final is sufficient
- add max_bursts_exponent, max_ftms_per_burst to capability
- rename "frames per burst" -> "FTMs per burst"
v6:
- rename cfg80211_pmsr_free_wdev() to cfg80211_pmsr_wdev_down()
and call it in leave, so the device can't go down with any
pending measurements
v7:
- wording fixes (Lior)
- fix ftm.max_bursts_exponent to allow having the limit of 0 (Lior)
---
include/net/cfg80211.h | 261
include/uapi/linux/nl80211.h | 417 +
net/wireless/Makefile| 1 +
net/wireless/core.c | 33 ++
net/wireless/core.h | 4 +
net/wireless/nl80211.c | 192 ++--
net/wireless/nl80211.h | 28 ++
net/wireless/pmsr.c | 590 +++
net/wireless/rdev-ops.h | 25 ++
net/wireless/trace.h | 68
10 files changed, 1600 insertions(+), 19 deletions(-)
create mode 100644 net/wireless/pmsr.c
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 1fa41b7a1be3..8d8c4682fd89 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2848,6 +2848,190 @@ struct cfg80211_ftm_responder_stats {
u32 out_of_window_triggers_num;
};
+/**
+ * struct cfg80211_pmsr_ftm_result - FTM result
+ * @failure_reason: if this measurement failed (PMSR status is
+ * %NL80211_PMSR_STATUS_FAILURE), this gives a more precise
+ * reason than just "failure"
+ * @burst_index: if reporting partial results, this is the index
+ * in [0 .. num_bursts-1] of the burst that's being reported
+ * @num_ftmr_attempts: number of FTM request frames transmitted
+ * @num_ftmr_successes: number of FTM request frames acked
+ * @busy_retry_time: if failure_reason is %NL80211_PMSR_FTM_FAILURE_PEER_BUSY,
+ * fill this to indicate in how many seconds a retry is deemed possible
+ * by the responder
+ * @num_bursts_exp: actual number of bursts exponent negotiated
+ * @burst_duration: actual burst duration negotiated
+ * @ftms_per_burst: actual FTMs per burst negotiated
+ * @lci_len: length of LCI information (if present)
+ * @civicloc_len: length of civic location information (if present)
+ * @lci: LCI data (may be %NULL)
+ * @civicloc: civic location data (may be %NULL)
+ * @rssi_avg: average RSSI over FTM action frames reported
+ * @rssi_spread: spread of the RSSI over FTM action frames reported
+ * @tx_rate: bitrate for transmitted FTM action frame response
+ * @rx_rate: bitrate of received FTM action frame
+ * @rtt_avg: average of RTTs measured (must have either this or @dist_avg)
+ * @rtt_variance: variance of RTTs measured (note that standard deviation is
+ * the square root of the variance)
+ * @rtt_spread: spread of the RTTs measured
+ * @dist_avg: average of distances (mm) measured
+ * (must have either this or @rtt_avg)
+ * @dist_variance: variance of distances measured (see also @rtt_variance)
+ * @dist_spread: spread of distances measured (see also @rtt_spread)
+ * @num_ftmr_attempts_valid: @num_ftmr_attempts is valid
+ * @num_ftmr_successes_valid: @num_ftmr_successes is valid
+ * @rssi_avg_valid: @rssi_avg is valid
+ * @rssi_spread_valid: @rssi_spread is valid
+ * @tx_rate_valid: @tx_rate is valid
+ * @rx_rate_valid: @rx_rate is valid
+ * @rtt_avg_valid: @rtt_avg is valid
+ * @rtt_variance_valid: @rtt_variance is valid
+ * @rtt_spread_valid: @rtt_spread is valid
+ * @dist_avg_valid: @dist_avg is valid
+ * @dist_variance_valid: @dist_variance i
[PATCH 2/2] mac80211: allow drivers to use peer measurement API
From: Johannes Berg
There's nothing much for mac80211 to do, so only pass through
the requests with minimal checks and tracing. The driver must
call cfg80211's results APIs.
Signed-off-by: Johannes Berg
---
v4: first version - just takes the same number as the cfg80211 patch
v5: no changes
v6: no changes
v7: no changes
---
include/net/mac80211.h| 7 +++
net/mac80211/cfg.c| 22 ++
net/mac80211/driver-ops.h | 34 ++
net/mac80211/trace.h | 12
4 files changed, 75 insertions(+)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 71985e95d2d9..e3d57e7a55cc 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3623,6 +3623,9 @@ enum ieee80211_reconfig_type {
* skb is always a real frame, head may or may not be an A-MSDU.
* @get_ftm_responder_stats: Retrieve FTM responder statistics, if available.
* Statistics should be cumulative, currently no way to reset is provided.
+ *
+ * @start_pmsr: start peer measurement (e.g. FTM) (this call can sleep)
+ * @abort_pmsr: abort peer measurement (this call can sleep)
*/
struct ieee80211_ops {
void (*tx)(struct ieee80211_hw *hw,
@@ -3911,6 +3914,10 @@ struct ieee80211_ops {
int (*get_ftm_responder_stats)(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
struct cfg80211_ftm_responder_stats
*ftm_stats);
+ int (*start_pmsr)(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+ struct cfg80211_pmsr_request *request);
+ void (*abort_pmsr)(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+ struct cfg80211_pmsr_request *request);
};
/**
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 51622333d460..2ffbbf4d 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3849,6 +3849,26 @@ ieee80211_get_ftm_responder_stats(struct wiphy *wiphy,
return drv_get_ftm_responder_stats(local, sdata, ftm_stats);
}
+static int
+ieee80211_start_pmsr(struct wiphy *wiphy, struct wireless_dev *dev,
+struct cfg80211_pmsr_request *request)
+{
+ struct ieee80211_local *local = wiphy_priv(wiphy);
+ struct ieee80211_sub_if_data *sdata = IEEE80211_WDEV_TO_SUB_IF(dev);
+
+ return drv_start_pmsr(local, sdata, request);
+}
+
+static void
+ieee80211_abort_pmsr(struct wiphy *wiphy, struct wireless_dev *dev,
+struct cfg80211_pmsr_request *request)
+{
+ struct ieee80211_local *local = wiphy_priv(wiphy);
+ struct ieee80211_sub_if_data *sdata = IEEE80211_WDEV_TO_SUB_IF(dev);
+
+ return drv_abort_pmsr(local, sdata, request);
+}
+
const struct cfg80211_ops mac80211_config_ops = {
.add_virtual_intf = ieee80211_add_iface,
.del_virtual_intf = ieee80211_del_iface,
@@ -3944,4 +3964,6 @@ const struct cfg80211_ops mac80211_config_ops = {
.tx_control_port = ieee80211_tx_control_port,
.get_txq_stats = ieee80211_get_txq_stats,
.get_ftm_responder_stats = ieee80211_get_ftm_responder_stats,
+ .start_pmsr = ieee80211_start_pmsr,
+ .abort_pmsr = ieee80211_abort_pmsr,
};
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 0b1747a2313d..3e0d5922a440 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1199,6 +1199,40 @@ drv_get_ftm_responder_stats(struct ieee80211_local
*local,
return ret;
}
+static inline int drv_start_pmsr(struct ieee80211_local *local,
+struct ieee80211_sub_if_data *sdata,
+struct cfg80211_pmsr_request *request)
+{
+ int ret = -EOPNOTSUPP;
+
+ might_sleep();
+ if (!check_sdata_in_driver(sdata))
+ return -EIO;
+
+ trace_drv_start_pmsr(local, sdata);
+
+ if (local->ops->start_pmsr)
+ ret = local->ops->start_pmsr(>hw, >vif, request);
+ trace_drv_return_int(local, ret);
+
+ return ret;
+}
+
+static inline void drv_abort_pmsr(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ struct cfg80211_pmsr_request *request)
+{
+ trace_drv_abort_pmsr(local, sdata);
+
+ might_sleep();
+ if (!check_sdata_in_driver(sdata))
+ return;
+
+ if (local->ops->abort_pmsr)
+ local->ops->abort_pmsr(>hw, >vif, request);
+ trace_drv_return_void(local);
+}
+
static inline int drv_start_nan(struct ieee80211_local *local,
struct ieee80211_sub_if_data *sdata,
struct cfg80211_nan_conf *conf)
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index 588c51a67c89..ac2f1922d469 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -1882,6 +188
Re: [PATCH v6 1/2] cfg80211: add peer measurement with FTM initiator API
Hi, > > > > + * @ftm.max_bursts_exponent: maximum burst exponent supported > > + * (set to 0 if not limited; note that setting this will necessarily > > + * forbid using the value 15 to let the responder pick) > > How can driver specify it only supports a single burst? (This is what our > internal wil6210 implementation currently supports...) Hah, good catch. I'll make it unsigned and have -1 the "don't care"... That's annoying though. Perhaps then we'll have a few drivers that support only 1 if they should have more (or arbitrary), but that's a better bug ... johannes
[PATCH 2/2] mac80211: fix CSA beacon allocation size
From: Johannes Berg
If the FTM responder settings are changed simultaneously with
the CSA beacon, the buffer size allocated isn't sufficient and
we'll have a heap overrun. Fix this.
While at it, also clean up the ftm_responder assignment, doing
it only if ftm_responder is non-zero is valid as it's 0 to start
with, but not really useful to understand the code.
Fixes: bc847970f432 ("mac80211: support FTM responder configuration/statistics")
Signed-off-by: Johannes Berg
---
net/mac80211/cfg.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 51622333d460..818aa0060349 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2891,7 +2891,7 @@ cfg80211_beacon_dup(struct cfg80211_beacon_data *beacon)
len = beacon->head_len + beacon->tail_len + beacon->beacon_ies_len +
beacon->proberesp_ies_len + beacon->assocresp_ies_len +
- beacon->probe_resp_len;
+ beacon->probe_resp_len + beacon->lci_len + beacon->civicloc_len;
new_beacon = kzalloc(sizeof(*new_beacon) + len, GFP_KERNEL);
if (!new_beacon)
@@ -2934,8 +2934,9 @@ cfg80211_beacon_dup(struct cfg80211_beacon_data *beacon)
memcpy(pos, beacon->probe_resp, beacon->probe_resp_len);
pos += beacon->probe_resp_len;
}
- if (beacon->ftm_responder)
- new_beacon->ftm_responder = beacon->ftm_responder;
+
+ /* might copy -1, meaning no changes requested */
+ new_beacon->ftm_responder = beacon->ftm_responder;
if (beacon->lci) {
new_beacon->lci_len = beacon->lci_len;
new_beacon->lci = pos;
--
2.17.2
[PATCH 1/2] cfg80211/mac80211: fix FTM settings across CSA
From: Johannes Berg
When FTM is enabled, doing a CSA will unexpectedly lose it since
the value of ftm_responder may be initialized to 0 instead of -1,
so fix that.
Fixes: 81e54d08d9d8 ("cfg80211: support FTM responder configuration/statistics")
Signed-off-by: Johannes Berg
---
net/wireless/nl80211.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 744b5851bbf9..8d763725498c 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -7870,6 +7870,7 @@ static int nl80211_channel_switch(struct sk_buff *skb,
struct genl_info *info)
}
memset(, 0, sizeof(params));
+ params.beacon_csa.ftm_responder = -1;
if (!info->attrs[NL80211_ATTR_WIPHY_FREQ] ||
!info->attrs[NL80211_ATTR_CH_SWITCH_COUNT])
--
2.17.2
Re: [PATCH 04/19] wilc: add host_interface.c
On Mon, 2018-10-29 at 21:32 +, adham.aboza...@microchip.com wrote: > > Correct. The speed of executing the work will be the same in both > cases (maybe a little slower in case of deferring the work) > The intuition here was to do minimum work in the cfg's context, but > since this isn't a concern, I can skip deferring the work. It sort of depends. Practically all of this is done with the RTNL held, so you might be blocking _other_ things that need the RTNL. I'm not sure if that matters that much, but still ... Regardless, I'd try this first. Perhaps you can even see later that only some commands take a long time, and others are quicker. Having wpa_supplicant/hostapd not know when an operation is really completed is unlikely to be a good idea, IMHO. johannes
Re: [PATCH 12/19] wilc: add wilc_wfi_cfgoperations.c
Hi, Sorry for the late reply. On Fri, 2018-10-19 at 21:47 +, adham.aboza...@microchip.com wrote: > > Johannes, shadow buffer has 2 more usage that I missed in my first email: > 1- It keeps a copy of scan results to be able to auto-select from if > the cfg80211 didn't supply a specific bssid in cfg's connect request > (struct cfg80211_connect_params). > In this case the driver will select a network that matches the ssid, > while having the highest rssi. You should be able to find this from cfg80211's BSS list as well. If we lack some API in this area, which is possible, then we can add it. > 2- It keeps network parameters that the device will need to connect > to a network, since the device doesn't keep the scan results > internally. > These parameters are stored in struct join_bss_param, and passed to > the device when a connect request is received. > Some of these parameters can be extracted from cfg's > cfg80211_connect_params (like cap_info.. etc), but others (like bssid, > beacon period.. etc) are still required. Again though, you should be able to extract these from struct cfg80211_bss, I'd argue? johannes
Re: [PATCH 04/19] wilc: add host_interface.c
> > I'd argue for trying it though as it makes the code MUCH simpler. > > I totally agree that it will be simpler to do the work directly from > the cfg context rather than scheduling it. > On a cortex A5 MPU and SPI bus running on 48Mhz, this takes 20ms in > the idle case, and 100 to 300ms in case of data being transferred in > parallel. That does sound pretty slow ... but OTOH, mostly wpa_s etc. would expect this to actually complete before the next step, so if this is slow and wpa_s/hostapd is much faster, it might send you a lot of things to do and so you'd end up being slow anyway? johannes
Re: [PATCH] mac80211: support FTM responder configuration/statistics
On Mon, 2018-10-29 at 11:59 -0700, Pradeep Kumar Chitrapu wrote:
> > memset(, 0, sizeof(params));
> > + params.beacon_csa.ftm_responder = -1;
>
> Hi Johannes,
>
> Agree with the rest, however, I think this may not be needed because
> ftm_responder is already being set to -1,
> if NL80211_ATTR_FTM_RESPONDER attribute is not included, in
> nl80211_parse_beacon, which will be called from
> nl80211_channel_switch. Please let me know if I may have missed
> something..
>
> if (attrs[NL80211_ATTR_FTM_RESPONDER]) {
>
> } else {
> bcn->ftm_responder = -1;
> }
>
Yes, but we may not get there?
if (!need_new_beacon)
goto skip_beacons;
err = nl80211_parse_beacon(rdev, info->attrs, _after);
[...]
skip_beacons:
[...]
err = rdev_channel_switch(rdev, dev, );
johannes
Re: [PATCH] mac80211: support FTM responder configuration/statistics
Hi,
Actually, I think my suggestion _would_ in fact fix the whole issue.
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index 51622333d460..70d6de29425b 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -2934,19 +2934,20 @@ static int
> ieee80211_start_radar_detection(struct wiphy *wiphy,
> memcpy(pos, beacon->probe_resp, beacon->probe_resp_len);
> pos += beacon->probe_resp_len;
> }
> - if (beacon->ftm_responder)
> + if (beacon->ftm_responder != -1) {
This doesn't make sense without the change I suggested, since if we
don't do the change I suggested, beacon->ftm_responder will never
actually be -1.
I'd change it like this, fixing the memory overflow bug along the way:
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 51622333d460..818aa0060349 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2891,7 +2891,7 @@ cfg80211_beacon_dup(struct cfg80211_beacon_data *beacon)
len = beacon->head_len + beacon->tail_len + beacon->beacon_ies_len +
beacon->proberesp_ies_len + beacon->assocresp_ies_len +
- beacon->probe_resp_len;
+ beacon->probe_resp_len + beacon->lci_len + beacon->civicloc_len;
new_beacon = kzalloc(sizeof(*new_beacon) + len, GFP_KERNEL);
if (!new_beacon)
@@ -2934,8 +2934,9 @@ cfg80211_beacon_dup(struct cfg80211_beacon_data *beacon)
memcpy(pos, beacon->probe_resp, beacon->probe_resp_len);
pos += beacon->probe_resp_len;
}
- if (beacon->ftm_responder)
- new_beacon->ftm_responder = beacon->ftm_responder;
+
+ /* might copy -1, meaning no changes requested */
+ new_beacon->ftm_responder = beacon->ftm_responder;
if (beacon->lci) {
new_beacon->lci_len = beacon->lci_len;
new_beacon->lci = pos;
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 744b5851bbf9..8d763725498c 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -7870,6 +7870,7 @@ static int nl80211_channel_switch(struct sk_buff *skb,
struct genl_info *info)
}
memset(, 0, sizeof(params));
+ params.beacon_csa.ftm_responder = -1;
if (!info->attrs[NL80211_ATTR_WIPHY_FREQ] ||
!info->attrs[NL80211_ATTR_CH_SWITCH_COUNT])
If that seems good to you I'll submit the patch.
johannes
Re: [PATCH] mac80211: support FTM responder configuration/statistics
Hi Pradeep, On Wed, 2018-10-03 at 20:19 -0700, Pradeep Kumar Chitrapu wrote: > New bss param ftm_responder is used to notify the driver to > enable fine timing request (FTM) responder role in AP mode. I just realized that this is broken in nl80211_channel_switch() and ieee80211_set_csa_beacon(), doing a CSA will always disable FTM unless it was actually included in the new configuration. Doing the trivial thing: memset(, 0, sizeof(params)); + params.beacon_after.ftm_responder = -1; in nl80211_channel_switch() will not help because then mac80211 will lose all the extra configuration, and will actually store -1 into its enabled value which is really strange. I'd appreciate if you could take a look at this. Thanks, johannes
Re: [PATCH] iwlwifi: pcie: align licensing to dual GPL/BSD
On Wed, 2018-10-24 at 09:33 +0200, Sedat Dilek wrote: > > > Align the licenses with their permission to clean up and to > > make it all identical. > > > > Does it make sense to put the BSD license (text) in a separate file > (like GPL) and reference it? I agree that it would, and in fact we already have it available to reference with the SPDX in the LICENSES directory. However, no files of the iwlwifi driver currently do that, and I'd prefer doing that sort of cleanup more generally as a separate step. johannes
[PATCH] iwlwifi: pcie: align licensing to dual GPL/BSD
From: Johannes Berg
These files have a long history of code changes, but analysing
the remaining code leads to having only a few changes that are
not already owned by Intel, notably from
- Andy Lutomirski
- Joonwoo Park
- Kirtika Ruchandani
- Rajat Jain
- Stanislaw Gruszka
remaining in the code today.
Note that
- I myself was working for Intel and for any possibly code
that might be before my employment there give permission
- Wizery employees were working for Intel
More specifically, we identified the following commits that
(partially may) remain today:
25c03d8e8c13 Joonwoo Park ("iwlwifi: do not
schedule tasklet when rcv unused irq")
f36d04abe684 Stanislaw Gruszka("iwlwifi: use
dma_alloc_coherent")
387f3381f732 Stanislaw Gruszka("iwlwifi: fix dma
mappings and skbs leak")
2624e96ce16b Stanislaw Gruszka("iwlwifi: fix possible
data overwrite in hcmd callback")
bfe4b80e9f73 Stanislaw Gruszka("iwlwifi: always check
if got h/w access before write")
d536c32b45d2 Andy Lutomirski ("iwlwifi: pcie: log
when waking the NIC for hcmd submission fails")
a6d24fad00d9 Rajat Jain("iwlwifi: pcie: dump
registers when HW becomes inaccessible")
fb12777ab59b Kirtika Ruchandani ("iwlwifi: Add more
call-sites for pcie reg dumper")
3a73a30049f2 Stanislaw Gruszka("iwlwifi: cleanup/fix
memory barriers")
aa5affbacb24 Stanislaw Gruszka("iwlwifi: dump stack
when fail to gain access to the device")
Align the licenses with their permission to clean up and to
make it all identical.
CC: Joonwoo Park
CC: Stanislaw Gruszka
CC: Andy Lutomirski
CC: Rajat Jain
CC: Kirtika Ruchandani
Acked-by: Johannes Berg
Signed-off-by: Johannes Berg
---
I'd appreciate if you (in CC lines) could provide your acked-by here.
---
drivers/net/wireless/intel/iwlwifi/iwl-io.c | 41 +++--
drivers/net/wireless/intel/iwlwifi/iwl-io.h | 38 ++--
.../wireless/intel/iwlwifi/pcie/internal.h| 44 +--
drivers/net/wireless/intel/iwlwifi/pcie/rx.c | 44 +--
drivers/net/wireless/intel/iwlwifi/pcie/tx.c | 44 +--
5 files changed, 192 insertions(+), 19 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-io.c
b/drivers/net/wireless/intel/iwlwifi/iwl-io.c
index 4f10914f6048..ffd1e649bfa0 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-io.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-io.c
@@ -1,10 +1,13 @@
/**
+ *
+ * This file is provided under a dual BSD/GPLv2 license. When using or
+ * redistributing this file, you may do so under either license.
+ *
+ * GPL LICENSE SUMMARY
*
* Copyright(c) 2003 - 2014 Intel Corporation. All rights reserved.
* Copyright(c) 2015 - 2016 Intel Deutschland GmbH
*
- * Portions of this file are derived from the ipw3945 project.
- *
* This program is free software; you can redistribute it and/or modify it
* under the terms of version 2 of the GNU General Public License as
* published by the Free Software Foundation.
@@ -15,12 +18,44 @@
* more details.
*
* The full GNU General Public License is included in this distribution in the
- * file called LICENSE.
+ * file called COPYING.
*
* Contact Information:
* Intel Linux Wireless
* Intel Corporation, 5200 N.E. Elam Young Parkway, Hillsboro, OR 97124-6497
*
+ * BSD LICENSE
+ *
+ * Copyright(c) 2003 - 2014 Intel Corporation. All rights reserved.
+ * Copyright(c) 2015 - 2016 Intel Deutschland GmbH
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ *notice, this list of conditions and the following disclaimer.
+ * * Redistributions in binary form must reproduce the above copyright
+ *notice, this list of conditions and the following disclaimer in
+ *the documentation and/or other materials provided with the
+ *distribution.
+ * * Neither the name Intel Corporation nor the names of its
+ *contributors may be used to endorse or promote products derived
+ *from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSE
Re: [PATCH v2] wireless: mark expected switch fall-throughs
On Tue, 2018-10-23 at 12:58 +0200, Gustavo A. R. Silva wrote: > On 10/23/18 10:59 AM, Gustavo A. R. Silva wrote: > > > > On 10/23/18 9:01 AM, Johannes Berg wrote: > > > On Tue, 2018-10-23 at 02:13 +0200, Gustavo A. R. Silva wrote: > > > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases > > > > where we are expecting to fall through. > > > > > > > > Warning level 3 was used: -Wimplicit-fallthrough=3 > > > > > > > > This code was not tested and GCC 7.2.0 was used to compile it. > > > > > > Look, I'm not going to make this any clearer: I'm not applying patches > > > like that where you've invested no effort whatsoever on verifying that > > > they're correct. > > > > > > > How do you suggest me to verify that every part is correct in this type > > of patches? > > > > BTW... I'm under the impression you think that I don't even look at > the code. Is that correct? That's what your commit log looks like, yes. This is your full commit log: In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Warning level 3 was used: -Wimplicit-fallthrough=3 This code was not tested and GCC 7.2.0 was used to compile it. For all I know, you could've run spatch to add the comments wherever there was no break, and then compiled it once. > I've been working on this for quite a while, and in every case I try to > understand the code in terms of the context in which every warning is > reported. That's great. > Here is a bug I found yesterday at > drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > > 5690 case WLAN_CIPHER_SUITE_CCMP: > 5691 key->flags |= IEEE80211_KEY_FLAG_SW_MGMT_TX; > 5692 break; > 5693 case WLAN_CIPHER_SUITE_TKIP: > 5694 key->flags |= IEEE80211_KEY_FLAG_GENERATE_MMIC; > 5695 default: > 5696 return -EOPNOTSUPP; > 5697 } Indeed, that looks like a bug, although kinda benign since it just means TKIP will always use software crypto and TKIP is slow anyway ;-) > I do this analysis for every warning. Now, when I say I haven't tested the > code > is because I don't have any log as evidence for anything. Not that I haven't > put > any effort on trying to understand it and its context. When I started > working on > this task there were more than 2000 of these issues, now there are around 600 > left. > > I have fixed many bugs on the way, so a good amount of work is being invested > on > this, and it's paying off. :) :-) > Now, let me ask you this question: > > It would be easier for you to review this patch if I turn it into a series? > > I can do that without a problem. I'd be happy if you were to actually just mention that you've looked at them, and found them to be expected fall throughs. I'll still review them, but without that information I feel like I'm doing the first round of reviews this code ever got. johannes
Re: mac80211 keeps trying to start ampdu session??
On Tue, 2018-10-23 at 15:22 +0300, Ali MJ Al-Nasrawy wrote: > > This depends on the rate control algorithm, it triggers this. > > In this case, it is minstrel_ht. > > > > > I think - perhaps depending on the return value - mac80211 *does* give > > up eventually, but not really sure. > > The return value is handled only in > agg-tx.c:ieee80211_tx_ba_session_handle_start and it doesn't pass it > down or recognize difference between non-zero return values. Indeed. > And I don't think that it gives up retrying: I already have 800MB of > compressed logs for the last four days only full of this message and > additionally I ran a simple test for the last hour and found that it > tries to start ampdu session for every frame to be sent with that TID! Indeed, but that does seem excessive. Perhaps minstrel_ht should learn to back off, Felix what do you think? In this case the driver basically says "I don't want to do agg on this TID" and that repeats for every packet. > > Perhaps just remove the message? > > OK, I report this just in case of it being unintended behavior with > probably a negative performance impact. It might actually, yeah ... johannes
Re: Should we check netif_running in cfg80211_calculate_bi_data?
On Tue, 2018-10-23 at 13:22 -0700, Ben Greear wrote:
> On 10/23/2018 12:53 PM, Johannes Berg wrote:
> > On Tue, 2018-10-23 at 12:19 -0700, Ben Greear wrote:
> > >
> > > Oct 23 12:11:05 ben-ota-2.candelatech.com kernel: Assigning beacon-in-gcd
> > > to: 240 from wdev: vap39
> > > Oct 23 12:11:05 ben-ota-2.candelatech.com kernel: beacon-int-diff,
> > > beacon-int-gcd: 240 new-beacon-int: 100
> >
> > This new-beacon-int 100 seems strange and suspicious. Why is it even
> > trying to look at this? Hmm.
> >
> > > Maybe we need to clear beacon-interval back to 0 on admin down of the
> > > wifi dev?
> >
> > We should be doing this, we should end up in __cfg80211_stop_ap() and
> > that does clear it? Hmm... perhaps this _fails_ somehow, and we don't
> > clear it in the error path? I suppose we really should make that
> > unconditional because there's nothing we can do to recover from that
> > error ...
>
> I am suspicious about this... that is not the same memory location as
> wdev->beacon_interval,
> so maybe this is the thing that is not properly cleared?
>
> if (sdata->vif.type == NL80211_IFTYPE_AP ||
> sdata->vif.type == NL80211_IFTYPE_MESH_POINT) {
> /*
>* always passing this is harmless, since it'll be the
>* same value that cfg80211 finds if it finds the same
>* interface ... and that's always allowed
>*/
> params.new_beacon_int = sdata->vif.bss_conf.beacon_int;
> }
Oh, this is mac80211. Yes, that has a different place, and that does
indeed look like it doesn't get cleared? Odd. Perhaps you can try what
happens if you just clear that in ieee80211_stop_ap()?
johannes
Re: Should we check netif_running in cfg80211_calculate_bi_data?
On Tue, 2018-10-23 at 12:19 -0700, Ben Greear wrote: > > Oct 23 12:11:05 ben-ota-2.candelatech.com kernel: Assigning beacon-in-gcd to: > 240 from wdev: vap39 > Oct 23 12:11:05 ben-ota-2.candelatech.com kernel: beacon-int-diff, > beacon-int-gcd: 240 new-beacon-int: 100 This new-beacon-int 100 seems strange and suspicious. Why is it even trying to look at this? Hmm. > Maybe we need to clear beacon-interval back to 0 on admin down of the wifi > dev? We should be doing this, we should end up in __cfg80211_stop_ap() and that does clear it? Hmm... perhaps this _fails_ somehow, and we don't clear it in the error path? I suppose we really should make that unconditional because there's nothing we can do to recover from that error ... johannes
Re: Where to report mpdus tx vs failed?
On Mon, 2018-10-22 at 10:16 -0700, Ben Greear wrote:
> > > We have NL80211_STA_INFO_TX_RETRIES, shouldn't you be able to capture it
> > > there?
> >
> > Or, per TID, NL80211_TID_STATS_TX_MSDU_RETRIES
>
> I added this code (rate is struct ieee80211_tx_rate)
>
> if (tx_done->mpdus_failed) {
> /* Maybe there is a better way to report this tried vs failed
> stat up the stack? */
> rate->count = tx_done->mpdus_failed + 1;
> }
> else {
> rate->count = 1;
> }
Ah, you were asking about mac80211?
I guess if you have the statistic, you can override it in
get_sta_stats() instead of trying to make mac80211 keep them based on
the rate (control) information.
johannes
Re: Should we check netif_running in cfg80211_calculate_bi_data?
On Mon, 2018-10-22 at 13:27 -0700, Ben Greear wrote: > I was testing on my 4.16 kernel with a bunch of VAPs and I had two configured > for 100 beacon interval, and more at 240. This failed for reasons I figured > out > (gcd logic), but even once I tried to configure the vaps for 240 interval they > could not come up. I am thinking maybe it was because I could only > re-configure > admin-up interfaces, and they couldn't come up due the gcd thing? > > Anyway, while poking, I thought maybe the patch below would be helpful since > we shouldn't care about admin-down interfaces in this case? > > diff --git a/net/wireless/util.c b/net/wireless/util.c > index fbc880e..56d7583 100644 > --- a/net/wireless/util.c > +++ b/net/wireless/util.c > @@ -1541,6 +1541,9 @@ static void cfg80211_calculate_bi_data(struct wiphy > *wiphy, u32 new_beacon_int, > if (wdev->beacon_interval == *beacon_int_gcd) > continue; > > + if (!netif_running(wdev->netdev)) > + continue; > + I don't think we'd ever get to this check, since those interfaces will always have wdev->beacon_interval == 0, checked a few lines before this code at the beginning of the loop? At least they should have, but a quick check suggests that is true. johannes
Re: mac80211 keeps trying to start ampdu session??
On Tue, 2018-10-23 at 10:44 +0300, Ali MJ Al-Nasrawy wrote: > Hi, > > I am trying to debug brcmsmac wireless driver for spamming the log with > the message: > [...] START: tid 2 is not agg'able > for it does not support AMPDU aggreggation for TID 2. > > And after quick tracing, I found that mac80211 keeps trying to start > AMPDU session for the _same TID and STA_ despite that the driver returns > non-zero code via its ampdu_action callback and that non-zero return > codes are recognized as the "HW unavailable" for such session > (agg-tx.c:ieee80211_tx_ba_session_handle_start) > > So, Is that an expected behaviour from mac80211 or not?? This depends on the rate control algorithm, it triggers this. I think - perhaps depending on the return value - mac80211 *does* give up eventually, but not really sure. Perhaps just remove the message? johannes
Re: Where to report mpdus tx vs failed?
On Mon, 2018-10-22 at 14:06 +0200, Johannes Berg wrote: > On Fri, 2018-10-19 at 11:32 -0700, Ben Greear wrote: > > > > I was hoping I could fit it into some existing stat. It is sort of like > > retries, so that will be my first attempt. > > > > By investigating an RF sniff, I notice the 9880 ath10k (with my fw > > and driver, at least), will retransmit about 30% of the frames when running > > at least one of my test cases (small udp frame transmit to AP that can only > > do about mcs5 or mcs7 > > reliably at 3x3 nss). > > > > I'd like to report this stat in my wifi test tool if nothing else, but > > likely other people would make use of it as well. > > > We have NL80211_STA_INFO_TX_RETRIES, shouldn't you be able to capture it > there? Or, per TID, NL80211_TID_STATS_TX_MSDU_RETRIES johannes
Re: Where to report mpdus tx vs failed?
On Fri, 2018-10-19 at 11:32 -0700, Ben Greear wrote: > > I was hoping I could fit it into some existing stat. It is sort of like > retries, so that will be my first attempt. > > By investigating an RF sniff, I notice the 9880 ath10k (with my fw > and driver, at least), will retransmit about 30% of the frames when running > at least one of my test cases (small udp frame transmit to AP that can only > do about mcs5 or mcs7 > reliably at 3x3 nss). > > I'd like to report this stat in my wifi test tool if nothing else, but > likely other people would make use of it as well. We have NL80211_STA_INFO_TX_RETRIES, shouldn't you be able to capture it there? johannes
Re: 4.18.16: memory leak in sta_set_sinfo
On Mon, 2018-10-22 at 10:35 +0200, Johannes Stezenbach wrote: > > commit 848e616e66d4592fe9afc40743d3504deb7632b4 > > Author: Stefan Seyfried > > Date: Sun Sep 30 12:53:00 2018 +0200 > > > > cfg80211: fix wext-compat memory leak > > > > Hopefully that'll eventually propagate to stable. > > Good to know it's fixed, but "hopefully" makes me wonder, > I'd love to hear the confirmation that it's been queued. Well, normally it works automatically when you have a Fixes tag, so I don't usually try to queue anything manually. Maybe Greg has had his hands full with the 4.19 release :-) johannes
Re: 4.18.16: memory leak in sta_set_sinfo
On Mon, 2018-10-22 at 10:02 +0200, Johannes Stezenbach wrote: > > [] sta_set_sinfo+0x634/0x900 [mac80211] > [ ] ieee80211_get_station+0x50/0x70 [mac80211] > [<9fd8a7aa>] cfg80211_wext_giwrate+0x111/0x2b0 [cfg80211] > I guess it relates to > 0fdf1493b41 " mac80211: allocate and fill tidstats only when needed" > which Fixes: 8689c051a201 ("cfg80211: dynamically allocate per-tid stats for > station info") Yes, you're using wext which we don't any more, so we didn't see this. However, it's already fixed: commit 848e616e66d4592fe9afc40743d3504deb7632b4 Author: Stefan Seyfried Date: Sun Sep 30 12:53:00 2018 +0200 cfg80211: fix wext-compat memory leak Hopefully that'll eventually propagate to stable. johannes
Re: FTM support on ath10k
Hi, > I am interested in contributing to the development and testing of FTM > using ath10k devices. > Currently I am running a standard 4.14 kernel I guess you'd have to upgrade to something you compiled yourself :-) > and have QCA988X (WLE600VX) and QCA9984 (WLE1216V5) devices. > My QCA9984 is running 10.4.3 firmware and the QCA988X is using > 10.2.4.70.66. > > Would it be possible for me to build/test some of the FTM changes, or > is this very much a work in progress and not useable by other people. > I can see that FTM spans a number of components: > Userpace (hostapd, wpa_supplicant) > cfg80211 (nl80211) > mac80211 > ath10k driver. > I would also be interested if it was possible to test FTM from a test > harness around the ath10k? I don't know. So far ath10k only has FTM responder code out, I think, not FTM initiator. No userspace tooling around it exists, as far as I know. We're still working on both initiator and responder support for iwlwifi. johannes
Re: [PATCH 04/19] wilc: add host_interface.c
> > > Do you mean we should be doing the work from the cfg context? > > > Note that this is called cfg80211_ops.set_wiphy_params(), and involves > > > locking mutexes, packing the wids, bus operations, and waiting for the > > > device to respond. > > > > That *should* be fine - how long do you expect that to take? > > > > It depends on the IO bus used (SPI/SDIO), clock speed.. etc > It would also vary according to the data packets being transferred > with the device. > If there's heavy data transfer, configuration packets would take > longer to be sent, and for their response to be received. How much time do you think you're looking at? I'd argue for trying it though as it makes the code MUCH simpler. johannes
Re: [PATCH] nl80211: Emit a NEW_INTERFACE on iftype change
On Fri, 2018-10-19 at 04:37 +0200, Andrew Zaborowski wrote: > Let userspace learn about iftype changes by sending an > NL80211_CMD_NEW_INTERFACE when handling a NL80211_CMD_SET_INTERFACE > command. There seems to be no other place where the iftype can change: > nl80211_set_interface is the only caller of cfg80211_change_iface which > is the only caller of ops->change_virtual_intf. Why not use SET_INTERFACE? I think I'd be more comfortable with that, since it's not really used now and existing userspace might do weird things with NEW_INTERFACE? johannes
Re: [PATCH] mac80211: allow hardware scan to fall back to software
On Thu, 2018-10-18 at 12:42 +0200, Arend van Spriel wrote: > > > + * This callback is also allowed to return the special return value 1, > > + * this indicates that hardware scan isn't desirable right now and a > > + * software scan should be done instead. A driver wishing to use this > > + * capability must ensure its (hardware) scan capabilities aren't > > + * advertised as more capable than mac80211's software scan is. > > Not sure what is meant by the last sentence. What does "more capable" > mean? What are (or where to find) the mac80211 software scan capabilities? I was thinking in terms of the scan capabilities exposed to userspace via nl80211. For example, mac80211 supports max 4 scan SSIDs (though this is a soft limit, it doesn't really care later), and a maximum probe request size. That's it though, so it doesn't support things like NL80211_EXT_FEATURE_LOW_SPAN_SCAN, for example. johannes
[PATCH] mac80211: allow hardware scan to fall back to software
From: Johannes Berg
In some cases, like in the rsi driver hardware scan offload, there
may be scenarios in which hardware scan might not be available or
desirable.
Allow drivers to cope with this by letting them fall back to software
scan by returning the special value 1 from the hardware scan method.
Requested-by: Sushant Kumar Mishra
Requested-by: Siva Rebbagondla
Signed-off-by: Johannes Berg
---
include/net/mac80211.h | 5 +
net/mac80211/scan.c| 22 ++
2 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 71985e95d2d9..4631b3b6fc37 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3239,6 +3239,11 @@ enum ieee80211_reconfig_type {
* When the scan finishes, ieee80211_scan_completed() must be called;
* note that it also must be called when the scan cannot finish due to
* any error unless this callback returned a negative error code.
+ * This callback is also allowed to return the special return value 1,
+ * this indicates that hardware scan isn't desirable right now and a
+ * software scan should be done instead. A driver wishing to use this
+ * capability must ensure its (hardware) scan capabilities aren't
+ * advertised as more capable than mac80211's software scan is.
* The callback can sleep.
*
* @cancel_hw_scan: Ask the low-level tp cancel the active hw scan.
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 5d2a1118..95413413f98c 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -356,7 +356,7 @@ static bool ieee80211_prep_hw_scan(struct ieee80211_local
*local)
static void __ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted)
{
struct ieee80211_local *local = hw_to_local(hw);
- bool hw_scan = local->ops->hw_scan;
+ bool hw_scan = test_bit(SCAN_HW_SCANNING, >scanning);
bool was_scanning = local->scanning;
struct cfg80211_scan_request *scan_req;
struct ieee80211_sub_if_data *scan_sdata;
@@ -606,6 +606,7 @@ static int __ieee80211_start_scan(struct
ieee80211_sub_if_data *sdata,
struct cfg80211_scan_request *req)
{
struct ieee80211_local *local = sdata->local;
+ bool hw_scan = local->ops->hw_scan;
int rc;
lockdep_assert_held(>mtx);
@@ -620,7 +621,8 @@ static int __ieee80211_start_scan(struct
ieee80211_sub_if_data *sdata,
return 0;
}
- if (local->ops->hw_scan) {
+ again:
+ if (hw_scan) {
u8 *ies;
local->hw_scan_ies_bufsize = local->scan_ies_len + req->ie_len;
@@ -679,7 +681,7 @@ static int __ieee80211_start_scan(struct
ieee80211_sub_if_data *sdata,
else
memcpy(local->scan_addr, sdata->vif.addr, ETH_ALEN);
- if (local->ops->hw_scan) {
+ if (hw_scan) {
__set_bit(SCAN_HW_SCANNING, >scanning);
} else if ((req->n_channels == 1) &&
(req->channels[0] == local->_oper_chandef.chan)) {
@@ -722,7 +724,7 @@ static int __ieee80211_start_scan(struct
ieee80211_sub_if_data *sdata,
ieee80211_recalc_idle(local);
- if (local->ops->hw_scan) {
+ if (hw_scan) {
WARN_ON(!ieee80211_prep_hw_scan(local));
rc = drv_hw_scan(local, sdata, local->hw_scan_req);
} else {
@@ -740,6 +742,18 @@ static int __ieee80211_start_scan(struct
ieee80211_sub_if_data *sdata,
RCU_INIT_POINTER(local->scan_sdata, NULL);
}
+ if (hw_scan && rc == 1) {
+ /*
+* we can't fall back to software for P2P-GO
+* as it must update NoA etc.
+*/
+ if (ieee80211_vif_type_p2p(>vif) ==
+ NL80211_IFTYPE_P2P_GO)
+ return -EOPNOTSUPP;
+ hw_scan = false;
+ goto again;
+ }
+
return rc;
}
--
2.17.2
Re: [PATCH 04/19] wilc: add host_interface.c
On Fri, 2018-10-12 at 22:08 +, adham.aboza...@microchip.com wrote: > > On 10/11/2018 12:01 AM, Johannes Berg wrote: > > > > > Agree. parameter validation can be done before scheduling the work, > > > and hence appropriate error can be returned to caller . > > > > > > > If I got your point correctly, you are referring to the lines that > > > stores the parameters into the hif_drv->cfg_values. > > > > Well, I was actually thinking that I'm not even sure why you schedule > > work at all! > > Do you mean we should be doing the work from the cfg context? > Note that this is called cfg80211_ops.set_wiphy_params(), and involves > locking mutexes, packing the wids, bus operations, and waiting for the > device to respond. That *should* be fine - how long do you expect that to take? johannes
Re: [PATCH 04/19] wilc: add host_interface.c
On Fri, 2018-10-12 at 21:55 +, adham.aboza...@microchip.com wrote: > > Here the driver is configuring parameters in the device by sending a > WID command for each parameters. > The val pointer points to the value of the parameter to be set, and > here all parameters being set to 0 were sharing the dummyval variable. Ok. > Looking again at this, these constant parameters can be omitted from > the driver and done on the device instead. I think it's fine, just the pointers look strange. Would probably be different once you resolve the middle layer though. > > > + if (conn_attr->bssid) > > > + memcpy(cur_byte, conn_attr->bssid, 6); > > > + cur_byte += 6; > > > > u8 bssid[ETH_ALEN]; > > > > > + if (conn_attr->bssid) > > > + memcpy(cur_byte, conn_attr->bssid, 6); > > > + cur_byte += 6; > > > > again? > > Agree. Can be changed to avoid duplication. Requires a matching change on the > device. Again, like above, don't worry too much about changing the device/firmware. I was just pointing out it's duplicate, if that's the way it is then too bad, but it doesn't really matter. The more interesting thing here is to change it to use a struct and not pack it manually. johannes
Re: [PATCH v2 1/2] mac80211_hwsim: allow setting iftype support
> Ah ok, that makes sense. Ok, well I can fixup that typo, as well as > reordering the use_chanctx stuff I mentioned. Unless there was anything > else? If not I can submit v3. I haven't really looked at it much yet, it's 11pm here and I'm tired :-) Feel free to resubmit, but I'm not going to promise I won't make you submit v4 if you do ;-) johannes
Re: [PATCH v2 1/2] mac80211_hwsim: allow setting iftype support
On Wed, 2018-10-17 at 14:00 -0700, James Prestwood wrote: > > > That makes me wonder if you'd also want to support limiting the # of > > interfaces/channels to mimic another device? > > Yeah it seems like it would be pretty easy to add that on top of this > change, although I don't really see us actually using it. Sure, fair enough. > I can add it, > but my only concern is I would not test it up to the same level I > tested iftype/ciphers. If you don't really want to use it, then I don't think you need to add it. I was just wondering if it made a difference. > Maybe this is also because I am not entirely > sure what num_different_channels is doing. Is this only relevant when > multiple interfaces exist on a radio? Like setting how many channels > can be use simultaneously? Correct. "Simultaneously" is a bit of an overstatement, typically today it's implemented by TDM - if you have two interfaces with a connection to an AP each, but on different channels, each interface would just tell its AP that it's going to sleep, but instead hop to the other channel and tell that AP that it woke up... etc. johannes
Re: [LKP] f77477b162: hwsim.ap_open_multicast_to_unicast.fail
On Wed, 2018-10-17 at 13:53 +0800, kernel test robot wrote:
> FYI, we noticed the following commit (built with gcc-7):
>
> commit: f77477b1624c0ac20c0c85c8b529da48686ab54d ("[PATCH] New functionality
> for aborting ongoing CAC.")
> url:
> https://github.com/0day-ci/linux/commits/Enrique-Giraldo/New-functionality-for-aborting-ongoing-CAC/20180925-181417
> base: https://git.kernel.org/cgit/linux/kernel/git/jberg/mac80211-next.git
> master
>
> in testcase: hwsim
> with following parameters:
>
> group: hwsim-01
Yeah, this patch completely broke the nl80211 API :-)
johannes
[PATCH v6 1/2] cfg80211: add peer measurement with FTM initiator API
From: Johannes Berg
Add a new "peer measurement" API, that can be used to measure
certain things related to a peer. Right now, only implement
FTM (flight time measurement) over it, but the idea is that
it'll be extensible to also support measuring the necessary
things to calculate e.g. angle-of-arrival for WiGig.
The API is structured to have a generic list of peers and
channels to measure with/on, and then for each of those a
set of measurements (again, only FTM right now) to perform.
Results are sent to the requesting socket, including a final
complete message.
Closing the controlling netlink socket will abort a running
measurement.
Signed-off-by: Johannes Berg
---
v3:
- add a bit to report "final" for partial results
- remove list keeping etc. and just unicast out the results
to the requester (big code reduction ...)
- also send complete message unicast, and as a result
remove the multicast group
- separate out struct cfg80211_pmsr_ftm_request_peer
from struct cfg80211_pmsr_request_peer
- document timeout == 0 if no timeout
- disallow setting timeout nl80211 attribute to 0,
must not include attribute for no timeout
- make MAC address randomization optional
- change num bursts exponent default to 0 (1 burst, rather
rather than the old default of 15==don't care)
v4:
- clarify NL80211_ATTR_TIMEOUT documentation
v5:
- remove unnecessary nl80211 multicast/family changes
- remove partial results bit/flag, final is sufficient
- add max_bursts_exponent, max_ftms_per_burst to capability
- rename "frames per burst" -> "FTMs per burst"
v6:
- rename cfg80211_pmsr_free_wdev() to cfg80211_pmsr_wdev_down()
and call it in leave, so the device can't go down with any
pending measurements
---
include/net/cfg80211.h | 261
include/uapi/linux/nl80211.h | 417 +
net/wireless/Makefile| 1 +
net/wireless/core.c | 33 ++
net/wireless/core.h | 4 +
net/wireless/nl80211.c | 192 ++--
net/wireless/nl80211.h | 28 ++
net/wireless/pmsr.c | 590 +++
net/wireless/rdev-ops.h | 25 ++
net/wireless/trace.h | 68
10 files changed, 1600 insertions(+), 19 deletions(-)
create mode 100644 net/wireless/pmsr.c
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 1fa41b7a1be3..40fe81cb4ad4 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2848,6 +2848,190 @@ struct cfg80211_ftm_responder_stats {
u32 out_of_window_triggers_num;
};
+/**
+ * struct cfg80211_pmsr_ftm_result - FTM result
+ * @failure_reason: if this measurement failed (PMSR status is
+ * %NL80211_PMSR_STATUS_FAILURE), this gives a more precise
+ * reason than just "failure"
+ * @burst_index: if reporting partial results, this is the index
+ * in [0 .. num_bursts-1] of the burst that's being reported
+ * @num_ftmr_attempts: number of FTM request frames transmitted
+ * @num_ftmr_successes: number of FTM request frames acked
+ * @busy_retry_time: if failure_reason is %NL80211_PMSR_FTM_FAILURE_PEER_BUSY,
+ * fill this to indicate in how many seconds a retry is deemed possible
+ * by the responder
+ * @num_bursts_exp: actual number of bursts exponent negotiated
+ * @burst_duration: actual burst duration negotiated
+ * @ftms_per_burst: actual frames per burst negotiated
+ * @lci_len: length of LCI information (if present)
+ * @civicloc_len: length of civic location information (if present)
+ * @lci: LCI data (may be %NULL)
+ * @civicloc: civic location data (may be %NULL)
+ * @rssi_avg: average RSSI over FTM action frames reported
+ * @rssi_spread: spread of the RSSI over FTM action frames reported
+ * @tx_rate: bitrate for transmitted FTM action frame response
+ * @rx_rate: bitrate of received FTM action frame
+ * @rtt_avg: average of RTTs measured (must have either this or @dist_avg)
+ * @rtt_variance: variance of RTTs measured (note that standard deviation is
+ * the square root of the variance)
+ * @rtt_spread: spread of the RTTs measured
+ * @dist_avg: average of distances (mm) measured
+ * (must have either this or @rtt_avg)
+ * @dist_variance: variance of distances measured (see also @rtt_variance)
+ * @dist_spread: spread of distances measured (see also @rtt_spread)
+ * @num_ftmr_attempts_valid: @num_ftmr_attempts is valid
+ * @num_ftmr_successes_valid: @num_ftmr_successes is valid
+ * @rssi_avg_valid: @rssi_avg is valid
+ * @rssi_spread_valid: @rssi_spread is valid
+ * @tx_rate_valid: @tx_rate is valid
+ * @rx_rate_valid: @rx_rate is valid
+ * @rtt_avg_valid: @rtt_avg is valid
+ * @rtt_variance_valid: @rtt_variance is valid
+ * @rtt_spread_valid: @rtt_spread is valid
+ * @dist_avg_valid: @dist_avg is valid
+ * @dist_variance_valid: @dist_variance is valid
+ * @dist_spread_valid: @dist_spread is valid
+ */
+struct cfg80211_pmsr_ftm_resul
[PATCH v6 2/2] mac80211: allow drivers to use peer measurement API
From: Johannes Berg
There's nothing much for mac80211 to do, so only pass through
the requests with minimal checks and tracing. The driver must
call cfg80211's results APIs.
Signed-off-by: Johannes Berg
---
v4: first version - just takes the same number as the cfg80211 patch
v5: no changes
v6: no changes
---
include/net/mac80211.h| 7 +++
net/mac80211/cfg.c| 22 ++
net/mac80211/driver-ops.h | 34 ++
net/mac80211/trace.h | 12
4 files changed, 75 insertions(+)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 71985e95d2d9..e3d57e7a55cc 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3623,6 +3623,9 @@ enum ieee80211_reconfig_type {
* skb is always a real frame, head may or may not be an A-MSDU.
* @get_ftm_responder_stats: Retrieve FTM responder statistics, if available.
* Statistics should be cumulative, currently no way to reset is provided.
+ *
+ * @start_pmsr: start peer measurement (e.g. FTM) (this call can sleep)
+ * @abort_pmsr: abort peer measurement (this call can sleep)
*/
struct ieee80211_ops {
void (*tx)(struct ieee80211_hw *hw,
@@ -3911,6 +3914,10 @@ struct ieee80211_ops {
int (*get_ftm_responder_stats)(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
struct cfg80211_ftm_responder_stats
*ftm_stats);
+ int (*start_pmsr)(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+ struct cfg80211_pmsr_request *request);
+ void (*abort_pmsr)(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+ struct cfg80211_pmsr_request *request);
};
/**
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 51622333d460..2ffbbf4d 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3849,6 +3849,26 @@ ieee80211_get_ftm_responder_stats(struct wiphy *wiphy,
return drv_get_ftm_responder_stats(local, sdata, ftm_stats);
}
+static int
+ieee80211_start_pmsr(struct wiphy *wiphy, struct wireless_dev *dev,
+struct cfg80211_pmsr_request *request)
+{
+ struct ieee80211_local *local = wiphy_priv(wiphy);
+ struct ieee80211_sub_if_data *sdata = IEEE80211_WDEV_TO_SUB_IF(dev);
+
+ return drv_start_pmsr(local, sdata, request);
+}
+
+static void
+ieee80211_abort_pmsr(struct wiphy *wiphy, struct wireless_dev *dev,
+struct cfg80211_pmsr_request *request)
+{
+ struct ieee80211_local *local = wiphy_priv(wiphy);
+ struct ieee80211_sub_if_data *sdata = IEEE80211_WDEV_TO_SUB_IF(dev);
+
+ return drv_abort_pmsr(local, sdata, request);
+}
+
const struct cfg80211_ops mac80211_config_ops = {
.add_virtual_intf = ieee80211_add_iface,
.del_virtual_intf = ieee80211_del_iface,
@@ -3944,4 +3964,6 @@ const struct cfg80211_ops mac80211_config_ops = {
.tx_control_port = ieee80211_tx_control_port,
.get_txq_stats = ieee80211_get_txq_stats,
.get_ftm_responder_stats = ieee80211_get_ftm_responder_stats,
+ .start_pmsr = ieee80211_start_pmsr,
+ .abort_pmsr = ieee80211_abort_pmsr,
};
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 0b1747a2313d..3e0d5922a440 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1199,6 +1199,40 @@ drv_get_ftm_responder_stats(struct ieee80211_local
*local,
return ret;
}
+static inline int drv_start_pmsr(struct ieee80211_local *local,
+struct ieee80211_sub_if_data *sdata,
+struct cfg80211_pmsr_request *request)
+{
+ int ret = -EOPNOTSUPP;
+
+ might_sleep();
+ if (!check_sdata_in_driver(sdata))
+ return -EIO;
+
+ trace_drv_start_pmsr(local, sdata);
+
+ if (local->ops->start_pmsr)
+ ret = local->ops->start_pmsr(>hw, >vif, request);
+ trace_drv_return_int(local, ret);
+
+ return ret;
+}
+
+static inline void drv_abort_pmsr(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ struct cfg80211_pmsr_request *request)
+{
+ trace_drv_abort_pmsr(local, sdata);
+
+ might_sleep();
+ if (!check_sdata_in_driver(sdata))
+ return;
+
+ if (local->ops->abort_pmsr)
+ local->ops->abort_pmsr(>hw, >vif, request);
+ trace_drv_return_void(local);
+}
+
static inline int drv_start_nan(struct ieee80211_local *local,
struct ieee80211_sub_if_data *sdata,
struct cfg80211_nan_conf *conf)
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index 588c51a67c89..ac2f1922d469 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -1882,6 +1882,18 @@ TRACE_EVENT(drv_del_nan
[PATCH v5 2/2] mac80211: allow drivers to use peer measurement API
From: Johannes Berg
There's nothing much for mac80211 to do, so only pass through
the requests with minimal checks and tracing. The driver must
call cfg80211's results APIs.
Signed-off-by: Johannes Berg
---
v4: first version - just takes the same number as the cfg80211 patch
v5: no changes
---
include/net/mac80211.h| 7 +++
net/mac80211/cfg.c| 22 ++
net/mac80211/driver-ops.h | 34 ++
net/mac80211/trace.h | 12
4 files changed, 75 insertions(+)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 71985e95d2d9..e3d57e7a55cc 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3623,6 +3623,9 @@ enum ieee80211_reconfig_type {
* skb is always a real frame, head may or may not be an A-MSDU.
* @get_ftm_responder_stats: Retrieve FTM responder statistics, if available.
* Statistics should be cumulative, currently no way to reset is provided.
+ *
+ * @start_pmsr: start peer measurement (e.g. FTM) (this call can sleep)
+ * @abort_pmsr: abort peer measurement (this call can sleep)
*/
struct ieee80211_ops {
void (*tx)(struct ieee80211_hw *hw,
@@ -3911,6 +3914,10 @@ struct ieee80211_ops {
int (*get_ftm_responder_stats)(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
struct cfg80211_ftm_responder_stats
*ftm_stats);
+ int (*start_pmsr)(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+ struct cfg80211_pmsr_request *request);
+ void (*abort_pmsr)(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+ struct cfg80211_pmsr_request *request);
};
/**
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 51622333d460..2ffbbf4d 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3849,6 +3849,26 @@ ieee80211_get_ftm_responder_stats(struct wiphy *wiphy,
return drv_get_ftm_responder_stats(local, sdata, ftm_stats);
}
+static int
+ieee80211_start_pmsr(struct wiphy *wiphy, struct wireless_dev *dev,
+struct cfg80211_pmsr_request *request)
+{
+ struct ieee80211_local *local = wiphy_priv(wiphy);
+ struct ieee80211_sub_if_data *sdata = IEEE80211_WDEV_TO_SUB_IF(dev);
+
+ return drv_start_pmsr(local, sdata, request);
+}
+
+static void
+ieee80211_abort_pmsr(struct wiphy *wiphy, struct wireless_dev *dev,
+struct cfg80211_pmsr_request *request)
+{
+ struct ieee80211_local *local = wiphy_priv(wiphy);
+ struct ieee80211_sub_if_data *sdata = IEEE80211_WDEV_TO_SUB_IF(dev);
+
+ return drv_abort_pmsr(local, sdata, request);
+}
+
const struct cfg80211_ops mac80211_config_ops = {
.add_virtual_intf = ieee80211_add_iface,
.del_virtual_intf = ieee80211_del_iface,
@@ -3944,4 +3964,6 @@ const struct cfg80211_ops mac80211_config_ops = {
.tx_control_port = ieee80211_tx_control_port,
.get_txq_stats = ieee80211_get_txq_stats,
.get_ftm_responder_stats = ieee80211_get_ftm_responder_stats,
+ .start_pmsr = ieee80211_start_pmsr,
+ .abort_pmsr = ieee80211_abort_pmsr,
};
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 0b1747a2313d..3e0d5922a440 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1199,6 +1199,40 @@ drv_get_ftm_responder_stats(struct ieee80211_local
*local,
return ret;
}
+static inline int drv_start_pmsr(struct ieee80211_local *local,
+struct ieee80211_sub_if_data *sdata,
+struct cfg80211_pmsr_request *request)
+{
+ int ret = -EOPNOTSUPP;
+
+ might_sleep();
+ if (!check_sdata_in_driver(sdata))
+ return -EIO;
+
+ trace_drv_start_pmsr(local, sdata);
+
+ if (local->ops->start_pmsr)
+ ret = local->ops->start_pmsr(>hw, >vif, request);
+ trace_drv_return_int(local, ret);
+
+ return ret;
+}
+
+static inline void drv_abort_pmsr(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ struct cfg80211_pmsr_request *request)
+{
+ trace_drv_abort_pmsr(local, sdata);
+
+ might_sleep();
+ if (!check_sdata_in_driver(sdata))
+ return;
+
+ if (local->ops->abort_pmsr)
+ local->ops->abort_pmsr(>hw, >vif, request);
+ trace_drv_return_void(local);
+}
+
static inline int drv_start_nan(struct ieee80211_local *local,
struct ieee80211_sub_if_data *sdata,
struct cfg80211_nan_conf *conf)
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index 588c51a67c89..ac2f1922d469 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -1882,6 +1882,18 @@ TRACE_EVENT(drv_del_nan_func,
)
[PATCH v5 1/2] cfg80211: add peer measurement with FTM initiator API
From: Johannes Berg
Add a new "peer measurement" API, that can be used to measure
certain things related to a peer. Right now, only implement
FTM (flight time measurement) over it, but the idea is that
it'll be extensible to also support measuring the necessary
things to calculate e.g. angle-of-arrival for WiGig.
The API is structured to have a generic list of peers and
channels to measure with/on, and then for each of those a
set of measurements (again, only FTM right now) to perform.
Results are sent to the requesting socket, including a final
complete message.
Closing the controlling netlink socket will abort a running
measurement.
Signed-off-by: Johannes Berg
---
v3:
- add a bit to report "final" for partial results
- remove list keeping etc. and just unicast out the results
to the requester (big code reduction ...)
- also send complete message unicast, and as a result
remove the multicast group
- separate out struct cfg80211_pmsr_ftm_request_peer
from struct cfg80211_pmsr_request_peer
- document timeout == 0 if no timeout
- disallow setting timeout nl80211 attribute to 0,
must not include attribute for no timeout
- make MAC address randomization optional
- change num bursts exponent default to 0 (1 burst, rather
rather than the old default of 15==don't care)
v4:
- clarify NL80211_ATTR_TIMEOUT documentation
v5:
- remove unnecessary nl80211 multicast/family changes
- remove partial results bit/flag, final is sufficient
- add max_bursts_exponent, max_ftms_per_burst to capability
- rename "frames per burst" -> "FTMs per burst"
---
include/net/cfg80211.h | 261
include/uapi/linux/nl80211.h | 417 +
net/wireless/Makefile| 1 +
net/wireless/core.c | 33 ++
net/wireless/core.h | 4 +
net/wireless/nl80211.c | 192 ++--
net/wireless/nl80211.h | 28 ++
net/wireless/pmsr.c | 590 +++
net/wireless/rdev-ops.h | 25 ++
net/wireless/trace.h | 68
10 files changed, 1600 insertions(+), 19 deletions(-)
create mode 100644 net/wireless/pmsr.c
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 1fa41b7a1be3..40fe81cb4ad4 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2848,6 +2848,190 @@ struct cfg80211_ftm_responder_stats {
u32 out_of_window_triggers_num;
};
+/**
+ * struct cfg80211_pmsr_ftm_result - FTM result
+ * @failure_reason: if this measurement failed (PMSR status is
+ * %NL80211_PMSR_STATUS_FAILURE), this gives a more precise
+ * reason than just "failure"
+ * @burst_index: if reporting partial results, this is the index
+ * in [0 .. num_bursts-1] of the burst that's being reported
+ * @num_ftmr_attempts: number of FTM request frames transmitted
+ * @num_ftmr_successes: number of FTM request frames acked
+ * @busy_retry_time: if failure_reason is %NL80211_PMSR_FTM_FAILURE_PEER_BUSY,
+ * fill this to indicate in how many seconds a retry is deemed possible
+ * by the responder
+ * @num_bursts_exp: actual number of bursts exponent negotiated
+ * @burst_duration: actual burst duration negotiated
+ * @ftms_per_burst: actual frames per burst negotiated
+ * @lci_len: length of LCI information (if present)
+ * @civicloc_len: length of civic location information (if present)
+ * @lci: LCI data (may be %NULL)
+ * @civicloc: civic location data (may be %NULL)
+ * @rssi_avg: average RSSI over FTM action frames reported
+ * @rssi_spread: spread of the RSSI over FTM action frames reported
+ * @tx_rate: bitrate for transmitted FTM action frame response
+ * @rx_rate: bitrate of received FTM action frame
+ * @rtt_avg: average of RTTs measured (must have either this or @dist_avg)
+ * @rtt_variance: variance of RTTs measured (note that standard deviation is
+ * the square root of the variance)
+ * @rtt_spread: spread of the RTTs measured
+ * @dist_avg: average of distances (mm) measured
+ * (must have either this or @rtt_avg)
+ * @dist_variance: variance of distances measured (see also @rtt_variance)
+ * @dist_spread: spread of distances measured (see also @rtt_spread)
+ * @num_ftmr_attempts_valid: @num_ftmr_attempts is valid
+ * @num_ftmr_successes_valid: @num_ftmr_successes is valid
+ * @rssi_avg_valid: @rssi_avg is valid
+ * @rssi_spread_valid: @rssi_spread is valid
+ * @tx_rate_valid: @tx_rate is valid
+ * @rx_rate_valid: @rx_rate is valid
+ * @rtt_avg_valid: @rtt_avg is valid
+ * @rtt_variance_valid: @rtt_variance is valid
+ * @rtt_spread_valid: @rtt_spread is valid
+ * @dist_avg_valid: @dist_avg is valid
+ * @dist_variance_valid: @dist_variance is valid
+ * @dist_spread_valid: @dist_spread is valid
+ */
+struct cfg80211_pmsr_ftm_result {
+ const u8 *lci;
+ const u8 *civicloc;
+ unsigned int lci_len;
+ unsigned int civicloc_len;
+ enum nl80211_peer_measurement_ftm_failure_
Re: [PATCH v4 1/2] cfg80211: add peer measurement with FTM initiator API
On Wed, 2018-10-17 at 14:52 +0300, Lior David wrote:
>
> On 10/16/2018 12:30 PM, Johannes Berg wrote:
>
> [...]
> > + * @rtt_avg: average of RTTs measured (must have either this or @dist_avg)
> > + * @rtt_variance: variance of RTTs measured (note that standard deviation
> > is
> > + * the square root of the variance)
> > + * @rtt_spread: spread of the RTTs measured
> > + * @dist_avg: average of distances (mm) measured
> > + * (must have either this or @rtt_avg)
> > + * @dist_variance: variance of distances measured (see also @rtt_variance)
> > + * @dist_spread: spread of distances measured (see also @rtt_spread)
>
> I don't remember much from my statistics class, can you please provide some
> details about the variance and spread fields? Alternatively I can look at the
> first driver implementation for reference, unless it is calculated by FW :-)
It can only be calculated by firmware, unless the firmware reports all
measurements.
Spread here is just "highest value - lowest value.
Variance is a more complicated measure of the distribution... What do
you want to know? Here we have a finite set of values, so
https://en.wikipedia.org/wiki/Variance#Population_variance
> > + * @partial: indicates that this is a partial result for this type
> > + * @final: if reporting partial results, mark this as the last one
>
> Maybe it is enough to have just the "final" bit? I mean if final bit is clear
> doesn't this imply the result is partial since more results will follow?
Hmm, good point.
> > +/**
> > + * struct cfg80211_pmsr_capabilities - cfg80211 peer measurement
> > capabilities
> > + * @max_peers: maximum number of peers in a single measurement
> > + * @report_ap_tsf: can report assoc AP's TSF for radio resource measurement
> > + * @randomize_mac_addr: can randomize MAC address for measurement
> > + * @ftm.supported: FTM measurement is supported
> > + * @ftm.asap: ASAP-mode is supported
> > + * @ftm.non_asap: non-ASAP-mode is supported
> > + * @ftm.request_lci: can request LCI data
> > + * @ftm.request_civicloc: can request civic location data
> > + * @ftm.preambles: bitmap of preambles supported ( nl80211_preamble)
> > + * @ftm.bandwidths: bitmap of bandwidths supported (
> > nl80211_chan_width)
>
> Consider adding ftm.max_bursts (or max_bursts_exponent)
Ok.
> and
> ftm.max_measurements_per_burst (is this the same as frames_per_burst?). For
> example in our implementation we can't do more than 6 measurements per burst
> because of resource limitations.
I guess measurements per burst are frames per burst? I'm not counting 4x
the frames because those are exchanged.
The spec says "FTMs per Burst", so perhaps I should align with that.
I suppose we can add that.
> Ok I see variance and spread are better documented here, maybe move the units
> information to the above structure definitions?
Well, they may seem above - but I thought it was more important to
document it better in the userspace API.
> > +/* multicast groups */
> > +enum nl80211_multicast_groups {
> > + NL80211_MCGRP_CONFIG,
> > + NL80211_MCGRP_SCAN,
> > + NL80211_MCGRP_REGULATORY,
> > + NL80211_MCGRP_MLME,
> > + NL80211_MCGRP_VENDOR,
> > + NL80211_MCGRP_NAN,
> > + NL80211_MCGRP_TESTMODE /* keep last - ifdef! */
> > +};
> > +
>
> Are these changes needed anymore since you don't send results as multicast?
No, good point.
johannes
Re: [RFC v2] cfg80211: add peer measurement with FTM API
On Wed, 2018-10-17 at 13:05 +0300, Lior David wrote: > > On 10/11/2018 1:05 PM, Johannes Berg wrote: > > > > > Thanks for the explanation. The send to same socket does sound more > > > efficient. > > > (In our internal implementation with vendor commands we were forced > > > to send the results as broadcast...) > > > > I suppose we can fix that, in the sense that we can add API to allow > > vendor commands to know the socket to send back to etc. > > > > I think that would be useful in general though as far as I know we don't have > any pending feature that requires it right now. Ok. For the record, I have no objection to somebody who needs this doing it, but it's not entirely trivial so I'm not going to just do it now. johannes
Re: [bug report] iwlwifi: mvm: kill INACTIVE queue state
Hi Dan, > 1239 ret = iwl_mvm_remove_inactive_tids(mvm, mvmsta, i, > 1240 > inactive_tid_bitmap, > 1241 _queues, > 1242 _queues); > 1243 if (ret >= 0 && free_queue < 0) > > > The iwl_mvm_remove_inactive_tids() returns a bool so it doesn't make > sense to test for >= 0. Probably we should test for ret == true? Huh, thanks for the report, we'll check. johannes
[PATCH v4 2/2] mac80211: allow drivers to use peer measurement API
From: Johannes Berg
There's nothing much for mac80211 to do, so only pass through
the requests with minimal checks and tracing. The driver must
call cfg80211's results APIs.
Signed-off-by: Johannes Berg
---
v4: first version - just takes the same number as the cfg80211 patch
---
include/net/mac80211.h| 7 +++
net/mac80211/cfg.c| 22 ++
net/mac80211/driver-ops.h | 34 ++
net/mac80211/trace.h | 12
4 files changed, 75 insertions(+)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 71985e95d2d9..e3d57e7a55cc 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3623,6 +3623,9 @@ enum ieee80211_reconfig_type {
* skb is always a real frame, head may or may not be an A-MSDU.
* @get_ftm_responder_stats: Retrieve FTM responder statistics, if available.
* Statistics should be cumulative, currently no way to reset is provided.
+ *
+ * @start_pmsr: start peer measurement (e.g. FTM) (this call can sleep)
+ * @abort_pmsr: abort peer measurement (this call can sleep)
*/
struct ieee80211_ops {
void (*tx)(struct ieee80211_hw *hw,
@@ -3911,6 +3914,10 @@ struct ieee80211_ops {
int (*get_ftm_responder_stats)(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
struct cfg80211_ftm_responder_stats
*ftm_stats);
+ int (*start_pmsr)(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+ struct cfg80211_pmsr_request *request);
+ void (*abort_pmsr)(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+ struct cfg80211_pmsr_request *request);
};
/**
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 51622333d460..2ffbbf4d 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3849,6 +3849,26 @@ ieee80211_get_ftm_responder_stats(struct wiphy *wiphy,
return drv_get_ftm_responder_stats(local, sdata, ftm_stats);
}
+static int
+ieee80211_start_pmsr(struct wiphy *wiphy, struct wireless_dev *dev,
+struct cfg80211_pmsr_request *request)
+{
+ struct ieee80211_local *local = wiphy_priv(wiphy);
+ struct ieee80211_sub_if_data *sdata = IEEE80211_WDEV_TO_SUB_IF(dev);
+
+ return drv_start_pmsr(local, sdata, request);
+}
+
+static void
+ieee80211_abort_pmsr(struct wiphy *wiphy, struct wireless_dev *dev,
+struct cfg80211_pmsr_request *request)
+{
+ struct ieee80211_local *local = wiphy_priv(wiphy);
+ struct ieee80211_sub_if_data *sdata = IEEE80211_WDEV_TO_SUB_IF(dev);
+
+ return drv_abort_pmsr(local, sdata, request);
+}
+
const struct cfg80211_ops mac80211_config_ops = {
.add_virtual_intf = ieee80211_add_iface,
.del_virtual_intf = ieee80211_del_iface,
@@ -3944,4 +3964,6 @@ const struct cfg80211_ops mac80211_config_ops = {
.tx_control_port = ieee80211_tx_control_port,
.get_txq_stats = ieee80211_get_txq_stats,
.get_ftm_responder_stats = ieee80211_get_ftm_responder_stats,
+ .start_pmsr = ieee80211_start_pmsr,
+ .abort_pmsr = ieee80211_abort_pmsr,
};
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 0b1747a2313d..3e0d5922a440 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1199,6 +1199,40 @@ drv_get_ftm_responder_stats(struct ieee80211_local
*local,
return ret;
}
+static inline int drv_start_pmsr(struct ieee80211_local *local,
+struct ieee80211_sub_if_data *sdata,
+struct cfg80211_pmsr_request *request)
+{
+ int ret = -EOPNOTSUPP;
+
+ might_sleep();
+ if (!check_sdata_in_driver(sdata))
+ return -EIO;
+
+ trace_drv_start_pmsr(local, sdata);
+
+ if (local->ops->start_pmsr)
+ ret = local->ops->start_pmsr(>hw, >vif, request);
+ trace_drv_return_int(local, ret);
+
+ return ret;
+}
+
+static inline void drv_abort_pmsr(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ struct cfg80211_pmsr_request *request)
+{
+ trace_drv_abort_pmsr(local, sdata);
+
+ might_sleep();
+ if (!check_sdata_in_driver(sdata))
+ return;
+
+ if (local->ops->abort_pmsr)
+ local->ops->abort_pmsr(>hw, >vif, request);
+ trace_drv_return_void(local);
+}
+
static inline int drv_start_nan(struct ieee80211_local *local,
struct ieee80211_sub_if_data *sdata,
struct cfg80211_nan_conf *conf)
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index 588c51a67c89..ac2f1922d469 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -1882,6 +1882,18 @@ TRACE_EVENT(drv_del_nan_func,
)
[PATCH v4 1/2] cfg80211: add peer measurement with FTM initiator API
From: Johannes Berg
Add a new "peer measurement" API, that can be used to measure
certain things related to a peer. Right now, only implement
FTM (flight time measurement) over it, but the idea is that
it'll be extensible to also support measuring the necessary
things to calculate e.g. angle-of-arrival for WiGig.
The API is structured to have a generic list of peers and
channels to measure with/on, and then for each of those a
set of measurements (again, only FTM right now) to perform.
Results are sent to the requesting socket, including a final
complete message.
Closing the controlling netlink socket will abort a running
measurement.
Signed-off-by: Johannes Berg
---
v3:
- add a bit to report "final" for partial results
- remove list keeping etc. and just unicast out the results
to the requester (big code reduction ...)
- also send complete message unicast, and as a result
remove the multicast group
- separate out struct cfg80211_pmsr_ftm_request_peer
from struct cfg80211_pmsr_request_peer
- document timeout == 0 if no timeout
- disallow setting timeout nl80211 attribute to 0,
must not include attribute for no timeout
- make MAC address randomization optional
- change num bursts exponent default to 0 (1 burst, rather
rather than the old default of 15==don't care)
v4:
- clarify NL80211_ATTR_TIMEOUT documentation
---
include/net/cfg80211.h | 255
include/uapi/linux/nl80211.h | 412 +
net/wireless/Makefile| 1 +
net/wireless/core.c | 33 ++
net/wireless/core.h | 4 +
net/wireless/nl80211.c | 200 +---
net/wireless/nl80211.h | 41 +++
net/wireless/pmsr.c | 576 +++
net/wireless/rdev-ops.h | 25 ++
net/wireless/trace.h | 68 +
10 files changed, 1581 insertions(+), 34 deletions(-)
create mode 100644 net/wireless/pmsr.c
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 1fa41b7a1be3..69b34b8e22ea 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2848,6 +2848,191 @@ struct cfg80211_ftm_responder_stats {
u32 out_of_window_triggers_num;
};
+/**
+ * struct cfg80211_pmsr_ftm_result - FTM result
+ * @failure_reason: if this measurement failed (PMSR status is
+ * %NL80211_PMSR_STATUS_FAILURE), this gives a more precise
+ * reason than just "failure"
+ * @burst_index: if reporting partial results, this is the index
+ * in [0 .. num_bursts-1] of the burst that's being reported
+ * @num_ftmr_attempts: number of FTM request frames transmitted
+ * @num_ftmr_successes: number of FTM request frames acked
+ * @busy_retry_time: if failure_reason is %NL80211_PMSR_FTM_FAILURE_PEER_BUSY,
+ * fill this to indicate in how many seconds a retry is deemed possible
+ * by the responder
+ * @num_bursts_exp: actual number of bursts exponent negotiated
+ * @burst_duration: actual burst duration negotiated
+ * @frames_per_burst: actual frames per burst negotiated
+ * @lci_len: length of LCI information (if present)
+ * @civicloc_len: length of civic location information (if present)
+ * @lci: LCI data (may be %NULL)
+ * @civicloc: civic location data (may be %NULL)
+ * @rssi_avg: average RSSI over FTM action frames reported
+ * @rssi_spread: spread of the RSSI over FTM action frames reported
+ * @tx_rate: bitrate for transmitted FTM action frame response
+ * @rx_rate: bitrate of received FTM action frame
+ * @rtt_avg: average of RTTs measured (must have either this or @dist_avg)
+ * @rtt_variance: variance of RTTs measured (note that standard deviation is
+ * the square root of the variance)
+ * @rtt_spread: spread of the RTTs measured
+ * @dist_avg: average of distances (mm) measured
+ * (must have either this or @rtt_avg)
+ * @dist_variance: variance of distances measured (see also @rtt_variance)
+ * @dist_spread: spread of distances measured (see also @rtt_spread)
+ * @num_ftmr_attempts_valid: @num_ftmr_attempts is valid
+ * @num_ftmr_successes_valid: @num_ftmr_successes is valid
+ * @rssi_avg_valid: @rssi_avg is valid
+ * @rssi_spread_valid: @rssi_spread is valid
+ * @tx_rate_valid: @tx_rate is valid
+ * @rx_rate_valid: @rx_rate is valid
+ * @rtt_avg_valid: @rtt_avg is valid
+ * @rtt_variance_valid: @rtt_variance is valid
+ * @rtt_spread_valid: @rtt_spread is valid
+ * @dist_avg_valid: @dist_avg is valid
+ * @dist_variance_valid: @dist_variance is valid
+ * @dist_spread_valid: @dist_spread is valid
+ */
+struct cfg80211_pmsr_ftm_result {
+ const u8 *lci;
+ const u8 *civicloc;
+ unsigned int lci_len;
+ unsigned int civicloc_len;
+ enum nl80211_peer_measurement_ftm_failure_reasons failure_reason;
+ u32 num_ftmr_attempts, num_ftmr_successes;
+ s16 burst_index;
+ u8 busy_retry_time;
+ u8 num_bursts_exp;
+ u8 burst_duration;
+ u8 frames_per_burst;
+ s32 rssi_avg;
+ s
Re: [RFC v3] cfg80211: add peer measurement with FTM API
> v3: > - add a bit to report "final" for partial results > - remove list keeping etc. and just unicast out the results >to the requester (big code reduction ...) > - also send complete message unicast, and as a result >remove the multicast group > - separate out struct cfg80211_pmsr_ftm_request_peer >from struct cfg80211_pmsr_request_peer > - document timeout == 0 if no timeout > - disallow setting timeout nl80211 attribute to 0, >must not include attribute for no timeout > - make MAC address randomization optional > - change num bursts exponent default to 0 (1 burst, rather >rather than the old default of 15==don't care) Forgot to say ... I didn't make the channel optional (yet), because I think that doing so now could possibly cause an issue where it's required for some devices and not required for others, unless we can do lookups in cfg80211. But if we can do it there, then userspace can also easily do it (station dump is the only data cfg80211 has), so then there's not that much point ... I think we could relax this later if we really needed to. johannes
Re: [RFC v3] cfg80211: add peer measurement with FTM API
On Sat, 2018-10-13 at 12:55 +0300, Kalle Valo wrote:
>
> > struct cfg80211_ops {
> > int (*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
>
> It would be nice to document if these ops can sleep or not.
I don't think we've documented that *anywhere* in cfg80211, but
everything can sleep there ...
At the mac80211 level things are different, so there we do document a
case-by-case basis.
johannes
Re: [RFC v3] cfg80211: add peer measurement with FTM API
> You have no recollection what happened in the earlier versions, right? :-p
v1 was very incomplete, it didn't have any results reporting, etc.
> > v3:
> > - add a bit to report "final" for partial results
> > - remove list keeping etc. and just unicast out the results
> >to the requester (big code reduction ...)
> > - also send complete message unicast, and as a result
> >remove the multicast group
> > - separate out struct cfg80211_pmsr_ftm_request_peer
> >from struct cfg80211_pmsr_request_peer
> > - document timeout == 0 if no timeout
> > - disallow setting timeout nl80211 attribute to 0,
> >must not include attribute for no timeout
>
> All these negations make my head spin (a little). Let's look at the
> actual documentation further down...
:-)
> > +struct cfg80211_pmsr_ftm_result {
> > + const u8 *lci;
> > + const u8 *civicloc;
> > + unsigned int lci_len;
> > + unsigned int civicloc_len;
> > + enum nl80211_peer_measurement_ftm_failure_reasons failure_reason;
> > + u32 num_ftmr_attempts, num_ftmr_successes;
>
> Maybe there is a good reason, but can we move the above line a bit down...
The reason was to avoid having padding for alignment.
> > + NL80211_ATTR_TIMEOUT,
> > +
>
> Guess you consider reuse of the TIMEOUT attribute?
Yes, I was actually surprised we don't have one already :-)
> I checked the policy
> definition in nl80211_policy so it disallows 0 value as mentioned in the
> changelog. How about adding that to the documentation here, ie. "when
> timeout attribute is not provided the timeout is disabled for the given
> operation" or something like that.
Sure, that makes sense, will do.
johannes
[RFC v3] cfg80211: add peer measurement with FTM API
From: Johannes Berg
Add a new "peer measurement" API, that can be used to measure
certain things related to a peer. Right now, only implement
FTM (flight time measurement) over it, but the idea is that
it'll be extensible to also support measuring the necessary
things to calculate e.g. angle-of-arrival for WiGig.
The API is structured to have a generic list of peers and
channels to measure with/on, and then for each of those a
set of measurements (again, only FTM right now) to perform.
Results are sent to the requesting socket, including a final
complete message.
Closing the controlling netlink socket will abort a running
measurement.
Signed-off-by: Johannes Berg
---
v3:
- add a bit to report "final" for partial results
- remove list keeping etc. and just unicast out the results
to the requester (big code reduction ...)
- also send complete message unicast, and as a result
remove the multicast group
- separate out struct cfg80211_pmsr_ftm_request_peer
from struct cfg80211_pmsr_request_peer
- document timeout == 0 if no timeout
- disallow setting timeout nl80211 attribute to 0,
must not include attribute for no timeout
- make MAC address randomization optional
- change num bursts exponent default to 0 (1 burst, rather
rather than the old default of 15==don't care)
---
include/net/cfg80211.h | 255 +++
include/uapi/linux/nl80211.h | 410 ++
net/wireless/Makefile| 1 +
net/wireless/core.c | 33 +++
net/wireless/core.h | 4 +
net/wireless/nl80211.c | 200 ---
net/wireless/nl80211.h | 41 +++
net/wireless/pmsr.c | 576 +++
net/wireless/rdev-ops.h | 25 ++
net/wireless/trace.h | 68 +
10 files changed, 1579 insertions(+), 34 deletions(-)
create mode 100644 net/wireless/pmsr.c
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 0e16e723dcef..2837ea5f37e1 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2841,6 +2841,191 @@ struct cfg80211_ftm_responder_stats {
u32 out_of_window_triggers_num;
};
+/**
+ * struct cfg80211_pmsr_ftm_result - FTM result
+ * @failure_reason: if this measurement failed (PMSR status is
+ * %NL80211_PMSR_STATUS_FAILURE), this gives a more precise
+ * reason than just "failure"
+ * @burst_index: if reporting partial results, this is the index
+ * in [0 .. num_bursts-1] of the burst that's being reported
+ * @num_ftmr_attempts: number of FTM request frames transmitted
+ * @num_ftmr_successes: number of FTM request frames acked
+ * @busy_retry_time: if failure_reason is %NL80211_PMSR_FTM_FAILURE_PEER_BUSY,
+ * fill this to indicate in how many seconds a retry is deemed possible
+ * by the responder
+ * @num_bursts_exp: actual number of bursts exponent negotiated
+ * @burst_duration: actual burst duration negotiated
+ * @frames_per_burst: actual frames per burst negotiated
+ * @lci_len: length of LCI information (if present)
+ * @civicloc_len: length of civic location information (if present)
+ * @lci: LCI data (may be %NULL)
+ * @civicloc: civic location data (may be %NULL)
+ * @rssi_avg: average RSSI over FTM action frames reported
+ * @rssi_spread: spread of the RSSI over FTM action frames reported
+ * @tx_rate: bitrate for transmitted FTM action frame response
+ * @rx_rate: bitrate of received FTM action frame
+ * @rtt_avg: average of RTTs measured (must have either this or @dist_avg)
+ * @rtt_variance: variance of RTTs measured (note that standard deviation is
+ * the square root of the variance)
+ * @rtt_spread: spread of the RTTs measured
+ * @dist_avg: average of distances (mm) measured
+ * (must have either this or @rtt_avg)
+ * @dist_variance: variance of distances measured (see also @rtt_variance)
+ * @dist_spread: spread of distances measured (see also @rtt_spread)
+ * @num_ftmr_attempts_valid: @num_ftmr_attempts is valid
+ * @num_ftmr_successes_valid: @num_ftmr_successes is valid
+ * @rssi_avg_valid: @rssi_avg is valid
+ * @rssi_spread_valid: @rssi_spread is valid
+ * @tx_rate_valid: @tx_rate is valid
+ * @rx_rate_valid: @rx_rate is valid
+ * @rtt_avg_valid: @rtt_avg is valid
+ * @rtt_variance_valid: @rtt_variance is valid
+ * @rtt_spread_valid: @rtt_spread is valid
+ * @dist_avg_valid: @dist_avg is valid
+ * @dist_variance_valid: @dist_variance is valid
+ * @dist_spread_valid: @dist_spread is valid
+ */
+struct cfg80211_pmsr_ftm_result {
+ const u8 *lci;
+ const u8 *civicloc;
+ unsigned int lci_len;
+ unsigned int civicloc_len;
+ enum nl80211_peer_measurement_ftm_failure_reasons failure_reason;
+ u32 num_ftmr_attempts, num_ftmr_successes;
+ s16 burst_index;
+ u8 busy_retry_time;
+ u8 num_bursts_exp;
+ u8 burst_duration;
+ u8 frames_per_burst;
+ s32 rssi_avg;
+ s32 rssi_spread;
+ str
Re: [RFC v2] cfg80211: add peer measurement with FTM API
On Sun, 2018-10-07 at 21:58 +0200, Johannes Berg wrote: > > > > + * @partial: indicates that this is a partial result for this type > > > > Is partial set to false for the last result of this measurement type? This > > may > > be useful, for example if requesting multiple measurement types, user space > > can > > start processing one measurement type before the entire session is > > completed. > > Yes, that was the intent, e.g. for multiple FTM bursts, but I see how > this might be misleading at this level. I'll clarify the documentation > (and probably over in nl80211.h as well) Actually, I changed my mind - I'll add a "final" bit as well, so "partial" will be set on all of them. johannes
Re: linux-next: manual merge of the crypto tree with the mac80211-next tree
On Thu, 2018-10-11 at 11:13 +1100, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the crypto tree got conflicts in:
>
> net/wireless/lib80211_crypt_tkip.c
> net/wireless/lib80211_crypt_wep.c
>
> between commit:
>
> b802a5d6f345 ("lib80211: don't use skcipher")
>
> from the mac80211-next tree and commit:
>
> db20f570e17a ("lib80211: Remove VLA usage of skcipher")
>
> from the crypto tree.
Thanks Stephen.
The fixup (use mac80211-next file) is fine, but I'm not sure we want to
bother Linus/Greg with it?
Herbert, maybe you can drop the patch from the crypto tree since my
change also removes the VLA usage?
johannes
Re: [RFC v2] cfg80211: add peer measurement with FTM API
On Tue, 2018-10-09 at 17:40 +0300, Lior David wrote: > Thanks for the explanation. The send to same socket does sound more efficient. > (In our internal implementation with vendor commands we were forced > to send the results as broadcast...) I suppose we can fix that, in the sense that we can add API to allow vendor commands to know the socket to send back to etc. > Yes as far as I remember rtt<->distance is a trivial calculation. What I meant > here, you return the average of few measurements done in a burst, and for > debugging we could return all the measurements done in the burst. For example > if > there were 4 measurements per burst return the 4 distance measurements done. Ah, OK. I guess you'd have to report separate measurements or something. Though if it's for debugging I'd think it may not make sense in this API but rather something out-of-band. > I meant we could have an AoA value (angle in degrees or other units) which > drivers can fill up if they want. For example if the driver has 2 antennas on > both sides it can report either 90 or 270 degrees. It will be usually very low > accuracy but at least can provide some information like from which side of the > AP you are. This can certainly be added later if at all (hopefully we will > have > AoA measurement with higher accuracy in the future) Yeah, ok, I get it now - I guess we can add it if somebody plays with it and finds how to obtain and use the value. > > > For connected station, usually you will want to do the measurement on the > > > connected channel (possibly some chips will not be able to do otherwise) > > > Maybe add option to use default channel? > > > > Perhaps. It's somewhat complicated to look up in general, userspace > > generally has a decent idea, and making it optional means you end up > > with an invalid chandef? > > > > I'll take a look, perhaps just leaving all the fields 0/NULL can work > > reasonably well, but drivers would have to support it. > > > > As I remember the driver/FW can easily find the connected channel for > connected > station. For unconnected station we should probably force this parameter. Should be able to, yes, but I suppose even userspace can. It just seems like extra complexity to impose on the driver, since cfg80211 doesn't necessarily have all the right information. Actually, hmm, that would imply "use maximum bandwidth" or something? And then what if that bandwidth isn't possible with FTM for some reason? It's a bit tricky then. > > If so, that sounds like something that generally needs improvement? > > > > Good point. I see we currently use 20_NOHT for DMG, guess we can continue > using it. Well, I think it'd make more sense to just enforce the DMG bandwidth everywhere, but I won't force the issue over this. > Ok I thought 15 meant to actually request 65535 bursts :-) > I still prefer the default to be 1 burst. Supporting the "no preference" means > it will be difficult to pre-allocate memory for burst results. Also all > drivers > should know to do one burst. Fair, I'll change it. johannes
Re: [RFC v3 01/12] rtw88: main files
On Thu, 2018-10-11 at 07:23 +, Tony Chuang wrote:
> > > + switch (vif->type) {
> > > + case NL80211_IFTYPE_AP:
> > > + case NL80211_IFTYPE_MESH_POINT:
> > > + net_type = RTW_NET_AP_MODE;
> > > + break;
> > > + case NL80211_IFTYPE_ADHOC:
> > > + net_type = RTW_NET_AD_HOC;
> > > + break;
> > > + default:
> > > + net_type = RTW_NET_NO_LINK;
> >
> > you might to add STATION and then fail in the default case?
>
>
> Yeah, station starts with NO_LINK until it's associated with an AP
Right. I was just thinking of the switch statement - you might want to
handle STATION explicitly, instead of in the default case, and then fail
in the default case for this to be a little more readable and robust.
Not all that important.
> > This will provoke error messages to be printed for e.g. CMAC keys, or do
> > you really not support protected management frames? If you were to pick
> > "-EOPNOTSUPP" then no errors would be printed.
>
> We do not support PMF hw encryption/decryption now, perhaps we need
> to register the cipher_schemes when ieee80211_register_hw.
Ok, that's fine.
> Even if HW does not support it, I think mac80211 can use SW
> encryption/decryption
> after driver failed to upload key to hardware?
Yes.
> So if driver has not declared MFP_CAPABLE, the mac80211 will ignore it and
> wpa_supplicant will guess we cannot perform MFP. It is strange.
Right, no, it's not strange. That was my point though, if you do want to
support it you should set MFP_CAPABLE, but you should return a different
error code to avoid an error message being printed from mac80211. That's
all. The logic is fine, just use -EOPNOTSUPP (rather than -ENOTSUPP) to
suppress any error messages.
> > why should statistics be reset evyer 2 seconds?
>
> All of our statistics are counted in 2 seconds, ex. pkts, bytes, fa ...
> So just reset them every seconds.
No other device behaves this way though, so you shouldn't do this
either.
> > > + ieee80211_hw_set(hw, MFP_CAPABLE);
> >
> > so you do have MFP - I guess you should test it and check for spurious
> > hardware crypto messages
>
> We don't have now, should remove them. But as I have mentioned, if we don't
> declare it here, mac80211 will discard the cipher and pass it to wiphy.
> And we still should be able to work with MFP because mac80211 can do
> software encryption/decryption for us.
Right. So this is fine, see above regarding the error message that gets
printed.
> Finally, I removed the vif_list and sta_list. And use the iterator
> provided by mac80211,
> But there is one question that how can we find all of the sta associated
> with specific vif,
> Has there an only way to iterate every sta and see if (sta->vif == vif) ?
Yes, looks like that's the only way - I guess you could pass the vif as
the data pointer. I suppose we could add a vif filter argument to the
iteration and ignore it if it's NULL, but is it worth it?
johannes
Re: [PATCH 14/19] wilc: add linux_mon.c
On Thu, 2018-10-11 at 12:42 +0530, Ajay Singh wrote: > > I'm > > just doing a drive-by review for the stack integration aspects (and > > haven't even found where that happens yet). [...] > Currently, I am not clear about which stack integration part is missing > . Can you please provide some more details about it. No, I'm just saying that *I* was looking for how you integrate with the stack (cfg80211), and not so much trying to review everything else. johannes
Re: [PATCH 13/19] wilc: add linux_wlan.c
On Thu, 2018-10-11 at 12:30 +0530, Ajay Singh wrote:
>
> > > + if (queue_count > FLOW_CONTROL_UPPER_THRESHOLD) {
> > > + netif_stop_queue(wilc->vif[0]->ndev);
> > > + netif_stop_queue(wilc->vif[1]->ndev);
> > > + }
> >
> > It seems like a pretty bad idea to hard-code two interfaces, we do
> > dynamic addition/removal these days, in *particular* for P2P.
> >
>
> Did you mean it not good to call stop queue for both the interfaces.
> Can you please provide some more details about this comments.
No, I mean you should be more dynamic and have e.g. a list of interfaces
(actually, you can use cfg80211's list, I believe!), instead of hard-
coding that you have "wlan0" and "p2p0".
johannes
Re: [PATCH 04/19] wilc: add host_interface.c
> Agree. parameter validation can be done before scheduling the work, > and hence appropriate error can be returned to caller . > If I got your point correctly, you are referring to the lines that > stores the parameters into the hif_drv->cfg_values. Well, I was actually thinking that I'm not even sure why you schedule work at all! > Instead of packing the parameters in host structures like struct > add_sta_param, then repacking it in the device format, it can use > struct station_parameters and pack them directly into the device > format Makes sense. Also note my other email on how there should be structs involved, rather than open-coded WID entry lists. johannes
Re: [PATCH 04/19] wilc: add host_interface.c
Looking at all this wid_list stuff again,
> + wid_list[wid_cnt].id = WID_SUCCESS_FRAME_COUNT;
> + wid_list[wid_cnt].type = WID_INT;
> + wid_list[wid_cnt].size = sizeof(u32);
> + wid_list[wid_cnt].val = (s8 *)(&(dummyval));
> + wid_cnt++;
Doesn't that have endian issues?
> + wid_list[wid_cnt].id = WID_RECEIVED_FRAGMENT_COUNT;
> + wid_list[wid_cnt].type = WID_INT;
> + wid_list[wid_cnt].size = sizeof(u32);
> + wid_list[wid_cnt].val = (s8 *)(&(dummyval));
> + wid_cnt++;
But I'm not really sure what the pointer does, tbh.
> + wid_list[wid_cnt].id = WID_JOIN_REQ_EXTENDED;
> + wid_list[wid_cnt].type = WID_STR;
> + wid_list[wid_cnt].size = 112;
> + wid_list[wid_cnt].val = kmalloc(wid_list[wid_cnt].size, GFP_KERNEL);
I think you should declare a structure for these 112 bytes, clearly it's
something like
> + if (conn_attr->ssid) {
> + memcpy(cur_byte, conn_attr->ssid, conn_attr->ssid_len);
> + cur_byte[conn_attr->ssid_len] = '\0';
> + }
> + cur_byte += MAX_SSID_LEN;
u8 ssid[32];
> + *(cur_byte++) = INFRASTRUCTURE;
u8 type;
> +
> + if (conn_attr->ch >= 1 && conn_attr->ch <= 14) {
> + *(cur_byte++) = conn_attr->ch;
> + } else {
> + netdev_err(vif->ndev, "Channel out of range\n");
> + *(cur_byte++) = 0xFF;
> + }
u8 channel;
> + *(cur_byte++) = (bss_param->cap_info) & 0xFF;
> + *(cur_byte++) = ((bss_param->cap_info) >> 8) & 0xFF;
__le16 cap_info;
> + if (conn_attr->bssid)
> + memcpy(cur_byte, conn_attr->bssid, 6);
> + cur_byte += 6;
u8 bssid[ETH_ALEN];
> + if (conn_attr->bssid)
> + memcpy(cur_byte, conn_attr->bssid, 6);
> + cur_byte += 6;
again?
> + *(cur_byte++) = (bss_param->beacon_period) & 0xFF;
> + *(cur_byte++) = ((bss_param->beacon_period) >> 8) & 0xFF;
__le16 beacon_period;
> + *(cur_byte++) = bss_param->dtim_period;
u8 dtim_period;
etc.
Declaring it as a struct also means you don't have to do all the
put_le16_unaligned() or whatever, but can just fill the struct properly.
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 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 03/19] wilc: add host_interface.h
On Tue, 2018-10-09 at 20:01 +, adham.aboza...@microchip.com wrote: > > Do you mean that it can also be used for probing specific networks even if > they are broadcasting their SSID? Yes. > I think this might be a possible case if the user is trying to limit the scan > results to a specific network. No. Don't do any filtering based on this. Basically the only thing you should do with the (list of) SSIDs is to send a probe request containing each of them. Userspace/cfg80211 will take care that the usual empty (wildcard) SSID is included in the list, so don't send one with that by yourself, just iterate the list. If you only support sending one probe request in each scan request, then set the max # of SSIDs supported to 1, otherwise set it to the maximum (or a reasonable limit like 20 if it's in software). 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
Re: [PATCH 03/19] wilc: add host_interface.h
On Tue, 2018-10-09 at 18:36 +, adham.aboza...@microchip.com wrote: > > Johannes, is the cfg80211_scan_request.ssid used for something else > other than specifying the hidden networks that the controller should > scan for? Ahh, now I understand where you're coming from. Well, it's the SSID to include in the probe request(s), the most common thing there is the 0-length wildcard, of course. So while it may be required for hidden networks, it's not strictly related to that. johannes
Re: [PATCH 03/19] wilc: add host_interface.h
On Tue, 2018-10-09 at 17:14 +0530, Ajay Singh wrote:
> On 10/9/2018 4:06 PM, Johannes Berg wrote:
> > On Tue, 2018-10-09 at 16:04 +0530, Ajay Singh wrote:
> >
> > > > > +typedef void (*wilc_remain_on_chan_expired)(void *, u32);
> > > > > +typedef void (*wilc_remain_on_chan_ready)(void *);
> > >
> > > I think as per coding style the typedef for function pointer are allowed.
> >
> > True, I guess, but why do you need them?
>
> Actually these function pointer are used in multiple places i.e inside
> the struct and also for passing as the argument for the function. So i
> think its better to keep them as typedef to simplify and avoid any 'line
> over 80 chars' checkpatch issue. But anyway if you suggest we can modify
> to remove these typedefs .
I guess that must be part of the internal bounce buffer mechanism? I
guess leave them for now and see what falls out.
> > > > > +struct hidden_network {
> > > > >
> Yes, its not related to hidden SSID. Suppose cfg80211 scan is called
> with SSID information(active scan) then SSID info will be maintained in
> this structure.
so maybe rename this?
johannes
Re: [PATCH 03/19] wilc: add host_interface.h
On Tue, 2018-10-09 at 16:04 +0530, Ajay Singh wrote:
> > > +typedef void (*wilc_remain_on_chan_expired)(void *, u32);
> > > +typedef void (*wilc_remain_on_chan_ready)(void *);
> I think as per coding style the typedef for function pointer are allowed.
True, I guess, but why do you need them?
> > > +struct rcvd_net_info {
> > > + u8 *buffer;
> > > + u32 len;
> > > +};
> > > +
> > > +struct hidden_net_info {
> > > + u8 *ssid;
> > > + u8 ssid_len;
> > > +};
> > > +
> > > +struct hidden_network {
> > > + struct hidden_net_info *net_info;
> > > + u8 n_ssids;
> > > +};
> >
> > This seems really odd - what part doesn't cfg80211 already handle?
>
> If I understood your question correctly, you meant what extra
> functionality 'hidden_network' struct is providing.
Pretty much. It seems like you're trying to handle hidden SSIDs in some
way, but ... that's odd.
> Actually this structure is just used to keeps list of SSID's requested
> in cfg80211 'scan' callback which is passed to firmware. The values are
> extracted from 'cfg80211_scan_request[struct cfg80211_ssid *ssids
> - int n_ssids] received during scan.
So then this has nothing to do with hidden SSID?
johannes
wireless workshop (was: Re: Announcing Netdev 0x13 conference)
Hi all, On Tue, 2018-09-18 at 08:11 -0400, Jamal Hadi Salim wrote: > On behalf of the NetDev Society, this is a formal announcement that > Netdev 0x13 conference will be held in Prague, Czech Republic. > Tentative dates are March 20-22, 2019. Kalle and I have (more or less) decided to propose a wireless workshop for Netdev 0x13. In order to gauge interest and plan room size, can you reply (privately if you like) if this would work for you and you'd (want to) attend? Thanks, johannes
Re: [PATCH 02/19] wilc: add coreconfigurator.c
On Tue, 2018-10-09 at 15:12 +0530, Ajay Singh wrote:
> > > +static inline enum sub_frame_type get_sub_type(u8 *header)
> > > +{
> > > + return ((enum sub_frame_type)(header[0] & 0xFC));
> > > +}
> >
> > remove, use include/linux/ieee80211.h.
> >
>
> Did you mean to use '& IEEE80211_FCTL_STYPE' to get the frame sub type,
> instead of adding this API.
Perhaps, but I suspect you can do mostly with ieee80211_is_probe_resp()
and friends.
> > > +static inline u8 get_to_ds(u8 *header)
> > > +{
> > > + return (header[1] & 0x01);
> > > +}
> > > +
> > > +static inline u8 get_from_ds(u8 *header)
> > > +{
> > > + return ((header[1] & 0x02) >> 1);
> > > +}
> >
> > dito
> >
>
> Ack.
For this you have ieee80211_has_tods()/_fromds()/_a4() instead.
johannes
[PATCH] mac80211: avoid reflecting frames back to the client
From: Johannes Berg
I'm not really sure exactly _why_ I've been carrying a note
for what's probably _years_ to check that we don't do this,
but we clearly do reflect frames back to the station itself
if it sends such.
One way or the other, it's useless since the station doesn't
really need the AP to talk to itself, so suppress it.
While at it, clarify some of the logic by removing skb->data
references in favour of the destination address (pointer) we
already have separately.
Signed-off-by: Johannes Berg
---
net/mac80211/rx.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 96611d5dfadb..9d93d60d0635 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -2425,8 +2425,9 @@ ieee80211_deliver_skb(struct ieee80211_rx_data *rx)
if (!xmit_skb)
net_info_ratelimited("%s: failed to clone
multicast frame\n",
dev->name);
- } else if (!is_multicast_ether_addr(ehdr->h_dest)) {
- dsta = sta_info_get(sdata, skb->data);
+ } else if (!is_multicast_ether_addr(ehdr->h_dest) &&
+ !ether_addr_equal(ehdr->h_dest, ehdr->h_source)) {
+ dsta = sta_info_get(sdata, ehdr->h_dest);
if (dsta) {
/*
* The destination station is associated to
@@ -4207,11 +4208,10 @@ static bool ieee80211_invoke_fast_rx(struct
ieee80211_rx_data *rx,
if (fast_rx->internal_forward) {
struct sk_buff *xmit_skb = NULL;
- bool multicast = is_multicast_ether_addr(skb->data);
-
- if (multicast) {
+ if (is_multicast_ether_addr(addrs.da)) {
xmit_skb = skb_copy(skb, GFP_ATOMIC);
- } else if (sta_info_get(rx->sdata, skb->data)) {
+ } else if (!ether_addr_equal(addrs.da, addrs.sa) &&
+ sta_info_get(rx->sdata, addrs.da)) {
xmit_skb = skb;
skb = NULL;
}
--
2.14.4
Re: [PATCH iw] iw: fix enum warnings
On Mon, 2018-10-08 at 10:51 -0700, Brian Norris wrote: > clang warns about the misuse of enums: > > reg.c:246:26: warning: implicit conversion from enumeration type 'enum > command_identify_by' to different enumeration type 'enum id_input' > [-Wenum-conversion] > err = handle_cmd(state, CIB_NONE, 2, dump_args); > ~~^~~~ > info.c:645:26: warning: implicit conversion from enumeration type 'enum > command_identify_by' to different enumeration type 'enum id_input' > [-Wenum-conversion] > err = handle_cmd(state, CIB_NONE, 2, feat_args); > ~~^~~~ Applied, thanks. johannes
Re: [PATCH 12/19] wilc: add wilc_wfi_cfgoperations.c
On Tue, 2018-10-09 at 04:23 +, adham.aboza...@microchip.com wrote: > > > I don't know what you need the shadow stuff for, but you should remove > > it anyway, and use the cfg80211 functionality instead. If not > > sufficient, propose patches to improve it? > > The point behind using a shadow buffer was to keep the scan results > consistent between consecutive scans, and smooth out the cases where > an AP isn't found momentarily during scanning. > In this implementation, APs found during scanning are added to the > shadow list, and removed if not found again for a period of time. > > I'm not much in favour of this implementation neither since it > complicates the driver's logic, but it was serving the purpose. You really should remove it - cfg80211 *and* wpa_s already do this if required. johannes
Re: [PATCH 12/19] wilc: add wilc_wfi_cfgoperations.c
On Wed, 2018-09-26 at 15:55 +0530, Ajay Singh wrote:
>
> +#define NO_ENCRYPT 0
> +#define ENCRYPT_ENABLED BIT(0)
> +#define WEP BIT(1)
> +#define WEP_EXTENDED BIT(2)
> +#define WPA BIT(3)
> +#define WPA2 BIT(4)
> +#define AES BIT(5)
> +#define TKIP BIT(6)
> +
> +#define FRAME_TYPE_ID0
> +#define ACTION_CAT_ID24
> +#define ACTION_SUBTYPE_ID25
> +#define P2P_PUB_ACTION_SUBTYPE 30
> +
> +#define ACTION_FRAME 0xd0
> +#define GO_INTENT_ATTR_ID0x04
> +#define CHANLIST_ATTR_ID 0x0b
> +#define OPERCHAN_ATTR_ID 0x11
> +#define PUB_ACTION_ATTR_ID 0x04
> +#define P2PELEM_ATTR_ID 0xdd
> +
> +#define GO_NEG_REQ 0x00
> +#define GO_NEG_RSP 0x01
> +#define GO_NEG_CONF 0x02
> +#define P2P_INV_REQ 0x03
> +#define P2P_INV_RSP 0x04
> +#define PUBLIC_ACT_VENDORSPEC0x09
> +#define GAS_INITIAL_REQ 0x0a
> +#define GAS_INITIAL_RSP 0x0b
> +
> +#define INVALID_CHANNEL 0
> +
> +#define nl80211_SCAN_RESULT_EXPIRE (3 * HZ)
???
I mentioned namespacing, but you can't steal a different one :-)
> +#define AGING_TIME (9 * 1000)
> +#define DURING_IP_TIME_OUT 15000
Not clear what the units are - should be using HZ?
> +static void clear_shadow_scan(struct wilc_priv *priv)
> +{
> + int i;
> +
> + for (i = 0; i < priv->scanned_cnt; i++) {
> + kfree(priv->scanned_shadow[i].ies);
> + priv->scanned_shadow[i].ies = NULL;
> +
> + kfree(priv->scanned_shadow[i].join_params);
> + priv->scanned_shadow[i].join_params = NULL;
> + }
> + priv->scanned_cnt = 0;
> +}
This seems unlikely to be a good idea - why keep things around in the
driver?
> +static u32 get_rssi_avg(struct network_info *network_info)
> +{
> + u8 i;
> + int rssi_v = 0;
> + u8 num_rssi = (network_info->rssi_history.full) ?
> +NUM_RSSI : (network_info->rssi_history.index);
> +
> + for (i = 0; i < num_rssi; i++)
> + rssi_v += network_info->rssi_history.samples[i];
> +
> + rssi_v /= num_rssi;
> + return rssi_v;
> +}
Why do you need a "real" average rather than EWMA which we have helpers
for?
> +static void refresh_scan(struct wilc_priv *priv, bool direct_scan)
> +{
> + struct wiphy *wiphy = priv->dev->ieee80211_ptr->wiphy;
> + int i;
> +
> + for (i = 0; i < priv->scanned_cnt; i++) {
> + struct network_info *network_info;
> + s32 freq;
> + struct ieee80211_channel *channel;
> + int rssi;
> + struct cfg80211_bss *bss;
> +
> + network_info = >scanned_shadow[i];
> +
> + if (!memcmp("DIRECT-", network_info->ssid, 7) && !direct_scan)
> + continue;
Err, no? Don't do that? What's the point?
I don't know what you need the shadow stuff for, but you should remove
it anyway, and use the cfg80211 functionality instead. If not
sufficient, propose patches to improve it?
> + if (memcmp("DIRECT-", network_info->ssid, 7))
> + return;
same here
> +static int cancel_remain_on_channel(struct wiphy *wiphy,
> + struct wireless_dev *wdev,
> + u64 cookie)
> +{
> + struct wilc_priv *priv = wiphy_priv(wiphy);
> + struct wilc_vif *vif = netdev_priv(priv->dev);
> +
> + return wilc_listen_state_expired(vif,
> + priv->remain_on_ch_params.listen_session_id);
> +}
You really should be using the cookie.
> +static int mgmt_tx(struct wiphy *wiphy,
> +struct wireless_dev *wdev,
> +struct cfg80211_mgmt_tx_params *params,
> +u64 *cookie)
> +{
> + struct ieee80211_channel *chan = params->chan;
> + unsigned int wait = params->wait;
> + const u8 *buf = params->buf;
> + size_t len = params->len;
> + const struct ieee80211_mgmt *mgmt;
> + struct p2p_mgmt_data *mgmt_tx;
> + struct wilc_priv *priv = wiphy_priv(wiphy);
> + struct host_if_drv *wfi_drv = priv->hif_drv;
> + struct wilc_vif *vif = netdev_priv(wdev->netdev);
> + u32 buf_len = len + sizeof(p2p_vendor_spec) +
> sizeof(priv->p2p.local_random);
> + int ret = 0;
> +
> + *cookie = (unsigned long)buf;
Don't use pointers for the cookie, it leaks valuable data about KASLR.
> +static int del_virtual_intf(struct wiphy *wiphy, struct wireless_dev *wdev)
> +{
> + return 0;
> +}
Uh, not a good idea. Well, a good idea would be to actually support it,
but not to pretend to.
> +static struct wireless_dev *wilc_wfi_cfg_alloc(void)
> +{
> + struct wireless_dev
Re: [PATCH 14/19] wilc: add linux_mon.c
On Wed, 2018-09-26 at 15:55 +0530, Ajay Singh wrote:
>
> +static struct net_device *wilc_wfi_mon; /* global monitor netdev */
There might not exist platforms with multiple devices (yet), but it's
really bad practice to do this anyway.
> +static u8 srcadd[6];
> +static u8 bssid[6];
> +
> +#define IEEE80211_RADIOTAP_F_TX_RTS 0x0004 /* used rts/cts handshake */
> +#define IEEE80211_RADIOTAP_F_TX_FAIL 0x0001 /* failed due to excessive*/
Uh.. we have a radiotap include file and you already use it, why?
> +void wilc_wfi_deinit_mon_interface(void)
> +{
> + bool rollback_lock = false;
> +
> + if (wilc_wfi_mon) {
> + if (rtnl_is_locked()) {
> + rtnl_unlock();
> + rollback_lock = true;
> + }
> + unregister_netdev(wilc_wfi_mon);
> +
> + if (rollback_lock) {
> + rtnl_lock();
> + rollback_lock = false;
> + }
> + wilc_wfi_mon = NULL;
> + }
> +}
Uh, no, you really cannot do conditional locking like this.
But seeing things like this pretty much destroys all of the confidence I
might have had of the code, so I'd say we cannot merge this until you
can demonstrate somebody more familiar with Linux has reviewed it, I'm
just doing a drive-by review for the stack integration aspects (and
haven't even found where that happens yet).
johannes
Re: [PATCH 13/19] wilc: add linux_wlan.c
On Wed, 2018-09-26 at 15:55 +0530, Ajay Singh wrote:
> Moved '/driver/staging/wilc1000/linux_wlan.c' to
> 'drivers/net/wireless/microchip/wilc/'.
>
> Signed-off-by: Ajay Singh
> ---
> drivers/net/wireless/microchip/wilc/linux_wlan.c | 1161
> ++
> 1 file changed, 1161 insertions(+)
> create mode 100644 drivers/net/wireless/microchip/wilc/linux_wlan.c
Hmm. It's pretty obviously a linux driver, what's the point?
> diff --git a/drivers/net/wireless/microchip/wilc/linux_wlan.c
> b/drivers/net/wireless/microchip/wilc/linux_wlan.c
> new file mode 100644
> index 000..76c9012
> --- /dev/null
> +++ b/drivers/net/wireless/microchip/wilc/linux_wlan.c
> @@ -0,0 +1,1161 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2012 - 2018 Microchip Technology Inc., and its subsidiaries.
> + * All rights reserved.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "wilc_wfi_cfgoperations.h"
> +
> +static int dev_state_ev_handler(struct notifier_block *this,
> + unsigned long event, void *ptr)
> +{
> + struct in_ifaddr *dev_iface = ptr;
> + struct wilc_priv *priv;
> + struct host_if_drv *hif_drv;
> + struct net_device *dev;
> + u8 *ip_addr_buf;
> + struct wilc_vif *vif;
> + u8 null_ip[4] = {0};
> + char wlan_dev_name[5] = "wlan0";
Regardless of what you're trying to do, thta seems like a bad idea.
> + if (!dev_iface || !dev_iface->ifa_dev || !dev_iface->ifa_dev->dev)
> + return NOTIFY_DONE;
> +
> + if (memcmp(dev_iface->ifa_label, "wlan0", 5) &&
> + memcmp(dev_iface->ifa_label, "p2p0", 4))
> + return NOTIFY_DONE;
That too. What???
> + dev = (struct net_device *)dev_iface->ifa_dev->dev;
> + if (!dev->ieee80211_ptr || !dev->ieee80211_ptr->wiphy)
> + return NOTIFY_DONE;
> +
> + priv = wiphy_priv(dev->ieee80211_ptr->wiphy);
> + if (!priv)
> + return NOTIFY_DONE;
> +
> + hif_drv = (struct host_if_drv *)priv->hif_drv;
> + vif = netdev_priv(dev);
> + if (!vif || !hif_drv)
> + return NOTIFY_DONE;
> +
> + switch (event) {
> + case NETDEV_UP:
> + if (vif->iftype == STATION_MODE || vif->iftype == CLIENT_MODE) {
> + hif_drv->ifc_up = 1;
> + vif->obtaining_ip = false;
> + del_timer(>during_ip_timer);
> + }
> +
> + if (vif->wilc->enable_ps)
> + wilc_set_power_mgmt(vif, 1, 0);
> +
> + netdev_dbg(dev, "[%s] Up IP\n", dev_iface->ifa_label);
> +
> + ip_addr_buf = (char *)_iface->ifa_address;
> + netdev_dbg(dev, "IP add=%d:%d:%d:%d\n",
> +ip_addr_buf[0], ip_addr_buf[1],
> +ip_addr_buf[2], ip_addr_buf[3]);
%pI4, I believe, but I think you should just remove it, it likely won't
have an IP address anyway, and you might have multiple, and so this is
just broken.
> + eth_h = (struct ethhdr *)(skb->data);
> + if (eth_h->h_proto == cpu_to_be16(0x8e88))
> + netdev_dbg(ndev, "EAPOL transmitted\n");
Err, no, just remove that.
> + ih = (struct iphdr *)(skb->data + sizeof(struct ethhdr));
Sure, everything is IP. You just checked that it wasn't EAPOL?
> + udp_buf = (char *)ih + sizeof(struct iphdr);
> + if ((udp_buf[1] == 68 && udp_buf[3] == 67) ||
> + (udp_buf[1] == 67 && udp_buf[3] == 68))
> + netdev_dbg(ndev, "DHCP Message transmitted, type:%x %x %x\n",
> +udp_buf[248], udp_buf[249], udp_buf[250]);
Umm... no. Just remove that too.
> + vif->netstats.tx_packets++;
> + vif->netstats.tx_bytes += tx_data->size;
> + tx_data->bssid = wilc->vif[vif->idx]->bssid;
> + queue_count = wilc_wlan_txq_add_net_pkt(ndev, (void *)tx_data,
> + tx_data->buff, tx_data->size,
> + linux_wlan_tx_complete);
> +
> + if (queue_count > FLOW_CONTROL_UPPER_THRESHOLD) {
> + netif_stop_queue(wilc->vif[0]->ndev);
> + netif_stop_queue(wilc->vif[1]->ndev);
> + }
It seems like a pretty bad idea to hard-code two interfaces, we do
dynamic addition/removal these days, in *particular* for P2P.
> +static int wilc_mac_close(struct net_device *ndev)
> +{
> + struct wilc_priv *priv;
> + struct wilc_vif *vif = netdev_priv(ndev);
> + struct host_if_drv *hif_drv;
> + struct wilc *wl;
> +
> + if (!vif || !vif->ndev || !vif->ndev->ieee80211_ptr ||
> + !vif->ndev->ieee80211_ptr->wiphy)
> + return 0;
I'm not really sure why you're so paranoid, none of that can possibly
happen.
> + priv = wiphy_priv(vif->ndev->ieee80211_ptr->wiphy);
> + wl = vif->wilc;
> +
> + if (!priv)
> + return 0;
Nor can this.
> + hif_drv = (struct host_if_drv *)priv->hif_drv;
> +
> +
Re: [PATCH 05/19] wilc: add wilc_wlan_if.h
> +#define WILC_TX_ERR_NO_BUF (-2)
Hmm? what's wrong with just e.g. -ENOBUFS? If it doesn't go to userspace
it doesn't matter, and if it does you can't use this anyway? This would
be -ENOENT which is a bad idea.
> +
> +/
> + *
> + * Wlan Configuration ID
> + *
> + /
> +#define WILC_MULTICAST_TABLE_SIZE8
> +#define MAX_SSID_LEN33
Err, it's 32?
> +#define MAX_RATES_SUPPORTED 12
> +
> +enum bss_types {
> + INFRASTRUCTURE = 0,
> + INDEPENDENT,
> + AP,
> +};
> +
> +enum {
> + B_ONLY_MODE = 0,/* 1, 2 M, otherwise 5, 11 M */
> + G_ONLY_MODE,/* 6,12,24 otherwise 9,18,36,48,54 */
> + G_MIXED_11B_1_MODE, /* 1,2,5.5,11 otherwise all on */
> + G_MIXED_11B_2_MODE, /* 1,2,5,11,6,12,24 otherwise all on */
> +};
> +
> +enum {
> + G_SHORT_PREAMBLE= 0,/* Short Preamble */
> + G_LONG_PREAMBLE = 1,/* Long Preamble */
> + G_AUTO_PREAMBLE = 2,/* Auto Preamble Selection */
> +};
here we have a lot of those "constants should have some sort of prefix"
things ... it's not even clear if they're spec or not:
> +enum authtype {
> + OPEN_SYSTEM = 1,
> + SHARED_KEY = 2,
> + ANY = 3,
> + IEEE8021= 5
> +};
These look like they're spec but aren't ... not a good idea.
johannes
Re: [PATCH 04/19] wilc: add host_interface.c
> + *currbyte = (u32)0 & DRV_HANDLER_MASK;
You do this a few times, not sure what it's supposed to achieve?
> + if (param->flag & RETRY_LONG) {
> + u16 limit = param->long_retry_limit;
> +
> + if (limit > 0 && limit < 256) {
> + wid_list[i].id = WID_LONG_RETRY_LIMIT;
> + wid_list[i].val = (s8 *)>long_retry_limit;
> + wid_list[i].type = WID_SHORT;
> + wid_list[i].size = sizeof(u16);
> + hif_drv->cfg_values.long_retry_limit = limit;
> + } else {
> + netdev_err(vif->ndev, "Range(1~256) over\n");
> + goto unlock;
> + }
> + i++;
> + }
So ... can anyone tell me why there's a complete driver-internal
messaging infrastructure in this, that even suppresses errors like here
(out of range just results in a message rather than returning an error
to wherever it originated)?
It almost *seems* like it's a to-device infrastructure, but it can't be
since it uses host pointers everywhere?
I think this code would be far better off without the "bounce in driver
to resolve host pointers" step.
> + if (conn_attr->ssid) {
> + memcpy(cur_byte, conn_attr->ssid, conn_attr->ssid_len);
> + cur_byte[conn_attr->ssid_len] = '\0';
> + }
> + cur_byte += MAX_SSID_LEN;
again, SSIDs are not 0-terminated strings
> +static void host_int_fill_join_bss_param(struct join_bss_param *param, u8
> *ies,
> + u16 *out_index, u8 *pcipher_tc,
> + u8 *auth_total_cnt, u32 tsf_lo,
> + u8 *rates_no)
> +{
> + u8 ext_rates_no;
> + u16 offset;
> + u8 pcipher_cnt;
> + u8 auth_cnt;
> + u8 i, j;
> + u16 index = *out_index;
> +
> + if (ies[index] == WLAN_EID_SUPP_RATES) {
> + *rates_no = ies[index + 1];
> + param->supp_rates[0] = *rates_no;
> + index += 2;
> +
> + for (i = 0; i < *rates_no; i++)
> + param->supp_rates[i + 1] = ies[index + i];
> +
> + index += *rates_no;
> + } else if (ies[index] == WLAN_EID_EXT_SUPP_RATES) {
> + ext_rates_no = ies[index + 1];
> + if (ext_rates_no > (MAX_RATES_SUPPORTED - *rates_no))
> + param->supp_rates[0] = MAX_RATES_SUPPORTED;
> + else
> + param->supp_rates[0] += ext_rates_no;
> + index += 2;
> + for (i = 0; i < (param->supp_rates[0] - *rates_no); i++)
> + param->supp_rates[*rates_no + i + 1] = ies[index + i];
> +
> + index += ext_rates_no;
> + } else if (ies[index] == WLAN_EID_HT_CAPABILITY) {
> + param->ht_capable = true;
> + index += ies[index + 1] + 2;
> + } else if ((ies[index] == WLAN_EID_VENDOR_SPECIFIC) &&
> +(ies[index + 2] == 0x00) && (ies[index + 3] == 0x50) &&
> +(ies[index + 4] == 0xF2) && (ies[index + 5] == 0x02) &&
> +((ies[index + 6] == 0x00) || (ies[index + 6] == 0x01)) &&
> +(ies[index + 7] == 0x01)) {
> + param->wmm_cap = true;
> +
> + if (ies[index + 8] & BIT(7))
> + param->uapsd_cap = true;
> + index += ies[index + 1] + 2;
> + } else if ((ies[index] == WLAN_EID_VENDOR_SPECIFIC) &&
> + (ies[index + 2] == 0x50) && (ies[index + 3] == 0x6f) &&
> + (ies[index + 4] == 0x9a) &&
> + (ies[index + 5] == 0x09) && (ies[index + 6] == 0x0c)) {
> + u16 p2p_cnt;
> +
> + param->tsf = tsf_lo;
> + param->noa_enabled = 1;
> + param->idx = ies[index + 9];
> +
> + if (ies[index + 10] & BIT(7)) {
> + param->opp_enabled = 1;
> + param->ct_window = ies[index + 10];
> + } else {
> + param->opp_enabled = 0;
> + }
> +
> + param->cnt = ies[index + 11];
> + p2p_cnt = index + 12;
> +
> + memcpy(param->duration, ies + p2p_cnt, 4);
> + p2p_cnt += 4;
> +
> + memcpy(param->interval, ies + p2p_cnt, 4);
> + p2p_cnt += 4;
> +
> + memcpy(param->start_time, ies + p2p_cnt, 4);
> +
> + index += ies[index + 1] + 2;
> + } else if ((ies[index] == WLAN_EID_RSN) ||
> + ((ies[index] == WLAN_EID_VENDOR_SPECIFIC) &&
> + (ies[index + 2] == 0x00) &&
> + (ies[index + 3] == 0x50) && (ies[index + 4] == 0xF2) &&
> + (ies[index + 5] == 0x01))) {
> + u16 rsn_idx = index;
> +
> + if (ies[rsn_idx] == WLAN_EID_RSN) {
> + param->mode_802_11i = 2;
> + } else {
> + if (param->mode_802_11i == 0)
> +
Re: [PATCH 03/19] wilc: add host_interface.h
On Wed, 2018-09-26 at 15:55 +0530, Ajay Singh wrote:
> +#include
you include it
> +#include "coreconfigurator.h"
> +
> +#define IDLE_MODE0x00
> +#define AP_MODE 0x01
> +#define STATION_MODE 0x02
> +#define GO_MODE 0x03
> +#define CLIENT_MODE 0x04
> +#define ACTION 0xD0
> +#define PROBE_REQ0x40
> +#define PROBE_RESP 0x50
please use it too.
> +#define ACTION_FRM_IDX 0
> +#define PROBE_REQ_IDX1
> +#define MAX_NUM_STA 9
> +#define ACTIVE_SCAN_TIME 10
> +#define PASSIVE_SCAN_TIME1200
> +#define MIN_SCAN_TIME10
> +#define MAX_SCAN_TIME1200
> +#define DEFAULT_SCAN 0
> +#define USER_SCANBIT(0)
> +#define OBSS_PERIODIC_SCAN BIT(1)
> +#define OBSS_ONETIME_SCANBIT(2)
> +#define GTK_RX_KEY_BUFF_LEN 24
> +#define ADDKEY 0x1
> +#define REMOVEKEY0x2
> +#define DEFAULTKEY 0x4
> +#define ADDKEY_AP0x8
> +#define MAX_NUM_SCANNED_NETWORKS 100
> +#define MAX_NUM_SCANNED_NETWORKS_SHADOW 130
> +#define MAX_NUM_PROBED_SSID 10
> +#define CHANNEL_SCAN_TIME250
> +
> +#define TX_MIC_KEY_LEN 8
> +#define RX_MIC_KEY_LEN 8
> +#define PTK_KEY_LEN 16
> +
> +#define TX_MIC_KEY_MSG_LEN 26
> +#define RX_MIC_KEY_MSG_LEN 48
> +#define PTK_KEY_MSG_LEN 39
> +
> +#define PMKSA_KEY_LEN22
> +#define ETH_ALEN 6
umm?
> +#define PMKID_LEN16
??
> +#define WILC_MAX_NUM_PMKIDS 16
> +#define WILC_ADD_STA_LENGTH 40
> +#define NUM_CONCURRENT_IFC 2
> +#define DRV_HANDLER_SIZE 5
> +#define DRV_HANDLER_MASK 0x00FF
Also this file is strangely mixing
* 802.11 constants (that you shouldn't have anyway)
* driver constants/structs
* hardware/firmware-related things (at least it seems like - e.g. the
"REMOVEKEY" constant)
Please clean that up, separate the things, and pick a better
namespace... just having "REMOVEKEY" is probably not a good idea.
> +typedef void (*wilc_remain_on_chan_expired)(void *, u32);
> +typedef void (*wilc_remain_on_chan_ready)(void *);
Please no typedefs.
> +struct rcvd_net_info {
> + u8 *buffer;
> + u32 len;
> +};
> +
> +struct hidden_net_info {
> + u8 *ssid;
> + u8 ssid_len;
> +};
> +
> +struct hidden_network {
> + struct hidden_net_info *net_info;
> + u8 n_ssids;
> +};
This seems really odd - what part doesn't cfg80211 already handle?
johannes
