* Re: [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.
From: Toke Høiland-Jørgensen @ 2016-10-05 19:56 UTC (permalink / raw)
To: Kalle Valo
Cc: linux-wireless, make-wifi-fast, ath9k-devel, Tim Shepard,
Felix Fietkau
In-Reply-To: <874m4qof58.fsf@kamboji.qca.qualcomm.com>
Kalle Valo <kvalo@codeaurora.org> writes:
> Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk> writes:
>
>> Kalle Valo <kvalo@codeaurora.org> writes:
>>
>>> Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk> writes:
>>>
>>>> Kalle Valo <kvalo@codeaurora.org> writes:
>>>>
>>>>> Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk> writes:
>>>>>
>>>>> I understand your point, but I don't want to rush this to 4.9 and then
>>>>> start getting lots of bug reports and eventually forced to revert it.=
If
>>>>> we just found a new serious regression the chances are that there are
>>>>> more lurking somewhere and this patch is just not ready yet.
>>>>
>>>> So, the changes to mac80211 that fixes the known regressions of this
>>>> patch have gone in.
>>>
>>> I guess you mean this commit:
>>>
>>> bb42f2d13ffc mac80211: Move reorder-sensitive TX handlers to after TXQ =
dequeue
>>>
>>> (Just making sure that I have the same commit in my tree when I apply
>>> this)
>>
>> Yup, that's the one :)
>>
>>>> Any chance of seeing this merged during the current merge window? :)
>>>
>>> I sent last new feature ("-next") patches for 4.9 last week, sorry. So
>>> this has to wait for 4.10.
>>
>> Ah, right, I think I got my merge windows confused. You already said you
>> wouldn't take it for 4.9. So I guess what I'm asking is for you to put
>> it into the appropriate -next tree so it can get some wider exposure
>> ahead of the *next* merge window...
>
> Yeah, we have plenty of time for 4.10 :) So my plan is to apply this
> after I open wireless-drivers-next in 2-3 weeks or so. That would mean
> that the patch would hit Linus' tree when 4.10-rc1 is released
> (estimated to happen on 2017-01-01). The timing is actually perfect as
> now we get maximal testing time on -next.
So the -next trees are those that are open outside the merge window.
Right, got it; thanks :)
>>> And I assume I need to take v5:
>>>
>>> https://patchwork.kernel.org/patch/9311037/
>>
>> Yes. Haven't noticed anything that changed since that might conflict
>> with it, but let me know if I missed something and you want a refreshed
>> version.
>
> Thanks, I'll let you know if there are any problems.
Cool.
-Toke
^ permalink raw reply
* Re: [PATCH RESEND v3] mwifiex: parse device tree node for PCIe
From: Brian Norris @ 2016-10-05 19:48 UTC (permalink / raw)
To: Amitkumar Karwar; +Cc: linux-wireless, devicetree, rajatja, Xinming Hu
In-Reply-To: <1475167183-18376-1-git-send-email-akarwar@marvell.com>
Hi,
On Thu, Sep 29, 2016 at 10:09:43PM +0530, Amitkumar Karwar wrote:
> From: Xinming Hu <huxm@marvell.com>
>
> This patch derives device tree node from pcie bus layer framework.
> Device tree bindings file has been renamed(marvell-sd8xxx.txt ->
> marvell-8xxx.txt) to accomodate PCIe changes.
>
> Signed-off-by: Xinming Hu <huxm@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> ---
> v2: Included vendor and product IDs in compatible strings for PCIe
> chipsets(Rob Herring)
> v3: Patch is created using -M option so that it will only include diff of
> original and renamed files(Rob Herring)
> Resend v3: Resending the patch because I missed to include device tree mailing
> while sending v3.
> ---
Why is this patch so different than the SDIO DT handling for the same
driver?
> .../{marvell-sd8xxx.txt => marvell-8xxx.txt} | 8 +++++---
> drivers/net/wireless/marvell/mwifiex/pcie.c | 20 ++++++++++++++++++++
> drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 3 ++-
> 3 files changed, 27 insertions(+), 4 deletions(-)
> rename Documentation/devicetree/bindings/net/wireless/{marvell-sd8xxx.txt => marvell-8xxx.txt} (91%)
>
> diff --git a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt b/Documentation/devicetree/bindings/net/wireless/marvell-8xxx.txt
> similarity index 91%
> rename from Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
> rename to Documentation/devicetree/bindings/net/wireless/marvell-8xxx.txt
> index c421aba..dfe5f8e 100644
> --- a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
> +++ b/Documentation/devicetree/bindings/net/wireless/marvell-8xxx.txt
> @@ -1,8 +1,8 @@
> -Marvell 8897/8997 (sd8897/sd8997) SDIO devices
> +Marvell 8897/8997 (sd8897/sd8997/pcie8997) SDIO/PCIE devices
> ------
>
> -This node provides properties for controlling the marvell sdio wireless device.
> -The node is expected to be specified as a child node to the SDIO controller that
> +This node provides properties for controlling the marvell sdio/pcie wireless device.
> +The node is expected to be specified as a child node to the SDIO/PCIE controller that
> connects the device to the system.
>
> Required properties:
> @@ -10,6 +10,8 @@ Required properties:
> - compatible : should be one of the following:
> * "marvell,sd8897"
> * "marvell,sd8997"
> + * "pci11ab,2b42"
> + * "pci1b4b,2b42"
>
> Optional properties:
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 063c707..f1eeb73 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -37,6 +37,23 @@ static struct mwifiex_if_ops pcie_ops;
>
> static struct semaphore add_remove_card_sem;
>
> +static const struct of_device_id mwifiex_pcie_of_match_table[] = {
> + { .compatible = "pci11ab,2b42" },
> + { .compatible = "pci1b4b,2b42" },
> + { }
> +};
> +
> +static int mwifiex_pcie_probe_of(struct device *dev)
> +{
> + if (!dev->of_node ||
In sdio.c, this check is done silently; that way, we don't error out
(and print an error string) just because there's no DT node. And I think
that makes sense.
Maybe pull this condition out separately and return 0?
> + !of_match_node(mwifiex_pcie_of_match_table, dev->of_node)) {
> + pr_err("pcie device node not available");
> + return -1;
> + }
> +
Just an FYI, the equivalent sdio.c code (mwifiex_sdio_probe_of()) also
parses a platform-specific wakeup pin. Rajat and I were considering
moving that to mwifiex_register(), so we can get the same handling in
pcie.c. But I think it's probably sane to leave the compatible table
here in the interface driver for now.
> + return 0;
> +}
> +
> static int
> mwifiex_map_pci_memory(struct mwifiex_adapter *adapter, struct sk_buff *skb,
> size_t size, int flags)
> @@ -206,6 +223,9 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
> card->pcie.can_ext_scan = data->can_ext_scan;
> }
>
> + /* device tree node parsing and platform specific configuration*/
> + mwifiex_pcie_probe_of(&pdev->dev);
You aren't catching the error code here. Probably because you're wrongly
returning an error above for the !of_node case.
Brian
> +
> if (mwifiex_add_card(card, &add_remove_card_sem, &pcie_ops,
> MWIFIEX_PCIE)) {
> pr_err("%s failed\n", __func__);
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
> index 638d30a..f2a7a13 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
> @@ -2218,7 +2218,8 @@ int mwifiex_sta_init_cmd(struct mwifiex_private *priv, u8 first_sta, bool init)
> * The cal-data can be read from device tree and/or
> * a configuration file and downloaded to firmware.
> */
> - if (priv->adapter->iface_type == MWIFIEX_SDIO &&
> + if ((priv->adapter->iface_type == MWIFIEX_SDIO ||
> + priv->adapter->iface_type == MWIFIEX_PCIE) &&
> adapter->dev->of_node) {
> adapter->dt_node = adapter->dev->of_node;
> if (of_property_read_u32(adapter->dt_node,
^ permalink raw reply
* Re: ath10k: device stops working ("failed to install key for vdev" error in kernel log)
From: Ben Greear @ 2016-10-05 19:11 UTC (permalink / raw)
To: Martin Blumenstingl; +Cc: ath10k, linux-wireless
In-Reply-To: <CAFBinCAv9PDYQ7J+0LtQg7vKnqp2HVpotqwjALrAUQf57YM4tg@mail.gmail.com>
On 10/05/2016 12:06 PM, Martin Blumenstingl wrote:
> On Wed, Oct 5, 2016 at 8:58 PM, Ben Greear <greearb@candelatech.com> wrote:
>> On 10/05/2016 11:51 AM, Martin Blumenstingl wrote:
>>>>
>>>> [54064.293597] ath10k_pci 0000:02:00.0: failed to install key for vdev
>>>> 0 peer [AP MAC addr]: -145
>>>> [54064.301234] wlan0: failed to remove key (1, ff:ff:ff:ff:ff:ff) from
>>>> hardware (-145)
>>>> [54067.305703] ath10k_pci 0000:02:00.0: failed to install key for vdev
>>>> 0 peer [AP MAC addr]: -145
>>>> [54067.313307] wlan0: failed to set key (1, ff:ff:ff:ff:ff:ff) to
>>>> hardware (-145)
>>>
>>> it just happened again:
>>> ...
>>> [130266.948005] ath10k_pci 0000:02:00.0: failed to install key for
>>> vdev 0 peer [AP MAC address]: -145
>>> [130266.955697] wlan0: failed to remove key (2, ff:ff:ff:ff:ff:ff)
>>> from hardware (-145)
>>> [130269.964069] ath10k_pci 0000:02:00.0: failed to install key for
>>> vdev 0 peer [AP MAC address]: -145
>>> [130269.971775] wlan0: failed to set key (2, ff:ff:ff:ff:ff:ff) to
>>> hardware (-145)
>>> [172198.889700] ath10k_pci 0000:02:00.0: failed to send pdev bss chan
>>> info request
>>> [172201.897770] ath10k_pci 0000:02:00.0: failed to send pdev bss chan
>>> info request
>>>
>>> I tried to get more information from the firmware by looking at the
>>> fw_* debugfs files:
>>> # cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_reset_stats
>>> fw_crash_counter 0
>>> fw_warm_reset_counter 4
>>> fw_cold_reset_counter 0
>>> # cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_stats
>>> cat: can't open '/sys/kernel/debug/ieee80211/phy0/ath10k/fw_stats':
>>> Resource temporarily unavailable
>>> # cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_crash_dump
>>> cat: can't open
>>> '/sys/kernel/debug/ieee80211/phy0/ath10k/fw_crash_dump': No data
>>> available
>>> # cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_dbglog
>>> 0x00000000 0
>>> # cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_checksums
>>> firmware-N.bin 9d340dd9
>>> athwlan 8d25deed
>>> otp f3efeb4f
>>> codeswap 00000000
>>> board-N.bin bebc7c08
>>> board bebc7c08
>>>
>>> This is still with firmware 10.2.4.70.54.
>>> Please let me know if you need further information.
>>
>>
>> Not sure about your firmware exactly, but the timeout might happen because
>> firmware has leaked and/or run-out of resources, fails to insert the key,
>> and then it just doesn't respond instead of sending an event. So, driver
>> gets the timeout message and who knows what state your system is in.
>>
>> I hit this when doing capacity tests, and I modified my firmware to always
>> send an event, and driver to deal with it. I also fixed some resource leaks
>> and tuned firmware objects to make sure I do not hit the key exhaustion
>> state.
> That sounds bad.
> Especially as I would not describe my current setup as "high capacity" network.
> The worst-case I have is 5 devices:
> - Nexus 5
> - Sony Xperia Z3 Compact
> - Notebook with Intel AC 7260
> - QCA9880-2R4E in station (client) mode
> - BCM4330 based device
>
>> What is your test scenario in this case?
> with this specific crash it was pretty easy:
> - AP did not have any connections while I was at work
> - when I came back home two devices (Nexus 5 and Sony Xperia Z3
> Compact) tried to connect to the AP
> - device went into error state
>
> I already had days where only one phone was turned on and I was still
> able to reproduce it.
Firmware sometimes sends dbglog messages when it detects errors. You can hack your driver with patches
I have posted to enable printing this out, and then maybe QCA firmware guys could debug this issue
for you.
Or, if you can reproduce this with my firmware and kernel (which enables the dbglog print
by default), email dmesg or similar output to me and maybe I will have a clue as to what
is going on.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: ath10k: device stops working ("failed to install key for vdev" error in kernel log)
From: Martin Blumenstingl @ 2016-10-05 19:06 UTC (permalink / raw)
To: Ben Greear; +Cc: ath10k, linux-wireless
In-Reply-To: <bdd2b4f1-114b-f114-c908-fc0f57b08ce8@candelatech.com>
On Wed, Oct 5, 2016 at 8:58 PM, Ben Greear <greearb@candelatech.com> wrote:
> On 10/05/2016 11:51 AM, Martin Blumenstingl wrote:
>>>
>>> [54064.293597] ath10k_pci 0000:02:00.0: failed to install key for vdev
>>> 0 peer [AP MAC addr]: -145
>>> [54064.301234] wlan0: failed to remove key (1, ff:ff:ff:ff:ff:ff) from
>>> hardware (-145)
>>> [54067.305703] ath10k_pci 0000:02:00.0: failed to install key for vdev
>>> 0 peer [AP MAC addr]: -145
>>> [54067.313307] wlan0: failed to set key (1, ff:ff:ff:ff:ff:ff) to
>>> hardware (-145)
>>
>> it just happened again:
>> ...
>> [130266.948005] ath10k_pci 0000:02:00.0: failed to install key for
>> vdev 0 peer [AP MAC address]: -145
>> [130266.955697] wlan0: failed to remove key (2, ff:ff:ff:ff:ff:ff)
>> from hardware (-145)
>> [130269.964069] ath10k_pci 0000:02:00.0: failed to install key for
>> vdev 0 peer [AP MAC address]: -145
>> [130269.971775] wlan0: failed to set key (2, ff:ff:ff:ff:ff:ff) to
>> hardware (-145)
>> [172198.889700] ath10k_pci 0000:02:00.0: failed to send pdev bss chan
>> info request
>> [172201.897770] ath10k_pci 0000:02:00.0: failed to send pdev bss chan
>> info request
>>
>> I tried to get more information from the firmware by looking at the
>> fw_* debugfs files:
>> # cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_reset_stats
>> fw_crash_counter 0
>> fw_warm_reset_counter 4
>> fw_cold_reset_counter 0
>> # cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_stats
>> cat: can't open '/sys/kernel/debug/ieee80211/phy0/ath10k/fw_stats':
>> Resource temporarily unavailable
>> # cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_crash_dump
>> cat: can't open
>> '/sys/kernel/debug/ieee80211/phy0/ath10k/fw_crash_dump': No data
>> available
>> # cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_dbglog
>> 0x00000000 0
>> # cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_checksums
>> firmware-N.bin 9d340dd9
>> athwlan 8d25deed
>> otp f3efeb4f
>> codeswap 00000000
>> board-N.bin bebc7c08
>> board bebc7c08
>>
>> This is still with firmware 10.2.4.70.54.
>> Please let me know if you need further information.
>
>
> Not sure about your firmware exactly, but the timeout might happen because
> firmware has leaked and/or run-out of resources, fails to insert the key,
> and then it just doesn't respond instead of sending an event. So, driver
> gets the timeout message and who knows what state your system is in.
>
> I hit this when doing capacity tests, and I modified my firmware to always
> send an event, and driver to deal with it. I also fixed some resource leaks
> and tuned firmware objects to make sure I do not hit the key exhaustion
> state.
That sounds bad.
Especially as I would not describe my current setup as "high capacity" network.
The worst-case I have is 5 devices:
- Nexus 5
- Sony Xperia Z3 Compact
- Notebook with Intel AC 7260
- QCA9880-2R4E in station (client) mode
- BCM4330 based device
> What is your test scenario in this case?
with this specific crash it was pretty easy:
- AP did not have any connections while I was at work
- when I came back home two devices (Nexus 5 and Sony Xperia Z3
Compact) tried to connect to the AP
- device went into error state
I already had days where only one phone was turned on and I was still
able to reproduce it.
Regards,
Martin
^ permalink raw reply
* Re: ath10k: device stops working ("failed to install key for vdev" error in kernel log)
From: Ben Greear @ 2016-10-05 18:58 UTC (permalink / raw)
To: Martin Blumenstingl, ath10k, linux-wireless
In-Reply-To: <CAFBinCAuCfTeWw3SMVLbeARkUs_66YT6MJVbsd9QH_CqW9SCtg@mail.gmail.com>
On 10/05/2016 11:51 AM, Martin Blumenstingl wrote:
>> [54064.293597] ath10k_pci 0000:02:00.0: failed to install key for vdev
>> 0 peer [AP MAC addr]: -145
>> [54064.301234] wlan0: failed to remove key (1, ff:ff:ff:ff:ff:ff) from
>> hardware (-145)
>> [54067.305703] ath10k_pci 0000:02:00.0: failed to install key for vdev
>> 0 peer [AP MAC addr]: -145
>> [54067.313307] wlan0: failed to set key (1, ff:ff:ff:ff:ff:ff) to
>> hardware (-145)
> it just happened again:
> ...
> [130266.948005] ath10k_pci 0000:02:00.0: failed to install key for
> vdev 0 peer [AP MAC address]: -145
> [130266.955697] wlan0: failed to remove key (2, ff:ff:ff:ff:ff:ff)
> from hardware (-145)
> [130269.964069] ath10k_pci 0000:02:00.0: failed to install key for
> vdev 0 peer [AP MAC address]: -145
> [130269.971775] wlan0: failed to set key (2, ff:ff:ff:ff:ff:ff) to
> hardware (-145)
> [172198.889700] ath10k_pci 0000:02:00.0: failed to send pdev bss chan
> info request
> [172201.897770] ath10k_pci 0000:02:00.0: failed to send pdev bss chan
> info request
>
> I tried to get more information from the firmware by looking at the
> fw_* debugfs files:
> # cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_reset_stats
> fw_crash_counter 0
> fw_warm_reset_counter 4
> fw_cold_reset_counter 0
> # cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_stats
> cat: can't open '/sys/kernel/debug/ieee80211/phy0/ath10k/fw_stats':
> Resource temporarily unavailable
> # cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_crash_dump
> cat: can't open
> '/sys/kernel/debug/ieee80211/phy0/ath10k/fw_crash_dump': No data
> available
> # cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_dbglog
> 0x00000000 0
> # cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_checksums
> firmware-N.bin 9d340dd9
> athwlan 8d25deed
> otp f3efeb4f
> codeswap 00000000
> board-N.bin bebc7c08
> board bebc7c08
>
> This is still with firmware 10.2.4.70.54.
> Please let me know if you need further information.
Not sure about your firmware exactly, but the timeout might happen because
firmware has leaked and/or run-out of resources, fails to insert the key,
and then it just doesn't respond instead of sending an event. So, driver
gets the timeout message and who knows what state your system is in.
I hit this when doing capacity tests, and I modified my firmware to always
send an event, and driver to deal with it. I also fixed some resource leaks
and tuned firmware objects to make sure I do not hit the key exhaustion state.
What is your test scenario in this case?
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: ath10k: device stops working ("failed to install key for vdev" error in kernel log)
From: Martin Blumenstingl @ 2016-10-05 18:51 UTC (permalink / raw)
To: ath10k, linux-wireless
> [54064.293597] ath10k_pci 0000:02:00.0: failed to install key for vdev
> 0 peer [AP MAC addr]: -145
> [54064.301234] wlan0: failed to remove key (1, ff:ff:ff:ff:ff:ff) from
> hardware (-145)
> [54067.305703] ath10k_pci 0000:02:00.0: failed to install key for vdev
> 0 peer [AP MAC addr]: -145
> [54067.313307] wlan0: failed to set key (1, ff:ff:ff:ff:ff:ff) to
> hardware (-145)
it just happened again:
...
[130266.948005] ath10k_pci 0000:02:00.0: failed to install key for
vdev 0 peer [AP MAC address]: -145
[130266.955697] wlan0: failed to remove key (2, ff:ff:ff:ff:ff:ff)
from hardware (-145)
[130269.964069] ath10k_pci 0000:02:00.0: failed to install key for
vdev 0 peer [AP MAC address]: -145
[130269.971775] wlan0: failed to set key (2, ff:ff:ff:ff:ff:ff) to
hardware (-145)
[172198.889700] ath10k_pci 0000:02:00.0: failed to send pdev bss chan
info request
[172201.897770] ath10k_pci 0000:02:00.0: failed to send pdev bss chan
info request
I tried to get more information from the firmware by looking at the
fw_* debugfs files:
# cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_reset_stats
fw_crash_counter 0
fw_warm_reset_counter 4
fw_cold_reset_counter 0
# cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_stats
cat: can't open '/sys/kernel/debug/ieee80211/phy0/ath10k/fw_stats':
Resource temporarily unavailable
# cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_crash_dump
cat: can't open
'/sys/kernel/debug/ieee80211/phy0/ath10k/fw_crash_dump': No data
available
# cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_dbglog
0x00000000 0
# cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_checksums
firmware-N.bin 9d340dd9
athwlan 8d25deed
otp f3efeb4f
codeswap 00000000
board-N.bin bebc7c08
board bebc7c08
This is still with firmware 10.2.4.70.54.
Please let me know if you need further information.
Regards,
Martin
^ permalink raw reply
* Re: [RFC 0/3] of: add common bindings to (de)activate IEEE 802.11 bands
From: Felix Fietkau @ 2016-10-05 18:36 UTC (permalink / raw)
To: Martin Blumenstingl, Rob Herring
Cc: Mark Rutland, Frank Rowand, devicetree@vger.kernel.org,
linux-wireless, ath9k-devel, ath9k-devel, Kalle Valo
In-Reply-To: <CAFBinCCDjZs6szRw0D1rwB89tUo+-RNFH2_g=K+Xj2iRJuUZDQ@mail.gmail.com>
On 2016-10-05 20:25, Martin Blumenstingl wrote:
> On Mon, Oct 3, 2016 at 5:22 PM, Rob Herring <robh+dt@kernel.org> wrote:
>> On Sun, Oct 2, 2016 at 5:50 PM, Martin Blumenstingl
>> <martin.blumenstingl@googlemail.com> wrote:
>>> There are at least two drivers (ath9k and mt76) out there, where
>>> devicetree authors need to override the enabled bands.
>>>
>>> For ath9k there is only one use-case: disabling a band which has been
>>> incorrectly enabled by the vendor in the EEPROM (enabling a band is not
>>> possible because the calibration data would be missing in the EEPROM).
>>> The mt76 driver (currently pending for review) however allows enabling
>>> and disabling the 2.4GHz and 5GHz band, see [0].
>>>
>>> Based on the discussion of (earlier versions of) my ath9k devicetree
>>> patch it was suggested [1] that we use enable- and disable- properties.
>>> The current patch implements:
>>> - bands can be enabled or disabled with the corresponding property
>>> - if both (disable and enable) properties are given and a driver asks
>>> whether the band is enabled then the logic will return false (= not
>>> enabled, preferring the disabled flag)
>>> - if both (disable and enable) properties are given and a driver asks
>>> whether the band is disabled then the logic will return true (again,
>>> preferring the disabled flag over the enabled flag)
>>>
>>> We can still change the logic to do what the mt76 driver does (I am
>>> fine with both solutions):
>>> - property not available: driver decides which bands to enable
>>> - property set to 0: disable the band
>>> - property set to 1: enable the band
>>
>> I prefer this way. Really, I'd prefer just a boolean disable property.
>> I'm not sure why you need the enable. We usually have these tri-state
>> properties when you want not present to mean use the bootloader or
>> default setting. Try to make not present the most common case.
> Felix: could you please give a few details why mt76 can not only
> disable but also enable a specific band?
> There is no specific comment in the sources [0] why this is needed.
> All other drivers that I am aware of (ath9k, rt2x00) only allow
> disabling a specific band, they never enable it (this has to be done
> explicitly by the EEPROM).
None of the devices use it at the moment, I probably added it when I
played with a device that had broken EEPROM data. I guess I decided to
do it this way because on many devices the band capability field simply
cannot be trusted.
I guess I would be okay with just having a disable property and adding
an enable property later only if we end up actually needing it.
- Felix
^ permalink raw reply
* Re: [RFC 0/3] of: add common bindings to (de)activate IEEE 802.11 bands
From: Martin Blumenstingl @ 2016-10-05 18:25 UTC (permalink / raw)
To: Rob Herring, Felix Fietkau
Cc: Mark Rutland, Frank Rowand, devicetree@vger.kernel.org,
linux-wireless, ath9k-devel, ath9k-devel, Kalle Valo
In-Reply-To: <CAL_Jsq+cie=enhUYBF-OSrMdQ5S_048JF31ghEjtBNAmkGbB9Q@mail.gmail.com>
On Mon, Oct 3, 2016 at 5:22 PM, Rob Herring <robh+dt@kernel.org> wrote:
> On Sun, Oct 2, 2016 at 5:50 PM, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>> There are at least two drivers (ath9k and mt76) out there, where
>> devicetree authors need to override the enabled bands.
>>
>> For ath9k there is only one use-case: disabling a band which has been
>> incorrectly enabled by the vendor in the EEPROM (enabling a band is not
>> possible because the calibration data would be missing in the EEPROM).
>> The mt76 driver (currently pending for review) however allows enabling
>> and disabling the 2.4GHz and 5GHz band, see [0].
>>
>> Based on the discussion of (earlier versions of) my ath9k devicetree
>> patch it was suggested [1] that we use enable- and disable- properties.
>> The current patch implements:
>> - bands can be enabled or disabled with the corresponding property
>> - if both (disable and enable) properties are given and a driver asks
>> whether the band is enabled then the logic will return false (= not
>> enabled, preferring the disabled flag)
>> - if both (disable and enable) properties are given and a driver asks
>> whether the band is disabled then the logic will return true (again,
>> preferring the disabled flag over the enabled flag)
>>
>> We can still change the logic to do what the mt76 driver does (I am
>> fine with both solutions):
>> - property not available: driver decides which bands to enable
>> - property set to 0: disable the band
>> - property set to 1: enable the band
>
> I prefer this way. Really, I'd prefer just a boolean disable property.
> I'm not sure why you need the enable. We usually have these tri-state
> properties when you want not present to mean use the bootloader or
> default setting. Try to make not present the most common case.
Felix: could you please give a few details why mt76 can not only
disable but also enable a specific band?
There is no specific comment in the sources [0] why this is needed.
All other drivers that I am aware of (ath9k, rt2x00) only allow
disabling a specific band, they never enable it (this has to be done
explicitly by the EEPROM).
>> The new code has been integrated into ath9k to demonstrate how to use
>> it (with the benefit that the disable_2ghz and disable_5ghz settings
>> from the ath9k_platform_data can now also be configured via .dts).
>>
>> open questions/decisions needed:
>> - where to place this new code? I put it into drivers/of/of_ieee80211
>> because that's where most other functions live.
>> However, I found that this makes backporting the code harder (using
>> wireless-backports from the driver-backports project [2])
>
> We are generally moving subsystem specific code like this out of
> drivers/of/, so please do that here. Maybe someone will get motivated
> to move the other networking code out too.
OK, thanks for the hint.
[0] https://github.com/openwrt/mt76/blob/master/eeprom.c
^ permalink raw reply
* Re: [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.
From: Kalle Valo @ 2016-10-05 17:54 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: linux-wireless, make-wifi-fast, ath9k-devel, Tim Shepard,
Felix Fietkau
In-Reply-To: <87d1jeenwa.fsf@toke.dk>
Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk> writes:
> Kalle Valo <kvalo@codeaurora.org> writes:
>
>> Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk> writes:
>>
>>> Kalle Valo <kvalo@codeaurora.org> writes:
>>>
>>>> Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk> writes:
>>>>
>>>> I understand your point, but I don't want to rush this to 4.9 and then
>>>> start getting lots of bug reports and eventually forced to revert it. =
If
>>>> we just found a new serious regression the chances are that there are
>>>> more lurking somewhere and this patch is just not ready yet.
>>>
>>> So, the changes to mac80211 that fixes the known regressions of this
>>> patch have gone in.
>>
>> I guess you mean this commit:
>>
>> bb42f2d13ffc mac80211: Move reorder-sensitive TX handlers to after TXQ d=
equeue
>>
>> (Just making sure that I have the same commit in my tree when I apply
>> this)
>
> Yup, that's the one :)
>
>>> Any chance of seeing this merged during the current merge window? :)
>>
>> I sent last new feature ("-next") patches for 4.9 last week, sorry. So
>> this has to wait for 4.10.
>
> Ah, right, I think I got my merge windows confused. You already said you
> wouldn't take it for 4.9. So I guess what I'm asking is for you to put
> it into the appropriate -next tree so it can get some wider exposure
> ahead of the *next* merge window...
Yeah, we have plenty of time for 4.10 :) So my plan is to apply this
after I open wireless-drivers-next in 2-3 weeks or so. That would mean
that the patch would hit Linus' tree when 4.10-rc1 is released
(estimated to happen on 2017-01-01). The timing is actually perfect as
now we get maximal testing time on -next.
>> And I assume I need to take v5:
>>
>> https://patchwork.kernel.org/patch/9311037/
>
> Yes. Haven't noticed anything that changed since that might conflict
> with it, but let me know if I missed something and you want a refreshed
> version.
Thanks, I'll let you know if there are any problems.
--=20
Kalle Valo
^ permalink raw reply
* ath9k excessive delay in handling EAPOL frames
From: Daniel Drake @ 2016-10-05 17:45 UTC (permalink / raw)
To: linux-wireless; +Cc: Linux Upstreaming Team, ath9k-devel
Hi,
As this is remote problem debugging I haven't gathered quite as much
info as I would like, and won't be investigating further immediately,
but I would like to share what I have found so far, maybe it is useful
knowledge and we can revisit later.
With the following hardware on Linux 4.4, we cannot connect to our
office WPA2-PSK network. Other networks seem fine.
02:00.0 Network controller [0280]: Qualcomm Atheros QCA9565 / AR9565
Wireless Network Adapter [168c:0036] (rev 01)
Subsystem: AzureWave Device [1a3b:218d]
Kernel driver in use: ath9k
The logs show:
wpa_supplicant[585]: wlp2s0: SME: Trying to authenticate with
0c:11:67:33:8d:50 (SSID='Endless' freq=2457 MHz)
kernel: wlp2s0: authenticate with 0c:11:67:33:8d:50
NetworkManager[620]: <info> [1474483556.0677] device (wlp2s0):
supplicant interface state: inactive -> authenticating
kernel: wlp2s0: send auth to 0c:11:67:33:8d:50 (try 1/3)
kernel: wlp2s0: send auth to 0c:11:67:33:8d:50 (try 2/3)
kernel: wlp2s0: send auth to 0c:11:67:33:8d:50 (try 3/3)
wpa_supplicant[585]: wlp2s0: Trying to associate with
0c:11:67:33:8d:50 (SSID='Endless' freq=2457 MHz)
kernel: wlp2s0: authenticated
NetworkManager[620]: <info> [1474483558.1078] device (wlp2s0):
supplicant interface state: authenticating -> associating
kernel: wlp2s0: associate with 0c:11:67:33:8d:50 (try 1/3)
kernel: wlp2s0: associate with 0c:11:67:33:8d:50 (try 2/3)
kernel: wlp2s0: associate with 0c:11:67:33:8d:50 (try 3/3)
wpa_supplicant[585]: wlp2s0: Associated with 0c:11:67:33:8d:50
kernel: wlp2s0: RX AssocResp from 0c:11:67:33:8d:50 (capab=0x431 status=0 aid=5)
kernel: wlp2s0: associated
kernel: wlp2s0: deauthenticated from 0c:11:67:33:8d:50 (Reason:
23=IEEE8021X_FAILED)
Using monitor mode from another station, I observe:
- STA sends association request
- AP sends association response 0.01s later, STA acks
- AP sends EAPOL 0.002s later, STA acks
- AP sends another EAPOL 0.1s later, STA acks
- AP sends deauthentication 0.3s later (presumably a timeout waiting
for EAPOL response), STA acks
- STA sends another association request 0.5s later
- AP replies with Deauthentication (can't associated as you are deauthed)
- STA sends another association request 1s later
- AP replies with Deauthentication again
- STA sends EAPOL response message, a full 2 seconds after the first
EAPOL was received
It is as if the processing of incoming frames is getting stuck for 2
seconds, even though they were already ACKed. i.e. The first
association requests succeeds immediately but the processing of the
AssocResp frame (and the following EAPOLs and deauth) is delayed by
more than 2 seconds, far longer than the AP is willing to wait.
I have confirmed this perspective in the wpa_supplicant debug logs
too, there is 2 seconds of RX silence after the first association
request is sent before all the frames come in at once.
Hope this partial info is useful in some way, I'll come back to this
problem as time permits.
Daniel
^ permalink raw reply
* RE: [PATCH v2 2/2] mwifiex: check hw_status in suspend and resume handlers
From: Cathy Luo @ 2016-10-05 17:19 UTC (permalink / raw)
To: Brian Norris, Amitkumar Karwar
Cc: linux-wireless@vger.kernel.org, Nishant Sarmukadam,
rajatja@google.com, Xinming Hu
In-Reply-To: <20161005164138.GB54237@google.com>
Hi Brian
I think your concern is right, we will update patch to handle the case you mentioned below.
If our firmware is dead or we init failure, we should allow the system to suspend.
We have following hardware status supported in driver. We should return -EBUSY only when hardware status is
MWIFIEX_HW_STATUS_INITIALIZING/MWIFIEX_HW_STATUS_INIT_DONE/ MWIFIEX_HW_STATUS_CLOSING.
enum MWIFIEX_HARDWARE_STATUS {
MWIFIEX_HW_STATUS_READY,
MWIFIEX_HW_STATUS_INITIALIZING,
MWIFIEX_HW_STATUS_INIT_DONE,
MWIFIEX_HW_STATUS_RESET,
MWIFIEX_HW_STATUS_CLOSING,
MWIFIEX_HW_STATUS_NOT_READY
};
Amit will help prepare the new patch to handle this.
Regards
Cathy
-----Original Message-----
From: Brian Norris [mailto:briannorris@chromium.org]
Sent: Wednesday, October 05, 2016 9:42 AM
To: Amitkumar Karwar
Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam; rajatja@google.com; Xinming Hu
Subject: Re: [PATCH v2 2/2] mwifiex: check hw_status in suspend and resume handlers
Hi Amit,
On Wed, Oct 05, 2016 at 12:26:36PM +0000, Amitkumar Karwar wrote:
> > From: Brian Norris [mailto:briannorris@chromium.org]
> > Sent: Wednesday, October 05, 2016 2:35 AM
> > To: Amitkumar Karwar
> > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > rajatja@google.com; briannorris@google.com; Xinming Hu
> > Subject: Re: [PATCH v2 2/2] mwifiex: check hw_status in suspend and
> > resume handlers
> >
> > On Tue, Oct 04, 2016 at 10:38:25PM +0530, Amitkumar Karwar wrote:
> > > From: Xinming Hu <huxm@marvell.com>
> > >
> > > We have observed a kernel crash when system immediately suspends
> > > after booting. There is a race between suspend and driver
> > > initialization paths.
> > > This patch adds hw_status checks to fix the problem
> > >
> > > Signed-off-by: Xinming Hu <huxm@marvell.com>
> > > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> > > ---
> > > v2: Return failure in suspend/resume handler in this scenario.
> > > ---
> > > drivers/net/wireless/marvell/mwifiex/pcie.c | 10 ++++++----
> > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > index ba9e068..fa6bf85 100644
> > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > @@ -122,9 +122,10 @@ static int mwifiex_pcie_suspend(struct device
> > > *dev)
> > >
> > > if (pdev) {
> > > card = pci_get_drvdata(pdev);
> > > - if (!card || !card->adapter) {
> > > + if (!card || !card->adapter ||
> > > + card->adapter->hw_status != MWIFIEX_HW_STATUS_READY) {
> >
> > Wait, is there no locking on the 'hw_status' field? That is
> > inherently an unsafe race all on its own; you're not guaranteed that
> > this will be read/written atomically. And you also aren't guaranteed
> > that writes to this happen in the order they appear in the code --
> > in other words, reading this flag doesn't necessarily guarantee that
> > initialization is actually complete (even if that's very likely to
> > be true, given that it's probably just a single-instruction
> > word-access, and any prior HW polling or interrupts likely have done
> > some synchronization and can't be reordered).
> > actually complete
>
> Here is the brief info on how "hw_status" flag is updated.
> 1) It gets changed incrementally during initialization.
> MWIFIEX_HW_STATUS_INITIALIZING -> MWIFIEX_HW_STATUS_INIT_DONE ->
> MWIFIEX_HW_STATUS_READY
>
> 2) Status will remain READY once driver+firmware is up and running.
>
> 3) Below is status during teardown
> MWIFIEX_HW_STATUS_READY -> MWIFIEX_HW_STATUS_RESET ->
> MWIFIEX_HW_STATUS_CLOSING -> MWIFIEX_HW_STATUS_NOT_READY
>
> As the events occur one after another, we don't expect a race and
> don't need locking here for the flag. Flag status
> MWIFIEX_HW_STATUS_READY guarantees that initialization is completed.
It seems like, as with patch 1, you're mostly arguing about the writes to this variable. But writes race with reads as well; how do you guarantee that you're not seeing incorrect values of 'hw_status' here in
suspend() -- e.g., either old or new values of it, or even partially-written values, if for some reason the compiler decides it can't read/write this all in one go?
> In worst case scenario, only first system suspend attempt issued
> immediately after system boot will be aborted with BUSY error. I
> think, that should be fine.
(For the record, my concern about -EBUSY is separate from my concern about the potential race condition.)
> Let me know if you have any concerns.
Sorry, I probably didn't completely flesh out my thought here.
I think I was concerned about a failed initialization causing the system to never enter suspend again. So specifically: what happens if (e.g.) the firmware fails to load? AFAICT, the device doesn't actually unbind itself from the driver, so instead, you have a device in limbo that will always return -EBUSY in suspend(), and your system can never again enter suspend. Am I correct? If so, that doesn't sound great.
> > This is probably better than nothing, but it's not very good.
> >
> > > pr_err("Card or adapter structure is not valid\n");
> > > - return 0;
> > > + return -EBUSY;
> >
> > So the above cases all mean that the driver hasn't finished loading,
> > right?
> >
> > !card => can't happen (PCIe probe() would have failed
> > mwifiex_add_card()), but fine to check
>
> NULL "card" or "card->adapter" pointers are expected in below cases
> 1) Driver initialization failed after downloading the firmware. Driver is performing cleanup in init failure path. Now system suspends.
> 2) Race of teardown + suspend
>
> >
> > !card->adapter => only happens after patch 1; i.e., when tearing
> > down the device and detaching it from the driver
> >
> > card->adapter->hw_status != MWIFIEX_HW_STATUS_READY => FW is not
> > card->adapter->loaded
> > (i.e., in the process of starting or stopping FW?)
> >
> > I guess all of those cases make sense to be -EBUSY.
^^ I'm second-guessing my claim here then.
> Yes. we can keep -EBUSY for all of the cases.
Brian
^ permalink raw reply
* Re: [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.
From: Toke Høiland-Jørgensen @ 2016-10-05 16:55 UTC (permalink / raw)
To: Kalle Valo
Cc: linux-wireless, make-wifi-fast, ath9k-devel, Tim Shepard,
Felix Fietkau
In-Reply-To: <878tu2okv7.fsf@kamboji.qca.qualcomm.com>
Kalle Valo <kvalo@codeaurora.org> writes:
> Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk> writes:
>
>> Kalle Valo <kvalo@codeaurora.org> writes:
>>
>>> Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk> writes:
>>>
>>> I understand your point, but I don't want to rush this to 4.9 and then
>>> start getting lots of bug reports and eventually forced to revert it. If
>>> we just found a new serious regression the chances are that there are
>>> more lurking somewhere and this patch is just not ready yet.
>>
>> So, the changes to mac80211 that fixes the known regressions of this
>> patch have gone in.
>
> I guess you mean this commit:
>
> bb42f2d13ffc mac80211: Move reorder-sensitive TX handlers to after TXQ de=
queue
>
> (Just making sure that I have the same commit in my tree when I apply
> this)
Yup, that's the one :)
>> Any chance of seeing this merged during the current merge window? :)
>
> I sent last new feature ("-next") patches for 4.9 last week, sorry. So
> this has to wait for 4.10.
Ah, right, I think I got my merge windows confused. You already said you
wouldn't take it for 4.9. So I guess what I'm asking is for you to put
it into the appropriate -next tree so it can get some wider exposure
ahead of the *next* merge window...
> And I assume I need to take v5:
>
> https://patchwork.kernel.org/patch/9311037/
Yes. Haven't noticed anything that changed since that might conflict
with it, but let me know if I missed something and you want a refreshed
version.
-Toke
^ permalink raw reply
* Re: [PATCH v2 2/2] mwifiex: check hw_status in suspend and resume handlers
From: Brian Norris @ 2016-10-05 16:41 UTC (permalink / raw)
To: Amitkumar Karwar
Cc: linux-wireless@vger.kernel.org, Cathy Luo, Nishant Sarmukadam,
rajatja@google.com, Xinming Hu
In-Reply-To: <39b7881045ec42628a4ffa654840681e@SC-EXCH04.marvell.com>
Hi Amit,
On Wed, Oct 05, 2016 at 12:26:36PM +0000, Amitkumar Karwar wrote:
> > From: Brian Norris [mailto:briannorris@chromium.org]
> > Sent: Wednesday, October 05, 2016 2:35 AM
> > To: Amitkumar Karwar
> > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > rajatja@google.com; briannorris@google.com; Xinming Hu
> > Subject: Re: [PATCH v2 2/2] mwifiex: check hw_status in suspend and
> > resume handlers
> >
> > On Tue, Oct 04, 2016 at 10:38:25PM +0530, Amitkumar Karwar wrote:
> > > From: Xinming Hu <huxm@marvell.com>
> > >
> > > We have observed a kernel crash when system immediately suspends after
> > > booting. There is a race between suspend and driver initialization
> > > paths.
> > > This patch adds hw_status checks to fix the problem
> > >
> > > Signed-off-by: Xinming Hu <huxm@marvell.com>
> > > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> > > ---
> > > v2: Return failure in suspend/resume handler in this scenario.
> > > ---
> > > drivers/net/wireless/marvell/mwifiex/pcie.c | 10 ++++++----
> > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > index ba9e068..fa6bf85 100644
> > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > @@ -122,9 +122,10 @@ static int mwifiex_pcie_suspend(struct device
> > > *dev)
> > >
> > > if (pdev) {
> > > card = pci_get_drvdata(pdev);
> > > - if (!card || !card->adapter) {
> > > + if (!card || !card->adapter ||
> > > + card->adapter->hw_status != MWIFIEX_HW_STATUS_READY) {
> >
> > Wait, is there no locking on the 'hw_status' field? That is inherently
> > an unsafe race all on its own; you're not guaranteed that this will be
> > read/written atomically. And you also aren't guaranteed that writes to
> > this happen in the order they appear in the code -- in other words,
> > reading this flag doesn't necessarily guarantee that initialization is
> > actually complete (even if that's very likely to be true, given that
> > it's probably just a single-instruction word-access, and any prior HW
> > polling or interrupts likely have done some synchronization and can't be
> > reordered).
> > actually complete
>
> Here is the brief info on how "hw_status" flag is updated.
> 1) It gets changed incrementally during initialization.
> MWIFIEX_HW_STATUS_INITIALIZING -> MWIFIEX_HW_STATUS_INIT_DONE -> MWIFIEX_HW_STATUS_READY
>
> 2) Status will remain READY once driver+firmware is up and running.
>
> 3) Below is status during teardown
> MWIFIEX_HW_STATUS_READY -> MWIFIEX_HW_STATUS_RESET -> MWIFIEX_HW_STATUS_CLOSING -> MWIFIEX_HW_STATUS_NOT_READY
>
> As the events occur one after another, we don't expect a race and
> don't need locking here for the flag. Flag status
> MWIFIEX_HW_STATUS_READY guarantees that initialization is completed.
It seems like, as with patch 1, you're mostly arguing about the writes
to this variable. But writes race with reads as well; how do you
guarantee that you're not seeing incorrect values of 'hw_status' here in
suspend() -- e.g., either old or new values of it, or even
partially-written values, if for some reason the compiler decides it
can't read/write this all in one go?
> In worst case scenario, only first system suspend attempt issued
> immediately after system boot will be aborted with BUSY error. I
> think, that should be fine.
(For the record, my concern about -EBUSY is separate from my concern
about the potential race condition.)
> Let me know if you have any concerns.
Sorry, I probably didn't completely flesh out my thought here.
I think I was concerned about a failed initialization causing the system
to never enter suspend again. So specifically: what happens if (e.g.)
the firmware fails to load? AFAICT, the device doesn't actually unbind
itself from the driver, so instead, you have a device in limbo that will
always return -EBUSY in suspend(), and your system can never again enter
suspend. Am I correct? If so, that doesn't sound great.
> > This is probably better than nothing, but it's not very good.
> >
> > > pr_err("Card or adapter structure is not valid\n");
> > > - return 0;
> > > + return -EBUSY;
> >
> > So the above cases all mean that the driver hasn't finished loading,
> > right?
> >
> > !card => can't happen (PCIe probe() would have failed
> > mwifiex_add_card()), but fine to check
>
> NULL "card" or "card->adapter" pointers are expected in below cases
> 1) Driver initialization failed after downloading the firmware. Driver is performing cleanup in init failure path. Now system suspends.
> 2) Race of teardown + suspend
>
> >
> > !card->adapter => only happens after patch 1; i.e., when tearing down
> > the device and detaching it from the driver
> >
> > card->adapter->hw_status != MWIFIEX_HW_STATUS_READY => FW is not loaded
> > (i.e., in the process of starting or stopping FW?)
> >
> > I guess all of those cases make sense to be -EBUSY.
^^ I'm second-guessing my claim here then.
> Yes. we can keep -EBUSY for all of the cases.
Brian
^ permalink raw reply
* Re: [PATCH] ath10k: cache calibration data when the core is stopped.
From: Marty Faltesek @ 2016-10-05 16:40 UTC (permalink / raw)
To: Michal Kazior; +Cc: ath10k@lists.infradead.org, linux-wireless
In-Reply-To: <CA+BoTQkwSX954pPEh3oYzL62H9y4Y6UuPVLSxxxCkM5AznY7NA@mail.gmail.com>
On Mon, Oct 3, 2016 at 4:02 AM, Michal Kazior <michal.kazior@tieto.com> wro=
te:
> On 13 September 2016 at 23:11, Marty Faltesek <mfaltesek@google.com> wrot=
e:
> [...]
>> +int
>> +ath10k_cal_data_alloc(struct ath10k *ar, void **buf)
>> +{
>> + u32 hi_addr;
>> + __le32 addr;
>> + int ret;
>> +
>> + vfree(*buf);
>> + *buf =3D vmalloc(QCA988X_CAL_DATA_LEN);
>
> Shouldn't you use ar->hw_params to get hw-specific caldata length?
yup, it was because we were based on backports from earlier this year.
>
>
> [...]
>> @@ -1714,6 +1750,12 @@ int ath10k_core_start(struct ath10k *ar, enum ath=
10k_firmware_mode mode)
>>
>> INIT_LIST_HEAD(&ar->arvifs);
>>
>> + /*
>> + * We are up now, so no need to cache calibration data.
>> + */
>
> The comment style is:
>
> /* comment */
>
> If it's multi-line it should be:
>
> /* comment
> * comment
> */
>
> Ditto for other instances.
fixed, thanks.
>
>
> [...]
>> @@ -1757,6 +1799,11 @@ void ath10k_core_stop(struct ath10k *ar)
>> lockdep_assert_held(&ar->conf_mutex);
>> ath10k_debug_stop(ar);
>>
>> + /*
>> + * Cache caclibration data while stopped.
>
> typo. "calibration"
>
>
> Micha=C5=82
fixed thanks.
^ permalink raw reply
* Re: [PATCH] ath10k: cache calibration data when the core is stopped.
From: Marty Faltesek @ 2016-10-05 16:39 UTC (permalink / raw)
To: Valo, Kalle; +Cc: ath10k@lists.infradead.org, linux-wireless@vger.kernel.org
In-Reply-To: <87fuodswlu.fsf@kamboji.qca.qualcomm.com>
On Mon, Oct 3, 2016 at 3:46 AM, Valo, Kalle <kvalo@qca.qualcomm.com> wrote:
> Marty Faltesek <mfaltesek@google.com> writes:
>
>> Caching calibration data allows it to be accessed when the
>> device is not active.
>>
>> Signed-off-by: Marty Faltesek <mfaltesek@google.com>
>
> No comma in the title, please.
>
> What tree did you use as the baseline? This doesn't seem to apply to
> ath.git:
We use backports 20160122 which has not been updated since earlier this year.
I can forward port it to your tree, and make sure
it builds but won't be able to test it. Will that be OK?
>
> Applying: ath10k: cache calibration data when the core is stopped.
> fatal: sha1 information is lacking or useless (drivers/net/wireless/ath/ath10k/core.c).
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 ath10k: cache calibration data when the core is stopped.
>
>> --- a/drivers/net/wireless/ath/ath10k/core.c
>> +++ b/drivers/net/wireless/ath/ath10k/core.c
>> @@ -1227,6 +1227,42 @@ success:
>> return 0;
>> }
>>
>> +int
>> +ath10k_cal_data_alloc(struct ath10k *ar, void **buf)
>> +{
>> + u32 hi_addr;
>> + __le32 addr;
>> + int ret;
>
> I think this function should be in debug.c. That way the code is not
> wasting memory if DEBUGFS is disabled.
ok.
>
>> + vfree(*buf);
>> + *buf = vmalloc(QCA988X_CAL_DATA_LEN);
>> + if (!*buf) {
>> + return -EAGAIN;
>> + }
>
> Is it really necessary to allocate memory every time? What if allocate
> it only once when module is loaded, just like with
> ar->debug.fw_crash_data?
yup, good suggestion.
>
> Also please note that this patch (which I'm queuing to 4.9) touches the
> same area:
>
> ath10k: fix debug cal data file
>
> https://patchwork.kernel.org/patch/9340953/
I've modified this too, and this won't be necessary, so can you drop
it? If not, let me know and I'll
pull it in and make sure I'm based off it too.
^ permalink raw reply
* Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device unregister
From: Brian Norris @ 2016-10-05 16:30 UTC (permalink / raw)
To: Amitkumar Karwar
Cc: linux-wireless@vger.kernel.org, Cathy Luo, Nishant Sarmukadam,
rajatja@google.com, Xinming Hu
In-Reply-To: <8ec683dc72a746909157fca4dcbd10e8@SC-EXCH04.marvell.com>
Hi,
On Wed, Oct 05, 2016 at 02:04:53PM +0000, Amitkumar Karwar wrote:
> > From: Brian Norris [mailto:briannorris@chromium.org]
> > Sent: Wednesday, October 05, 2016 3:28 AM
> > To: Amitkumar Karwar
> > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > rajatja@google.com; briannorris@google.com; Xinming Hu
> > Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device
> > unregister
> >
> > Hi,
> >
> > On Tue, Oct 04, 2016 at 10:38:24PM +0530, Amitkumar Karwar wrote:
> > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct
> > mwifiex_adapter *adapter)
> > > pci_disable_msi(pdev);
> > > }
> > > }
> > > + card->adapter = NULL;
> >
> > I think you have a similar problem here as in patch 2; there is no
> > locking to protect fields in struct pcie_service_card or struct
> > sdio_mmc_card below. That problem kind of already exists, except that
> > you only write the value of card->adapter once at registration time, so
> > it's not actually unsafe. But now that you're introducing a second
> > write, you have a problem.
> >
> > Brian
> >
>
> We have a global "add_remove_card_sem" semaphore in our code for
> synchronizing initialization and teardown threads. Ideally "init +
> teardown/reboot" should not have a race issue with this logic
>
> Later there was a loophole introduced in this after using async
> firmware download API. During initialization, firmware downloads
> asynchronously in a separate thread where might have released the
> semaphore. I am working on a patch to fix this.
>
> So "card->adapter" doesn't need to have locking. Even if we have two
> write operations, those two threads can't run simultaneously due to
> above mentioned logic.
What about writes racing with reads? You have lots of unsynchronized
cases that read this, although most of them should be halted by now
(e.g., cmd processing). I was looking at suspend() in particular, which
I thought you were looking at in this patch series.
Brian
^ permalink raw reply
* Re: [-next] wlcore: sdio: drop kfree for memory allocated with devm_kzalloc
From: Kalle Valo @ 2016-10-05 16:05 UTC (permalink / raw)
To: Wei Yongjun; +Cc: Bruno Herrera, Tony Lindgren, Wei Yongjun, linux-wireless
In-Reply-To: <1475073516-7328-1-git-send-email-weiyj.lk@gmail.com>
Wei Yongjun <weiyj.lk@gmail.com> wrote:
> From: Wei Yongjun <weiyongjun1@huawei.com>
>
> It's not necessary to free memory allocated with devm_kzalloc
> and using kfree leads to a double free.
>
> Fixes: d776fc86b82f ("wlcore: sdio: Populate config firmware data")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> Acked-by: Tony Lindgren <tony@atomide.com>
Patch applied to wireless-drivers.git, thanks.
f38b7c254753 wlcore: sdio: drop kfree for memory allocated with devm_kzalloc
--
https://patchwork.kernel.org/patch/9353985/
Documentation about submitting wireless patches and checking status
from patchwork:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.
From: Kalle Valo @ 2016-10-05 15:50 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: linux-wireless, make-wifi-fast, ath9k-devel, Tim Shepard,
Felix Fietkau
In-Reply-To: <87k2dm6git.fsf@toke.dk>
Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk> writes:
> Kalle Valo <kvalo@codeaurora.org> writes:
>
>> Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk> writes:
>>
>> I understand your point, but I don't want to rush this to 4.9 and then
>> start getting lots of bug reports and eventually forced to revert it. If
>> we just found a new serious regression the chances are that there are
>> more lurking somewhere and this patch is just not ready yet.
>
> So, the changes to mac80211 that fixes the known regressions of this
> patch have gone in.
I guess you mean this commit:
bb42f2d13ffc mac80211: Move reorder-sensitive TX handlers to after TXQ dequ=
eue
(Just making sure that I have the same commit in my tree when I apply this)
> Any chance of seeing this merged during the current merge window? :)
I sent last new feature ("-next") patches for 4.9 last week, sorry. So
this has to wait for 4.10.
And I assume I need to take v5:
https://patchwork.kernel.org/patch/9311037/
--=20
Kalle Valo
^ permalink raw reply
* Re: [PATCHv4] mac80211: check A-MSDU inner frame source address on AP interfaces
From: Johannes Berg @ 2016-10-05 14:59 UTC (permalink / raw)
To: Michael Braun
Cc: linux-wireless, projekt-wlan, kvalo, akarwar, nishants,
Larry.Finger, Jes.Sorensen
In-Reply-To: <1475661751-26262-1-git-send-email-michael-dev@fami-braun.de>
On Wed, 2016-10-05 at 12:02 +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.
> This might affect ARP/IP filtering on the AccessPoint.
>
> 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, as a wifi client may send all
> its traffic to the AP in order to have it forwarded.
>
> Signed-off-by: Michael Braun <michael-dev@fami-braun.de>
So as you can see by my own version of this patch, I'm not super happy
with the way you did things here :)
Obviously, the commit log is now pretty much wrong here in your patch,
since you do much more than that now and don't focus on the SA only.
> @@ -1436,7 +1436,8 @@ static void
> iwl_mvm_report_wakeup_reasons(struct iwl_mvm *mvm,
>
> memcpy(skb_put(pkt, pktsize), pktdata,
> pktsize);
>
> - if (ieee80211_data_to_8023(pkt, vif->addr,
> vif->type))
> + if (ieee80211_data_to_8023(pkt, NULL, vif-
> >addr,
> + vif->type))
> goto report;
I did something similar in the first patch I sent, but without changing
the drivers (by using a static inline and a new function name)
> void ieee80211_amsdu_to_8023s(struct sk_buff *skb, struct
> sk_buff_head *list,
> - const u8 *addr, enum nl80211_iftype
> iftype,
> + const u8 *addr,
> + enum nl80211_iftype iftype,
> const unsigned int extra_headroom,
> - bool has_80211_header);
> + const u8 *ta, const u8 *ra, bool
> is_4addr,
> + bool is_tdls_data);
Instead of adding 4 new arguments, (ta, ra, is_4addr, is_tdls_data), I
opted to just add two (check_da and check_sa) and make those NULL when
no checks are desired.
I *think* that works equivalently, but it'd be great if you could take
a look.
I had also removed the has_80211_header argument in patch 2, so we
don't clutter this thing as much.
> - if (is_multicast_ether_addr(hdr->addr1) &&
> - ((rx->sdata->vif.type == NL80211_IFTYPE_AP_VLAN &&
> - rx->sdata->u.vlan.sta) ||
> - (rx->sdata->vif.type == NL80211_IFTYPE_STATION &&
> - rx->sdata->u.mgd.use_4addr)))
> + is_4addr = ((rx->sdata->vif.type == NL80211_IFTYPE_AP_VLAN
> &&
> + rx->sdata->u.vlan.sta) ||
> + (rx->sdata->vif.type == NL80211_IFTYPE_STATION
> &&
> + rx->sdata->u.mgd.use_4addr));
> + if (is_multicast_ether_addr(hdr->addr1) && is_4addr)
> return RX_DROP_UNUSABLE;
This also conflicts with the earlier patch I sent to just always drop
when it's multicast.
> skb->dev = dev;
> __skb_queue_head_init(&frame_list);
>
> + if (ieee80211_data_to_8023(skb, ð_80211, dev->dev_addr,
> + rx->sdata->vif.type) < 0)
> + return RX_DROP_UNUSABLE;
> +
> + is_tdls_data = !ieee80211_has_tods(fc) &&
> !ieee80211_has_fromds(fc);
> ieee80211_amsdu_to_8023s(skb, &frame_list, dev->dev_addr,
> rx->sdata->vif.type,
> - rx->local->hw.extra_tx_headroom,
> true);
> + rx->local->hw.extra_tx_headroom,
> + eth_80211.h_source,
> + eth_80211.h_dest, is_4addr,
> is_tdls_data);
Because you're passing eth_80211.h_* unconditionally, you need those
extra arguments, but I don't see why my approach wouldn't work.
> + /* limit inner src/dst checks depending on iftype */
> + switch (iftype) {
> + case NL80211_IFTYPE_AP:
> + case NL80211_IFTYPE_AP_VLAN:
> + if (is_4addr)
> + ta = NULL;
> + ra = NULL;
> + break;
> + case NL80211_IFTYPE_ADHOC:
> + break;
> + case NL80211_IFTYPE_STATION:
> + if (is_4addr || !is_tdls_data)
> + ta = NULL;
> + if (is_4addr)
> + ra = NULL;
> + break;
> + default:
> + ta = NULL;
> + ra = NULL;
> }
I have this in mac80211, which imho makes it easier.
I had half in mind to actually pass something like "expected frame
type", which wouldn't just be iftype, but be more like "AP, CLIENT,
TDLS, MESH, IBSS, ...", but it ultimately seemed too complex.
johannes
^ permalink raw reply
* [RFC 3/3] mac80211: validate DA/SA during A-MSDU decapsulation
From: Johannes Berg @ 2016-10-05 14:44 UTC (permalink / raw)
To: linux-wireless; +Cc: Michael Braun, Johannes Berg
In-Reply-To: <1475678690-29226-1-git-send-email-johannes@sipsolutions.net>
From: Johannes Berg <johannes.berg@intel.com>
As pointed out by Michael Braun, we don't check inner L2 addresses
during A-MSDU decapsulation, leading to the possibility that, for
example, a station associated to an AP sends frames as though they
came from somewhere else.
Fix this problem by letting cfg80211 validate the addresses, as
indicated by passing in the ones that need to be validated.
Reported-by: Michael Braun <michael-dev@fami-braun.de>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
net/mac80211/rx.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index cf53fe1a0aa2..a47bbc973f2d 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -2299,6 +2299,7 @@ ieee80211_rx_h_amsdu(struct ieee80211_rx_data *rx)
struct sk_buff_head frame_list;
struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(rx->skb);
struct ethhdr ethhdr;
+ const u8 *check_da = ethhdr.h_dest, *check_sa = ethhdr.h_source;
if (unlikely(!ieee80211_is_data(fc)))
return RX_CONTINUE;
@@ -2322,6 +2323,23 @@ ieee80211_rx_h_amsdu(struct ieee80211_rx_data *rx)
default:
return RX_DROP_UNUSABLE;
}
+ check_da = NULL;
+ check_sa = NULL;
+ } else switch (rx->sdata->vif.type) {
+ case NL80211_IFTYPE_AP:
+ case NL80211_IFTYPE_AP_VLAN:
+ check_da = NULL;
+ break;
+ case NL80211_IFTYPE_STATION:
+ if (!rx->sta ||
+ !test_sta_flag(rx->sta, WLAN_STA_TDLS_PEER))
+ check_sa = NULL;
+ break;
+ case NL80211_IFTYPE_MESH_POINT:
+ check_sa = NULL;
+ break;
+ default:
+ break;
}
if (is_multicast_ether_addr(hdr->addr1))
@@ -2338,7 +2356,7 @@ ieee80211_rx_h_amsdu(struct ieee80211_rx_data *rx)
ieee80211_amsdu_to_8023s(skb, &frame_list, dev->dev_addr,
rx->sdata->vif.type,
rx->local->hw.extra_tx_headroom,
- NULL, NULL);
+ check_da, check_sa);
while (!skb_queue_empty(&frame_list)) {
rx->skb = __skb_dequeue(&frame_list);
--
2.8.1
^ permalink raw reply related
* [RFC 1/3] cfg80211: let ieee80211_amsdu_to_8023s() take only header-less SKB
From: Johannes Berg @ 2016-10-05 14:44 UTC (permalink / raw)
To: linux-wireless; +Cc: Michael Braun, Johannes Berg
From: Johannes Berg <johannes.berg@intel.com>
There's only a single case where has_80211_header is passed as true,
which is in mac80211. Given that there's only simple code that needs
to be done before calling it, export that function from cfg80211
instead and let mac80211 call it itself.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
.../net/wireless/marvell/mwifiex/11n_rxreorder.c | 2 +-
drivers/staging/rtl8723au/core/rtw_recv.c | 2 +-
include/net/cfg80211.h | 31 +++++++++++++++-------
net/mac80211/rx.c | 8 +++++-
net/wireless/util.c | 24 ++++-------------
5 files changed, 36 insertions(+), 31 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
index 94480123efa3..e9f462e3730b 100644
--- a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
+++ b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
@@ -45,7 +45,7 @@ static int mwifiex_11n_dispatch_amsdu_pkt(struct mwifiex_private *priv,
skb_trim(skb, le16_to_cpu(local_rx_pd->rx_pkt_length));
ieee80211_amsdu_to_8023s(skb, &list, priv->curr_addr,
- priv->wdev.iftype, 0, false);
+ priv->wdev.iftype, 0);
while (!skb_queue_empty(&list)) {
struct rx_packet_hdr *rx_hdr;
diff --git a/drivers/staging/rtl8723au/core/rtw_recv.c b/drivers/staging/rtl8723au/core/rtw_recv.c
index 150dabc2a58d..e1873c906c42 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);
while (!skb_queue_empty(&skb_list)) {
sub_skb = __skb_dequeue(&skb_list);
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index fe78f02a242e..f372eadd1963 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -4040,14 +4040,29 @@ unsigned int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr);
*/
/**
+ * ieee80211_data_to_8023_exthdr - convert an 802.11 data frame to 802.3
+ * @skb: the 802.11 data frame
+ * @ehdr: pointer to a &struct ethhdr that will get the header, instead
+ * of it being pushed into the SKB
+ * @addr: the device MAC address
+ * @iftype: the virtual interface type
+ * Return: 0 on success. Non-zero on error.
+ */
+int ieee80211_data_to_8023_exthdr(struct sk_buff *skb, struct ethhdr *ehdr,
+ const u8 *addr, enum nl80211_iftype iftype);
+
+/**
* ieee80211_data_to_8023 - convert an 802.11 data frame to 802.3
* @skb: the 802.11 data frame
* @addr: the device MAC address
* @iftype: the virtual interface type
* Return: 0 on success. Non-zero on error.
*/
-int ieee80211_data_to_8023(struct sk_buff *skb, const u8 *addr,
- enum nl80211_iftype iftype);
+static inline int ieee80211_data_to_8023(struct sk_buff *skb, const u8 *addr,
+ enum nl80211_iftype iftype)
+{
+ return ieee80211_data_to_8023_exthdr(skb, NULL, addr, iftype);
+}
/**
* ieee80211_data_from_8023 - convert an 802.3 frame to 802.11
@@ -4065,22 +4080,20 @@ int ieee80211_data_from_8023(struct sk_buff *skb, const u8 *addr,
/**
* ieee80211_amsdu_to_8023s - decode an IEEE 802.11n A-MSDU frame
*
- * Decode an IEEE 802.11n A-MSDU frame and convert it to a list of
- * 802.3 frames. The @list will be empty if the decode fails. The
- * @skb is consumed after the function returns.
+ * Decode an IEEE 802.11 A-MSDU and convert it to a list of 802.3 frames.
+ * The @list will be empty if the decode fails. The @skb must be fully
+ * header-less before being passed in here; it is freed in this function.
*
- * @skb: The input IEEE 802.11n A-MSDU frame.
+ * @skb: The input A-MSDU frame without any headers.
* @list: The output list of 802.3 frames. It must be allocated and
* initialized by by the caller.
* @addr: The device MAC address.
* @iftype: The device interface type.
* @extra_headroom: The hardware extra headroom for SKBs in the @list.
- * @has_80211_header: Set it true if SKB is with IEEE 802.11 header.
*/
void ieee80211_amsdu_to_8023s(struct sk_buff *skb, struct sk_buff_head *list,
const u8 *addr, enum nl80211_iftype iftype,
- const unsigned int extra_headroom,
- bool has_80211_header);
+ const unsigned int extra_headroom);
/**
* cfg80211_classify8021d - determine the 802.1p/1d tag for a data frame
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 3ac2f1cba317..de2d75fa5c51 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -2298,6 +2298,7 @@ ieee80211_rx_h_amsdu(struct ieee80211_rx_data *rx)
__le16 fc = hdr->frame_control;
struct sk_buff_head frame_list;
struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(rx->skb);
+ struct ethhdr ethhdr;
if (unlikely(!ieee80211_is_data(fc)))
return RX_CONTINUE;
@@ -2329,9 +2330,14 @@ ieee80211_rx_h_amsdu(struct ieee80211_rx_data *rx)
skb->dev = dev;
__skb_queue_head_init(&frame_list);
+ if (ieee80211_data_to_8023_exthdr(skb, ðhdr,
+ rx->sdata->vif.addr,
+ rx->sdata->vif.type))
+ return RX_DROP_UNUSABLE;
+
ieee80211_amsdu_to_8023s(skb, &frame_list, dev->dev_addr,
rx->sdata->vif.type,
- rx->local->hw.extra_tx_headroom, true);
+ rx->local->hw.extra_tx_headroom);
while (!skb_queue_empty(&frame_list)) {
rx->skb = __skb_dequeue(&frame_list);
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 8edce22d1b93..e36ede840b88 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -420,8 +420,8 @@ unsigned int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr)
}
EXPORT_SYMBOL(ieee80211_get_mesh_hdrlen);
-static int __ieee80211_data_to_8023(struct sk_buff *skb, struct ethhdr *ehdr,
- const u8 *addr, enum nl80211_iftype iftype)
+int ieee80211_data_to_8023_exthdr(struct sk_buff *skb, struct ethhdr *ehdr,
+ const u8 *addr, enum nl80211_iftype iftype)
{
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
struct {
@@ -525,13 +525,7 @@ static int __ieee80211_data_to_8023(struct sk_buff *skb, struct ethhdr *ehdr,
return 0;
}
-
-int ieee80211_data_to_8023(struct sk_buff *skb, const u8 *addr,
- enum nl80211_iftype iftype)
-{
- return __ieee80211_data_to_8023(skb, NULL, addr, iftype);
-}
-EXPORT_SYMBOL(ieee80211_data_to_8023);
+EXPORT_SYMBOL(ieee80211_data_to_8023_exthdr);
int ieee80211_data_from_8023(struct sk_buff *skb, const u8 *addr,
enum nl80211_iftype iftype,
@@ -745,25 +739,18 @@ __ieee80211_amsdu_copy(struct sk_buff *skb, unsigned int hlen,
void ieee80211_amsdu_to_8023s(struct sk_buff *skb, struct sk_buff_head *list,
const u8 *addr, enum nl80211_iftype iftype,
- const unsigned int extra_headroom,
- bool has_80211_header)
+ const unsigned int extra_headroom)
{
unsigned int hlen = ALIGN(extra_headroom, 4);
struct sk_buff *frame = NULL;
u16 ethertype;
u8 *payload;
- int offset = 0, remaining, err;
+ int offset = 0, remaining;
struct ethhdr eth;
bool reuse_frag = skb->head_frag && !skb_has_frag_list(skb);
bool reuse_skb = false;
bool last = false;
- if (has_80211_header) {
- err = __ieee80211_data_to_8023(skb, ð, addr, iftype);
- if (err)
- goto out;
- }
-
while (!last) {
unsigned int subframe_len;
int len;
@@ -819,7 +806,6 @@ void ieee80211_amsdu_to_8023s(struct sk_buff *skb, struct sk_buff_head *list,
purge:
__skb_queue_purge(list);
- out:
dev_kfree_skb(skb);
}
EXPORT_SYMBOL(ieee80211_amsdu_to_8023s);
--
2.8.1
^ permalink raw reply related
* [RFC 2/3] cfg80211: add ability to check DA/SA in A-MSDU decapsulation
From: Johannes Berg @ 2016-10-05 14:44 UTC (permalink / raw)
To: linux-wireless; +Cc: Michael Braun, Johannes Berg
In-Reply-To: <1475678690-29226-1-git-send-email-johannes@sipsolutions.net>
From: Johannes Berg <johannes.berg@intel.com>
We should not accept arbitrary DA/SA inside A-MSDUs, it could be used
to circumvent protections, like allowing a station to send frames and
make them seem to come from somewhere else.
Add the necessary infrastructure in cfg80211 to allow such checks, in
further patches we'll start using them.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c | 2 +-
drivers/staging/rtl8723au/core/rtw_recv.c | 2 +-
include/net/cfg80211.h | 5 ++++-
net/mac80211/rx.c | 3 ++-
net/wireless/util.c | 14 ++++++++++++--
5 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
index e9f462e3730b..274dd5a1574a 100644
--- a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
+++ b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
@@ -45,7 +45,7 @@ static int mwifiex_11n_dispatch_amsdu_pkt(struct mwifiex_private *priv,
skb_trim(skb, le16_to_cpu(local_rx_pd->rx_pkt_length));
ieee80211_amsdu_to_8023s(skb, &list, priv->curr_addr,
- priv->wdev.iftype, 0);
+ priv->wdev.iftype, 0, NULL, NULL);
while (!skb_queue_empty(&list)) {
struct rx_packet_hdr *rx_hdr;
diff --git a/drivers/staging/rtl8723au/core/rtw_recv.c b/drivers/staging/rtl8723au/core/rtw_recv.c
index e1873c906c42..d2a9ae2fe51a 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);
+ ieee80211_amsdu_to_8023s(skb, &skb_list, NULL, 0, 0, NULL, NULL);
while (!skb_queue_empty(&skb_list)) {
sub_skb = __skb_dequeue(&skb_list);
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index f372eadd1963..7df600c463eb 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -4090,10 +4090,13 @@ int ieee80211_data_from_8023(struct sk_buff *skb, const u8 *addr,
* @addr: The device MAC address.
* @iftype: The device interface type.
* @extra_headroom: The hardware extra headroom for SKBs in the @list.
+ * @check_da: DA to check in the inner ethernet header, or NULL
+ * @check_sa: SA to check in the inner ethernet header, or NULL
*/
void ieee80211_amsdu_to_8023s(struct sk_buff *skb, struct sk_buff_head *list,
const u8 *addr, enum nl80211_iftype iftype,
- const unsigned int extra_headroom);
+ const unsigned int extra_headroom,
+ const u8 *check_da, const u8 *check_sa);
/**
* cfg80211_classify8021d - determine the 802.1p/1d tag for a data frame
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index de2d75fa5c51..cf53fe1a0aa2 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -2337,7 +2337,8 @@ ieee80211_rx_h_amsdu(struct ieee80211_rx_data *rx)
ieee80211_amsdu_to_8023s(skb, &frame_list, dev->dev_addr,
rx->sdata->vif.type,
- rx->local->hw.extra_tx_headroom);
+ rx->local->hw.extra_tx_headroom,
+ NULL, NULL);
while (!skb_queue_empty(&frame_list)) {
rx->skb = __skb_dequeue(&frame_list);
diff --git a/net/wireless/util.c b/net/wireless/util.c
index e36ede840b88..5ea12afc7706 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -739,7 +739,8 @@ __ieee80211_amsdu_copy(struct sk_buff *skb, unsigned int hlen,
void ieee80211_amsdu_to_8023s(struct sk_buff *skb, struct sk_buff_head *list,
const u8 *addr, enum nl80211_iftype iftype,
- const unsigned int extra_headroom)
+ const unsigned int extra_headroom,
+ const u8 *check_da, const u8 *check_sa)
{
unsigned int hlen = ALIGN(extra_headroom, 4);
struct sk_buff *frame = NULL;
@@ -767,8 +768,17 @@ void ieee80211_amsdu_to_8023s(struct sk_buff *skb, struct sk_buff_head *list,
goto purge;
offset += sizeof(struct ethhdr);
- /* reuse skb for the last subframe */
last = remaining <= subframe_len + padding;
+
+ /* FIXME: should we really accept multicast DA? */
+ if ((check_da && !is_multicast_ether_addr(eth.h_dest) &&
+ !ether_addr_equal(check_da, eth.h_dest)) ||
+ (check_sa && !ether_addr_equal(check_sa, eth.h_source))) {
+ offset += len + padding;
+ continue;
+ }
+
+ /* reuse skb for the last subframe */
if (!skb_is_nonlinear(skb) && !reuse_frag && last) {
skb_pull(skb, offset);
frame = skb;
--
2.8.1
^ permalink raw reply related
* Issue with rt61-pci
From: Eddi De Pieri @ 2016-10-05 14:15 UTC (permalink / raw)
To: linux-wireless
Hi,
I have a router with onboard a rt2661 pci card and it use rt61pci driver.
With latest openwrt (it use compat wireless, so even the kernel isn't
the latest, the driver is near to linux-next) I get:
Lede-Trunk/Openwrt 15 and trunk:
by using the wifi card(AP mode) dmesg show:
[ 257.472000] ieee80211 phy1: rt61pci_txdone: Warning - TX status
report missed for entry 28
[ 257.480000] ieee80211 phy1: rt61pci_txdone: Warning - TX status
report missed for entry 29
[ 257.488000] ieee80211 phy1: rt61pci_txdone: Warning - TX status
report missed for entry 30
[ 257.496000] ieee80211 phy1: rt61pci_txdone: Warning - TX status
report missed for entry 31
[ 257.504000] ieee80211 phy1: rt61pci_txdone: Warning - TX status
report missed for entry 1
(don't remember which distribution i used to get this log... but all
suffer of the same issue)
and after few seconds of usage the wifi card stop working and it need
a reboot or rmmod rt61-pci/modprobe to make it work again for few
seconds
root@OpenWrt:/lib/firmware# lspci -v
00:01.0 Network controller: Ralink corp. RT2600 802.11 MIMO
Subsystem: Ralink corp. Device 2661
Flags: bus master, slow devsel, latency 64, IRQ 39
Memory at 30000000 (32-bit, non-prefetchable) [size=32K]
Memory at <unassigned> (32-bit, non-prefetchable) [size=2]
Memory at <unassigned> (32-bit, non-prefetchable) [size=2]
Memory at <unassigned> (32-bit, non-prefetchable) [size=2]
Memory at <unassigned> (32-bit, non-prefetchable) [size=2]
Memory at <unassigned> (32-bit, non-prefetchable) [size=2]
Expansion ROM at <unassigned> [disabled] [size=2]
Capabilities: [40] Power Management version 2
Kernel driver in use: rt61pci
lspci: Unable to load libkmod resources: error -12
root@OpenWrt:/proc/irq/39# cat /proc/interrupts
CPU0
0: 0 MIPS 0 smp_ipi0
1: 0 MIPS 1 smp_ipi1
7: 7906287 MIPS 7 timer
8: 0 bcm6345-periph-intc 0 bcm63xx_timer
10: 27 bcm6345-periph-intc 2 bcm63xx_uart
13: 1 bcm6345-periph-intc 5 ohci_hcd:usb2
14: 6 bcm6345-periph-intc 6 eth1
17: 0 bcm6345-periph-intc 9 phy_interrupt
18: 0 bcm6345-periph-intc 10 ehci_hcd:usb1
25: 168436 bcm6345-periph-intc 17 eth1
26: 96231 bcm6345-periph-intc 18 eth1
39: 492016 bcm6345-periph-intc 31 rt61pci
ERR: 0
root@OpenWrt:/proc/irq/39#
it seems that other people even on desktop systems get same error and
is not clear if it is solved on latest ubuntu as well there is already
a ticket opened on openwrt: https://dev.openwrt.org/ticket/18228
On Openwrt 12.09 work correctly.
Have you any idea about what could have introduced this regression?
ATTITUDE ADJUSTMENT (12.09, r36088)
root@OpenWrt:/# cat /proc/version
Linux version 3.3.8 (blogic@Debian-60-squeeze-64-minimal) (gcc version
4.6.3 20120201 (prerelease) (Linaro GCC 4.6-2012.02) ) #1 Sat Mar 23
18:09:20 UTC 2013
root@OpenWrt:/# cat /proc/interrupts
CPU0
2: 0 MIPS cascade_ip2
7: 479612 MIPS timer
8: 0 bcm63xx_ipic bcm63xx_timer
10: 18149 bcm63xx_ipic bcm63xx_uart
14: 19 bcm63xx_ipic eth1
17: 0 bcm63xx_ipic phy_interrupt
25: 247853 bcm63xx_ipic eth1
26: 117400 bcm63xx_ipic eth1
39: 552385 bcm63xx_ipic 0000:00:01.0
ERR: 0
root@OpenWrt:/# lspci -v
00:01.0 Network controller: Ralink corp. RT2600 802.11 MIMO
Subsystem: Ralink corp. Device 2661
Flags: bus master, slow devsel, latency 64, IRQ 39
Memory at 30000000 (32-bit, non-prefetchable) [size=32K]
Capabilities: [40] Power Management version 2
Kernel driver in use: rt61pci
01:1e.0 CardBus bridge: Broadcom Corporation Device 6358
Subsystem: Broadcom Corporation Device 6358
Flags: bus master, slow devsel, latency 0, IRQ 39
Bus: primary=01, secondary=02, subordinate=05, sec-latency=176
Memory window 0: 00000000-00000fff
Memory window 1: 00000001-00000fff
I/O window 0: 00000000-00000003
I/O window 1: 00000000-00000003
Please, can you give me some hint to troubleshoot this issue?
Regards
Eddi De Pieri
^ permalink raw reply
* RE: [PATCH v2 1/2] mwifiex: reset card->adapter during device unregister
From: Amitkumar Karwar @ 2016-10-05 14:04 UTC (permalink / raw)
To: Brian Norris
Cc: linux-wireless@vger.kernel.org, Cathy Luo, Nishant Sarmukadam,
rajatja@google.com, briannorris@google.com, Xinming Hu
In-Reply-To: <20161004215814.GB31652@localhost>
Hi Brian,
> From: Brian Norris [mailto:briannorris@chromium.org]
> Sent: Wednesday, October 05, 2016 3:28 AM
> To: Amitkumar Karwar
> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> rajatja@google.com; briannorris@google.com; Xinming Hu
> Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device
> unregister
>
> Hi,
>
> On Tue, Oct 04, 2016 at 10:38:24PM +0530, Amitkumar Karwar wrote:
> > From: Xinming Hu <huxm@marvell.com>
> >
> > card->adapter gets initialized during device registration.
> > As it's not cleared, we may end up accessing invalid memory in some
> > corner cases. This patch fixes the problem.
> >
> > Signed-off-by: Xinming Hu <huxm@marvell.com>
> > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> > ---
> > v2: Same as v1
> > ---
> > drivers/net/wireless/marvell/mwifiex/pcie.c | 1 +
> > drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
> > 2 files changed, 2 insertions(+)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > index f1eeb73..ba9e068 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct
> mwifiex_adapter *adapter)
> > pci_disable_msi(pdev);
> > }
> > }
> > + card->adapter = NULL;
>
> I think you have a similar problem here as in patch 2; there is no
> locking to protect fields in struct pcie_service_card or struct
> sdio_mmc_card below. That problem kind of already exists, except that
> you only write the value of card->adapter once at registration time, so
> it's not actually unsafe. But now that you're introducing a second
> write, you have a problem.
>
> Brian
>
We have a global "add_remove_card_sem" semaphore in our code for synchronizing initialization and teardown threads. Ideally "init + teardown/reboot" should not have a race issue with this logic
Later there was a loophole introduced in this after using async firmware download API. During initialization, firmware downloads asynchronously in a separate thread where might have released the semaphore. I am working on a patch to fix this.
So "card->adapter" doesn't need to have locking. Even if we have two write operations, those two threads can't run simultaneously due to above mentioned logic.
Regards,
Amitkumar
^ permalink raw reply
* Re: [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.
From: Toke Høiland-Jørgensen @ 2016-10-05 14:02 UTC (permalink / raw)
To: Kalle Valo
Cc: linux-wireless, make-wifi-fast, ath9k-devel, Tim Shepard,
Felix Fietkau
In-Reply-To: <877fb8ug0x.fsf@kamboji.qca.qualcomm.com>
Kalle Valo <kvalo@codeaurora.org> writes:
> Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk> writes:
>
>>>>> This is great work but due to the regressions I'm not sure if this
>>>>> will be ready for 4.9. To get more testing time I wonder if we should
>>>>> wait for 4.10? IMHO applying this in the end of the cycle is too risky
>>>>> and we should try to maximise the time linux-next by applying this
>>>>> just after -rc1 is released.
>>>>>
>>>>> Thoughts?
>>>>
>>>> Well, now that we understand what is causing the throughput regression=
s,
>>>> fixing them should be fairly straight forward (yeah, famous last words,
>>>> but still...). I already have a patch for the fast path and will go po=
ke
>>>> at the slow path next. It'll probably require another workaround or tw=
o,
>>>> so I guess it won't be the architecturally clean ideal solution; but it
>>>> would make it possible to have something that works for 4.9 and then
>>>> iterate for a cleaner design for 4.10.
>>>
>>> But if we try to rush this to 4.9 it won't be in linux-next for long. We
>>> are now in -rc3 and let's say that the patches are ready to apply in two
>>> weeks. That would leave us only two weeks of -next time before the merge
>>> window, which I think is not enough for a controversial patch like this
>>> one. There might be other bugs lurking which haven't been found yet.
>>
>> What, other hidden bugs? Unpossible! :)
>
> Yeah, right ;)
>
>> Would it be possible to merge the partial solution (which is ready now,
>> basically) and fix the slow path in a separate patch later?
>
> What do you mean with partial solution? You mean ath9k users would
> suffer from regressions until they are fixed? We can't do that.
>
>> (Just spit-balling here; I'm still fairly new to this process. But I am
>> concerned that we'll hit a catch-22 where we can't get wider testing
>> before it's "ready" and we can't prove that it's "ready" until we've had
>> wider testing...)
>
> I understand your point, but I don't want to rush this to 4.9 and then
> start getting lots of bug reports and eventually forced to revert it. If
> we just found a new serious regression the chances are that there are
> more lurking somewhere and this patch is just not ready yet.
So, the changes to mac80211 that fixes the known regressions of this
patch have gone in. Any chance of seeing this merged during the current
merge window? :)
-Toke
^ 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