* Re: [PATCH] ath10k: Fix spinlock use in coverage class hack
From: Valo, Kalle @ 2016-09-30 12:58 UTC (permalink / raw)
To: Benjamin Berg
Cc: Simon Wunderlich, Thiagarajan, Vasanthakumar,
linux-wireless@vger.kernel.org, Sebastian Gottschall,
ath10k @ lists . infradead . org, michal.kazior@tieto.com,
Mathias Kretschmer
In-Reply-To: <20160914163231.20863-1-benjamin@sipsolutions.net>
Benjamin Berg <benjamin@sipsolutions.net> writes:
> ath10k_hw_qca988x_set_coverage_class needs to hold both conf_mutex and
> the data_lock spin lock for parts of the function. However, data_lock
> is only needed while storing the coverage_class to store the value that
> the card is configured to.
>
> Fix the locking issue by only holding data_lock for the required duration=
.
>
> Signed-off-by: Benjamin Berg <benjamin@sipsolutions.net>
Thanks, I also folded this with the patch in the pending branch.
> And yes, I fully agree with your points of it being rather fragile. But a=
s
> you said, it should be entirely safe if not used.
That's good.
> Obviously a firmware implementation would be preferential.
That's a shame as this feature is quite often requested. But if the
firmware ever starts supporting the featrue we can then remove this hack
from ath10k.
> This locking issue was pretty unnecessary. Lets see if any more issues sh=
ow
> up in a closer review.
I can't see the locking problem anymore so it seems to be fixed. I'll
fix some minor things and send v2. I'll also CC linux-wireless.
--=20
Kalle Valo=
^ permalink raw reply
* Re: [PATCH 1/3] cw1200: Don't leak memory if krealloc failes
From: Sergei Shtylyov @ 2016-09-30 12:56 UTC (permalink / raw)
To: Johannes Thumshirn, Solomon Peachy, Kalle Valo
Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <1475237495-15030-1-git-send-email-jthumshirn@suse.de>
Hello.
On 9/30/2016 3:11 PM, Johannes Thumshirn wrote:
> The call to krealloc() in wsm_buf_reserve() directly assigns the newly
> returned memory to buf->begin. This is all fine except when krealloc()
> failes we loose the ability to free the old memory pointed to by
Fails.
> buf->begin. If we just create a temporary variable to assign memory to
> and assign the memory to it we can mitigate the memory leak.
>
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
> drivers/net/wireless/st/cw1200/wsm.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/wireless/st/cw1200/wsm.c b/drivers/net/wireless/st/cw1200/wsm.c
> index 680d60e..12fad99 100644
> --- a/drivers/net/wireless/st/cw1200/wsm.c
> +++ b/drivers/net/wireless/st/cw1200/wsm.c
> @@ -1807,16 +1807,18 @@ static int wsm_buf_reserve(struct wsm_buf *buf, size_t extra_size)
> {
> size_t pos = buf->data - buf->begin;
> size_t size = pos + extra_size;
> + u8 *tmp;
>
> size = round_up(size, FWLOAD_BLOCK_SIZE);
>
> - buf->begin = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA);
> - if (buf->begin) {
> - buf->data = &buf->begin[pos];
> - buf->end = &buf->begin[size];
> - return 0;
> - } else {
> - buf->end = buf->data = buf->begin;
> + tmp = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA);
> + if (tmp) {
!tmp, you mean?
> + wsm_buf_deinit(buf);
> return -ENOMEM;
> }
> +
> + buf->begin = tmp;
> + buf->data = &buf->begin[pos];
> + buf->end = &buf->begin[size];
> + return 0;
> }
MBR, Sergei
^ permalink raw reply
* Re: [PATCH 3/3] mac80211: Set lower memory limit for non-VHT devices
From: Johannes Berg @ 2016-09-30 12:51 UTC (permalink / raw)
To: Toke Høiland-Jørgensen; +Cc: make-wifi-fast, linux-wireless, netdev
In-Reply-To: <87twcx5zll.fsf@toke.dk>
> > I kinda see the logic here - we really don't need to queue as much
> > if we can't possibly transmit it out quickly - but it seems to me
> > we should also throw in some kind of limit that's relative to the
> > amount of memory you have on the system?
>
> Yes, ideally. That goes for FQ-CoDel as well, BTW. LEDE currently
> carries a patch for that which just changes the hard-coded default to
> another hard-coded default. Not sure how to get a good value to use,
> though; and deciding on how large a fraction of memory to use for
> packets starts smelling an awful lot like setting policy in the
> kernel, doesn't it?
Yeah, I agree it does seem awkward.
Perhaps we should instead pick a low limit and let users change it more
easily (i.e. not debugfs)? I don't know a good answer to this either.
johannes
^ permalink raw reply
* Re: [PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue
From: Johannes Berg @ 2016-09-30 12:49 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, make-wifi-fast, linux-wireless
In-Reply-To: <20160922170420.5193-3-toke@toke.dk>
Applied, with the nits fixed as discussed.
Come to think of it, if somebody is bored ;-) perhaps a hwsim option to
use TXQs (should be optional I guess) would be nice so we can exercise
this code with the wpa_supplicant hwsim tests. That would have caught
the TKIP issues etc. pretty early on too, I think.
johannes
^ permalink raw reply
* Re: [PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue
From: Toke Høiland-Jørgensen @ 2016-09-30 12:45 UTC (permalink / raw)
To: Johannes Berg; +Cc: make-wifi-fast, linux-wireless
In-Reply-To: <1475239412.17481.59.camel@sipsolutions.net>
Johannes Berg <johannes@sipsolutions.net> writes:
>> Not sure if you want a v10, or if you're satisfied with the above
>> comments and will just fix up the nits on merging?
>>
>
> I'll fix it up. Thanks!
Cool, thanks :)
-Toke
^ permalink raw reply
* Re: [PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue
From: Johannes Berg @ 2016-09-30 12:43 UTC (permalink / raw)
To: Toke Høiland-Jørgensen; +Cc: make-wifi-fast, linux-wireless
In-Reply-To: <87y4295zpy.fsf@toke.dk>
> Because I need to run it anyway for the xmit_fast path on dequeue. I
> thought doing it this way simplifies the code (at the cost of the
> handler getting called twice when xmit_fast is not active).
Ok, that's fair.
> > I *think* it should commute with the rate control handler, but even
> > so, wouldn't it make more sense to have rate control late? Assuming
> > the packets are queued for some amount of time, having rate control
> > information queued with them would get stale.
>
> Yes, having rate control run at dequeue would be good, and that's
> what I did initially. However, I found that this would lead to a
> deadlock because the rate control handler would send out packets in
> some cases (I forget the details but can go back and check if
> needed). And since the dequeue function is called with the driver TXQ
> lock held, that would lead to a deadlock when those packets made it
> to the driver TX path.
That seems really odd, but I can see how a deadlock happens then.
> So I decided to just keep it this way for now; I plan to go poking
> into the rate controller later anyway, so moving the handler to later
> could be part of that.
Sure, that's fair.
> But that handler only sets a few flags? Is
> tx->sdata->control_port_protocol likely to change while the packet is
> queued?
Oh right, I confused things there. We check the controlled port much
earlier, but anyway that should be OK.
> > It's a bit unfortunate that you lose fast-xmit here completely for
> > the key stuff, but I don't see a good way to avoid that, other than
> > completely rejiggering all the (possibly affected) queues when keys
> > change... might be very complex to do that, certainly a follow-up
> > patch if it's desired.
>
> Yeah, figured it was better to have something that's correct and then
> go back and change it if the performance hit turns out to be too
> high.
Makes sense.
> > This check seems a bit weird though - how could fast-xmit be set
> > without a TXQ station?
>
> I think that is probably just left over from before I introduced the
> control flag. Should be fine to remove it.
Ok.
> >
> > >
> > > +++ b/net/mac80211/util.c
> > > @@ -3393,11 +3393,18 @@ void ieee80211_txq_get_depth(struct
> > > ieee80211_txq *txq,
> > > unsigned long *byte_cnt)
> > > {
> > > struct txq_info *txqi = to_txq_info(txq);
> > > + u32 frag_cnt = 0, frag_bytes = 0;
> > > + struct sk_buff *skb;
> > > +
> > > + skb_queue_walk(&txqi->frags, skb) {
> > > + frag_cnt++;
> > > + frag_bytes += skb->len;
> > > + }
> >
> > I hope this is called infrequently :)
>
> Well, ath10k is the only user. It does get called on each
> wake_tx_queue, though, so not that infrequently. My reasoning was
> that since the frags queue is never going to have more than a fairly
> small number of packets in it (those produced from a single split
> packet), counting this way is acceptable instead of keeping a state
> variable up to date. Can change it if you disagree :)
No, I guess you're right, it can't be a long queue.
> Not sure if you want a v10, or if you're satisfied with the above
> comments and will just fix up the nits on merging?
>
I'll fix it up. Thanks!
johannes
^ permalink raw reply
* Re: [PATCH 3/3] mac80211: Set lower memory limit for non-VHT devices
From: Toke Høiland-Jørgensen @ 2016-09-30 12:41 UTC (permalink / raw)
To: Johannes Berg; +Cc: make-wifi-fast, linux-wireless, netdev
In-Reply-To: <1475235230.17481.43.camel@sipsolutions.net>
Johannes Berg <johannes@sipsolutions.net> writes:
> On Fri, 2016-09-23 at 21:59 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>> Small devices can run out of memory from queueing too many packets.
>> If VHT is not supported by the PHY, having more than 4 MBytes of
>> total queue in the TXQ intermediate queues is not needed, and so we
>> can safely limit the memory usage in these cases and avoid OOM.
>
> I kinda see the logic here - we really don't need to queue as much if
> we can't possibly transmit it out quickly - but it seems to me we
> should also throw in some kind of limit that's relative to the amount
> of memory you have on the system?
Yes, ideally. That goes for FQ-CoDel as well, BTW. LEDE currently
carries a patch for that which just changes the hard-coded default to
another hard-coded default. Not sure how to get a good value to use,
though; and deciding on how large a fraction of memory to use for
packets starts smelling an awful lot like setting policy in the kernel,
doesn't it?
> I've applied these anyway though. I just don't like your assumption (b)
> much as the rationale for it.
Right, thanks. I'll come up with a better rationale next time ;)
-Toke
^ permalink raw reply
* [PATCH v2] cw1200: Don't leak memory if krealloc failes
From: Johannes Thumshirn @ 2016-09-30 12:39 UTC (permalink / raw)
To: Solomon Peachy, Kalle Valo
Cc: linux-wireless, netdev, linux-kernel, Johannes Thumshirn,
Johannes Berg
The call to krealloc() in wsm_buf_reserve() directly assigns the newly
returned memory to buf->begin. This is all fine except when krealloc()
failes we loose the ability to free the old memory pointed to by
buf->begin. If we just create a temporary variable to assign memory to
and assign the memory to it we can mitigate the memory leak.
Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Johannes Berg <johannes@sipsolutions.net>
---
drivers/net/wireless/st/cw1200/wsm.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
Changes from v1:
* Do the correct check for a failed re-allocation (Johannes Berg)
diff --git a/drivers/net/wireless/st/cw1200/wsm.c b/drivers/net/wireless/st/cw1200/wsm.c
index 680d60e..ffb245e 100644
--- a/drivers/net/wireless/st/cw1200/wsm.c
+++ b/drivers/net/wireless/st/cw1200/wsm.c
@@ -1807,16 +1807,18 @@ static int wsm_buf_reserve(struct wsm_buf *buf, size_t extra_size)
{
size_t pos = buf->data - buf->begin;
size_t size = pos + extra_size;
+ u8 *tmp;
size = round_up(size, FWLOAD_BLOCK_SIZE);
- buf->begin = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA);
- if (buf->begin) {
- buf->data = &buf->begin[pos];
- buf->end = &buf->begin[size];
- return 0;
- } else {
- buf->end = buf->data = buf->begin;
+ tmp = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA);
+ if (!tmp) {
+ wsm_buf_deinit(buf);
return -ENOMEM;
}
+
+ buf->begin = tmp;
+ buf->data = &buf->begin[pos];
+ buf->end = &buf->begin[size];
+ return 0;
}
--
1.8.5.6
^ permalink raw reply related
* Re: [PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue
From: Toke Høiland-Jørgensen @ 2016-09-30 12:39 UTC (permalink / raw)
To: Johannes Berg; +Cc: make-wifi-fast, linux-wireless
In-Reply-To: <1475231220.17481.32.camel@sipsolutions.net>
Johannes Berg <johannes@sipsolutions.net> writes:
> Hi Toke,
>
> Sorry for the delay reviewing this.
>
> I think I still have a few comments/questions.
No worries. And not terribly surprised ;)
>> +static inline bool txq_has_queue(struct ieee80211_txq *txq)
>> +{
>> + struct txq_info *txqi =3D to_txq_info(txq);
>> + return !(skb_queue_empty(&txqi->frags) && !txqi->tin.backlog_packets);
>> +}
>
> Tiny nit - there should probably be a blank line between the two lines
> here, but I could just fix that when I apply if you don't resend anyway
> for some other reason.
Noted.
> [snip helper stuff that looks fine]
>
>> - if (!tx->sta->sta.txq[0])
>> - hdr->seq_ctrl =3D ieee80211_tx_next_seq(tx->sta, tid);
>> + hdr->seq_ctrl =3D ieee80211_tx_next_seq(tx->sta, tid);
>
> Just to make sure I get this right - this is because the handler is now
> run on dequeue, so the special case is no longer needed?
Yup. The same change is made in xmit_fast (but obscured by the moving of
the surrounding code into _finish()).
>> =C2=A0#define CALL_TXH(txh) \
>> @@ -1656,6 +1684,31 @@ static int invoke_tx_handlers(struct ieee80211_tx=
_data *tx)
>> =C2=A0 if (!ieee80211_hw_check(&tx->local->hw, HAS_RATE_CONTROL))
>> =C2=A0 CALL_TXH(ieee80211_tx_h_rate_ctrl);
>
> Just for reference - the code block here that's unchanged contains
> this:
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 CALL_TXH(ieee80211_tx_h_dynamic_ps);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0CALL_TXH(ieee80211_tx_h_c=
heck_assoc);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0CALL_TXH(ieee80211_tx_h_p=
s_buf);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0CALL_TXH(ieee80211_tx_h_c=
heck_control_port_protocol);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0CALL_TXH(ieee80211_tx_h_s=
elect_key);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!ieee80211_hw_check(&=
tx->local->hw, HAS_RATE_CONTROL))
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0CALL_TXH(ieee80211_tx_h_rate_ctrl);
>
>> +static int invoke_tx_handlers_late(struct ieee80211_tx_data *tx)
>> +{
>> + struct ieee80211_tx_info *info =3D IEEE80211_SKB_CB(tx->skb);
>> + ieee80211_tx_result res =3D TX_CONTINUE;
>> +
>> =C2=A0 if (unlikely(info->flags &
>> IEEE80211_TX_INTFL_RETRANSMISSION)) {
>> =C2=A0 __skb_queue_tail(&tx->skbs, tx->skb);
>> =C2=A0 tx->skb =3D NULL;
>
> And this code here is also unchanged from the original TX handler
> invocation, so contains this:
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (unlikely(info->flags & IEEE80211_TX_INTFL=
_RETRANSMISSION)) {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0__skb_queue_tail(&tx->skbs, tx->skb);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0tx->skb =3D NULL;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0goto txh_done;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
>
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0CALL_TXH(ieee80211_tx_h_m=
ichael_mic_add);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0CALL_TXH(ieee80211_tx_h_s=
equence);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0CALL_TXH(ieee80211_tx_h_f=
ragment);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* handlers after fragmen=
t must be aware of tx info fragmentation! */
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0CALL_TXH(ieee80211_tx_h_s=
tats);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0CALL_TXH(ieee80211_tx_h_e=
ncrypt);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!ieee80211_hw_check(&=
tx->local->hw, HAS_RATE_CONTROL))
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0CALL_TXH(ieee80211_tx_h_calculate_duration);
>
> But now you have a problem (that you solved) that the key pointer can
> be invalidated while you have the packet queued between the two points,
> and then the tx_h_michael_mic_add and/or tx_h_encrypt would crash.
>
> You solve this by re-running tx_h_select_key() on dequeue, but it's not
> clear to me why you didn't move that to the late handlers instead?
Because I need to run it anyway for the xmit_fast path on dequeue. I
thought doing it this way simplifies the code (at the cost of the
handler getting called twice when xmit_fast is not active).
> I *think* it should commute with the rate control handler, but even
> so, wouldn't it make more sense to have rate control late? Assuming
> the packets are queued for some amount of time, having rate control
> information queued with them would get stale.
Yes, having rate control run at dequeue would be good, and that's what I
did initially. However, I found that this would lead to a deadlock
because the rate control handler would send out packets in some cases (I
forget the details but can go back and check if needed). And since the
dequeue function is called with the driver TXQ lock held, that would
lead to a deadlock when those packets made it to the driver TX path.
So I decided to just keep it this way for now; I plan to go poking into
the rate controller later anyway, so moving the handler to later could
be part of that.
> Similarly, it seems to me that checking the control port protocol later
> (or perhaps duplicating that?) would be a good idea?
But that handler only sets a few flags? Is
tx->sdata->control_port_protocol likely to change while the packet is
queued?
>> +/*
>> + * Can be called while the sta lock is held. Anything that can cause
>> packets to
>> + * be generated will cause deadlock!
>> + */
>> +static bool ieee80211_xmit_fast_finish(struct ieee80211_sub_if_data
>> *sdata,
>> + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct sta_info *sta, u8
>> pn_offs,
>> + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct ieee80211_key *key,
>> + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct sk_buff *skb)
>
> That should be a void function now, you never check the return value
> and only return true anyway.
Noted.
>> + struct ieee80211_tx_info *info;
>> + struct ieee80211_tx_data tx;
>> + ieee80211_tx_result r;
>> +
>
> nit: extra blank line
The horror ;) (thought I got rid of all those; ah well, will fix)
>
>> =C2=A0 spin_lock_bh(&fq->lock);
>> =C2=A0
>> =C2=A0 if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags))
>> =C2=A0 goto out;
>> =C2=A0
>> + /* Make sure fragments stay together. */
>> + skb =3D __skb_dequeue(&txqi->frags);
>> + if (skb)
>> + goto out;
>> +
>> +begin:
>
> I guess now that you introduced that anyway, we should consider making
> the skb_linearize() failure go there. Should be a follow-up patch
> though.
Can do.
>
>> + /*
>> + =C2=A0* The key can be removed while the packet was queued, so
>> need to call
>> + =C2=A0* this here to get the current key.
>> + =C2=A0*/
>> + r =3D ieee80211_tx_h_select_key(&tx);
>> + if (r !=3D TX_CONTINUE) {
>> + ieee80211_free_txskb(&local->hw, skb);
>> + goto begin;
>> + }
>> +
>> + if (txq->sta && info->control.flags & IEEE80211_TX_CTRL_FAST_XMIT) {
>
> It's a bit unfortunate that you lose fast-xmit here completely for the
> key stuff, but I don't see a good way to avoid that, other than
> completely rejiggering all the (possibly affected) queues when keys
> change... might be very complex to do that, certainly a follow-up
> patch if it's desired.
Yeah, figured it was better to have something that's correct and then go
back and change it if the performance hit turns out to be too high.
> This check seems a bit weird though - how could fast-xmit be set
> without a TXQ station?
I think that is probably just left over from before I introduced the
control flag. Should be fine to remove it.
>> +++ b/net/mac80211/util.c
>> @@ -3393,11 +3393,18 @@ void ieee80211_txq_get_depth(struct
>> ieee80211_txq *txq,
>> =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned long *byte_cnt)
>> =C2=A0{
>> =C2=A0 struct txq_info *txqi =3D to_txq_info(txq);
>> + u32 frag_cnt =3D 0, frag_bytes =3D 0;
>> + struct sk_buff *skb;
>> +
>> + skb_queue_walk(&txqi->frags, skb) {
>> + frag_cnt++;
>> + frag_bytes +=3D skb->len;
>> + }
>
> I hope this is called infrequently :)
Well, ath10k is the only user. It does get called on each wake_tx_queue,
though, so not that infrequently. My reasoning was that since the frags
queue is never going to have more than a fairly small number of packets
in it (those produced from a single split packet), counting this way is
acceptable instead of keeping a state variable up to date. Can change it
if you disagree :)
Not sure if you want a v10, or if you're satisfied with the above
comments and will just fix up the nits on merging?
-Toke
^ permalink raw reply
* Re: [PATCH 1/3] cw1200: Don't leak memory if krealloc failes
From: Johannes Thumshirn @ 2016-09-30 12:33 UTC (permalink / raw)
To: Johannes Berg
Cc: Solomon Peachy, Kalle Valo, linux-wireless, netdev, linux-kernel
In-Reply-To: <1475238579.17481.56.camel@sipsolutions.net>
On Fri, Sep 30, 2016 at 02:29:39PM +0200, Johannes Berg wrote:
>
> > + tmp = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA);
> > + if (tmp) {
> >
> I think that check is inverted?
>
> johannes
Fu.. you're right, of cause it's !tmp.
I'll resend.
Thanks,
Johannes
--
Johannes Thumshirn Storage
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply
* Re: [PATCH 1/3] cw1200: Don't leak memory if krealloc failes
From: Johannes Berg @ 2016-09-30 12:29 UTC (permalink / raw)
To: Johannes Thumshirn, Solomon Peachy, Kalle Valo
Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <1475237495-15030-1-git-send-email-jthumshirn@suse.de>
> + tmp = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA);
> + if (tmp) {
>
I think that check is inverted?
johannes
^ permalink raw reply
* [PATCH 1/3] cw1200: Don't leak memory if krealloc failes
From: Johannes Thumshirn @ 2016-09-30 12:11 UTC (permalink / raw)
To: Solomon Peachy, Kalle Valo
Cc: linux-wireless, netdev, linux-kernel, Johannes Thumshirn
The call to krealloc() in wsm_buf_reserve() directly assigns the newly
returned memory to buf->begin. This is all fine except when krealloc()
failes we loose the ability to free the old memory pointed to by
buf->begin. If we just create a temporary variable to assign memory to
and assign the memory to it we can mitigate the memory leak.
Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
drivers/net/wireless/st/cw1200/wsm.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/st/cw1200/wsm.c b/drivers/net/wireless/st/cw1200/wsm.c
index 680d60e..12fad99 100644
--- a/drivers/net/wireless/st/cw1200/wsm.c
+++ b/drivers/net/wireless/st/cw1200/wsm.c
@@ -1807,16 +1807,18 @@ static int wsm_buf_reserve(struct wsm_buf *buf, size_t extra_size)
{
size_t pos = buf->data - buf->begin;
size_t size = pos + extra_size;
+ u8 *tmp;
size = round_up(size, FWLOAD_BLOCK_SIZE);
- buf->begin = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA);
- if (buf->begin) {
- buf->data = &buf->begin[pos];
- buf->end = &buf->begin[size];
- return 0;
- } else {
- buf->end = buf->data = buf->begin;
+ tmp = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA);
+ if (tmp) {
+ wsm_buf_deinit(buf);
return -ENOMEM;
}
+
+ buf->begin = tmp;
+ buf->data = &buf->begin[pos];
+ buf->end = &buf->begin[size];
+ return 0;
}
--
1.8.5.6
^ permalink raw reply related
* Re: [PATCH 0/4] ath10k: fix mesh sync operation
From: Johannes Berg @ 2016-09-30 11:47 UTC (permalink / raw)
To: Thomas Pedersen, ath10k; +Cc: linux-wireless
In-Reply-To: <20160928235631.9197-1-twp@qca.qualcomm.com>
On Wed, 2016-09-28 at 16:56 -0700, Thomas Pedersen wrote:
> This patchset introduces a new ieee80211_op offset_tsf(), which gives
> the driver a s64 TSF offset for TSF adjustment. This is more accurate
> than programming absolute TSF since programming delay is avoided.
>
You were lucky we have patchwork, otherwise I would've missed the
mac80211 patches in this series :)
Applied both now - no reason to apply them in the order you sent them.
johannes
^ permalink raw reply
* Re: [PATCH] mac80211: fix CMD_FRAME for AP_VLAN
From: Johannes Berg @ 2016-09-30 11:41 UTC (permalink / raw)
To: Michael Braun; +Cc: linux-wireless, projekt-wlan
In-Reply-To: <1474786035-15410-1-git-send-email-michael-dev@fami-braun.de>
> net/mac80211/offchannel.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
> index 55a9c5b..2afd329 100644
> --- a/net/mac80211/offchannel.c
> +++ b/net/mac80211/offchannel.c
> @@ -819,7 +819,10 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy,
> struct wireless_dev *wdev,
> mgmt->u.action.category ==
> WLAN_CATEGORY_SPECTRUM_MGMT)
> break;
> rcu_read_lock();
> - sta = sta_info_get(sdata, mgmt->da);
> + if (ieee80211_vif_is_mesh(&sdata->vif))
> + sta = sta_info_get(sdata, mgmt->da);
> + else
> + sta = sta_info_get_bss(sdata, mgmt->da);
>
I don't see why you need to distinguish between mesh and non-mesh here?
get_bss() will ignore the BSS pointer if it's NULL, and that will
always be the case when the type is mesh, so ... why?
johannes
^ permalink raw reply
* Re: [PATCH 3/3] mac80211: Set lower memory limit for non-VHT devices
From: Johannes Berg @ 2016-09-30 11:33 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, make-wifi-fast, linux-wireless,
netdev
In-Reply-To: <20160923195911.4572-4-toke@toke.dk>
On Fri, 2016-09-23 at 21:59 +0200, Toke Høiland-Jørgensen wrote:
> Small devices can run out of memory from queueing too many packets.
> If VHT is not supported by the PHY, having more than 4 MBytes of
> total queue in the TXQ intermediate queues is not needed, and so we
> can safely limit the memory usage in these cases and avoid OOM.
I kinda see the logic here - we really don't need to queue as much if
we can't possibly transmit it out quickly - but it seems to me we
should also throw in some kind of limit that's relative to the amount
of memory you have on the system?
I've applied these anyway though. I just don't like your assumption (b)
much as the rationale for it.
johannes
^ permalink raw reply
* Re: [PATCH v3 1/9] cfg80211: add start / stop NAN commands
From: Johannes Berg @ 2016-09-30 11:25 UTC (permalink / raw)
To: Luca Coelho
Cc: linux-wireless, ayala.beker, andrei.otcheretianski,
arend.vanspriel, Emmanuel Grumbach, Luca Coelho
In-Reply-To: <20160920143121.28247-2-luca@coelho.fi>
All applied.
johannes
^ permalink raw reply
* Re: [PATCH] cfg80211: Add support for static WEP in the driver
From: Johannes Berg @ 2016-09-30 11:19 UTC (permalink / raw)
To: Luca Coelho; +Cc: linux-wireless, David Spinadel, Luca Coelho
In-Reply-To: <20160922201650.16722-1-luca@coelho.fi>
On Thu, 2016-09-22 at 23:16 +0300, Luca Coelho wrote:
> From: David Spinadel <david.spinadel@intel.com>
>
> Add support for drivers that implement static WEP internally, i.e.
> expose connection keys to the driver in connect flow and don't
> upload the keys after the connection.
>
Applied.
johannes
^ permalink raw reply
* Re: [PATCH v9 1/2] mac80211: Move ieee802111_tx_dequeue() to later in tx.c
From: Johannes Berg @ 2016-09-30 11:13 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, make-wifi-fast, linux-wireless
In-Reply-To: <20160922170420.5193-2-toke@toke.dk>
On Thu, 2016-09-22 at 19:04 +0200, Toke Høiland-Jørgensen wrote:
> The TXQ path restructure requires ieee80211_tx_dequeue() to call TX
> handlers and parts of the xmit_fast path. Move the function to later
> in tx.c in preparation for this.
>
Applied.
johannes
^ permalink raw reply
* Re: [PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue
From: Johannes Berg @ 2016-09-30 10:27 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, make-wifi-fast, linux-wireless
In-Reply-To: <20160922170420.5193-3-toke@toke.dk>
Hi Toke,
Sorry for the delay reviewing this.
I think I still have a few comments/questions.
> +static inline bool txq_has_queue(struct ieee80211_txq *txq)
> +{
> + struct txq_info *txqi = to_txq_info(txq);
> + return !(skb_queue_empty(&txqi->frags) && !txqi->tin.backlog_packets);
> +}
Tiny nit - there should probably be a blank line between the two lines
here, but I could just fix that when I apply if you don't resend anyway
for some other reason.
[snip helper stuff that looks fine]
> - if (!tx->sta->sta.txq[0])
> - hdr->seq_ctrl = ieee80211_tx_next_seq(tx->sta, tid);
> + hdr->seq_ctrl = ieee80211_tx_next_seq(tx->sta, tid);
Just to make sure I get this right - this is because the handler is now
run on dequeue, so the special case is no longer needed?
> #define CALL_TXH(txh) \
> @@ -1656,6 +1684,31 @@ static int invoke_tx_handlers(struct ieee80211_tx_data *tx)
> if (!ieee80211_hw_check(&tx->local->hw, HAS_RATE_CONTROL))
> CALL_TXH(ieee80211_tx_h_rate_ctrl);
Just for reference - the code block here that's unchanged contains
this:
CALL_TXH(ieee80211_tx_h_dynamic_ps);
CALL_TXH(ieee80211_tx_h_check_assoc);
CALL_TXH(ieee80211_tx_h_ps_buf);
CALL_TXH(ieee80211_tx_h_check_control_port_protocol);
CALL_TXH(ieee80211_tx_h_select_key);
if (!ieee80211_hw_check(&tx->local->hw, HAS_RATE_CONTROL))
CALL_TXH(ieee80211_tx_h_rate_ctrl);
> +static int invoke_tx_handlers_late(struct ieee80211_tx_data *tx)
> +{
> + struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
> + ieee80211_tx_result res = TX_CONTINUE;
> +
> if (unlikely(info->flags &
> IEEE80211_TX_INTFL_RETRANSMISSION)) {
> __skb_queue_tail(&tx->skbs, tx->skb);
> tx->skb = NULL;
And this code here is also unchanged from the original TX handler
invocation, so contains this:
if (unlikely(info->flags & IEEE80211_TX_INTFL_RETRANSMISSION)) {
__skb_queue_tail(&tx->skbs, tx->skb);
tx->skb = NULL;
goto txh_done;
}
CALL_TXH(ieee80211_tx_h_michael_mic_add);
CALL_TXH(ieee80211_tx_h_sequence);
CALL_TXH(ieee80211_tx_h_fragment);
/* handlers after fragment must be aware of tx info fragmentation! */
CALL_TXH(ieee80211_tx_h_stats);
CALL_TXH(ieee80211_tx_h_encrypt);
if (!ieee80211_hw_check(&tx->local->hw, HAS_RATE_CONTROL))
CALL_TXH(ieee80211_tx_h_calculate_duration);
But now you have a problem (that you solved) that the key pointer can
be invalidated while you have the packet queued between the two points,
and then the tx_h_michael_mic_add and/or tx_h_encrypt would crash.
You solve this by re-running tx_h_select_key() on dequeue, but it's not
clear to me why you didn't move that to the late handlers instead?
I *think* it should commute with the rate control handler, but even so,
wouldn't it make more sense to have rate control late? Assuming the
packets are queued for some amount of time, having rate control
information queued with them would get stale.
Similarly, it seems to me that checking the control port protocol later
(or perhaps duplicating that?) would be a good idea?
> +/*
> + * Can be called while the sta lock is held. Anything that can cause
> packets to
> + * be generated will cause deadlock!
> + */
> +static bool ieee80211_xmit_fast_finish(struct ieee80211_sub_if_data
> *sdata,
> + struct sta_info *sta, u8
> pn_offs,
> + struct ieee80211_key *key,
> + struct sk_buff *skb)
That should be a void function now, you never check the return value
and only return true anyway.
> + struct ieee80211_tx_info *info;
> + struct ieee80211_tx_data tx;
> + ieee80211_tx_result r;
> +
nit: extra blank line
> spin_lock_bh(&fq->lock);
>
> if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags))
> goto out;
>
> + /* Make sure fragments stay together. */
> + skb = __skb_dequeue(&txqi->frags);
> + if (skb)
> + goto out;
> +
> +begin:
I guess now that you introduced that anyway, we should consider making
the skb_linearize() failure go there. Should be a follow-up patch
though.
> + /*
> + * The key can be removed while the packet was queued, so
> need to call
> + * this here to get the current key.
> + */
> + r = ieee80211_tx_h_select_key(&tx);
> + if (r != TX_CONTINUE) {
> + ieee80211_free_txskb(&local->hw, skb);
> + goto begin;
> + }
> +
> + if (txq->sta && info->control.flags & IEEE80211_TX_CTRL_FAST_XMIT) {
It's a bit unfortunate that you lose fast-xmit here completely for the
key stuff, but I don't see a good way to avoid that, other than
completely rejiggering all the (possibly affected) queues when keys
change... might be very complex to do that, certainly a follow-up patch
if it's desired.
This check seems a bit weird though - how could fast-xmit be set
without a TXQ station?
> +++ b/net/mac80211/util.c
> @@ -3393,11 +3393,18 @@ void ieee80211_txq_get_depth(struct
> ieee80211_txq *txq,
> unsigned long *byte_cnt)
> {
> struct txq_info *txqi = to_txq_info(txq);
> + u32 frag_cnt = 0, frag_bytes = 0;
> + struct sk_buff *skb;
> +
> + skb_queue_walk(&txqi->frags, skb) {
> + frag_cnt++;
> + frag_bytes += skb->len;
> + }
I hope this is called infrequently :)
johannes
^ permalink raw reply
* Re: [PATCHv3] wireless: check A-MSDU inner frame source address on AP interfaces
From: Johannes Berg @ 2016-09-30 10:01 UTC (permalink / raw)
To: Michael Braun
Cc: kvalo, akarwar, nishants, Larry.Finger, Jes.Sorensen,
linux-wireless, projekt-wlan
In-Reply-To: <1475075672-30549-1-git-send-email-michael-dev@fami-braun.de>
A few more things:
First of all - there's nothing specific to "AP interfaces", which you
say in the subject, as far as I can tell? That should be removed?
On Wed, 2016-09-28 at 17:14 +0200, Michael Braun wrote:
> When using WPA security, the station and thus the required key is
> identified by its mac address when packets are received. So a
> station usually cannot spoof its source mac address.
>
> But when a station sends an A-MSDU frame, port control and crypto
> is done using the outer mac address, while the packets delivered
> and forwarded use the inner mac address.
>
> IEEE 802.11-2012 mandates that the outer source mac address should
> match the inner source address (section 8.3.2.2). For the
> destination mac address, matching is not required (section 10.23.15).
I think this is wrong. As we do not support DMS (yet), we should adhere
to 8.3.2.2 and only accept matching TA/SA and DA/RA.
And even when we add DMS, we should also restrict it to multicast
addresses, but that's future work anyway.
> > - if (ieee80211_data_to_8023(pkt, vif->addr, vif->type))
> > + if (ieee80211_data_to_8023(pkt, NULL, vif->addr,
> > + vif->type))
indentation is bad here - consider running checkpatch
> static int mwifiex_11n_dispatch_amsdu_pkt(struct mwifiex_private *priv,
> > - struct sk_buff *skb)
> > + struct sk_buff *skb,
> > + const u8 *ta)
[... more mwifiex changes ...]
It's great that you're doing this, but I think this patch should only
carry the bare minimum change for mwifiex to keep it compiling, with a
follow-up patch that actually implements the correct checks. That way,
should any issues arise, it's easier to determine where the problem is
and to fix/revert it.
> diff --git a/drivers/staging/rtl8723au/core/rtw_recv.c b/drivers/staging/rtl8723au/core/rtw_recv.c
> index 150dabc..38ba7dd 100644
> --- a/drivers/staging/rtl8723au/core/rtw_recv.c
> +++ b/drivers/staging/rtl8723au/core/rtw_recv.c
> @@ -1687,7 +1687,7 @@ int amsdu_to_msdu(struct rtw_adapter *padapter, struct recv_frame *prframe)
> > skb_pull(skb, prframe->attrib.hdrlen);
> > __skb_queue_head_init(&skb_list);
>
> > - ieee80211_amsdu_to_8023s(skb, &skb_list, NULL, 0, 0, false);
> > + ieee80211_amsdu_to_8023s(skb, &skb_list, NULL, 0, 0, pattrib->ta);
A bit debatable here, but I'd actually also prefer you add NULL here
first and then submit a separate patch that puts the right thing.
Not that it actually matters, since this driver has been removed
already... since you have to resubmit anyway though, I'd prefer you
just put NULL, and then not worry about it from there on.
> > - if (has_80211_header) {
> > - err = __ieee80211_data_to_8023(skb, ð, addr, iftype);
> > - if (err)
> > - goto out;
> > - }
Good approach to split that :)
> > + if (unlikely(ta &&
> > + (iftype == NL80211_IFTYPE_AP ||
> > + iftype == NL80211_IFTYPE_AP_VLAN) &&
> > + !ether_addr_equal(ta, eth.h_source)
> > + ))
> > + goto purge;
Coding style here is very odd. All those closing parens should be on
the line before.
Thanks,
johannes
^ permalink raw reply
* Re: [PATCH 3/3] mac80211: cache the only AP_VLAN station
From: Johannes Berg @ 2016-09-30 9:47 UTC (permalink / raw)
To: Michael Braun; +Cc: linux-wireless
In-Reply-To: <1474821596-12155-4-git-send-email-michael-dev@fami-braun.de>
On Sun, 2016-09-25 at 18:39 +0200, Michael Braun wrote:
> If an AP_VLAN is only used with one station, cache a pointer to this
> station to avoid lookup. This should speed up station lookup when
"should speed up"? Do you have numbers to back that up?
Read this as - you need to do more to convince me that the double
accounting and software complexity is worth it.
And then, if we really want/need to have the double accounting and all
the associated complexity and potential problems, perhaps making per-
interface lists, perhaps even replacing the global list (since we still
have a global hash table), or something similar would make more sense?
johannes
^ permalink raw reply
* Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding
From: Felix Fietkau @ 2016-09-30 8:48 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Kalle Valo, linux-wireless, devicetree
In-Reply-To: <201609300041.41876.arnd@arndb.de>
On 2016-09-30 00:41, Arnd Bergmann wrote:
> On Thursday 29 September 2016, Felix Fietkau wrote:
>> On 2016-09-08 12:54, Kalle Valo wrote:
>> > Felix Fietkau <nbd@nbd.name> writes:
>> >
>> >> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> >> ---
>> >> .../bindings/net/wireless/mediatek,mt76.txt | 26 ++++++++++++++++++++++
>> >> 1 file changed, 26 insertions(+)
>> >> create mode 100644 Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt b/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
>> >> new file mode 100644
>> >> index 0000000..d51c35f
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
>> >> @@ -0,0 +1,26 @@
>> >> +* MediaTek mt76xx devices
>> >> +
>> >> +This node provides properties for configuring the MediaTek mt76xx wireless
>> >> +device. The node is expected to be specified as a child node of the PCI
>> >> +controller to which the wireless chip is connected.
>> >> +
>> >> +Optional properties:
>> >> +
>> >> +- mac-address: See ethernet.txt in the parent directory
>> >> +- local-mac-address: See ethernet.txt in the parent directory
>> >> +- mediatek,2ghz: Override the 2.4 GHz band capability from EEPROM
>> >> +- mediatek,5ghz: Override the 5 GHz band capability from EEPROM
>> >> +- mediatek,mtd-eeprom: Specify a MTD partition + offset containing EEPROM data
>> >> +
>> >> +&pcie {
>> >> + status = "okay";
>> >> +
>> >> + pcie0 {
>> >> + mt76@0,0 {
>> >> + reg = <0x0000 0 0 0 0>;
>
> Maybe have an examplep of a real register address other than zero?
This is a real example referring to the first device on a PCI bus.
I copy&pasted this from a .dts file that we use in LEDE.
>> >> + device_type = "pci";
>> >> + mediatek,mtd-eeprom = <&factory 0x8000>;
>> >> + mediatek,2ghz = <0>;
>
> It's not clear what the possible values for the 2ghz property are,
> can you be more verbose in the description? How is <0> different
> from no property?
0 means disabled, no property means unchanged (compared to EEPROM).
- Felix
^ permalink raw reply
* Re: [PATCH] brcmfmac: implement more accurate skb tracking
From: Arend Van Spriel @ 2016-09-30 8:29 UTC (permalink / raw)
To: Rafał Miłecki
Cc: Kalle Valo, Franky Lin, Hante Meuleman, Pieter-Paul Giesberts,
Franky Lin, linux-wireless@vger.kernel.org,
open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER,
Network Development, Linux Kernel Mailing List,
Rafał Miłecki
In-Reply-To: <CACna6rxg5r=YWh7m1TYAhD5OrG3+KXAWMihs9mgh-EBVfKvt+g@mail.gmail.com>
On 29-9-2016 23:57, Rafał Miłecki wrote:
> On 27 September 2016 at 11:24, Arend Van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>> On 26-9-2016 14:38, Rafał Miłecki wrote:
>>> On 26 September 2016 at 14:13, Rafał Miłecki <zajec5@gmail.com> wrote:
>>>> On 26 September 2016 at 13:46, Arend Van Spriel
>>>> <arend.vanspriel@broadcom.com> wrote:
>>>>> On 26-9-2016 12:23, Rafał Miłecki wrote:
>>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>>
>>>>>> We need to track 802.1x packets to know if there are any pending ones
>>>>>> for transmission. This is required for performing key update in the
>>>>>> firmware.
>>>>>
>>>>> The problem we are trying to solve is a pretty old one. The problem is
>>>>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>>>>> through data path and key configuration though nl80211.
>>>>
>>>> Can I find it described/reported somewhere?
>>>>
>>>>
>>>>>> Unfortunately our old tracking code wasn't very accurate. It was
>>>>>> treating skb as pending as soon as it was passed by the netif. Actual
>>>>>> handling packet to the firmware was happening later as brcmfmac
>>>>>> internally queues them and uses its own worker(s).
>>>>>
>>>>> That does not seem right. As soon as we get a 1x packet we need to wait
>>>>> with key configuration regardless whether it is still in the driver or
>>>>> handed over to firmware already.
>>>>
>>>> OK, thanks.
>>>
>>> Actually, it's not OK. I was trying to report/describe/discuss this
>>> problem for over a week. I couldn't get much of answer from you.
>>>
>>> I had to come with a patch I worked on for quite some time. Only then
>>> you decided to react and reply with a reason for a nack. I see this
>>> patch may be wrong (but it's still hard to know what's going wrong
>>> without a proper hostapd bug report). I'd expect you to somehow work &
>>> communicate with open source community.
>>
>> We do or at least make an honest attempt, but there is more on our plate
>> so responses may be delayed. It also does not help when you get anal and
>> preachy when we do respond. Also not OK. In this case the delay is
>> caused because I had to pick up the thread(s) as Hante is on vacation
>> (he needed a break :-p ). However, you started sending patches so I
>> decided to look at and respond to those. Sorry if you felt like we left
>> you hanging to dry.
>
> I believe I get easily irritated due to my communication experience I
> got so far :(
>
>
> Over a year ago I reported brcmfmac can't recover from failed
> register_netdev(ice). This bug remains unfixed.
>
> In 2014 I reported problem with 80 MHz support. I didn't have hardware
> to fix & test it on my own (you weren't able/allowed to send me one of
> your PCIe cards). In remained broken until I fixed it year later.
>
> You missed my crash bug report about caused by missing eth_type_trans
> and came with patch on your own a month later.
>
> Earlier this year I reported you problem with BCM4366 and multiple
> interfaces. I didn't get much help. 3 months later I came with patch
> to workaround the problem but you said there's a better way to do
> this. It took me 2 weeks to figure out a new wlioctl API for that
> while all I needed was a simple hint on "interface_remove".
>
> Right now I'm waiting to get any answer from you about 4366c0
> firmware. It's still less than 2 weeks since I asked for it, but a
> simple ETA would be nice. I'm actually not sure if I should report
> more problems to you to don't distract you from pending things.
This is a difficult question. All upstream firmware releases for router
chips are put on hold until further notice. Some decisions have been
made, but I have not seen a detailed plan to give an ETA.
> Problems with brcmf_netdev_wait_pend8021x were reported multiples
> times for last few months. When I finally got time for that it took me
> a week to debug them.
For the pend8021x you were sending a number of messages showing debug
progress so not sure whether you wanted our feedback on that. If so a
ping might have done it.
> As you can see, it takes me months to get help on some things. And in
> few cases I never got much help at all. Yes, I was hoping to have you
> more involved into brcmfmac development and problems solving. I guess
> things didn't meet my expectations and I got grumpy & preachy.
Thanks for listing all our failures. Somehow we are very good at getting
each other grumpy. When we provide a patch and you break it up and
submit that to Kalle, we get grumpy and it all piles up to the point
where we have this kind of conversation. As long as it helps to get
things of our chest I can live with that. Hope you can too. We strive to
give support to the community, but the priority is low as it is not
full-time activity.
Regards,
Arend
^ permalink raw reply
* Re: [PATCH 2/3] mac80211: multicast to unicast conversion
From: Johannes Berg @ 2016-09-30 7:29 UTC (permalink / raw)
To: Michael Braun; +Cc: linux-wireless
In-Reply-To: <1474821596-12155-3-git-send-email-michael-dev@fami-braun.de>
> +static ssize_t ieee80211_if_fmt_unicast(
> + const struct ieee80211_sub_if_data *sdata, char *buf, int
> buflen)
That's a very ... unusual way to break the lines here - please either
break after the first argument, or before the function name. You should
see lots of examples for both.
However, regardless of that, I don't think that debugfs is an
appropriate way to configure a production option like this.
> + int unicast;
bool, quite obviously.
> +/* rewrite destination mac address */
> +static int ieee80211_tx_dnat(struct sk_buff *skb, struct sta_info
> *sta)
How is this DNAT?
johannes
^ permalink raw reply
* Re: ath10k stuck on 6Mbps and spam syslog
From: Matteo Grandi @ 2016-09-30 7:23 UTC (permalink / raw)
To: Ben Greear; +Cc: LinuxWireless Mailing List
In-Reply-To: <57ED2982.4020002@candelatech.com>
Dear Ben,
Thank you for your reply.
The fact that stock ath10k firmware does not report tx-rate explains
why the station dump shows 6Mbps but the iperf test reach 14.6 (even
if it is really under the expected data rate...).
About the syslog spam, this is the code of the function that seems to
cause the error (I put a comment at line 2621 side). (The warning
start to spam as soon as I assign the channel "iw dev <if_name> set
channel <ch_number>)
/**
* ieee80211_calculate_rx_timestamp - calculate timestamp in frame
* @local: mac80211 hw info struct
* @status: RX status
* @mpdu_len: total MPDU length (including FCS)
* @mpdu_offset: offset into MPDU to calculate timestamp at
*
* This function calculates the RX timestamp at the given MPDU offset, taking
* into account what the RX timestamp was. An offset of 0 will just normalize
* the timestamp to TSF at beginning of MPDU reception.
*/
u64 ieee80211_calculate_rx_timestamp(struct ieee80211_local *local,
struct ieee80211_rx_status *status,
unsigned int mpdu_len,
unsigned int mpdu_offset)
{
u64 ts = status->mactime;
struct rate_info ri;
u16 rate;
if (WARN_ON(!ieee80211_have_rx_timestamp(status))) //this is the
line 2621 reported in the warning
return 0;
memset(&ri, 0, sizeof(ri));
/* Fill cfg80211 rate info */
if (status->flag & RX_FLAG_HT) {
ri.mcs = status->rate_idx;
ri.flags |= RATE_INFO_FLAGS_MCS;
if (status->flag & RX_FLAG_40MHZ)
ri.bw = RATE_INFO_BW_40;
else
ri.bw = RATE_INFO_BW_20;
if (status->flag & RX_FLAG_SHORT_GI)
ri.flags |= RATE_INFO_FLAGS_SHORT_GI;
} else if (status->flag & RX_FLAG_VHT) {
ri.flags |= RATE_INFO_FLAGS_VHT_MCS;
ri.mcs = status->rate_idx;
ri.nss = status->vht_nss;
if (status->flag & RX_FLAG_40MHZ)
ri.bw = RATE_INFO_BW_40;
else if (status->vht_flag & RX_VHT_FLAG_80MHZ)
ri.bw = RATE_INFO_BW_80;
else if (status->vht_flag & RX_VHT_FLAG_160MHZ)
ri.bw = RATE_INFO_BW_160;
else
ri.bw = RATE_INFO_BW_20;
if (status->flag & RX_FLAG_SHORT_GI)
ri.flags |= RATE_INFO_FLAGS_SHORT_GI;
} else {
struct ieee80211_supported_band *sband;
int shift = 0;
int bitrate;
if (status->flag & RX_FLAG_10MHZ) {
shift = 1;
ri.bw = RATE_INFO_BW_10;
} else if (status->flag & RX_FLAG_5MHZ) {
shift = 2;
ri.bw = RATE_INFO_BW_5;
} else {
ri.bw = RATE_INFO_BW_20;
}
sband = local->hw.wiphy->bands[status->band];
bitrate = sband->bitrates[status->rate_idx].bitrate;
ri.legacy = DIV_ROUND_UP(bitrate, (1 << shift));
}
rate = cfg80211_calculate_bitrate(&ri);
if (WARN_ONCE(!rate,
"Invalid bitrate: flags=0x%x, idx=%d, vht_nss=%d\n",
status->flag, status->rate_idx, status->vht_nss))
return 0;
/* rewind from end of MPDU */
if (status->flag & RX_FLAG_MACTIME_END)
ts -= mpdu_len * 8 * 10 / rate;
ts += mpdu_offset * 8 * 10 / rate;
/* [SESAME] I2CAT. dbg*/
//printk(KERN_DEBUG "calculate_rx_timestamp: ts = %lu;\t rate =
%lu;\tmpdu_offset = %lu;\tmpdu_len = %lu\n",
// (long unsigned int) ts, (long unsigned int) rate, (long unsigned
int) mpdu_offset, (long unsigned int) mpdu_len );
return ts;
}
Thank you so much!
All the best
Matteo
2016-09-29 16:47 GMT+02:00 Ben Greear <greearb@candelatech.com>:
> stock ath10k firmware does not report tx-rate so the kernel always sees
> 6Mbps.
>
> I don't know about the splat..maybe post the function
> that is causing that?
>
> /home/matteo/linux-imx6/backports4.4.2-i2CAT/net/mac80211/util.c:2621
>
> Thanks,
> Ben
>
>
> On 09/29/2016 06:39 AM, Matteo Grandi wrote:
>>
>> Hello all,
>>
>> I'm struggling with a problem related on ath10k drivers:
>> I'm using a Compex WLE600V5-27 (802.11ac) miniPCIe card for some HT
>> tests needed for my thesis.
>> I'm using ath10k drivers for this card, and backports-4.4.2, in
>> particular the firmware-5.bin_10.2.4.70.54 because it seem to be the
>> more recent.
>> I've connected in mesh mode the WLE600V5 card with an 802.11n card
>> (using ath9k drivers) but looking at the station dump, the tx bitrate
>> is stuck on 6.0 Mbit/s for the ath10k. The ath9k one works well and
>> watch -n1 iw <interfacename> station dump
>> let me see changes of the tx rate and MCS on the ath9k during iperf
>> tests, but the ath10k stucks on 6.0 Mbit/s.
>>
>> Then something misterious (for me) happen while the channel
>> assignment: the syslog is spammed by this message:
>>
>> [17554.919459] ------------[ cut here ]------------
>> [17554.919839] WARNING: CPU: 0 PID: 0 at
>> /home/matteo/linux-imx6/backports4.4.2-i2CAT/net/mac80211/util.c:2621
>> ieee80211_calculate_rx_timestamp+0x204/0x278 [mac80211]()
>> [17554.919855] Modules linked in: arc4 sky2 ath10k_pci(O)
>> ath10k_core(O) ath(O) mac80211(O) cfg80211(O) compat(O)
>> [17554.919926] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W O
>> 3.14.48-g408ccb9 #4
>> [17554.919990] [<80015050>] (unwind_backtrace) from [<80011330>]
>> (show_stack+0x10/0x14)
>> [17554.920038] [<80011330>] (show_stack) from [<806537dc>]
>> (dump_stack+0x80/0x90)
>> [17554.920074] [<806537dc>] (dump_stack) from [<8002c578>]
>> (warn_slowpath_common+0x6c/0x88)
>> [17554.920103] [<8002c578>] (warn_slowpath_common) from [<8002c630>]
>> (warn_slowpath_null+0x1c/0x24)
>> [17554.920377] [<8002c630>] (warn_slowpath_null) from [<7f089c74>]
>> (ieee80211_calculate_rx_timestamp+0x204/0x278 [mac80211])
>> [17554.920826] [<7f089c74>] (ieee80211_calculate_rx_timestamp
>> [mac80211]) from [<7f07d724>] (ieee80211_rx_napi+0xcc/0x8d4
>> [mac80211])
>> [17554.921259] [<7f07d724>] (ieee80211_rx_napi [mac80211]) from
>> [<7f117fd0>] (ath10k_wmi_event_mgmt_rx+0x1f4/0x35c [ath10k_core])
>> [17554.921540] [<7f117fd0>] (ath10k_wmi_event_mgmt_rx [ath10k_core])
>> from [<7f10d844>] (ath10k_htc_rx_completion_handler+0x1cc/0x464
>> [ath10k_core])
>> [17554.921706] [<7f10d844>] (ath10k_htc_rx_completion_handler
>> [ath10k_core]) from [<7f157b4c>] (ath10k_pci_process_rx_cb+0x1ac/0x1fc
>> [ath10k_pci])
>> [17554.921773] [<7f157b4c>] (ath10k_pci_process_rx_cb [ath10k_pci])
>> from [<7f15b3ac>] (ath10k_ce_per_engine_service+0x5c/0x94
>> [ath10k_pci])
>> [17554.921835] [<7f15b3ac>] (ath10k_ce_per_engine_service
>> [ath10k_pci]) from [<7f15b464>]
>> (ath10k_ce_per_engine_service_any+0x80/0x88 [ath10k_pci])
>> [17554.921892] [<7f15b464>] (ath10k_ce_per_engine_service_any
>> [ath10k_pci]) from [<7f15a6d8>] (ath10k_pci_tasklet+0x24/0x5c
>> [ath10k_pci])
>> [17554.921946] [<7f15a6d8>] (ath10k_pci_tasklet [ath10k_pci]) from
>> [<800304c8>] (tasklet_action+0x80/0x110)
>> [17554.921979] [<800304c8>] (tasklet_action) from [<800306b8>]
>> (__do_softirq+0x10c/0x248)
>> [17554.922009] [<800306b8>] (__do_softirq) from [<80030a6c>]
>> (irq_exit+0xac/0xf4)
>> [17554.922042] [<80030a6c>] (irq_exit) from [<8000e904>]
>> (handle_IRQ+0x44/0x90)
>> [17554.922072] [<8000e904>] (handle_IRQ) from [<800084f8>]
>> (gic_handle_irq+0x2c/0x5c)
>> [17554.922105] [<800084f8>] (gic_handle_irq) from [<80011e00>]
>> (__irq_svc+0x40/0x50)
>> [17554.922122] Exception stack(0x80917f18 to 0x80917f60)
>> [17554.922141] 7f00:
>> 80917f60 000d3334
>> [17554.922166] 7f20: 5221e106 00000ff7 4d693c44 00000ff7 a7705010
>> 80924060 00000001 a7705014
>> [17554.922190] 7f40: 8096243d 80916000 00000017 80917f60 a6aaaaab
>> 80492940 60000013 ffffffff
>> [17554.922224] [<80011e00>] (__irq_svc) from [<80492940>]
>> (cpuidle_enter_state+0x50/0xe0)
>> [17554.922252] [<80492940>] (cpuidle_enter_state) from [<80492ac8>]
>> (cpuidle_idle_call+0xf8/0x148)
>> [17554.922281] [<80492ac8>] (cpuidle_idle_call) from [<8000ec48>]
>> (arch_cpu_idle+0x8/0x44)
>> [17554.922322] [<8000ec48>] (arch_cpu_idle) from [<80066648>]
>> (cpu_startup_entry+0xfc/0x140)
>> [17554.922362] [<80066648>] (cpu_startup_entry) from [<808c5b08>]
>> (start_kernel+0x360/0x36c)
>> [17554.922379] ---[ end trace 87d4775146813aed ]---
>> [17555.943454] ------------[ cut here ]------------
>>
>> that repeat continuously...
>> Forcing legacy bitrates doesn't change the situation.
>>
>> I made some measurements using iperf, please find it in attachment.
>>
>> Other info:
>>
>> root@Yazi:~# modinfo ath10k_pci
>> filename:
>>
>> /lib/modules/3.14.48-g408ccb9/kernel/drivers/net/wireless/ath/ath10k/ath10k_pci.ko
>> firmware: ath10k/QCA9377/hw1.0/board.bin
>> firmware: ath10k/QCA9377/hw1.0/firmware-5.bin
>> firmware: ath10k/QCA6174/hw3.0/board-2.bin
>> firmware: ath10k/QCA6174/hw3.0/board.bin
>> firmware: ath10k/QCA6174/hw3.0/firmware-5.bin
>> firmware: ath10k/QCA6174/hw3.0/firmware-4.bin
>> firmware: ath10k/QCA6174/hw2.1/board-2.bin
>> firmware: ath10k/QCA6174/hw2.1/board.bin
>> firmware: ath10k/QCA6174/hw2.1/firmware-5.bin
>> firmware: ath10k/QCA6174/hw2.1/firmware-4.bin
>> firmware: ath10k/QCA988X/hw2.0/board-2.bin
>> firmware: ath10k/QCA988X/hw2.0/board.bin
>> firmware: ath10k/QCA988X/hw2.0/firmware-5.bin
>> firmware: ath10k/QCA988X/hw2.0/firmware-4.bin
>> firmware: ath10k/QCA988X/hw2.0/firmware-3.bin
>> firmware: ath10k/QCA988X/hw2.0/firmware-2.bin
>> firmware: ath10k/QCA988X/hw2.0/firmware.bin
>> license: Dual BSD/GPL
>> description: Driver support for Atheros QCA988X PCIe devices
>> author: Qualcomm Atheros
>> version: backported from Linux (v4.4.2-0-g1cb8570) using
>> backports v4.4.2-1-0-gbec4037
>> srcversion: EBB3D4E36DE49B7EC8057D0
>> alias: pci:v0000168Cd00000042sv*sd*bc*sc*i*
>> alias: pci:v0000168Cd00000040sv*sd*bc*sc*i*
>> alias: pci:v0000168Cd0000003Esv*sd*bc*sc*i*
>> alias: pci:v0000168Cd00000041sv*sd*bc*sc*i*
>> alias: pci:v0000168Cd0000003Csv*sd*bc*sc*i*
>> depends: ath10k_core,compat
>> vermagic: 3.14.48-g408ccb9 SMP mod_unload modversions ARMv7 p2v8
>> parm: irq_mode:0: auto, 1: legacy, 2: msi (default: 0) (uint)
>> parm: reset_mode:0: auto, 1: warm only (default: 0) (uint)
>>
>> I don't know if it's only a problem of iw station dump that can't show
>> the tx rate, but the spammed syslog honestly warn me...
>>
>> How shall I check what's wrong and see the HT work?
>>
>> Thanks a lot!
>>
>> Matteo
>>
>
> --
> Ben Greear <greearb@candelatech.com>
> Candela Technologies Inc http://www.candelatech.com
^ 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