* [PATCH 2/4] ath9k: dynack: properly set last timeout timestamp in ath_dynack_reset
From: Lorenzo Bianconi @ 2019-08-20 16:20 UTC (permalink / raw)
To: kvalo; +Cc: lorenzo.bianconi, linux-wireless, koen.vandeputte
In-Reply-To: <cover.1566317488.git.lorenzo@kernel.org>
Add compute timeout to last computation timestamp in
ath_dynack_reset in order to not run ath_dynack_compute_ackto
immediately
Tested-by: Koen Vandeputte <koen.vandeputte@ncentric.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
drivers/net/wireless/ath/ath9k/dynack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath9k/dynack.c b/drivers/net/wireless/ath/ath9k/dynack.c
index c244dd15dce4..5d5809d160af 100644
--- a/drivers/net/wireless/ath/ath9k/dynack.c
+++ b/drivers/net/wireless/ath/ath9k/dynack.c
@@ -338,7 +338,7 @@ void ath_dynack_reset(struct ath_hw *ah)
u32 ackto = 9 + 16 + 64;
struct ath_dynack *da = &ah->dynack;
- da->lto = jiffies;
+ da->lto = jiffies + COMPUTE_TO;
da->ackto = ackto;
da->st_rbf.t_rb = 0;
--
2.21.0
^ permalink raw reply related
* [PATCH 3/4] ath9k: dynack: set max timeout according to channel width
From: Lorenzo Bianconi @ 2019-08-20 16:20 UTC (permalink / raw)
To: kvalo; +Cc: lorenzo.bianconi, linux-wireless, koen.vandeputte
In-Reply-To: <cover.1566317488.git.lorenzo@kernel.org>
Compute maximum configurable ackimeout/ctstimeout according to channel
width (clockrate)
Tested-by: Koen Vandeputte <koen.vandeputte@ncentric.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
drivers/net/wireless/ath/ath9k/dynack.c | 38 +++++++++++++++++++------
1 file changed, 30 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/dynack.c b/drivers/net/wireless/ath/ath9k/dynack.c
index 5d5809d160af..f5acaa577d62 100644
--- a/drivers/net/wireless/ath/ath9k/dynack.c
+++ b/drivers/net/wireless/ath/ath9k/dynack.c
@@ -20,11 +20,30 @@
#define COMPUTE_TO (5 * HZ)
#define LATEACK_DELAY (10 * HZ)
-#define LATEACK_TO 256
-#define MAX_DELAY 300
#define EWMA_LEVEL 96
#define EWMA_DIV 128
+/**
+ * ath_dynack_get_max_to - set max timeout according to channel width
+ * @ah: ath hw
+ *
+ */
+static u32 ath_dynack_get_max_to(struct ath_hw *ah)
+{
+ const struct ath9k_channel *chan = ah->curchan;
+
+ if (!chan)
+ return 300;
+
+ if (IS_CHAN_HT40(chan))
+ return 300;
+ if (IS_CHAN_HALF_RATE(chan))
+ return 750;
+ if (IS_CHAN_QUARTER_RATE(chan))
+ return 1500;
+ return 600;
+}
+
/**
* ath_dynack_ewma - EWMA (Exponentially Weighted Moving Average) calculation
*
@@ -126,15 +145,16 @@ static void ath_dynack_compute_ackto(struct ath_hw *ah)
*/
static void ath_dynack_compute_to(struct ath_hw *ah)
{
- u32 ackto, ack_ts;
- u8 *dst, *src;
+ struct ath_dynack *da = &ah->dynack;
+ u32 ackto, ack_ts, max_to;
struct ieee80211_sta *sta;
- struct ath_node *an;
struct ts_info *st_ts;
- struct ath_dynack *da = &ah->dynack;
+ struct ath_node *an;
+ u8 *dst, *src;
rcu_read_lock();
+ max_to = ath_dynack_get_max_to(ah);
while (da->st_rbf.h_rb != da->st_rbf.t_rb &&
da->ack_rbf.h_rb != da->ack_rbf.t_rb) {
ack_ts = da->ack_rbf.tstamp[da->ack_rbf.h_rb];
@@ -150,7 +170,7 @@ static void ath_dynack_compute_to(struct ath_hw *ah)
if (ack_ts > st_ts->tstamp + st_ts->dur) {
ackto = ack_ts - st_ts->tstamp - st_ts->dur;
- if (ackto < MAX_DELAY) {
+ if (ackto < max_to) {
sta = ieee80211_find_sta_by_ifaddr(ah->hw, dst,
src);
if (sta) {
@@ -207,8 +227,10 @@ void ath_dynack_sample_tx_ts(struct ath_hw *ah, struct sk_buff *skb,
if (ieee80211_is_assoc_req(hdr->frame_control) ||
ieee80211_is_assoc_resp(hdr->frame_control) ||
ieee80211_is_auth(hdr->frame_control)) {
+ u32 max_to = ath_dynack_get_max_to(ah);
+
ath_dbg(common, DYNACK, "late ack\n");
- ath_dynack_set_timeout(ah, LATEACK_TO);
+ ath_dynack_set_timeout(ah, max_to);
if (sta) {
struct ath_node *an;
--
2.21.0
^ permalink raw reply related
* [PATCH 4/4] ath9k: dynack: set ackto to max timeout in ath_dynack_reset
From: Lorenzo Bianconi @ 2019-08-20 16:20 UTC (permalink / raw)
To: kvalo; +Cc: lorenzo.bianconi, linux-wireless, koen.vandeputte
In-Reply-To: <cover.1566317488.git.lorenzo@kernel.org>
Initialize acktimeout to the maximum configurable value in
ath_dynack_reset in order to not disconnect long distance static links
enabling dynack and even to take care of possible errors configuring
a static timeout. Moreover initialize station timeout value to the current
acktimeout value
Tested-by: Koen Vandeputte <koen.vandeputte@ncentric.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
drivers/net/wireless/ath/ath9k/dynack.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/dynack.c b/drivers/net/wireless/ath/ath9k/dynack.c
index f5acaa577d62..fbeb4a739d32 100644
--- a/drivers/net/wireless/ath/ath9k/dynack.c
+++ b/drivers/net/wireless/ath/ath9k/dynack.c
@@ -321,11 +321,9 @@ EXPORT_SYMBOL(ath_dynack_sample_ack_ts);
*/
void ath_dynack_node_init(struct ath_hw *ah, struct ath_node *an)
{
- /* ackto = slottime + sifs + air delay */
- u32 ackto = 9 + 16 + 64;
struct ath_dynack *da = &ah->dynack;
- an->ackto = ackto;
+ an->ackto = da->ackto;
spin_lock_bh(&da->qlock);
list_add_tail(&an->list, &da->nodes);
@@ -356,20 +354,26 @@ EXPORT_SYMBOL(ath_dynack_node_deinit);
*/
void ath_dynack_reset(struct ath_hw *ah)
{
- /* ackto = slottime + sifs + air delay */
- u32 ackto = 9 + 16 + 64;
struct ath_dynack *da = &ah->dynack;
+ struct ath_node *an;
+
+ spin_lock_bh(&da->qlock);
da->lto = jiffies + COMPUTE_TO;
- da->ackto = ackto;
da->st_rbf.t_rb = 0;
da->st_rbf.h_rb = 0;
da->ack_rbf.t_rb = 0;
da->ack_rbf.h_rb = 0;
+ da->ackto = ath_dynack_get_max_to(ah);
+ list_for_each_entry(an, &da->nodes, list)
+ an->ackto = da->ackto;
+
/* init acktimeout */
- ath_dynack_set_timeout(ah, ackto);
+ ath_dynack_set_timeout(ah, da->ackto);
+
+ spin_unlock_bh(&da->qlock);
}
EXPORT_SYMBOL(ath_dynack_reset);
@@ -386,6 +390,8 @@ void ath_dynack_init(struct ath_hw *ah)
spin_lock_init(&da->qlock);
INIT_LIST_HEAD(&da->nodes);
+ /* ackto = slottime + sifs + air delay */
+ da->ackto = 9 + 16 + 64;
ah->hw->wiphy->features |= NL80211_FEATURE_ACKTO_ESTIMATION;
}
--
2.21.0
^ permalink raw reply related
* Re: [PATCH 31/49] ath11k: add mac.c
From: Toke Høiland-Jørgensen @ 2019-08-20 16:51 UTC (permalink / raw)
To: Kalle Valo, linux-wireless; +Cc: ath11k, devicetree
In-Reply-To: <1566316095-27507-32-git-send-email-kvalo@codeaurora.org>
[... snip ... ]
> +static const struct ieee80211_ops ath11k_ops = {
> + .tx = ath11k_mac_op_tx,
No wake_tx_queue? :(
-Toke
^ permalink raw reply
* Re: [PATCH 02/49] ath11k: add Kconfig
From: John Crispin @ 2019-08-20 16:52 UTC (permalink / raw)
To: Kalle Valo, linux-wireless; +Cc: ath11k, devicetree
In-Reply-To: <1566316095-27507-3-git-send-email-kvalo@codeaurora.org>
On 20/08/2019 17:47, Kalle Valo wrote:
> ---
> drivers/net/wireless/ath/ath11k/Kconfig | 34 +++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath11k/Kconfig b/drivers/net/wireless/ath/ath11k/Kconfig
> new file mode 100644
> index 000000000000..26bf98fa648e
> --- /dev/null
> +++ b/drivers/net/wireless/ath/ath11k/Kconfig
> @@ -0,0 +1,34 @@
> +config ATH11K
> + tristate "Qualcomm Technologies 802.11ax chipset support"
> + depends on MAC80211 && HAS_DMA
^^ space vs tab here
John
> + depends on REMOTEPROC
> + depends on ARCH_QCOM || COMPILE_TEST
> + select ATH_COMMON
> + select QCOM_QMI_HELPERS
> + help
> + This module adds support for Qualcomm Technologies 802.11ax family of
> + chipsets.
> +
> + If you choose to build a module, it'll be called ath11k.
> +
^ permalink raw reply
* Re: [RFC 0/1] Allow MAC change on up interface
From: Dan Williams @ 2019-08-20 17:53 UTC (permalink / raw)
To: Denis Kenzior, Johannes Berg, James Prestwood, linux-wireless
In-Reply-To: <7b9a7df3-6880-98f6-5c09-257165025559@gmail.com>
On Tue, 2019-08-20 at 10:40 -0500, Denis Kenzior wrote:
> Hi Johannes,
>
> > Stop.
> >
> > Your tone, and in particular the constant snide comments and
> > attacks on
> > me are, quite frankly, getting extremely tiring.
> >
>
> Look, I'm sorry I hit a nerve, but from where I am sitting, it had to
> be
> said...
But did it really? And in that way? There were certainly better ways
to go about that response.
I don't recall seeing a NAK anywhere his email chain (which you'd get
with some other kernel maintainers) but instead (a) an explanation of
why the proposed solution had some problems, (b) some alternative
possibilities and (c) requests for more information so the discussion
could continue.
It does the requested changes no good to take that kind of tone. Let's
move on from here and keep things constructive to solve the problem at
hand, which is:
"Changing the MAC address of a WiFi interface takes longer than I'd
like and clears some state that I'd like it to keep."
That is a technical problem we can solve, so let's keep it at that
level.
Dan
> Regardless. Peace, I'm not trying to start drama here. We just want
> to
> improve things and it feels like you're shutting down every idea
> without
> truly understanding the issues we're facing.
>
> > It almost seems like you're just trying to bully me into taking
> > your
> > patches by constantly trying to make me feel that I cannot know
> > better
> > anyway. This is not how you should be treating anyone.
> >
>
> Before lecturing me on my tone, can you go back and re-read your
> responses to many of the contributors here? I mean really read
> them?
> Your tone is quite dismissive, whether intentional or not. When one
> of
> my guys comes to me and says:
> "Johannes' response was completely useless, it feels like he
> didn't
> even read my cover letter"
>
> I will come out and call you on it. So if you don't mean to come
> off
> that way, great. We'll just chalk it up to a mis-understanding.
>
> > Look, I did say I don't see a point in this, but you're taking that
> > out
> > of context. I also stated that I didn't understand the whole thing
> > about
> > "race conditions" and all, because nobody actually explained the
> > reasoning behind the changes here.
>
> Fine. I get that. But how about asking what the use case is? Or
> say
> you don't quite understand why something is needed? We'll happily
> explain. When the first reaction to an RFC is: "I don't see the
> point"
> or "You're doing it wrong" from a maintainer who's job (by
> definition)
> is to encourage new contributors and improve the subsystem he
> maintains...? Well that's kind of a downer, don't you
> agree? You're
> the maintainer and you should be held to a higher standard...
>
> I maintain 3 projects, I know the job isn't great, but you still
> should
> be (more) civil to people...
>
> > James, unlike you, managed to reply on point and explain why it was
> > needed. If all you can do is accuse me of not using the software
> > and
> > therefore not knowing how it should be used, even implying that I'm
> > not
> > smart enough to understand the use cases, then I don't know why you
> > bother replying at all.
>
> Good on James. I council all my guys to keep cool when dealing with
> upstream. But that doesn't mean you should be dismissing things out
> of
> hand on the very first interaction you have with a new contributor.
>
> > I can understand your frustration to some extent, and I want to
> > give you
> > the benefit of doubt and want to believe this behaviour was borne
> > out of
> > it, since I've been reviewing your changes relatively critically.
> >
>
> Good. I want you to do that. The changes are in very tricky areas
> and
> you know the code best.
>
> > However, I also want to believe that I've been (trying to) keep the
> > discussion on a technical level, telling you why I believe some
> > things
> > shouldn't be done the way you did them, rather than telling you
> > that
> > you're not smart enough to be working on this. If you feel
> > otherwise,
> > please do let me know and I'll go back to understand, in order to
> > improve my behaviour in the future.
>
> If you interpreted my rants as an assault to your intelligence, then
> I'm
> sorry. They really were not meant this way. But sometimes we
> really
> had to wonder if you were using the same API we were? So the
> question I
> asked above was purely logical consequence of what I was seeing.
>
> You yourself admitted that you have never implemented an event
> driven
> stack. So how about listening to the guys that are?
>
> We are using your APIs in different ways. So instead of questioning
> why
> or attacking those ways, how about asking whether improvements can be
> made?
>
> We are facing serious pain points with the API. So instead of
> dismissing things out of hand, can we work together to improve
> things?
>
> We are trying to make things fast. The API is frequently not setup
> for
> that. The MAC randomization is just one example. Bringing down the
> interface (and shutting down the firmware, toggling power state,
> cleaning up resources/state) prior to every connection is just not
> acceptable. Look at the link I sent. The Android guys state 3
> seconds
> is the typical 'hit'. This is literally wasting everyone's time.
>
> > Please help keep the discussion technical, without demeaning
> > undertones.
>
> Point taken. And I apologize again. But please consider what I said
> above.
>
> Regards,
> -Denis
^ permalink raw reply
* Re: [RFC 0/1] Allow MAC change on up interface
From: Denis Kenzior @ 2019-08-20 18:21 UTC (permalink / raw)
To: Dan Williams, Johannes Berg, James Prestwood, linux-wireless
In-Reply-To: <32547793981b3997f7e0f74137745f38615e5c54.camel@redhat.com>
Hi Dan,
On 8/20/19 12:53 PM, Dan Williams wrote:
> On Tue, 2019-08-20 at 10:40 -0500, Denis Kenzior wrote:
>> Hi Johannes,
>>
>>> Stop.
>>>
>>> Your tone, and in particular the constant snide comments and
>>> attacks on
>>> me are, quite frankly, getting extremely tiring.
>>>
>>
>> Look, I'm sorry I hit a nerve, but from where I am sitting, it had to
>> be
>> said...
>
> But did it really? And in that way? There were certainly better ways
> to go about that response.
The issue is that this isn't the first such occurrence. There is a
pattern here and it needs to change. So +1 on handling this better.
>
> I don't recall seeing a NAK anywhere his email chain (which you'd get
> with some other kernel maintainers) but instead (a) an explanation of
> why the proposed solution had some problems, (b) some alternative
> possibilities and (c) requests for more information so the discussion
> could continue.
So the cover letter states:
"Set IFF_LIVE_ADDR_CHANGE on net_device. Note: I know setting this
where I did is likely not the right way to do it, but for this
proof-of-concept it works. With guidance I can move this around
to a proper place."
and I'll leave it up to you to read the first response from the maintainer.
>
> It does the requested changes no good to take that kind of tone. Let's
Neither is:
"don't do that then"
or
"I'm not really sure I see any point in this to start with"
or
"To me, the whole thing seems like more of a problem than a solution."
> move on from here and keep things constructive to solve the problem at
> hand, which is:
>
> "Changing the MAC address of a WiFi interface takes longer than I'd
> like and clears some state that I'd like it to keep."
>
> That is a technical problem we can solve, so let's keep it at that
> level.
>
I'm all for moving on and having the people that know this stuff well
giving actual guidance, as was requested originally.
Regards,
-Denis
^ permalink raw reply
* Re: [RFC 0/1] Allow MAC change on up interface
From: Toke Høiland-Jørgensen @ 2019-08-20 18:54 UTC (permalink / raw)
To: Denis Kenzior, Dan Williams, Johannes Berg, James Prestwood,
linux-wireless
In-Reply-To: <32a1bddd-2472-10a3-4d0d-5634814e635b@gmail.com>
Denis Kenzior <denkenz@gmail.com> writes:
> Hi Dan,
>
> On 8/20/19 12:53 PM, Dan Williams wrote:
>> On Tue, 2019-08-20 at 10:40 -0500, Denis Kenzior wrote:
>>> Hi Johannes,
>>>
>>>> Stop.
>>>>
>>>> Your tone, and in particular the constant snide comments and
>>>> attacks on
>>>> me are, quite frankly, getting extremely tiring.
>>>>
>>>
>>> Look, I'm sorry I hit a nerve, but from where I am sitting, it had to
>>> be
>>> said...
>>
>> But did it really? And in that way? There were certainly better ways
>> to go about that response.
>
> The issue is that this isn't the first such occurrence. There is a
> pattern here and it needs to change. So +1 on handling this better.
>
>>
>> I don't recall seeing a NAK anywhere his email chain (which you'd get
>> with some other kernel maintainers) but instead (a) an explanation of
>> why the proposed solution had some problems, (b) some alternative
>> possibilities and (c) requests for more information so the discussion
>> could continue.
>
> So the cover letter states:
> "Set IFF_LIVE_ADDR_CHANGE on net_device. Note: I know setting this
> where I did is likely not the right way to do it, but for this
> proof-of-concept it works. With guidance I can move this around
> to a proper place."
>
> and I'll leave it up to you to read the first response from the
> maintainer.
I went back and re-read the whole thread, and all I see is Johannes and
James having a technical discussion, and you barging in with accusations
of bad faith. So yeah, going to agree with Dan here; you were not
"hitting a nerve", you were just being rude.
-Toke
^ permalink raw reply
* Re: [RFC 0/1] Allow MAC change on up interface
From: Denis Kenzior @ 2019-08-20 19:22 UTC (permalink / raw)
To: Johannes Berg, James Prestwood
Cc: linux-wireless@vger.kernel.org, Dan Williams
In-Reply-To: <6835732fcc59ba8dbbcda4abc6e17dad499a7d8d.camel@sipsolutions.net>
Hi Johannes,
So keeping things purely technical now :)
> I thought so, but I had another thought later. It might be possible to
> set LIVE_ADDR_CHANGE, but then block it in mac80211 when the interface
> is already connected (or beaconing, or whatever, using the MAC address
> in some way - even while scanning, remain-on-channel is active, etc.)
>
Here's what we would like to see:
- The ability for userspace to add a 'Local Mac Address to use'
attribute on CMD_CONNECT and CMD_AUTHENTICATE.
- It doesn't seem useful to add this to CMD_ASSOCIATE at the moment, as
for new connections you'd always go either through CMD_AUTHENTICATE,
CMD_ASSOCIATE sequence or use CMD_CONNECT. This should take care of
some (most) of your objections about specifying different addresses for
authenticate/associate. Feel free to correct me here.
- For Fast Transitions, Re-associations, roaming, etc userspace would
simply omit the 'Local Mac Address to use' attribute and the currently
set address would be used.
- Optionally (and I'm thinking of tools like NM here), add the ability
to set the mac address via RTNL while the device is UP but has no
carrier, and things like scanning, offchannel, etc are not in progress.
Though really I don't see how NM could guarantee this without bringing
the device down first, so I'll let NM guys chime in to see if this is a
good idea.
- We definitely do not want to to mess with the device state otherwise.
E.g. no firmware downloading, powering the adapter down, etc. That just
takes too much time.
The goal here is to keep things as fast as possible. The randomization
feature is basically standard on every modern OS. So users expect it to
be used on every connection. Slowing down the connection time by up to
3 seconds just isn't acceptable.
So tell us what you would like to see. A new
IFF_NO_CARRIER_ADDRESS_CHANGE or whether it is possible to use
IFF_LIVE_ADDR_CHANGE with some additional restrictions.
> I still think you'd have to bake it into the mac80211<->driver API
> somehow, because we normally "add_interface()" with the MAC address, and
> nothing says that the driver cannot ignore the MAC address from that
> point on. The fact that iwlwifi just copies it into every new MAC_CTXT
> command and the firmware actually accepts the update seems rather
> accidental and therefore fragile to rely on.
>
Since you seem to have a clear(er?) idea here, can you elaborate or
possibly provide the driver interface changes you want to see?
Regards,
-Denis
^ permalink raw reply
* Re: [RFC 0/1] Allow MAC change on up interface
From: Johannes Berg @ 2019-08-20 19:32 UTC (permalink / raw)
To: Denis Kenzior, James Prestwood, linux-wireless
In-Reply-To: <7b9a7df3-6880-98f6-5c09-257165025559@gmail.com>
Hi Denis,
Rather than replying to all the separate items here, just two things:
1) I'll reiterate - please keep things civil. You're taking things out
of context a *lot* here, in what seems like an attempt to draw a
parallel between my and your utterances.
2) I'll take your point that I've been somewhat dismissive in tone, and
will try to change that.
I do want to reply to two things specifically though:
> Fine. I get that. But how about asking what the use case is? Or say
> you don't quite understand why something is needed?
Really, I should *not* have to ask that. You should consider it your
obligation to provide enough information for me to review patches
without having to go back and ask what it is you actually want to
achieve.
Compared to some other subsystems and maintainers I've dealt with, I
think I've actually been rather patient in trying to extract the purpose
of your changes, rather than *really* just dismissing them (which I've
felt like many times.)
> a maintainer who's job (by definition)
> is to encourage new contributors and improve the subsystem he
> maintains...?
This is what maybe you see as the maintainer's role, but I disagree, at
least to some extent. I see the role more of a supervisor, somebody
who's ensuring that all the changes coming in will work together. Yes, I
have my own incentive to improve things, but if you have a particular
incentive to improve a particular use case, the onus really is on you to
convince me of that, not on me to try to ferret the reasoning out of you
and make those improvements my own.
So please - come with some actual reasoning. This particular thread only
offered "would elminate a few potential race conditions", in the cover
letter, not even the patch itself, so it wasn't very useful. I was
perhaps less than courteous trying to extract the reasoning, but I
shouldn't have to in the first place.
Thanks,
johannes
^ permalink raw reply
* Re: [RFC 0/1] Allow MAC change on up interface
From: Johannes Berg @ 2019-08-20 19:43 UTC (permalink / raw)
To: Denis Kenzior, James Prestwood
Cc: linux-wireless@vger.kernel.org, Dan Williams
In-Reply-To: <b115e933-a357-9904-e831-dea7df1b46b9@gmail.com>
On Tue, 2019-08-20 at 14:22 -0500, Denis Kenzior wrote:
> Hi Johannes,
>
> So keeping things purely technical now :)
>
> > I thought so, but I had another thought later. It might be possible to
> > set LIVE_ADDR_CHANGE, but then block it in mac80211 when the interface
> > is already connected (or beaconing, or whatever, using the MAC address
> > in some way - even while scanning, remain-on-channel is active, etc.)
> >
>
> Here's what we would like to see:
>
> - The ability for userspace to add a 'Local Mac Address to use'
> attribute on CMD_CONNECT and CMD_AUTHENTICATE.
Why here though? I don't really like this as it's extra complexity
there, and dev_set_mac_address() is really easy to call from userspace.
Yes, that's sort of a round-trip more (you wouldn't even really have to
wait for it I guess), but we have to make trade-offs like that (vs.
kernel complexity) at some point.
> - It doesn't seem useful to add this to CMD_ASSOCIATE at the moment, as
> for new connections you'd always go either through CMD_AUTHENTICATE,
> CMD_ASSOCIATE sequence or use CMD_CONNECT. This should take care of
> some (most) of your objections about specifying different addresses for
> authenticate/associate. Feel free to correct me here.
That wasn't really my objection, I was more wondering why James
implemented it *only* for CMD_CONNECT, when I had been under the
(apparently mistaken) impression that one would generally prefer
CMD_ASSOCIATE over CMD_CONNECT, if available.
> - Optionally (and I'm thinking of tools like NM here), add the ability
> to set the mac address via RTNL while the device is UP but has no
> carrier, and things like scanning, offchannel, etc are not in progress.
> Though really I don't see how NM could guarantee this without bringing
> the device down first, so I'll let NM guys chime in to see if this is a
> good idea.
I'm thinking along the lines of letting you do this *instead* of the
scheme you described above. That way, we don't even need to have a
discussion over whether it makes sense at CONNECT, AUTH, ASSOC, or
whatever because you can essentially do it before/with any of these
commands.
Why would this not be sufficient?
> - We definitely do not want to to mess with the device state otherwise.
> E.g. no firmware downloading, powering the adapter down, etc. That just
> takes too much time.
I can understand this part.
> So tell us what you would like to see. A new
> IFF_NO_CARRIER_ADDRESS_CHANGE or whether it is possible to use
> IFF_LIVE_ADDR_CHANGE with some additional restrictions.
I don't know. This is not something I'm intimately familiar with. I
could imagine (as you quoted above) that we just set
IFF_LIVE_ADDR_CHANGE and manage the exclusions local to e.g. mac80211.
> > I still think you'd have to bake it into the mac80211<->driver API
> > somehow, because we normally "add_interface()" with the MAC address, and
> > nothing says that the driver cannot ignore the MAC address from that
> > point on. The fact that iwlwifi just copies it into every new MAC_CTXT
> > command and the firmware actually accepts the update seems rather
> > accidental and therefore fragile to rely on.
> >
>
> Since you seem to have a clear(er?) idea here, can you elaborate or
> possibly provide the driver interface changes you want to see?
I can see a few ways of doing this, for example having an optional
"change_vif_addr" method in the mac80211 ops, so that drivers can do the
necessary work. I suppose iwlwifi would actually want to send a new
MAC_CONTEXT command at this time, for example, because the firmware is
notoriously finicky when it comes to command sequences.
Alternatively, and this would work with all drivers, you could just
pretend to remove/add the interface, ie. in mac80211 call
drv_remove_interface(sdata)
// set new mac addr
drv_add_interface(sdata)
This has the advantage that it'll be guaranteed to work with all
drivers, at the expense of perhaps a few more firmware commands
(depending on the driver).
You can probably come up with other ways, like having a feature flag
whether this is supported and then the driver has to detect it as
certain documented points in time, etc., but all of those feel
relatively fragile to me.
My personal preference would be for
* allow RTNL MAC address changes at "idle" times (TBD what that really
means) with IFF_LIVE_ADDR_CHANGE, but mac80211 guarding it
* mac80211 doing remove/add as far as the driver is concerned
Yes, you can probably shave a few more milliseconds off by adding more
complexity, but unless we measure that and find it to be significant, I
think the added complexity (in cfg80211 code) and restrictions (many
drivers not supporting it if it's opt-in) wouldn't be worth it.
johannes
^ permalink raw reply
* Re: [RFC 0/1] Allow MAC change on up interface
From: James Prestwood @ 2019-08-20 19:53 UTC (permalink / raw)
To: Johannes Berg, linux-wireless
In-Reply-To: <6835732fcc59ba8dbbcda4abc6e17dad499a7d8d.camel@sipsolutions.net>
On Tue, 2019-08-20 at 08:59 +0200, Johannes Berg wrote:
> Hi James,
>
> Thanks for staying on topic.
>
> > > I don't, short of
> > >
> > > 1) don't do that then
> > > 2) extend the network stack to have
> > > IFF_LIVE_BUT_NO_CARRIER_ADDR_CHANGE
> > > or something like that
> >
> > So you mean 2 is my only option... ;) I am fine with this.
>
> :-)
>
> I thought so, but I had another thought later. It might be possible
> to
> set LIVE_ADDR_CHANGE, but then block it in mac80211 when the
> interface
> is already connected (or beaconing, or whatever, using the MAC
> address
> in some way - even while scanning, remain-on-channel is active, etc.)
Yeah that makes sense.
>
> I still think you'd have to bake it into the mac80211<->driver API
> somehow, because we normally "add_interface()" with the MAC address,
> and
> nothing says that the driver cannot ignore the MAC address from that
> point on. The fact that iwlwifi just copies it into every new
> MAC_CTXT
> command and the firmware actually accepts the update seems rather
> accidental and therefore fragile to rely on.
I havent looked into the actual drivers WRT add_interface so I'll take
a look. But I think I see the separation now and why it may not work
for all drivers/firmwares the way I did it.
So are you thinking we need another driver method:
"change_mac/set_mac"?
>
> > The iwlwifi change was just an example. It ultimately would be up
> > to
> > the maintainers of each driver to support this or not.
>
> Sure. I was just trying to say what I wrote one paragraph up.
>
> > > You've also not really explained what exactly is troubling you
> > > with
> > > changing the MAC address, you just mentioned some sort of "race
> > > condition"?
> >
> > In order to change the MAC on a per-AP/SSID is to: ifdown, change
> > MAC,
> > ifup via RTNL. The problem is that ifdown generates an RTNL link
> > down
> > event and there is no way of knowing how this event was generated
> > (by
> > you, hot-unplug, or some other issue in kernel?). Handling this
> > without
> > a race is simply not possible. You sort of just have to pray none
> > of
> > this happens (and its unlikely but it *could* happen).
>
> I see, at least sort of. I'm having a hard time seeing how this
> really
> is a problem in practice, but I suppose that's because I haven't
> tried
> implementing a fully event-driven stack.
>
> > The connect path is just what we (IWD) use for almost all types of
> > connections that support it (apart from things like SAE/OWE/FT).
> > Not
> > sure what you mean for "usually not taken for iwlwifi"? If you have
> > an
> > iwlwifi card and you issue CMD_CONNECT thats the path it takes...
>
> Interesting. I didn't think you'd do that, since it gives you far
> less
> control over things, and you need the other paths anyway for the
> features you mention, and the implementation in cfg80211 is far less
> complete than a typical firmware implementation would be.
There was talk about ditching CMD_CONNECT at one point, but for the
most part it does everything we need for the majority of connections.
But ultimately yes I think we do want this feature for
CMD_AUTHENTICATE/ASSOCIATE, and those details may be a bit more
involved. CMD_CONNECT was just an easier way to get my foot in the door
:)
Thanks,
James
>
> johannes
>
^ permalink raw reply
* Re: [RFC 0/1] Allow MAC change on up interface
From: Denis Kenzior @ 2019-08-20 19:46 UTC (permalink / raw)
To: Johannes Berg, James Prestwood, linux-wireless
In-Reply-To: <dd58815c934ab52acbd8a21d04dd0d44e9d32913.camel@sipsolutions.net>
Hi Johannes,
On 8/20/19 2:32 PM, Johannes Berg wrote:
> Hi Denis,
>
> Rather than replying to all the separate items here, just two things:
>
> 1) I'll reiterate - please keep things civil. You're taking things out
> of context a *lot* here, in what seems like an attempt to draw a
> parallel between my and your utterances.
>
> 2) I'll take your point that I've been somewhat dismissive in tone, and
> will try to change that.
>
I'll apologize for the methods I used, but you were not getting to 2)
above via our earlier discussions. Anyway, peace.
>
> I do want to reply to two things specifically though:
>
>> Fine. I get that. But how about asking what the use case is? Or say
>> you don't quite understand why something is needed?
>
> Really, I should *not* have to ask that. You should consider it your
> obligation to provide enough information for me to review patches
> without having to go back and ask what it is you actually want to
> achieve.
So let me ask you this. What do you _want_ to see when a contributor
submits something as an RFC, which that contributor states is not ready
for 'true' review and is really there to generate feedback?
Do you want to have that contributor use a different prefix? Every
maintainer is differrent. I get that. So if we want to start an actual
brainstorming session with you, how do we accomplish that?
>
> Compared to some other subsystems and maintainers I've dealt with, I
> think I've actually been rather patient in trying to extract the purpose
> of your changes, rather than *really* just dismissing them (which I've
> felt like many times.)
>
I'll admit you're not the worst I've dealt with, but you can always
improve, right? :)
>> a maintainer who's job (by definition)
>> is to encourage new contributors and improve the subsystem he
>> maintains...?
>
> This is what maybe you see as the maintainer's role, but I disagree, at
> least to some extent. I see the role more of a supervisor, somebody
> who's ensuring that all the changes coming in will work together. Yes, I
> have my own incentive to improve things, but if you have a particular
> incentive to improve a particular use case, the onus really is on you to
> convince me of that, not on me to try to ferret the reasoning out of you
> and make those improvements my own.
>
So this goes back to my earlier point. How do you want us to start a
discussion with you such that you don't become a 'supervisor' and
instead try to understand the pain points first?
And really, we are not expecting you to do the improvements on your own.
But you know the subsystem best. So you really should consider giving
actual guidance on how to accomplish something in the best way.
Also, look at it from the PoV of any new contributor. So while I can
definitely relate to what you're saying here, I think you should put
yourself in your peer's shoes and try to be more understanding of their
perspective. At least make the effort to hear people out...
>
> So please - come with some actual reasoning. This particular thread only
> offered "would elminate a few potential race conditions", in the cover
> letter, not even the patch itself, so it wasn't very useful. I was
> perhaps less than courteous trying to extract the reasoning, but I
> shouldn't have to in the first place.
>
Okay, so we'll work on that. But this is a two way street too. And
sometimes it seems like you're not actually reading the cover letters ;)
Regards,
-Denis
^ permalink raw reply
* Re: [RFC 0/1] Allow MAC change on up interface
From: Johannes Berg @ 2019-08-20 20:01 UTC (permalink / raw)
To: Denis Kenzior, James Prestwood, linux-wireless
In-Reply-To: <fcfc70bc-78ba-cd06-55ab-d53d80ac2cc8@gmail.com>
Hi,
> So let me ask you this. What do you _want_ to see when a contributor
> submits something as an RFC, which that contributor states is not ready
> for 'true' review and is really there to generate feedback?
>
> Do you want to have that contributor use a different prefix? Every
> maintainer is differrent. I get that. So if we want to start an actual
> brainstorming session with you, how do we accomplish that?
I'd be happy if you were to just state what you want to achieve, for
starters! Whether it's [RFC] with a cover letter saying "hey, we feel
this could be faster, and have come up with the following as an idea",
or just another email without any code changes saying "hey, we feel this
could be faster, how do you think we could do this" ... doesn't really
matter.
What you have *actually* been doing though (at least from my
perspective) is something along the lines of
"hey, here's a new way to do <X>"
without even really stating why the existing <X> doesn't work for you.
And where I questioned the need to have a new way to do <X>, well, we
got the other part of this thread.
> So this goes back to my earlier point. How do you want us to start a
> discussion with you such that you don't become a 'supervisor' and
> instead try to understand the pain points first?
Just explaining the pain points would help?
johannes
^ permalink raw reply
* Re: [PATCH 04/49] ath11k: add ahb.c
From: Johannes Berg @ 2019-08-20 20:05 UTC (permalink / raw)
To: Kalle Valo, linux-wireless; +Cc: ath11k, devicetree
In-Reply-To: <1566316095-27507-5-git-send-email-kvalo@codeaurora.org>
On Tue, 2019-08-20 at 18:47 +0300, Kalle Valo wrote:
>
> +static const struct service_to_pipe target_service_to_ce_map_wlan[] = {
> + {
> + __cpu_to_le32(ATH11K_HTC_SVC_ID_WMI_DATA_VO),
> + __cpu_to_le32(PIPEDIR_OUT), /* out = UL = host -> target */
> + __cpu_to_le32(3),
> + },
this might be nicer as C99 initializers as well? It's a struct of some
sort, after all.
> + { /* must be last */
> + __cpu_to_le32(0),
> + __cpu_to_le32(0),
> + __cpu_to_le32(0),
> + },
You don't need endian conversion for 0, even sparse will not complain,
but I'd argue it should anyway be something like
{ /* terminator entry */ }
since that's why it's there I guess?
> +#define ATH11K_TX_RING_MASK_3 0x0
You have a LOT of masks here that are 0, that seems odd?
> +/* enum ext_irq_num - irq nubers that can be used by external modules
typo ("numbers")
> +inline u32 ath11k_ahb_read32(struct ath11k_base *ab, u32 offset)
> +{
> + return ioread32(ab->mem + offset);
> +}
> +
> +inline void ath11k_ahb_write32(struct ath11k_base *ab, u32 offset, u32 value)
> +{
> + iowrite32(value, ab->mem + offset);
> +}
Just "inline" doesn't seem to make that much sense? If it's only used
here then I guess it should be static, otherwise not inline? Or maybe
you want it to be inlined *in this file* but available out-of-line
otherwise? I'm not sure that actually is guaranteed to work right in C?
> + val = ath11k_ahb_read32(ab, CE_HOST_IE_ADDRESS);
> + val |= BIT(ce_id);
> + ath11k_ahb_write32(ab, CE_HOST_IE_ADDRESS, val);
You could perhaps benefit from ath11k_ahb_setbit32() or something like
that, this repeats a few times?
> + if (__le32_to_cpu(ce_config->pipedir) & PIPEDIR_OUT) {
> + val = ath11k_ahb_read32(ab, CE_HOST_IE_ADDRESS);
> + val &= ~BIT(ce_id);
> + ath11k_ahb_write32(ab, CE_HOST_IE_ADDRESS, val);
and clearbit32() maybe. Dunno, I saw only 3 instances of each here I
guess, but still, feels repetitive.
> +int ath11k_ahb_start(struct ath11k_base *ab)
> +{
> + ath11k_ahb_ce_irqs_enable(ab);
> + ath11k_ce_rx_post_buf(ab);
> +
> + /* Bring up other components as appropriate */
Hmm. What would be appropriate? It's not really doing anything else?
> +void ath11k_ahb_stop(struct ath11k_base *ab)
> +{
> + if (!test_bit(ATH11K_FLAG_CRASH_FLUSH, &ab->dev_flags))
> + ath11k_ahb_ce_irqs_disable(ab);
> + ath11k_ahb_sync_ce_irqs(ab);
> + ath11k_ahb_kill_tasklets(ab);
> + del_timer_sync(&ab->rx_replenish_retry);
> + ath11k_ce_cleanup_pipes(ab);
> + /* Shutdown other components as appropriate */
likewise, it's not doing anything else?
johannes
^ permalink raw reply
* Re: [RFC 0/1] Allow MAC change on up interface
From: Denis Kenzior @ 2019-08-20 19:58 UTC (permalink / raw)
To: Johannes Berg, James Prestwood
Cc: linux-wireless@vger.kernel.org, Dan Williams
In-Reply-To: <3576ad937c0b40b971a1b9c1a7c7396731a94bad.camel@sipsolutions.net>
Hi Johannes,
On 8/20/19 2:43 PM, Johannes Berg wrote:
> On Tue, 2019-08-20 at 14:22 -0500, Denis Kenzior wrote:
>> Hi Johannes,
>>
>> So keeping things purely technical now :)
>>
>>> I thought so, but I had another thought later. It might be possible to
>>> set LIVE_ADDR_CHANGE, but then block it in mac80211 when the interface
>>> is already connected (or beaconing, or whatever, using the MAC address
>>> in some way - even while scanning, remain-on-channel is active, etc.)
>>>
>>
>> Here's what we would like to see:
>>
>> - The ability for userspace to add a 'Local Mac Address to use'
>> attribute on CMD_CONNECT and CMD_AUTHENTICATE.
>
> Why here though? I don't really like this as it's extra complexity
> there, and dev_set_mac_address() is really easy to call from userspace.
> Yes, that's sort of a round-trip more (you wouldn't even really have to
> wait for it I guess), but we have to make trade-offs like that (vs.
> kernel complexity) at some point.
But what actual complexity are we talking about here? If the kernel can
do this while the CONNECT is pending, why not? It makes things simpler
and faster for userspace. I don't see the downside unless you can
somehow objectively explain 'complexity'.
>
>> - It doesn't seem useful to add this to CMD_ASSOCIATE at the moment, as
>> for new connections you'd always go either through CMD_AUTHENTICATE,
>> CMD_ASSOCIATE sequence or use CMD_CONNECT. This should take care of
>> some (most) of your objections about specifying different addresses for
>> authenticate/associate. Feel free to correct me here.
>
> That wasn't really my objection, I was more wondering why James
> implemented it *only* for CMD_CONNECT, when I had been under the
> (apparently mistaken) impression that one would generally prefer
> CMD_ASSOCIATE over CMD_CONNECT, if available.
This was an RFC. There isn't much point for us to cross all the 't's
and dot all the 'i's if you hate the idea in the first place.
>
>> - Optionally (and I'm thinking of tools like NM here), add the ability
>> to set the mac address via RTNL while the device is UP but has no
>> carrier, and things like scanning, offchannel, etc are not in progress.
>> Though really I don't see how NM could guarantee this without bringing
>> the device down first, so I'll let NM guys chime in to see if this is a
>> good idea.
>
> I'm thinking along the lines of letting you do this *instead* of the
> scheme you described above. That way, we don't even need to have a
> discussion over whether it makes sense at CONNECT, AUTH, ASSOC, or
> whatever because you can essentially do it before/with any of these
> commands.
>
> Why would this not be sufficient?
>
It would get the job done. But it is still a waste of time and still
slowing us down. Look at it this way, even if we save 10ms here. Take
that and multiply by 3-4 billion devices and then by the number of
connections one does each day. This adds up to some serious time
wasted. So why not do this elegantly and faster in the first place?
>> - We definitely do not want to to mess with the device state otherwise.
>> E.g. no firmware downloading, powering the adapter down, etc. That just
>> takes too much time.
>
> I can understand this part.
>
>> So tell us what you would like to see. A new
>> IFF_NO_CARRIER_ADDRESS_CHANGE or whether it is possible to use
>> IFF_LIVE_ADDR_CHANGE with some additional restrictions.
>
> I don't know. This is not something I'm intimately familiar with. I
> could imagine (as you quoted above) that we just set
> IFF_LIVE_ADDR_CHANGE and manage the exclusions local to e.g. mac80211.
>
Okay, so lets operate under the assumption we can hi-jack
IFF_LIVE_ADDR_CHANGE for this
>>> I still think you'd have to bake it into the mac80211<->driver API
>>> somehow, because we normally "add_interface()" with the MAC address, and
>>> nothing says that the driver cannot ignore the MAC address from that
>>> point on. The fact that iwlwifi just copies it into every new MAC_CTXT
>>> command and the firmware actually accepts the update seems rather
>>> accidental and therefore fragile to rely on.
>>>
>>
>> Since you seem to have a clear(er?) idea here, can you elaborate or
>> possibly provide the driver interface changes you want to see?
>
> I can see a few ways of doing this, for example having an optional
> "change_vif_addr" method in the mac80211 ops, so that drivers can do the
> necessary work. I suppose iwlwifi would actually want to send a new
> MAC_CONTEXT command at this time, for example, because the firmware is
> notoriously finicky when it comes to command sequences.
>
> Alternatively, and this would work with all drivers, you could just
> pretend to remove/add the interface, ie. in mac80211 call
>
> drv_remove_interface(sdata)
> // set new mac addr
> drv_add_interface(sdata)
>
> This has the advantage that it'll be guaranteed to work with all
> drivers, at the expense of perhaps a few more firmware commands
> (depending on the driver).
>
> You can probably come up with other ways, like having a feature flag
> whether this is supported and then the driver has to detect it as
> certain documented points in time, etc., but all of those feel
> relatively fragile to me.
>
>
> My personal preference would be for
>
> * allow RTNL MAC address changes at "idle" times (TBD what that really
> means) with IFF_LIVE_ADDR_CHANGE, but mac80211 guarding it
> * mac80211 doing remove/add as far as the driver is concerned
Okay, so that's really what we wanted from you. :)
>
> Yes, you can probably shave a few more milliseconds off by adding more
> complexity, but unless we measure that and find it to be significant, I
> think the added complexity (in cfg80211 code) and restrictions (many
> drivers not supporting it if it's opt-in) wouldn't be worth it.
>
So we have a tool in the works that can measure some of these details.
Also, we can simply attempt to implement both ways and see if the
complexity is as bad as you say. Then you can make that choice.
Regards,
-Denis
^ permalink raw reply
* Re: [RFC 0/1] Allow MAC change on up interface
From: Johannes Berg @ 2019-08-20 20:06 UTC (permalink / raw)
To: James Prestwood, linux-wireless
In-Reply-To: <661903fa345563615cb781a6d9608607a3db963d.camel@gmail.com>
On Tue, 2019-08-20 at 15:53 -0400, James Prestwood wrote:
> > I thought so, but I had another thought later. It might be possible
> > to
> > set LIVE_ADDR_CHANGE, but then block it in mac80211 when the
> > interface
> > is already connected (or beaconing, or whatever, using the MAC
> > address
> > in some way - even while scanning, remain-on-channel is active, etc.)
>
> Yeah that makes sense.
>
> > I still think you'd have to bake it into the mac80211<->driver API
> > somehow, because we normally "add_interface()" with the MAC address,
> > and
> > nothing says that the driver cannot ignore the MAC address from that
> > point on. The fact that iwlwifi just copies it into every new
> > MAC_CTXT
> > command and the firmware actually accepts the update seems rather
> > accidental and therefore fragile to rely on.
>
> I havent looked into the actual drivers WRT add_interface so I'll take
> a look. But I think I see the separation now and why it may not work
> for all drivers/firmwares the way I did it.
>
> So are you thinking we need another driver method:
> "change_mac/set_mac"?
Perhaps. Let's continue that in the other sub-thread, where I replied in
more detail to Denis.
johannes
^ permalink raw reply
* Re: [RFC 0/1] Allow MAC change on up interface
From: Johannes Berg @ 2019-08-20 20:15 UTC (permalink / raw)
To: Denis Kenzior, James Prestwood
Cc: linux-wireless@vger.kernel.org, Dan Williams
In-Reply-To: <8c04da29-7515-1196-8431-67a6390bc00d@gmail.com>
On Tue, 2019-08-20 at 14:58 -0500, Denis Kenzior wrote:
>
> But what actual complexity are we talking about here? If the kernel can
> do this while the CONNECT is pending, why not? It makes things simpler
> and faster for userspace. I don't see the downside unless you can
> somehow objectively explain 'complexity'.
It's just extra code that we have to worry about. Right now you want it
for CMD_CONNECT and CMD_AUTH. Somebody will come up with a reason to do
it in CMD_ASSOC next, perhaps, who knows. Somebody else will say "oh,
this is how it's done, so let's add it to CMD_JOIN_IBSS", because of
course that's what they care about. OCB? Mesh? AP mode for tethering?
Etc.
I don't see how this will not keep proliferating, and each new thing
will come with its own dozen lines of code, a new feature flag, etc.
Relaxing and defining once and for all in which situations while the
interface is up you can actually allow changing the address, and then
having userspace do it that way is IMHO a better way to design the
system, since it forgoes entirely all those questions of when and how
and which new use cases will come up etc.
> This was an RFC. There isn't much point for us to cross all the 't's
> and dot all the 'i's if you hate the idea in the first place.
Sure, but I cannot distinguish between "we only want it on CMD_CONNECT"
and "we'll extend this once we agree" unless you actually say so. It'd
help to communicate which t's and i's you didn't cross or dot.
> It would get the job done. But it is still a waste of time and still
> slowing us down. Look at it this way, even if we save 10ms here. Take
> that and multiply by 3-4 billion devices and then by the number of
> connections one does each day. This adds up to some serious time
> wasted. So why not do this elegantly and faster in the first place?
It may be a bit faster, but I don't agree with elegantly. It may be more
elegant from the point of view of that single operation, but to me it's
a lot less elegant from a system view, with all the possible extensions
to it that we'll no doubt see, and not have thought about today.
johannes
^ permalink raw reply
* Re: [PATCH 06/49] ath11k: add ce.c
From: Johannes Berg @ 2019-08-20 20:23 UTC (permalink / raw)
To: Kalle Valo, linux-wireless; +Cc: ath11k, devicetree
In-Reply-To: <1566316095-27507-7-git-send-email-kvalo@codeaurora.org>
On Tue, 2019-08-20 at 18:47 +0300, Kalle Valo wrote:
> +static const struct ce_attr host_ce_config_wlan[] = {
> + /* CE0: host->target HTC control and raw streams */
> + {
> + .flags = CE_ATTR_FLAGS,
> + .src_nentries = 16,
> + .src_sz_max = 2048,
> + .dest_nentries = 0,
> + },
>
Curious - this looks a lot like a similar thing in AHB, but there you
did it all in little endian? Totally different really, just looks
similar, or what's the reason?
> + ring->skb[write_index] = skb;
> + write_index = CE_RING_IDX_INCR(nentries_mask, write_index);
> + ring->write_index = write_index;
> +
> + ath11k_hal_srng_access_end(ab, srng);
> +
> + spin_unlock_bh(&srng->lock);
> +
> + pipe->rx_buf_needed--;
> +
> + return 0;
> +
> +err:
> + ath11k_hal_srng_access_end(ab, srng);
> +
> + spin_unlock_bh(&srng->lock);
Seems like you could unify those unlock paths, the rx_buf_needed-- can
almost certainly be before the access_end/unlock, and then just set
ret=0?
> +static int ath11k_ce_completed_send_next(struct ath11k_ce_pipe *pipe,
> + struct sk_buff **skb)
Personally, I'd have preferred to have the *skb as the return value, and
use ERR_PTR()/IS_ERR() etc. to encode the error values, rather than the
double pointer.
> +{
> + struct ath11k_base *ab = pipe->ab;
> + struct hal_srng *srng;
> + unsigned int sw_index;
> + unsigned int nentries_mask;
> + u32 *desc;
> + int ret = 0;
Maybe don't initialize that to a success value, just for robustness? You
don't actually need to initialize it at all though if you set ret=0 when
you actually succeed, which might be even nicer given that the function
is simple enough for the compiler to figure out if you used 'ret'
uninitialized or not.
> +static struct ath11k_ce_ring *
> +ath11k_ce_alloc_ring(struct ath11k_base *ab, int nentries, int desc_sz)
> +{
> + struct ath11k_ce_ring *ce_ring;
> + dma_addr_t base_addr;
> +
> + ce_ring = kzalloc(sizeof(*ce_ring) + (nentries * sizeof(*ce_ring->skb)),
> + GFP_KERNEL);
You should probably use struct_size().
johannes
^ permalink raw reply
* Re: regression in ath10k dma allocation
From: Tobias Klausmann @ 2019-08-20 20:24 UTC (permalink / raw)
To: Christoph Hellwig, Hillf Danton
Cc: Nicolin Chen, kvalo, davem, ath10k, linux-wireless, netdev,
linux-kernel, m.szyprowski, robin.murphy, iommu, tobias.klausmann
In-Reply-To: <20190820071250.GA28968@lst.de>
On 20.08.19 09:12, Christoph Hellwig wrote:
> On Tue, Aug 20, 2019 at 02:58:33PM +0800, Hillf Danton wrote:
>> On Tue, 20 Aug 2019 05:05:14 +0200 Christoph Hellwig wrote:
>>> Tobias, plase try this patch:
>>>
> New version below:
>
> ---
> From b8a805e93be5a5662323b8ac61fe686df839c4ac Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Tue, 20 Aug 2019 11:45:49 +0900
> Subject: dma-direct: fix zone selection after an unaddressable CMA allocation
>
> The new dma_alloc_contiguous hides if we allocate CMA or regular
> pages, and thus fails to retry a ZONE_NORMAL allocation if the CMA
> allocation succeeds but isn't addressable. That means we either fail
> outright or dip into a small zone that might not succeed either.
>
> Thanks to Hillf Danton for debugging this issue.
>
> Fixes: b1d2dc009dec ("dma-contiguous: add dma_{alloc,free}_contiguous() helpers")
> Reported-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/iommu/dma-iommu.c | 3 +++
> include/linux/dma-contiguous.h | 5 +----
> kernel/dma/contiguous.c | 9 +++------
> kernel/dma/direct.c | 10 +++++++++-
> 4 files changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index d991d40f797f..f68a62c3c32b 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -965,10 +965,13 @@ static void *iommu_dma_alloc_pages(struct device *dev, size_t size,
> {
> bool coherent = dev_is_dma_coherent(dev);
> size_t alloc_size = PAGE_ALIGN(size);
> + int node = dev_to_node(dev);
> struct page *page = NULL;
> void *cpu_addr;
>
> page = dma_alloc_contiguous(dev, alloc_size, gfp);
> + if (!page)
> + page = alloc_pages_node(node, gfp, get_order(alloc_size));
> if (!page)
> return NULL;
>
> diff --git a/include/linux/dma-contiguous.h b/include/linux/dma-contiguous.h
> index c05d4e661489..03f8e98e3bcc 100644
> --- a/include/linux/dma-contiguous.h
> +++ b/include/linux/dma-contiguous.h
> @@ -160,10 +160,7 @@ bool dma_release_from_contiguous(struct device *dev, struct page *pages,
> static inline struct page *dma_alloc_contiguous(struct device *dev, size_t size,
> gfp_t gfp)
> {
> - int node = dev ? dev_to_node(dev) : NUMA_NO_NODE;
> - size_t align = get_order(PAGE_ALIGN(size));
> -
> - return alloc_pages_node(node, gfp, align);
> + return NULL;
> }
>
> static inline void dma_free_contiguous(struct device *dev, struct page *page,
> diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
> index 2bd410f934b3..e6b450fdbeb6 100644
> --- a/kernel/dma/contiguous.c
> +++ b/kernel/dma/contiguous.c
> @@ -230,9 +230,7 @@ bool dma_release_from_contiguous(struct device *dev, struct page *pages,
> */
> struct page *dma_alloc_contiguous(struct device *dev, size_t size, gfp_t gfp)
> {
> - int node = dev ? dev_to_node(dev) : NUMA_NO_NODE;
> - size_t count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> - size_t align = get_order(PAGE_ALIGN(size));
> + size_t count = size >> PAGE_SHIFT;
> struct page *page = NULL;
> struct cma *cma = NULL;
>
> @@ -243,14 +241,12 @@ struct page *dma_alloc_contiguous(struct device *dev, size_t size, gfp_t gfp)
>
> /* CMA can be used only in the context which permits sleeping */
> if (cma && gfpflags_allow_blocking(gfp)) {
> + size_t align = get_order(size);
> size_t cma_align = min_t(size_t, align, CONFIG_CMA_ALIGNMENT);
>
> page = cma_alloc(cma, count, cma_align, gfp & __GFP_NOWARN);
> }
>
> - /* Fallback allocation of normal pages */
> - if (!page)
> - page = alloc_pages_node(node, gfp, align);
> return page;
> }
>
> @@ -258,6 +254,7 @@ struct page *dma_alloc_contiguous(struct device *dev, size_t size, gfp_t gfp)
> * dma_free_contiguous() - release allocated pages
> * @dev: Pointer to device for which the pages were allocated.
> * @page: Pointer to the allocated pages.
> + int node = dev ? dev_to_node(dev) : NUMA_NO_NODE;
> * @size: Size of allocated pages.
> *
> * This function releases memory allocated by dma_alloc_contiguous(). As the
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 795c9b095d75..706113c6bebc 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -85,6 +85,8 @@ static bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size)
> struct page *__dma_direct_alloc_pages(struct device *dev, size_t size,
> dma_addr_t *dma_handle, gfp_t gfp, unsigned long attrs)
> {
> + size_t alloc_size = PAGE_ALIGN(size);
> + int node = dev_to_node(dev);
> struct page *page = NULL;
> u64 phys_mask;
>
> @@ -95,8 +97,14 @@ struct page *__dma_direct_alloc_pages(struct device *dev, size_t size,
> gfp &= ~__GFP_ZERO;
> gfp |= __dma_direct_optimal_gfp_mask(dev, dev->coherent_dma_mask,
> &phys_mask);
> + page = dma_alloc_contiguous(dev, alloc_size, gfp);
> + if (page && !dma_coherent_ok(dev, page_to_phys(page), size)) {
> + dma_free_contiguous(dev, page, alloc_size);
> + page = NULL;
> + }
> again:
> - page = dma_alloc_contiguous(dev, size, gfp);
> + if (!page)
> + page = alloc_pages_node(node, gfp, get_order(alloc_size));
> if (page && !dma_coherent_ok(dev, page_to_phys(page), size)) {
> dma_free_contiguous(dev, page, size);
> page = NULL;
I can confirm this resolves the regression!
Tested-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>
Thanks for the work of all involved,
Tobias
^ permalink raw reply
* Re: [PATCH 46/49] ath11k: add wmi.h
From: Johannes Berg @ 2019-08-20 20:29 UTC (permalink / raw)
To: Kalle Valo, linux-wireless; +Cc: ath11k, devicetree
In-Reply-To: <1566316095-27507-47-git-send-email-kvalo@codeaurora.org>
On Tue, 2019-08-20 at 18:48 +0300, Kalle Valo wrote:
>
> +enum wmi_cmd_group {
> + /* 0 to 2 are reserved */
> + WMI_GRP_START = 0x3,
> + WMI_GRP_SCAN = WMI_GRP_START, /* 0x3 */
> + WMI_GRP_PDEV, /* 0x4 */
If you're going to spell out the numbers anyway, why not do it in C
rather than a comment?
WMI_GRP_PDEV = 0x4,
would tell you just as much, and be much less error-prone.
> +struct wmi_pdev_set_hw_mode_cmd_param {
> + u32 tlv_header;
> + u32 pdev_id;
> + u32 hw_mode_index;
> + u32 num_band_to_mac;
> +} __packed;
Does it really makes sense for something to be using "u32" (i.e. host
endian) but then __packed (kinda tagging it as "I am using this with the
hardware, don't change the layout")?
That really applies to a lot of the things here.
> +struct channel_param {
> + u8 chan_id;
> + u8 pwr;
> + u32 mhz;
> + u32 half_rate:1,
> + quarter_rate:1,
> + dfs_set:1,
> + dfs_set_cfreq2:1,
> + is_chan_passive:1,
> + allow_ht:1,
> + allow_vht:1,
> + set_agile:1;
> + u32 phy_mode;
> + u32 cfreq1;
> + u32 cfreq2;
> + char maxpower;
> + char minpower;
> + char maxregpower;
> + u8 antennamax;
> + u8 reg_class_id;
> +} __packed;
Bitfields in FW structs are even less likely to work right, I'd avoid
that.
(and if you have this copy engine do endian conversion, then the u8
fields won't work right since that ending seems to be working on u32s?)
That probably all applies elsewhere too, but the file is pretty long ;-)
Personally, I'd also consider splitting internal driver usage stuff and
FW API into different files, but that's your decision. I just find it
lets me understand it better even when I'm looking at it myself.
johannes
^ permalink raw reply
* Re: [PATCH 08/49] ath11k: add core.c
From: Johannes Berg @ 2019-08-20 20:32 UTC (permalink / raw)
To: Kalle Valo, linux-wireless; +Cc: ath11k, devicetree
In-Reply-To: <1566316095-27507-9-git-send-email-kvalo@codeaurora.org>
> +module_param_named(debug_mask, ath11k_debug_mask, uint, 0644);
> +
> +MODULE_PARM_DESC(debug_mask, "Debugging mask");
> +
> +static const struct ath11k_hw_params ath11k_hw_params = {
> + .name = "ipq8074",
indentation here seems a bit too much
> +MODULE_LICENSE("Dual BSD/GPL");
All your files state "ISC", shouldn't that be reflected here?
johannes
^ permalink raw reply
* Re: [RFC 0/1] Allow MAC change on up interface
From: Denis Kenzior @ 2019-08-20 20:37 UTC (permalink / raw)
To: Johannes Berg, James Prestwood
Cc: linux-wireless@vger.kernel.org, Dan Williams
In-Reply-To: <3fd41591acd55535863f11a0cc4f0f5f2afd5bdf.camel@sipsolutions.net>
Hi Johannes,
On 8/20/19 3:15 PM, Johannes Berg wrote:
> On Tue, 2019-08-20 at 14:58 -0500, Denis Kenzior wrote:
>>
>> But what actual complexity are we talking about here? If the kernel can
>> do this while the CONNECT is pending, why not? It makes things simpler
>> and faster for userspace. I don't see the downside unless you can
>> somehow objectively explain 'complexity'.
>
> It's just extra code that we have to worry about. Right now you want it
> for CMD_CONNECT and CMD_AUTH. Somebody will come up with a reason to do
> it in CMD_ASSOC next, perhaps, who knows. Somebody else will say "oh,
> this is how it's done, so let's add it to CMD_JOIN_IBSS", because of
> course that's what they care about. OCB? Mesh? AP mode for tethering?
> Etc.
I don't buy the extra code argument. If you want to do something useful
you need to write 'extra code'.
The rest, I'm not sure why you are worried about them now? For station
there's a very clear & present use case. If such a clear and present
use case is presented for AP or Mesh, then deal with it then.
>
> I don't see how this will not keep proliferating, and each new thing
> will come with its own dozen lines of code, a new feature flag, etc.
Such is life? :)
>
> Relaxing and defining once and for all in which situations while the
> interface is up you can actually allow changing the address, and then
> having userspace do it that way is IMHO a better way to design the
> system, since it forgoes entirely all those questions of when and how
> and which new use cases will come up etc.
>
That would be great in theory, but practically never works at least in
my experience. So maybe keep and open mind? There is a clear need to
make this path as fast as possible for STA. There is no such need (yet)
for the other cases you mentioned.
>> This was an RFC. There isn't much point for us to cross all the 't's
>> and dot all the 'i's if you hate the idea in the first place.
>
> Sure, but I cannot distinguish between "we only want it on CMD_CONNECT"
> and "we'll extend this once we agree" unless you actually say so. It'd
> help to communicate which t's and i's you didn't cross or dot.
Okay, I'll admit the RFC description could have been better. But in the
end you're human last I checked (at least I recall meeting you several
times? ;) How about a simple "Why do you think you need this?" first?
Regards,
-Denis
^ permalink raw reply
* Re: [PATCH 31/49] ath11k: add mac.c
From: Johannes Berg @ 2019-08-20 20:46 UTC (permalink / raw)
To: Kalle Valo, linux-wireless; +Cc: ath11k, devicetree
In-Reply-To: <1566316095-27507-32-git-send-email-kvalo@codeaurora.org>
On Tue, 2019-08-20 at 18:47 +0300, Kalle Valo wrote:
> +static int ath11k_mac_op_config(struct ieee80211_hw *hw, u32 changed)
> +{
> + struct ath11k *ar = hw->priv;
> + int ret = 0;
> +
> + /* mac80211 requires this op to be present and that's why
> + * there's an empty function, this can be extended when
> + * required.
> + */
Well, oops. Maybe it shouldn't be required?
> + mutex_lock(&ar->conf_mutex);
> +
> + /* TODO: Handle configuration changes as appropriate */
> +
> + mutex_unlock(&ar->conf_mutex);
It's not actually empty though - why bother locking the mutex for
nothing?
> + if (sta->mfp) {
> + /* TODO: Need to check if FW supports PMF? */
Probably not? shouldn't get a sta with MFP unless you advertised support
for it. At least I'd think so, and consider it a mac80211 bug if you
still did.
> + /* This is a workaround for HT-enabled STAs which break the spec
> + * and have no HT capabilities RX mask (no HT RX MCS map).
> + *
> + * As per spec, in section 20.3.5 Modulation and coding scheme (MCS),
> + * MCS 0 through 7 are mandatory in 20MHz with 800 ns GI at all STAs.
Wouldn't that better be in mac80211?
> + ampdu_factor = (vht_cap->cap &
> + IEEE80211_VHT_CAP_MAX_A_MPDU_LENGTH_EXPONENT_MASK) >>
> + IEEE80211_VHT_CAP_MAX_A_MPDU_LENGTH_EXPONENT_SHIFT;
consider u32_get_bits() or something like that from bitfield.h
> + /* Workaround: Some Netgear/Linksys 11ac APs set Rx A-MPDU factor to
> + * zero in VHT IE. Using it would result in degraded throughput.
> + * arg->peer_max_mpdu at this point contains HT max_mpdu so keep
> + * it if VHT max_mpdu is smaller.
> + */
> + arg->peer_max_mpdu = max(arg->peer_max_mpdu,
> + (1U << (IEEE80211_HT_MAX_AMPDU_FACTOR +
> + ampdu_factor)) - 1);
Wait, that seems familiar. Again, put it into mac80211?
> +static void ath11k_peer_assoc_h_smps(struct ieee80211_sta *sta,
> + struct peer_assoc_params *arg)
> +{
> + const struct ieee80211_sta_ht_cap *ht_cap = &sta->ht_cap;
> + int smps;
> +
> + if (!ht_cap->ht_supported)
> + return;
> +
> + smps = ht_cap->cap & IEEE80211_HT_CAP_SM_PS;
> + smps >>= IEEE80211_HT_CAP_SM_PS_SHIFT;
also here, u*_get_bits() or something might be nicer
(and yes, I've written tons of code like this myself before that
existed, which is why I'm pointing it out - it's much nicer)
> +void ath11k_mac_drain_tx(struct ath11k *ar)
> +{
> + /* make sure rcu-protected mac80211 tx path itself is drained */
> + synchronize_net();
Doesn't mac80211 ensure that in the relevant places like flush()? But
then again, not sure where you call this.
> + ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "mac set fixed rate params vdev %i rate 0x%02hhx nss %hhu sgi %hhu\n",
> + arvif->vdev_id, rate, nss, sgi);
nit: that could use a line-break
> + vdev_param = WMI_VDEV_PARAM_FIXED_RATE;
> + ret = ath11k_wmi_vdev_set_param_cmd(ar, arvif->vdev_id,
> + vdev_param, rate);
> + if (ret) {
> + ath11k_warn(ar->ab, "failed to set fixed rate param 0x%02x: %d\n",
>
> + /* TODO: Check if HT capability advertised from firmware is different
> + * for each band for a dual band capable radio. It will be tricky to
> + * handle it when the ht capability different for each band.
> + */
For each band shouldn't really be that tricky?
johannes
^ permalink raw reply
* Re: [PATCH] cfg80211: VLAN offload support for set_key and set_sta_vlan
From: Jouni Malinen @ 2019-08-20 20:50 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, Gurumoorthi Gnanasambandhan
In-Reply-To: <3a8edc6691a03ed3c253d95811d5fd6cae453a03.camel@sipsolutions.net>
On Mon, Aug 19, 2019 at 12:16:03PM +0200, Johannes Berg wrote:
> On Thu, 2019-08-15 at 16:38 +0300, Jouni Malinen wrote:
> > From: Gurumoorthi Gnanasambandhan <gguru@codeaurora.org>
> >
> > This provides an alternative mechanism for AP VLAN support where a
> > single netdev is used with VLAN tagged frames instead of separate
> > netdevs for each VLAN without tagged frames from the WLAN driver.
> >
> > By setting NL80211_EXT_FEATURE_VLAN_OFFLOAD flag the driver indicates
> > support for a single netdev with VLAN tagged frames. Separate
> > VLAN-specific netdevs are added using vcongig similar to Ethernet.
> > NL80211_CMD_NEW_KEY and NL80211_CMD_SET_STATION will optionally specify
> > vlan_id using NL80211_ATTR_VLAN_ID.
>
> Without really looking at the specifics, it might be relatively simple
> to support this in mac80211?
Yes, that is something that I was thinking about when going through
this.. I don't remember why exactly mac80211 ended up with its current
design for per-AP_VLAN netdevs without tags, but it could indeed be
quite convenient to have this alternative approach available. I guess
both of them have some benefits, so this would likely be left with two
different mechanisms left to maintain, but the needed implementation in
mac80211 for this would seem to be pretty minimal (also without looking
at the exact details..).
--
Jouni Malinen PGP id EFC895FA
^ 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