Re: [RFC v4 06/21] ath10k: sdio support

[Valo, Kalle]

"Valo, Kalle" <kv...@qca.qualcomm.com> writes:

> Erik Stromdahl <erik.stromd...@gmail.com> writes:
>
>> sdio/mailbox HIF implementation.
>>
>> Signed-off-by: Erik Stromdahl <erik.stromd...@gmail.com>
>
> I'm looking at this more carefully now and noticed this:
>
>> +static int ath10k_sdio_bmi_credits(struct ath10k *ar)
>> +{
>> +int ret;
>> +u32 addr, *cmd_credits;
>> +unsigned long timeout;
>> +
>> +cmd_credits = kzalloc(sizeof(*cmd_credits), GFP_KERNEL);
>> +if (!cmd_credits) {
>> +ret = -ENOMEM;
>> +goto err;
>> +}
>> +
>> +/* Read the counter register to get the command credits */
>> +addr = MBOX_COUNT_DEC_ADDRESS + ATH10K_HIF_MBOX_NUM_MAX * 4;
>> +
>> +timeout = jiffies + BMI_COMMUNICATION_TIMEOUT_HZ;
>> +while (time_before(jiffies, timeout) && !*cmd_credits) {
>> +/* Hit the credit counter with a 4-byte access, the first byte
>> + * read will hit the counter and cause a decrement, while the
>> + * remaining 3 bytes has no effect. The rationale behind this
>> + * is to make all HIF accesses 4-byte aligned.
>> + */
>> +ret = ath10k_sdio_read_write_sync(ar, addr,
>> +  (u8 *)cmd_credits,
>> +  sizeof(*cmd_credits),
>> +  HIF_RD_SYNC_BYTE_INC);
>> +if (ret) {
>> +ath10k_warn(ar,
>> + "Unable to decrement the command credit count register: %d\n",
>> +ret);
>> +goto err_free;
>> +}
>> +
>> +/* The counter is only 8 bits.
>> + * Ignore anything in the upper 3 bytes
>> + */
>> +*cmd_credits &= 0xFF;
>> +}
>> +
>> +if (!*cmd_credits) {
>> +ath10k_warn(ar, "bmi communication timeout\n");
>> +ret = -ETIMEDOUT;
>> +goto err_free;
>> +}
>> +
>> +return 0;
>> +err_free:
>> +kfree(cmd_credits);
>> +err:
>> +return ret;
>> +}
>
> AFAICS we are leaking cmd_credits if there's no error. Or is the buffer
> freed somewhere within the mmc stack or something? The reason why I ask
> is that I saw the same pattern in multiple functions so I'm curious.

Also I'm worried about endianness. We are reading from the device
directly to an u32 variable and not converting the bytes. Is the MMC
subsystem doing the conversion, I guess not?

-- 
Kalle Valo

Re: [RFC v4 06/21] ath10k: sdio support

[Valo, Kalle]

Erik Stromdahl  writes:

> sdio/mailbox HIF implementation.
>
> Signed-off-by: Erik Stromdahl 

I'm looking at this more carefully now and noticed this:

> +static int ath10k_sdio_bmi_credits(struct ath10k *ar)
> +{
> + int ret;
> + u32 addr, *cmd_credits;
> + unsigned long timeout;
> +
> + cmd_credits = kzalloc(sizeof(*cmd_credits), GFP_KERNEL);
> + if (!cmd_credits) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + /* Read the counter register to get the command credits */
> + addr = MBOX_COUNT_DEC_ADDRESS + ATH10K_HIF_MBOX_NUM_MAX * 4;
> +
> + timeout = jiffies + BMI_COMMUNICATION_TIMEOUT_HZ;
> + while (time_before(jiffies, timeout) && !*cmd_credits) {
> + /* Hit the credit counter with a 4-byte access, the first byte
> +  * read will hit the counter and cause a decrement, while the
> +  * remaining 3 bytes has no effect. The rationale behind this
> +  * is to make all HIF accesses 4-byte aligned.
> +  */
> + ret = ath10k_sdio_read_write_sync(ar, addr,
> +   (u8 *)cmd_credits,
> +   sizeof(*cmd_credits),
> +   HIF_RD_SYNC_BYTE_INC);
> + if (ret) {
> + ath10k_warn(ar,
> + "Unable to decrement the command credit 
> count register: %d\n",
> + ret);
> + goto err_free;
> + }
> +
> + /* The counter is only 8 bits.
> +  * Ignore anything in the upper 3 bytes
> +  */
> + *cmd_credits &= 0xFF;
> + }
> +
> + if (!*cmd_credits) {
> + ath10k_warn(ar, "bmi communication timeout\n");
> + ret = -ETIMEDOUT;
> + goto err_free;
> + }
> +
> + return 0;
> +err_free:
> + kfree(cmd_credits);
> +err:
> + return ret;
> +}

AFAICS we are leaking cmd_credits if there's no error. Or is the buffer
freed somewhere within the mmc stack or something? The reason why I ask
is that I saw the same pattern in multiple functions so I'm curious.

-- 
Kalle Valo

Re: ath6kl: Remove old 802.11a-only channels

[Valo, Kalle]

Rostyslav Khudolii  writes:

> Following our conversation ("AR6003 5.2GHz AP on channel 38") on the
> mailing list, sending a patch which removes channels 34/38/42/46 from
> ath6kl driver.

No attachments, please. The best is to use 'git send-email' to submit
patches.

Also read the documentation as Signed-off-by line is missing.

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

-- 
Kalle Valo

Re: [OpenWrt-Devel] ATH10K VLAN firmware issue

[Valo, Kalle]

voncken  writes:

> Do you know if the firmware team planned to fix the VLAN issue on ath10k
> firmware?

I reported it forward only this week.

-- 
Kalle Valo

Re: [RFC v3 0/8] ath10k sdio support

[Valo, Kalle]

Erik Stromdahl  writes:

> Ok, I'll do another round of checkpatch before I submit anything.
> I couldn't find the script you mentioned though (ath10k-check).

Did you check the link I gave you:

https://wireless.wiki.kernel.org/en/users/drivers/ath10k/codingstyle#checking_code

> Is it some kind of checkpatch wrapper?

It runs various tests (gcc, sparse, checkpatch), sets some checkpatch
settings (like line length) and filters out warnings we don't care
about.

> Anyway, I have a few warnings related to 'line over 80 chars' that
> is really hard to get rid of (without breaking indentation etc.) so
> I won't do anything about those for now.
>
> Then there are some other warnings about the BIT macro being preferred
> over (1 << x). I have used (1 << x) in some files despite the checkpatch
> warning in order to keep the patches consistent with the existing code.
> I think the best approach is to have a separate round of cleanup-patches
> replacing all (1 << x) with BIT(x).

These are all disabled by ath10k-check. I think it's easiest that you
forget ath10k-check for now and let me fix those in the next round.

-- 
Kalle Valo

Re: [RFC v3 0/8] ath10k sdio support

[Valo, Kalle]

Erik Stromdahl  writes:

> I just noticed that Ryan Hsu submitted two patches containing similar
> functionality to what I have in my tree on github. They have not been
> integrated in the ath master yet, but they have been added to master-pending.
>
> Below are the commits I am talking about
>
> bc1054a15be3c962c83aa22476a3e9c1266da792 ath10k: improve the firmware
> download time for QCA9377
> db5af2a25bd8c1410ec4456d1d4f4e6ded2649ca ath10k: improve the firmware
> download time for QCA6174
>
> Since some of my patches overlap with these I think it would be best if
> I add these to my tree and rewrite my patches.
>
> If they haven't made it into master before I submit my updated patch series
> I will include them in my set.
>
> Is this OK?
>
> If they will be added to master soon there is no issue since I will of course
> rebase to the latest ath-MMDDHHmm before I submit anything.

That's ok, just mention this in the cover letter.

Even better is that if you can use master-pending from ath.git as the
baseline (compared to the master branch normally recommended), Ryan's
patches are there now. That way you don't need to apply anything extra
to your tree. But I'm hoping to apply Ryan's patches soon anyway.

-- 
Kalle Valo

Re: [RFC v3 0/8] ath10k sdio support

[Valo, Kalle]

Kalle Valo  writes:

> Erik Stromdahl  writes:
>
>> I was actually about to email you about this.
>>
>> I have made a few more updates to the sdio code so I think it would be
>> best if I could submit a new series of patches based on this code (v4).
>>
>> Then you can tweak it (v5).
>>
>> It is only minor updates to the HIF layer (added QCA9377 support) and
>> a setup of some pll registers.
>
> Ok, that sounds good. I'll wait for v4.

Forgot to mention can you run ath10k-check before submitting them, it
shows few problems:

https://wireless.wiki.kernel.org/en/users/drivers/ath10k/codingstyle#checking_code

But if you wonder what this is about it's checkpatch warning about camel
case (doesn't like the lower case x character):

drivers/net/wireless/ath/ath10k/sdio.h:68: 

I guess I have a bug in the script when it parses checkpatch output.

-- 
Kalle Valo

Re: [RFC v3 0/8] ath10k sdio support

[Valo, Kalle]

Erik Stromdahl  writes:

> I was actually about to email you about this.
>
> I have made a few more updates to the sdio code so I think it would be
> best if I could submit a new series of patches based on this code (v4).
>
> Then you can tweak it (v5).
>
> It is only minor updates to the HIF layer (added QCA9377 support) and
> a setup of some pll registers.

Ok, that sounds good. I'll wait for v4.

> btw, should I still mark them as RFC or should it be PATCH this time?
>
> If I go for PATCH, should the version be v4 or should I start from v1?

v4 would be fine. Thanks.

-- 
Kalle Valo

Re: [RFC v3 0/8] ath10k sdio support

[Valo, Kalle]

Hej,

Erik Stromdahl  writes:

> This is the third version of the sdio RFC patch series.
> The actual sdio code (patch 6) has been subject to a massive overhaul,
> mainly as a result of Kalle's review comments.
> It no longer has any strong resemblance of the original ath6kl code from
> which it was originally based upon.
>
> Previous pathes 6 to 10 have been merged into one single patch.
>
> The previous series had a rework of the "HTC fake service" connect
> (ep 0 connect) that introduced a race between the actual endpoint
> connect and the HTC ready message. This issue has been addressed,
> and the current patches (3 and 4) has been rewritten accordingly.
>
> * overview of patches *
>
> Patches 1 to 4 are more or less identical to the previous RFC, with an
> exception for patch 3 that changes the "HTC fake service" connect
> (mentioned above).
>
> Patch 5 is a squashed version of previous patches 6 to 10.
>
> Patch 6 is the actual sdio patch
>
> Patches 7 to 8 are new and adds special sdio versions of BMI get
> target info and HTC ready.
>
> The new version was built and tested against:
> tag: ath-201701121109
>
> Erik Stromdahl (8):
>   ath10k: htc: made static function public
>   ath10k: htc: rx trailer lookahead support
>   ath10k: htc: move htc ctrl ep connect to htc_init
>   ath10k: htc: refactorization
>   ath10k: various sdio related definitions
>   ath10k: sdio support
>   ath10k: sdio get target info
>   ath10k: htc: ready_ext msg support

Sorry for not getting back to you earlier, haven't found time to look
this in detail during the last few weeks.

This is looking quite good now. I have some nitpicks (build warnings,
maybe reorder some patches etc) still but I think it's faster if I fix
those and send v4 as a proper patch (no RFC anymore), naturally
attributing you as the author. Is that ok for you?

-- 
Kalle Valo

Re: [PATCH v3] ath10k: add support for controlling tx power to a station

[Valo, Kalle]

kbuild test robot  writes:

> Hi Ashok,
>
> [auto build test ERROR on v4.9-rc8]
> [cannot apply to ath6kl/ath-next next-20170210]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
>
> url:
> https://github.com/0day-ci/linux/commits/Ashok-Raj-Nagarajan/ath10k-add-support-for-controlling-tx-power-to-a-station/20170208-203305
> config: x86_64-randconfig-in0-02120025 (attached as .config)
> compiler: gcc-4.6 (Debian 4.6.4-7) 4.6.4
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64 
>
> All errors (new ones prefixed by >>):
>
>drivers/net/wireless/ath/ath10k/mac.c: In function 'ath10k_sta_set_txpwr':
>>> drivers/net/wireless/ath/ath10k/mac.c:5900:10: error: 'struct
>>> ieee80211_sta' has no member named 'txpwr'
>drivers/net/wireless/ath/ath10k/mac.c: At top level:
>>> drivers/net/wireless/ath/ath10k/mac.c:7469:2: error: unknown field
>>> 'sta_set_txpwr' specified in initializer
>drivers/net/wireless/ath/ath10k/mac.c:7469:2: warning:
> initialization from incompatible pointer type [enabled by default]
>drivers/net/wireless/ath/ath10k/mac.c:7469:2: warning: (near
> initialization for 'ath10k_ops.sta_pre_rcu_remove') [enabled by
> default]
>drivers/net/wireless/ath/ath10k/mac.c: In function 'ath10k_mac_register':
>>> drivers/net/wireless/ath/ath10k/mac.c:8008:11: error:
>>> 'NL80211_EXT_FEATURE_STA_TX_PWR' undeclared (first use in this
>>> function)

These are expected as this depends on cfg80211 patch not yet applied.

-- 
Kalle Valo

Re: [PATCH v3] ath10k: Fix crash during rmmod when probe firmware fails

[Valo, Kalle]

Kalle Valo  writes:

> BTW, just curious but why do you have "during rmmod" in the title? I
> think I was able to reproduce this crash by removing all firmware files
> and didn't use rmmod at all.

Nevermind, I was blind again. It was my script which calls rmmod and I
failed to realise that. Sorry for the noise.

-- 
Kalle Valo

Re: [PATCH v3] ath10k: Fix crash during rmmod when probe firmware fails

[Valo, Kalle]

Mohammed Shafi Shajakhan  writes:

> the change suggested by you helps, and the device probe, scan
> is successful as well. Still good to have this change part of your
> basic sanity and regression testing !

Sure. I was sort of expecting that you would send v4 but I haven't seen
one so I guess you assumed I send that? :) I'll then submit v4.

BTW, just curious but why do you have "during rmmod" in the title? I
think I was able to reproduce this crash by removing all firmware files
and didn't use rmmod at all.

-- 
Kalle Valo

Re: [PATCH 1/3] ath10k: remove ath10k_vif_to_arvif()

[Valo, Kalle]

Ben Greear <gree...@candelatech.com> writes:

> On 02/07/2017 01:14 AM, Valo, Kalle wrote:
>> Adrian Chadd <adr...@freebsd.org> writes:
>>
>>> Removing this method makes the diff to FreeBSD larger, as "vif" in
>>> FreeBSD is a different pointer.
>>>
>>> (Yes, I have ath10k on freebsd working and I'd like to find a way to
>>> reduce the diff moving forward.)
>>
>> I don't like this "(void *) vif->drv_priv" style that much either but
>> apparently it's commonly used in Linux wireless code and already parts
>> of ath10k. So this patch just unifies the coding style.
>
> Surely the code compiles to the same thing, so why add a patch that
> makes it more difficult for Adrian and makes the code no easier to read
> for the rest of us?

Because that's the coding style used already in Linux. It's great to see
that parts of ath10k can be used also in other systems but in principle
I'm not very fond of the idea starting to reject valid upstream patches
because of driver forks.

I think backports project is doing it right, it's not limiting upstream
development in any way and handles all the API changes internally. Maybe
FreeBSD could do something similar?

-- 
Kalle Valo

Re: [PATCH 1/3] ath10k: remove ath10k_vif_to_arvif()

[Valo, Kalle]

Adrian Chadd  writes:

> Removing this method makes the diff to FreeBSD larger, as "vif" in
> FreeBSD is a different pointer.
>
> (Yes, I have ath10k on freebsd working and I'd like to find a way to
> reduce the diff moving forward.)

I don't like this "(void *) vif->drv_priv" style that much either but
apparently it's commonly used in Linux wireless code and already parts
of ath10k. So this patch just unifies the coding style.

-- 
Kalle Valo

Re: [PATCH v2] ath10k: add support for controlling tx power to a station

[Valo, Kalle]

kbuild test robot  writes:

> Hi Ashok,
>
> [auto build test WARNING on ath6kl/ath-next]
> [also build test WARNING on v4.10-rc6 next-20170131]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
>
> url:
> https://github.com/0day-ci/linux/commits/Ashok-Raj-Nagarajan/ath10k-add-support-for-controlling-tx-power-to-a-station/20170201-032529
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git ath-next
> config: m68k-allyesconfig (attached as .config)
> compiler: m68k-linux-gcc (GCC) 4.9.0
> reproduce:
> wget 
> https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
>  -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=m68k 
>
> All warnings (new ones prefixed by >>):
>
>drivers/net/wireless/ath/ath10k/mac.c: In function 'ath10k_sta_set_txpwr':
>drivers/net/wireless/ath/ath10k/mac.c:5997:13: error: 'struct 
> ieee80211_sta' has no member named 'txpwr'
>  txpwr = sta->txpwr;
> ^
>drivers/net/wireless/ath/ath10k/mac.c: At top level:
>drivers/net/wireless/ath/ath10k/mac.c:7581:2: error: unknown field 
> 'sta_set_txpwr' specified in initializer
>  .sta_set_txpwr   = ath10k_sta_set_txpwr,
>  ^

This is expected as this patch depends on a mac80211 feature not yet
applied.

-- 
Kalle Valo

Re: [PATCH v2 01/13] wil6210: add sysfs file for FTM calibration

[Valo, Kalle]

Lior David <li...@codeaurora.org> writes:

> On 1/19/2017 3:14 PM, Arend Van Spriel wrote:
>> On 19-1-2017 13:36, Lior David wrote:
>>> On 1/19/2017 2:24 PM, Valo, Kalle wrote:
>>>> Maya Erez <qca_me...@qca.qualcomm.com> writes:
>>>>
>>>>> From: Lior David <qca_li...@qca.qualcomm.com>
>>>>>
>>>>> In fine timing measurements, the calculation is affected by
>>>>> 2 parts: timing of packets over the air, which is platform
>>>>> independent, and platform-specific delays, which are dependent
>>>>> on things like antenna cable length and type.
>>>>> Add a sysfs file which allows to get/set these platform specific
>>>>> delays, separated into the TX and RX components.
>>>>> There are 2 key scenarios where the file can be used:
>>>>> 1. Calibration - start with some initial values (for example,
>>>>> the default values at startup), make measurements at a known
>>>>> distance, then iteratively change the values until the
>>>>> measurement results match the known distance.
>>>>> 2. Adjust the delays when platform starts up, based on known
>>>>> values.
>>>>>
>>>>> Signed-off-by: Lior David <qca_li...@qca.qualcomm.com>
>>>>> Signed-off-by: Maya Erez <qca_me...@qca.qualcomm.com>
>>>>
>>>> Can't this go via nl80211? sysfs is not really supposed to be used for
>>>> something like this.
>>>>
>>> There is no nl80211 API for this (yet?).
>> 
>> So come up with one...?
> I checked this further and had some more internal discussion.
> This change is only about FTM calibration which is highly vendor specific so I
> don't think NL80211 API is appropriate for it. Since it is needed in 
> production
> (to calibrate the platform after boot using pre-computed values)

For calibration and manufacturing testing we have NL80211_CMD_TESTMODE,
which is a vendor specific interface.

> I think sysfs is a reasonable place for it.

Wireless drivers really should not use sysfs. I guess there might be
valid cases when using sysfs is ok but I can't really come up with
anything right now.

-- 
Kalle Valo

Re: Searching new home for ath[59]k-devel mailing lists

[Valo, Kalle]

Kalle Valo  writes:

> (dropping ath5k-devel and ath9k-devel, I assume they are closed now)
>
> "Michael Renzmann"  writes:
>
>> Kalle Valo wrote:
>>> So feel free to close both of the lists and thanks for the heads up.
>>
>> Ok. I will send a shutdown notice to both lists during the next few days,
>> then unsubscribe all subscribers, and close the lists for new
>> subscriptions. I intend to keep the mailing list archives online, for
>> reference purpose.
>>
>>> Are you planning to update the MAINTAINERS file or should I?
>>
>> It would be great if you could take care of that.
>
> Ok, I'll send a patch.

MAINTAINERS: ath9k-devel is closed

https://patchwork.kernel.org/patch/9541737/

-- 
Kalle Valo

Re: [PATCH v3] ath10k: Fix crash during rmmod when probe firmware fails

[Valo, Kalle]

Kalle Valo  writes:

> Mohammed Shafi Shajakhan  writes:
>
>> From: Mohammed Shafi Shajakhan 
>>
>> This fixes the below crash when ath10k probe firmware fails,
>> NAPI polling tries to access a rx ring resource which was never
>> allocated, fix this by disabling NAPI right away once the probe
>> firmware fails by calling 'ath10k_hif_stop'. Its good to note
>> that the error is never propogated to 'ath10k_pci_probe' when
>> ath10k_core_register fails, so calling 'ath10k_hif_stop' to cleanup
>> PCI related things seems to be ok
>>
>> BUG: unable to handle kernel NULL pointer dereference at (null)
>> IP:  __ath10k_htt_rx_ring_fill_n+0x19/0x230 [ath10k_core]
>> __ath10k_htt_rx_ring_fill_n+0x19/0x230 [ath10k_core]
>>
>> Call Trace:
>>
>> [] ath10k_htt_rx_msdu_buff_replenish+0x42/0x90
>> [ath10k_core]
>> [] ath10k_htt_txrx_compl_task+0x433/0x17d0
>> [ath10k_core]
>> [] ? __wake_up_common+0x4d/0x80
>> [] ? cpu_load_update+0xdc/0x150
>> [] ? ath10k_pci_read32+0xd/0x10 [ath10k_pci]
>> [] ath10k_pci_napi_poll+0x47/0x110 [ath10k_pci]
>> [] net_rx_action+0x20f/0x370
>>
>> Reported-by: Ben Greear 
>> Fixes: 3c97f5de1f28 ("ath10k: implement NAPI support")
>> Signed-off-by: Mohammed Shafi Shajakhan 
>
> Is there an easy way to reproduce this bug? I don't see it on my x86
> laptop with qca988x and I call rmmod all the time. I would like to test
> this myself.
>
>> --- a/drivers/net/wireless/ath/ath10k/core.c
>> +++ b/drivers/net/wireless/ath/ath10k/core.c
>> @@ -2164,6 +2164,7 @@ static int ath10k_core_probe_fw(struct ath10k *ar)
>>  ath10k_core_free_firmware_files(ar);
>>  
>>  err_power_down:
>> +ath10k_hif_stop(ar);
>>  ath10k_hif_power_down(ar);
>>  
>>  return ret;
>
> This breaks the symmetry, we should not be calling ath10k_hif_stop() if
> we haven't called ath10k_hif_start() from the same function. This can
> just create a bigger mess later, for example with other bus support like
> sdio or usb. In theory it should enough that we call
> ath10k_hif_power_down() and pci.c does the rest correctly "behind the
> scenes".
>
> I investigated this a bit and I think the real cause is that we call
> napi_enable() from ath10k_pci_hif_power_up() and napi_disable() from
> ath10k_pci_hif_stop(). Does anyone remember why?
>
> I was expecting that we would call napi_enable()/napi_disable() either
> in ath10k_hif_power_up/down() or ath10k_hif_start()/stop(), but not
> mixed like it's currently.

So below is something I was thinking of, now napi_enable() is called
from ath10k_hif_start() and napi_disable() from ath10k_hif_stop(). Would
that work?

--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1648,6 +1648,8 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
 
ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot hif start\n");
 
+   napi_enable(>napi);
+
ath10k_pci_irq_enable(ar);
ath10k_pci_rx_post(ar);
 
@@ -2532,7 +2534,6 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)
ath10k_err(ar, "could not wake up target CPU: %d\n", ret);
goto err_ce;
}
-   napi_enable(>napi);
 
return 0;

-- 
Kalle Valo

Re: [PATCH v3] ath10k: Fix crash during rmmod when probe firmware fails

[Valo, Kalle]

Mohammed Shafi Shajakhan  writes:

> From: Mohammed Shafi Shajakhan 
>
> This fixes the below crash when ath10k probe firmware fails,
> NAPI polling tries to access a rx ring resource which was never
> allocated, fix this by disabling NAPI right away once the probe
> firmware fails by calling 'ath10k_hif_stop'. Its good to note
> that the error is never propogated to 'ath10k_pci_probe' when
> ath10k_core_register fails, so calling 'ath10k_hif_stop' to cleanup
> PCI related things seems to be ok
>
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP:  __ath10k_htt_rx_ring_fill_n+0x19/0x230 [ath10k_core]
> __ath10k_htt_rx_ring_fill_n+0x19/0x230 [ath10k_core]
>
> Call Trace:
>
> [] ath10k_htt_rx_msdu_buff_replenish+0x42/0x90
> [ath10k_core]
> [] ath10k_htt_txrx_compl_task+0x433/0x17d0
> [ath10k_core]
> [] ? __wake_up_common+0x4d/0x80
> [] ? cpu_load_update+0xdc/0x150
> [] ? ath10k_pci_read32+0xd/0x10 [ath10k_pci]
> [] ath10k_pci_napi_poll+0x47/0x110 [ath10k_pci]
> [] net_rx_action+0x20f/0x370
>
> Reported-by: Ben Greear 
> Fixes: 3c97f5de1f28 ("ath10k: implement NAPI support")
> Signed-off-by: Mohammed Shafi Shajakhan 

Is there an easy way to reproduce this bug? I don't see it on my x86
laptop with qca988x and I call rmmod all the time. I would like to test
this myself.

> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -2164,6 +2164,7 @@ static int ath10k_core_probe_fw(struct ath10k *ar)
>   ath10k_core_free_firmware_files(ar);
>  
>  err_power_down:
> + ath10k_hif_stop(ar);
>   ath10k_hif_power_down(ar);
>  
>   return ret;

This breaks the symmetry, we should not be calling ath10k_hif_stop() if
we haven't called ath10k_hif_start() from the same function. This can
just create a bigger mess later, for example with other bus support like
sdio or usb. In theory it should enough that we call
ath10k_hif_power_down() and pci.c does the rest correctly "behind the
scenes".

I investigated this a bit and I think the real cause is that we call
napi_enable() from ath10k_pci_hif_power_up() and napi_disable() from
ath10k_pci_hif_stop(). Does anyone remember why?

I was expecting that we would call napi_enable()/napi_disable() either
in ath10k_hif_power_up/down() or ath10k_hif_start()/stop(), but not
mixed like it's currently.

-- 
Kalle Valo

Re: [PATCH 2/3] ath10k: use dma_zalloc_coherent()

[Valo, Kalle]

Joe Perches <j...@perches.com> writes:

> On Tue, 2017-01-24 at 05:18 +0000, Valo, Kalle wrote:
>> Joe Perches <j...@perches.com> writes:
>> 
>> > On Mon, 2017-01-23 at 15:04 +, Srinivas Kandagatla wrote:
>> > > use dma_zalloc_coherent() instead of dma_alloc_coherent and memset().
>> > 
>> > []
>> > > diff --git a/drivers/net/wireless/ath/ath10k/pci.c
>> > > b/drivers/net/wireless/ath/ath10k/pci.c
>> > 
>> > []
>> > > @@ -896,7 +896,7 @@ static int ath10k_pci_diag_read_mem(struct ath10k 
>> > > *ar, u32 address, void *data,
>> > >   */
>> > >  alloc_nbytes = min_t(unsigned int, nbytes, DIAG_TRANSFER_LIMIT);
>> > >  
>> > > -data_buf = (unsigned char *)dma_alloc_coherent(ar->dev,
>> > > +data_buf = (unsigned char *)dma_zalloc_coherent(ar->dev,
>> > > alloc_nbytes,
>> > > _data_base,
>> > > GFP_ATOMIC);
>> > 
>> > trivia:
>> > 
>> > Nicer to realign arguments and remove the unnecessary cast.
>> > 
>> > Perhaps:
>> > 
>> >data_buf = dma_zalloc_coherent(ar->dev, alloc_nbytes, _data_base,
>> >   GFP_ATOMIC);
>> 
>> Sure, but that should be in a separate patch.
>
> I don't think so, trivial patches can be combined.
>
> It's also nicer to realign all modified multiline
> arguments when performing these changes.
>
> Coccinelle generally does it automatically.

A matter of preference really. I prefer keeping style and functional
changes in separate patches, keeps the review simple. And style changes
can hide bugs.

-- 
Kalle Valo

Re: [PATCH 2/3] ath10k: use dma_zalloc_coherent()

[Valo, Kalle]

Joe Perches  writes:

> On Mon, 2017-01-23 at 15:04 +, Srinivas Kandagatla wrote:
>> use dma_zalloc_coherent() instead of dma_alloc_coherent and memset().
> []
>> diff --git a/drivers/net/wireless/ath/ath10k/pci.c 
>> b/drivers/net/wireless/ath/ath10k/pci.c
> []
>> @@ -896,7 +896,7 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, 
>> u32 address, void *data,
>>   */
>>  alloc_nbytes = min_t(unsigned int, nbytes, DIAG_TRANSFER_LIMIT);
>>  
>> -data_buf = (unsigned char *)dma_alloc_coherent(ar->dev,
>> +data_buf = (unsigned char *)dma_zalloc_coherent(ar->dev,
>> alloc_nbytes,
>> _data_base,
>> GFP_ATOMIC);
>
> trivia:
>
> Nicer to realign arguments and remove the unnecessary cast.
>
> Perhaps:
>
>   data_buf = dma_zalloc_coherent(ar->dev, alloc_nbytes, _data_base,
>  GFP_ATOMIC);

Sure, but that should be in a separate patch.

-- 
Kalle Valo

Re: [RFC v3 0/8] ath10k sdio support

[Valo, Kalle]

Erik Stromdahl  writes:

> This is the third version of the sdio RFC patch series.
> The actual sdio code (patch 6) has been subject to a massive overhaul,
> mainly as a result of Kalle's review comments.
> It no longer has any strong resemblance of the original ath6kl code from
> which it was originally based upon.
>
> Previous pathes 6 to 10 have been merged into one single patch.
>
> The previous series had a rework of the "HTC fake service" connect
> (ep 0 connect) that introduced a race between the actual endpoint
> connect and the HTC ready message. This issue has been addressed,
> and the current patches (3 and 4) has been rewritten accordingly.
>
> * overview of patches *
>
> Patches 1 to 4 are more or less identical to the previous RFC, with an
> exception for patch 3 that changes the "HTC fake service" connect
> (mentioned above).
>
> Patch 5 is a squashed version of previous patches 6 to 10.
>
> Patch 6 is the actual sdio patch
>
> Patches 7 to 8 are new and adds special sdio versions of BMI get
> target info and HTC ready.
>
> The new version was built and tested against:
> tag: ath-201701121109
>
> Erik Stromdahl (8):
>   ath10k: htc: made static function public
>   ath10k: htc: rx trailer lookahead support
>   ath10k: htc: move htc ctrl ep connect to htc_init
>   ath10k: htc: refactorization
>   ath10k: various sdio related definitions
>   ath10k: sdio support
>   ath10k: sdio get target info
>   ath10k: htc: ready_ext msg support

I pushed now this, and your usb series as well, to ath.git
master-pending branch. Let's see if kbuild bot finds any problems.

-- 
Kalle Valo

Re: [PATCH v2 01/13] wil6210: add sysfs file for FTM calibration

[Valo, Kalle]

Maya Erez  writes:

> From: Lior David 
>
> In fine timing measurements, the calculation is affected by
> 2 parts: timing of packets over the air, which is platform
> independent, and platform-specific delays, which are dependent
> on things like antenna cable length and type.
> Add a sysfs file which allows to get/set these platform specific
> delays, separated into the TX and RX components.
> There are 2 key scenarios where the file can be used:
> 1. Calibration - start with some initial values (for example,
> the default values at startup), make measurements at a known
> distance, then iteratively change the values until the
> measurement results match the known distance.
> 2. Adjust the delays when platform starts up, based on known
> values.
>
> Signed-off-by: Lior David 
> Signed-off-by: Maya Erez 

Can't this go via nl80211? sysfs is not really supposed to be used for
something like this.

-- 
Kalle Valo

Re: [PATCH 1/3] rsi: Renamed the file rsi_91x_pkt.c to rsi_91x_hal.c

[Valo, Kalle]

Kalle Valo  writes:

> Prameela Rani Garnepudi  writes:
>
>> Can you please review these patches. You slipped through these patches  
>> and reviewing resent patches. Is there a specific reason? 
>
> As you are new, your patches just take more time to review than others
> and haven't had a good slot to look at those yet.

Prameela, I noticed that patchwork seems to drop your emails for some
reason. For example, in this patch I do not see your mail above, only my
reply:

https://patchwork.kernel.org/patch/9465833/

That might create problems because I usually check the discussion from
patchwork, not from my own mail archives. Keep this in mind then
commenting.

-- 
Kalle Valo

Re: [1/2] ath10k: add accounting for the extended peer statistics

[Valo, Kalle]

Christian Lamparter  writes:

> Hello Shafi and Kalle,
>
> On Tuesday, January 3, 2017 10:58:27 AM CET Mohammed Shafi Shajakhan wrote:
>> On Fri, Dec 30, 2016 at 03:35:10PM +0100, Christian Lamparter wrote:
>> > On Thu, Dec 29, 2016 at 3:11 PM, Kalle Valo  wrote:
>> > > Christian Lamparter  wrote:
>> > >> The 10.4 firmware adds extended peer information to the
>> > >> firmware's statistics payload. This additional info is
>> > >> stored as a separate data field and the elements are
>> > >> stored in their own "peers_extd" list.
>> > >>
>> > >> These elements can pile up in the same way as the peer
>> > >> information elements. This is because the
>> > >> ath10k_wmi_10_4_op_pull_fw_stats() function tries to
>> > >> pull the same amount (num_peer_stats) for every statistic
>> > >> data unit.
>> > >>
>> > >> Fixes: 4a49ae94a448faa ("ath10k: fix 10.4 extended peer stats update")
>> > >> Signed-off-by: Christian Lamparter 
>> > >
>> > > My understanding is that I should skip this patch 1. Please let me know 
>> > > if
>> > > I misunderstood. But I'm still plannning to apply patch 2.
>> > 
>> > I added Mohammed (I hope, he can reply to the open question when he
>> > returns), Since I'm not sure what he wants or not.
>> > 
>> > As far as I'm aware, the "extended" boolean serves no purpose
>> > because it was only used in once place in debugfs_sta which was
>> > removed in the patch. ( "ath10k_sta_update_stats_rx_duration"
>> > and "ath10k_sta_update_extd_stats_rx_duration" have been unified).
>> 
>> [shafi] We will wait for Kalle to review from the de-ferred stage
>> and get his opinion as well(regarding the design change).
>> I have no concerns as long this does not changes the existing behaviour.
>> thank you !
>
> Thank you Shafi for sticking around. I just fished your response to 
> "Re: [PATCH] ath10k: merge extended peer info data with existing peers info" 
> [0].
> out of my spam-bucket. Kalle, please look if your copy of the mail got 
> flagged/deleted as well. Judging from the reply in this thread, I think you
> overlooked it as well? 

I think I just read the discussion to hastily as it was rather long,
sorry about that.

After really long or confusin discussions, just to help the maintainers
and also avoid miscommunication between participants, it's usually a
good idea to summarise the conclusion. If us maintainers need to figure
out the conclusion ourselves from a long discussion we are bound to make
mistakes, just like I did here.

So something like this would help me a lot:

"Kalle, please drop these patches. I need to work on these a bit more."

Or: 

"Kalle, me and John came to agreement about foo. So these should be good
to apply."

> After reading it, I think the previous post and the request to put the patch
> on wait was unnecessary. As of now, it seems to me that the open questions
> between us have been settled amically (so to speak). Kalle, do you have any
> concerns or can you put this in the next round then?

If you both are happy with the patch, I'm happy to take it :)

I actived the patch again in my queue by moving the state from Deferred
to New:

https://patchwork.kernel.org/patch/9461631/

If all goes well I'm expecting to apply it later this week.

-- 
Kalle Valo

Re: [1/2] ath10k: add accounting for the extended peer statistics

[Valo, Kalle]

Christian Lamparter  writes:

> On Thu, Dec 29, 2016 at 3:11 PM, Kalle Valo  wrote:
>> Christian Lamparter  wrote:
>>> The 10.4 firmware adds extended peer information to the
>>> firmware's statistics payload. This additional info is
>>> stored as a separate data field and the elements are
>>> stored in their own "peers_extd" list.
>>>
>>> These elements can pile up in the same way as the peer
>>> information elements. This is because the
>>> ath10k_wmi_10_4_op_pull_fw_stats() function tries to
>>> pull the same amount (num_peer_stats) for every statistic
>>> data unit.
>>>
>>> Fixes: 4a49ae94a448faa ("ath10k: fix 10.4 extended peer stats update")
>>> Signed-off-by: Christian Lamparter 
>>
>> My understanding is that I should skip this patch 1. Please let me know if
>> I misunderstood. But I'm still plannning to apply patch 2.
>
> I added Mohammed (I hope, he can reply to the open question when he
> returns), Since I'm not sure what he wants or not.
>
> As far as I'm aware, the "extended" boolean serves no purpose
> because it was only used in once place in debugfs_sta which was
> removed in the patch. ( "ath10k_sta_update_stats_rx_duration"
> and "ath10k_sta_update_extd_stats_rx_duration" have been unified).

I had problems following the discussion so the conclusion was not clear
for me.

>> Patch set to Changes Requested.
>
> Isn't there a: "Waiting for Maintainer" state as well?
> Otherwise, if nobody has any complains or question:
> can you please queue it for the next merge window?

There is a state called "Deferred" which I use whenever I need to
revisit a patch later time. I moved this patch to that state now:

https://patchwork.kernel.org/patch/9461631/

-- 
Kalle Valo

Re: regression: ath_tx_edma_tasklet() Illegal idle entry in RCU read-side critical section

[Valo, Kalle]

Gabriel C <nix.or@gmail.com> writes:

> On 18.12.2016 21:14, Paul E. McKenney wrote:
>> On Sun, Dec 18, 2016 at 07:57:42PM +0000, Valo, Kalle wrote:
>>> Tobias Klausmann <tobias.johannes.klausm...@mni.thm.de> writes:
>>>
>>>> A patch for this is already floating on the ML for a while now latest:
>>>> (ath9k: do not return early to fix rcu unlocking)
>>>
>>> It's here:
>>>
>>> https://patchwork.kernel.org/patch/9472709/
>>
>> Feel free to add:
>>
>> Acked-by: Paul E. McKenney <paul...@linux.vnet.ibm.com>
>>
>
> This patch fixes the bug for me.
>
> You can add my Tested-by if you wish.
>
> Tested-by: Gabriel Craciunescu <nix.or@gmail.com>

I added both of these to the commit log, thanks.

-- 
Kalle Valo

Re: regression: ath_tx_edma_tasklet() Illegal idle entry in RCU read-side critical section

[Valo, Kalle]

Tobias Klausmann  writes:

> A patch for this is already floating on the ML for a while now latest:
> (ath9k: do not return early to fix rcu unlocking)

It's here:

https://patchwork.kernel.org/patch/9472709/

> Hopefully Kalle will include it in one of his upcoming pull requests.

Yes, I'll try to get it to 4.10-rc2.

-- 
Kalle Valo

Re: [RFC v2 11/11] ath10k: Added sdio support

[Valo, Kalle]

Erik Stromdahl  writes:

> Initial HIF sdio/mailbox implementation.
>
> Signed-off-by: Erik Stromdahl 

I know most of this coming from ath6kl but I think we should still
improve the code. Lots of comments will follow, don't get scared :)

> +#define CALC_TXRX_PADDED_LEN(ar_sdio, len) \
> + (__ALIGN_MASK((len), (ar_sdio)->mbox_info.block_mask))

I think this could be a proper static inline function. Andrew Morton
once said: "Write in C, not CPP" (or something like that), I think
that's a really good point.

> +static int ath10k_sdio_read_write_sync(struct ath10k *ar, u32 addr, u8 *buf,
> +u32 len, u32 request);
> +static int ath10k_sdio_hif_diag_read(struct ath10k *ar, u32 address, void 
> *buf,
> +  size_t buf_len);
> +static int ath10k_sdio_hif_diag_read32(struct ath10k *ar, u32 address,
> +u32 *value);

We prefer to avoid forward declarations if at all possible. I didn't
check but if there's a clean way to avoid these please remove them.

> +/* HIF mbox interrupt handling */
> +
> +static int ath10k_sdio_mbox_rx_process_packet(struct ath10k_sdio *ar_sdio,
> +   struct ath10k_sdio_rx_data *pkt,
> +   u32 *lookaheads,
> +   int *n_lookaheads)
> +{

So the style in ath10k is that all functions (of course this doesn't
apply to callbacks etc) have "struct ath10k *ar" as the first parameter.
This way there's no need to check if a function takes ar or ar_sdio.

> + int status = 0;

In ath10k we prefer to use ret. And avoid initialising it, please.

> + struct ath10k_htc *htc = _sdio->ar->htc;
> + struct sk_buff *skb = pkt->skb;
> + struct ath10k_htc_hdr *htc_hdr = (struct ath10k_htc_hdr *)skb->data;
> + bool trailer_present = htc_hdr->flags & ATH10K_HTC_FLAG_TRAILER_PRESENT;
> + u16 payload_len;
> +
> + payload_len = le16_to_cpu(htc_hdr->len);
> +
> + if (trailer_present) {
> + u8 *trailer;
> + enum ath10k_htc_ep_id eid;
> +
> + trailer = skb->data + sizeof(struct ath10k_htc_hdr) +
> +   payload_len - htc_hdr->trailer_len;
> +
> + eid = (enum ath10k_htc_ep_id)htc_hdr->eid;

A some kind of mapping function for ep id would be nice, it makes it
more visible how it works.

> +static int ath10k_sdio_mbox_rx_process_packets(struct ath10k_sdio *ar_sdio,
> +u32 lookaheads[],
> +int *n_lookahead)
> +{
> + struct ath10k *ar = ar_sdio->ar;
> + struct ath10k_htc *htc = >htc;
> + struct ath10k_sdio_rx_data *pkt;
> + int status = 0, i;
> +
> + for (i = 0; i < ar_sdio->n_rx_pkts; i++) {
> + struct ath10k_htc_ep *ep;
> + enum ath10k_htc_ep_id id;
> + u32 *lookaheads_local = lookaheads;
> + int *n_lookahead_local = n_lookahead;
> +
> + id = ((struct ath10k_htc_hdr *)[i])->eid;
> +
> + if (id >= ATH10K_HTC_EP_COUNT) {
> + ath10k_err(ar, "Invalid endpoint in look-ahead: %d\n",
> +id);

In ath10k we use ath10k_err() for errors from which can't survive and
ath10k_warn() for errors where we try to continue. So ath10k_warn()
would be more approriate here and most of other cases in sdio.c

> + status = -ENOMEM;
> + goto out;
> + }
> +
> + ep = >endpoint[id];
> +
> + if (ep->service_id == 0) {
> + ath10k_err(ar, "ep %d is not connected !\n", id);
> + status = -ENOMEM;
> + goto out;
> + }
> +
> + pkt = _sdio->rx_pkts[i];
> +
> + if (pkt->part_of_bundle && !pkt->last_in_bundle) {
> + /* Only read lookahead's from RX trailers
> +  * for the last packet in a bundle.
> +  */
> + lookaheads_local = NULL;
> + n_lookahead_local = NULL;
> + }
> +
> + status = ath10k_sdio_mbox_rx_process_packet(ar_sdio,
> + pkt,
> + lookaheads_local,
> + n_lookahead_local);
> + if (status)
> + goto out;
> +
> + ep->ep_ops.ep_rx_complete(ar_sdio->ar, pkt->skb);
> + /* The RX complete handler now owns the skb...*/
> + pkt->skb = NULL;
> + pkt->alloc_len = 0;
> + }
> +
> +out:
> + /* Free all packets that was not passed on to the RX completion
> +  * handler...
> +  */
> + for (; i < ar_sdio->n_rx_pkts; i++)
> 

Re: [RFC v2 06/11] ath10k: htc: Added ATH10K_HTC_FLAG_BUNDLE_LSB

[Valo, Kalle]

Erik Stromdahl  writes:

> Signed-off-by: Erik Stromdahl 
> ---
>  drivers/net/wireless/ath/ath10k/htc.h |2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath10k/htc.h 
> b/drivers/net/wireless/ath/ath10k/htc.h
> index f94b25a..2963694 100644
> --- a/drivers/net/wireless/ath/ath10k/htc.h
> +++ b/drivers/net/wireless/ath/ath10k/htc.h
> @@ -62,6 +62,8 @@ enum ath10k_htc_rx_flags {
>   ATH10K_HTC_FLAG_BUNDLE_MASK = 0xF0
>  };
>  
> +#define ATH10K_HTC_FLAG_BUNDLE_LSB 4
> +

I think you could fold patches from 6 to 11 into one patch. They are all
about adding code, and most of the commit logs are empty anyway.

-- 
Kalle Valo

Re: [RFC v2 11/11] ath10k: Added sdio support

[Valo, Kalle]

Erik Stromdahl  writes:

> Initial HIF sdio/mailbox implementation.
>
> Signed-off-by: Erik Stromdahl 
> ---
>  drivers/net/wireless/ath/ath10k/Kconfig  |6 +
>  drivers/net/wireless/ath/ath10k/Makefile |3 +
>  drivers/net/wireless/ath/ath10k/sdio.c   | 1860 
> ++
>  drivers/net/wireless/ath/ath10k/sdio.h   |  276 +
>  4 files changed, 2145 insertions(+)
>  create mode 100644 drivers/net/wireless/ath/ath10k/sdio.c
>  create mode 100644 drivers/net/wireless/ath/ath10k/sdio.h

AFAIK the sdio firmware binary is different from pci and usb. And as all
the firmware images need to coexist in the same repository I suspect we
can't continue to use firmware-N.bin for both pcie and sdio (and in
future usb). So should we somehow rename the sdio firmware filename to
something else, like firmware-sdio-N.bin?

-- 
Kalle Valo

Re: [PATCH v2 0/7] ath9k: EEPROM swapping improvements

[Valo, Kalle]

Martin Blumenstingl  writes:

> There are two types of swapping the EEPROM data in the ath9k driver.
> Before this series one type of swapping could not be used without the
> other.
>
> The first type of swapping looks at the "magic bytes" at the start of
> the EEPROM data and performs swab16 on the EEPROM contents if needed.
> The second type of swapping is EEPROM format specific and swaps
> specific fields within the EEPROM itself (swab16, swab32 - depends on
> the EEPROM format).
>
> With this series the second part now looks at the EEPMISC register
> inside the EEPROM, which uses a bit to indicate if the EEPROM data
> is Big Endian (this is also done by the FreeBSD kernel).
> This has a nice advantage: currently there are some out-of-tree hacks
> (in OpenWrt and LEDE) where the EEPROM has a Big Endian header on a
> Big Endian system (= no swab16 is performed) but the EEPROM itself
> indicates that it's data is Little Endian. Until now the out-of-tree
> code simply did a swab16 before passing the data to ath9k, so ath9k
> first did the swab16 - this also enabled the format specific swapping.
> These out-of-tree hacks are still working with the new logic, but it
> is recommended to remove them. This implementation is based on a
> discussion with Arnd Bergmann who raised concerns about the
> robustness and portability of the swapping logic in the original OF
> support patch review, see [0].
>
> After a second round of patches (= v1 of this series) neither Arnd
> Bergmann nor I were really happy with the complexity of the EEPROM
> swapping logic. Based on a discussion (see [1] and [2]) we decided
> that ath9k should use a defined format (specifying the endianness
> of the data - I went with __le16 and __le32) when accessing the
> EEPROM fields. A benefit of this is that we enable the EEPMISC based
> swapping logic by default, just like the FreeBSD driver, see [3]. On
> the devices which I have tested (see below) ath9k now works without
> having to specify the "endian_check" field in ath9k_platform_data (or
> a similar logic which could provide this via devicetree) as ath9k now
> detects the endianness automatically. Only EEPROMs which are mangled
> by some out-of-tree code still need the endian_check flag (or one can
> simply remove that mangling from the out-of-tree code).
>
> Testing:
> - tested by myself on AR9287 with Big Endian EEPROM
> - tested by myself on AR9227 with Little Endian EEPROM
> - tested by myself on AR9381 (using the ar9003_eeprom implementation,
>   which did not suffer from this whole problem)
> - how do we proceed with testing? maybe we could keep this in a
>   feature-branch and add these patches to LEDE once we have an ACK to
>   get more people to test this
>
> This series depends on my other series (v7):
> "add devicetree support to ath9k" - see [4]
>
> Changes since v1:
> - reworked description in the cover-letter to describe the reasons
>   behind the new patch 7
> - reworked patch "Set the "big endian" bit of the AR9003 EEPROM
>   templates" as ar9003_eeprom.c sets all values as Little Endian, thus
>   the Big Endian bit should never be set (the new patch makes this
>   clear)
> - dropped "ath9k: Make EEPROM endianness swapping configurable via
>   devicetree" as it is not needed anymore with the new logic from
>   patch 7
> - added patches 4 and 5 as small cleanup (this made it easier to
>   implement the le{16,32}_to_cpu() changes where needed)
>
>
> [0] http://www.spinics.net/lists/linux-wireless/msg152634.html
> [1] https://marc.info/?l=linux-wireless=147250597503174=2
> [2] https://marc.info/?l=linux-wireless=147254388611344=2
> [3] 
> https://github.com/freebsd/freebsd/blob/50719b56d9ce8d7d4beb53b16e9edb2e9a4a7a18/sys/dev/ath/ath_hal/ah_eeprom_9287.c#L351
> [4] https://marc.info/?l=linux-wireless=147544488619822=2
>
> Martin Blumenstingl (7):
>   ath9k: Add a #define for the EEPROM "eepmisc" endianness bit
>   ath9k: indicate that the AR9003 EEPROM template values are little
> endian
>   ath9k: Add an eeprom_ops callback for retrieving the eepmisc value
>   ath9k: replace eeprom_param EEP_MINOR_REV with get_eeprom_rev
>   ath9k: consistently use get_eeprom_rev(ah)
>   ath9k: Make the EEPROM swapping check use the eepmisc register
>   ath9k: define all EEPROM fields in Little Endian format

Applied to ath-next on ath.git, thanks.

(My automatic "accepted" email failed, so had to send this manually.)

-- 
Kalle Valo

Re: [RFC v2 05/11] ath10k: htc: refactorization

[Valo, Kalle]

Michal Kazior  writes:

>> I have made a few updates since I submitted the original RFC and created
>> a repo on github:
>>
>> https://github.com/erstrom/linux-ath
>>
>> I have a bunch of branches that are all based on the tags on the ath master.
>>
>> As of this moment the latest version is:
>>
>> ath-201612131156-ath10k-sdio
>>
>> This branch contains the original RFC patches plus some addons/fixes.
>>
>> In the above mentioned branch there are a few commits related to this
>> race condition. Perhaps you can have a look at them?
>>
>> The commits are:
>> 821672913328cf737c9616786dc28d2e4e8a4a90
>
> I would avoid if(bus==xx) checks.

Very much agreed. For example to enable HTT high latency support you
could add an enum to ath10k_core_create() with values for both high and
low latency. This way sdio.c and pci.c can enable the correct mode
during initialisation.

-- 
Kalle Valo

Re: [RFC v2 05/11] ath10k: htc: refactorization

[Valo, Kalle]

Erik Stromdahl  writes:

> I have made a few updates since I submitted the original RFC and created
> a repo on github:
>
> https://github.com/erstrom/linux-ath
>
> I have a bunch of branches that are all based on the tags on the ath master.
>
> As of this moment the latest version is:
>
> ath-201612131156-ath10k-sdio
>
> This branch contains the original RFC patches plus some addons/fixes.

Good, this makes it easier to follow the development. So what's the
current status with this branch? What works and what doesn't?

Especially I'm interested about the state of the HTT high latency
support. How much work is to add that? It would also make it easier to
add USB support to ath10k.

-- 
Kalle Valo

Re: [RFC v2 05/11] ath10k: htc: refactorization

[Valo, Kalle]

Michal Kazior <michal.kaz...@tieto.com> writes:

> On 13 December 2016 at 14:44, Valo, Kalle <kv...@qca.qualcomm.com> wrote:
>> Erik Stromdahl <erik.stromd...@gmail.com> writes:
>>
>>> Code refactorization:
>>>
>>> Moved the code for ep 0 in ath10k_htc_rx_completion_handler
>>> to ath10k_htc_control_rx_complete.
>>>
>>> This eases the implementation of SDIO/mbox significantly since
>>> the ep_rx_complete cb is invoked directly from the SDIO/mbox
>>> hif layer.
>>>
>>> Since the ath10k_htc_control_rx_complete already is present
>>> (only containing a warning message) there is no reason for not
>>> using it (instead of having a special case for ep 0 in
>>> ath10k_htc_rx_completion_handler).
>>>
>>> Signed-off-by: Erik Stromdahl <erik.stromd...@gmail.com>
>>
>> I tested this on QCA988X PCI board just to see if there are any
>> regressions. It crashes immediately during module load, every time, and
>> bisected that the crashing starts on this patch:
>>
>> [ 1239.715325] ath10k_pci :02:00.0: pci irq msi oper_irq_mode 2 irq_mode 
>> 0 reset_mode 0
>> [ 1239.885125] ath10k_pci :02:00.0: Direct firmware load for 
>> ath10k/pre-cal-pci-:02:00.0.bin failed with error -2
>> [ 1239.885260] ath10k_pci :02:00.0: Direct firmware load for 
>> ath10k/cal-pci-:02:00.0.bin failed with error -2
>> [ 1239.885687] ath10k_pci :02:00.0: qca988x hw2.0 target 0x4100016c 
>> chip_id 0x043202ff sub :
>> [ 1239.885699] ath10k_pci :02:00.0: kconfig debug 1 debugfs 1 tracing 1 
>> dfs 1 testmode 1
>> [ 1239.885899] ath10k_pci :02:00.0: firmware ver 10.2.4.70.59-2 api 5 
>> features no-p2p,raw-mode,mfp,allows-mesh-bcast crc32 4159f498
>> [ 1239.941836] ath10k_pci :02:00.0: Direct firmware load for 
>> ath10k/QCA988X/hw2.0/board-2.bin failed with error -2
>> [ 1239.941993] ath10k_pci :02:00.0: board_file api 1 bmi_id N/A crc32 
>> bebc7c08
>> [ 1241.136693] BUG: unable to handle kernel NULL pointer dereference at   
>> (null)
>> [ 1241.136738] IP: [<  (null)>]   (null)
>> [ 1241.136759] *pdpt =  *pde = f0002a55f0002a55 [ 
>> 1241.136781]
>> [ 1241.136793] Oops: 0010 [#1] SMP
>>
>> What's odd is that when I added some printks on my own and enabled both
>> boot and htc debug levels it doesn't crash anymore. After everything
>> works normally after that, I can start AP mode and connect to it. Is it
>> a race somewhere?
>
> Yes. htc_wait_target() is called after hif_start(). The ep_rx_complete
> is set in htc_wait_target() [changed patch 4, but still too late].
>
> ep_rx_complete must be set prior to calling hif_start(). You probably
> crash on end of ath10k_htc_rx_completion_handler() when trying to call
> ep->ep_ops.ep_rx_complete(ar, skb).

Yeah, just checked and ep->ep_ops.ep_rx_complete is NULL at the end of
ath10k_htc_rx_completion_handler().

-- 
Kalle Valo

Re: [RFC v2 05/11] ath10k: htc: refactorization

[Valo, Kalle]

Erik Stromdahl  writes:

> Code refactorization:
>
> Moved the code for ep 0 in ath10k_htc_rx_completion_handler
> to ath10k_htc_control_rx_complete.
>
> This eases the implementation of SDIO/mbox significantly since
> the ep_rx_complete cb is invoked directly from the SDIO/mbox
> hif layer.
>
> Since the ath10k_htc_control_rx_complete already is present
> (only containing a warning message) there is no reason for not
> using it (instead of having a special case for ep 0 in
> ath10k_htc_rx_completion_handler).
>
> Signed-off-by: Erik Stromdahl 

I tested this on QCA988X PCI board just to see if there are any
regressions. It crashes immediately during module load, every time, and
bisected that the crashing starts on this patch:

[ 1239.715325] ath10k_pci :02:00.0: pci irq msi oper_irq_mode 2 irq_mode 0 
reset_mode 0
[ 1239.885125] ath10k_pci :02:00.0: Direct firmware load for 
ath10k/pre-cal-pci-:02:00.0.bin failed with error -2
[ 1239.885260] ath10k_pci :02:00.0: Direct firmware load for 
ath10k/cal-pci-:02:00.0.bin failed with error -2
[ 1239.885687] ath10k_pci :02:00.0: qca988x hw2.0 target 0x4100016c chip_id 
0x043202ff sub :
[ 1239.885699] ath10k_pci :02:00.0: kconfig debug 1 debugfs 1 tracing 1 dfs 
1 testmode 1
[ 1239.885899] ath10k_pci :02:00.0: firmware ver 10.2.4.70.59-2 api 5 
features no-p2p,raw-mode,mfp,allows-mesh-bcast crc32 4159f498
[ 1239.941836] ath10k_pci :02:00.0: Direct firmware load for 
ath10k/QCA988X/hw2.0/board-2.bin failed with error -2
[ 1239.941993] ath10k_pci :02:00.0: board_file api 1 bmi_id N/A crc32 
bebc7c08
[ 1241.136693] BUG: unable to handle kernel NULL pointer dereference at   (null)
[ 1241.136738] IP: [<  (null)>]   (null)
[ 1241.136759] *pdpt =  *pde = f0002a55f0002a55 [ 1241.136781] 
[ 1241.136793] Oops: 0010 [#1] SMP

What's odd is that when I added some printks on my own and enabled both
boot and htc debug levels it doesn't crash anymore. After everything
works normally after that, I can start AP mode and connect to it. Is it
a race somewhere?

-- 
Kalle Valo

Re: [RFC v2 11/11] ath10k: Added sdio support

[Valo, Kalle]

Erik Stromdahl  writes:

> Initial HIF sdio/mailbox implementation.
>
> Signed-off-by: Erik Stromdahl 

While testing this I noticed few new warnings:

drivers/net/wireless/ath/ath10k/sdio.c: In function ath10k_sdio_probe:
drivers/net/wireless/ath/ath10k/sdio.c:1723:6: warning: 'ret' may be used 
uninitialized in this function [-Wuninitialized]
drivers/net/wireless/ath/ath10k/sdio.c:375:5: warning: symbol 
'ath10k_sdio_mbox_rxmsg_pending_handler' was not declared. Should it be static?
drivers/net/wireless/ath/ath10k/sdio.c:1018:5: warning: symbol 
'ath10k_sdio_hif_tx_sg' was not declared. Should it be static?
drivers/net/wireless/ath/ath10k/sdio.c:1415:5: warning: symbol 
'ath10k_sdio_hif_exchange_bmi_msg' was not declared. Should it be static?
drivers/net/wireless/ath/ath10k/sdio.c:1555:5: warning: symbol 
'ath10k_sdio_hif_map_service_to_pipe' was not declared. Should it be static?
drivers/net/wireless/ath/ath10k/sdio.c:1635:6: warning: symbol 
'ath10k_sdio_hif_get_default_pipe' was not declared. Should it be static?
drivers/net/wireless/ath/ath10k/htc.c:265: line over 90 characters
drivers/net/wireless/ath/ath10k/htc.c:355: line over 90 characters

-- 
Kalle Valo

Re: [PATCH v2 0/7] ath9k: EEPROM swapping improvements

[Valo, Kalle]

Martin Blumenstingl <martin.blumensti...@googlemail.com> writes:

> Hello Kalle,
>
> On Fri, Nov 25, 2016 at 4:06 PM, Valo, Kalle <kv...@qca.qualcomm.com> wrote:
>> Kalle Valo <kv...@codeaurora.org> writes:
>>
>>> Martin Blumenstingl <martin.blumensti...@googlemail.com> writes:
>>>
>>>> There are two types of swapping the EEPROM data in the ath9k driver.
>>>> Before this series one type of swapping could not be used without the
>>>> other.
>>>>
>>>> The first type of swapping looks at the "magic bytes" at the start of
>>>> the EEPROM data and performs swab16 on the EEPROM contents if needed.
>>>> The second type of swapping is EEPROM format specific and swaps
>>>> specific fields within the EEPROM itself (swab16, swab32 - depends on
>>>> the EEPROM format).
>>>>
>>>> With this series the second part now looks at the EEPMISC register
>>>> inside the EEPROM, which uses a bit to indicate if the EEPROM data
>>>> is Big Endian (this is also done by the FreeBSD kernel).
>>>> This has a nice advantage: currently there are some out-of-tree hacks
>>>> (in OpenWrt and LEDE) where the EEPROM has a Big Endian header on a
>>>> Big Endian system (= no swab16 is performed) but the EEPROM itself
>>>> indicates that it's data is Little Endian. Until now the out-of-tree
>>>> code simply did a swab16 before passing the data to ath9k, so ath9k
>>>> first did the swab16 - this also enabled the format specific swapping.
>>>> These out-of-tree hacks are still working with the new logic, but it
>>>> is recommended to remove them. This implementation is based on a
>>>> discussion with Arnd Bergmann who raised concerns about the
>>>> robustness and portability of the swapping logic in the original OF
>>>> support patch review, see [0].
>>>>
>>>> After a second round of patches (= v1 of this series) neither Arnd
>>>> Bergmann nor I were really happy with the complexity of the EEPROM
>>>> swapping logic. Based on a discussion (see [1] and [2]) we decided
>>>> that ath9k should use a defined format (specifying the endianness
>>>> of the data - I went with __le16 and __le32) when accessing the
>>>> EEPROM fields. A benefit of this is that we enable the EEPMISC based
>>>> swapping logic by default, just like the FreeBSD driver, see [3]. On
>>>> the devices which I have tested (see below) ath9k now works without
>>>> having to specify the "endian_check" field in ath9k_platform_data (or
>>>> a similar logic which could provide this via devicetree) as ath9k now
>>>> detects the endianness automatically. Only EEPROMs which are mangled
>>>> by some out-of-tree code still need the endian_check flag (or one can
>>>> simply remove that mangling from the out-of-tree code).
>>>>
>>>> Testing:
>>>> - tested by myself on AR9287 with Big Endian EEPROM
>>>> - tested by myself on AR9227 with Little Endian EEPROM
>>>> - tested by myself on AR9381 (using the ar9003_eeprom implementation,
>>>>   which did not suffer from this whole problem)
>>>> - how do we proceed with testing? maybe we could keep this in a
>>>>   feature-branch and add these patches to LEDE once we have an ACK to
>>>>   get more people to test this
>>>>
>>>> This series depends on my other series (v7):
>>>> "add devicetree support to ath9k" - see [4]
>>>
>>> I think this looks pretty good. If there's a bug somewhere it should be
>>> quite easy to fix so I'm not that worried and would be willing to take
>>> these as soon as I have applied the dependency series. IIRC your
>>> devicetree patches will have at least one more review round so that will
>>> take some time still. In the meantime it would be great if LEDE folks
>>> could take a look at these and comment (or test).
>>
>> So are everyone happy with this? I haven't seen any comments. If I don't
>> here anything I'm planning to take these, most likely for 4.11.
>
> the patches have been in LEDE for almost two weeks now and I did not
> see any reports of ath9k breakage (footnote below).
>
> It seems that there are a few devices out there where the whole EEPROM
> is swab16'ed which switches the position of the 1-byte fields
> opCapFlags and eepMisc.
> those still work fine with the new code, however I had a second patch
> in LEDE [0] which results in ath9k_platform_data.endian_check NOT
> being set anymore.
> that endian_check flag was used before to swab16 the whole EEPROM, to
> correct the position of the 1-byte fields again.
> Currently we are fixing this in the firmware hotplug script: [1]
> This is definitely not a blocker for this series though (if we want to
> have a devicetree replacement for "ath9k_platform_data.endian_check"
> then I'd work on that within a separate series, but I somewhat
> consider these EEPROMs as "broken" so fixing them in
> userspace/firmware hotplug script is fine for me)

Sounds good to me, thanks for the thorough followup. I'm planning to
apply these any day.

-- 
Kalle Valo

Re: [PATCH] ath10k: free host-mem with DMA_BIRECTIONAL flag.

[Valo, Kalle]

gree...@candelatech.com writes:

> From: Ben Greear 
>
> Hopefully this fixes the problem reported by Kalle:
>
> Noticed this in my log, but I don't have time to investigate this in
> detail right now:
>
> [  413.795346] IPv6: ADDRCONF(NETDEV_UP): wlan0: link is not ready
> [  414.158755] IPv6: ADDRCONF(NETDEV_CHANGE): wlan0: link becomes ready
> [  477.439659] ath10k_pci :02:00.0: could not get mac80211 beacon
> [  481.30] [ cut here ]
> [  481.69] WARNING: CPU: 0 PID: 1978 at lib/dma-debug.c:1155 
> check_unmap+0x320/0x8e0
> [  481.88] ath10k_pci :02:00.0: DMA-API: device driver frees DMA 
> memory with different direction [device address=0x2d13] 
> [size=63800 bytes] [mapped with DMA_BIDIRECTIONAL] [unmapped with 
> DMA_TO_DEVICE]
> [  481.666703] Modules linked in: ctr ccm ath10k_pci(E-) ath10k_core(E) 
> ath(E) mac80211(E) cfg80211(E) snd_hda_codec_hdmi snd_hda_codec_idt 
> snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hda_core snd_hwdep 
> snd_pcm snd_seq_midi arc4 snd_rawmidi snd_seq_midi_event snd_seq btusb 
> btintel snd_seq_device joydev coret
> [  481.671468] CPU: 0 PID: 1978 Comm: rmmod Tainted: GE   
> 4.9.0-rc7-wt+ #54
> [  481.671478] Hardware name: Hewlett-Packard HP ProBook 6540b/1722, BIOS 
> 68CDD Ver. F.04 01/27/2010
> [  481.671489]  ef49dcec c842ee92 c8b5830e ef49dd34 ef49dd20 c80850f5 
> c8b5a13c ef49dd50
> [  481.671560]  07ba c8b5830e 0483 c8461830 c8461830 0483 
> ef49ddcc f34e64b8
> [  481.671641]  c8b58360 ef49dd3c c80851bb 0009  ef49dd34 
> c8b5a13c ef49dd50
> [  481.671716] Call Trace:
> [  481.671731]  [] dump_stack+0x76/0xb4
> [  481.671745]  [] __warn+0xe5/0x100
> [  481.671757]  [] ? check_unmap+0x320/0x8e0
> [  481.671769]  [] ? check_unmap+0x320/0x8e0
> [  481.671780]  [] warn_slowpath_fmt+0x3b/0x40
> [  481.671791]  [] check_unmap+0x320/0x8e0
> [  481.671804]  [] debug_dma_unmap_page+0x84/0xa0
> [  481.671835]  [] ath10k_wmi_free_host_mem+0x9a/0xe0 [ath10k_core]
> [  481.671861]  [] ath10k_core_destroy+0x50/0x60 [ath10k_core]
> [  481.671875]  [] ath10k_pci_remove+0x79/0xa0 [ath10k_pci]
> [  481.671889]  [] pci_device_remove+0x38/0xb0
> [  481.671901]  [] __device_release_driver+0x7b/0x110
> [  481.671913]  [] driver_detach+0x97/0xa0
> [  481.671923]  [] bus_remove_driver+0x4b/0xb0
> [  481.671934]  [] driver_unregister+0x2a/0x60
> [  481.671949]  [] pci_unregister_driver+0x18/0x70
> [  481.671965]  [] ath10k_pci_exit+0xd/0x25f [ath10k_pci]
> [  481.671979]  [] SyS_delete_module+0xf4/0x180
> [  481.671995]  [] ? __might_fault+0x8b/0xa0
> [  481.672009]  [] do_fast_syscall_32+0xa0/0x1e0
> [  481.672025]  [] sysenter_past_esp+0x45/0x74
> [  481.672037] ---[ end trace 3fd23759e17e1622 ]---
> [  481.672049] Mapped at:
> [  481.672060]  [  481.672072] [] 
> debug_dma_map_page.part.25+0x1c/0xf0
> [  481.672083]  [  481.672095] [] debug_dma_map_page+0x99/0xc0
> [  481.672106]  [  481.672132] [] 
> ath10k_wmi_alloc_chunk+0x12c/0x1f0 [ath10k_core]
> [  481.672142]  [  481.672168] [] 
> ath10k_wmi_event_service_ready_work+0x304/0x540 [ath10k_core]
> [  481.672178]  [  481.672190] [] process_one_work+0x1c3/0x670
> [  482.137134] ath10k_pci :02:00.0: pci irq msi oper_irq_mode 2 irq_mode 
> 0 reset_mode 0
> [  482.313144] ath10k_pci :02:00.0: Direct firmware load for 
> ath10k/pre-cal-pci-:02:00.0.bin failed with error -2
> [  482.313274] ath10k_pci :02:00.0: Direct firmware load for 
> ath10k/cal-pci-:02:00.0.bin failed with error -2
> [  482.313768] ath10k_pci :02:00.0: qca988x hw2.0 target 0x4100016c 
> chip_id 0x043202ff sub :
> [  482.313777] ath10k_pci :02:00.0: kconfig debug 1 debugfs 1 tracing 1 
> dfs 0 testmode 1
> [  482.313974] ath10k_pci :02:00.0: firmware ver 10.2.4.70.59-2 api 5 
> features no-p2p,raw-mode,mfp,allows-mesh-bcast crc32 4159f498
> [  482.369858] ath10k_pci :02:00.0: Direct firmware load for 
> ath10k/QCA988X/hw2.0/board-2.bin failed with error -2
> [  482.370011] ath10k_pci :02:00.0: board_file api 1 bmi_id N/A crc32 
> bebc7c08
> [  483.596770] ath10k_pci :02:00.0: htt-ver 2.1 wmi-op 5 htt-op 2 cal otp 
> max-sta 128 raw 0 hwcrypto 1
> [  483.701686] ath: EEPROM regdomain: 0x0
> [  483.701706] ath: EEPROM indicates default country code should be used
> [  483.701713] ath: doing EEPROM country->regdmn map search
> [  483.701721] ath: country maps to regdmn code: 0x3a
> [  483.701730] ath: Country alpha2 being used: US
> [  483.701737] ath: Regpair used: 0x3a
>
> Reported-by: Kalle Valo 
> Signed-off-by: Ben Greear 

Thanks, I don't see the warning anymore so this seems to be fixed. I'll
push this to 4.10.

-- 
Kalle Valo

Re: [v5,1/5] soc: qcom: smem_state: Fix include for ERR_PTR()

[Valo, Kalle]

Hi Dave,

Andy Gross <andy.gr...@linaro.org> writes:

> On 1 December 2016 at 04:17, Valo, Kalle <kv...@qca.qualcomm.com> wrote:
>> Kalle Valo <kv...@qca.qualcomm.com> writes:
>>
>>> It found the same problem. Interestingly I'm also building x86 with 32
>>> bit, maybe it's related?
>>>
>>> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git 
>>> pending
>>> head:   1ea16a1c457939b4564643f7637d5cc639a8d3b7
>>> commit: 5eb09c672b01460804fd49b1c9cc7d1072a102f0 [96/99] wcn36xx: 
>>> Transition driver to SMD client
>>> config: i386-allmodconfig (attached as .config)
>>> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
>>> reproduce:
>>> git checkout 5eb09c672b01460804fd49b1c9cc7d1072a102f0
>>> # save the attached .config to linux build tree
>>> make ARCH=i386
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>>> ERROR: "qcom_wcnss_open_channel" 
>>>>> [drivers/net/wireless/ath/wcn36xx/wcn36xx.ko] undefined!
>>
>> Bjorn mentioned me on IRC that this is because of a missing commit in my
>> tree:
>>
>> daa6e41ce2b5 soc: qcom: wcnss_ctrl: Stub wcnss_ctrl API
>>
>> When I pull the tag below (which contains the above commit) wcn36xx
>> builds fine for me:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/agross/linux.git 
>> tags/qcom-drivers-for-4.10
>>
>> Andy, is it ok if I pull your tag also to my ath.git tree to solve the
>> wcn36xx build problem? My trees go to Linus via net-next and I don't
>> know when exactly Dave would send a pull request to Linus, before or
>> after the arm trees, but as the tag seems to contain only few patches I
>> hope it doesn't matter.
>
> The qcom-drivers-for-4.10 tag was already merged into arm-soc.  But
> having you pull it as well won't cause issues so long as you are using
> the tag (which you are).  I don't see any issues with this approach.

Andy, thanks for the confirmation.

Dave, how do you suggest to handle depency issues like this? I have
pending important wcn36xx patches (converting the driver to use the
recently introduced proper SMD subsystem) which have a build dependency
on a commit which is in Andy's tag qcom-drivers-for-4.10. The commit in
question is currently in arm-soc tree going to 4.10, but not in your
net-next tree. I assume Linus will pull that during the next merge
window.

What I'm planning to do is to pull tag qcom-drivers-for-4.10 to my tree
and then send the patches to you. This will mean that from my pull
request you would get four new qcom-drivers commits which are not in
your tree, yet. Or do you prefer that I wait the qcom-drivers commits
trickle down from Linux until I send you wcn36xx patches? Or something
else?

$ git pull git://git.kernel.org/pub/scm/linux/kernel/git/agross/linux.git 
tags/qcom-drivers-for-4.10
>From git://git.kernel.org/pub/scm/linux/kernel/git/agross/linux
 * tag qcom-drivers-for-4.10 -> FETCH_HEAD
Auto-merging MAINTAINERS
Merge made by the 'recursive' strategy.
 MAINTAINERS  |1 +
 drivers/firmware/qcom_scm.c  |4 +++-
 include/dt-bindings/pinctrl/qcom,pmic-gpio.h |4 
 include/dt-bindings/pinctrl/qcom,pmic-mpp.h  |6 ++
 include/linux/soc/qcom/wcnss_ctrl.h  |   13 +
 5 files changed, 27 insertions(+), 1 deletion(-)

$ git log --oneline ORIG_HEAD..
6d0491261ecc Merge tag 'qcom-drivers-for-4.10' of 
git://git.kernel.org/pub/scm/linux/kernel/git/agross/linux into test
bd4760ca0315 firmware: qcom: scm: Use devm_reset_controller_register()
4fb1a4207804 MAINTAINERS: add drivers/pinctrl/qcom to ARM/QUALCOMM SUPPORT
636959fc1232 pinctrl: pm8994: add pad voltage regulator defines
daa6e41ce2b5 soc: qcom: wcnss_ctrl: Stub wcnss_ctrl API
$

-- 
Kalle Valo

Re: ath10k: wmi-alloc-chunk should use DMA_BIDIRECTIONAL.

[Valo, Kalle]

gree...@candelatech.com writes:

> From: Ben Greear 
>
> These memory chunks are often used as 'swap' by the NIC,
> so it will be both reading and writing to these areas.
>
> This seems to fix errors like this on my x86-64 machine:
>
> kernel: DMAR: DMAR:[DMA Write] Request device [05:00.0] fault addr ff5de000
> DMAR:[fault reason 05] PTE Write access is not set

Noticed this in my log, but I don't have time to investigate this in
detail right now:

[  413.795346] IPv6: ADDRCONF(NETDEV_UP): wlan0: link is not ready
[  414.158755] IPv6: ADDRCONF(NETDEV_CHANGE): wlan0: link becomes ready
[  477.439659] ath10k_pci :02:00.0: could not get mac80211 beacon
[  481.30] [ cut here ]
[  481.69] WARNING: CPU: 0 PID: 1978 at lib/dma-debug.c:1155 
check_unmap+0x320/0x8e0
[  481.88] ath10k_pci :02:00.0: DMA-API: device driver frees DMA memory 
with different direction [device address=0x2d13] [size=63800 bytes] 
[mapped with DMA_BIDIRECTIONAL] [unmapped with DMA_TO_DEVICE]
[  481.666703] Modules linked in: ctr ccm ath10k_pci(E-) ath10k_core(E) ath(E) 
mac80211(E) cfg80211(E) snd_hda_codec_hdmi snd_hda_codec_idt 
snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hda_core snd_hwdep 
snd_pcm snd_seq_midi arc4 snd_rawmidi snd_seq_midi_event snd_seq btusb btintel 
snd_seq_device joydev coret
[  481.671468] CPU: 0 PID: 1978 Comm: rmmod Tainted: GE   
4.9.0-rc7-wt+ #54
[  481.671478] Hardware name: Hewlett-Packard HP ProBook 6540b/1722, BIOS 68CDD 
Ver. F.04 01/27/2010
[  481.671489]  ef49dcec c842ee92 c8b5830e ef49dd34 ef49dd20 c80850f5 c8b5a13c 
ef49dd50
[  481.671560]  07ba c8b5830e 0483 c8461830 c8461830 0483 ef49ddcc 
f34e64b8
[  481.671641]  c8b58360 ef49dd3c c80851bb 0009  ef49dd34 c8b5a13c 
ef49dd50
[  481.671716] Call Trace:
[  481.671731]  [] dump_stack+0x76/0xb4
[  481.671745]  [] __warn+0xe5/0x100
[  481.671757]  [] ? check_unmap+0x320/0x8e0
[  481.671769]  [] ? check_unmap+0x320/0x8e0
[  481.671780]  [] warn_slowpath_fmt+0x3b/0x40
[  481.671791]  [] check_unmap+0x320/0x8e0
[  481.671804]  [] debug_dma_unmap_page+0x84/0xa0
[  481.671835]  [] ath10k_wmi_free_host_mem+0x9a/0xe0 [ath10k_core]
[  481.671861]  [] ath10k_core_destroy+0x50/0x60 [ath10k_core]
[  481.671875]  [] ath10k_pci_remove+0x79/0xa0 [ath10k_pci]
[  481.671889]  [] pci_device_remove+0x38/0xb0
[  481.671901]  [] __device_release_driver+0x7b/0x110
[  481.671913]  [] driver_detach+0x97/0xa0
[  481.671923]  [] bus_remove_driver+0x4b/0xb0
[  481.671934]  [] driver_unregister+0x2a/0x60
[  481.671949]  [] pci_unregister_driver+0x18/0x70
[  481.671965]  [] ath10k_pci_exit+0xd/0x25f [ath10k_pci]
[  481.671979]  [] SyS_delete_module+0xf4/0x180
[  481.671995]  [] ? __might_fault+0x8b/0xa0
[  481.672009]  [] do_fast_syscall_32+0xa0/0x1e0
[  481.672025]  [] sysenter_past_esp+0x45/0x74
[  481.672037] ---[ end trace 3fd23759e17e1622 ]---
[  481.672049] Mapped at:
[  481.672060]  [  481.672072] [] debug_dma_map_page.part.25+0x1c/0xf0
[  481.672083]  [  481.672095] [] debug_dma_map_page+0x99/0xc0
[  481.672106]  [  481.672132] [] ath10k_wmi_alloc_chunk+0x12c/0x1f0 
[ath10k_core]
[  481.672142]  [  481.672168] [] 
ath10k_wmi_event_service_ready_work+0x304/0x540 [ath10k_core]
[  481.672178]  [  481.672190] [] process_one_work+0x1c3/0x670
[  482.137134] ath10k_pci :02:00.0: pci irq msi oper_irq_mode 2 irq_mode 0 
reset_mode 0
[  482.313144] ath10k_pci :02:00.0: Direct firmware load for 
ath10k/pre-cal-pci-:02:00.0.bin failed with error -2
[  482.313274] ath10k_pci :02:00.0: Direct firmware load for 
ath10k/cal-pci-:02:00.0.bin failed with error -2
[  482.313768] ath10k_pci :02:00.0: qca988x hw2.0 target 0x4100016c chip_id 
0x043202ff sub :
[  482.313777] ath10k_pci :02:00.0: kconfig debug 1 debugfs 1 tracing 1 dfs 
0 testmode 1
[  482.313974] ath10k_pci :02:00.0: firmware ver 10.2.4.70.59-2 api 5 
features no-p2p,raw-mode,mfp,allows-mesh-bcast crc32 4159f498
[  482.369858] ath10k_pci :02:00.0: Direct firmware load for 
ath10k/QCA988X/hw2.0/board-2.bin failed with error -2
[  482.370011] ath10k_pci :02:00.0: board_file api 1 bmi_id N/A crc32 
bebc7c08
[  483.596770] ath10k_pci :02:00.0: htt-ver 2.1 wmi-op 5 htt-op 2 cal otp 
max-sta 128 raw 0 hwcrypto 1
[  483.701686] ath: EEPROM regdomain: 0x0
[  483.701706] ath: EEPROM indicates default country code should be used
[  483.701713] ath: doing EEPROM country->regdmn map search
[  483.701721] ath: country maps to regdmn code: 0x3a
[  483.701730] ath: Country alpha2 being used: US
[  483.701737] ath: Regpair used: 0x3a

-- 
Kalle Valo

Re: ath10k: Fix soft lockup during firmware crash/hw-restart

[Valo, Kalle]

Mohammed Shafi Shajakhan  writes:

> On Tue, Nov 29, 2016 at 10:16:50AM -0800, Ben Greear wrote:
>> Is this something for stable?  And if so, how far back should it be applied?
>
> @Kalle,
>
> [shafi] kindly suggest. If i am not wrong this is only needed for 4.9

Correct, commit 3c97f5de1f28 went to 4.9-rc1, it was not in 4.8. I'll
add CC stable to the commit log.

-- 
Kalle Valo

Re: [PATCH v2] ath10k: Fix soft lockup during firmware crash/hw-restart

[Valo, Kalle]

Mohammed Shafi Shajakhan  writes:

> From: Mohammed Shafi Shajakhan 
>
> During firmware crash (or) user requested manual restart
> the system gets into a soft lock up state because of the
> below root cause.
>
> During user requested hardware restart / firmware crash
> the system goes into a soft lockup state as 'napi_synchronize'
> is called after 'napi_disable' (which sets 'NAPI_STATE_SCHED'
> bit) and it sleeps into infinite loop as it waits for
> 'NAPI_STATE_SCHED' to be cleared. This condition is hit because
> 'ath10k_hif_stop' is called twice as below (resulting in calling
> 'napi_synchronize' after 'napi_disable')
>
> 'ath10k_core_restart' -> 'ath10k_hif_stop' (ATH10K_STATE_ON) ->
> -> 'ieee80211_restart_hw' -> 'ath10k_start' -> 'ath10k_halt' ->
> 'ath10k_core_stop' -> 'ath10k_hif_stop' (ATH10K_STATE_RESTARTING)
>
> Fix this by calling 'ath10k_halt' in ath10k_core_restart itself
> as it makes more sense before informing mac80211 to restart h/w
> Also remove 'ath10k_halt' in ath10k_start for the state of 'restarting'
>
> Fixes: 3c97f5de1f28 ("ath10k: implement NAPI support")
> Signed-off-by: Mohammed Shafi Shajakhan 
> ---
> [v2 Added Fixes ]

I'll also add:

Cc:  # v4.9

-- 
Kalle Valo

Re: [v5,1/5] soc: qcom: smem_state: Fix include for ERR_PTR()

[Valo, Kalle]

Kalle Valo <kv...@qca.qualcomm.com> writes:

> Kalle Valo <kv...@qca.qualcomm.com> writes:
>
>> "Valo, Kalle" <kv...@qca.qualcomm.com> writes:
>>
>>> Bjorn Andersson <bjorn.anders...@linaro.org> writes:
>>>
>>>> On Wed 16 Nov 10:49 PST 2016, Kalle Valo wrote:
>>>>
>>>>> Bjorn Andersson <bjorn.anders...@linaro.org> wrote:
>>>>> > The correct include file for getting errno constants and ERR_PTR() is
>>>>> > linux/err.h, rather than linux/errno.h, so fix the include.
>>>>> > 
>>>>> > Fixes: e8b123e60084 ("soc: qcom: smem_state: Add stubs for disabled 
>>>>> > smem_state")
>>>>> > Acked-by: Andy Gross <andy.gr...@linaro.org>
>>>>> > Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
>>>>> 
>>>>> For some reason this fails to compile now. Can you take a look, please?
>>>>> 
>>>>> ERROR: "qcom_wcnss_open_channel" 
>>>>> [drivers/net/wireless/ath/wcn36xx/wcn36xx.ko] undefined!
>>>>> make[1]: *** [__modpost] Error 1
>>>>> make: *** [modules] Error 2
>>>>> 
>>>>> 5 patches set to Changes Requested.
>>>>> 
>>>>> 9429045 [v5,1/5] soc: qcom: smem_state: Fix include for ERR_PTR()
>>>>> 9429047 [v5,2/5] wcn36xx: Transition driver to SMD client
>>>>
>>>> This patch was updated with the necessary depends in Kconfig to catch
>>>> this exact issue and when I pull in your .config (which has QCOM_SMD=n,
>>>> QCOM_WCNSS_CTRL=n and WCN36XX=y) I can build this just fine.
>>>>
>>>> I've tested the various combinations and it seems to work fine. Do you
>>>> have any other patches in your tree?
>>>
>>> This was with the pending branch of my ath.git tree. There are other
>>> wireless patches (ath10k etc) but I would guess they don't affect here.
>>>
>>>> Any stale objects?
>>>
>>> Not sure what you mean with this question, but I didn't run 'make clean'
>>> if that's what you are asking.
>>>
>>>> Would you mind retesting this, before I invest more time in trying to
>>>> reproduce the issue you're seeing?
>>>
>>> Sure, I'll take a look but that might take few days.
>>
>> I didn't find enough time to look at this in detail. I applied this to
>> my ath.git pending branch, let's see what the kbuild bot finds.
>
> It found the same problem. Interestingly I'm also building x86 with 32
> bit, maybe it's related?
>
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git pending
> head:   1ea16a1c457939b4564643f7637d5cc639a8d3b7
> commit: 5eb09c672b01460804fd49b1c9cc7d1072a102f0 [96/99] wcn36xx: Transition 
> driver to SMD client
> config: i386-allmodconfig (attached as .config)
> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> reproduce:
> git checkout 5eb09c672b01460804fd49b1c9cc7d1072a102f0
> # save the attached .config to linux build tree
> make ARCH=i386 
>
> All errors (new ones prefixed by >>):
>
>>> ERROR: "qcom_wcnss_open_channel" 
>>> [drivers/net/wireless/ath/wcn36xx/wcn36xx.ko] undefined!

Bjorn mentioned me on IRC that this is because of a missing commit in my
tree:

daa6e41ce2b5 soc: qcom: wcnss_ctrl: Stub wcnss_ctrl API

When I pull the tag below (which contains the above commit) wcn36xx
builds fine for me:

git://git.kernel.org/pub/scm/linux/kernel/git/agross/linux.git 
tags/qcom-drivers-for-4.10

Andy, is it ok if I pull your tag also to my ath.git tree to solve the
wcn36xx build problem? My trees go to Linus via net-next and I don't
know when exactly Dave would send a pull request to Linus, before or
after the arm trees, but as the tag seems to contain only few patches I
hope it doesn't matter.

-- 
Kalle Valo

Re: [v5,1/5] soc: qcom: smem_state: Fix include for ERR_PTR()

[Valo, Kalle]

Kalle Valo <kv...@qca.qualcomm.com> writes:

> "Valo, Kalle" <kv...@qca.qualcomm.com> writes:
>
>> Bjorn Andersson <bjorn.anders...@linaro.org> writes:
>>
>>> On Wed 16 Nov 10:49 PST 2016, Kalle Valo wrote:
>>>
>>>> Bjorn Andersson <bjorn.anders...@linaro.org> wrote:
>>>> > The correct include file for getting errno constants and ERR_PTR() is
>>>> > linux/err.h, rather than linux/errno.h, so fix the include.
>>>> > 
>>>> > Fixes: e8b123e60084 ("soc: qcom: smem_state: Add stubs for disabled 
>>>> > smem_state")
>>>> > Acked-by: Andy Gross <andy.gr...@linaro.org>
>>>> > Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
>>>> 
>>>> For some reason this fails to compile now. Can you take a look, please?
>>>> 
>>>> ERROR: "qcom_wcnss_open_channel" 
>>>> [drivers/net/wireless/ath/wcn36xx/wcn36xx.ko] undefined!
>>>> make[1]: *** [__modpost] Error 1
>>>> make: *** [modules] Error 2
>>>> 
>>>> 5 patches set to Changes Requested.
>>>> 
>>>> 9429045 [v5,1/5] soc: qcom: smem_state: Fix include for ERR_PTR()
>>>> 9429047 [v5,2/5] wcn36xx: Transition driver to SMD client
>>>
>>> This patch was updated with the necessary depends in Kconfig to catch
>>> this exact issue and when I pull in your .config (which has QCOM_SMD=n,
>>> QCOM_WCNSS_CTRL=n and WCN36XX=y) I can build this just fine.
>>>
>>> I've tested the various combinations and it seems to work fine. Do you
>>> have any other patches in your tree?
>>
>> This was with the pending branch of my ath.git tree. There are other
>> wireless patches (ath10k etc) but I would guess they don't affect here.
>>
>>> Any stale objects?
>>
>> Not sure what you mean with this question, but I didn't run 'make clean'
>> if that's what you are asking.
>>
>>> Would you mind retesting this, before I invest more time in trying to
>>> reproduce the issue you're seeing?
>>
>> Sure, I'll take a look but that might take few days.
>
> I didn't find enough time to look at this in detail. I applied this to
> my ath.git pending branch, let's see what the kbuild bot finds.

It found the same problem. Interestingly I'm also building x86 with 32
bit, maybe it's related?

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git pending
head:   1ea16a1c457939b4564643f7637d5cc639a8d3b7
commit: 5eb09c672b01460804fd49b1c9cc7d1072a102f0 [96/99] wcn36xx: Transition 
driver to SMD client
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
git checkout 5eb09c672b01460804fd49b1c9cc7d1072a102f0
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

>> ERROR: "qcom_wcnss_open_channel" 
>> [drivers/net/wireless/ath/wcn36xx/wcn36xx.ko] undefined!

-- 
Kalle Valo

Re: [v5,1/5] soc: qcom: smem_state: Fix include for ERR_PTR()

[Valo, Kalle]

"Valo, Kalle" <kv...@qca.qualcomm.com> writes:

> Bjorn Andersson <bjorn.anders...@linaro.org> writes:
>
>> On Wed 16 Nov 10:49 PST 2016, Kalle Valo wrote:
>>
>>> Bjorn Andersson <bjorn.anders...@linaro.org> wrote:
>>> > The correct include file for getting errno constants and ERR_PTR() is
>>> > linux/err.h, rather than linux/errno.h, so fix the include.
>>> > 
>>> > Fixes: e8b123e60084 ("soc: qcom: smem_state: Add stubs for disabled 
>>> > smem_state")
>>> > Acked-by: Andy Gross <andy.gr...@linaro.org>
>>> > Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
>>> 
>>> For some reason this fails to compile now. Can you take a look, please?
>>> 
>>> ERROR: "qcom_wcnss_open_channel" 
>>> [drivers/net/wireless/ath/wcn36xx/wcn36xx.ko] undefined!
>>> make[1]: *** [__modpost] Error 1
>>> make: *** [modules] Error 2
>>> 
>>> 5 patches set to Changes Requested.
>>> 
>>> 9429045 [v5,1/5] soc: qcom: smem_state: Fix include for ERR_PTR()
>>> 9429047 [v5,2/5] wcn36xx: Transition driver to SMD client
>>
>> This patch was updated with the necessary depends in Kconfig to catch
>> this exact issue and when I pull in your .config (which has QCOM_SMD=n,
>> QCOM_WCNSS_CTRL=n and WCN36XX=y) I can build this just fine.
>>
>> I've tested the various combinations and it seems to work fine. Do you
>> have any other patches in your tree?
>
> This was with the pending branch of my ath.git tree. There are other
> wireless patches (ath10k etc) but I would guess they don't affect here.
>
>> Any stale objects?
>
> Not sure what you mean with this question, but I didn't run 'make clean'
> if that's what you are asking.
>
>> Would you mind retesting this, before I invest more time in trying to
>> reproduce the issue you're seeing?
>
> Sure, I'll take a look but that might take few days.

I didn't find enough time to look at this in detail. I applied this to
my ath.git pending branch, let's see what the kbuild bot finds.

-- 
Kalle Valo

Re: [PATCH 2/3] ath9k: export configurable parameters of PTA

[Valo, Kalle]

miaoq...@codeaurora.org writes:

> From: Miaoqing Pan 
>
> Export time_extend, pritority_time, first_slot_time, wl_active_time
> and wl_qc_time those PTA(aka slotted mode) configurable parameters,
> allow user to change/debug the timing easily.
>
> Also set wl_active_time and wl_qc_time default value to 0, as in this
> period WLAN chip may send out ACK, and it will corrupt the BT(or other)
> received packet in the PTA cycle.
>
> Signed-off-by: Miaoqing Pan 

Please try to always write commit logs so that everyone will understand,
for example spelling out what PTA means would be nice.

> --- a/drivers/net/wireless/ath/ath9k/init.c
> +++ b/drivers/net/wireless/ath/ath9k/init.c
> @@ -77,6 +77,26 @@ struct ath9k_eeprom_ctx {
>  static int ath9k_btcoex_duty_cycle = ATH_BTCOEX_DEF_DUTY_CYCLE;
>  module_param_named(btcoex_duty_cycle, ath9k_btcoex_duty_cycle, int, 0444);
>  MODULE_PARM_DESC(btcoex_duty_cycle, "BT coexistence duty cycle");
> +
> +static int ath9k_btcoex_time_extend;
> +module_param_named(btcoex_time_extend, ath9k_btcoex_time_extend, int, 0444);
> +MODULE_PARM_DESC(btcoex_time_extend, "BT coexistence time extend");
> +
> +static int ath9k_btcoex_priority_time = 2;
> +module_param_named(btcoex_priority_time, ath9k_btcoex_priority_time, int, 
> 0444);
> +MODULE_PARM_DESC(btcoex_priority_time, "BT coexistence priority time");
> +
> +static int ath9k_btcoex_first_slot_time = 5;
> +module_param_named(btcoex_first_slot_time, ath9k_btcoex_first_slot_time, 
> int, 0444);
> +MODULE_PARM_DESC(btcoex_first_slot_time, "BT coexistence first slot time");
> +
> +static int ath9k_btcoex_wl_active_time;
> +module_param_named(btcoex_wl_active_time, ath9k_btcoex_wl_active_time, int, 
> 0444);
> +MODULE_PARM_DESC(btcoex_wl_active_time, "BT coexistence wlan active time");
> +
> +static int ath9k_btcoex_wl_qc_time;
> +module_param_named(btcoex_wl_qc_time, ath9k_btcoex_wl_qc_time, int, 0444);
> +MODULE_PARM_DESC(btcoex_wl_qc_time, "BT coexistence wlan quiet collision 
> time");
>  #endif

Same as with the previous patch, I don't think these should be set via
module parameters.

-- 
Kalle Valo

Re: [PATCH 1/3] ath9k: Add a module parameter to set btcoex duty cycle

[Valo, Kalle]

miaoq...@codeaurora.org writes:

> From: Miaoqing Pan 
>
> btcoex duty cyle allows user to balance the performance
> between WLAN and BT.
>
> Signed-off-by: Miaoqing Pan 

[...]

> --- a/drivers/net/wireless/ath/ath9k/init.c
> +++ b/drivers/net/wireless/ath/ath9k/init.c
> @@ -73,6 +73,12 @@ struct ath9k_eeprom_ctx {
>  
>  #endif /* CONFIG_ATH9K_CHANNEL_CONTEXT */
>  
> +#ifdef CONFIG_ATH9K_BTCOEX_SUPPORT
> +static int ath9k_btcoex_duty_cycle = ATH_BTCOEX_DEF_DUTY_CYCLE;
> +module_param_named(btcoex_duty_cycle, ath9k_btcoex_duty_cycle, int, 0444);
> +MODULE_PARM_DESC(btcoex_duty_cycle, "BT coexistence duty cycle");
> +#endif

I don't think module parameters are really meant for providing protocol
settings like this, especially as these would be global for all radios.
nl80211 (if used in production) or debugfs (if used only in testing) are
much better choises.

-- 
Kalle Valo

Re: [PATCH v2] ath9k: Introduce airtime fairness scheduling between stations

[Valo, Kalle]

(The make-wifi-fast list is annoying as it always spams me when it's on
CC, so dropped it.)

Toke Høiland-Jørgensen  writes:

> This reworks the ath9k driver to schedule transmissions to connected
> stations in a way that enforces airtime fairness between them. It
> accomplishes this by measuring the time spent transmitting to or
> receiving from a station at TX and RX completion, and accounting this to
> a per-station, per-QoS level airtime deficit. Then, an FQ-CoDel based
> deficit scheduler is employed at packet dequeue time, to control which
> station gets the next transmission opportunity.
>
> Airtime fairness can significantly improve the efficiency of the network
> when station rates vary. The following throughput values are from a
> simple three-station test scenario, where two stations operate at the
> highest HT20 rate, and one station at the lowest, and the scheduler is
> employed at the access point:
>
>   Before   /   After
> Fast station 1:19.17   /   25.09 Mbps
> Fast station 2:19.83   /   25.21 Mbps
> Slow station:   2.58   /1.77 Mbps
> Total: 41.58   /   52.07 Mbps
>
> The benefit of airtime fairness goes up the more stations are present.
> In a 30-station test with one station artificially limited to 1 Mbps,
> we have seen aggregate throughput go from 2.14 to 17.76 Mbps.
>
> Signed-off-by: Toke Høiland-Jørgensen 

[...]

> +void ath_acq_lock(struct ath_softc *sc, struct ath_acq *acq)
> + __acquires(>lock)
> +{
> + spin_lock_bh(>lock);
> +}
> +
> +void ath_acq_unlock(struct ath_softc *sc, struct ath_acq *acq)
> + __releases(>lock)
> +{
> + spin_unlock_bh(>lock);
> +}

Why these? To me it looks like they just add an extra function jump and
unneccessary extra layer.

-- 
Kalle Valo

Re: [PATCH v2 0/7] ath9k: EEPROM swapping improvements

[Valo, Kalle]

Kalle Valo  writes:

> Martin Blumenstingl  writes:
>
>> There are two types of swapping the EEPROM data in the ath9k driver.
>> Before this series one type of swapping could not be used without the
>> other.
>>
>> The first type of swapping looks at the "magic bytes" at the start of
>> the EEPROM data and performs swab16 on the EEPROM contents if needed.
>> The second type of swapping is EEPROM format specific and swaps
>> specific fields within the EEPROM itself (swab16, swab32 - depends on
>> the EEPROM format).
>>
>> With this series the second part now looks at the EEPMISC register
>> inside the EEPROM, which uses a bit to indicate if the EEPROM data
>> is Big Endian (this is also done by the FreeBSD kernel).
>> This has a nice advantage: currently there are some out-of-tree hacks
>> (in OpenWrt and LEDE) where the EEPROM has a Big Endian header on a
>> Big Endian system (= no swab16 is performed) but the EEPROM itself
>> indicates that it's data is Little Endian. Until now the out-of-tree
>> code simply did a swab16 before passing the data to ath9k, so ath9k
>> first did the swab16 - this also enabled the format specific swapping.
>> These out-of-tree hacks are still working with the new logic, but it
>> is recommended to remove them. This implementation is based on a
>> discussion with Arnd Bergmann who raised concerns about the
>> robustness and portability of the swapping logic in the original OF
>> support patch review, see [0].
>>
>> After a second round of patches (= v1 of this series) neither Arnd
>> Bergmann nor I were really happy with the complexity of the EEPROM
>> swapping logic. Based on a discussion (see [1] and [2]) we decided
>> that ath9k should use a defined format (specifying the endianness
>> of the data - I went with __le16 and __le32) when accessing the
>> EEPROM fields. A benefit of this is that we enable the EEPMISC based
>> swapping logic by default, just like the FreeBSD driver, see [3]. On
>> the devices which I have tested (see below) ath9k now works without
>> having to specify the "endian_check" field in ath9k_platform_data (or
>> a similar logic which could provide this via devicetree) as ath9k now
>> detects the endianness automatically. Only EEPROMs which are mangled
>> by some out-of-tree code still need the endian_check flag (or one can
>> simply remove that mangling from the out-of-tree code).
>>
>> Testing:
>> - tested by myself on AR9287 with Big Endian EEPROM
>> - tested by myself on AR9227 with Little Endian EEPROM
>> - tested by myself on AR9381 (using the ar9003_eeprom implementation,
>>   which did not suffer from this whole problem)
>> - how do we proceed with testing? maybe we could keep this in a
>>   feature-branch and add these patches to LEDE once we have an ACK to
>>   get more people to test this
>>
>> This series depends on my other series (v7):
>> "add devicetree support to ath9k" - see [4]
>
> I think this looks pretty good. If there's a bug somewhere it should be
> quite easy to fix so I'm not that worried and would be willing to take
> these as soon as I have applied the dependency series. IIRC your
> devicetree patches will have at least one more review round so that will
> take some time still. In the meantime it would be great if LEDE folks
> could take a look at these and comment (or test).

So are everyone happy with this? I haven't seen any comments. If I don't
here anything I'm planning to take these, most likely for 4.11.

-- 
Kalle Valo

Re: [v5,1/5] soc: qcom: smem_state: Fix include for ERR_PTR()

[Valo, Kalle]

Bjorn Andersson  writes:

> On Wed 16 Nov 10:49 PST 2016, Kalle Valo wrote:
>
>> Bjorn Andersson  wrote:
>> > The correct include file for getting errno constants and ERR_PTR() is
>> > linux/err.h, rather than linux/errno.h, so fix the include.
>> > 
>> > Fixes: e8b123e60084 ("soc: qcom: smem_state: Add stubs for disabled 
>> > smem_state")
>> > Acked-by: Andy Gross 
>> > Signed-off-by: Bjorn Andersson 
>> 
>> For some reason this fails to compile now. Can you take a look, please?
>> 
>> ERROR: "qcom_wcnss_open_channel" 
>> [drivers/net/wireless/ath/wcn36xx/wcn36xx.ko] undefined!
>> make[1]: *** [__modpost] Error 1
>> make: *** [modules] Error 2
>> 
>> 5 patches set to Changes Requested.
>> 
>> 9429045 [v5,1/5] soc: qcom: smem_state: Fix include for ERR_PTR()
>> 9429047 [v5,2/5] wcn36xx: Transition driver to SMD client
>
> This patch was updated with the necessary depends in Kconfig to catch
> this exact issue and when I pull in your .config (which has QCOM_SMD=n,
> QCOM_WCNSS_CTRL=n and WCN36XX=y) I can build this just fine.
>
> I've tested the various combinations and it seems to work fine. Do you
> have any other patches in your tree?

This was with the pending branch of my ath.git tree. There are other
wireless patches (ath10k etc) but I would guess they don't affect here.

> Any stale objects?

Not sure what you mean with this question, but I didn't run 'make clean'
if that's what you are asking.

> Would you mind retesting this, before I invest more time in trying to
> reproduce the issue you're seeing?

Sure, I'll take a look but that might take few days.

-- 
Kalle Valo

Re: [RFC 1/2] ath9k: work around AR_CFG 0xdeadbeef chip hang

[Valo, Kalle]

Sven Eckelmann  writes:

> From: Simon Wunderlich 
>
> QCA 802.11n chips (especially AR9330/AR9340) sometimes end up in a state in
> which a read of AR_CFG always returns 0xdeadbeef. This should not happen
> when when the power_mode of the device is ATH9K_PM_AWAKE.
>
> This problem is not yet detected by any other workaround in ath9k. No way
> is known to reproduce the problem easily.
>
> Signed-off-by: Simon Wunderlich 
> [sven.eckelm...@open-mesh.com: port to recent ath9k, add commit message]
> Signed-off-by: Sven Eckelmann 

[...]

> +void ath_hw_hang_work(struct work_struct *work)
> +{
> + struct ath_softc *sc = container_of(work, struct ath_softc,
> + hw_hang_work.work);
> +
> + if (ath_hw_hang_deadbeef(sc))
> + goto requeue_worker;
> +
> +requeue_worker:
> + ieee80211_queue_delayed_work(sc->hw, >hw_hang_work,
> +  msecs_to_jiffies(ATH_HANG_WORK_INTERVAL));
> +}

The goto doesn't make any sense, either me or the function is missing
something :)

-- 
Kalle Valo

Re: [RFC 02/12] ath10k: htc: rx trailer lookahead support

[Valo, Kalle]

Erik Stromdahl  writes:

> On 11/15/2016 10:57 AM, Michal Kazior wrote:
>> On 14 November 2016 at 17:33, Erik Stromdahl  
>> wrote:
>>> The RX trailer parsing is now capable of parsing lookahead reports.
>>> This is needed by SDIO/mbox.
>> 
>> It'd be useful to know what exactly lookahead will be used for. What
>> payload does it carry.
>> 
> It carries the 4 first bytes of the next RX message, i.e. the first 4
> bytes of an HTC header.
>
> It is used by the sdio interrupt handler to know if the next packet is a
> part of an RX bundle, which endpoint it belongs to and how long it is
> (so we know how many bytes to allocate).

Please add that to the commit log, it's useful information. Or maybe a
code comment would be better?

>>> +static int
>>> +ath10k_htc_process_lookahead(struct ath10k_htc *htc,
>>> +const struct ath10k_htc_lookahead_report 
>>> *report,
>>> +int len,
>>> +enum ath10k_htc_ep_id eid,
>>> +u32 *next_lk_ahds,
>> 
>> next_lk_ahds should be u8 or void. From what I understand by glancing
>> through the code it is an agnostic buffer that carries payload which
>> is later interpreted either as eid or htc header, right? void is
>> probably most suitable in this case for passing around and u8 for
>> inline-based storage.
>> 
> Sounds reasonable, I'll change to void pointer.
>
>> I'd also avoid silly abbreviations. Probably "lookahead" alone is enough.
>> 
> Ok, this abbreviation was actually taken from the ath6kl code. I think
> the intention was to reduce line lengths.

Most likely that comes from the staging ath6kl driver which again is a
fork of the Atheros internal driver. I agree with Michal, better to
avoid silly abbreviations as much as possible. Readability is most
important.

-- 
Kalle Valo

Re: [PATCH] ath9k: Move generic entries below specific ones in ath_pci_id_table.

[Valo, Kalle]

"Vittorio Gambaletta (VittGam)" <linux-wirel...@vittgam.net> writes:

> Hello,
>
> On 12/10/2016 17:01:08 CEST, Valo, Kalle wrote:
>
>> So to tell the full story I'll change the commit log to something like
>> below. Does it look ok to you?
>
> Yes; but I'd change "So" to "This turned out to be wrong", and add a note
> about changing the order for 0x002A too:
>
> --
> ath9k: Really fix LED polarity for some Mini PCI AR9220 MB92 cards.
>
> The active_high LED of my Wistron DNMA-92 is still being recognized as
> active_low on 4.7.6 mainline. When I was preparing my former commit
> 0f9edcdd88a9 ("ath9k: Fix LED polarity for some Mini PCI AR9220 MB92
> cards.") to fix that I must have somehow messed up with testing, because
> I tested the final version of that patch before sending it, and it was
> apparently working; but now it is not working on 4.7.6 mainline.
>
> I initially added the PCI_DEVICE_SUB section for 0x0029/0x2096 above the
> PCI_VDEVICE section for 0x0029; but then I moved the former below the
> latter after seeing how 0x002A sections were sorted in the file.
>
> This turned out to be wrong: if a generic PCI_VDEVICE entry (that has
> both subvendor and subdevice IDs set to PCI_ANY_ID) is put before a more
> specific one (PCI_DEVICE_SUB), then the generic PCI_VDEVICE entry will
> match first and will be used.
>
> With this patch, 0x0029/0x2096 has finally got active_high LED on 4.7.6.
>
> While I'm at it, let's fix 0x002A too by also moving its generic definition
> below its specific ones.
>
> Fixes: 0f9edcdd88a9 ("ath9k: Fix LED polarity for some Mini PCI AR9220 MB92 
> cards.")
> Cc: <sta...@vger.kernel.org> #4.7+
> Signed-off-by: Vittorio Gambaletta <linuxb...@vittgam.net>
> [kv...@qca.qualcomm.com: improve the commit log based on email discussions]
> Signed-off-by: Kalle Valo <kv...@qca.qualcomm.com>
> --

Thanks, I updated the commit with your changes.

-- 
Kalle Valo

Re: [OpenWrt-Devel] ATH10K VLAN firmware issue

[Valo, Kalle]

Bruno Antunes <baantu...@gmail.com> writes:

> On 7 November 2016 at 18:06, Valo, Kalle <kv...@qca.qualcomm.com> wrote:
>> Bruno Antunes <baantu...@gmail.com> writes:
>>
>>> On 4 November 2016 at 21:17, Valo, Kalle <kv...@qca.qualcomm.com> wrote:
>>>
>>>> Can someone file a bug to bugzilla about this so that all the info is
>>>> properly stored? The more comprehensive the report is the better.
>>>>
>>>> https://bugzilla.kernel.org/
>>>
>>> I will file a bug report.
>>
>> Thanks, it's good to store all in one place so that it's easier to find
>> the relevant info.
>
> Just file the bug with the ID 187241 - VLAN support in ATH10k Feel
> free to ask for adicional info. I did not mention any names in the bug
> report fell free to take credit if wanted.

Thanks. I'll report it to the firmware team, let's see what happens. If
there's more information which might help to fix this feel free to
update that to the bug report.

https://bugzilla.kernel.org/show_bug.cgi?id=187241

-- 
Kalle Valo

Re: [PATCH 3/4] dt: bindings: add new dt entry for BTCOEX feature in qcom, ath10k.txt

[Valo, Kalle]

(Adding devicetree list)

 writes:

> From: Tamizh chelvam 
>
> There two things done in this patch.
>
> 1) 'btcoex_support' flag for BTCOEX feature support by the hardware.
> 2) 'wlan_btcoex_gpio' is used to fill wlan priority pin number for
>BTCOEX priority feature support.
>
> Signed-off-by: Tamizh chelvam 
> ---
>  .../bindings/net/wireless/qcom,ath10k.txt  |4 
>  1 file changed, 4 insertions(+)

As this changes the device tree bindings you need to CC the device tree
list. Please resend the whole patchset (and mark it as v2).

-- 
Kalle Valo

Re: [RFC 10/12] ath10k: Added QCA65XX hw definition

[Valo, Kalle]

Michal Kazior  writes:

> On 14 November 2016 at 17:33, Erik Stromdahl  wrote:
>> Signed-off-by: Erik Stromdahl 
>> ---
>>  drivers/net/wireless/ath/ath10k/hw.h |1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/hw.h 
>> b/drivers/net/wireless/ath/ath10k/hw.h
>> index 46142e9..ef45ecf 100644
>> --- a/drivers/net/wireless/ath/ath10k/hw.h
>> +++ b/drivers/net/wireless/ath/ath10k/hw.h
>> @@ -224,6 +224,7 @@ enum ath10k_hw_rev {
>> ATH10K_HW_QCA9377,
>> ATH10K_HW_QCA4019,
>> ATH10K_HW_QCA9887,
>> +   ATH10K_HW_QCA65XX,
>
> Are you 100% positive that you're supporting all QCA65XX chips? The
> rule of thumb is to avoid Xs as much as possible unless totally
> perfectly completely sure.

But the thing is that nobody can't be absolutely sure as we never know
what marketing comes up within few years again. So I would say that XX
in chip names should be completely avoided to avoid any confusion. We
already suffer from this in ath10k with QCA988X and QCA9888 which are
different designs but the names overlap.

I haven't reviewed Erik's patches yet but I'm surprised why even a new
enum value is needed here. I was assuming we could use ATH10K_HW_QCA6174
because AFAIK they share the same design. Any particular reason for
this?

-- 
Kalle Valo

Re: [RFC 09/12] ath10k: Mailbox address definitions

[Valo, Kalle]

Michal Kazior  writes:

> On 14 November 2016 at 17:33, Erik Stromdahl  wrote:
>> Address definitions for SDIO/mbox based chipsets.
>>
>> Signed-off-by: Erik Stromdahl 
>> ---
>>  drivers/net/wireless/ath/ath10k/hw.h |   53 
>> ++
>>  1 file changed, 53 insertions(+)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/hw.h 
>> b/drivers/net/wireless/ath/ath10k/hw.h
>> index 883547f..46142e9 100644
>> --- a/drivers/net/wireless/ath/ath10k/hw.h
>> +++ b/drivers/net/wireless/ath/ath10k/hw.h
>> @@ -814,6 +814,59 @@ struct ath10k_hw_ops {
>>  #define QCA9887_EEPROM_ADDR_LO_MASK0x00ff
>>  #define QCA9887_EEPROM_ADDR_LO_LSB 16
>>
>> +#define MBOX_RESET_CONTROL_ADDRESS 0x
>> +#define MBOX_HOST_INT_STATUS_ADDRESS   0x0800
>> +#define MBOX_HOST_INT_STATUS_ERROR_LSB 7
>> +#define MBOX_HOST_INT_STATUS_ERROR_MASK0x0080
>
> I would again suggest using Jakub's bitfield GET_FIELD stuff to get
> rid of MASK+LSB and just have single define per register field. Kalle,
> thoughts?

Same comment as with the other patch, very much preferred but not a hard
requirement.

-- 
Kalle Valo

Re: [RFC 05/12] ath10k: htc: Added ATH10K_HTC_FLAG_BUNDLE_LSB

[Valo, Kalle]

Michal Kazior  writes:

> On 14 November 2016 at 17:33, Erik Stromdahl  wrote:
>> Signed-off-by: Erik Stromdahl 
>> ---
>>  drivers/net/wireless/ath/ath10k/htc.h |2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/htc.h 
>> b/drivers/net/wireless/ath/ath10k/htc.h
>> index 589800a..df16a04 100644
>> --- a/drivers/net/wireless/ath/ath10k/htc.h
>> +++ b/drivers/net/wireless/ath/ath10k/htc.h
>> @@ -62,6 +62,8 @@ enum ath10k_htc_rx_flags {
>> ATH10K_HTC_FLAG_BUNDLE_MASK = 0xF0
>>  };
>>
>> +#define ATH10K_HTC_FLAG_BUNDLE_LSB 4
>
> Just an idea - we could start using FIELD_GET() with
> ATH10K_HTC_FLAG_BUNDLE_MASK alone. I would love to see Jakub's
> bitfield stuff be used more. Kalle, thoughts?

Yeah, the bitfield macros are handy and we should definitely switch to
them (slowly). But definitely not a must for these SDIO patches, more
like a nice bonus.

-- 
Kalle Valo

Re: [v5] ath9k: Switch to using mac80211 intermediate software queues.

[Valo, Kalle]

Toke Høiland-Jørgensen  writes:

> Tim Shepard  writes:
>
>>> While at it, could you also add to the commit log some info how awesome this
>>> patch is from user's point of view and how it helps. For example, before and
>>> and after numbers and other results.
>>
>> That varies wildly, depending on many details of the scenario
>> (including the wireless capabilities of the clients connected to the
>> AP using this patch, and how far away those clients are). There's
>> really not enough room in a commit message to explain enough to make
>> any such claimed numbers reproducible.
>
> I disagree; it's quite straightforward to demonstrate a gain from this
> on an ath9k access point. And while of course the mac80211 queues is the
> reason for this, the commit that uses it (i.e. this one) can explain
> that and include some numbers.

Yeah, so with that you are basically answering "Why?" part in the commit
log so that others can also understand the motivation behind all this.

> I'll add that and resend :)

Thanks. And sorry for taking so long with this, I have been basically
travelling most of the last month. I should get back home on Friday and
things should get normal soon.

-- 
Kalle Valo

Re: ATH10K VLAN firmware issue

[Valo, Kalle]

Bruno Antunes <baantu...@gmail.com> writes:

> On 4 November 2016 at 21:17, Valo, Kalle <kv...@qca.qualcomm.com> wrote:
>> Bruno Antunes <baantu...@gmail.com> writes:
>>
>>> Old thread but I think the issue is still present.
>>>
>>> I'm running a setup with VLANs with WDS and ath10k cards.
>>>
>>> To make it work both cards must be loaded in rawmode, AP
>>> and Sta, and with no security.
>>>
>>> I'm using a OpenWrt trunk r49941 and the most recent firmware,
>>> 10.2.4.70.58, from Kalle ath10k firmware tree.
>>>
>>> Although it works the throughput is very bad.
>>> Are there any alternatives to improve the throughput.
>>
>> Can someone file a bug to bugzilla about this so that all the info is
>> properly stored? The more comprehensive the report is the better.
>>
>> https://bugzilla.kernel.org/
>
> I will file a bug report.

Thanks, it's good to store all in one place so that it's easier to find
the relevant info.

> But since it appears to be a firmware related issue
> under what category can fill in the bug?

You can file it under Drivers/network-wireless, AFAIK we don't have any
separate components for firmware bugs. Here's one example to follow:

https://bugzilla.kernel.org/show_bug.cgi?id=186161

-- 
Kalle Valo

Re: ATH10K VLAN firmware issue

[Valo, Kalle]

Bruno Antunes  writes:

> Old thread but I think the issue is still present.
>
> I'm running a setup with VLANs with WDS and ath10k cards.
>
> To make it work both cards must be loaded in rawmode, AP
> and Sta, and with no security.
>
> I'm using a OpenWrt trunk r49941 and the most recent firmware,
> 10.2.4.70.58, from Kalle ath10k firmware tree.
>
> Although it works the throughput is very bad.
> Are there any alternatives to improve the throughput.

Can someone file a bug to bugzilla about this so that all the info is
properly stored? The more comprehensive the report is the better.

https://bugzilla.kernel.org/

-- 
Kalle Valo

Re: [PATCH 09/10] iwlwifi: mvm: operate in dqa mode

[Valo, Kalle]

Kalle Valo  writes:

>> Since we've been working on this for so long and already use the term
>> DQA broadly, we thought it wouldn't be necessary to explain more when
>> we are finally enabling it by default.  But of course I can change that
>> if you prefer.
>
> I guessed I could find more information from the history and I know this
> is obvious to your team, but it's not obvious to everyone. The commit
> log should always answer the questions "Why?" and this isn't answering
> that. For example, I need this information when sending pull requests to
> Dave and I'm sure lots of other people find it useful as well,
> especially when enabling a new feature.
>
> So I'm not asking for a long essay, something like this would be
> adequate:
>
> "Run DQA flows by default, as long as the FW supports it. It's currently
> supported on 1234, 3456 and 7654, maybe more in the future. DQA improves
> latency when X is used or throughput when Y is disabled. On the downside
> it sometimes slows down throughput when using Z but that's still
> accetable as it's so rarely used."

Oh, I forgot that the acronym should be also spelled out in the commit
log.

-- 
Kalle Valo

Re: [RFC 0/5] ath6kl: non WMI data service support

[Valo, Kalle]

(Adding ath10k list)

Erik Stromdahl <erik.stromd...@gmail.com> writes:

> On 10/14/2016 09:34 AM, Valo, Kalle wrote:
>> Erik Stromdahl <erik.stromd...@gmail.com> writes:
>> 
>>> This patch series is intended to prepare the ath6kl driver
>>> for newer chipsets that doesn't use the current WMI data
>>> endpoints for data traffic.
>>>
>>> The chipset I have been working with (and used for testing)
>>> is QCA6584. It is SDIO based (at least the variant I have
>>> been using) with 802.11p WAVE DSRC capabilities.
>>>
>>> This chipset is different from the AR600X family in that
>>> it does not use the WMI data services (service id's 0x101
>>> to 0x104 ) for data traffic.
>>> Instead it uses the HTT data service for data and wmi unified
>>> for control messages.
>>> It is also different when it comes to mailbox addresses
>>> and HTC header format as well, but these differences are not
>>> part of this patch series.
>> 
>> Do you have more patches implemented, like something already working or
>> have just started?
>> 
>
> I have an implementation with all features I need so far, but the other
> patches will require cleanup before I can submit anything.
>
> I have been using the qcacld driver as a basis for the work and some of
> the stuff in that driver is not really compliant with the kernel coding
> style (to say the least).
>
> I have so far mainly been focused on getting all features up and running
> and that has (unfortunately) resulted in some copy-pasting from
> qcacld.

Can you share the current code you have somewhere so that I could take a
quick look? I don't care how ugly it is, I would just like to understand
what kind of changes are needed.

> I will start having a look at ath10k and see how much work it would be
> to add sdio support to it...

Thanks, let me know how it goes or if I can help somehow. My time is
limited but if nothing else I can give some tips.

-- 
Kalle Valo

Re: [RFC 0/5] ath6kl: non WMI data service support

[Valo, Kalle]

(Adding ath10k list to CC)

Erik Stromdahl  writes:

>> Exactly what I was thinking. When I saw terms like "HTT" and "unified
>> WMI" my first thought was that is this actually an ath10k based design?
>> The product numbers really don't give any indication what driver
>> supports, the division goes something like this:
>> 
>> * ath9k: "non-mobile" 11n chips
>> * ath6k: mobile 11n chips
>> * ath10k: mobile and "non-mobile" 11ac chips
>> 
>> For example QCA6174 is an 11ac mobile chip supported by ath10k. ath10k
>> only supports PCI bus at the moment, but I'm hoping someone would add
>> USB and SDIO support. Patches are very welcome.
>> 
>> I'm starting to suspect that QCA6584 is actually based on the same
>> design as QCA6174. If that's the case when we should also think if
>> instead we should add SDIO support to ath10k, maybe by taking relevant
>> parts from ath6kl?
>> 
>> I'll investigate more what this QCA6584 is, I haven't heard about it
>> before.
>> 
>
> The reason I have patched to ath6kl driver was that it is the
> only driver with sdio/mbox support.
>
> I was actually thinking of writing a new driver from scratch, but I
> thought that it was less work to modify the existing ath6kl driver.

Ok.

> I just haven't considered the option to add sdio/mbox support to ath10k.
> This is definitely something I will have a look at.
>
> I should mention that I have been using the qcacld2.0 driver as
> "documentation" of the chipset.
>
> The qcacld driver identifies the chipset as AR6320
>
> From the qcacld2.0 driver bmi_msg.h:
>
> #define TARGET_TYPE_AR63208
>
> Perhaps this can shed some light on what kind of chip this is?

I'm pretty sure that this is QCA6174 based design which is already
supported by ath10k. So my suggestion is to first look at adding SDIO
support to ath10k and see if that's feasible. We already have PCI code
split from the core code (ath10k_core.ko and ath10k_pci.ko) just because
I was expecting that we would add SDIO and USB support later. That has
just never happened due to lack of time, hopefully you can fix that now
;)

I haven't studied SDIO support at all yet but hopefully WMI, core.c and
mac.c won't need that much modifications. HTT and HTC most likely need
bigger changes. And then you would need to add sdio.c, similarly like we
have pci.c now. Keep in mind that later we might want to add USB support
also so if there's something which both SDIO and USB need to that would
need to be easily sharable. Actually after adding SDIO I hope USB would
be easier to add.

-- 
Kalle Valo

Re: [PATCH v2 2/2] ath10k: add VHT160 support

[Valo, Kalle]

Kalle Valo  writes:

> From: Sebastian Gottschall 
>
> This patch adds full VHT160 support for QCA9984 chipsets Tested on Netgear
> R7800. 80+80 is possible, but disabled so far since it seems to contain
> glitches like missing vht station flags (this may be firmware or mac80211
> related).
>
> Signed-off-by: Sebastian Gottschall 
> Patchwork-Id: 9335111

Oops, the patchwork id is a leftover from my patchwork script. I'll
remove it before I apply the patch.

-- 
Kalle Valo

Re: [RFC 0/5] ath6kl: non WMI data service support

[Valo, Kalle]

Steve deRosier  writes:

> Hi Eric,
>
> On Thu, Oct 13, 2016 at 9:39 AM, Erik Stromdahl
>  wrote:
>> This patch series is intended to prepare the ath6kl driver
>> for newer chipsets that doesn't use the current WMI data
>> endpoints for data traffic.
>>
>> The chipset I have been working with (and used for testing)
>> is QCA6584. It is SDIO based (at least the variant I have
>> been using) with 802.11p WAVE DSRC capabilities.
>>
>> This chipset is different from the AR600X family in that
>> it does not use the WMI data services (service id's 0x101
>> to 0x104 ) for data traffic.
>> Instead it uses the HTT data service for data and wmi unified
>> for control messages.
>> It is also different when it comes to mailbox addresses
>> and HTC header format as well, but these differences are not
>> part of this patch series.
>>
>
> I've only taken a quick look and I'll make specific comments to
> specific patches later, but I've got to ask the question, should these
> changes go into ath6kl or be a new driver?
>
> Just because the number starts with 6000 doesn't mean it's a ath6kl
> chip. The 10k series chips all start with 9000 for example, but they
> rate their own driver.
>
> You state that all of the underpinnings of the communication with the
> chip are totally different:
> * Doesn't use WMI
> * Different mailboxing
> * Different HTC layer
>
> If all of the commands and all of the communication layers to the chip
> are totally different, then perhaps it isn't an ath6kl chip. So if
> it's largely similar, then OK, but seems to me with the changes you're
> saying above, it's mostly different.
>
> I'm saying all that without any knowledge of this chip. My experience
> is limited to various versions of the 6003 and 6004 chips.

Exactly what I was thinking. When I saw terms like "HTT" and "unified
WMI" my first thought was that is this actually an ath10k based design?
The product numbers really don't give any indication what driver
supports, the division goes something like this:

* ath9k: "non-mobile" 11n chips
* ath6k: mobile 11n chips
* ath10k: mobile and "non-mobile" 11ac chips

For example QCA6174 is an 11ac mobile chip supported by ath10k. ath10k
only supports PCI bus at the moment, but I'm hoping someone would add
USB and SDIO support. Patches are very welcome.

I'm starting to suspect that QCA6584 is actually based on the same
design as QCA6174. If that's the case when we should also think if
instead we should add SDIO support to ath10k, maybe by taking relevant
parts from ath6kl?

I'll investigate more what this QCA6584 is, I haven't heard about it
before.

-- 
Kalle Valo

Re: [PATCH v2] ath10k: Cleanup calling ath10k_htt_rx_h_unchain

[Valo, Kalle]

Mohammed Shafi Shajakhan  writes:

> From: Mohammed Shafi Shajakhan 
>
> 'ath10k_htt_rx_h_unchain' needs to be called only if the return
> value from 'ath10k_htt_rx_amsdu_pop' is 1('chained msdu's'), this
> change makes it more explicit and avoids doing a skb_peek, fetching
> rx descriptor pointer, checking rx msdu decap format for the case of
> ret = 0 (unchained msdus). Found this change during code walk through,
> not sure if this addresses any issue.
>
> Signed-off-by: Mohammed Shafi Shajakhan 

Applied to ath-next, thanks.

(The SMTP server failed, needed to send this manually)

-- 
Kalle Valo

Re: [PATCH] rsi: update in vap_capabilities frame to device

[Valo, Kalle]

Kalle Valo  writes:

> Prameela Rani Garnepudi  writes:
>
>> added vap status(add/delete) field in vap capabilities frame to device
>> added sending vap capabilites frame(with vap status 'delete') in remove 
>> interface
>>
>> Signed-off-by: Prameela Rani Garnepudi 
>
> Why? How is this better now? I'm sure there is a good reason why you add
> this but we don't know as the commit tells nothing we don't already know
> just from looking at the patch.
>
> The most important part of commit log is to answer the question "Why?",
> not "What?".

Oh, and USE capitalisation, punctuation and so on in the commit log. It
will be archived to git.

-- 
Kalle Valo

Re: [PATCH 1/2] ath10k: add per peer htt tx stats support for 10.4

[Valo, Kalle]

Yeoh Chun-Yeow  writes:

>> My understanding is that 10.2 branch does not have this feature,
>> unfortunately.
>>
>
> Alright, noted.
>
> Is QCA988X going to have firmware 10.4 support?

I wish it would have but I don't know the current status.

-- 
Kalle Valo

Re: QCA9887 Firmware Memory Usage

[Valo, Kalle]

David Hutchison  writes:

> I am using a Mikrotik hAP AC Lite which has a QCA9887 radio. It
> appears to be working; however ath10k ( or the qca9887 firmware ) is
> utilizing 15 - 20mb of memory. I applied the kfree(caldata) patch, to
> insure there is no memory leak.
>
> It doesn't appear to be leaking, and it will run Ok with the high
> memory footprint. However the Mikrotik hAP AC Lite is only a 64mb
> platform; so when I initialize the 2nd radio with ath9k I get OOMs.
>
> It just doesn't seem right that when ath10k + qca9887 is in AP mode
> that it's utilizing as much memory as it is. I don't think the issue
> is with ath10k, I think it's the qca9887 firmware (
> firmware-5.bin_10.2.4-1.0-00013 ).
>
> Is there a newer build available that may potentially fix these memory
> issues? The latest one available on
> https://github.com/kvalo/ath10k-firmware/blob/master/QCA9887/hw1.0/firmware-5.bin_10.2.4-1.0-00013
> is 2 months old. Perhaps there is a newer build to test that may have
> the fix?
>
> Any thoughts as to what it could be? I can provide more information if needed.

64MB of RAM nowadays is not much and ath10k is not really tested in such
setups AFAIK. I guess you could try if reducing TARGET_10X_NUM_MSDU_DESC
helps with the memory consumption, but no idea if that would even work
without further modifications to ath10k.

Also if you have fq-codel enabled that might increase memory
consumption. Adding linux-wireless as people there might have better
ideas.

-- 
Kalle Valo

Re: [v2,1/2] ath10k: clean up HTT tx buffer allocation and free

[Valo, Kalle]

Mohammed Shafi Shajakhan  writes:

> Hi Kalle,
>
> On Tue, Oct 11, 2016 at 01:36:22PM +0200, Kalle Valo wrote:
>> Mohammed Shafi Shajakhan  wrote:
>> > From: Mohammed Shafi Shajakhan 
>> > 
>> > cleanup 'ath10k_htt_tx_alloc' by introducing the API's
>> > 'ath10k_htt_tx_alloc/free_{cont_txbuf, txdone_fifo} and
>> > re-use them whereever needed
>> > 
>> > Signed-off-by: Mohammed Shafi Shajakhan 
>> 
>> Applies but fails to build:
>
> [shafi] sorry for the trouble again, I just figured out I had a private patch 
> by
> mistake  and it worked for me, I will make sure that v3 is properly rebased
> without any errors.

Thanks. And no worries, I now have a script which does commit and build
tests semi-automatically and sends an email if it fails. Yay! This was a
good real life test for the script :)

-- 
Kalle Valo

Re: [PATCH 1/2] ath10k: add per peer htt tx stats support for 10.4

[Valo, Kalle]

 writes:

> From: Anilkumar Kolli 
>
> Per peer tx stats are part of 'HTT_10_4_T2H_MSG_TYPE_PEER_STATS'
> event, Firmware sends one HTT event for every four PPDUs.
> HTT payload has success pkts/bytes, failed pkts/bytes, retry
> pkts/bytes and rate info per ppdu.
> Peer stats are enabled through 'WMI_SERVICE_PEER_STATS',
> which are nowadays enabled by default.
>
> Parse peer stats and update the tx rate information per STA.
>
> tx rate, Peer stats are tested on QCA4019 with Firmware version
> 10.4-3.2.1-00028.
>
> Signed-off-by: Anilkumar Kolli 

This and patch 2 add few new warnings:

drivers/net/wireless/ath/ath10k/htt_rx.c: In function 
'ath10k_htt_fetch_peer_stats':
drivers/net/wireless/ath/ath10k/htt_rx.c:2279:3: warning: too many arguments 
for format [-Wformat-extra-args]
drivers/net/wireless/ath/ath10k/htt_rx.c:2285:17: warning: incorrect type in 
assignment (different base types)
drivers/net/wireless/ath/ath10k/htt_rx.c:2285:17:expected unsigned int 
[unsigned] [usertype] peer_id
drivers/net/wireless/ath/ath10k/htt_rx.c:2285:17:got restricted __le16 
[usertype] peer_id
drivers/net/wireless/ath/ath10k/debugfs_sta.c:84: space required before the 
open parenthesis '('

-- 
Kalle Valo

Re: [PATCH 1/2] ath10k: add per peer htt tx stats support for 10.4

[Valo, Kalle]

Yeoh Chun-Yeow  writes:

> On Fri, Oct 7, 2016 at 10:58 PM,   wrote:
>> From: Anilkumar Kolli 
>>
>> Per peer tx stats are part of 'HTT_10_4_T2H_MSG_TYPE_PEER_STATS'
>> event, Firmware sends one HTT event for every four PPDUs.
>> HTT payload has success pkts/bytes, failed pkts/bytes, retry
>> pkts/bytes and rate info per ppdu.
>> Peer stats are enabled through 'WMI_SERVICE_PEER_STATS',
>> which are nowadays enabled by default.
>>
>> Parse peer stats and update the tx rate information per STA.
>>
>> tx rate, Peer stats are tested on QCA4019 with Firmware version
>> 10.4-3.2.1-00028.
>>
>
> Is QCA988X supported? I have tried to test with
> firmware-5.bin_10.2.4.70.56 but not working.

My understanding is that 10.2 branch does not have this feature,
unfortunately.

-- 
Kalle Valo

Re: [PATCH v2] ath10k: cache calibration data when the core is stopped

[Valo, Kalle]

Marty Faltesek  writes:

> ath10k: cache calibration data when the core is stopped
>
> Caching calibration data allows it to be accessed when the
> device is not active.
>
> Signed-off-by: Marty Faltesek 

Thanks, I'll now send v3.

-- 
Kalle Valo

Re: [PATCH v2] ath10k: cache calibration data when the core is stopped

[Valo, Kalle]

Marty Faltesek  writes:

> Caching calibration data allows it to be accessed when the
> device is not active.
>
> ---

Signed-off-by missing. Can you send it as a reply to this message and
I'll add it to v3?

>  drivers/net/wireless/ath/ath10k/core.h  |  1 +
>  drivers/net/wireless/ath/ath10k/debug.c | 72 
> ++---
>  2 files changed, 31 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.h 
> b/drivers/net/wireless/ath/ath10k/core.h
> index 6e5aa2d..7274ebe 100644
> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -452,6 +452,7 @@ struct ath10k_debug {
>   u32 nf_cal_period;
>  
>   struct ath10k_fw_crash_data *fw_crash_data;
> + void *cal_data;

To properly document locking I'll move this under the "protected by
conf_mutex" comment.

> -static int ath10k_debug_cal_data_open(struct inode *inode, struct file *file)
> +static int ath10k_debug_cal_data_fetch(struct ath10k *ar)
>  {
> - struct ath10k *ar = inode->i_private;
> - void *buf;
>   u32 hi_addr;
>   __le32 addr;
>   int ret;
>  
> - mutex_lock(>conf_mutex);

I'll add lockdep_assert_held() here.

>  static ssize_t ath10k_debug_cal_data_read(struct file *file,
> -   char __user *user_buf,
> -   size_t count, loff_t *ppos)
> +  char __user *user_buf,
> +  size_t count, loff_t *ppos)
>  {
>   struct ath10k *ar = file->private_data;
> - void *buf = file->private_data;
>  
>   return simple_read_from_buffer(user_buf, count, ppos,
> -buf, ar->hw_params.cal_data_len);
> -}

I'll add locking to this function.

> @@ -2343,7 +2327,9 @@ int ath10k_debug_create(struct ath10k *ar)
>   ar->debug.fw_crash_data = vzalloc(sizeof(*ar->debug.fw_crash_data));
>   if (!ar->debug.fw_crash_data)
>   return -ENOMEM;
> -
> + ar->debug.cal_data = vzalloc(ar->hw_params.cal_data_len);
> + if (!ar->debug.cal_data)
> + return -ENOMEM;
>   INIT_LIST_HEAD(>debug.fw_stats.pdevs);
>   INIT_LIST_HEAD(>debug.fw_stats.vdevs);
>   INIT_LIST_HEAD(>debug.fw_stats.peers);
> @@ -2355,7 +2341,9 @@ int ath10k_debug_create(struct ath10k *ar)
>  void ath10k_debug_destroy(struct ath10k *ar)
>  {
>   vfree(ar->debug.fw_crash_data);
> + vfree(ar->debug.cal_data);
>   ar->debug.fw_crash_data = NULL;
> + ar->debug.cal_data = NULL;

Actually I gave you a bad advice, this won't work as
ar->hw_params.cal_data_len is not yet initialised, we initialise it only
after we have probed the capabilities during the first firmware boot. I
changed the allocation to a fixed length buffer for now, it's only few
kBs virtual memory anyway so it shouldn't matter to anyone.

I have now v3 ready and tested. I'll just need your Signed-off-by and I
can then submit it.

-- 
Kalle Valo

Re: [PATCH] ath10k: cache calibration data when the core is stopped.

[Valo, Kalle]

Marty Faltesek <mfalte...@google.com> writes:

> On Mon, Oct 3, 2016 at 3:46 AM, Valo, Kalle <kv...@qca.qualcomm.com> wrote:
>> Marty Faltesek <mfalte...@google.com> writes:
>>
>>> Caching calibration data allows it to be accessed when the
>>> device is not active.
>>>
>>> Signed-off-by: Marty Faltesek <mfalte...@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?

Sure, I can test it.

>> 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.

Actually I was first planning to push 9340953 to 4.9 and take your patch
to 4.10. But your patch is a cleaner approach to this and maybe I should
push that to 4.9 instead? Need to think a bit more.

-- 
Kalle Valo

Re: [PATCH 2/2] ath10k: add platform regulatory domain support

[Valo, Kalle]

Bartosz Markowski <bartosz.markow...@tieto.com> writes:

> On 14 September 2016 at 09:06, Valo, Kalle <kv...@qca.qualcomm.com> wrote:
>>
>> Bartosz Markowski <bartosz.markow...@tieto.com> writes:
>>
>> > On 12 September 2016 at 17:35, Valo, Kalle <kv...@qca.qualcomm.com> wrote:
>> >
>> >> > +#ifdef CONFIG_ACPI
>> >> > +#define WRD_METHOD "WRDD"
>> >> > +#define WRDD_WIFI  (0x07)
>> >> > +
>> >> > +static u32 ath10k_mac_wrdd_get_mcc(struct ath10k *ar, union 
>> >> > acpi_object *wrdd)
>> >> > +{
>> >>
>> >> I don't think the ifdef is really necessary, acpi.h should handle that
>> >> (hopefully). Also I changed the error handling to use standard error
>> >> values and changed the info messages to dbg, they are too spammy in my
>> >> opinion. Please check carefully my changes in the pending branch:
>> >>
>> >> https://git.kernel.org/cgit/linux/kernel/git/kvalo/ath.git/commit/?h=pending=fe91745381ec3999d8de6dedb07b396c82539717
>> >
>> > I'm OK with the changes, I have not tried that though, except of
>> > reviewing and compiling it (do not have access to the chromebook for
>> > next few days). If you want to wait with it until I test it, it's fine
>> > too.
>>
>> Ok, I'll wait for few days in case you have time to test it.
>
>
> Sorry, it took so long. I've final check this and can confirm the patch.

Thanks, I'll apply the patch soon. Do note that I changed more of the
warnings messages to debug level.

-- 
Kalle Valo

Re: [PATCH] ath10k: cache calibration data when the core is stopped.

[Valo, Kalle]

Marty Faltesek  writes:

> Caching calibration data allows it to be accessed when the
> device is not active.
>
> Signed-off-by: Marty Faltesek 

[...]

> +int
> +ath10k_cal_data_alloc(struct ath10k *ar, void **buf)
> +{

[...]

> + ath10k_cal_data_alloc(ar, >cal_data);
[...]

> + ret = ath10k_cal_data_alloc(ar, >private_data);

Pointer to pointer parameters can be a source of problems and if we
could use one shared buffer for both of these cases when it would
simplify the code and we would need the buf parameter at all.

-- 
Kalle Valo

Re: [PATCH] ath10k: cache calibration data when the core is stopped.

[Valo, Kalle]

Marty Faltesek  writes:

> Caching calibration data allows it to be accessed when the
> device is not active.
>
> Signed-off-by: Marty Faltesek 

No comma in the title, please.

What tree did you use as the baseline? This doesn't seem to apply to
ath.git:

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. 

> + 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?

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/

> + hi_addr = host_interest_item_address(HI_ITEM(hi_board_data));
> +
> + ret = ath10k_hif_diag_read(ar, hi_addr, , sizeof(addr));
> +
> + if (ret) {
> + ath10k_warn(ar, "failed to read hi_board_data address: %d\n", 
> ret);
> + } else {
> + ret = ath10k_hif_diag_read(ar, le32_to_cpu(addr), *buf,
> +QCA988X_CAL_DATA_LEN);
> + if (ret) {
> + ath10k_warn(ar, "failed to read calibration data: 
> %d\n", ret);
> + }
> + }

We try to avoid using else branches so that only error handling is
indented and the main code flow is not.

> + /*
> +  * We are up now, so no need to cache calibration data.
> +  */
> + vfree(ar->cal_data);
> +ar->cal_data = NULL;

Indentation looks wrongs here.

> @@ -1757,6 +1799,11 @@ void ath10k_core_stop(struct ath10k *ar)
>   lockdep_assert_held(>conf_mutex);
>   ath10k_debug_stop(ar);
>  
> + /*
> +  * Cache caclibration data while stopped.
> +  */
> + ath10k_cal_data_alloc(ar, >cal_data);

I like the idea that the calibration data is copied during stop(), but
can you do this from debug.c? For example, add ath10k_debug_stop() and
call it from there? I don't think any of this should be in core.c.

-- 
Kalle Valo

Re: [PATCH] ath10k: Fix spinlock use in coverage class hack

[Valo, Kalle]

Benjamin Berg  writes:

> ath10k_hw_qca988x_set_coverage_class needs to hold both conf_mutex and
> the data_lock spin lock for parts of the function. However, data_lock
> is only needed while storing the coverage_class to store the value that
> the card is configured to.
>
> Fix the locking issue by only holding data_lock for the required duration.
>
> Signed-off-by: Benjamin Berg 

Thanks, I also folded this with the patch in the pending branch.

> And yes, I fully agree with your points of it being rather fragile. But as
> you said, it should be entirely safe if not used.

That's good.

> Obviously a firmware implementation would be preferential.

That's a shame as this feature is quite often requested. But if the
firmware ever starts supporting the featrue we can then remove this hack
from ath10k.

> This locking issue was pretty unnecessary. Lets see if any more issues show
> up in a closer review.

I can't see the locking problem anymore so it seems to be fixed. I'll
fix some minor things and send v2. I'll also CC linux-wireless.

-- 
Kalle Valo

Re: [PATCH] ath10k: fix debug cal data file

[Valo, Kalle]

Nikolay Martynov  writes:

> It got broken by 0b8e3c4ca29fe2c0efd3d41a76e34a657b9f17a4
>
> Signed-off-by: Nikolay Martynov 

Good catch, I'll queue this to 4.9.

There was one checkpatch warning I fixed:

drivers/net/wireless/ath/ath10k/debug.c:1477: Prefer vmalloc(sizeof(*data)...) 
over vmalloc(sizeof(struct ath10k_debug_cal_data)...)

The commit log is quite short so added more information about the bug.
The full patch is in the pending branch.

Author: Nikolay Martynov 
Date:   Wed Sep 28 15:11:52 2016 +0300

ath10k: fix debug cal data file

Commit 0b8e3c4ca29f ("ath10k: move cal data len to hw_params") broke 
retrieving
the calibration data from cal_data debugfs file. The length of file was 
always
zero. The reason is:

static ssize_t ath10k_debug_cal_data_read(struct file *file,
  char __user *user_buf,
  size_t count, loff_t *ppos)
{
struct ath10k *ar = file->private_data;
void *buf = file->private_data;


This is obviously bogus, private_data cannot contain both struct ath10k and 
the
buffer. Fix it by introducing a new temporary structure for storing both the
length of the buffer and the actual buffer, then struct ath10k is not needed
anymore.

Fixes: 0b8e3c4ca29f ("ath10k: move cal data len to hw_params")
Cc: sta...@vger.kernel.org # 4.7+
Signed-off-by: Nikolay Martynov 
[kv...@qca.qualcomm.com: improve commit log, fix a checkpatch warning]
Signed-off-by: Kalle Valo 

-- 
Kalle Valo

Re: [PATCH v2 03/21] ath10k: Allow changing ath10k debug mask at runtime.

[Valo, Kalle]

Ben Greear <gree...@candelatech.com> writes:

> On 09/14/2016 07:06 AM, Valo, Kalle wrote:
>> gree...@candelatech.com writes:
>>
>>> From: Ben Greear <gree...@candelatech.com>
>>>
>>> Using debugfs.  More convenient than module options
>>> in some cases.
>>>
>>> Signed-off-by: Ben Greear <gree...@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: 0x4000\n"
>>> +   "FW:  0x8000\n"
>>> +   "ALL: 0x\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, );
>>> +   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 0x > /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.

The debug log levels are documented in the wiki:

https://wireless.wiki.kernel.org/en/users/drivers/ath10k/debug#debug_log_messages

And they are not supposed to change, there should be only additions.

-- 
Kalle Valo

Re: [PATCH v2 15/21] ath10k: support CT firmware flag.

[Valo, Kalle]

Ben Greear <gree...@candelatech.com> writes:

> On 09/14/2016 07:30 AM, Valo, Kalle wrote:
>> gree...@candelatech.com writes:
>>
>>> From: Ben Greear <gree...@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 <gree...@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.

You can do with firmware version string, add something like "-CT" (maybe
you already have?) and it's clearly visible in boot logs and also when
firmware crashes.

-- 
Kalle Valo

Re: [PATCH v2 10/21] ath10k: support logging ath10k_info as KERN_DEBUG

[Valo, Kalle]

Ben Greear <gree...@candelatech.com> writes:

> On 09/14/2016 07:19 AM, Valo, Kalle wrote:
>> gree...@candelatech.com writes:
>>
>>> From: Ben Greear <gree...@candelatech.com>
>>>
>>> Helps keep messages off of (serial) console when
>>> that is desired.
>>>
>>> Signed-off-by: Ben Greear <gree...@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?

It should be based on KERN_ log levels. I don't know what your kernel
does, but ath10k should be printing only very few messages with level
KERN_INFO or above, all of of the debug messages. So you should be
easily able to filter out all ath10k debug messages as they are sent
with KERN_DEBUG.

-- 
Kalle Valo

Re: [PATCH v2 09/21] ath10k: print fw debug messages in hex.

[Valo, Kalle]

Ben Greear <gree...@candelatech.com> writes:

> On 09/14/2016 07:18 AM, Valo, Kalle wrote:
>> gree...@candelatech.com writes:
>>
>>> From: Ben Greear <gree...@candelatech.com>
>>>
>>> This allows user-space tools to decode debug-log
>>> messages by parsing dmesg or /var/log/messages.
>>>
>>> Signed-off-by: Ben Greear <gree...@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.

Reinventing the wheel by using printk as the delivery mechanism doesn't
sound like a good idea. IIRC Emmanuel talked about some kind of firmware
debugging framework, he might have some ideas.

-- 
Kalle Valo

Re: [PATCH v2 08/21] ath10k: make firmware text debug messages more verbose.

[Valo, Kalle]

Ben Greear <gree...@candelatech.com> writes:

> On 09/14/2016 07:12 AM, Valo, Kalle wrote:
>> gree...@candelatech.com writes:
>>
>>> From: Ben Greear <gree...@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 <gree...@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.

I'm sure it's useful for you, but we have quite a few firmware versions
to support. We do not know what kind of messages they print.

-- 
Kalle Valo

Re: [PATCH v2 04/21] ath10k: rate-limit packet tx errors

[Valo, Kalle]

Ben Greear <gree...@candelatech.com> writes:

> On 09/14/2016 07:07 AM, Valo, Kalle wrote:
>> gree...@candelatech.com writes:
>>
>>> From: Ben Greear <gree...@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 <gree...@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 = 
>>  dev_warn_ratelimited(ar->dev, "%pV", );
>>  trace_ath10k_log_warn(ar, );
>>
>>  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 think this is a common problem in kernel, not specific to ath10k. For
starters you could configure the limits dev_warn_ratelimited() has, not
trying to workaround it in the driver.

> I would like to remove the ratelimit from ath10k_warn eventually.

I think that's not a good idea, it might cause unnecessary host reboots
in problem cases. Rate limitting the messages is much better option.

-- 
Kalle Valo

Re: [PATCH v2 12/21] ath10k: Support up to 64 vdevs.

[Valo, Kalle]

gree...@candelatech.com writes:

> From: Ben Greear 
>
> 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 
> ---
>  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/wireless/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 
> ath10k_firmware_mode mode,
>   if (status)
>   goto err_hif_stop;
>  
> - ar->free_vdev_map = (1LL << ar->max_num_vdevs) - 1;
> + if (ar->max_num_vdevs >= 64)
> + ar->free_vdev_map = 0xLL;
> + else
> + ar->free_vdev_map = (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.

-- 
Kalle Valo

Re: [PATCH v2 19/21] ath10k: Enable adhoc mode for CT firmware.

[Valo, Kalle]

gree...@candelatech.com writes:

> From: Ben Greear 
>
> CT firmware can support IBSS mode, so allow users to configure this.
>
> Signed-off-by: Ben Greear 
> ---
>  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 :(

-- 
Kalle Valo

Re: [PATCH v2 15/21] ath10k: support CT firmware flag.

[Valo, Kalle]

gree...@candelatech.com writes:

> From: Ben Greear 
>
> 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 
> ---
>  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.

-- 
Kalle Valo

Re: [PATCH v2 11/21] ath10k: add fw-powerup-fail to ethtool stats.

[Valo, Kalle]

gree...@candelatech.com writes:

> From: Ben Greear 
>
> 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 
> ---
>  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(>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.

-- 
Kalle Valo

Re: [PATCH v2 10/21] ath10k: support logging ath10k_info as KERN_DEBUG

[Valo, Kalle]

gree...@candelatech.com writes:

> From: Ben Greear 
>
> Helps keep messages off of (serial) console when
> that is desired.
>
> Signed-off-by: Ben Greear 

Isn't /proc/sys/kernel/print exactly for this purpose? At least I recall
using it.

-- 
Kalle Valo

Re: [PATCH v2 09/21] ath10k: print fw debug messages in hex.

[Valo, Kalle]

gree...@candelatech.com writes:

> From: Ben Greear 
>
> This allows user-space tools to decode debug-log
> messages by parsing dmesg or /var/log/messages.
>
> Signed-off-by: Ben Greear 

Don't tracing points already provide the same information?

> +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?

-- 
Kalle Valo

Re: [PATCH v2 08/21] ath10k: make firmware text debug messages more verbose.

[Valo, Kalle]

gree...@candelatech.com writes:

> From: Ben Greear 
>
> 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 
> ---
>  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.

-- 
Kalle Valo

Re: [PATCH v2 04/21] ath10k: rate-limit packet tx errors

[Valo, Kalle]

gree...@candelatech.com writes:

> From: Ben Greear 
>
> 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 
> ---
>  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 = 
dev_warn_ratelimited(ar->dev, "%pV", );
trace_ath10k_log_warn(ar, );

va_end(args);
}

-- 
Kalle Valo

Re: [PATCH v2 03/21] ath10k: Allow changing ath10k debug mask at runtime.

[Valo, Kalle]

gree...@candelatech.com writes:

> From: Ben Greear 
>
> Using debugfs.  More convenient than module options
> in some cases.
>
> Signed-off-by: Ben Greear 
> ---
>  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: 0x4000\n"
> + "FW:  0x8000\n"
> + "ALL: 0x\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, );
> + 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 0x > /sys/module/ath10k_core/parameters/debug_mask

-- 
Kalle Valo

Re: [PATCH] ath10k: Add missing CONFIG_ATH10K_DEBUGFS check

[Valo, Kalle]

Benjamin Berg  writes:

> The patch "ath10k: allow setting coverage class" was missing a check for
> CONFIG_ATH10K_DEBUGFS so it would try to use non-existing struct elements
> in some configurations. Fix this by adding the appropriate ifdef.
>
> Signed-off-by: Benjamin Berg 
> ---
>
> Sorry, so turns out the kbuild test robot is correct and I forgot the ifdef
> to check for CONFIG_ATH10K_DEBUGFS, so here a fixup commit for it.
>
> Other than that everything looks good to me in the pending branch.

Thanks, I folded this to the original patch in the pending branch.

-- 
Kalle Valo

Re: [PATCH 2/2] ath10k: add platform regulatory domain support

[Valo, Kalle]

Bartosz Markowski <bartosz.markow...@tieto.com> writes:

> On 12 September 2016 at 17:35, Valo, Kalle <kv...@qca.qualcomm.com> wrote:
>
>> > +#ifdef CONFIG_ACPI
>> > +#define WRD_METHOD "WRDD"
>> > +#define WRDD_WIFI  (0x07)
>> > +
>> > +static u32 ath10k_mac_wrdd_get_mcc(struct ath10k *ar, union acpi_object 
>> > *wrdd)
>> > +{
>>
>> I don't think the ifdef is really necessary, acpi.h should handle that
>> (hopefully). Also I changed the error handling to use standard error
>> values and changed the info messages to dbg, they are too spammy in my
>> opinion. Please check carefully my changes in the pending branch:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/kvalo/ath.git/commit/?h=pending=fe91745381ec3999d8de6dedb07b396c82539717
>
> I'm OK with the changes, I have not tried that though, except of
> reviewing and compiling it (do not have access to the chromebook for
> next few days). If you want to wait with it until I test it, it's fine
> too.

Ok, I'll wait for few days in case you have time to test it.

-- 
Kalle Valo

Re: [PATCH] ath10k: Spelling and miscellaneous neatening

[Valo, Kalle]

Joe Perches  writes:

> Correct some trivial comment typos.
> Remove unnecessary parentheses in a long line.
> Convert a return; before the end of a void function definition to just ;
>
> Signed-off-by: Joe Perches 

[...]

> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -2118,7 +2118,7 @@ err:
>   /* TODO: It's probably a good idea to release device from the driver
>* but calling device_release_driver() here will cause a deadlock.
>*/
> - return;
> + ;
>  }

I don't think this improves anything, I dropped this part from the patch
in my pending branch.

-- 
Kalle Valo