* Re: [PATCH v8] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.
From: Johannes Berg @ 2016-09-12 13:19 UTC (permalink / raw)
To: Toke Høiland-Jørgensen; +Cc: make-wifi-fast, linux-wireless
In-Reply-To: <87wpihe045.fsf@toke.dk>
> Well, the TXQ already adds a lot of other overhead (hashing on the
> packet header, for one), so my guess would be that this would be
> negligible compared to all that?
>
> >
> > I suppose I don't have to care all that much about the TXQs, but
> > ...
> >
> > Then again, adding a field in the skb->cb for the sake of this? No,
> > not really either.
>
> So that's a "keep it", then? :)
Yeah I think so :)
> > > + ieee80211_xmit_fast_finish(sta->sdata, sta,
> > > pn_offs,
> > > + info->control.hw_key,
> > > skb);
> >
> > I don't see how keeping the info->control.hw_key pointer across the
> > TXQ/FQ/Codel queueing isn't a potential bug? Probably one that
> > already exists in your code today, before this patch, of course.
>
> You mean the key could get removed from the hardware while the packet
> was queued? Can certainly add a check for that. Under what conditions
> does that happen? Does it make sense to try to recover from it (I
> guess by calling tx_h_select_key), or is it rare enough that giving
> up and dropping the packet makes more sense?
Not just from the hardware, more importantly the whole key structure
can be kfree()d, leading to use-after-free here, no?
Fast-xmit solves this by invalidating the fast-xmit cache when the key
pointer changes/goes away and possibly punting some frames to the slow
path, but you've absolutely no protection on these pointers here within
the TXQs, afaict?
A similar situation occurs with other pointers, like stations and vifs,
but when those are removed then obviously the entire TXQs are flushed,
so they're not relevant.
With the key though, frames can be on the queue while a key is removed,
and even before this patch, drivers would consequently access an
invalid key pointer.
Mind you, as I just wrote I think that issue exists even before this
patch, so you should probably look at it separately. Felix might know
better too.
johannes
^ permalink raw reply
* Re: [PATCH 00/26] constify local structures
From: Jarkko Sakkinen @ 2016-09-12 13:16 UTC (permalink / raw)
To: Julia Lawall
Cc: linux-renesas-soc, joe, kernel-janitors, Sergei Shtylyov,
linux-pm, platform-driver-x86, linux-media, linux-can,
Tatyana Nikolova, Shiraz Saleem, Mustafa Ismail, Chien Tin Tung,
linux-rdma, netdev, devel, alsa-devel, linux-kernel, linux-fbdev,
linux-wireless, Jason Gunthorpe, tpmdd-devel, linux-scsi,
linux-spi, linux-usb, linux-acpi
In-Reply-To: <alpine.DEB.2.10.1609121051050.3049@hadrien>
On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
>
>
> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
>
> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> > > Constify local structures.
> > >
> > > The semantic patch that makes this change is as follows:
> > > (http://coccinelle.lip6.fr/)
> >
> > Just my two cents but:
> >
> > 1. You *can* use a static analysis too to find bugs or other issues.
> > 2. However, you should manually do the commits and proper commit
> > messages to subsystems based on your findings. And I generally think
> > that if one contributes code one should also at least smoke test changes
> > somehow.
> >
> > I don't know if I'm alone with my opinion. I just think that one should
> > also do the analysis part and not blindly create and submit patches.
>
> All of the patches are compile tested. And the individual patches are
Compile-testing is not testing. If you are not able to test a commit,
you should explain why.
> submitted to the relevant maintainers. The individual commit messages
> give a more detailed explanation of the strategy used to decide that the
> structure was constifiable. It seemed redundant to put that in the cover
> letter, which will not be committed anyway.
I don't mean to be harsh but I do not care about your thought process
*that much* when I review a commit (sometimes it might make sense to
explain that but it depends on the context).
I mostly only care why a particular change makes sense for this
particular subsystem. The report given by a static analysis tool can
be a starting point for making a commit but it's not sufficient.
Based on the report you should look subsystems as individuals.
> julia
/Jarkko
^ permalink raw reply
* Re: [PATCH v8] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.
From: Toke Høiland-Jørgensen @ 2016-09-12 13:08 UTC (permalink / raw)
To: Johannes Berg; +Cc: make-wifi-fast, linux-wireless
In-Reply-To: <1473683716.29016.37.camel@sipsolutions.net>
Johannes Berg <johannes@sipsolutions.net> writes:
>> +static int invoke_tx_handlers_late(struct ieee80211_tx_data *tx);
>> +static bool ieee80211_xmit_fast_finish(struct ieee80211_sub_if_data *sd=
ata,
>> + =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_conf=
*key_conf,
>> + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct sk_buff *skb);
>> +
>
> I'm not very happy with this - I think you should do some
> refactoring/code move in a separate prior patch to avoid this.
Noted, will do.
>> + if (txq->sta && info->control.flags & IEEE80211_TX_CTRL_FAST_XMIT) {
>> =C2=A0 struct sta_info *sta =3D container_of(txq->sta, struct sta_info,
>> =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0sta);
>> - struct ieee80211_tx_info *info =3D IEEE80211_SKB_CB(skb);
>> + u8 pn_offs =3D 0;
>> =C2=A0
>> - hdr->seq_ctrl =3D ieee80211_tx_next_seq(sta, txq->tid);
>> - if (test_bit(IEEE80211_TXQ_AMPDU, &txqi->flags))
>> - info->flags |=3D IEEE80211_TX_CTL_AMPDU;
>> - else
>> - info->flags &=3D ~IEEE80211_TX_CTL_AMPDU;
>> + if (info->control.hw_key)
>> + pn_offs =3D ieee80211_hdrlen(hdr->frame_control);
>
> Not very happy with this either - the fast-xmit path explicitly tries
> to avoid all these calculations.
Well, the TXQ already adds a lot of other overhead (hashing on the
packet header, for one), so my guess would be that this would be
negligible compared to all that?=20
> I suppose I don't have to care all that much about the TXQs, but ...
>
> Then again, adding a field in the skb->cb for the sake of this? No,
> not really either.
So that's a "keep it", then? :)
>> + ieee80211_xmit_fast_finish(sta->sdata, sta, pn_offs,
>> + =C2=A0=C2=A0=C2=A0info->control.hw_key, skb);
>
> I don't see how keeping the info->control.hw_key pointer across the
> TXQ/FQ/Codel queueing isn't a potential bug? Probably one that already
> exists in your code today, before this patch, of course.
You mean the key could get removed from the hardware while the packet
was queued? Can certainly add a check for that. Under what conditions
does that happen? Does it make sense to try to recover from it (I guess
by calling tx_h_select_key), or is it rare enough that giving up and
dropping the packet makes more sense?
>> + } else {
>> + struct ieee80211_tx_data tx =3D { };
>> +
>> + __skb_queue_head_init(&tx.skbs);
>> + tx.local =3D local;
>> + tx.skb =3D skb;
>
> an empty initializer is weird - why not at least move local/skb
> initializations into it? Even txq->sta, I guess, since you can assign
> txq->sta either way.
Yup, makes sense. Noted.
>> - CALL_TXH(ieee80211_tx_h_select_key);
>> +
>> =C2=A0 if (!ieee80211_hw_check(&tx->local->hw, HAS_RATE_CONTROL))
>> =C2=A0 CALL_TXH(ieee80211_tx_h_rate_ctrl);
> [...]
>> if (unlikely(info->flags & IEEE80211_TX_INTFL_RETRANSMISSION)) {
>> =C2=A0 __skb_queue_tail(&tx->skbs, tx->skb);
>> =C2=A0 tx->skb =3D NULL;
>> =C2=A0 goto txh_done;
>> =C2=A0 }
>>=20
>> + CALL_TXH(ieee80211_tx_h_select_key);
>
> What happens for the=C2=A0IEEE80211_TX_INTFL_RETRANSMISSION packets wrt.
> key selection? Why is it OK to change this?
You're right, that's an oversight on my part. Will fix.
-Toke
^ permalink raw reply
* Re: [PATCH 0/9] rsi: code clean-up
From: Kalle Valo @ 2016-09-12 12:53 UTC (permalink / raw)
To: Prameela Rani Garnepudi
Cc: linux-wireless, johannes.berg, hofrat, prameela.garnepudi
In-Reply-To: <cover.1473669618.git.prameela.j04cs@gmail.com>
Prameela Rani Garnepudi <prameela.j04cs@gmail.com> writes:
> *** BLURB HERE ***
This has a meaning :)
> Prameela Rani Garnepudi (9):
> rsi: cleanup and added debug prints in file rsi_91x_core.c
> rsi: Spelling and code format corrections in rsi_91x_debugfs.c
> rsi: Added freeing of allocated buffers (sbands and dfsentry)
> Cleanup (spelling mistakes, code format corrections,
> empty lines after declaration)
> rsi: code clean up in file rsi_91x_main.c
> rsi: code cleanup in file rsi_91x_mgmt.c
> rsi: code cleanup (spelling mistakes and new line after declaration)
> in file rsi_91x_sdio.c Added developer name
> rsi: zone changed for debug message and cleanup in file
> rsi_91x_sdio_ops.c
> rsi: code clean up and replaced ctrl message timeouts with macros in
> file rsi_91x_usb.c
> rsi: code cleanup in file rsi_91x_usb_ops.c
Usually patches are split based on type of changes, not by files
affected. So one patch does one type of logical change to all files in
the driver and another patch does again something else. Also the commit
logs are not correctly formatted, for example the title is too long.
I recommend to start with something small, for example one small patch a
time. And once you get more familiar with the process you can submit
more, and bigger, patches at a time. Also code cleanup is not really the
best way to start as it's a very subjective matter.
Also try to get more familiar how development is done, here are few
links:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
https://www.kernel.org/doc/Documentation/SubmittingPatches
https://kernelnewbies.org/FirstKernelPatch
Also mailing list archives and git logs are a great way to learn from
how others do it.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH] mac80211: allow driver to handle packet-loss mechanism
From: Johannes Berg @ 2016-09-12 12:44 UTC (permalink / raw)
To: Rajkumar Manoharan; +Cc: linux-wireless, rmanohar
In-Reply-To: <20160906065624.4062-1-rmanohar@qti.qualcomm.com>
On Tue, 2016-09-06 at 12:26 +0530, Rajkumar Manoharan wrote:
> mac80211 keeps track of missing acks and triggers CQM packet-loss
> mechanism whenever consecutive msdu failure reaches threshold limit
> (STA_LOST_PKT_THRESHOLD). Drivers like ath10k offlaoded rate countrol
> and aggregation to firmware. Such drivers have its own connection
> monitoring algorithm that is offloaded to firmware for triggering
> station kickout due to excessive tries. In VHT mode, single PPDU can
> have
> more than 50 msdus at higher rates. Under noisy environment, single
> ppdu
> failure can cause station kickout by current mac80211 lost_packet
> mechanism
> while firmware is trying to adapt its rate table. This is causing
> frequent
> connect and disconnect iteration when station is roaming around.
>
> In such scenario, driver (or firmware) is not given enough chance to
> tune its rate control. So for devices that report low ack events, add
> a
> hardware flag to rely on their mechnism.
>
The way you describe this it sounds like somehow you'll be reporting
the indication to userspace from the driver; but you do not, and cannot
do that. The description seems thus misleading?
johannes
^ permalink raw reply
* pull-request mwifiex-firmware 2015-09-12
From: Amitkumar Karwar @ 2016-09-12 12:40 UTC (permalink / raw)
To: linux-firmware@kernel.org
Cc: linux-wireless@vger.kernel.org, Cathy Luo, Nishant Sarmukadam
The following changes since commit e92f8b3f65443764297b947b1843955d9a65dde7:
linux-firmware: update Marvell USB8797-B0 firmware image (2015-11-02 06:25:05 -0500)
are available in the git repository at:
git://git.marvell.com/mwifiex-firmware.git master
for you to fetch changes up to 1e0f28ad5a26b2c86e55dbad3141b2ec2de804f7:
linux-firmware: update WHENCE file for Marvell firmware versions (2016-09-12 17:12:42 +0530)
----------------------------------------------------------------
Amitkumar Karwar (4):
linux-firmware: update Marvell 8897-B0 firmware images to 7.p66
linux-firmware: update Marvell 8897-B0 firmware images to 7.p77
linux-firmware: add Marvell PCIe8997 firmware images
linux-firmware: update WHENCE file for Marvell firmware versions
WHENCE | 10 ++++++++--
mrvl/pcie8897_uapsta.bin | Bin 803884 -> 820212 bytes
mrvl/pcieuart8997_combo_v4.bin | Bin 0 -> 583628 bytes
mrvl/pcieusb8997_combo_v4.bin | Bin 0 -> 589048 bytes
mrvl/sd8897_uapsta.bin | Bin 780760 -> 802164 bytes
5 files changed, 8 insertions(+), 2 deletions(-)
create mode 100644 mrvl/pcieuart8997_combo_v4.bin
create mode 100644 mrvl/pcieusb8997_combo_v4.bin
^ permalink raw reply
* Re: [PATCH v8] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.
From: Johannes Berg @ 2016-09-12 12:35 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, make-wifi-fast, linux-wireless
In-Reply-To: <20160906114426.25520-1-toke@toke.dk>
> +static int invoke_tx_handlers_late(struct ieee80211_tx_data *tx);
> +static bool ieee80211_xmit_fast_finish(struct ieee80211_sub_if_data *sdata,
> + struct sta_info *sta, u8 pn_offs,
> + struct ieee80211_key_conf *key_conf,
> + struct sk_buff *skb);
> +
I'm not very happy with this - I think you should do some
refactoring/code move in a separate prior patch to avoid this.
> + if (txq->sta && info->control.flags & IEEE80211_TX_CTRL_FAST_XMIT) {
> struct sta_info *sta = container_of(txq->sta, struct sta_info,
> sta);
> - struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> + u8 pn_offs = 0;
>
> - hdr->seq_ctrl = ieee80211_tx_next_seq(sta, txq->tid);
> - if (test_bit(IEEE80211_TXQ_AMPDU, &txqi->flags))
> - info->flags |= IEEE80211_TX_CTL_AMPDU;
> - else
> - info->flags &= ~IEEE80211_TX_CTL_AMPDU;
> + if (info->control.hw_key)
> + pn_offs = ieee80211_hdrlen(hdr->frame_control);
Not very happy with this either - the fast-xmit path explicitly tries
to avoid all these calculations.
I suppose I don't have to care all that much about the TXQs, but ...
Then again, adding a field in the skb->cb for the sake of this? No, not really either.
> + ieee80211_xmit_fast_finish(sta->sdata, sta, pn_offs,
> + info->control.hw_key, skb);
I don't see how keeping the info->control.hw_key pointer across the
TXQ/FQ/Codel queueing isn't a potential bug? Probably one that already
exists in your code today, before this patch, of course.
> + } else {
> + struct ieee80211_tx_data tx = { };
> +
> + __skb_queue_head_init(&tx.skbs);
> + tx.local = local;
> + tx.skb = skb;
an empty initializer is weird - why not at least move local/skb
initializations into it? Even txq->sta, I guess, since you can assign
txq->sta either way.
> - CALL_TXH(ieee80211_tx_h_select_key);
> +
> if (!ieee80211_hw_check(&tx->local->hw, HAS_RATE_CONTROL))
> CALL_TXH(ieee80211_tx_h_rate_ctrl);
[...]
> 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_select_key);
What happens for the IEEE80211_TX_INTFL_RETRANSMISSION packets wrt. key
selection? Why is it OK to change this?
johannes
^ permalink raw reply
* Re: Support of rtl8723bs
From: Bastien Nocera @ 2016-09-12 12:23 UTC (permalink / raw)
To: Hanno Zulla, linux-wireless, Jes.Sorensen; +Cc: Larry.Finger, anthony.wong
In-Reply-To: <688574b6-1141-f408-d397-535f1f69999e@hanno.de>
On Mon, 2016-09-12 at 11:20 +0200, Hanno Zulla wrote:
> Hi Jes,
> hello linux-wireless,
>
> I'd like to know more about the current progress of rtl8723bs support
> for Linux.
>
> Googling and reading the mailing list archives found an older vendor
> driver
> https://github.com/hadess/rtl8723bs
This is the canonical location.
> plus patches at
> https://github.com/lwfinger/rtl8723bs
This is an old tree.
> which both aren't ready for Linux according to
> https://github.com/hadess/rtl8723bs/issues/96
>
> According to
> https://github.com/hadess/rtl8723bs/issues/98
>
> there is a newer vendor driver from Realtek at
> https://github.com/anthonywong/rtl8723bs
> which Anthony wants to get ready for inclusion to the Ubuntu kernel.
>
> But after reading
> https://marc.info/?l=linux-wireless&m=146902477024130&w=2
> it appears that even that newer vendor driver isn't ready for Jes's
> work.
The "hadess" driver above is an older vendor driver with 2 years of
cleanups, and lots of code removal:
430 files changed, 81387 insertions(+), 254812 deletions(-)
Anthony's repo is just a dump of a newer version of the driver.
What would be useful would be to compare the newer vendor driver with
the last time the "hadess" driver was rebased
(a2e5bb366f6d6c19dcab6db47b68a944256e8228) and pick out bug fixes.
Anyway, as an owner of such a device, I'd like to help somehow, and
be
> it as a guinea pig to test on my hardware. The newer vendor driver
> crashed on Ubuntu' 4.4 sources and doesn't compile yet on Ubuntu 4.8-
> rc
> sources.
> https://github.com/anthonywong/rtl8723bs/issues/1
I really don't care about the vendor driver. There's no way to track
changes other than through code dumps.
Longer term, it seems likely that improving Jes' driver is the way to
go. At least there's 170k less lines of code to read in the rtl8723bs
driver to get things going.
Cheers
^ permalink raw reply
* [PATCH] rsi: fix ifnullfree.cocci warnings
From: kbuild test robot @ 2016-09-12 12:05 UTC (permalink / raw)
To: Prameela Rani Garnepudi
Cc: kbuild-all, linux-wireless, kvalo, johannes.berg, hofrat,
prameela.j04cs, prameela.garnepudi
In-Reply-To: <9e848b4549d9e919eb3a772e0d292db5eee39b43.1473669618.git.prameela.j04cs@gmail.com>
drivers/net/wireless/rsi/rsi_91x_mac80211.c:209:3-8: WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values.
NULL check before some freeing functions is not needed.
Based on checkpatch warning
"kfree(NULL) is safe this check is probably not required"
and kfreeaddr.cocci by Julia Lawall.
Generated by: scripts/coccinelle/free/ifnullfree.cocci
CC: Prameela Rani Garnepudi <prameela.j04cs@gmail.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
rsi_91x_mac80211.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
--- a/drivers/net/wireless/rsi/rsi_91x_mac80211.c
+++ b/drivers/net/wireless/rsi/rsi_91x_mac80211.c
@@ -205,8 +205,7 @@ void rsi_mac80211_detach(struct rsi_hw *
for(i = 0; i < 2; i++) {
struct ieee80211_supported_band *sbands = &adapter->sbands[i];
- if (sbands->channels)
- kfree(sbands->channels);
+ kfree(sbands->channels);
}
#ifdef CONFIG_RSI_DEBUGFS
^ permalink raw reply
* Re: [PATCH 3/9] rsi: Added freeing of allocated buffers (sbands and dfsentry) Cleanup (spelling mistakes, code format corrections, empty lines after declaration)
From: kbuild test robot @ 2016-09-12 12:05 UTC (permalink / raw)
To: Prameela Rani Garnepudi
Cc: kbuild-all, linux-wireless, kvalo, johannes.berg, hofrat,
prameela.j04cs, prameela.garnepudi
In-Reply-To: <9e848b4549d9e919eb3a772e0d292db5eee39b43.1473669618.git.prameela.j04cs@gmail.com>
Hi Prameela,
[auto build test WARNING on wireless-drivers-next/master]
[also build test WARNING on v4.8-rc6 next-20160912]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]
url: https://github.com/0day-ci/linux/commits/Prameela-Rani-Garnepudi/rsi-cleanup-and-added-debug-prints-in-file-rsi_91x_core-c/20160912-184326
base: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
coccinelle warnings: (new ones prefixed by >>)
>> drivers/net/wireless/rsi/rsi_91x_mac80211.c:209:3-8: WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values.
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* Re: [PATCH 08/26] iwlegacy: constify local structures
From: Stanislaw Gruszka @ 2016-09-12 11:24 UTC (permalink / raw)
To: Julia Lawall
Cc: joe, kernel-janitors, Kalle Valo, linux-wireless, netdev,
linux-kernel
In-Reply-To: <1473599168-30561-9-git-send-email-Julia.Lawall@lip6.fr>
On Sun, Sep 11, 2016 at 03:05:50PM +0200, Julia Lawall wrote:
> For structure types defined in the same file or local header files, find
> top-level static structure declarations that have the following
> properties:
> 1. Never reassigned.
> 2. Address never taken
> 3. Not passed to a top-level macro call
> 4. No pointer or array-typed field passed to a function or stored in a
> variable.
> Declare structures having all of these properties as const.
>
> Done using Coccinelle.
> Based on a suggestion by Joe Perches <joe@perches.com>.
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
Acked-by: Stanislaw Gruszka <sgruszka@redhat.com>
^ permalink raw reply
* Re: [PATCH] mac80211: fix sequence number assignment for PS response frames
From: Toke Høiland-Jørgensen @ 2016-09-12 10:56 UTC (permalink / raw)
To: Felix Fietkau; +Cc: Johannes Berg, linux-wireless
In-Reply-To: <96ab269f-a40c-8824-1e61-fd17666ded46@nbd.name>
Felix Fietkau <nbd-Vt+b4OUoWG0@public.gmane.org> writes:
> On 2016-09-12 12:07, Johannes Berg wrote:
>> On Mon, 2016-09-12 at 12:05 +0200, Felix Fietkau wrote:
>>> On 2016-09-12 12:03, Johannes Berg wrote:
>>> >
>>> >
>>> > >
>>> > > Hm, I guess I didn't think of that. I guess this potential issue
>>> > > will
>>> > > go away once we get Toke's tx handler reorder patch fixed,
>>> > > rebased
>>> > > and integrated.
>>> > >
>>> >
>>> > I don't really see how that helps?
>>> It replaces the changes that I made.
>>>
>>
>> But this is a more general problem, no?
> Will look into it some more soon.
>
> - Felix
Well, ath9k calls ieee80211_tx_dequeue while holding the (driver) TXQ
lock. Which means that a packet going through the old TX path can block
waiting for the driver to finish pulling packets from the mac80211
queue. So that could definitely lead to reordering of sequence numbers.
And the obvious fix of taking a lock in mac80211 could then lead to
deadlock. Fun times! :)
Hmm, is there a reason why those packets *have* to go through the old TX
path? My reordering patchset introduces a queue that takes priority over
the FQ (for fragments created after dequeue). Would sticking the PS
response frame on there and having the driver pull it work?
-Toke
^ permalink raw reply
* Re: [PATCH] mac80211: make mpath path fixing more robust
From: Johannes Berg @ 2016-09-12 10:32 UTC (permalink / raw)
To: Thomas Pedersen, linux-wireless
In-Reply-To: <1473188340-13620-1-git-send-email-twp@qca.qualcomm.com>
On Tue, 2016-09-06 at 11:59 -0700, Thomas Pedersen wrote:
> A fixed mpath was not quite being treated as such:
>
Applied.
johannes
^ permalink raw reply
* Re: [PATCH v3] cfg80211: Add support to configure a beacon data rate
From: Johannes Berg @ 2016-09-12 10:26 UTC (permalink / raw)
To: Purushottam Kushwaha; +Cc: linux-wireless, jouni, usdutt, amarnath
In-Reply-To: <1472814834-2728-1-git-send-email-pkushwah@qti.qualcomm.com>
> +static u32 rateset_to_mask(struct ieee80211_supported_band *sband,
> + u8 *rates, u8 rates_len)
All of these just move around, right?
> + if (info->attrs[NL80211_ATTR_TX_RATES]) {
> + u32 rate, count_ht, count_vht, i;
> + enum nl80211_band band;
> +
> + err = nl80211_parse_tx_bitrate_mask(info,
> ¶ms.beacon_rate);
> + if (err)
> + return err;
> +
> + band = params.chandef.chan->band;
> + rate = params.beacon_rate.control[band].legacy;
> + /* Allow only one rate */
> + if (rate) {
> + if (rate & (rate - 1))
> + return -EINVAL;
> + } else {
> + count_ht = 0;
> + for (i = 0; i < IEEE80211_HT_MCS_MASK_LEN;
> i++) {
> + if
> (params.beacon_rate.control[band].ht_mcs[i]) {
> + count_ht++;
> + if (count_ht > 1)
> + return -EINVAL;
> + }
> + }
> + count_vht = 0;
> + for (i = 0; i < NL80211_VHT_NSS_MAX; i++) {
> + if
> (params.beacon_rate.control[band].vht_mcs[i]) {
> + count_vht++;
> + if (count_vht > 1)
> + return -EINVAL;
> + }
> + }
> + if (!count_ht && !count_vht)
> + return -EINVAL;
> + }
> + }
>
I think it would be good to split out this "single rate" thing into a
helper function.
Thanks,
johannes
^ permalink raw reply
* Re: [PATCH v8] cfg80211: Provision to allow the support for different beacon intervals
From: Johannes Berg @ 2016-09-12 10:24 UTC (permalink / raw)
To: Purushottam Kushwaha; +Cc: linux-wireless, jouni, usdutt, amarnath
In-Reply-To: <1472649972-6649-1-git-send-email-pkushwah@qti.qualcomm.com>
Hmm. Apart from some misleading documentation, that doesn't mention
"GCD" at all, the logic seems odd.
In order to validate a new beacon interval, you're first looking up the
min GCD value of all the combinations that allow the *current*
scenario, but doing that matching without the right # of channels or
radar detect parameters? And then you're trying to match that to the
new beacon interval?
It seems to me that the logic should be something like:
If the driver specified diff_beacon_int_gcd_min, then don't do anything
in cfg80211_validate_beacon_int(), other than perhaps a minimal range
check against the minimum of all diff_beacon_int_gcd_min values for all
combinations.
Then, add a new argument to cfg80211_check_combinations() (and
cfg80211_iter_combinations()) to have the beacon intervals for each
beaconing interface, and use that in order to match if a combination
fits, inside cfg80211_iter_combinations().
That new argument could be made the GCD of all existing beaconing
interfaces (or 0 if no such exists), since that's sufficient for
checking against a new min_gcd.
johannes
^ permalink raw reply
* Re: [PATCH] cfg80211: Add HT and VHT information in start_ap
From: Johannes Berg @ 2016-09-12 10:09 UTC (permalink / raw)
To: Malinen, Jouni; +Cc: linux-wireless@vger.kernel.org, Xu, Peng
In-Reply-To: <1472198948.390.20.camel@sipsolutions.net>
So I haven't seen a response from you guys:
I have no major objections to this. However, a few things:
1) are you planning to add support for this into a kernel driver at
all, anyway?
2) are you planning to have a driver upstream that contains the now
necessary parsing?
Depending on the answers, I suppose we could/should merge this:
no * : don't merge
yes no : merge
yes yes: don't merge, put parsing into cfg80211
I guess?
We don't have to always draw a too rigid line for non-upstream things, but it's still disappointing that you keep proposing things that are only used for non-upstream drivers.
johannes
^ permalink raw reply
* Re: [PATCH] mac80211: fix sequence number assignment for PS response frames
From: Felix Fietkau @ 2016-09-12 10:08 UTC (permalink / raw)
To: Johannes Berg, linux-wireless
In-Reply-To: <1473674822.29016.6.camel@sipsolutions.net>
On 2016-09-12 12:07, Johannes Berg wrote:
> On Mon, 2016-09-12 at 12:05 +0200, Felix Fietkau wrote:
>> On 2016-09-12 12:03, Johannes Berg wrote:
>> >
>> >
>> > >
>> > > Hm, I guess I didn't think of that. I guess this potential issue
>> > > will
>> > > go away once we get Toke's tx handler reorder patch fixed,
>> > > rebased
>> > > and integrated.
>> > >
>> >
>> > I don't really see how that helps?
>> It replaces the changes that I made.
>>
>
> But this is a more general problem, no?
Will look into it some more soon.
- Felix
^ permalink raw reply
* Re: [PATCH] mac80211: fix sequence number assignment for PS response frames
From: Johannes Berg @ 2016-09-12 10:07 UTC (permalink / raw)
To: Felix Fietkau, linux-wireless
In-Reply-To: <521f75cd-2203-efc0-77c9-d9f7b6c4079f@nbd.name>
On Mon, 2016-09-12 at 12:05 +0200, Felix Fietkau wrote:
> On 2016-09-12 12:03, Johannes Berg wrote:
> >
> >
> > >
> > > Hm, I guess I didn't think of that. I guess this potential issue
> > > will
> > > go away once we get Toke's tx handler reorder patch fixed,
> > > rebased
> > > and integrated.
> > >
> >
> > I don't really see how that helps?
> It replaces the changes that I made.
>
But this is a more general problem, no?
johannes
^ permalink raw reply
* Re: [PATCH] mac80211: fix sequence number assignment for PS response frames
From: Felix Fietkau @ 2016-09-12 10:05 UTC (permalink / raw)
To: Johannes Berg, linux-wireless
In-Reply-To: <1473674624.29016.4.camel@sipsolutions.net>
On 2016-09-12 12:03, Johannes Berg wrote:
>
>> Hm, I guess I didn't think of that. I guess this potential issue will
>> go away once we get Toke's tx handler reorder patch fixed, rebased
>> and integrated.
>>
>
> I don't really see how that helps?
It replaces the changes that I made.
- Felix
^ permalink raw reply
* Re: [PATCH] mac80211: fix sequence number assignment for PS response frames
From: Johannes Berg @ 2016-09-12 10:03 UTC (permalink / raw)
To: Felix Fietkau, linux-wireless
In-Reply-To: <afb51baf-ec2d-3600-9e5b-f406b9e836b7@nbd.name>
> Hm, I guess I didn't think of that. I guess this potential issue will
> go away once we get Toke's tx handler reorder patch fixed, rebased
> and integrated.
>
I don't really see how that helps?
johannes
^ permalink raw reply
* Re: [PATCH] mac80211: fix sequence number assignment for PS response frames
From: Felix Fietkau @ 2016-09-12 10:01 UTC (permalink / raw)
To: Johannes Berg, linux-wireless
In-Reply-To: <1473674327.29016.3.camel@sipsolutions.net>
On 2016-09-12 11:58, Johannes Berg wrote:
> On Sun, 2016-09-04 at 18:00 +0200, Felix Fietkau wrote:
>> When using intermediate queues, sequence number allocation is
>> deferred
>> until dequeue. This doesn't work for PS response frames, which bypass
>> those queues.
>>
> Applied.
>
> This worries me a bit though - there's nothing, afaict, that guarantees
> that dequeue and real TX are not concurrent, and that would corrupt a
> number of things, no?
Hm, I guess I didn't think of that. I guess this potential issue will go
away once we get Toke's tx handler reorder patch fixed, rebased and
integrated.
- Felix
^ permalink raw reply
* Re: [PATCH] mac80211: fix sequence number assignment for PS response frames
From: Johannes Berg @ 2016-09-12 9:58 UTC (permalink / raw)
To: Felix Fietkau, linux-wireless
In-Reply-To: <20160904160059.66297-1-nbd@nbd.name>
On Sun, 2016-09-04 at 18:00 +0200, Felix Fietkau wrote:
> When using intermediate queues, sequence number allocation is
> deferred
> until dequeue. This doesn't work for PS response frames, which bypass
> those queues.
>
Applied.
This worries me a bit though - there's nothing, afaict, that guarantees
that dequeue and real TX are not concurrent, and that would corrupt a
number of things, no?
johannes
^ permalink raw reply
* Re: [PATCH] mac80211: fix tim recalculation after PS response
From: Johannes Berg @ 2016-09-12 9:56 UTC (permalink / raw)
To: Felix Fietkau, linux-wireless
In-Reply-To: <20160828111037.46801-1-nbd@nbd.name>
On Sun, 2016-08-28 at 13:10 +0200, Felix Fietkau wrote:
> Handle the case where the mac80211 intermediate queues are empty and
> the driver has buffered frames
>
Applied.
johannes
^ permalink raw reply
* Re: [PATCH v2 0/6] some mac80211/cfg80211 patches
From: Johannes Berg @ 2016-09-12 9:47 UTC (permalink / raw)
To: Luca Coelho; +Cc: linux-wireless
In-Reply-To: <1472502319-10847-1-git-send-email-luca@coelho.fi>
Applied all now. It seems I got held up somewhere in the middle before.
johannes
^ permalink raw reply
* [PATCH 9/9] rsi: code cleanup in file rsi_91x_usb_ops.c
From: Prameela Rani Garnepudi @ 2016-09-12 9:38 UTC (permalink / raw)
To: linux-wireless, kvalo, johannes.berg, hofrat
Cc: prameela.j04cs, prameela.garnepudi
In-Reply-To: <cover.1473669618.git.prameela.j04cs@gmail.com>
Signed-off-by: Prameela Rani Garnepudi <prameela.j04cs@gmail.com>
---
drivers/net/wireless/rsi/rsi_91x_usb_ops.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/rsi/rsi_91x_usb_ops.c b/drivers/net/wireless/rsi/rsi_91x_usb_ops.c
index de49008..5ab2ce3 100644
--- a/drivers/net/wireless/rsi/rsi_91x_usb_ops.c
+++ b/drivers/net/wireless/rsi/rsi_91x_usb_ops.c
@@ -96,26 +96,25 @@ void rsi_usb_rx_thread(struct rsi_common *common)
rsi_wait_event(&dev->rx_thread.event, EVENT_WAIT_FOREVER);
if (atomic_read(&dev->rx_thread.thread_done))
- goto out;
+ break;
mutex_lock(&common->tx_rxlock);
status = rsi_read_pkt(common, 0);
if (status) {
rsi_dbg(ERR_ZONE, "%s: Failed To read data", __func__);
mutex_unlock(&common->tx_rxlock);
- return;
+ break;
}
mutex_unlock(&common->tx_rxlock);
rsi_reset_event(&dev->rx_thread.event);
if (adapter->rx_urb_submit(adapter)) {
rsi_dbg(ERR_ZONE,
"%s: Failed in urb submission", __func__);
- return;
+ break;
}
} while (1);
-out:
- rsi_dbg(INFO_ZONE, "%s: Terminated thread\n", __func__);
+ rsi_dbg(INFO_ZONE, "%s: Terminated USB RX thread\n", __func__);
complete_and_exit(&dev->rx_thread.completion, 0);
}
--
2.4.11
^ permalink raw reply related
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