* Re: TCP data throughput for BCM43362
From: Arend Van Spriel @ 2016-09-14 18:13 UTC (permalink / raw)
To: Jörg Krause, Franky Lin
Cc: Brett Rudley, brcm80211-dev-list, Hante Meuleman, Franky Lin,
linux-wireless, Arend van Spriel
In-Reply-To: <1473860503.19492.19.camel@embedded.rocks>
On 14-9-2016 15:41, Jörg Krause wrote:
> Hi,
>
> On Mon, 2016-08-29 at 23:15 +0200, Jörg Krause wrote:
>> On Mi, 2016-08-24 at 20:35 +0200, Arend Van Spriel wrote:
>>>
>>> On 22-8-2016 15:37, Jörg Krause wrote:
>>>>
>>>>
>>>> Hi all,
>>>>
>>>> I am back from vacation and I'd like to do more investigations
>>>> about
>>>> this issue. Please see my comments below...
>>>>
>>>> On Sun, 2016-08-07 at 13:41 +0200, Arend van Spriel wrote:
>>>>>
>>>>>
>>>>> On 06-08-16 16:12, Jörg Krause wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hi all,
>>>>>
>>>>> A bit weird email format making it a bit hard to determine
>>>>> where
>>>>> your
>>>>> last reply starts...
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Fr, 2016-08-05 at 17:56 -0700, Franky Lin wrote:
>>>>>>
>>>>>> On Fri, Aug 5, 2016 at 2:29 PM, Jörg Krause <joerg.krause@emb
>>>>>> ed
>>>>>> ded.
>>>>>> ro
>>>>>> cks>
>>>>>> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Am 5. August 2016 23:01:10 MESZ, schrieb Arend Van Spriel <
>>>>>> arend.vanspriel@broadcom.com>:
>>>>>>
>>>>>>
>>>>>> Op 5 aug. 2016 22:46 schreef "Jörg Krause"
>>>>>> <joerg.krause@embedded.rocks>:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> I'm using a custom ARM board with an BCM43362 wifi chip from
>>>>>>
>>>>>> Broadcom.
>>>>>>
>>>>>>
>>>>>> The wifi chip is attached via SDIO to the controller with a
>>>>>> clock of
>>>>>> 48MHz. Linux kernel version is 4.7.
>>>>>>
>>>>>> When measuring the network bandwidth with iperf3 I get a
>>>>>> bandwith of
>>>>>> only around 5 Mbps. I found a similar thread at the Broadcom
>>>>>>
>>>>>> community
>>>>>>
>>>>>>
>>>>>> [1] where the test was done with a M4 CPU + BCM43362 and an
>>>>>> average
>>>>>> result of 3.3 Mbps.
>>>>>>
>>>>>> Interestingly, a BCM43362 Wi-Fi Dev Kit [2] notes a TCP data
>>>>>>
>>>>>> throughput
>>>>>>
>>>>>>
>>>>>> greater than 20 Mbps.
>>>>>>
>>>>>> Why is the throughput I measured much lower? Note that I
>>>>>> measured
>>>>>> several times with almost no neighbor devices or networks.
>>>>>>
>>>>>> This is a test sample measured with iperf3:
>>>>>>
>>>>>> $ iperf3 -c 192.168.2.1 -i 1 -t 10
>>>>>> Connecting to host 192.168.2.1, port 5201
>>>>>> [ 4] local 192.168.2.155 port 36442 connected to
>>>>>> 192.168.2.1
>>>>>>
>>>>>> port
>>>>>>
>>>>>>
>>>>>> 5201
>>>>>> [ ID]
>>>>>> Interval Transfer Bandwidth Retr Cwnd
>>>>>> [ 4] 0.00-1.00 sec 615 KBytes 5.04
>>>>>> Mbits/sec 0 56.6
>>>>>> KBytes
>>>>>> [ 4] 1.00-2.00 sec 622 KBytes 5.10
>>>>>> Mbits/sec 0 84.8
>>>>>> KBytes
>>>>>> [ 4] 2.00-3.00 sec 625 KBytes 5.12
>>>>>> Mbits/sec 0 113
>>>>>> KBytes
>>>>>> [ 4] 3.00-4.00 sec 571 KBytes 4.68
>>>>>> Mbits/sec 0 140
>>>>>> KBytes
>>>>>> [ 4] 4.00-5.00 sec 594 KBytes 4.87
>>>>>> Mbits/sec 0 167
>>>>>> KBytes
>>>>>> [ 4] 5.00-6.00 sec 628 KBytes 5.14
>>>>>> Mbits/sec 0 195
>>>>>> KBytes
>>>>>> [ 4] 6.00-7.00 sec 619 KBytes 5.07
>>>>>> Mbits/sec 0 202
>>>>>> KBytes
>>>>>> [ 4] 7.00-8.00 sec 608 KBytes 4.98
>>>>>> Mbits/sec 0 202
>>>>>> KBytes
>>>>>> [ 4] 8.00-9.00 sec 602 KBytes 4.93
>>>>>> Mbits/sec 0 202
>>>>>> KBytes
>>>>>> [ 4] 9.00-10.00 sec 537 KBytes 4.40
>>>>>> Mbits/sec 0 202
>>>>>> KBytes
>>>>>> - - - - - - - - - - - - - - - - - - - - - - - - -
>>>>>> [ ID]
>>>>>> Interval Transfer Bandwidth Retr
>>>>>> [ 4] 0.00-10.00 sec 5.88 MBytes 4.93
>>>>>> Mbits/sec 0 sender
>>>>>> [ 4] 0.00-10.00 sec 5.68 MBytes 4.76
>>>>>> Mbits/sec receiver
>>>>>>
>>>>>>
>>>>>> Not overly familiar with iperf3. Do these lines mean you are
>>>>>> doing
>>>>>> bidirectional test, ie. upstream and downstream at the same
>>>>>> time.
>>>>>> Another
>>>>>> thing affecting tput could be power-save.
>>>>>>
>>>>>>
>>>>>> No, iperf3 does not support bidrectional test. Power-save is
>>>>>> turned
>>>>>> off.
>>>>>>
>>>>>> What does iw link say?
>>>>>>
>>>>>
>>>>> but I guess it starts here!
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> I compared the results with a Cubietruck I have:
>>>>>>
>>>>>> # iperf3 -s
>>>>>> -----------------------------------------------------------
>>>>>> Server listening on 5201
>>>>>> -----------------------------------------------------------
>>>>>> Accepted connection from 192.168.178.46, port 42906
>>>>>> [ 5] local 192.168.178.38 port 5201 connected to
>>>>>> 192.168.178.46
>>>>>> port
>>>>>> 42908
>>>>>> [ ID] Interval Transfer Bandwidth
>>>>>> [ 5] 0.00-1.00 sec 2.29 MBytes 19.2
>>>>>> Mbits/sec
>>>>>> [ 5] 1.00-2.00 sec 2.21 MBytes 18.5
>>>>>> Mbits/sec
>>>>>> [ 5] 2.00-3.00 sec 2.17 MBytes 18.2
>>>>>> Mbits/sec
>>>>>> [ 5] 3.00-4.00 sec 2.09 MBytes 17.6
>>>>>> Mbits/sec
>>>>>> [ 5] 4.00-5.00 sec 2.20 MBytes 18.5
>>>>>> Mbits/sec
>>>>>> [ 5] 5.00-6.00 sec 2.64 MBytes 22.1
>>>>>> Mbits/sec
>>>>>> [ 5] 6.00-7.00 sec 2.67 MBytes 22.4
>>>>>> Mbits/sec
>>>>>> [ 5] 7.00-8.00 sec 2.62 MBytes 22.0
>>>>>> Mbits/sec
>>>>>> [ 5] 8.00-9.00 sec 2.35 MBytes 19.8
>>>>>> Mbits/sec
>>>>>> [ 5] 9.00-10.00 sec 2.30 MBytes 19.3
>>>>>> Mbits/sec
>>>>>> [ 5] 10.00-10.03 sec 83.4 KBytes 23.5
>>>>>> Mbits/sec
>>>>>> - - - - - - - - - - - - - - - - - - - - - - - - -
>>>>>> [ ID] Interval Transfer Bandwidth Retr
>>>>>> [ 5] 0.00-10.03 sec 23.9 MBytes 20.0
>>>>>> Mbits/sec 0 sender
>>>>>> [ 5] 0.00-10.03 sec 23.6 MBytes 19.8
>>>>>> Mbits/sec receiver
>>>>>>
>>>>>> # iw dev wlan0 link
>>>>>> Connected to xx:xx:xx:xx:xx (on wlan0)
>>>>>> SSID: xxx
>>>>>> freq: 2437
>>>>>> tx bitrate: 65.0 MBit/s
>>>>>>
>>>>>> bss flags: short-preamble short-slot-time
>>>>>> dtim period: 1
>>>>>> beacon int: 100
>>>>>
>>>>> Too bad RSSI is not in the output above. That may be due to a
>>>>> regression
>>>>> in our driver which has been fixed by commit 94abd778a7bb
>>>>> ("brcmfmac:
>>>>> add fallback for devices that do not report per-chain values").
>>>>> However,
>>>>> the tx bitrate seems within the same range as the other
>>>>> platform.
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> The Cubietruck works also with the brcmfmac driver.
>>>>>>
>>>>>> May it depend on the NVRAM file?
>>>>>
>>>>> Not sure. Can you tell me a bit more about the custom ARM
>>>>> board.
>>>>> Does
>>>>> it
>>>>> use the same wifi module as Cubietruck, ie. the AMPAK AP6210?
>>>>> If
>>>>> you
>>>>> can
>>>>> make a wireshark sniff we can check the actual bitrate and
>>>>> medium
>>>>> density in terms of packets. Another thing to look at is the
>>>>> SDIO
>>>>> host
>>>>> controller. In brcmf_sdiod_sgtable_alloc() some key values are
>>>>> used
>>>>> from
>>>>> the host controller. It only logs the number of entries of the
>>>>> scatter-gather table, but could you add the other values in
>>>>> this
>>>>> function that are used to determine the number of entries.
>>>>
>>>> My board uses the BCM43362 chip solely (no Bluetooth) attached to
>>>> the
>>>> SDIO interface of a NXP i.MX28 processor.
>>>>
>>>> I added some additional printk() to brcmf_sdiod_sgtable_alloc().
>>>> These
>>>> are the values printed after modprobe brcmfmac:
>>>>
>>>> [ 8.926657] sg_support=1
>>>> [ 8.929440] max_blocks=511
>>>> [ 8.932213] max_request_size=261632
>>>> [ 8.935741] max_segment_count=52
>>>> [ 8.939005] max_segment_size=65280
>>>> [ 8.946095] nents=35
>>>
>>> Thanks. That looks good.
>>>
>>>>
>>>>
>>>> Additionally I attached a xz compresses wireshark sniff while
>>>> running
>>>> iper3 between the BCM43362 running as in AP mode with iperf3 as a
>>>> server and a PC in station mode running iperf3 as a client.
>>>
>>> Looking at the sniff it seems you captured on the ethernet side.
>>> That
>>> does not give me any 802.11 specific info. Can you make a wireless
>>> capture preferably without encryption.
>>
>> You,re right! Sorry for this mistake. I did a re-capture on the
>> wireless side now.
>
> Anything new about this? Anything I can do to help?
I missed your previous email. Was already wondering whether to ping you.
Digging around in my email folders I found it so will take a look at it.
Regards,
Arend
^ permalink raw reply
* Re: [PATCH 3/5] wlcore: sdio: Populate config firmware data
From: Tony Lindgren @ 2016-09-14 17:13 UTC (permalink / raw)
To: Kalle Valo
Cc: Eyal Reizer, Guy Mishol, Maital Hahn, Maxim Altshul,
Shahar Patury, linux-wireless, linux-omap
In-Reply-To: <87intycsta.fsf@kamboji.qca.qualcomm.com>
* Kalle Valo <kvalo@codeaurora.org> [160914 10:09]:
> Tony Lindgren <tony@atomide.com> writes:
>
> > Configure the config firmware names and make it available
> > in platform data.
> >
> > Let's also fix the order of the struct wilink_family_data
> > while at it.
> >
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > ---
> > drivers/net/wireless/ti/wlcore/spi.c | 42 ++++++++++++++++++++----------------
> > 1 file changed, 24 insertions(+), 18 deletions(-)
>
> I think the title should be:
>
> wlcore: spi: Populate config firmware data
>
> I can fix that during commit.
Oops sorry shift+Insert caused typo :)
Tony
^ permalink raw reply
* Re: [PATCH 3/5] wlcore: sdio: Populate config firmware data
From: Kalle Valo @ 2016-09-14 17:08 UTC (permalink / raw)
To: Tony Lindgren
Cc: Eyal Reizer, Guy Mishol, Maital Hahn, Maxim Altshul,
Shahar Patury, linux-wireless, linux-omap
In-Reply-To: <20160913215043.29454-4-tony@atomide.com>
Tony Lindgren <tony@atomide.com> writes:
> Configure the config firmware names and make it available
> in platform data.
>
> Let's also fix the order of the struct wilink_family_data
> while at it.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> drivers/net/wireless/ti/wlcore/spi.c | 42 ++++++++++++++++++++----------------
> 1 file changed, 24 insertions(+), 18 deletions(-)
I think the title should be:
wlcore: spi: Populate config firmware data
I can fix that during commit.
--
Kalle Valo
^ permalink raw reply
* Re: [1/2] mwifiex: Command 7 handling for USB chipsets
From: Kalle Valo @ 2016-09-14 17:03 UTC (permalink / raw)
To: Amitkumar Karwar
Cc: linux-wireless, Cathy Luo, Nishant Sarmukadam, Ganapathi Bhat,
Amitkumar Karwar
In-Reply-To: <1473686728-8101-2-git-send-email-akarwar@marvell.com>
Amitkumar Karwar <akarwar@marvell.com> wrote:
> From: Ganapathi Bhat <gbhat@marvell.com>
>
> Firmware image for newer USB chipsets starts with a command 7 block
> (special command). It doesn't contain data length field. This patch adds
> necessary handling.
>
> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Thanks, 2 patches applied to wireless-drivers-next.git:
787764676f94 mwifiex: Command 7 handling for USB chipsets
b7450e248d71 mwifiex: firmware name correction for usb8997 chipset
--
Sent by pwcli
https://patchwork.kernel.org/patch/9326515/
^ permalink raw reply
* Re: [06/26] ath: constify local structures
From: Kalle Valo @ 2016-09-14 17:02 UTC (permalink / raw)
To: Julia Lawall
Cc: Luis R. Rodriguez, joe, kernel-janitors, linux-wireless, netdev,
linux-kernel
In-Reply-To: <1473599168-30561-7-git-send-email-Julia.Lawall@lip6.fr>
Julia Lawall <Julia.Lawall@lip6.fr> 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>
Thanks, 3 patches applied to wireless-drivers-next.git:
8136fd58ad60 ath: constify local structures
1dc80798a8ca iwlegacy: constify local structures
d86e64768859 rtlwifi: rtl818x: constify local structures
--
Sent by pwcli
https://patchwork.kernel.org/patch/9325363/
^ permalink raw reply
* Re: [1/2] rtl8xxxu: Reset device on module unload if still attached
From: Kalle Valo @ 2016-09-14 17:01 UTC (permalink / raw)
To: Jes Sorensen; +Cc: linux-wireless, Jes Sorensen
In-Reply-To: <1473444085-6363-2-git-send-email-Jes.Sorensen@redhat.com>
Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> If the USB dongle is still attached, reset it on module unload to
> avoid scans failing when reloading the driver.
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
Thanks, 2 patches applied to wireless-drivers-next.git:
54cdf5c727cb rtl8xxxu: Reset device on module unload if still attached
0cd7f70399f7 rtl8xxxu: fix spelling mistake "firmare" -> "firmware"
--
Sent by pwcli
https://patchwork.kernel.org/patch/9324069/
^ permalink raw reply
* Re: mwifiex: correction in Rx STBC field of htcapinfo
From: Kalle Valo @ 2016-09-14 17:00 UTC (permalink / raw)
To: Amitkumar Karwar; +Cc: linux-wireless, Nishant Sarmukadam, Amitkumar Karwar
In-Reply-To: <1473432979-16554-1-git-send-email-akarwar@marvell.com>
Amitkumar Karwar <akarwar@marvell.com> wrote:
> Currently Rx STBC in assoc request frame is advertised as 3. It should
> be 2, as our chipsets support two spatial streams.
>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Thanks, 1 patch applied to wireless-drivers-next.git:
ae1799a1cb13 mwifiex: correction in Rx STBC field of htcapinfo
--
Sent by pwcli
https://patchwork.kernel.org/patch/9323739/
^ permalink raw reply
* Re: [v2] mwifiex: handle error if IRQ request fails in mwifiex_sdio_of()
From: Kalle Valo @ 2016-09-14 17:00 UTC (permalink / raw)
To: Amitkumar Karwar; +Cc: linux-wireless, Amitkumar Karwar
In-Reply-To: <1473418678-14794-1-git-send-email-akarwar@marvell.com>
Amitkumar Karwar <akarwar@marvell.com> wrote:
> When this failure occurs, we will clear card->plt_wake_cfg so that
> device would initialize without wake up on external interrupt feature.
> This feature specific code in suspend and resume handlers will be
> skipped.
>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
Thanks, 1 patch applied to wireless-drivers-next.git:
6b03144d93fc mwifiex: handle error if IRQ request fails in mwifiex_sdio_of()
--
Sent by pwcli
https://patchwork.kernel.org/patch/9322927/
^ permalink raw reply
* Re: [PATCH 2/2] mwifiex: firmware name correction for usb8997 chipset
From: Kalle Valo @ 2016-09-14 16:59 UTC (permalink / raw)
To: Amitkumar Karwar
Cc: linux-wireless, Cathy Luo, Nishant Sarmukadam, Ganapathi Bhat
In-Reply-To: <1473686728-8101-3-git-send-email-akarwar@marvell.com>
Amitkumar Karwar <akarwar@marvell.com> writes:
> From: Ganapathi Bhat <gbhat@marvell.com>
>
> Similar to pcie8997 chipset, first firmware submitted for usb8997
> chipset will be usbusb8997_combo_v4.bin. This patch corrects the
> name used in driver.
>
> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> ---
> drivers/net/wireless/marvell/mwifiex/usb.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/usb.h b/drivers/net/wireless/marvell/mwifiex/usb.h
> index 1b49c52..30e8eb8 100644
> --- a/drivers/net/wireless/marvell/mwifiex/usb.h
> +++ b/drivers/net/wireless/marvell/mwifiex/usb.h
> @@ -46,7 +46,7 @@
> #define USB8766_DEFAULT_FW_NAME "mrvl/usb8766_uapsta.bin"
> #define USB8797_DEFAULT_FW_NAME "mrvl/usb8797_uapsta.bin"
> #define USB8801_DEFAULT_FW_NAME "mrvl/usb8801_uapsta.bin"
> -#define USB8997_DEFAULT_FW_NAME "mrvl/usb8997_uapsta.bin"
> +#define USB8997_DEFAULT_FW_NAME "mrvl/usbusb8997_combo_v4.bin"
Like discussed earlier, the firmware names are supposed to be stable. I
consider them to be an interface between kernel and user space. Instead
of changing the driver you should actually rename the firmware file. I'm
planning to take this anyway but in the future please pay extra
attention to do this properly.
My recommendation is to keep the firmware name simple as possible and
get rid of any extra cruft, for example in this case a good name would
be "mrvl/usb8997.bin". That extra "usb" and "combo_v4" don't bring any
benefit.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH v2] mwifiex: cfg80211 set_default_mgmt_key handler
From: Kalle Valo @ 2016-09-14 16:48 UTC (permalink / raw)
To: Amitkumar Karwar; +Cc: linux-wireless, Nishant Sarmukadam, Ganapathi Bhat
In-Reply-To: <1473428038-15308-1-git-send-email-akarwar@marvell.com>
Amitkumar Karwar <akarwar@marvell.com> writes:
> From: Ganapathi Bhat <gbhat@marvell.com>
>
> Previously device used to start using IGTK key as Tx key as soon as it
> gets downloaded in add_key(). This patch implements set_default_mgmt_key
> handler. We will update Tx key ID in set_default_mgmt_key().
>
> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> ---
> Changes in v2: v1 had a dummy handler. v2 addresses a corner case problem
> pointed by Jouni in which AP may send frames encrypted with new key when some
> of the stations are still using old key.
> ---
> drivers/net/wireless/marvell/mwifiex/cfg80211.c | 25 +++++++++++++++++++++++++
> drivers/net/wireless/marvell/mwifiex/fw.h | 1 +
> drivers/net/wireless/marvell/mwifiex/ioctl.h | 1 +
> drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 5 +++++
> 4 files changed, 32 insertions(+)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> index 0a03d3f..33e7a74 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> @@ -484,6 +484,30 @@ mwifiex_cfg80211_add_key(struct wiphy *wiphy, struct net_device *netdev,
> }
>
> /*
> + * CFG802.11 operation handler to set default mgmt key.
> + */
> +static int
> +mwifiex_cfg80211_set_default_mgmt_key(struct wiphy *wiphy,
> + struct net_device *netdev,
> + u8 key_index)
> +{
> + struct mwifiex_private *priv = mwifiex_netdev_get_priv(netdev);
> + struct mwifiex_ds_encrypt_key encrypt_key;
> + const u8 bc_mac[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
This should be static const, but...
> + wiphy_dbg(wiphy, "set default mgmt key, key index=%d\n", key_index);
> +
> + memset(&encrypt_key, 0, sizeof(struct mwifiex_ds_encrypt_key));
> + encrypt_key.key_len = WLAN_KEY_LEN_CCMP;
> + encrypt_key.key_index = key_index;
> + encrypt_key.is_igtk_def_key = true;
> + ether_addr_copy(encrypt_key.mac_addr, bc_mac);
...can't you use eth_broadcast_addr() here?
--
Kalle Valo
^ permalink raw reply
* Re: [2/3,v2] wlcore: Pass win_size taken from ieee80211_sta to FW
From: Kalle Valo @ 2016-09-14 16:41 UTC (permalink / raw)
To: Maxim Altshul; +Cc: linux-wireless, johannes, Maxim Altshul
In-Reply-To: <20160821112425.11302-2-maxim.altshul@ti.com>
Maxim Altshul <maxim.altshul@ti.com> wrote:
> When starting a new BA session, we must pass the win_size to the FW.
>
> To do this we take max_rx_aggregation_subframes (BA RX win size)
> which is stored in ieee80211_sta structure (e.g per link and not per HW)
>
> We will use the value stored per link when passing the win_size to
> firmware through the ACX_BA_SESSION_RX_SETUP command.
>
> Signed-off-by: Maxim Altshul <maxim.altshul@ti.com>
Depends on this commit which is currently in mac80211-next:
480dd46b9d68 mac80211: RX BA support for sta max_rx_aggregation_subframes
--
Sent by pwcli
https://patchwork.kernel.org/patch/9292121/
^ permalink raw reply
* [PATCH] ath10k: Fix spinlock use in coverage class hack
From: Benjamin Berg @ 2016-09-14 16:32 UTC (permalink / raw)
To: Valo, Kalle
Cc: ath10k @ lists . infradead . org, Simon Wunderlich,
Thiagarajan, Vasanthakumar, Sebastian Gottschall, michal.kazior,
Mathias Kretschmer, linux-wireless, Benjamin Berg
In-Reply-To: <8760q0j8st.fsf@kamboji.qca.qualcomm.com>
ath10k_hw_qca988x_set_coverage_class needs to hold both conf_mutex and
the data_lock spin lock for parts of the function. However, data_lock
is only needed while storing the coverage_class to store the value that
the card is configured to.
Fix the locking issue by only holding data_lock for the required duration.
Signed-off-by: Benjamin Berg <benjamin@sipsolutions.net>
---
And yes, I fully agree with your points of it being rather fragile. But as
you said, it should be entirely safe if not used. Obviously a firmware
implementation would be preferential.
This locking issue was pretty unnecessary. Lets see if any more issues show
up in a closer review.
drivers/net/wireless/ath/ath10k/core.h | 2 +-
drivers/net/wireless/ath/ath10k/hw.c | 5 +++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 89b07be..5f8c31f 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -915,7 +915,7 @@ struct ath10k {
struct work_struct set_coverage_class_work;
/* protected by conf_mutex */
struct {
- /* protected by data_lock */
+ /* writing also protected by data_lock */
s16 coverage_class;
u32 reg_phyclk;
diff --git a/drivers/net/wireless/ath/ath10k/hw.c b/drivers/net/wireless/ath/ath10k/hw.c
index e182f09..bd5ca6a 100644
--- a/drivers/net/wireless/ath/ath10k/hw.c
+++ b/drivers/net/wireless/ath/ath10k/hw.c
@@ -243,7 +243,6 @@ static void ath10k_hw_qca988x_set_coverage_class(struct ath10k *ar,
u32 fw_dbglog_level;
mutex_lock(&ar->conf_mutex);
- spin_lock_bh(&ar->data_lock);
/* Only modify registers if the core is started. */
if ((ar->state != ATH10K_STATE_ON) &&
@@ -356,12 +355,14 @@ static void ath10k_hw_qca988x_set_coverage_class(struct ath10k *ar,
store_regs:
/* After an error we will not retry setting the coverage class. */
+ spin_lock_bh(&ar->data_lock);
ar->fw_coverage.coverage_class = value;
+ spin_unlock_bh(&ar->data_lock);
+
ar->fw_coverage.reg_slottime_conf = slottime_reg;
ar->fw_coverage.reg_ack_cts_timeout_conf = timeout_reg;
unlock:
- spin_unlock_bh(&ar->data_lock);
mutex_unlock(&ar->conf_mutex);
}
--
2.9.3
^ permalink raw reply related
* Re: [PATCH v2 03/21] ath10k: Allow changing ath10k debug mask at runtime.
From: Ben Greear @ 2016-09-14 15:33 UTC (permalink / raw)
To: Valo, Kalle; +Cc: ath10k@lists.infradead.org, linux-wireless@vger.kernel.org
In-Reply-To: <871t0mfuej.fsf@kamboji.qca.qualcomm.com>
On 09/14/2016 07:06 AM, Valo, Kalle wrote:
> greearb@candelatech.com writes:
>
>> From: Ben Greear <greearb@candelatech.com>
>>
>> Using debugfs. More convenient than module options
>> in some cases.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>> ---
>> drivers/net/wireless/ath/ath10k/debug.c | 62 +++++++++++++++++++++++++++++++++
>> 1 file changed, 62 insertions(+)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
>> index e251155..d552a4a 100644
>> --- a/drivers/net/wireless/ath/ath10k/debug.c
>> +++ b/drivers/net/wireless/ath/ath10k/debug.c
>> @@ -870,6 +870,65 @@ static const struct file_operations fops_reg_addr = {
>> .llseek = default_llseek,
>> };
>>
>> +static ssize_t ath10k_read_debug_level(struct file *file,
>> + char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + int sz;
>> + const char buf[] =
>> + "To change debug level, set value adding up desired flags:\n"
>> + "PCI: 0x1\n"
>> + "WMI: 0x2\n"
>> + "HTC: 0x4\n"
>> + "HTT: 0x8\n"
>> + "MAC: 0x10\n"
>> + "BOOT: 0x20\n"
>> + "PCI-DUMP: 0x40\n"
>> + "HTT-DUMP: 0x80\n"
>> + "MGMT: 0x100\n"
>> + "DATA: 0x200\n"
>> + "BMI: 0x400\n"
>> + "REGULATORY: 0x800\n"
>> + "TESTMODE: 0x1000\n"
>> + "INFO-AS-DBG: 0x40000000\n"
>> + "FW: 0x80000000\n"
>> + "ALL: 0xFFFFFFFF\n";
>> + char wbuf[sizeof(buf) + 60];
>> + sz = snprintf(wbuf, sizeof(wbuf), "Current debug level: 0x%x\n\n%s",
>> + ath10k_debug_mask, buf);
>> + wbuf[sizeof(wbuf) - 1] = 0;
>> +
>> + return simple_read_from_buffer(user_buf, count, ppos, wbuf, sz);
>> +}
>> +
>> +/* Set logging level.
>> + */
>> +static ssize_t ath10k_write_debug_level(struct file *file,
>> + const char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + struct ath10k *ar = file->private_data;
>> + int ret;
>> + unsigned long mask;
>> +
>> + ret = kstrtoul_from_user(user_buf, count, 0, &mask);
>> + if (ret)
>> + return ret;
>> +
>> + ath10k_warn(ar, "Setting debug-mask to: 0x%lx old: 0x%x\n",
>> + mask, ath10k_debug_mask);
>> + ath10k_debug_mask = mask;
>> + return count;
>> +}
>
> There are already sysfs files for module parameters which seems to work
> just fine for this case:
>
> # echo 0xffffffff > /sys/module/ath10k_core/parameters/debug_mask
Ok, but it is still nice to have the printout info of what log levels
means. Otherwise, you have to go look at firmware source to even know how
to enable the proper flags. And as these flags are internal and might change,
we could change the printout text to match the specific kernel that is running.
In my opinion, this is a 'nice-to-have', not required, if sysfs works,
so I will also test the sysfs suggestion above.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH v2 19/21] ath10k: Enable adhoc mode for CT firmware.
From: Ben Greear @ 2016-09-14 15:28 UTC (permalink / raw)
To: Valo, Kalle; +Cc: linux-wireless@vger.kernel.org, ath10k@lists.infradead.org
In-Reply-To: <877faeeedm.fsf@kamboji.qca.qualcomm.com>
On 09/14/2016 07:37 AM, Valo, Kalle wrote:
> greearb@candelatech.com writes:
>
>> From: Ben Greear <greearb@candelatech.com>
>>
>> CT firmware can support IBSS mode, so allow users to configure this.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>> ---
>> drivers/net/wireless/ath/ath10k/mac.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>> index f1bfb3a..3fc9006 100644
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -7482,6 +7482,10 @@ static const struct ieee80211_iface_limit ath10k_10x_ct_if_limits[] = {
>> .max = 7,
>> .types = BIT(NL80211_IFTYPE_AP)
>> },
>> + {
>> + .max = 1,
>> + .types = BIT(NL80211_IFTYPE_ADHOC)
>> + },
>> };
>>
>> static const struct ieee80211_iface_combination ath10k_if_comb[] = {
>> @@ -7862,6 +7866,7 @@ int ath10k_mac_register(struct ath10k *ar)
>> ar->hw->wiphy->iface_combinations = ath10k_10x_ct_if_comb;
>> ar->hw->wiphy->n_iface_combinations =
>> ARRAY_SIZE(ath10k_10x_ct_if_comb);
>> + ar->hw->wiphy->interface_modes |= BIT(NL80211_IFTYPE_ADHOC);
>> } else {
>> ar->hw->wiphy->iface_combinations = ath10k_10x_if_comb;
>> ar->hw->wiphy->n_iface_combinations =
>
> There should a feature flag ATH10K_FW_FEATURE_SUPPORTS_ADHOC and we use
> that flag as an indication to enable the mode. I wish we had done that
> from the beginning, using wmi_op_version to guess that just creates
> problems :(
No argument there. If you want to add a feature flag I can start setting it in
my firmware.
But, the if-combinations crap is a bigger problem than that anyway. I have
some later patches to re-work it to make it dynamically created instead of static,
but that is a fairly big change and I figured I had a better chance of getting
smaller changes upstream first.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH v2 15/21] ath10k: support CT firmware flag.
From: Ben Greear @ 2016-09-14 15:24 UTC (permalink / raw)
To: Valo, Kalle; +Cc: linux-wireless@vger.kernel.org, ath10k@lists.infradead.org
In-Reply-To: <87bmzqeeox.fsf@kamboji.qca.qualcomm.com>
On 09/14/2016 07:30 AM, Valo, Kalle wrote:
> greearb@candelatech.com writes:
>
>> From: Ben Greear <greearb@candelatech.com>
>>
>> Add placeholder so CT firmware can more easily co-exist with upstream
>> kernel. CT firmware should be backwards compatible with existing kernels,
>> but it also has many new features. Subsequent patches, if acceptable for
>> upstream, can enable and further describe those features.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>> ---
>> drivers/net/wireless/ath/ath10k/core.c | 1 +
>> drivers/net/wireless/ath/ath10k/core.h | 3 +++
>> 2 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
>> index fa71d57..49c85c3 100644
>> --- a/drivers/net/wireless/ath/ath10k/core.c
>> +++ b/drivers/net/wireless/ath/ath10k/core.c
>> @@ -235,6 +235,7 @@ static const char *const ath10k_core_fw_feature_str[] = {
>> [ATH10K_FW_FEATURE_SUPPORTS_ADAPTIVE_CCA] = "adaptive-cca",
>> [ATH10K_FW_FEATURE_MFP_SUPPORT] = "mfp",
>> [ATH10K_FW_FEATURE_PEER_FLOW_CONTROL] = "peer-flow-ctrl",
>> + [ATH10K_FW_FEATURE_WMI_10X_CT] = "wmi-10.x-CT",
>> };
>>
>> static unsigned int ath10k_core_get_fw_feature_str(char *buf,
>> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
>> index cb6aa8c..f9e3b20 100644
>> --- a/drivers/net/wireless/ath/ath10k/core.h
>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>> @@ -566,6 +566,9 @@ enum ath10k_fw_features {
>> */
>> ATH10K_FW_FEATURE_PEER_FLOW_CONTROL = 13,
>>
>> + /* Firmware from Candela Technologies, enables more VIFs, etc */
>> + ATH10K_FW_FEATURE_WMI_10X_CT = 31,
>
> The idea of firmware feature flags to enable (or disable) one particular
> feature, not a group of features. This way it's easy to enable certain
> features on different firmware branches. It also makes the maintenance
> easier as you don't need to remember all the different features one flag
> enables.
One potential use for this flag: Bug reports could be automatically directed to me instead
of QCA.
I have actually re-written much of my earlier logic so that it uses specific
feature flags now. I still like the idea of having the driver (and user)
know it is using CT firmware, but I can live without this flag if needed.
Even if I change generation of my new firmware, all of my older firmware
will have this flag set though, so I would still prefer the flag exist
in the driver, even if it is not used for any serious decisions.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH v2 11/21] ath10k: add fw-powerup-fail to ethtool stats.
From: Ben Greear @ 2016-09-14 15:19 UTC (permalink / raw)
To: Valo, Kalle; +Cc: linux-wireless@vger.kernel.org, ath10k@lists.infradead.org
In-Reply-To: <87fup2eey2.fsf@kamboji.qca.qualcomm.com>
On 09/14/2016 07:25 AM, Valo, Kalle wrote:
> greearb@candelatech.com writes:
>
>> From: Ben Greear <greearb@candelatech.com>
>>
>> This gives user-space a normal-ish way to detect that
>> firmware has failed to start and that a reboot is
>> probably required.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>> ---
>> drivers/net/wireless/ath/ath10k/core.h | 1 +
>> drivers/net/wireless/ath/ath10k/debug.c | 2 ++
>> drivers/net/wireless/ath/ath10k/pci.c | 2 ++
>> 3 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
>> index 6aa7a14..e7c228a 100644
>> --- a/drivers/net/wireless/ath/ath10k/core.h
>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>> @@ -698,6 +698,7 @@ struct ath10k {
>>
>> enum ath10k_hw_rev hw_rev;
>> u16 dev_id;
>> + bool fw_powerup_failed; /* If true, might take reboot to recover. */
>> u32 chip_id;
>> u32 target_version;
>> u8 fw_version_major;
>> diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
>> index 76b5163..05b5ea4 100644
>> --- a/drivers/net/wireless/ath/ath10k/debug.c
>> +++ b/drivers/net/wireless/ath/ath10k/debug.c
>> @@ -1541,6 +1541,7 @@ static const char ath10k_gstrings_stats[][ETH_GSTRING_LEN] = {
>> "d_fw_crash_count",
>> "d_fw_warm_reset_count",
>> "d_fw_cold_reset_count",
>> + "d_fw_powerup_failed", /* boolean */
>> };
>>
>> #define ATH10K_SSTATS_LEN ARRAY_SIZE(ath10k_gstrings_stats)
>> @@ -1640,6 +1641,7 @@ void ath10k_debug_get_et_stats(struct ieee80211_hw *hw,
>> data[i++] = ar->stats.fw_crash_counter;
>> data[i++] = ar->stats.fw_warm_reset_counter;
>> data[i++] = ar->stats.fw_cold_reset_counter;
>> + data[i++] = ar->fw_powerup_failed;
>>
>> spin_unlock_bh(&ar->data_lock);
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
>> index dbf0db8..2adc459 100644
>> --- a/drivers/net/wireless/ath/ath10k/pci.c
>> +++ b/drivers/net/wireless/ath/ath10k/pci.c
>> @@ -2709,10 +2709,12 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)
>> goto err_ce;
>> }
>>
>> + ar->fw_powerup_failed = false;
>> return 0;
>>
>> err_ce:
>> ath10k_pci_ce_deinit(ar);
>> + ar->fw_powerup_failed = true;
>>
>> err_sleep:
>> return ret;
>
> I didn't test myself, but if the firmware boot fails during module load
> we should return an error and the module would fail to load (and hence
> no network interface available). And if the firmware boot fails during
> the interface up call (mac80211 calling ath10k_start()) we should return
> an error and the interface would stay down. IMHO that's enough of
> indications to the user space, I don't see how providing this boolean
> via ethtool stats is really needed.
There are lots of reasons that interfaces can fail to come up or exist.
The work-around for the power-up fail is a system reboot, so I wanted to
know for certain when this case existed so that I could warn users
that they needed a reboot.
That said, I haven't seen this problem in a while, so maybe this patch is
no longer to useful.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH v2 10/21] ath10k: support logging ath10k_info as KERN_DEBUG
From: Ben Greear @ 2016-09-14 15:14 UTC (permalink / raw)
To: Valo, Kalle; +Cc: ath10k@lists.infradead.org, linux-wireless@vger.kernel.org
In-Reply-To: <87k2eeef79.fsf@kamboji.qca.qualcomm.com>
On 09/14/2016 07:19 AM, Valo, Kalle wrote:
> greearb@candelatech.com writes:
>
>> From: Ben Greear <greearb@candelatech.com>
>>
>> Helps keep messages off of (serial) console when
>> that is desired.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>
> Isn't /proc/sys/kernel/print exactly for this purpose? At least I recall
> using it.
I just wanted to hide some ath10k logs from the console, not all system logs. I don't think
that /proc/sys/kernel/print has any granularity?
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH v2 09/21] ath10k: print fw debug messages in hex.
From: Ben Greear @ 2016-09-14 15:13 UTC (permalink / raw)
To: Valo, Kalle; +Cc: linux-wireless@vger.kernel.org, ath10k@lists.infradead.org
In-Reply-To: <87oa3qefac.fsf@kamboji.qca.qualcomm.com>
On 09/14/2016 07:18 AM, Valo, Kalle wrote:
> greearb@candelatech.com writes:
>
>> From: Ben Greear <greearb@candelatech.com>
>>
>> This allows user-space tools to decode debug-log
>> messages by parsing dmesg or /var/log/messages.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>
> Don't tracing points already provide the same information?
Tracing tools are difficult to set up and may not be available on
random embedded devices. And if we are dealing with bug reports from
the field, most users will not be able to set it up regardless.
There are similar ways to print out hex, but the logic below creates
specific and parseable logs in the 'dmesg' output and similar.
I have written a tool that can decode these messages into useful human-readable
text so that I can debug firmware issues both locally and from field reports.
Stock firmware generates similar logs and QCA could write their own decode logic
for their firmware versions.
Thanks,
Ben
>
>> +void ath10k_dbg_print_fw_dbg_buffer(struct ath10k *ar, __le32 *ibuf, int len,
>> + const char* lvl)
>> +{
>> + /* Print out raw hex, external tools can decode if
>> + * they care.
>> + * TODO: Add ar identifier to messages.
>> + */
>> + int q = 0;
>> +
>> + dev_printk(lvl, ar->dev, "ath10k_pci ATH10K_DBG_BUFFER:\n");
>> + while (q < len) {
>> + if (q + 8 <= len) {
>> + printk("%sath10k: [%04d]: %08X %08X %08X %08X %08X %08X %08X %08X\n",
>> + lvl, q,
>> + ibuf[q], ibuf[q+1], ibuf[q+2], ibuf[q+3],
>> + ibuf[q+4], ibuf[q+5], ibuf[q+6], ibuf[q+7]);
>> + q += 8;
>> + }
>> + else if (q + 7 <= len) {
>> + printk("%sath10k: [%04d]: %08X %08X %08X %08X %08X %08X %08X\n",
>> + lvl, q,
>> + ibuf[q], ibuf[q+1], ibuf[q+2], ibuf[q+3],
>> + ibuf[q+4], ibuf[q+5], ibuf[q+6]);
>> + q += 7;
>> + }
>> + else if (q + 6 <= len) {
>> + printk("%sath10k: [%04d]: %08X %08X %08X %08X %08X %08X\n",
>> + lvl, q,
>> + ibuf[q], ibuf[q+1], ibuf[q+2], ibuf[q+3],
>> + ibuf[q+4], ibuf[q+5]);
>> + q += 6;
>> + }
>> + else if (q + 5 <= len) {
>> + printk("%sath10k: [%04d]: %08X %08X %08X %08X %08X\n",
>> + lvl, q,
>> + ibuf[q], ibuf[q+1], ibuf[q+2], ibuf[q+3],
>> + ibuf[q+4]);
>> + q += 5;
>> + }
>> + else if (q + 4 <= len) {
>> + printk("%sath10k: [%04d]: %08X %08X %08X %08X\n",
>> + lvl, q,
>> + ibuf[q], ibuf[q+1], ibuf[q+2], ibuf[q+3]);
>> + q += 4;
>> + }
>> + else if (q + 3 <= len) {
>> + printk("%sath10k: [%04d]: %08X %08X %08X\n",
>> + lvl, q,
>> + ibuf[q], ibuf[q+1], ibuf[q+2]);
>> + q += 3;
>> + }
>> + else if (q + 2 <= len) {
>> + printk("%sath10k: [%04d]: %08X %08X\n",
>> + lvl, q,
>> + ibuf[q], ibuf[q+1]);
>> + q += 2;
>> + }
>> + else if (q + 1 <= len) {
>> + printk("%sath10k: [%04d]: %08X\n",
>> + lvl, q,
>> + ibuf[q]);
>> + q += 1;
>> + }
>> + else {
>> + break;
>> + }
>> + }/* while */
>> +
>> + dev_printk(lvl, ar->dev, "ATH10K_END\n");
>> +}
>
> Isn't this almost the same as what ath10k_dbg_dump() does?
>
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH v2 08/21] ath10k: make firmware text debug messages more verbose.
From: Ben Greear @ 2016-09-14 15:06 UTC (permalink / raw)
To: Valo, Kalle; +Cc: linux-wireless@vger.kernel.org, ath10k@lists.infradead.org
In-Reply-To: <87sht2efiu.fsf@kamboji.qca.qualcomm.com>
On 09/14/2016 07:12 AM, Valo, Kalle wrote:
> greearb@candelatech.com writes:
>
>> From: Ben Greear <greearb@candelatech.com>
>>
>> There are not many of these messages producted by the
>> firmware, but they are generally fairly useful, so print
>> them at info level.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>> ---
>> drivers/net/wireless/ath/ath10k/wmi.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
>> index 1758b4a..d9e4b77 100644
>> --- a/drivers/net/wireless/ath/ath10k/wmi.c
>> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
>> @@ -4050,7 +4050,7 @@ void ath10k_wmi_event_debug_print(struct ath10k *ar, struct sk_buff *skb)
>> /* the last byte is always reserved for the null character */
>> buf[i] = '\0';
>>
>> - ath10k_dbg(ar, ATH10K_DBG_WMI_PRINT, "wmi print '%s'\n", buf);
>> + ath10k_info(ar, "wmi print '%s'\n", buf);
>
> Useful to whom and how? I understand that for firmware developers this
> is very valuable information and that's why we have a special debug
> level for it. But I suspect that for normal users these are just
> confusing and unnecessarily spam the log.
CT firmare will print out some memory usage info on firmware boot, and that can
allow a discerning individual to tune their vdev, peer, rate-ctrl, and other
object usage in order to make best use of resources in the firmware.
These few lines can greatly aid debugging certain types of crashes and performance
loss in the firmware, so having them readily available in 'dmesg' or similar
for bug reports from the field helps me.
Stock firmware will also print out some resource usage info. It is just
a few lines on firmware boot, but it is quite useful in my experience.
Thank,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH v2 04/21] ath10k: rate-limit packet tx errors
From: Ben Greear @ 2016-09-14 15:02 UTC (permalink / raw)
To: Valo, Kalle; +Cc: linux-wireless@vger.kernel.org, ath10k@lists.infradead.org
In-Reply-To: <87wpieefr9.fsf@kamboji.qca.qualcomm.com>
On 09/14/2016 07:07 AM, Valo, Kalle wrote:
> greearb@candelatech.com writes:
>
>> From: Ben Greear <greearb@candelatech.com>
>>
>> When firmware crashes, stack can continue to send packets
>> for a bit, and existing code was spamming logs.
>>
>> So, rate-limit the error message for tx failures.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>> ---
>> drivers/net/wireless/ath/ath10k/mac.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>> index cd3016d..42cac32 100644
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -3432,8 +3432,9 @@ static int ath10k_mac_tx_submit(struct ath10k *ar,
>> }
>>
>> if (ret) {
>> - ath10k_warn(ar, "failed to transmit packet, dropping: %d\n",
>> - ret);
>> + if (net_ratelimit())
>> + ath10k_warn(ar, "failed to transmit packet, dropping: %d\n",
>> + ret);
>> ieee80211_free_txskb(ar->hw, skb);
>> }
>
> ath10k_warn() is already rate limited. If there's something wrong then
> that function should be fixed, not the callers.
>
> void ath10k_warn(struct ath10k *ar, const char *fmt, ...)
> {
> struct va_format vaf = {
> .fmt = fmt,
> };
> va_list args;
>
> va_start(args, fmt);
> vaf.va = &args;
> dev_warn_ratelimited(ar->dev, "%pV", &vaf);
> trace_ath10k_log_warn(ar, &vaf);
>
> va_end(args);
> }
The problem with having the ratelimit here is that you may miss
rare warnings due to a flood of common warnings.
That is why it is still useful to ratelimit potential floods
of warnings.
I would like to remove the ratelimit from ath10k_warn eventually.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH v2 12/21] ath10k: Support up to 64 vdevs.
From: Valo, Kalle @ 2016-09-14 15:01 UTC (permalink / raw)
To: greearb@candelatech.com
Cc: ath10k@lists.infradead.org, linux-wireless@vger.kernel.org
In-Reply-To: <1462986153-16318-13-git-send-email-greearb@candelatech.com>
greearb@candelatech.com writes:
> From: Ben Greear <greearb@candelatech.com>
>
> The (1 << x) - 1 trick won't work when you
> are trying to fill up all 64 bits, so add special
> case for that.
>
> And, move the limits to the per-nic structure instead
> of per-driver to allow better dynamic use of the limits.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
> drivers/net/wireless/ath/ath10k/core.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireles=
s/ath/ath10k/core.c
> index 3f1786c..fa71d57 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -1819,7 +1819,10 @@ int ath10k_core_start(struct ath10k *ar, enum ath1=
0k_firmware_mode mode,
> if (status)
> goto err_hif_stop;
> =20
> - ar->free_vdev_map =3D (1LL << ar->max_num_vdevs) - 1;
> + if (ar->max_num_vdevs >=3D 64)
> + ar->free_vdev_map =3D 0xFFFFFFFFFFFFFFFFLL;
> + else
> + ar->free_vdev_map =3D (1LL << ar->max_num_vdevs) - 1;
The last sentence in the commit log doesn't match the code, I removed
that in the pending branch.
--=20
Kalle Valo=
^ permalink raw reply
* Re: [PATCH v2 19/21] ath10k: Enable adhoc mode for CT firmware.
From: Valo, Kalle @ 2016-09-14 14:37 UTC (permalink / raw)
To: greearb@candelatech.com
Cc: ath10k@lists.infradead.org, linux-wireless@vger.kernel.org
In-Reply-To: <1462986153-16318-20-git-send-email-greearb@candelatech.com>
greearb@candelatech.com writes:
> From: Ben Greear <greearb@candelatech.com>
>
> CT firmware can support IBSS mode, so allow users to configure this.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
> drivers/net/wireless/ath/ath10k/mac.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless=
/ath/ath10k/mac.c
> index f1bfb3a..3fc9006 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -7482,6 +7482,10 @@ static const struct ieee80211_iface_limit ath10k_1=
0x_ct_if_limits[] =3D {
> .max =3D 7,
> .types =3D BIT(NL80211_IFTYPE_AP)
> },
> + {
> + .max =3D 1,
> + .types =3D BIT(NL80211_IFTYPE_ADHOC)
> + },
> };
> =20
> static const struct ieee80211_iface_combination ath10k_if_comb[] =3D {
> @@ -7862,6 +7866,7 @@ int ath10k_mac_register(struct ath10k *ar)
> ar->hw->wiphy->iface_combinations =3D ath10k_10x_ct_if_comb;
> ar->hw->wiphy->n_iface_combinations =3D
> ARRAY_SIZE(ath10k_10x_ct_if_comb);
> + ar->hw->wiphy->interface_modes |=3D BIT(NL80211_IFTYPE_ADHOC);
> } else {
> ar->hw->wiphy->iface_combinations =3D ath10k_10x_if_comb;
> ar->hw->wiphy->n_iface_combinations =3D
There should a feature flag ATH10K_FW_FEATURE_SUPPORTS_ADHOC and we use
that flag as an indication to enable the mode. I wish we had done that
from the beginning, using wmi_op_version to guess that just creates
problems :(
--=20
Kalle Valo=
^ permalink raw reply
* Re: [PATCH v2 15/21] ath10k: support CT firmware flag.
From: Valo, Kalle @ 2016-09-14 14:30 UTC (permalink / raw)
To: greearb@candelatech.com
Cc: ath10k@lists.infradead.org, linux-wireless@vger.kernel.org
In-Reply-To: <1462986153-16318-16-git-send-email-greearb@candelatech.com>
greearb@candelatech.com writes:
> From: Ben Greear <greearb@candelatech.com>
>
> Add placeholder so CT firmware can more easily co-exist with upstream
> kernel. CT firmware should be backwards compatible with existing kernels=
,
> but it also has many new features. Subsequent patches, if acceptable for
> upstream, can enable and further describe those features.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
> drivers/net/wireless/ath/ath10k/core.c | 1 +
> drivers/net/wireless/ath/ath10k/core.h | 3 +++
> 2 files changed, 4 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireles=
s/ath/ath10k/core.c
> index fa71d57..49c85c3 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -235,6 +235,7 @@ static const char *const ath10k_core_fw_feature_str[]=
=3D {
> [ATH10K_FW_FEATURE_SUPPORTS_ADAPTIVE_CCA] =3D "adaptive-cca",
> [ATH10K_FW_FEATURE_MFP_SUPPORT] =3D "mfp",
> [ATH10K_FW_FEATURE_PEER_FLOW_CONTROL] =3D "peer-flow-ctrl",
> + [ATH10K_FW_FEATURE_WMI_10X_CT] =3D "wmi-10.x-CT",
> };
> =20
> static unsigned int ath10k_core_get_fw_feature_str(char *buf,
> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireles=
s/ath/ath10k/core.h
> index cb6aa8c..f9e3b20 100644
> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -566,6 +566,9 @@ enum ath10k_fw_features {
> */
> ATH10K_FW_FEATURE_PEER_FLOW_CONTROL =3D 13,
> =20
> + /* Firmware from Candela Technologies, enables more VIFs, etc */
> + ATH10K_FW_FEATURE_WMI_10X_CT =3D 31,
The idea of firmware feature flags to enable (or disable) one particular
feature, not a group of features. This way it's easy to enable certain
features on different firmware branches. It also makes the maintenance
easier as you don't need to remember all the different features one flag
enables.
--=20
Kalle Valo=
^ permalink raw reply
* Re: [PATCH v2 11/21] ath10k: add fw-powerup-fail to ethtool stats.
From: Valo, Kalle @ 2016-09-14 14:25 UTC (permalink / raw)
To: greearb@candelatech.com
Cc: ath10k@lists.infradead.org, linux-wireless@vger.kernel.org
In-Reply-To: <1462986153-16318-12-git-send-email-greearb@candelatech.com>
greearb@candelatech.com writes:
> From: Ben Greear <greearb@candelatech.com>
>
> This gives user-space a normal-ish way to detect that
> firmware has failed to start and that a reboot is
> probably required.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
> drivers/net/wireless/ath/ath10k/core.h | 1 +
> drivers/net/wireless/ath/ath10k/debug.c | 2 ++
> drivers/net/wireless/ath/ath10k/pci.c | 2 ++
> 3 files changed, 5 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireles=
s/ath/ath10k/core.h
> index 6aa7a14..e7c228a 100644
> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -698,6 +698,7 @@ struct ath10k {
> =20
> enum ath10k_hw_rev hw_rev;
> u16 dev_id;
> + bool fw_powerup_failed; /* If true, might take reboot to recover. */
> u32 chip_id;
> u32 target_version;
> u8 fw_version_major;
> diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wirele=
ss/ath/ath10k/debug.c
> index 76b5163..05b5ea4 100644
> --- a/drivers/net/wireless/ath/ath10k/debug.c
> +++ b/drivers/net/wireless/ath/ath10k/debug.c
> @@ -1541,6 +1541,7 @@ static const char ath10k_gstrings_stats[][ETH_GSTRI=
NG_LEN] =3D {
> "d_fw_crash_count",
> "d_fw_warm_reset_count",
> "d_fw_cold_reset_count",
> + "d_fw_powerup_failed", /* boolean */
> };
> =20
> #define ATH10K_SSTATS_LEN ARRAY_SIZE(ath10k_gstrings_stats)
> @@ -1640,6 +1641,7 @@ void ath10k_debug_get_et_stats(struct ieee80211_hw =
*hw,
> data[i++] =3D ar->stats.fw_crash_counter;
> data[i++] =3D ar->stats.fw_warm_reset_counter;
> data[i++] =3D ar->stats.fw_cold_reset_counter;
> + data[i++] =3D ar->fw_powerup_failed;
> =20
> spin_unlock_bh(&ar->data_lock);
> =20
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless=
/ath/ath10k/pci.c
> index dbf0db8..2adc459 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -2709,10 +2709,12 @@ static int ath10k_pci_hif_power_up(struct ath10k =
*ar)
> goto err_ce;
> }
> =20
> + ar->fw_powerup_failed =3D false;
> return 0;
> =20
> err_ce:
> ath10k_pci_ce_deinit(ar);
> + ar->fw_powerup_failed =3D true;
> =20
> err_sleep:
> return ret;
I didn't test myself, but if the firmware boot fails during module load
we should return an error and the module would fail to load (and hence
no network interface available). And if the firmware boot fails during
the interface up call (mac80211 calling ath10k_start()) we should return
an error and the interface would stay down. IMHO that's enough of
indications to the user space, I don't see how providing this boolean
via ethtool stats is really needed.
--=20
Kalle Valo=
^ permalink raw reply
* Re: [PATCH v2 10/21] ath10k: support logging ath10k_info as KERN_DEBUG
From: Valo, Kalle @ 2016-09-14 14:19 UTC (permalink / raw)
To: greearb@candelatech.com
Cc: ath10k@lists.infradead.org, linux-wireless@vger.kernel.org
In-Reply-To: <1462986153-16318-11-git-send-email-greearb@candelatech.com>
greearb@candelatech.com writes:
> From: Ben Greear <greearb@candelatech.com>
>
> Helps keep messages off of (serial) console when
> that is desired.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
Isn't /proc/sys/kernel/print exactly for this purpose? At least I recall
using it.
--=20
Kalle Valo=
^ 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