* Re: [PATCH v2 02/16] wilc1000: add wilc_hif.c
From: Johannes Berg @ 2019-07-12 7:42 UTC (permalink / raw)
To: Ajay.Kathat, linux-wireless
Cc: gregkh, kvalo, Adham.Abozaeid, Venkateswara.Kaja, Nicolas.Ferre,
Claudiu.Beznea
In-Reply-To: <1562896697-8002-3-git-send-email-ajay.kathat@microchip.com>
> +struct wilc_set_multicast {
> + u32 enabled;
> + u32 cnt;
> + u8 *mc_list;
> +};
> +
> +struct wilc_del_all_sta {
> + u8 assoc_sta;
> + u8 mac[WILC_MAX_NUM_STA][ETH_ALEN];
> +};
> +
> +struct wilc_op_mode {
> + __le32 mode;
> +};
> +
> +struct wilc_reg_frame {
> + bool reg;
> + u8 reg_id;
> + __le16 frame_type;
> +} __packed;
'bool' is a pretty bad idea, there's no storage guarantee for it. Use u8
instead, especially in a firmware struct.
But overall, if I remember correctly, this is a massive improvement,
last time I looked I think you basically had something like
char msg[10];
int i = 0;
msg[i++] = reg;
msg[i++] = reg_id;
msg[i++] = frame_type >> 8;
msg[i++] = (u8)frame_type;
so obviously this is *much* better.
I still think you'd benefit from putting the firmware API structs into a
separate include file so you can differentiate them, but YMMV.
> +int wilc_scan(struct wilc_vif *vif, u8 scan_source, u8 scan_type,
> + u8 *ch_freq_list, u8 ch_list_len,
> + void (*scan_result_fn)(enum scan_event,
> + struct wilc_rcvd_net_info *, void *),
> + void *user_arg, struct cfg80211_scan_request *request)
> +{
> + int result = 0;
> + struct wid wid_list[5];
> + wid_list[index].id = WID_INFO_ELEMENT_PROBE;
> + wid_list[index].type = WID_BIN_DATA;
> + wid_list[index].val = (s8 *)request->ie;
> + wid_list[index].size = request->ie_len;
> + index++;
> +
> + wid_list[index].id = WID_SCAN_TYPE;
> + wid_list[index].type = WID_CHAR;
> + wid_list[index].size = sizeof(char);
> + wid_list[index].val = (s8 *)&scan_type;
> + index++;
I still find this whole wid_list stuff to be a bit confusing, especially
since it looks like a *firmware* thing but then you have the *host
pointer* inside the value ...
There must be a translation layer somewhere, but I can't help but wonder
if that's really worth the complexity, vs. just building the right thing
directly here (with some helpers perhaps).
johannes
^ permalink raw reply
* Re: [PATCH v3 0/3] mac80211/ath11k: HE mesh support
From: Johannes Berg @ 2019-07-12 7:58 UTC (permalink / raw)
To: Sven Eckelmann; +Cc: linux-wireless, ath11k, hostap
In-Reply-To: <1610842.TRhm9evnVP@bentobox>
On Wed, 2019-07-03 at 11:23 +0200, Sven Eckelmann wrote:
>
> ~/tmp/wireshark/build/run/tshark -r /tmp/hwsim-test-logs/latest/wpas_mesh_max_peering.hwsim0.pcapng -T fields -e wlan.sa -e wlan.mesh.config.cap 'wlan.fc.type_subtype == 8'
> 02:00:00:00:01:00 0x00000009
> 02:00:00:00:00:00 0x00000009
> 02:00:00:00:01:00 0x00000009
> 02:00:00:00:02:00 0x00000009
> 02:00:00:00:00:00 0x00000008
> 02:00:00:00:01:00 0x00000009
> 02:00:00:00:02:00 0x00000009
> 02:00:00:00:00:00 0x00000008
>
> With wireshark 3.0.0:
>
> tshark -r /tmp/hwsim-test-logs/latest/wpas_mesh_max_peering.hwsim0.pcapng -T fields -e wlan.sa -e wlan.mesh.config.cap 'wlan.fc.type_subtype == 8'
> 02:00:00:00:01:00 0x00000001
> 02:00:00:00:00:00 0x00000001
> 02:00:00:00:01:00 0x00000001
> 02:00:00:00:00:00 0x00000001
> 02:00:00:00:02:00 0x00000001
> 02:00:00:00:01:00 0x00000001
> 02:00:00:00:02:00 0x00000001
> 02:00:00:00:00:00 0x00000001
>
> So I had to change the wireshark version (see below) which is used by hostapd.
> (just to document for me what I have forced it to a different version)
What. +hostap list.
This makes no sense, is that a tshark bug in one of the versions?
Maybe we should just use the JSON output, and parse that, but if tshark
now has a different idea of what the "wlan.mesh.config.cap" field is,
that won't help us at all?
Older versions were misparsing the HE element, but that has a length so
should only affect the HE element *itself*?
So ultimately, what do we do here?
Should we take this and sort out the tests?
johannes
^ permalink raw reply
* Re: [PATCH v3 1/2] nl80211: Add support for EDMG channels
From: Johannes Berg @ 2019-07-12 8:03 UTC (permalink / raw)
To: Alexei Lazar; +Cc: linux-wireless, wil6210
In-Reply-To: <b3b97ea0dbe2fe115976c2728a041171@codeaurora.org>
Hi Alexei,
> Channels is a bitmap of 2.16GHz (normal) channels 1..6
> bw_config defines the allowed combinations (bonding) of these "normal"
> channels.
But bw_config is not itself some sort of bitmap, it's just a kind of
enum value, right? Maybe we should actually use an enum for it rather
than having the hardcoded values?
> Let's say driver reports support for channels number 1,2,3
> (channels=0x7).
> Example #1: driver reports bw_config=5
> bw_config=5 allows combinations of 1 or 2 channels.
> It means driver can operate in one of these combinations:
> Channel 1 only
> Channel 2 only
> Channel 3 only
> Channels 1+2 (cb2)
> Channels 2+3 (cb2)
OK.
> The primary channel BTW must be one of the operational channels so if
> driver choose channels 1+3 then primary channel cannot be 2.
Makes sense.
> > It seems to me that you should, however, expose n_bonded_ch to
> > userspace? We do expose all the details about the bitrate normally, see
> > nl80211_put_sta_rate(). We do calculate the bitrate to expose that too,
> > but do expose all the details too.
> >
> > Hmm. Looking at that, it looks like DMG doesn't actually do that. So
> > perhaps not, though it seems to me that it ought to be possible?
>
> We will look into that and address in separate patch.
Right, separate patch definitely makes sense. Thanks!
> > > + /* check bw_config against contiguous edmg channels */
> > > + switch (chandef->edmg.bw_config) {
> > > + case 4:
> > > + case 8:
> > > + case 12:
> > > + if (max_continuous < 1)
> > > + return false;
> > > + break;
I think that indeed this could be clearer if you have an enum.
johannes
^ permalink raw reply
* Re: [PATCH v4 1/2] nl80211: Add support for EDMG channels
From: Johannes Berg @ 2019-07-12 8:35 UTC (permalink / raw)
To: Alexei Avshalom Lazar; +Cc: linux-wireless, wil6210
In-Reply-To: <1562508727-17082-2-git-send-email-ailizaro@codeaurora.org>
On Sun, 2019-07-07 at 17:12 +0300, Alexei Avshalom Lazar wrote:
>
> /**
> + * struct ieee80211_edmg - EDMG configuration
> + *
> + * This structure describes most essential parameters needed
> + * to describe 802.11ay EDMG configuration
> + *
> + * @channels: bitmap that indicates the 2.16 GHz channel(s)
> + * that are allowed to be used for transmissions.
> + * Bit 0 indicates channel 1, bit 1 indicates channel 2, etc.
> + * Set to 0 indicate EDMG not supported.
> + * @bw_config: Channel BW Configuration subfield encodes
> + * the allowed channel bandwidth configurations
> + */
> +struct ieee80211_edmg {
> + u8 channels;
> + u8 bw_config;
> +};
So I think the enum here like I just said might be good. I don't know
what to really the values in it, but having something that says "yes
this is just a magic number from the spec" would be nice...
Maybe also call it "struct ieee80211_edmg_chan" or something? Not sure,
it's sort of covering both chan and cfg, so probably what you have is
better.
> @@ -350,6 +369,7 @@ struct ieee80211_supported_band {
> int n_bitrates;
> struct ieee80211_sta_ht_cap ht_cap;
> struct ieee80211_sta_vht_cap vht_cap;
> + struct ieee80211_edmg edmg_cap;
Yeah, I think if you have edmg_cap as the variable name, your naming is
better :)
> + * @edmg: define the EDMG channels.
> + * This may specify multiple channels and bonding options for the driver
> + * to choose from, based on BSS configuration.
Here actually I don't understand how you'd specify *multiple* bonding
options? The bw_config is an enum, right?
Or maybe this case should actually *not* be the same struct, but a
different struct with a *bitmap* of the enum values?
But then it'd need a u16 anyway since the enum values go higher, up to
16 according to your code below:
> +#define NL80211_EDMG_BW_CONFIG_MIN 4
> +#define NL80211_EDMG_BW_CONFIG_MAX 15
> +static bool cfg80211_valid_60g_freq(u32 freq)
> +{
> + return (freq >= 58320 && freq <= 70200);
nit: no need for the parentheses
> +static bool cfg80211_edmg_chandef_valid(const struct cfg80211_chan_def *chandef)
> +{
> + int max_contiguous = 0;
> + int num_of_enabled = 0;
> + int contiguous = 0;
> + int i;
> +
> + if (!chandef->edmg.channels && !chandef->edmg.bw_config)
> + return true;
> +
> + if ((!chandef->edmg.channels && chandef->edmg.bw_config) ||
> + (chandef->edmg.channels && !chandef->edmg.bw_config) ||
> + !cfg80211_valid_60g_freq(chandef->chan->center_freq))
> + return false;
That's a bit hard to read, maybe pull out the valid_60g_freq into a
separate if statement?
And after the "!channels && !bw_config" part, you don't actually need
the whole condition that way, you just need
if (!channels || !bw_config)
return false;
since both cannot be unset at this point.
> @@ -112,7 +206,7 @@ bool cfg80211_chandef_valid(const struct cfg80211_chan_def *chandef)
> return false;
> }
>
> - return true;
> + return cfg80211_edmg_chandef_valid(chandef);
I *think* I might prefer the "could this be an EDMG channel" condition
to be outside, i.e.
if ((chandef->edmg.channels || chandef->edmg.bw_config) &&
!cfg80211_edmg_chandef_valid(chandef))
return false;
return true;
That's clearly equivalent to what you have now, but I think it's easier
to understand that we only enter the "edmg_chandef_valid()" when it
looks like an EDMG channel and we thus need to validate it.
I'd even go as far as saying that we should have an inline for it in
cfg80211.h:
static inline bool cfg80211_chandef_is_edmg(...)
{
return chandef->edmg.channels || chandef->edmg.bw_config;
}
and we use that in the code I wrote above, as well as other places that
want to ask this question.
> + [NL80211_ATTR_WIPHY_EDMG_CHANNELS] = { .type = NLA_U8 },
Since you say there are only 6 channels, this probably also has a lower
bound of 1 (need to set at least one bit) and an upper bound of 63 (all
lower 6 bits set)?
In any case, thanks for your work on this, and especially for your
patience with me reviewing.
johannes
^ permalink raw reply
* Re: [RFC PATCH v3 0/2] cfg80211: fix duplicated scan entries after channel switch
From: Johannes Berg @ 2019-07-12 9:11 UTC (permalink / raw)
To: Sergey Matyukevich, linux-wireless@vger.kernel.org
Cc: Igor Mitsyanko, Mikhail Karpenko
In-Reply-To: <20190710173651.15770-1-sergey.matyukevich.os@quantenna.com>
> Suggested approach to handle non-transmitting BSS entries is simplified in the
> following sense. If new entries have been already created after channel switch,
> only transmitting bss will be updated using IEs of new entry for the same
> transmitting bss. Non-transmitting bss entries will be updated as soon as
> new mgmt frames are received. Updating non-transmitting bss entries seems
> too expensive: nested nontrans_list traversing is needed since we can not
> rely on the same order of old and new non-transmitting entries.
That sounds like a reasonable trade-off. I do wonder though what happens
if we're connected to a non-transmitting BSS?
johannes
^ permalink raw reply
* Re: [RFC PATCH v3 1/2] cfg80211: refactor cfg80211_bss_update
From: Johannes Berg @ 2019-07-12 9:12 UTC (permalink / raw)
To: Sergey Matyukevich, linux-wireless@vger.kernel.org
Cc: Igor Mitsyanko, Mikhail Karpenko
In-Reply-To: <20190710173651.15770-2-sergey.matyukevich.os@quantenna.com>
On Wed, 2019-07-10 at 17:37 +0000, Sergey Matyukevich wrote:
> This patch implements minor refactoring for cfg80211_bss_update function.
> Code path for updating known BSS is extracted into dedicated
> cfg80211_update_known_bss function.
>
Looks good, thanks for splitting this out.
I'm not going to apply anything until net-next also opens up again
though.
johannes
^ permalink raw reply
* Re: [RFC PATCH v3 0/2] cfg80211: fix duplicated scan entries after channel switch
From: Sergey Matyukevich @ 2019-07-12 9:27 UTC (permalink / raw)
To: Johannes Berg
Cc: linux-wireless@vger.kernel.org, Igor Mitsyanko, Mikhail Karpenko
In-Reply-To: <1c371a5921200a11da459b591df121bbcb0f967d.camel@sipsolutions.net>
On Fri, Jul 12, 2019 at 11:11:19AM +0200, Johannes Berg wrote:
>
> [External Email]: This email arrived from an external source - Please exercise caution when opening any attachments or clicking on links.
>
> > Suggested approach to handle non-transmitting BSS entries is simplified in the
> > following sense. If new entries have been already created after channel switch,
> > only transmitting bss will be updated using IEs of new entry for the same
> > transmitting bss. Non-transmitting bss entries will be updated as soon as
> > new mgmt frames are received. Updating non-transmitting bss entries seems
> > too expensive: nested nontrans_list traversing is needed since we can not
> > rely on the same order of old and new non-transmitting entries.
>
> That sounds like a reasonable trade-off. I do wonder though what happens
> if we're connected to a non-transmitting BSS?
Well, here I rely upon the assumption that CSA IEs of non-transmitting BSS
are handled correctly by mac80211 or any FullMAC firmware. And if we are
connected to non-transmitting BSS rather than transmitting one, the
following code in the beginning of new cfg80211_update_assoc_bss_entry
function is supposed to care about this use-case:
+ /* use transmitting bss */
+ if (cbss->pub.transmitted_bss)
+ cbss = container_of(cbss->pub.transmitted_bss,
+ struct cfg80211_internal_bss,
+ pub);
In other words, regardless of which BSS we are connected to, the whole
hierarchy of non-transmitting BSS entries will be updated, including
their channels and location in rb-tree.
Regards,
Sergey
^ permalink raw reply
* Re: [RFC 0/8] nl80211: add 6GHz band support
From: Johannes Berg @ 2019-07-12 9:30 UTC (permalink / raw)
To: Arend Van Spriel; +Cc: linux-wireless, Jouni Malinen, Tova Mussai
In-Reply-To: <8ca6ef2e-b41b-da40-b29a-77666d440495@broadcom.com>
On Thu, 2019-07-11 at 13:30 +0200, Arend Van Spriel wrote:
>
> I assume user-space does not necessarily need the band, but hostapd
> needs to be aware that it is operating in 6GHz to setup the correct
> (information) elements in the beacon. Obviously the operating classes
> can be used there, but I don't think there is any issue with nl80211
> exposing a 6G band.
Yeah, I don't really see any *issue* with it, in many places we don't
really even care about the band enum.
In a sense, *most* of the places that consider the channel don't
actually care about the band, the channel num/freq conversion helpers
are a bit of the odd ones out in that regard. In the chandef, for
example, we don't have the band.
Really the original use for the band enum was mostly around the
advertising if capabilities. As you pointed out, 6GHz actually *does*
have different capabilities, though it's not clear to me what exactly
the behaviour differences are. Scanning is a big area, of course.
When we discussed splitting up or not the band, I think what we mostly
considered was the channel num/freq conversion helpers, and Jouni
pointed out that really what we should be doing for those isn't to
consider the band but the operating class instead.
So I'm starting to think that perhaps the decision we came to in Prague
was a little hasty, without considering the full impact?
I do completely agree that we should have knowledge about the operating
classes in the kernel eventually, and probably we will need to have it
here if we need to parse reduced neighbor reports etc. OTOH, we have
already ieee80211_operating_class_to_band(), and that seems sufficient,
though probably we should consider a combined helper that takes
opclass/chan# instead of having to call two functions?
However, from a feature advertisement point of view, we might very well
consider 6 GHz to be a separate nl80211 band, in particular if there
*are* indeed differences around what rates are permitted? Which is
really the only place where we care. Or maybe, thinking about this more,
if there could be devices that have different capabilities in 6 GHz than
in 5 GHz, in the sense of HT/VHT/HE capabilities?
Can somebody do the legwork and really go look at the spec to figure out
what the differences are? I'm not even sure now legacy rates are
permitted or not - you (Arend) seemed to imply they're not, and Igor
said only for beacons ...
I tend to think that this would be the deciding factor. For example, if
we do end up sending a probe request on 6 GHz, would we include a
different supported rates element than on 5 GHz? Or might there even be
devices that have different PHY paths and thus possibly different
capabilities for 5 and 6 GHz, essentially requiring a new place (a new
band enumerator) to store those capabilities, so we don't have to try to
figure out the difference in code later?
I'm almost tempted to say that given all these possibilities we should
in fact add a new value to the band enum, worst case we just duplicate
some data, but if there do end up being differences we can handle them
much more gracefully than if we put everything into 5 GHz.
Jouni, what do you think?
Thanks,
johannes
^ permalink raw reply
* Re: [PATCH v3 0/3] mac80211/ath11k: HE mesh support
From: Sven Eckelmann @ 2019-07-12 9:36 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, ath11k, hostap
In-Reply-To: <06c7c1a2c8d0f955cb107475d17587c156fb19de.camel@sipsolutions.net>
[-- Attachment #1.1: Type: text/plain, Size: 2822 bytes --]
On Friday, 12 July 2019 09:58:50 CEST Johannes Berg wrote:
> On Wed, 2019-07-03 at 11:23 +0200, Sven Eckelmann wrote:
> >
> > ~/tmp/wireshark/build/run/tshark -r /tmp/hwsim-test-logs/latest/wpas_mesh_max_peering.hwsim0.pcapng -T fields -e wlan.sa -e wlan.mesh.config.cap 'wlan.fc.type_subtype == 8'
> > 02:00:00:00:01:00 0x00000009
[...]
> > With wireshark 3.0.0:
> >
> > tshark -r /tmp/hwsim-test-logs/latest/wpas_mesh_max_peering.hwsim0.pcapng -T fields -e wlan.sa -e wlan.mesh.config.cap 'wlan.fc.type_subtype == 8'
> > 02:00:00:00:01:00 0x00000001
[...]
> > So I had to change the wireshark version (see below) which is used by hostapd.
> > (just to document for me what I have forced it to a different version)
>
> What. +hostap list.
>
> This makes no sense, is that a tshark bug in one of the versions?
Yes (but more a bug in the ieee80211 dissector), see
commit f3ef8575d462 ("ieee80211: fix wrong offset for mesh configuration
capability bitmask") [1].
I've also attached the pcap in case there are still doubts about my statement.
As you can see, it will just fail to parse the mesh peering management element
correctly. It should parse the last byte of the element payload but it falls
back to parsing the first byte (path selection protocol) as capabilities. See
9.4.2.98 "Mesh Configuration element" from 802.11-2016 for details.
There is already a workaround for that in the hostap testcases:
if all_cap_one:
# It looks like tshark parser was broken at some point for
# wlan.mesh.config.cap which is now (tshark 2.6.3) pointing to incorrect
# field (same as wlan.mesh.config.ps_protocol). This used to work with
# tshark 2.2.6.
#
# For now, assume the capability field ends up being the last octet of
# the frame.
one = [0, 0, 0]
zero = [0, 0, 0]
addrs = [addr0, addr1, addr2]
for idx in range(3):
addr = addrs[idx]
out = run_tshark_json(capfile, filt + " && wlan.sa == " + addr)
pkts = json.loads(out)
for pkt in pkts:
frame = pkt["_source"]["layers"]["frame_raw"][0]
cap = int(frame[-2:], 16)
if cap & 0x01:
one[idx] += 1
else:
zero[idx] += 1
logger.info("one: " + str(one))
logger.info("zero: " + str(zero))
But maybe you already spotted the problem - it requires that mesh
configuration element is the last element. Which is not the case here -
similar to 5GHz tests (where you have most likely a VHT cap/oper element
after the meshconf_ie).
I hope that this makes more sense now.
Kind regards,
Sven
[1] https://github.com/wireshark/wireshark/commit/f3ef8575d4620a62f1c4609bf14961c3e78993f3
[-- Attachment #1.2: wpas_mesh_max_peering.hwsim0.pcapng.gz --]
[-- Type: application/x-pcapng, Size: 1793 bytes --]
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC PATCH v3 0/2] cfg80211: fix duplicated scan entries after channel switch
From: Johannes Berg @ 2019-07-12 9:40 UTC (permalink / raw)
To: Sergey Matyukevich
Cc: linux-wireless@vger.kernel.org, Igor Mitsyanko, Mikhail Karpenko
In-Reply-To: <20190712092716.ywnkns473s5rtoku@bars>
On Fri, 2019-07-12 at 09:27 +0000, Sergey Matyukevich wrote:
> On Fri, Jul 12, 2019 at 11:11:19AM +0200, Johannes Berg wrote:
> >
> > [External Email]: This email arrived from an external source - Please exercise caution when opening any attachments or clicking on links.
Heh, you have a not so fun email system that rewrites mails ...
> > > Suggested approach to handle non-transmitting BSS entries is simplified in the
> > > following sense. If new entries have been already created after channel switch,
> > > only transmitting bss will be updated using IEs of new entry for the same
> > > transmitting bss. Non-transmitting bss entries will be updated as soon as
> > > new mgmt frames are received. Updating non-transmitting bss entries seems
> > > too expensive: nested nontrans_list traversing is needed since we can not
> > > rely on the same order of old and new non-transmitting entries.
> >
> > That sounds like a reasonable trade-off. I do wonder though what happens
> > if we're connected to a non-transmitting BSS?
>
> Well, here I rely upon the assumption that CSA IEs of non-transmitting BSS
> are handled correctly by mac80211 or any FullMAC firmware. And if we are
> connected to non-transmitting BSS rather than transmitting one, the
> following code in the beginning of new cfg80211_update_assoc_bss_entry
> function is supposed to care about this use-case:
Right, it will be updated on RX. But then if we chanswitch, we would
probably (mac80211 using a pointer to the non-transmitting BSS) update
only one of the nontransmitting BSSes?
Just saying that maybe we need to be careful there - or your wording
might be incorrect. We might end up updating a *nontransmitting* BSS,
and then its transmitting/other non-tx ones only later?
johannes
^ permalink raw reply
* Re: [PATCH v3 0/3] mac80211/ath11k: HE mesh support
From: Johannes Berg @ 2019-07-12 9:42 UTC (permalink / raw)
To: Sven Eckelmann; +Cc: linux-wireless, ath11k, hostap
In-Reply-To: <2019422.XptUlqRJNA@bentobox>
On Fri, 2019-07-12 at 11:36 +0200, Sven Eckelmann wrote:
>
> There is already a workaround for that in the hostap testcases:
>
> if all_cap_one:
> # It looks like tshark parser was broken at some point for
> # wlan.mesh.config.cap which is now (tshark 2.6.3) pointing to incorrect
> # field (same as wlan.mesh.config.ps_protocol). This used to work with
> # tshark 2.2.6.
> #
> # For now, assume the capability field ends up being the last octet of
> # the frame.
> But maybe you already spotted the problem - it requires that mesh
> configuration element is the last element. Which is not the case here -
> similar to 5GHz tests (where you have most likely a VHT cap/oper element
> after the meshconf_ie).
>
> I hope that this makes more sense now.
Ah, yes, it does. So I guess we need to update/fix that workaround. And
I guess newer tshark (which you used) is fixed again, if I understand
correctly?
johannes
^ permalink raw reply
* Re: iwl_mvm_add_new_dqa_stream_wk BUG in lib/list_debug.c:56
From: Luciano Coelho @ 2019-07-12 9:57 UTC (permalink / raw)
To: Marc Haber, Yussuf Khalil
Cc: linux-wireless, linux-kernel, netdev, Johannes Berg,
Emmanuel Grumbach, Intel Linux Wireless
In-Reply-To: <20190607204421.GK31088@torres.zugschlus.de>
On Fri, 2019-06-07 at 22:44 +0200, Marc Haber wrote:
> On Fri, Jun 07, 2019 at 10:20:56PM +0200, Yussuf Khalil wrote:
> > CC'ing iwlwifi maintainers to get some attention for this issue.
> >
> > I am experiencing the very same bug on a ThinkPad T480s running 5.1.6 with
> > Fedora 30. A friend is seeing it on his X1 Carbon 6th Gen, too. Both have an
> > "Intel Corporation Wireless 8265 / 8275" card according to lspci.
>
> I have an older 04:00.0 Network controller [0280]: Intel Corporation
> Wireless 8260 [8086:24f3] (rev 3a) on a Thinkpad X260.
>
> > Notably, in all cases I've observed it occurred right after roaming from one
> > AP to another (though I can't guarantee this isn't a coincidence).
>
> I also have multiple Access Points broadcasting the same SSID in my
> house, and yes, I experience those issues often when I move from one
> part of the hose to another. I have, however, also experienced it in a
> hotel when I was using the mobile hotspot offered by my mobile, so that
> was clearly not a roaming situation.
Hi,
Sorry this got under the radar for a while. Yesterday someone created
a bugzilla entry with the same error:
https://bugzilla.kernel.org/show_bug.cgi?id=204141
I'm going to file an internal bug report and then have someone look
further into it.
Any additional comments/reproductions/etc. please use that bugzilla
entry.
Thanks for reporting!
--
Cheers,
Luca.
^ permalink raw reply
* [PATCH][master/pending] ath10k: assign new interfaces type to parent type
From: Isaac Hermida @ 2019-07-12 10:08 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless
This is needed to have the AP mode working for SDIO.
Tested with lastest master/pending code on a QCA6564 chip with firmware
firmware-sdio-6.bin_WLAN.RMH.4.4.1-00011-QCARMSWP-2
Signed-off-by: Isaac Hermida <isaac.hermida@digi.com>
---
drivers/net/wireless/ath/ath10k/mac.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index e43a566eef77..2c4f8da98b24 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -5150,6 +5150,9 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
arvif->ar = ar;
arvif->vif = vif;
+ // assign type of the new interface to the parent type (SDIO, PCI, etc)
+ ar->dev_type = arvif->ar->bus_param.dev_type;
+
INIT_LIST_HEAD(&arvif->list);
INIT_WORK(&arvif->ap_csa_work, ath10k_mac_vif_ap_csa_work);
INIT_DELAYED_WORK(&arvif->connection_loss_work,
^ permalink raw reply related
* Re: [RFC/RFT] rt2x00: do not set IEEE80211_TX_STAT_AMPDU_NO_BACK on tx status
From: Stanislaw Gruszka @ 2019-07-12 10:25 UTC (permalink / raw)
To: linux-wireless
Cc: Tomislav Požega, Daniel Golle, Felix Fietkau, Mathias Kresin
In-Reply-To: <20190704110652.3955-1-sgruszka@redhat.com>
On Thu, Jul 04, 2019 at 01:06:52PM +0200, Stanislaw Gruszka wrote:
> According to documentation IEEE80211_TX_STAT_AMPDU_NO_BACK is suppose
> to be used when we do not recive BA (BlockAck). However on rt2x00 we
> use it when remote station fail to decode one or more subframes within
> AMPDU (some bits are not set in BlockAck bitmap). Setting the flag result
> in sent of BAR (BlockAck Request) frame and this might result of abuse
> of BA session, since remote station can sent BA with incorrect
> sequence numbers after receiving BAR. This problem is visible especially
> when connecting two rt2800 devices.
>
> Previously I observed some performance benefits when using the flag
> when connecting with iwlwifi devices. But currently possibly due
> to reacent changes in rt2x00 removing the flag has no effect on
> those test cases.
>
> So remove the IEEE80211_TX_STAT_AMPDU_NO_BACK.
>
> Perhaps we should send BAR exlicitly on BA session start/stop
> and when remote STA went to PowerSave mode (for AP) like mt76 does.
> But I do not understand for what this is needed.
This commit
https://github.com/openwrt/mt76/commit/3e447e7797d64dbf4dc1dd5553d08be0d8150d7e
explained that sending BAR on stop aggregation is workaround for
buggy clients. I can implement that on rt2x00. But I will skip
sending BAR on remote station PS wakeup, since we do not implement
all PS related code in rt2x00.
Stanislaw
^ permalink raw reply
* [RFC/RFT v2] rt2x00: do not set IEEE80211_TX_STAT_AMPDU_NO_BACK on tx status
From: Stanislaw Gruszka @ 2019-07-12 10:32 UTC (permalink / raw)
To: linux-wireless
Cc: Tomislav Požega, Daniel Golle, Felix Fietkau, Mathias Kresin
According to documentation IEEE80211_TX_STAT_AMPDU_NO_BACK is suppose
to be used when we do not receive BA (BlockAck). However on rt2x00 we
use it when remote station fail to decode one or more subframes within
AMPDU (some bits are not set in BlockAck bitmap). Setting the flag result
in sent of BAR (BlockAck Request) frame and this might result of abuse
of BA session, since remote station can sent BA with incorrect
sequence numbers after receiving BAR. This problem is visible especially
when connecting two rt2800 devices.
Previously I observed some performance benefits when using the flag
when connecting with iwlwifi devices. But currently possibly due
to recent changes in rt2x00 removing the flag has no effect on
those test cases.
So remove the IEEE80211_TX_STAT_AMPDU_NO_BACK.
Additionally partially mimic mt76 behaviour: send BAR when
starting/stopping BA session to workaround problems with some buggy
clients. Do not sent BAR on PS wakeup since we lack all PS handling
code what mt76 has.
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 4 ++++
drivers/net/wireless/ralink/rt2x00/rt2x00.h | 1 +
drivers/net/wireless/ralink/rt2x00/rt2x00dev.c | 3 ---
drivers/net/wireless/ralink/rt2x00/rt2x00mac.c | 18 ++++++++++++++++++
4 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index c9b957ac5733..4a996550288e 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -10481,14 +10481,18 @@ int rt2800_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
*/
break;
case IEEE80211_AMPDU_TX_START:
+ sta_priv->agg_ssn[tid] = IEEE80211_SN_TO_SEQ(params->ssn);
ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid);
break;
case IEEE80211_AMPDU_TX_STOP_CONT:
case IEEE80211_AMPDU_TX_STOP_FLUSH:
+ ieee80211_send_bar(vif, sta->addr, tid, sta_priv->agg_ssn[tid]);
+ break;
case IEEE80211_AMPDU_TX_STOP_FLUSH_CONT:
ieee80211_stop_tx_ba_cb_irqsafe(vif, sta->addr, tid);
break;
case IEEE80211_AMPDU_TX_OPERATIONAL:
+ ieee80211_send_bar(vif, sta->addr, tid, sta_priv->agg_ssn[tid]);
break;
default:
rt2x00_warn((struct rt2x00_dev *)hw->priv,
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00.h b/drivers/net/wireless/ralink/rt2x00/rt2x00.h
index 7e43690a861c..d35ef06c5c7a 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00.h
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00.h
@@ -500,6 +500,7 @@ struct rt2x00intf_conf {
*/
struct rt2x00_sta {
int wcid;
+ u16 agg_ssn[IEEE80211_NUM_TIDS];
};
static inline struct rt2x00_sta* sta_to_rt2x00_sta(struct ieee80211_sta *sta)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
index 35414f97a978..ad063c920323 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
@@ -371,9 +371,6 @@ static void rt2x00lib_fill_tx_status(struct rt2x00_dev *rt2x00dev,
IEEE80211_TX_CTL_AMPDU;
tx_info->status.ampdu_len = 1;
tx_info->status.ampdu_ack_len = success ? 1 : 0;
-
- if (!success)
- tx_info->flags |= IEEE80211_TX_STAT_AMPDU_NO_BACK;
}
if (rate_flags & IEEE80211_TX_RC_USE_RTS_CTS) {
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
index beb20c5faf5f..14896d37f0cc 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
@@ -86,6 +86,21 @@ static int rt2x00mac_tx_rts_cts(struct rt2x00_dev *rt2x00dev,
return retval;
}
+static void rt2x00mac_save_agg_seqno(struct sk_buff *skb,
+ struct ieee80211_sta *sta)
+{
+ struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
+ struct rt2x00_sta *sta_priv = sta_to_rt2x00_sta(sta);
+ u8 tid;
+
+ if (!ieee80211_is_data_qos(hdr->frame_control) ||
+ !ieee80211_is_data_present(hdr->frame_control))
+ return;
+
+ tid = skb->priority & IEEE80211_QOS_CTL_TID_MASK;
+ sta_priv->agg_ssn[tid] = le16_to_cpu(hdr->seq_ctrl) + 0x10;
+}
+
void rt2x00mac_tx(struct ieee80211_hw *hw,
struct ieee80211_tx_control *control,
struct sk_buff *skb)
@@ -119,6 +134,9 @@ void rt2x00mac_tx(struct ieee80211_hw *hw,
goto exit_free_skb;
}
+ if (control->sta)
+ rt2x00mac_save_agg_seqno(skb, control->sta);
+
/*
* If CTS/RTS is required. create and queue that frame first.
* Make sure we have at least enough entries available to send
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v3 0/3] mac80211/ath11k: HE mesh support
From: Sven Eckelmann @ 2019-07-12 10:38 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, ath11k, hostap
In-Reply-To: <9a1d7a6651d3bf6c4a43c5bc8659df690c009328.camel@sipsolutions.net>
[-- Attachment #1: Type: text/plain, Size: 1529 bytes --]
On Friday, 12 July 2019 11:42:51 CEST Johannes Berg wrote:
> On Fri, 2019-07-12 at 11:36 +0200, Sven Eckelmann wrote:
> >
> > There is already a workaround for that in the hostap testcases:
> >
> > if all_cap_one:
> > # It looks like tshark parser was broken at some point for
> > # wlan.mesh.config.cap which is now (tshark 2.6.3) pointing to incorrect
> > # field (same as wlan.mesh.config.ps_protocol). This used to work with
> > # tshark 2.2.6.
> > #
> > # For now, assume the capability field ends up being the last octet of
> > # the frame.
>
> > But maybe you already spotted the problem - it requires that mesh
> > configuration element is the last element. Which is not the case here -
> > similar to 5GHz tests (where you have most likely a VHT cap/oper element
> > after the meshconf_ie).
> >
> > I hope that this makes more sense now.
>
> Ah, yes, it does. So I guess we need to update/fix that workaround.
I will prepare a patch now for hostap after lunch.
> And
> I guess newer tshark (which you used) is fixed again, if I understand
> correctly?
Yes and no, the master branch is fixed. But unfortunately, there is no
release with this fix.
And the problems is there since commit 3c427376579a ("802.11: Use
proto_tree_add_bitmask") and release v2.4.0rc0. But unfortunately, the
workaround was added with commit 2cbaf0de223b ("tests: Work around tshark bug
in wpas_mesh_max_peering") instead of bringing a fix upstream.
Kind regards,
Sven
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC 0/8] nl80211: add 6GHz band support
From: Arend Van Spriel @ 2019-07-12 10:40 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, Jouni Malinen, Tova Mussai
In-Reply-To: <a6e317539fbd53ad5e2fd62ce3544260a3f74e77.camel@sipsolutions.net>
On 7/12/2019 11:30 AM, Johannes Berg wrote:
> On Thu, 2019-07-11 at 13:30 +0200, Arend Van Spriel wrote:
>>
>> I assume user-space does not necessarily need the band, but hostapd
>> needs to be aware that it is operating in 6GHz to setup the correct
>> (information) elements in the beacon. Obviously the operating classes
>> can be used there, but I don't think there is any issue with nl80211
>> exposing a 6G band.
>
> Yeah, I don't really see any *issue* with it, in many places we don't
> really even care about the band enum.
>
> In a sense, *most* of the places that consider the channel don't
> actually care about the band, the channel num/freq conversion helpers
> are a bit of the odd ones out in that regard. In the chandef, for
> example, we don't have the band.
>
> Really the original use for the band enum was mostly around the
> advertising if capabilities. As you pointed out, 6GHz actually *does*
> have different capabilities, though it's not clear to me what exactly
> the behaviour differences are. Scanning is a big area, of course.
For starters a 6G STA has to add "HE extended capabilities" element.
This contains capabilities that are taken from HT/VHT. Reason being that
there is following requirement (clause 26.17.2.1):
"""
A 6 GHz HE STA shall not transmit an HT Capabilities element, VHT
Capabilities element, HT Operation
element, VHT Operation element or an HE Operation element that contains
a VHT Operation Information
field.
"""
The inclusion of the "HE extended capabilities" element is determined by
the dot116GOptionImplemented option. (band[6G] != NULL) provides that
condition although there are other ways to solve that I guess :-p
Come to think of it. Does mac80211 need that. Guess IEs are provided by
user-space, right?
> When we discussed splitting up or not the band, I think what we mostly
> considered was the channel num/freq conversion helpers, and Jouni
> pointed out that really what we should be doing for those isn't to
> consider the band but the operating class instead.
>
> So I'm starting to think that perhaps the decision we came to in Prague
> was a little hasty, without considering the full impact?
>
> I do completely agree that we should have knowledge about the operating
> classes in the kernel eventually, and probably we will need to have it
> here if we need to parse reduced neighbor reports etc. OTOH, we have
> already ieee80211_operating_class_to_band(), and that seems sufficient,
> though probably we should consider a combined helper that takes
> opclass/chan# instead of having to call two functions?
>
> However, from a feature advertisement point of view, we might very well
> consider 6 GHz to be a separate nl80211 band, in particular if there
> *are* indeed differences around what rates are permitted? Which is
> really the only place where we care. Or maybe, thinking about this more,
> if there could be devices that have different capabilities in 6 GHz than
> in 5 GHz, in the sense of HT/VHT/HE capabilities?
Regarding rates the answer seem to be in clause 26.17.2.1 as well:
"""
A STA shall not transmit an HT PPDU in the 6 GHz band. A STA shall not
transmit a VHT PPDU in the
6 GHz band. A STA shall not transmit a DSSS, HR/DSSS, or ERP-OFDM PPDU
in the 6 GHz band.
"""
I may be wrong but that seems to say only HE rates are allowed.
> Can somebody do the legwork and really go look at the spec to figure out
> what the differences are? I'm not even sure now legacy rates are
> permitted or not - you (Arend) seemed to imply they're not, and Igor
> said only for beacons ...
Regarding beacons the rate requirement is in clause 26.15.6, which
basically states that beacons have to be transmitted with HE rate where
NSS equals 1.
> I tend to think that this would be the deciding factor. For example, if
> we do end up sending a probe request on 6 GHz, would we include a
> different supported rates element than on 5 GHz? Or might there even be
> devices that have different PHY paths and thus possibly different
> capabilities for 5 and 6 GHz, essentially requiring a new place (a new
> band enumerator) to store those capabilities, so we don't have to try to
> figure out the difference in code later?
>
> I'm almost tempted to say that given all these possibilities we should
> in fact add a new value to the band enum, worst case we just duplicate
> some data, but if there do end up being differences we can handle them
> much more gracefully than if we put everything into 5 GHz.
>
> Jouni, what do you think?
Regards,
Arend
^ permalink raw reply
* Re: [RFC PATCH v3 0/2] cfg80211: fix duplicated scan entries after channel switch
From: Sergey Matyukevich @ 2019-07-12 10:52 UTC (permalink / raw)
To: Johannes Berg
Cc: linux-wireless@vger.kernel.org, Igor Mitsyanko, Mikhail Karpenko
In-Reply-To: <43055be7b2d7ff0f8dbadd19443fc73f30f93bb6.camel@sipsolutions.net>
> > > [External Email]: This email arrived from an external source - Please exercise caution when opening any attachments or clicking on links.
>
> Heh, you have a not so fun email system that rewrites mails ...
:(
> > > > Suggested approach to handle non-transmitting BSS entries is simplified in the
> > > > following sense. If new entries have been already created after channel switch,
> > > > only transmitting bss will be updated using IEs of new entry for the same
> > > > transmitting bss. Non-transmitting bss entries will be updated as soon as
> > > > new mgmt frames are received. Updating non-transmitting bss entries seems
> > > > too expensive: nested nontrans_list traversing is needed since we can not
> > > > rely on the same order of old and new non-transmitting entries.
> > >
> > > That sounds like a reasonable trade-off. I do wonder though what happens
> > > if we're connected to a non-transmitting BSS?
> >
> > Well, here I rely upon the assumption that CSA IEs of non-transmitting BSS
> > are handled correctly by mac80211 or any FullMAC firmware. And if we are
> > connected to non-transmitting BSS rather than transmitting one, the
> > following code in the beginning of new cfg80211_update_assoc_bss_entry
> > function is supposed to care about this use-case:
>
> Right, it will be updated on RX. But then if we chanswitch, we would
> probably (mac80211 using a pointer to the non-transmitting BSS) update
> only one of the nontransmitting BSSes?
>
> Just saying that maybe we need to be careful there - or your wording
> might be incorrect. We might end up updating a *nontransmitting* BSS,
> and then its transmitting/other non-tx ones only later?
Hmmm... I am not sure we are on the same page here. Could you please
clarify your concerns here ?
The normal (non multi-BSSID) BSS usecase seem to be clear: keep old and
remove new (if any), since it is not easy to update ifmgd->associated.
Now let me take another look at the usecase when STA is connected to
a transmitting or non-transmitting BSS of a multi-BSS AP. At the moment
suggested code does the following. If STA is connected to the non-transmitting
BSS, then we switch to its transmitting BSS, instead of working with
current_bss directly.
So we look for the new entry (with new channel) of the transmitting BSS.
If it exists, then we remove it and _all_ of its non-transmitting BSSs.
Finally, we update channel and location in rb-tree of the existing (old)
transmitting BSS as well as _all_ of its non-transmitting entries.
Regards,
Sergey
^ permalink raw reply
* Re: [PATCH v2 05/16] wilc1000: add wilc_wlan_cfg.c
From: Ajay.Kathat @ 2019-07-12 10:58 UTC (permalink / raw)
To: johannes, linux-wireless
Cc: gregkh, kvalo, Adham.Abozaeid, Venkateswara.Kaja, Nicolas.Ferre,
Claudiu.Beznea
In-Reply-To: <86bc79ccd379497d56bade79ec8f717603110ef7.camel@sipsolutions.net>
Hi Johannes,
Thanks a lot for again reviewing our driver.
On 7/12/2019 1:01 PM, Johannes Berg wrote:
> On Fri, 2019-07-12 at 01:58 +0000, Ajay.Kathat@microchip.com wrote:
>>
>> + buf[0] = (u8)id;
>> + buf[1] = (u8)(id >> 8);
>> + buf[2] = 2;
>> + buf[3] = 0;
>> + buf[4] = (u8)val16;
>> + buf[5] = (u8)(val16 >> 8);
>
> There are helpers for that, put_le16_unaligned() or so?
>
Thanks for pointing out and I will modify the same.
>> + if (w->id == wid) {
>> + w->val = get_unaligned_le32(&info[4]);
>
> You use the opposite one here :-)
>
As I see, the data received from the firmware in *le* format, which is
parsed in wilc_wlan_parse_response_frame(). So to extract value in
correct byteorder we should use get_unaligned_le32(). right?
>> + /*
>> + * The valid types of response messages are
>> + * 'R' (Response),
>> + * 'I' (Information), and
>> + * 'N' (Network Information)
>> + */
>> +
>> + switch (msg_type) {
> [...]
>
>> + case 'S':
>
> Hmm. :-)
>
Yes, the comments has not capture about 'S' message type. 'S' type is
used to notify scan completed from firmware to host.
>> + wl->cfg.str_vals = str_vals;
>> + /* store the string cfg parameters */
>> + wl->cfg.s[i].id = WID_FIRMWARE_VERSION;
>> + wl->cfg.s[i].str = str_vals->firmware_version;
>> + i++;
>> + wl->cfg.s[i].id = WID_MAC_ADDR;
>> + wl->cfg.s[i].str = str_vals->mac_address;
>> + i++;
>> + wl->cfg.s[i].id = WID_ASSOC_RES_INFO;
>> + wl->cfg.s[i].str = str_vals->assoc_rsp;
>> + i++;
>> + wl->cfg.s[i].id = WID_NIL;
>> + wl->cfg.s[i].str = NULL;
>
> I really don't understand this style. Why not give it a proper struct
> and just say
> > wl->cfg.assoc_rsp = str_vals->assoc_rsp;
Actually, WID’s are used to send the configuration data from host to
firmware. Its generic approach to manage the different size of wid.
The different WID’s are categorized based on their data size.
Instead of managing different variables for each WID's its kept under
the single array. These entries are managed like 'id' and 'value'.
struct wilc_cfg {
struct wilc_cfg_byte *b;
struct wilc_cfg_hword *hw;
struct wilc_cfg_word *w;
struct wilc_cfg_str *s;
struct wilc_cfg_str_vals *str_vals;
};
The 1-byte data are managed in ‘b’ array, 2-byte in ‘hw’ array and so
on. So whenever the WID is set/get by host, the corresponding value also
get updated in this array which can be access later. The response from
firmware also contains the WID id which is used to udpate the
corresponding value. This categorization helps in search and update the
exact WID value *wilc_cfg* struct.
Regard
Ajay
> > etc?
>
> johannes
>
^ permalink raw reply
* [PATCH 1/3] mt76: mt76x02: use params->ssn value directly
From: Stanislaw Gruszka @ 2019-07-12 12:07 UTC (permalink / raw)
To: linux-wireless; +Cc: Felix Fietkau, Lorenzo Bianconi, Ryder Lee, Roy Luo
There is no point to use pointer to params->ssn.
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
drivers/net/wireless/mediatek/mt76/mt76x02_util.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
index ad5323447ed4..fa45ed280ab1 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
@@ -348,7 +348,7 @@ int mt76x02_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
struct mt76x02_sta *msta = (struct mt76x02_sta *) sta->drv_priv;
struct ieee80211_txq *txq = sta->txq[params->tid];
u16 tid = params->tid;
- u16 *ssn = ¶ms->ssn;
+ u16 ssn = params->ssn;
struct mt76_txq *mtxq;
if (!txq)
@@ -359,7 +359,7 @@ int mt76x02_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
switch (action) {
case IEEE80211_AMPDU_RX_START:
mt76_rx_aggr_start(&dev->mt76, &msta->wcid, tid,
- *ssn, params->buf_size);
+ ssn, params->buf_size);
mt76_set(dev, MT_WCID_ADDR(msta->wcid.idx) + 4, BIT(16 + tid));
break;
case IEEE80211_AMPDU_RX_STOP:
@@ -378,7 +378,7 @@ int mt76x02_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
ieee80211_send_bar(vif, sta->addr, tid, mtxq->agg_ssn);
break;
case IEEE80211_AMPDU_TX_START:
- mtxq->agg_ssn = IEEE80211_SN_TO_SEQ(*ssn);
+ mtxq->agg_ssn = IEEE80211_SN_TO_SEQ(ssn);
ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid);
break;
case IEEE80211_AMPDU_TX_STOP_CONT:
--
2.20.1
^ permalink raw reply related
* [PATCH 2/3] mt76: mt7603: use params->ssn value directly
From: Stanislaw Gruszka @ 2019-07-12 12:07 UTC (permalink / raw)
To: linux-wireless; +Cc: Felix Fietkau, Lorenzo Bianconi, Ryder Lee, Roy Luo
In-Reply-To: <20190712120800.21506-1-sgruszka@redhat.com>
There is no point to use pointer to params->ssn.
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
drivers/net/wireless/mediatek/mt76/mt7603/main.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7603/main.c b/drivers/net/wireless/mediatek/mt76/mt7603/main.c
index e5d4cb6381a8..d70f42dac923 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7603/main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7603/main.c
@@ -569,7 +569,7 @@ mt7603_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
struct ieee80211_txq *txq = sta->txq[params->tid];
struct mt7603_sta *msta = (struct mt7603_sta *)sta->drv_priv;
u16 tid = params->tid;
- u16 *ssn = ¶ms->ssn;
+ u16 ssn = params->ssn;
u8 ba_size = params->buf_size;
struct mt76_txq *mtxq;
@@ -580,7 +580,7 @@ mt7603_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
switch (action) {
case IEEE80211_AMPDU_RX_START:
- mt76_rx_aggr_start(&dev->mt76, &msta->wcid, tid, *ssn,
+ mt76_rx_aggr_start(&dev->mt76, &msta->wcid, tid, ssn,
params->buf_size);
mt7603_mac_rx_ba_reset(dev, sta->addr, tid);
break;
@@ -599,7 +599,7 @@ mt7603_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
mt7603_mac_tx_ba_reset(dev, msta->wcid.idx, tid, -1);
break;
case IEEE80211_AMPDU_TX_START:
- mtxq->agg_ssn = IEEE80211_SN_TO_SEQ(*ssn);
+ mtxq->agg_ssn = IEEE80211_SN_TO_SEQ(ssn);
ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid);
break;
case IEEE80211_AMPDU_TX_STOP_CONT:
--
2.20.1
^ permalink raw reply related
* [PATCH 3/3] mt76: mt7615: use params->ssn value directly
From: Stanislaw Gruszka @ 2019-07-12 12:08 UTC (permalink / raw)
To: linux-wireless; +Cc: Felix Fietkau, Lorenzo Bianconi, Ryder Lee, Roy Luo
In-Reply-To: <20190712120800.21506-1-sgruszka@redhat.com>
There is no point to use pointer to params->ssn.
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
drivers/net/wireless/mediatek/mt76/mt7615/main.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/main.c b/drivers/net/wireless/mediatek/mt76/mt7615/main.c
index 3f5f355d1f9b..2c702b31d55f 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/main.c
@@ -443,7 +443,7 @@ mt7615_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
struct ieee80211_txq *txq = sta->txq[params->tid];
struct mt7615_sta *msta = (struct mt7615_sta *)sta->drv_priv;
u16 tid = params->tid;
- u16 *ssn = ¶ms->ssn;
+ u16 ssn = params->ssn;
struct mt76_txq *mtxq;
if (!txq)
@@ -453,7 +453,7 @@ mt7615_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
switch (action) {
case IEEE80211_AMPDU_RX_START:
- mt76_rx_aggr_start(&dev->mt76, &msta->wcid, tid, *ssn,
+ mt76_rx_aggr_start(&dev->mt76, &msta->wcid, tid, ssn,
params->buf_size);
mt7615_mcu_set_rx_ba(dev, params, 1);
break;
@@ -473,7 +473,7 @@ mt7615_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
mt7615_mcu_set_tx_ba(dev, params, 0);
break;
case IEEE80211_AMPDU_TX_START:
- mtxq->agg_ssn = IEEE80211_SN_TO_SEQ(*ssn);
+ mtxq->agg_ssn = IEEE80211_SN_TO_SEQ(ssn);
ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid);
break;
case IEEE80211_AMPDU_TX_STOP_CONT:
--
2.20.1
^ permalink raw reply related
* [PATCH] mt7601u: use params->ssn value directly
From: Stanislaw Gruszka @ 2019-07-12 12:09 UTC (permalink / raw)
To: linux-wireless; +Cc: Jakub Kicinski
There is no point to use pointer to params->ssn.
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
drivers/net/wireless/mediatek/mt7601u/main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt7601u/main.c b/drivers/net/wireless/mediatek/mt7601u/main.c
index 89a7b1234ffb..72e608cc53af 100644
--- a/drivers/net/wireless/mediatek/mt7601u/main.c
+++ b/drivers/net/wireless/mediatek/mt7601u/main.c
@@ -351,7 +351,7 @@ mt76_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
struct ieee80211_sta *sta = params->sta;
enum ieee80211_ampdu_mlme_action action = params->action;
u16 tid = params->tid;
- u16 *ssn = ¶ms->ssn;
+ u16 ssn = params->ssn;
struct mt76_sta *msta = (struct mt76_sta *) sta->drv_priv;
WARN_ON(msta->wcid.idx > GROUP_WCID(0));
@@ -371,7 +371,7 @@ mt76_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
case IEEE80211_AMPDU_TX_STOP_FLUSH_CONT:
break;
case IEEE80211_AMPDU_TX_START:
- msta->agg_ssn[tid] = *ssn << 4;
+ msta->agg_ssn[tid] = ssn << 4;
ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid);
break;
case IEEE80211_AMPDU_TX_STOP_CONT:
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 00/12] treewide: Fix GENMASK misuses
From: Andrzej Hajda @ 2019-07-12 12:54 UTC (permalink / raw)
To: Joe Perches, Andrew Morton, Patrick Venture, Nancy Yuen,
Benjamin Fair, Andrew Jeffery, openbmc, linux-kernel,
linux-aspeed, linux-arm-kernel, linux-amlogic, netdev,
linux-mediatek, linux-stm32, linux-wireless, linux-media
Cc: linux-iio, devel, alsa-devel, linux-mmc, dri-devel
In-Reply-To: <cover.1562734889.git.joe@perches.com>
Hi Joe,
On 10.07.2019 07:04, Joe Perches wrote:
> These GENMASK uses are inverted argument order and the
> actual masks produced are incorrect. Fix them.
>
> Add checkpatch tests to help avoid more misuses too.
>
> Joe Perches (12):
> checkpatch: Add GENMASK tests
> clocksource/drivers/npcm: Fix misuse of GENMASK macro
> drm: aspeed_gfx: Fix misuse of GENMASK macro
> iio: adc: max9611: Fix misuse of GENMASK macro
> irqchip/gic-v3-its: Fix misuse of GENMASK macro
> mmc: meson-mx-sdio: Fix misuse of GENMASK macro
> net: ethernet: mediatek: Fix misuses of GENMASK macro
> net: stmmac: Fix misuses of GENMASK macro
> rtw88: Fix misuse of GENMASK macro
> phy: amlogic: G12A: Fix misuse of GENMASK macro
> staging: media: cedrus: Fix misuse of GENMASK macro
> ASoC: wcd9335: Fix misuse of GENMASK macro
>
> drivers/clocksource/timer-npcm7xx.c | 2 +-
> drivers/gpu/drm/aspeed/aspeed_gfx.h | 2 +-
> drivers/iio/adc/max9611.c | 2 +-
> drivers/irqchip/irq-gic-v3-its.c | 2 +-
> drivers/mmc/host/meson-mx-sdio.c | 2 +-
> drivers/net/ethernet/mediatek/mtk_eth_soc.h | 2 +-
> drivers/net/ethernet/mediatek/mtk_sgmii.c | 2 +-
> drivers/net/ethernet/stmicro/stmmac/descs.h | 2 +-
> drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 4 ++--
> drivers/net/wireless/realtek/rtw88/rtw8822b.c | 2 +-
> drivers/phy/amlogic/phy-meson-g12a-usb2.c | 2 +-
> drivers/staging/media/sunxi/cedrus/cedrus_regs.h | 2 +-
> scripts/checkpatch.pl | 15 +++++++++++++++
> sound/soc/codecs/wcd-clsh-v2.c | 2 +-
> 14 files changed, 29 insertions(+), 14 deletions(-)
>
After adding following compile time check:
------
diff --git a/Makefile b/Makefile
index 5102b2bbd224..ac4ea5f443a9 100644
--- a/Makefile
+++ b/Makefile
@@ -457,7 +457,7 @@ KBUILD_AFLAGS := -D__ASSEMBLY__ -fno-PIE
KBUILD_CFLAGS := -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs \
-fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE \
-Werror=implicit-function-declaration
-Werror=implicit-int \
- -Wno-format-security \
+ -Wno-format-security -Werror=div-by-zero \
-std=gnu89
KBUILD_CPPFLAGS := -D__KERNEL__
KBUILD_AFLAGS_KERNEL :=
diff --git a/include/linux/bits.h b/include/linux/bits.h
index 669d69441a62..61d74b103055 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -19,11 +19,11 @@
* GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
*/
#define GENMASK(h, l) \
- (((~UL(0)) - (UL(1) << (l)) + 1) & \
+ (((~UL(0)) - (UL(1) << (l)) + 1 + 0/((h) >= (l))) & \
(~UL(0) >> (BITS_PER_LONG - 1 - (h))))
#define GENMASK_ULL(h, l) \
- (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
+ (((~ULL(0)) - (ULL(1) << (l)) + 1 + 0/((h) >= (l))) & \
(~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
#endif /* __LINUX_BITS_H */
-------
I was able to detect one more GENMASK misue (AARCH64, allyesconfig):
CC drivers/phy/rockchip/phy-rockchip-inno-hdmi.o
In file included from ../include/linux/bitops.h:5:0,
from ../include/linux/kernel.h:12,
from ../include/linux/clk.h:13,
from ../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:9:
../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c: In function
‘inno_hdmi_phy_rk3328_power_on’:
../include/linux/bits.h:22:37: error: division by zero [-Werror=div-by-zero]
(((~UL(0)) - (UL(1) << (l)) + 1 + 0/((h) >= (l))) & \
^
../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:24:42: note: in
expansion of macro ‘GENMASK’
#define UPDATE(x, h, l) (((x) << (l)) & GENMASK((h), (l)))
^~~~~~~
../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:201:50: note: in
expansion of macro ‘UPDATE’
#define RK3328_TERM_RESISTOR_CALIB_SPEED_7_0(x) UPDATE(x, 7, 9)
^~~~~~
../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:1046:26: note: in
expansion of macro ‘RK3328_TERM_RESISTOR_CALIB_SPEED_7_0’
inno_write(inno, 0xc6, RK3328_TERM_RESISTOR_CALIB_SPEED_7_0(v));
Of course I do not advise to add the check as is to Kernel - it is
undefined behavior area AFAIK.
Regards
Andrzej
^ permalink raw reply related
* Re: [PATCH v2 02/16] wilc1000: add wilc_hif.c
From: Ajay.Kathat @ 2019-07-12 14:52 UTC (permalink / raw)
To: johannes, linux-wireless
Cc: gregkh, kvalo, Adham.Abozaeid, Venkateswara.Kaja, Nicolas.Ferre,
Claudiu.Beznea
In-Reply-To: <b6bb8a8b61ebbca40611dee07e4980a792bf2386.camel@sipsolutions.net>
On 7/12/2019 1:12 PM, Johannes Berg wrote:
> External E-Mail
>
>
>> +struct wilc_set_multicast {
>> + u32 enabled;
>> + u32 cnt;
>> + u8 *mc_list;
>> +};
>> +
>> +struct wilc_del_all_sta {
>> + u8 assoc_sta;
>> + u8 mac[WILC_MAX_NUM_STA][ETH_ALEN];
>> +};
>> +
>> +struct wilc_op_mode {
>> + __le32 mode;
>> +};
>> +
>> +struct wilc_reg_frame {
>> + bool reg;
>> + u8 reg_id;
>> + __le16 frame_type;
>> +} __packed;
>
> 'bool' is a pretty bad idea, there's no storage guarantee for it. Use u8
> instead, especially in a firmware struct.
>
> But overall, if I remember correctly, this is a massive improvement,
> last time I looked I think you basically had something like
>
> char msg[10];
> int i = 0;
> msg[i++] = reg;
> msg[i++] = reg_id;
> msg[i++] = frame_type >> 8;
> msg[i++] = (u8)frame_type;
>
> so obviously this is *much* better.
>
> I still think you'd benefit from putting the firmware API structs into a
> separate include file so you can differentiate them, but YMMV.
>
>> +int wilc_scan(struct wilc_vif *vif, u8 scan_source, u8 scan_type,
>> + u8 *ch_freq_list, u8 ch_list_len,
>> + void (*scan_result_fn)(enum scan_event,
>> + struct wilc_rcvd_net_info *, void *),
>> + void *user_arg, struct cfg80211_scan_request *request)
>> +{
>> + int result = 0;
>> + struct wid wid_list[5];
>
>> + wid_list[index].id = WID_INFO_ELEMENT_PROBE;
>> + wid_list[index].type = WID_BIN_DATA;
>> + wid_list[index].val = (s8 *)request->ie;
>> + wid_list[index].size = request->ie_len;
>> + index++;
>> +
>> + wid_list[index].id = WID_SCAN_TYPE;
>> + wid_list[index].type = WID_CHAR;
>> + wid_list[index].size = sizeof(char);
>> + wid_list[index].val = (s8 *)&scan_type;
>> + index++;
>
>
> I still find this whole wid_list stuff to be a bit confusing, especially
> since it looks like a *firmware* thing but then you have the *host
> pointer* inside the value ...
>
> There must be a translation layer somewhere, but I can't help but wonder
> if that's really worth the complexity, vs. just building the right thing
> directly here (with some helpers perhaps).
>
The translation to firmware buffer happens in wilc_wlan_cfg_set() and
wilc_wlan_cfg_commit() adds a single *wilc_cfg_cmd_hdr* header before
sending to firmware.
There are two ways to send the wid's from host to firmware.
1/ Single Wid -> In this case, single wid is sent by adding
*wilc_cfg_cmd_hdr* in single command buffer to firmware.
i.e <wilc_cfg_cmd_hdr><wid1>
2/ Mutliple Wid's -> In this case, multiple wid's are clubbled together
and sent in single command buffer.
e.g. <wilc_cfg_cmd_hdr><wid1><wid2><wid3>
As the firmware is design to receive configuration under different
WID's, so it is required from the host side to club these parameters
whenever data is related.
Currently, wilc_send_config_pkt() is provided as helper API to construct
buffer based on passed *wids* and *counts*. This will avoid adding
similar logic in multiple places.
Regards,
Ajay
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox