Re: ath10k: Kconfig build warnings

2018-10-16 Thread Niklas Cassel 
On Mon, Oct 15, 2018 at 03:07:18PM -0700, Jeff Kirsher wrote:
> Not sure if you aware, with David Miller's latest net-next tree, doing a
> 'make allmodconfig' produces the following Kconfig errors:
> 
> [15:06:06]$ make allmodconfig
> scripts/kconfig/conf  --allmodconfig Kconfig
> 
> WARNING: unmet direct dependencies detected for QCOM_QMI_HELPERS
>   Depends on [n]: ARCH_QCOM && NET [=y]
>   Selected by [m]:
>   - ATH10K_SNOC [=m] && NETDEVICES [=y] && WLAN [=y] && WLAN_VENDOR_ATH
> [=y] && ATH10K [=m] && (ARCH_QCOM || COMPILE_TEST [=y])
> #
> # configuration written to .config
> #

Hello Jeff,

I believe this will be fixed by:
ccfb464cd106 ("soc: qcom: Allow COMPILE_TEST of qcom SoC Kconfigs")

Which is currently in linux-next.


Kind regards,
Niklas

> ___
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k


___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v5 0/6] *** Add support for wifi QMI client handshakes ***

2018-10-12 Thread Niklas Cassel 
On Mon, Oct 08, 2018 at 05:36:30PM -0700, Brian Norris wrote:
> + linux-msm
> 
> Hi Govind, Kalle,
> 
> On Wed, Aug 15, 2018 at 02:56:31PM +0530, Govind Singh wrote:
> > Add QMI client handshakes for Q6 integrated WLAN connectivity subsystem.
> > This module is responsible for communicating WLAN control messages to FW
> > over QMI interface. This patch series enables the qmi handshakes required 
> > for
> > WCN3990 chipset.
> [...]
> 
> What's the status of this patchset? It has seen various stages of
> review, and except for the fact that Govind seems to have dropped
> various Reviewed-by/Acked-by tags (which Rob noticed), I don't see any
> relevant feedback that should be blocking it.
> 
> I previously had concerns about the firmware boot sequence -- that it
> required a Qualcomm-specific TFTP service over QRTR, which had no open
> source implementations. There is now a published daemon that worked for
> me [1], as well as firmware releases that loaded modem and Wifi firmware
> together, such that this TFTP service is not needed at all. So my
> concerns there are no longer blocking.
> 
> And I think Rob already reviewed the relevant DT bindings (but again,
> Govind missed collecting that tag for this series).
> 
> So the only outstanding request I see is to collect the appropriate
> tags. Should Govind resend the whole series just for that?
> 
> FWIW, I've been using this series for a while now, and I reviewed
> earlier versions. I can provide this for the whole series:
> 
> Reviewed-by: Brian Norris 

Hello Kalle,

I see that this patch series has been added to your master-pending branch.

It seems to be lacking Brians Reviewed-by tags (from above).


The diff between v4 and v5 is just:

+++ b/drivers/net/wireless/ath/ath10k/qmi.c
@@ -1010,10 +1010,10 @@ int ath10k_qmi_deinit(struct ath10k *ar)
struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
struct ath10k_qmi *qmi = ar_snoc->qmi;
 
+   qmi_handle_release(>qmi_hdl);
cancel_work_sync(>event_work);
destroy_workqueue(qmi->event_wq);
-   qmi_handle_release(>qmi_hdl);
-   qmi = NULL;
+   ar_snoc->qmi = NULL;
 
return 0;
 }

I've given my Acked-by on v4:
https://patchwork.kernel.org/cover/10540111/
The change between v4 and v5 does not warrant the removal of those tags,
so please re-add them.

Rob has given his Reviewed-by on the dt-binding on v4:
https://patchwork.kernel.org/patch/10540115/
The dt-binding hasn't changed between v4 and v5, so please re-add it.


I also noted that kbuild test robot complain about this series on x86:
http://lists.infradead.org/pipermail/ath10k/2018-October/012268.html
Are test errors still valid?

My patch series that makes QMI_HELPERS selectable for compile test
(e.g. x86), is queued up for 4.20~5.0:
https://git.kernel.org/pub/scm/linux/kernel/git/agross/linux.git/log/?h=qcom-drivers-for-4.20

Does this patch series make the errors go away, or are they unrelated?


Kind regards,
Niklas

> 
> If nothing else, I think it's important to get someone to merge the DT
> bindings from this series, because Govind is now trying to extend the DT
> binding and to enable the WCN3990 device node in the SDM845 device tree
> nodes.
> 
> Regards,
> Brian
> 
> [1] https://github.com/andersson/tqftpserv
> [2] See this series:
> https://patchwork.kernel.org/cover/10621863/
> Govind didn't send it to the right mailing lists yet, but we're
> close...

___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v4 0/6] *** Add support for wifi QMI client handshakes ***

2018-07-24 Thread Niklas Cassel 
On Mon, Jul 23, 2018 at 06:04:22PM +0530, Govind Singh wrote:
> Add QMI client handshakes for Q6 integrated WLAN connectivity subsystem.
> This module is responsible for communicating WLAN control messages to FW
> over QMI interface. This patch series enables the qmi handshakes required for
> WCN3990 chipset.
> 
> QUALCOMM MSM Interface(QMI) provides the control interface between
> components running b/w remote processors with underlying transport layer
> based on integrated chipset(shared memory) or discrete 
> chipset(PCI/USB/SDIO/UART).
> 
> QMI client driver implementation is based on qmi framework 
> https://lwn.net/Articles/729924/.

For whole series:

Acked-by: Niklas Cassel 

___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v3 0/6] *** Add support for wifi QMI client handshakes ***

2018-07-19 Thread Niklas Cassel 
On Fri, Jul 06, 2018 at 02:26:17PM +0530, Govind Singh wrote:
> Add QMI client handshakes for Q6 integrated WLAN connectivity subsystem.
> This module is responsible for communicating WLAN control messages to FW
> over QMI interface. This patch series enables the qmi handshakes required for
> WCN3990 chipset.

(cut)

Hello Govind,

I ran make W=1, sparse, and checkpatch on your new iteration of patches.
(I didn't do a full manual review this time, since I did that last time.)

Building with warnings and checkpatch looks good,
but sparse reports the two following warnings:

  CHECK   drivers/net/wireless/ath/ath10k//qmi.c
drivers/net/wireless/ath/ath10k//qmi.c:935:29: warning: incorrect type in 
assignment (different address spaces)
drivers/net/wireless/ath/ath10k//qmi.c:935:29:expected void *msa_va
drivers/net/wireless/ath/ath10k//qmi.c:935:29:got void [noderef] *

and

drivers/net/wireless/ath/ath10k//snoc.c:76:22: warning: incorrect type in 
initializer (different base types)
drivers/net/wireless/ath/ath10k//snoc.c:76:22:expected restricted __le16 
[usertype] reg_offset
drivers/net/wireless/ath/ath10k//snoc.c:76:22:got int
drivers/net/wireless/ath/ath10k//snoc.c:77:19: warning: incorrect type in 
initializer (different base types)
drivers/net/wireless/ath/ath10k//snoc.c:77:19:expected restricted __le16 
[usertype] ce_id
drivers/net/wireless/ath/ath10k//snoc.c:77:19:got int
drivers/net/wireless/ath/ath10k//snoc.c:77:22: warning: incorrect type in 
initializer (different base types)
drivers/net/wireless/ath/ath10k//snoc.c:77:22:expected restricted __le16 
[usertype] reg_offset
drivers/net/wireless/ath/ath10k//snoc.c:77:22:got int
drivers/net/wireless/ath/ath10k//snoc.c:78:19: warning: incorrect type in 
initializer (different base types)
drivers/net/wireless/ath/ath10k//snoc.c:78:19:expected restricted __le16 
[usertype] ce_id
drivers/net/wireless/ath/ath10k//snoc.c:78:19:got int
drivers/net/wireless/ath/ath10k//snoc.c:78:22: warning: incorrect type in 
initializer (different base types)
drivers/net/wireless/ath/ath10k//snoc.c:78:22:expected restricted __le16 
[usertype] reg_offset
drivers/net/wireless/ath/ath10k//snoc.c:78:22:got int
drivers/net/wireless/ath/ath10k//snoc.c:79:19: warning: incorrect type in 
initializer (different base types)
drivers/net/wireless/ath/ath10k//snoc.c:79:19:expected restricted __le16 
[usertype] ce_id
drivers/net/wireless/ath/ath10k//snoc.c:79:19:got int
drivers/net/wireless/ath/ath10k//snoc.c:79:22: warning: incorrect type in 
initializer (different base types)
drivers/net/wireless/ath/ath10k//snoc.c:79:22:expected restricted __le16 
[usertype] reg_offset
drivers/net/wireless/ath/ath10k//snoc.c:79:22:got int
drivers/net/wireless/ath/ath10k//snoc.c:80:19: warning: incorrect type in 
initializer (different base types)
drivers/net/wireless/ath/ath10k//snoc.c:80:19:expected restricted __le16 
[usertype] ce_id
drivers/net/wireless/ath/ath10k//snoc.c:80:19:got int
drivers/net/wireless/ath/ath10k//snoc.c:80:22: warning: incorrect type in 
initializer (different base types)
drivers/net/wireless/ath/ath10k//snoc.c:80:22:expected restricted __le16 
[usertype] reg_offset
drivers/net/wireless/ath/ath10k//snoc.c:80:22:got int
drivers/net/wireless/ath/ath10k//snoc.c:81:19: warning: incorrect type in 
initializer (different base types)
drivers/net/wireless/ath/ath10k//snoc.c:81:19:expected restricted __le16 
[usertype] ce_id
drivers/net/wireless/ath/ath10k//snoc.c:81:19:got int
drivers/net/wireless/ath/ath10k//snoc.c:81:22: warning: incorrect type in 
initializer (different base types)
drivers/net/wireless/ath/ath10k//snoc.c:81:22:expected restricted __le16 
[usertype] reg_offset
drivers/net/wireless/ath/ath10k//snoc.c:81:22:got int
drivers/net/wireless/ath/ath10k//snoc.c:82:19: warning: incorrect type in 
initializer (different base types)
drivers/net/wireless/ath/ath10k//snoc.c:82:19:expected restricted __le16 
[usertype] ce_id
drivers/net/wireless/ath/ath10k//snoc.c:82:19:got int
drivers/net/wireless/ath/ath10k//snoc.c:82:22: warning: incorrect type in 
initializer (different base types)
drivers/net/wireless/ath/ath10k//snoc.c:82:22:expected restricted __le16 
[usertype] reg_offset
drivers/net/wireless/ath/ath10k//snoc.c:82:22:got int
drivers/net/wireless/ath/ath10k//snoc.c:83:19: warning: incorrect type in 
initializer (different base types)
drivers/net/wireless/ath/ath10k//snoc.c:83:19:expected restricted __le16 
[usertype] ce_id
drivers/net/wireless/ath/ath10k//snoc.c:83:19:got int
drivers/net/wireless/ath/ath10k//snoc.c:83:22: warning: incorrect type in 
initializer (different base types)
drivers/net/wireless/ath/ath10k//snoc.c:83:22:expected restricted __le16 
[usertype] reg_offset
drivers/net/wireless/ath/ath10k//snoc.c:83:22:got int
drivers/net/wireless/ath/ath10k//snoc.c:84:19: warning: incorrect type in 
initializer (different base types)

Re: [2/2] ath10k: allow ATH10K_SNOC with COMPILE_TEST

2018-07-04 Thread Niklas Cassel 
On Wed, Jul 04, 2018 at 09:59:13AM +, Kalle Valo wrote:
> Niklas Cassel  wrote:
> 
> > ATH10K_SNOC builds just fine with COMPILE_TEST, so make that possible.
> > 
> > Signed-off-by: Niklas Cassel 
> 
> Note to myself, I see these dependencies in linux-next (but not in Linus' 
> tree yet):
> 
> 67cd0eec5b62 rpmsg: smd: Add missing include of sizes.h
> 376e1f304fc7 firmware: qcom: scm: add a dummy qcom_scm_assign_mem()
> 
> These I don't see in linux-next yet:
> 
>   soc: qcom: smem: Add missing include of sizes.h
>   soc: qcom: smp2p: Add select IRQ_DOMAIN
>   soc: qcom: smsm: Add select IRQ_DOMAIN
>   soc: qcom: Remove depends on ARCH_QCOM

Hello Kalle,

You are correct.
I submitted a v3 of this just yesterday:
https://marc.info/?l=linux-kernel=153060245727472=2
So hopefully it will be in linux-next within 2 weeks.

Regards,
Niklas

> 
> -- 
> https://patchwork.kernel.org/patch/10460103/
> 
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
> 
> 
> ___
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k

___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/3] ath10k: sdio: use same endpoint id for all packets in a bundle

2018-06-29 Thread Niklas Cassel 
On Fri, Jun 29, 2018 at 04:51:51PM +0300, Kalle Valo wrote:
> Niklas Cassel  writes:
> 
> > On Fri, Jun 29, 2018 at 02:53:44PM +0300, Kalle Valo wrote:
> >> Kalle Valo  writes:
> >> 
> >> > Niklas Cassel  writes:
> >> >
> >> >> All packets in a bundle should use the same endpoint id as the
> >> >> first lookahead.
> >> >>
> >> >> This matches how things are done is ath6kl, however,
> >> >> this patch can theoretically handle several bundles
> >> >> in ath10k_sdio_mbox_rx_process_packets().
> >> >>
> >> >> Without this patch we get lots of errors about invalid endpoint id:
> >> >>
> >> >> ath10k_sdio mmc2:0001:1: invalid endpoint in look-ahead: 224
> >> >> ath10k_sdio mmc2:0001:1: failed to get pending recv messages: -12
> >> >> ath10k_sdio mmc2:0001:1: failed to process pending SDIO interrupts: -12
> >> >>
> >> >> Signed-off-by: Alagu Sankar 
> >> >> Signed-off-by: Niklas Cassel 
> >> >
> >> > You have Alagu's s-o-b first so that implies Alagu is the author. So
> >> > should I add From header for Alagu and add you (Niklas) as
> >> > Co-Developed-by? Or vice versa?
> >
> > Hello Kalle,
> >
> > It is not always obvious how the combination of git-author
> > and Co-Developed-by should be:
> > http://lkml.iu.edu/hypermail/linux/kernel/1801.2/00988.html
> 
> Yeah, that's true.
> 
> > Alagu deserves most credit, since I have simply taken parts
> > of a very big patch and split it into smaller pieces.
> > I tried to do the right thing by having his Signed-off-by first.
> >
> > Let's go with your suggestion and add a From: header with
> > Alagu's email, and a Co-Developed-by tag with my email.
> > (Note that both Signed-off-bys are still needed according to
> > submitting-patches.rst)
> 
> Ok.
> 
> > Do you want me to resend the patches with these two lines
> > added, or can you fix them up manually?
> 
> No need, I did that already in the pending branch. Please check:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending=6b23bd067990df1cc1e9a4c28327393a7a3ba718
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending=bf83a5c572e0737e59e026c989b190b4e52d2ef4
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending=218f924d80b76520a4e60eead5e89f3ebd2e86c1

Hello Kalle

It looks good :)

Regards,
Niklas

> 
> -- 
> Kalle Valo

___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/3] ath10k: sdio: use same endpoint id for all packets in a bundle

2018-06-29 Thread Niklas Cassel 
On Fri, Jun 29, 2018 at 02:53:44PM +0300, Kalle Valo wrote:
> Kalle Valo  writes:
> 
> > Niklas Cassel  writes:
> >
> >> All packets in a bundle should use the same endpoint id as the
> >> first lookahead.
> >>
> >> This matches how things are done is ath6kl, however,
> >> this patch can theoretically handle several bundles
> >> in ath10k_sdio_mbox_rx_process_packets().
> >>
> >> Without this patch we get lots of errors about invalid endpoint id:
> >>
> >> ath10k_sdio mmc2:0001:1: invalid endpoint in look-ahead: 224
> >> ath10k_sdio mmc2:0001:1: failed to get pending recv messages: -12
> >> ath10k_sdio mmc2:0001:1: failed to process pending SDIO interrupts: -12
> >>
> >> Signed-off-by: Alagu Sankar 
> >> Signed-off-by: Niklas Cassel 
> >
> > You have Alagu's s-o-b first so that implies Alagu is the author. So
> > should I add From header for Alagu and add you (Niklas) as
> > Co-Developed-by? Or vice versa?

Hello Kalle,

It is not always obvious how the combination of git-author
and Co-Developed-by should be:
http://lkml.iu.edu/hypermail/linux/kernel/1801.2/00988.html

Alagu deserves most credit, since I have simply taken parts
of a very big patch and split it into smaller pieces.
I tried to do the right thing by having his Signed-off-by first.

Let's go with your suggestion and add a From: header with
Alagu's email, and a Co-Developed-by tag with my email.
(Note that both Signed-off-bys are still needed according to
submitting-patches.rst)

Do you want me to resend the patches with these two lines
added, or can you fix them up manually?

Regards,
Niklas

> 
> Ah, the same issue is with all three patches. So whatever we decide,
> I'll do the same changes on all three.
> 
> -- 
> Kalle Valo

___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 3/3] ath10k: sdio: set skb len for all rx packets

2018-06-20 Thread Niklas Cassel 
Without this, packets larger than 1500 will silently be dropped.
Easily reproduced by sending a ping packet with a size larger
than 1500.

Signed-off-by: Alagu Sankar 
Signed-off-by: Niklas Cassel 
---
 drivers/net/wireless/ath/ath10k/sdio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/ath/ath10k/sdio.c 
b/drivers/net/wireless/ath/ath10k/sdio.c
index 0c57d6aaa437..8ac47b04a17e 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -396,6 +396,7 @@ static int ath10k_sdio_mbox_rx_process_packet(struct ath10k 
*ar,
int ret;
 
payload_len = le16_to_cpu(htc_hdr->len);
+   skb->len = payload_len + sizeof(struct ath10k_htc_hdr);
 
if (trailer_present) {
trailer = skb->data + sizeof(*htc_hdr) +
-- 
2.17.1


___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 1/3] ath10k: sdio: use same endpoint id for all packets in a bundle

2018-06-20 Thread Niklas Cassel 
All packets in a bundle should use the same endpoint id as the
first lookahead.

This matches how things are done is ath6kl, however,
this patch can theoretically handle several bundles
in ath10k_sdio_mbox_rx_process_packets().

Without this patch we get lots of errors about invalid endpoint id:

ath10k_sdio mmc2:0001:1: invalid endpoint in look-ahead: 224
ath10k_sdio mmc2:0001:1: failed to get pending recv messages: -12
ath10k_sdio mmc2:0001:1: failed to process pending SDIO interrupts: -12

Signed-off-by: Alagu Sankar 
Signed-off-by: Niklas Cassel 
---
 drivers/net/wireless/ath/ath10k/sdio.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/sdio.c 
b/drivers/net/wireless/ath/ath10k/sdio.c
index d612ce8c9cff..d46523b0472c 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -434,12 +434,14 @@ static int ath10k_sdio_mbox_rx_process_packets(struct 
ath10k *ar,
enum ath10k_htc_ep_id id;
int ret, i, *n_lookahead_local;
u32 *lookaheads_local;
+   int lookahead_idx = 0;
 
for (i = 0; i < ar_sdio->n_rx_pkts; i++) {
lookaheads_local = lookaheads;
n_lookahead_local = n_lookahead;
 
-   id = ((struct ath10k_htc_hdr *)[i])->eid;
+   id = ((struct ath10k_htc_hdr *)
+ [lookahead_idx++])->eid;
 
if (id >= ATH10K_HTC_EP_COUNT) {
ath10k_warn(ar, "invalid endpoint in look-ahead: %d\n",
@@ -462,6 +464,7 @@ static int ath10k_sdio_mbox_rx_process_packets(struct 
ath10k *ar,
/* Only read lookahead's from RX trailers
 * for the last packet in a bundle.
 */
+   lookahead_idx--;
lookaheads_local = NULL;
n_lookahead_local = NULL;
}
-- 
2.17.1


___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 0/3] ath10k: sdio: essential sdio fixes

2018-06-20 Thread Niklas Cassel 
These fixes were originally embedded in a patch
"ath10k_sdio: virtual scatter gather for receive"
posted by Alagu Sankar 
https://patchwork.kernel.org/patch/9979579/

However, while that patch implements scatter gather
(without using the proper sg types), it also
includes several other random fixes, including
but not limited to checkpatch.pl fixes.

This patch series is an attempt to extract
the most essential fixes included in that commit.

Each fix is now an individual commit, as upstream
requires that each logical change is self contained.

All of these fixes are essential to get working
sdio support for ath10k.

Other patches are needed to get working sdio support,
including the high-latency patch series from Erik Stromdahl.


Niklas Cassel (3):
  ath10k: sdio: use same endpoint id for all packets in a bundle
  ath10k: sdio: allocate correct size for RECV_1MORE_BLOCK rx packets
  ath10k: sdio: set skb len for all rx packets

 drivers/net/wireless/ath/ath10k/htc.h  | 1 +
 drivers/net/wireless/ath/ath10k/sdio.c | 9 -
 2 files changed, 9 insertions(+), 1 deletion(-)

-- 
2.17.1


___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 2/3] ath10k: sdio: allocate correct size for RECV_1MORE_BLOCK rx packets

2018-06-20 Thread Niklas Cassel 
Without this, when receiving a packet that has this flag set
from firmware, we will read invalid trailer data from the packet,
which will be shown as various errors, e.g. "sdio mbox lookahead
is zero" or "invalid rx packet" or "payload length x exceeds max
htc length".

Signed-off-by: Alagu Sankar 
Signed-off-by: Niklas Cassel 
---
 drivers/net/wireless/ath/ath10k/htc.h  | 1 +
 drivers/net/wireless/ath/ath10k/sdio.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/htc.h 
b/drivers/net/wireless/ath/ath10k/htc.h
index 34877597dd6a..cf1068dc3254 100644
--- a/drivers/net/wireless/ath/ath10k/htc.h
+++ b/drivers/net/wireless/ath/ath10k/htc.h
@@ -58,6 +58,7 @@ enum ath10k_htc_tx_flags {
 };
 
 enum ath10k_htc_rx_flags {
+   ATH10K_HTC_FLAGS_RECV_1MORE_BLOCK = 0x01,
ATH10K_HTC_FLAG_TRAILER_PRESENT = 0x02,
ATH10K_HTC_FLAG_BUNDLE_MASK = 0xF0
 };
diff --git a/drivers/net/wireless/ath/ath10k/sdio.c 
b/drivers/net/wireless/ath/ath10k/sdio.c
index d46523b0472c..0c57d6aaa437 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -603,6 +603,9 @@ static int ath10k_sdio_mbox_rx_alloc(struct ath10k *ar,
 * ATH10K_HTC_FLAG_BUNDLE_MASK flag set, all bundled
 * packet skb's have been allocated in the previous step.
 */
+   if (htc_hdr->flags & ATH10K_HTC_FLAGS_RECV_1MORE_BLOCK)
+   full_len += ATH10K_HIF_MBOX_BLOCK_SIZE;
+
ret = ath10k_sdio_mbox_alloc_rx_pkt(_sdio->rx_pkts[i],
act_len,
full_len,
-- 
2.17.1


___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 6/6] ath10k: Add QMI message handshake for wcn3990 client

2018-06-19 Thread Niklas Cassel 
On Tue, Jun 05, 2018 at 06:07:32PM +0530, Govind Singh wrote:
> Add WCN3990 QMI client handshakes for Q6 integrated WLAN connectivity
> subsystem. This layer is responsible for communicating qmi control
> messages to wifi fw QMI service using QMI messaging protocol.
> 
> Qualcomm MSM Interface(QMI) is a messaging format used to communicate
> between components running between application processor and remote
> processors with underlying transport layer based on integrated
> chipset(shared memory) or discrete chipset(PCI/USB/SDIO/UART).
> 
> With this patch-set basic functionality(STA/SAP) can be tested
> with WCN3990 chipset. The changes are verified with the firmware
> WLAN.HL.2.0-01192-QCAHLSWMTPLZ-1 and SDM845 MTP device.
> 
> Signed-off-by: Govind Singh 
> ---
>  drivers/net/wireless/ath/ath10k/Kconfig  |   13 +-
>  drivers/net/wireless/ath/ath10k/Makefile |4 +-
>  drivers/net/wireless/ath/ath10k/core.c   |6 +-
>  drivers/net/wireless/ath/ath10k/core.h   |2 +
>  drivers/net/wireless/ath/ath10k/qmi.c| 1030 ++
>  drivers/net/wireless/ath/ath10k/qmi.h|  136 +++
>  drivers/net/wireless/ath/ath10k/snoc.c   |  209 -
>  drivers/net/wireless/ath/ath10k/snoc.h   |3 +
>  8 files changed, 1387 insertions(+), 16 deletions(-)
>  create mode 100644 drivers/net/wireless/ath/ath10k/qmi.c
>  create mode 100644 drivers/net/wireless/ath/ath10k/qmi.h
> 
> diff --git a/drivers/net/wireless/ath/ath10k/Kconfig 
> b/drivers/net/wireless/ath/ath10k/Kconfig
> index 84f071ac0d84..4be6443f1e9d 100644
> --- a/drivers/net/wireless/ath/ath10k/Kconfig
> +++ b/drivers/net/wireless/ath/ath10k/Kconfig
> @@ -41,12 +41,13 @@ config ATH10K_USB
> work in progress and will not fully work.
>  
>  config ATH10K_SNOC
> -tristate "Qualcomm ath10k SNOC support (EXPERIMENTAL)"
> -depends on ATH10K && ARCH_QCOM
> ----help---
> -  This module adds support for integrated WCN3990 chip connected
> -  to system NOC(SNOC). Currently work in progress and will not
> -  fully work.
> + tristate "Qualcomm ath10k SNOC support (EXPERIMENTAL)"
> + depends on ATH10K && ARCH_QCOM
> + select QCOM_QMI_HELPERS
> + ---help---
> + This module adds support for integrated WCN3990 chip connected
> + to system NOC(SNOC). Currently work in progress and will not
> + fully work.

Hello Govind,

Please do clean ups in separate commits.
That way is would be easier to see that the only
functional change here is that you added
select QCOM_QMI_HELPERS.

(Also help text should normally be indented by two extra spaces.)

I've sent a fix for the mixed tabs/spaces when I tried to
add COMPILE_TEST for this, and Kalle has already picked it up
in his master branch:
https://marc.info/?l=linux-wireless=152880359200364

So in your next version of this series, this can be reduced to simply
select QCOM_QMI_HELPERS.

--

There are some checkpatch warnings on this patch:

drivers/net/wireless/ath/ath10k/qmi.c and
drivers/net/wireless/ath/ath10k/qmi.h
is missing SPDX-License-Identifier tag.

Several lines in drivers/net/wireless/ath/ath10k/qmi.c
and one line in drivers/net/wireless/ath/ath10k/snoc.c
is over 80 characters.

--

This patch is quite big, I think that it makes sense to split your patch in two.
One that adds the ath10k_qmi_* functions, and a follow up patch
that actually adds the function calls to snoc.c

>  
>  config ATH10K_DEBUG
>   bool "Atheros ath10k debugging"
> diff --git a/drivers/net/wireless/ath/ath10k/Makefile 
> b/drivers/net/wireless/ath/ath10k/Makefile
> index 44d60a61b242..66326b949ab1 100644
> --- a/drivers/net/wireless/ath/ath10k/Makefile
> +++ b/drivers/net/wireless/ath/ath10k/Makefile
> @@ -36,7 +36,9 @@ obj-$(CONFIG_ATH10K_USB) += ath10k_usb.o
>  ath10k_usb-y += usb.o
>  
>  obj-$(CONFIG_ATH10K_SNOC) += ath10k_snoc.o
> -ath10k_snoc-y += snoc.o
> +ath10k_snoc-y += qmi.o \
> +  qmi_wlfw_v01.o \
> +  snoc.o
>  
>  # for tracing framework to find trace.h
>  CFLAGS_trace.o := -I$(src)
> diff --git a/drivers/net/wireless/ath/ath10k/core.c 
> b/drivers/net/wireless/ath/ath10k/core.c
> index 8a592019cc4d..cd6c71a1eeed 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -1134,7 +1134,7 @@ static int ath10k_download_fw(struct ath10k *ar)
>   return ret;
>  }
>  
> -static void ath10k_core_free_board_files(struct ath10k *ar)
> +void ath10k_core_free_board_files(struct ath10k *ar)
>  {
>   if (!IS_ERR(ar->normal_mode_fw.board))
>   release_firmware(ar->normal_mode_fw.board);
> @@ -1143,6 +1143,7 @@ static void ath10k_core_free_board_files(struct ath10k 
> *ar)
>   ar->normal_mode_fw.board_data = NULL;
>   ar->normal_mode_fw.board_len = 0;
>  }
> +EXPORT_SYMBOL(ath10k_core_free_board_files);
>  
>  static void ath10k_core_free_firmware_files(struct ath10k *ar)
>  {
> @@ -1454,7 +1455,7 @@ static int ath10k_core_create_board_name(struct

Re: [PATCH v2 5/6] firmware: qcom: scm: Add WLAN VMID for Qualcomm SCM interface

2018-06-19 Thread Niklas Cassel 
Acked-by: Niklas Cassel 

On Tue, Jun 05, 2018 at 06:07:09PM +0530, Govind Singh wrote:
> Add WLAN related VMID's to support wlan driver to set up
> the remote's permissions call via TrustZone.
> 
> Signed-off-by: Govind Singh 
> ---
>  include/linux/qcom_scm.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> index b401b962afff..da63d84e1e91 100644
> --- a/include/linux/qcom_scm.h
> +++ b/include/linux/qcom_scm.h
> @@ -1,4 +1,4 @@
> -/* Copyright (c) 2010-2015, The Linux Foundation. All rights reserved.
> +/* Copyright (c) 2010-2015, 2018, The Linux Foundation. All rights reserved.
>   * Copyright (C) 2015 Linaro Ltd.
>   *
>   * This program is free software; you can redistribute it and/or modify
> @@ -33,6 +33,8 @@ struct qcom_scm_vmperm {
>  
>  #define QCOM_SCM_VMID_HLOS   0x3
>  #define QCOM_SCM_VMID_MSS_MSA0xF
> +#define QCOM_SCM_VMID_WLAN   0x18
> +#define QCOM_SCM_VMID_WLAN_CE0x19
>  #define QCOM_SCM_PERM_READ   0x4
>  #define QCOM_SCM_PERM_WRITE  0x2
>  #define QCOM_SCM_PERM_EXEC   0x1
> -- 
> 2.17.0
> 
> 
> ___
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k

___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 2/6] ath10k: Add support to create boardname for non-bmi target

2018-06-19 Thread Niklas Cassel 
Acked-by: Niklas Cassel 

On Tue, Jun 05, 2018 at 06:04:48PM +0530, Govind Singh wrote:
> From: Rakesh Pillai 
> 
> Add support to create the boardname for non-bmi targets
> like WCN3990, which uses qmi for bdf download. This
> boardname is used to parse the board data from board-2.bin.
> 
> Signed-off-by: Rakesh Pillai 
> Signed-off-by: Govind Singh 
> ---
>  drivers/net/wireless/ath/ath10k/core.c | 14 ++
>  drivers/net/wireless/ath/ath10k/core.h |  2 ++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/core.c 
> b/drivers/net/wireless/ath/ath10k/core.c
> index 4cf54a7ef09a..8a592019cc4d 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -1429,6 +1429,20 @@ static int ath10k_core_create_board_name(struct ath10k 
> *ar, char *name,
>   goto out;
>   }
>  
> + if (ar->id.qmi_ids_valid) {
> + if (ar->id.qmi_board_id > 0x99)
> + scnprintf(name, name_len,
> +   "bus=%s,qmi-board-id=%03x",
> +   ath10k_bus_str(ar->hif.bus),
> +   ar->id.qmi_board_id);
> + else
> + scnprintf(name, name_len,
> +   "bus=%s,qmi-board-id=b%02x",
> +   ath10k_bus_str(ar->hif.bus),
> +   ar->id.qmi_board_id);
> + goto out;
> + }
> +
>   scnprintf(name, name_len,
> 
> "bus=%s,vendor=%04x,device=%04x,subsystem-vendor=%04x,subsystem-device=%04x%s",
> ath10k_bus_str(ar->hif.bus),
> diff --git a/drivers/net/wireless/ath/ath10k/core.h 
> b/drivers/net/wireless/ath/ath10k/core.h
> index 951dbdd1c9eb..0a2e4a5c3612 100644
> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -940,7 +940,9 @@ struct ath10k {
>   u32 subsystem_device;
>  
>   bool bmi_ids_valid;
> + bool qmi_ids_valid;
>   u8 bmi_board_id;
> + u8 qmi_board_id;
>   u8 bmi_chip_id;
>  
>   char bdf_ext[ATH10K_SMBIOS_BDF_EXT_STR_LENGTH];
> -- 
> 2.17.0
> 
> 
> ___
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k

___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 4/6] ath10k: Add debug mask for QMI layer

2018-06-19 Thread Niklas Cassel 
Acked-by: Niklas Cassel 

On Tue, Jun 05, 2018 at 06:06:51PM +0530, Govind Singh wrote:
> Add debug mask to control debug info of ath10k qmi
> messaging layer.
> 
> Signed-off-by: Govind Singh 
> ---
>  drivers/net/wireless/ath/ath10k/debug.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/debug.h 
> b/drivers/net/wireless/ath/ath10k/debug.h
> index 0afca5c106b6..a2f84fdb9131 100644
> --- a/drivers/net/wireless/ath/ath10k/debug.h
> +++ b/drivers/net/wireless/ath/ath10k/debug.h
> @@ -44,6 +44,7 @@ enum ath10k_debug_mask {
>   ATH10K_DBG_USB  = 0x0004,
>   ATH10K_DBG_USB_BULK = 0x0008,
>   ATH10K_DBG_SNOC = 0x0010,
> + ATH10K_DBG_QMI  = 0x0020,
>   ATH10K_DBG_ANY  = 0x,
>  };
>  
> -- 
> 2.17.0
> 
> 
> ___
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k

___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 1/6] ath10k: Add qmi service helpers for wcn3990 qmi client

2018-06-19 Thread Niklas Cassel 
On Tue, Jun 05, 2018 at 06:03:40PM +0530, Govind Singh wrote:
> WLAN qmi server running in Q6 exposes host to target
> cold boot qmi handshakes. Add WLAN QMI service helpers
> for ath10k wcn3990 qmi client.
> 
> Signed-off-by: Govind Singh 
> ---
>  .../net/wireless/ath/ath10k/qmi_wlfw_v01.c| 2072 +
>  .../net/wireless/ath/ath10k/qmi_wlfw_v01.h|  677 ++
>  2 files changed, 2749 insertions(+)
>  create mode 100644 drivers/net/wireless/ath/ath10k/qmi_wlfw_v01.c
>  create mode 100644 drivers/net/wireless/ath/ath10k/qmi_wlfw_v01.h
> 
> diff --git a/drivers/net/wireless/ath/ath10k/qmi_wlfw_v01.c 
> b/drivers/net/wireless/ath/ath10k/qmi_wlfw_v01.c
> new file mode 100644
> index ..ba79c2e4aed6
> --- /dev/null
> +++ b/drivers/net/wireless/ath/ath10k/qmi_wlfw_v01.c
> @@ -0,0 +1,2072 @@

Hello Govind,

Please run checkpatch on this patch (and all other patches in the series).

WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#32: FILE: drivers/net/wireless/ath/ath10k/qmi_wlfw_v01.c:1:
+/*

WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#2110: FILE: drivers/net/wireless/ath/ath10k/qmi_wlfw_v01.h:1:
+/*

If I'm not mistaken you are using the ISC license, and
the proper SPDX-License-Identifier tag would then be:

/* SPDX-License-Identifier: ISC */

> +/*
> + * Copyright (c) 2018 The Linux Foundation. All rights reserved.
> + *
> + * Permission to use, copy, modify, and/or distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#include 
> +#include 
> +#include "qmi_wlfw_v01.h"
> +
> +static struct qmi_elem_info wlfw_ce_tgt_pipe_cfg_s_v01_ei[] = {
> + {
> + .data_type  = QMI_UNSIGNED_4_BYTE,
> + .elem_len   = 1,
> + .elem_size  = sizeof(u32),
> + .array_type = NO_ARRAY,
> + .tlv_type   = 0,
> + .offset = offsetof(struct wlfw_ce_tgt_pipe_cfg_s_v01,
> +pipe_num),
> + },

There's a lot of lines that are over 80 characters.

WARNING: line over 80 characters
#580: FILE: drivers/net/wireless/ath/ath10k/qmi_wlfw_v01.c:549:
+   .offset = offsetof(struct 
wlfw_ind_register_resp_msg_v01,

WARNING: line over 80 characters
#2402: FILE: drivers/net/wireless/ath/ath10k/qmi_wlfw_v01.h:293:
+   struct wlfw_shadow_reg_cfg_s_v01 
shadow_reg[QMI_WLFW_MAX_NUM_SHADOW_REG_V01];

Perhaps all these spaces to keep the same alignment isn't needed.


Kind regards,
Niklas

___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: FW: [PATCH 2/2] ath10k: allow ATH10K_SNOC with COMPILE_TEST

2018-06-14 Thread Niklas Cassel 
On Thu, Jun 14, 2018 at 05:09:04PM +0300, Kalle Valo wrote:
> Niklas Cassel  writes:
> 
> > On Tue, Jun 12, 2018 at 02:44:03PM +0200, Niklas Cassel wrote:
> >> On Tue, Jun 12, 2018 at 06:02:48PM +0530, Govind Singh wrote:
> >> > On 2018-06-12 17:45, Govind Singh wrote:
> >> > > 
> >> > > ATH10K_SNOC builds just fine with COMPILE_TEST, so make that possible.
> >> > > 
> >> > > Signed-off-by: Niklas Cassel 
> >> > > ---
> >> > >  drivers/net/wireless/ath/ath10k/Kconfig | 3 ++-
> >> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> >> > > 
> >> > > diff --git a/drivers/net/wireless/ath/ath10k/Kconfig
> >> > > b/drivers/net/wireless/ath/ath10k/Kconfig
> >> > > index 54ff5930126c..6572a43590a8 100644
> >> > > --- a/drivers/net/wireless/ath/ath10k/Kconfig
> >> > > +++ b/drivers/net/wireless/ath/ath10k/Kconfig
> >> > > @@ -42,7 +42,8 @@ config ATH10K_USB
> >> > > 
> >> > >  config ATH10K_SNOC
> >> > >tristate "Qualcomm ath10k SNOC support (EXPERIMENTAL)"
> >> > > -  depends on ATH10K && ARCH_QCOM
> >> > > +  depends on ATH10K
> >> > > +  depends on ARCH_QCOM || COMPILE_TEST
> >> > >---help---
> >> > >  This module adds support for integrated WCN3990 chip connected
> >> > >  to system NOC(SNOC). Currently work in progress and will not
> >> > 
> >> > Thanks Niklas for enabling COMPILE_TEST. With QMI set of
> >> > changes(https://patchwork.kernel.org/patch/10448183/), we need to enable
> >> > COMPILE_TEST for
> >> > QCOM_SCM/QMI_HELPERS which seems broken today. Are you planning to fix 
> >> > the
> >> > same.
> >
> > This patch is good as is.
> >
> > However, Govind's QMI patch set together with this patch
> > resulted in build errors.
> >
> > FTR, these are fixed by:
> > https://marc.info/?l=linux-kernel=152880985402356
> > https://marc.info/?l=linux-kernel=152889452326350
> 
> So the problem is that if I apply this patch I can't apply Govind's QMI
> patchset (due to the build problems) until Niklas' fixes to qcom and
> rpmsg subsystems propogate back to my tree and that might take weeks, or
> even months. But I really would like to apply the QMI patchset ASAP so
> that we can complete the wcn3990 support and not unnecessarily delay it.
> 
> So what I propose is that I put this patch 2 as 'Awaiting Upstream' in
> patchwork and apply it once Niklas' patches get to my tree. Does that
> sound good?

Absolutely.

I didn't realize this until after sending the patch.


Kind regards,
Niklas

___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: FW: [PATCH 2/2] ath10k: allow ATH10K_SNOC with COMPILE_TEST

2018-06-13 Thread Niklas Cassel 
On Tue, Jun 12, 2018 at 02:44:03PM +0200, Niklas Cassel wrote:
> On Tue, Jun 12, 2018 at 06:02:48PM +0530, Govind Singh wrote:
> > On 2018-06-12 17:45, Govind Singh wrote:
> > > -Original Message-
> > > From: ath10k  On Behalf Of Niklas
> > > Cassel
> > > Sent: Tuesday, June 12, 2018 5:09 PM
> > > To: Kalle Valo ; David S. Miller
> > > 
> > > Cc: Niklas Cassel ; net...@vger.kernel.org;
> > > linux-wirel...@vger.kernel.org; linux-ker...@vger.kernel.org;
> > > ath10k@lists.infradead.org
> > > Subject: [PATCH 2/2] ath10k: allow ATH10K_SNOC with COMPILE_TEST
> > > 
> > > ATH10K_SNOC builds just fine with COMPILE_TEST, so make that possible.
> > > 
> > > Signed-off-by: Niklas Cassel 
> > > ---
> > >  drivers/net/wireless/ath/ath10k/Kconfig | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/wireless/ath/ath10k/Kconfig
> > > b/drivers/net/wireless/ath/ath10k/Kconfig
> > > index 54ff5930126c..6572a43590a8 100644
> > > --- a/drivers/net/wireless/ath/ath10k/Kconfig
> > > +++ b/drivers/net/wireless/ath/ath10k/Kconfig
> > > @@ -42,7 +42,8 @@ config ATH10K_USB
> > > 
> > >  config ATH10K_SNOC
> > >   tristate "Qualcomm ath10k SNOC support (EXPERIMENTAL)"
> > > - depends on ATH10K && ARCH_QCOM
> > > + depends on ATH10K
> > > + depends on ARCH_QCOM || COMPILE_TEST
> > >   ---help---
> > > This module adds support for integrated WCN3990 chip connected
> > > to system NOC(SNOC). Currently work in progress and will not
> > 
> > Thanks Niklas for enabling COMPILE_TEST. With QMI set of
> > changes(https://patchwork.kernel.org/patch/10448183/), we need to enable
> > COMPILE_TEST for
> > QCOM_SCM/QMI_HELPERS which seems broken today. Are you planning to fix the
> > same.
> 
> 

This patch is good as is.

However, Govind's QMI patch set together with this patch
resulted in build errors.

FTR, these are fixed by:
https://marc.info/?l=linux-kernel=152880985402356
https://marc.info/?l=linux-kernel=152889452326350


Regards,
Niklas

___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 1/2] ath10k: do not mix spaces and tabs in Kconfig

2018-06-12 Thread Niklas Cassel 
Do not mix spaces and tabs in Kconfig.

Signed-off-by: Niklas Cassel 
---
 drivers/net/wireless/ath/ath10k/Kconfig | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/Kconfig 
b/drivers/net/wireless/ath/ath10k/Kconfig
index 84f071ac0d84..54ff5930126c 100644
--- a/drivers/net/wireless/ath/ath10k/Kconfig
+++ b/drivers/net/wireless/ath/ath10k/Kconfig
@@ -1,15 +1,15 @@
 config ATH10K
-tristate "Atheros 802.11ac wireless cards support"
-depends on MAC80211 && HAS_DMA
+   tristate "Atheros 802.11ac wireless cards support"
+   depends on MAC80211 && HAS_DMA
select ATH_COMMON
select CRC32
select WANT_DEV_COREDUMP
select ATH10K_CE
----help---
-  This module adds support for wireless adapters based on
-  Atheros IEEE 802.11ac family of chipsets.
+   ---help---
+ This module adds support for wireless adapters based on
+ Atheros IEEE 802.11ac family of chipsets.
 
-  If you choose to build a module, it'll be called ath10k.
+ If you choose to build a module, it'll be called ath10k.
 
 config ATH10K_CE
bool
@@ -41,12 +41,12 @@ config ATH10K_USB
  work in progress and will not fully work.
 
 config ATH10K_SNOC
-tristate "Qualcomm ath10k SNOC support (EXPERIMENTAL)"
-depends on ATH10K && ARCH_QCOM
----help---
-  This module adds support for integrated WCN3990 chip connected
-  to system NOC(SNOC). Currently work in progress and will not
-  fully work.
+   tristate "Qualcomm ath10k SNOC support (EXPERIMENTAL)"
+   depends on ATH10K && ARCH_QCOM
+   ---help---
+ This module adds support for integrated WCN3990 chip connected
+ to system NOC(SNOC). Currently work in progress and will not
+ fully work.
 
 config ATH10K_DEBUG
bool "Atheros ath10k debugging"
-- 
2.17.1


___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 2/2] ath10k: allow ATH10K_SNOC with COMPILE_TEST

2018-06-12 Thread Niklas Cassel 
ATH10K_SNOC builds just fine with COMPILE_TEST, so make that possible.

Signed-off-by: Niklas Cassel 
---
 drivers/net/wireless/ath/ath10k/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/Kconfig 
b/drivers/net/wireless/ath/ath10k/Kconfig
index 54ff5930126c..6572a43590a8 100644
--- a/drivers/net/wireless/ath/ath10k/Kconfig
+++ b/drivers/net/wireless/ath/ath10k/Kconfig
@@ -42,7 +42,8 @@ config ATH10K_USB
 
 config ATH10K_SNOC
tristate "Qualcomm ath10k SNOC support (EXPERIMENTAL)"
-   depends on ATH10K && ARCH_QCOM
+   depends on ATH10K
+   depends on ARCH_QCOM || COMPILE_TEST
---help---
  This module adds support for integrated WCN3990 chip connected
  to system NOC(SNOC). Currently work in progress and will not
-- 
2.17.1


___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH net-next] ath10k: make some functions static

2018-06-04 Thread Niklas Cassel 
On Mon, Jun 04, 2018 at 01:27:43PM +0300, Kalle Valo wrote:
> Wei Yongjun  writes:
> 
> > Fixes the following sparse warnings:
> >
> > drivers/net/wireless/ath/ath10k/snoc.c:823:5: warning:
> >  symbol 'ath10k_snoc_get_ce_id_from_irq' was not declared. Should it be 
> > static?
> > drivers/net/wireless/ath/ath10k/snoc.c:871:6: warning:
> >  symbol 'ath10k_snoc_init_napi' was not declared. Should it be static?
> >
> > Signed-off-by: Wei Yongjun 
> 
> BTW this goes to my ath.git tree, not to net-next.
> 
> I had missed these as I can't enable ATH10K_SNOC on x86 and hence I
> don't test compile snoc.c at all. Bjorn, is there a way to solve
> that? IIRC we had a similar problem with wcn36xx but I don't remember
> anymore how it was fixed.
> 

Perhaps something like:

--- a/drivers/net/wireless/ath/ath10k/Kconfig
+++ b/drivers/net/wireless/ath/ath10k/Kconfig
@@ -42,7 +42,8 @@ config ATH10K_USB
 
 config ATH10K_SNOC
 tristate "Qualcomm ath10k SNOC support (EXPERIMENTAL)"
-depends on ATH10K && ARCH_QCOM
+depends on ATH10K
+depends on ARCH_QCOM || COMPILE_TEST
 ---help---
   This module adds support for integrated WCN3990 chip connected
   to system NOC(SNOC). Currently work in progress and will not


Regards,
Niklas


> -- 
> Kalle Valo
> 
> ___
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k

___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: transmit queued frames after waking queues

2018-05-25 Thread Niklas Cassel 
On Fri, May 25, 2018 at 08:50:23AM -0400, Bob Copeland wrote:
> On Fri, May 25, 2018 at 02:36:56PM +0200, Niklas Cassel wrote:
> > A spin lock does have the advantage of ordering: memory operations issued
> > before the spin_unlock_bh() will be completed before the spin_unlock_bh()
> > operation has completed.
> > 
> > However, ath10k_htt_tx_dec_pending() was called earlier in the same 
> > function,
> > which decreases htt->num_pending_tx, so that write will be completed before
> > our read. That is the only ordering we care about here (if we should call
> > ath10k_mac_tx_push_pending() or not).
> 
> Sure.  I also understand that reading inside a lock and operating on the
> value outside the lock isn't really the definition of synchronization
> (doesn't really matter in this case though).
> 
> I was just suggesting that the implicit memory barrier in the spin unlock
> that we are already paying for would be sufficient here too, and it matches
> the semantic of "tx fields under tx_lock."  On the other hand, maybe it's
> just me, but I tend to look askance at just-in-case READ_ONCEs sprinkled
> about.

I agree, because of the implicit memory barrier from spin_unlock_bh(),
READ_ONCE shouldn't really be needed in this case.

I think that it's a good thing to be critical of all "just-in-case" things,
however, it's not always that obvious if you actually need READ_ONCE or not.

E.g. you might need to use it even when you are holding a spin_lock.

Some people recommend to use it for all concurrent non-read-only shared memory
accesses: https://github.com/google/ktsan/wiki/READ_ONCE-and-WRITE_ONCE

Is there a better guideline somewhere..?


Kind regards,
Niklas

___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: transmit queued frames after waking queues

2018-05-25 Thread Niklas Cassel 
On Thu, May 24, 2018 at 11:50:34AM -0400, Bob Copeland wrote:
> On Mon, May 21, 2018 at 10:37:01PM +0200, Niklas Cassel wrote:
> > On Thu, May 17, 2018 at 03:26:25PM -0700, Adrian Chadd wrote:
> > > On Thu, 17 May 2018 at 16:16, Niklas Cassel <niklas.cas...@linaro.org>
> > > wrote:
> > > 
> > > > diff --git a/drivers/net/wireless/ath/ath10k/txrx.c
> > > b/drivers/net/wireless/ath/ath10k/txrx.c
> > > > index cda164f6e9f6..1d3b2d2c3fee 100644
> > > > --- a/drivers/net/wireless/ath/ath10k/txrx.c
> > > > +++ b/drivers/net/wireless/ath/ath10k/txrx.c
> > > > @@ -95,6 +95,9 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
> > > >  wake_up(>empty_tx_wq);
> > > >  spin_unlock_bh(>tx_lock);
> > > 
> > > > +   if (htt->num_pending_tx <= 3 && !list_empty(>txqs))
> > > > +   ath10k_mac_tx_push_pending(ar);
> > > > +
> > > 
> > > Just sanity checking - what's protecting htt->num_pending_tx? or is it
> > > serialised some other way?
> [...]
> > I can't see that any of the examples applies, but let's add READ_ONCE(),
> > to make sure that the compiler doesn't try to optimize this.
> 
> Couldn't you just move the num_pending_tx read inside tx_lock which is 2 lines
> above?  I think all the other manipulations are protected by tx_lock.

Hello Bob,

There is both a V2 and V3 of this patchset, V3 moves this to a sdio.c and
calls ath10k_mac_tx_push_pending() unconditionally.


But to answer your question,
it shouldn't matter if the read is done while holding the lock or not.

Sure, the compiler could do things so that the code path is always or never
executed, but that is why I added READ_ONCE() in V2.

A spin lock does have the advantage of ordering: memory operations issued
before the spin_unlock_bh() will be completed before the spin_unlock_bh()
operation has completed.

However, ath10k_htt_tx_dec_pending() was called earlier in the same function,
which decreases htt->num_pending_tx, so that write will be completed before
our read. That is the only ordering we care about here (if we should call
ath10k_mac_tx_push_pending() or not).


Kind regards,
Niklas

___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2] ath10k: transmit queued frames after waking queues

2018-05-23 Thread Niklas Cassel 
On Wed, May 23, 2018 at 06:25:49PM +0200, Erik Stromdahl wrote:
> 
> 
> On 05/22/2018 11:15 PM, Niklas Cassel wrote:
> 
> 
> > > 
> > > Earlier we observed performance issues in calling push_pending from each
> > > tx completion. IMHO this change may introduce the same problem again.
> > 
> > I prefer functional TX over performance issues,
> > but I agree that it is unfortunate that SDIO doesn't use
> > ath10k_htt_txrx_compl_task().
> > Erik, is there a reason for this?
> The reason is that the SDIO code has been derived mainly from qcacld and 
> ath6kl
> and they don't implement napi.
> 
> ath10k_htt_txrx_compl_task is currently only called from the napi poll 
> function,
> and the sdio bus driver doesn't have such a function.

Ok, thanks for the explanation. Perhaps we can change the SDIO code so that it
uses NAPI in the future.



> > Another solution might be to change so that we only call
> > ath10k_mac_tx_push_pending() from ath10k_txrx_tx_unref()
> > if (htt->num_pending_tx == 0). That should decrease the number
> > of calls to ath10k_mac_tx_push_pending(), while still avoiding
> > a "TX deadlock" scenario for SDIO.
> Just out of curiosity, where did the limit of 3 come from?
> If it works with a limit of 0, I think it should be used instead.

It came from mt76_txq_schedule():

if (hwq->swq_queued >= 4 || list_empty(>swq))
break;

len = mt76_txq_schedule_list(dev, hwq);

Since this used a break, I simply inverted the logic,
and called ath10k_mac_tx_push_pending() rather than
mt76_txq_schedule_list().

However, I've submitted a V4 now that mimics the behavior
in ath10k_htt_txrx_compl_task() instead, so now I call
ath10k_mac_tx_push_pending() regardless of num_pending_tx.

In most cases, ath10k_mac_tx_push_pending() will not dequeue
any frames, since the ar->txqs list will be empty, so this
shouldn't be so bad after all.

> 
> Another intersting thing that I stumbled upon when looking into the
> code (while writing this email) is the *wake_up(>empty_tx_wq);*
> 
> For some reason I have considered it not to be applicable for HL devices.
> 
> The queue is waited for in the flush op (*ath10k_flush*).
> I am unsure what it is used for, but I don't think it affects the TX
> deadlock scenario.

It seems to be called by mac80211 in certain scenarios, but like you said,
it doesn't help with this problem.


Regards,
Niklas

___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v3] ath10k: transmit queued frames after processing rx packets

2018-05-23 Thread Niklas Cassel 
On Thu, May 24, 2018 at 12:15:08AM +0200, Niklas Cassel wrote:
> This problem cannot be reproduced on low-latency devices, e.g. pci,
> since they call ath10k_mac_tx_push_pending() from
> ath10k_htt_txrx_compl_task(). ath10k_htt_txrx_compl_task() is not called
> on high-latency devices.
> Fix the problem by calling ath10k_mac_tx_push_pending(), after
> processing rx packets, just like for low-latency devices, also in the
> SDIO case. Since we are calling ath10k_mac_tx_push_pending() directly,
> we also need to export it.
> 

Even if we are now calling ath10k_mac_tx_push_pending each time we process rx 
packets,
the number of packets we actually queue from ath10k_mac_tx_push_pending are 
quite few:

>From running iperf for 20 seconds:

# grep ath10k_mac_tx_push_txq /sys/kernel/debug/tracing/trace | grep -v 
ath10k_mac_op_wake_tx_queue | wc -l
233
number of times ath10k_mac_tx_push_txq was called, but not from 
ath10k_mac_op_wake_tx_queue,
i.e. number of times ath10k_mac_tx_push_txq was called from 
ath10k_mac_tx_push_pending.


# grep ath10k_mac_tx_push_txq /sys/kernel/debug/tracing/trace | grep -v 
ath10k_mac_tx_push_pending | wc -l
28415
number of times ath10k_mac_tx_push_txq was called, but not from 
ath10k_mac_tx_push_pending,
i.e number of times ath10k_mac_tx_push_txq was called from 
ath10k_mac_op_wake_tx_queue.


Regards,
Niklas

___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH v3] ath10k: transmit queued frames after processing rx packets

2018-05-23 Thread Niklas Cassel 
When running iperf on ath10k SDIO, TX can stop working:

iperf -c 192.168.1.1 -i 1 -t 20 -w 10K
[  3]  0.0- 1.0 sec  2.00 MBytes  16.8 Mbits/sec
[  3]  1.0- 2.0 sec  3.12 MBytes  26.2 Mbits/sec
[  3]  2.0- 3.0 sec  3.25 MBytes  27.3 Mbits/sec
[  3]  3.0- 4.0 sec   655 KBytes  5.36 Mbits/sec
[  3]  4.0- 5.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  5.0- 6.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  6.0- 7.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  7.0- 8.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  8.0- 9.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  9.0-10.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  0.0-10.3 sec  9.01 MBytes  7.32 Mbits/sec

There are frames in the ieee80211_txq and there are frames that have
been removed from from this queue, but haven't yet been sent on the wire
(num_pending_tx).

When num_pending_tx reaches max_num_pending_tx, we will stop the queues
by calling ieee80211_stop_queues().

As frames that have previously been sent for transmission
(num_pending_tx) are completed, we will decrease num_pending_tx and wake
the queues by calling ieee80211_wake_queue(). ieee80211_wake_queue()
does not call wake_tx_queue, so we might still have frames in the
queue at this point.

While the queues were stopped, the socket buffer might have filled up,
and in order for user space to write more, we need to free the frames
in the queue, since they are accounted to the socket. In order to free
them, we first need to transmit them.

This problem cannot be reproduced on low-latency devices, e.g. pci,
since they call ath10k_mac_tx_push_pending() from
ath10k_htt_txrx_compl_task(). ath10k_htt_txrx_compl_task() is not called
on high-latency devices.
Fix the problem by calling ath10k_mac_tx_push_pending(), after
processing rx packets, just like for low-latency devices, also in the
SDIO case. Since we are calling ath10k_mac_tx_push_pending() directly,
we also need to export it.

Signed-off-by: Niklas Cassel <niklas.cas...@linaro.org>
---
Changes since V2:
Moved ath10k_mac_tx_push_pending() call to ath10k_sdio_irq_handler().

 drivers/net/wireless/ath/ath10k/mac.c  | 1 +
 drivers/net/wireless/ath/ath10k/sdio.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
b/drivers/net/wireless/ath/ath10k/mac.c
index 487a7a7380fd..bfcd9705ed54 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4038,6 +4038,7 @@ void ath10k_mac_tx_push_pending(struct ath10k *ar)
rcu_read_unlock();
spin_unlock_bh(>txqs_lock);
 }
+EXPORT_SYMBOL(ath10k_mac_tx_push_pending);
 
 //
 /* Scanning */
diff --git a/drivers/net/wireless/ath/ath10k/sdio.c 
b/drivers/net/wireless/ath/ath10k/sdio.c
index d612ce8c9cff..2856c75f9011 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -30,6 +30,7 @@
 #include "debug.h"
 #include "hif.h"
 #include "htc.h"
+#include "mac.h"
 #include "targaddrs.h"
 #include "trace.h"
 #include "sdio.h"
@@ -1342,6 +1343,8 @@ static void ath10k_sdio_irq_handler(struct sdio_func 
*func)
break;
} while (time_before(jiffies, timeout) && !done);
 
+   ath10k_mac_tx_push_pending(ar);
+
sdio_claim_host(ar_sdio->func);
 
if (ret && ret != -ECANCELED)
-- 
2.17.0


___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2] ath10k: transmit queued frames after waking queues

2018-05-22 Thread Niklas Cassel 
On Mon, May 21, 2018 at 04:11:38PM -0700, Rajkumar Manoharan wrote:
> On 2018-05-21 13:43, Niklas Cassel wrote:
> > The following problem was observed when running iperf:
> [...]
> > 
> > In order to avoid trying to flush the queue every time we free a frame,
> > only do this when there are 3 or less frames pending, and while we
> > actually have frames in the queue. This logic was copied from
> > mt76_txq_schedule (mt76), one of few other drivers that are actually
> > using wake_tx_queue.
> > 
> > Suggested-by: Toke Høiland-Jørgensen <t...@toke.dk>
> > Signed-off-by: Niklas Cassel <niklas.cas...@linaro.org>
> > ---
> > Changes since V1: use READ_ONCE() to disallow the compiler
> > optimizing things in undesirable ways.
> > 
> >  drivers/net/wireless/ath/ath10k/txrx.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/net/wireless/ath/ath10k/txrx.c
> > b/drivers/net/wireless/ath/ath10k/txrx.c
> > index cda164f6e9f6..264cf0bd5c00 100644
> > --- a/drivers/net/wireless/ath/ath10k/txrx.c
> > +++ b/drivers/net/wireless/ath/ath10k/txrx.c
> > @@ -95,6 +95,9 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
> > wake_up(>empty_tx_wq);
> > spin_unlock_bh(>tx_lock);
> > 
> > +   if (READ_ONCE(htt->num_pending_tx) <= 3 && !list_empty(>txqs))
> > +   ath10k_mac_tx_push_pending(ar);
> > +
> Niklas,

Hello Rajkumar

> 
> Sorry for the late response. ath10k_mac_tx_push_pending is already called
> at the end of NAPI handler. Isn't that enough to process pending frames?

This is true for e.g. ATH10K_BUS_PCI and ATH10K_BUS_SNOC,
but not for e.g. ATH10K_BUS_SDIO and ATH10K_BUS_USB.

While there is some SDIO code merged in Kalle's tree already,
this problem was found when merging
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/?h=ath10k-pending-sdio-usb
with Kalle's ath-next branch.

> 
> Earlier we observed performance issues in calling push_pending from each
> tx completion. IMHO this change may introduce the same problem again.

I prefer functional TX over performance issues,
but I agree that it is unfortunate that SDIO doesn't use
ath10k_htt_txrx_compl_task().
Erik, is there a reason for this?

Perhaps it would be possible to call ath10k_mac_tx_push_pending()
from the equivalent to ath10k_htt_txrx_compl_task(),
but from SDIO's point of view.

Another solution might be to change so that we only call
ath10k_mac_tx_push_pending() from ath10k_txrx_tx_unref()
if (htt->num_pending_tx == 0). That should decrease the number
of calls to ath10k_mac_tx_push_pending(), while still avoiding
a "TX deadlock" scenario for SDIO.


Regards,
Niklas

___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH v2] ath10k: transmit queued frames after waking queues

2018-05-21 Thread Niklas Cassel 
The following problem was observed when running iperf:

[  3]  0.0- 1.0 sec  2.00 MBytes  16.8 Mbits/sec
[  3]  1.0- 2.0 sec  3.12 MBytes  26.2 Mbits/sec
[  3]  2.0- 3.0 sec  3.25 MBytes  27.3 Mbits/sec
[  3]  3.0- 4.0 sec   655 KBytes  5.36 Mbits/sec
[  3]  4.0- 5.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  5.0- 6.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  6.0- 7.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  7.0- 8.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  8.0- 9.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  9.0-10.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  0.0-10.3 sec  9.01 MBytes  7.32 Mbits/sec

There are frames in the ieee80211_txq and there are frames that have
been removed from from this queue, but haven't yet been sent on the wire
(num_pending_tx).

When num_pending_tx reaches max_num_pending_tx, we will stop the queues
by calling ieee80211_stop_queues().

As frames that have previously been sent for transmission
(num_pending_tx) are completed, we will decrease num_pending_tx and wake
the queues by calling ieee80211_wake_queue(). ieee80211_wake_queue()
does not call wake_tx_queue, so we might still have frames in the
queue at this point.

While the queues were stopped, the socket buffer might have filled up,
and in order for user space to write more, we need to free the frames
in the queue, since they are accounted to the socket. In order to free
them, we first need to transmit them.

In order to avoid trying to flush the queue every time we free a frame,
only do this when there are 3 or less frames pending, and while we
actually have frames in the queue. This logic was copied from
mt76_txq_schedule (mt76), one of few other drivers that are actually
using wake_tx_queue.

Suggested-by: Toke Høiland-Jørgensen <t...@toke.dk>
Signed-off-by: Niklas Cassel <niklas.cas...@linaro.org>
---
Changes since V1: use READ_ONCE() to disallow the compiler
optimizing things in undesirable ways.

 drivers/net/wireless/ath/ath10k/txrx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/txrx.c 
b/drivers/net/wireless/ath/ath10k/txrx.c
index cda164f6e9f6..264cf0bd5c00 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -95,6 +95,9 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
wake_up(>empty_tx_wq);
spin_unlock_bh(>tx_lock);
 
+   if (READ_ONCE(htt->num_pending_tx) <= 3 && !list_empty(>txqs))
+   ath10k_mac_tx_push_pending(ar);
+
dma_unmap_single(dev, skb_cb->paddr, msdu->len, DMA_TO_DEVICE);
 
ath10k_report_offchan_tx(htt->ar, msdu);
-- 
2.17.0


___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: transmit queued frames after waking queues

2018-05-21 Thread Niklas Cassel 
On Thu, May 17, 2018 at 03:26:25PM -0700, Adrian Chadd wrote:
> On Thu, 17 May 2018 at 16:16, Niklas Cassel <niklas.cas...@linaro.org>
> wrote:
> 
> > diff --git a/drivers/net/wireless/ath/ath10k/txrx.c
> b/drivers/net/wireless/ath/ath10k/txrx.c
> > index cda164f6e9f6..1d3b2d2c3fee 100644
> > --- a/drivers/net/wireless/ath/ath10k/txrx.c
> > +++ b/drivers/net/wireless/ath/ath10k/txrx.c
> > @@ -95,6 +95,9 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
> >  wake_up(>empty_tx_wq);
> >  spin_unlock_bh(>tx_lock);
> 
> > +   if (htt->num_pending_tx <= 3 && !list_empty(>txqs))
> > +   ath10k_mac_tx_push_pending(ar);
> > +
> 
> Just sanity checking - what's protecting htt->num_pending_tx? or is it
> serialised some other way?

Hello Adrian,

I did not take the htt->tx_lock lock for this since it should not be
needed.

There are however some compiler optimizations that you have to look out
for:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/memory-barriers.txt?h=v4.17-rc6#n1547

I can't see that any of the examples applies, but let's add READ_ONCE(),
to make sure that the compiler doesn't try to optimize this.

Will send a V2 for this.

Regards,
Niklas

> 
> >  dma_unmap_single(dev, skb_cb->paddr, msdu->len, DMA_TO_DEVICE);
> 
> >  ath10k_report_offchan_tx(htt->ar, msdu);
> > --
> > 2.17.0

___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: ath10k wake_tx_queue issues

2018-05-17 Thread Niklas Cassel 
On Wed, May 16, 2018 at 07:28:21PM +0200, Erik Stromdahl wrote:
> Hello Niklas
> 
> Quick question:
> Are you using my patch: "ath10k: add htt_tx num_pending window"?

Nope, but I definitely think that your patch should be merged,
since the current code can lock/unlock/lock a lot of times for
no good reason.

(I actually tried it, but I could still reproduce the bug with it.)

> 
> I assume (from your logs below) that you are not...
> 

Thanks a lot for you suggestions Erik!

Increasing max_num_pending might be a good idea (perhaps we will
get better thoughput in the SDIO case).

However, increasing either max_num_pending or
HTC_HOST_MAX_MSG_PER_TX_BUNDLE would probably just move the problem,
since it would still be possible for us to get hit by the same problem
again in the future.

I actually took Toke's suggestion and cooked up a patch:
https://marc.info/?l=linux-kernel=152659902128543=2


Kind regards,
Niklas



___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH] ath10k: transmit queued frames after waking queues

2018-05-17 Thread Niklas Cassel 
The following problem was observed when running iperf:

[  3]  0.0- 1.0 sec  2.00 MBytes  16.8 Mbits/sec
[  3]  1.0- 2.0 sec  3.12 MBytes  26.2 Mbits/sec
[  3]  2.0- 3.0 sec  3.25 MBytes  27.3 Mbits/sec
[  3]  3.0- 4.0 sec   655 KBytes  5.36 Mbits/sec
[  3]  4.0- 5.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  5.0- 6.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  6.0- 7.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  7.0- 8.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  8.0- 9.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  9.0-10.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  0.0-10.3 sec  9.01 MBytes  7.32 Mbits/sec

There are frames in the ieee80211_txq and there are frames that have
been removed from from this queue, but haven't yet been sent on the wire
(num_pending_tx).

When num_pending_tx reaches max_num_pending_tx, we will stop the queues
by calling ieee80211_stop_queues().

As frames that have previously been sent for transmission
(num_pending_tx) are completed, we will decrease num_pending_tx and wake
the queues by calling ieee80211_wake_queue(). ieee80211_wake_queue()
does not call wake_tx_queue, so we might still have frames in the
queue at this point.

While the queues were stopped, the socket buffer might have filled up,
and in order for user space to write more, we need to free the frames
in the queue, since they are accounted to the socket. In order to free
them, we first need to transmit them.

In order to avoid trying to flush the queue every time we free a frame,
only do this when there are 3 or less frames pending, and while we
actually have frames in the queue. This logic was copied from
mt76_txq_schedule (mt76), one of few other drivers that are actually
using wake_tx_queue.

Suggested-by: Toke Høiland-Jørgensen <t...@toke.dk>
Signed-off-by: Niklas Cassel <niklas.cas...@linaro.org>
---
 drivers/net/wireless/ath/ath10k/txrx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/txrx.c 
b/drivers/net/wireless/ath/ath10k/txrx.c
index cda164f6e9f6..1d3b2d2c3fee 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -95,6 +95,9 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
wake_up(>empty_tx_wq);
spin_unlock_bh(>tx_lock);
 
+   if (htt->num_pending_tx <= 3 && !list_empty(>txqs))
+   ath10k_mac_tx_push_pending(ar);
+
dma_unmap_single(dev, skb_cb->paddr, msdu->len, DMA_TO_DEVICE);
 
ath10k_report_offchan_tx(htt->ar, msdu);
-- 
2.17.0


___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: ath10k wake_tx_queue issues

2018-05-15 Thread Niklas Cassel 
On Tue, May 15, 2018 at 04:13:48PM +0200, Toke Høiland-Jørgensen wrote:
> [ Adding Felix ]
> 
> 
> Niklas Cassel <niklas.cas...@linaro.org> writes:
> 
> > Hello mac80211 and ath10k people
> >
> > Using ath10k, TX stops working when running iperf
> >
> > [  3]  0.0- 1.0 sec  2.00 MBytes  16.8 Mbits/sec
> > [  3]  1.0- 2.0 sec  3.12 MBytes  26.2 Mbits/sec
> > [  3]  2.0- 3.0 sec  3.25 MBytes  27.3 Mbits/sec
> > [  3]  3.0- 4.0 sec   655 KBytes  5.36 Mbits/sec
> > [  3]  4.0- 5.0 sec  0.00 Bytes  0.00 bits/sec
> > [  3]  5.0- 6.0 sec  0.00 Bytes  0.00 bits/sec
> > [  3]  6.0- 7.0 sec  0.00 Bytes  0.00 bits/sec
> > [  3]  7.0- 8.0 sec  0.00 Bytes  0.00 bits/sec
> > [  3]  8.0- 9.0 sec  0.00 Bytes  0.00 bits/sec
> > [  3]  9.0-10.0 sec  0.00 Bytes  0.00 bits/sec
> > [  3]  0.0-10.3 sec  9.01 MBytes  7.32 Mbits/sec
> >
> > The problem can be reproduced without specifying a send buffer size,
> > however, specifying a small send buffer helps to reproduce the problem 
> > faster.
> >
> > What happens is that iperf gets -EAGAIN on write().
> > It continues to get -EAGAIN, even if iperf runs for e.g. 300 seconds.
> > The reason why we get -EAGAIN is because the send socket buffer is full
> > (iperf uses non-blocking I/O).
> >
> > The problem is that the mac80211 wake_tx_queue callback never comes.
> >
> > I guess the best way to describe this is to show my ftrace buffer:
> >
> >  ksoftirqd/2-21[002] .ns474.711744: ath10k_htt_tx_dec_pending: 
> > num_pen: 60
> >  ksoftirqd/2-21[002] .ns374.711761: 
> > ath10k_mac_op_wake_tx_queue: txq: frame_cnt: 1 byte_cnt: 1534 f_txq: 
> > frame_cnt: 1 byte_cnt: 1534 num_pen: 60 queue: 0
> >  ksoftirqd/2-21[002] .ns474.711765: ath10k_htt_tx_inc_pending: 
> > num_pen: 61
> >  ksoftirqd/2-21[002] .ns474.711781: ath10k_htt_tx_inc_pending: 
> > num_pen: 62
> >  ksoftirqd/2-21[002] .ns474.711787: ath10k_htt_tx_dec_pending: 
> > num_pen: 61
> >  ksoftirqd/2-21[002] .ns374.711803: 
> > ath10k_mac_op_wake_tx_queue: txq: frame_cnt: 1 byte_cnt: 1534 f_txq: 
> > frame_cnt: 1 byte_cnt: 1534 num_pen: 61 queue: 0
> >  ksoftirqd/2-21[002] .ns474.711807: ath10k_htt_tx_inc_pending: 
> > num_pen: 62
> >  ksoftirqd/2-21[002] .ns474.711823: ath10k_htt_tx_inc_pending: 
> > num_pen: 63
> >  ksoftirqd/2-21[002] .ns474.711829: ath10k_htt_tx_dec_pending: 
> > num_pen: 62
> >  ksoftirqd/2-21[002] .ns374.711845: 
> > ath10k_mac_op_wake_tx_queue: txq: frame_cnt: 1 byte_cnt: 1534 f_txq: 
> > frame_cnt: 1 byte_cnt: 1534 num_pen: 62 queue: 0
> >  ksoftirqd/2-21[002] .ns474.711849: ath10k_htt_tx_inc_pending: 
> > num_pen: 63
> >  ksoftirqd/2-21[002] .ns474.711865: ath10k_htt_tx_inc_pending: 
> > num_pen: 64
> >  ksoftirqd/2-21[002] dns574.711870: stop_queue: phy0 queue:0, 
> > reason:0
> >  ksoftirqd/2-21[002] dns574.711874: stop_queue: phy0 queue:1, 
> > reason:0
> >  ksoftirqd/2-21[002] dns574.711877: stop_queue: phy0 queue:2, 
> > reason:0
> >  ksoftirqd/2-21[002] dns574.711880: stop_queue: phy0 queue:3, 
> > reason:0
> >  ksoftirqd/2-21[002] dns574.711882: stop_queue: phy0 queue:4, 
> > reason:0
> >  ksoftirqd/2-21[002] dns574.711885: stop_queue: phy0 queue:5, 
> > reason:0
> >  ksoftirqd/2-21[002] dns574.711887: stop_queue: phy0 queue:6, 
> > reason:0
> >  ksoftirqd/2-21[002] dns574.711890: stop_queue: phy0 queue:7, 
> > reason:0
> >  ksoftirqd/2-21[002] dns574.711892: stop_queue: phy0 queue:8, 
> > reason:0
> >  ksoftirqd/2-21[002] dns574.711895: stop_queue: phy0 queue:9, 
> > reason:0
> >  ksoftirqd/2-21[002] dns574.711898: stop_queue: phy0 queue:10, 
> > reason:0
> >  ksoftirqd/2-21[002] dns574.711900: stop_queue: phy0 queue:11, 
> > reason:0
> >  ksoftirqd/2-21[002] dns574.711903: stop_queue: phy0 queue:12, 
> > reason:0
> >  ksoftirqd/2-21[002] dns574.711905: stop_queue: phy0 queue:13, 
> > reason:0
> >  ksoftirqd/2-21[002] dns574.711908: stop_queue: phy0 queue:14, 
> > reason:0
> >  ksoftirqd/2-21[002] dns574.711910: stop_queue: phy0 queue:15, 
> > reason:0
> >  ksoftirqd/2-21[002] .ns474.711917: ath10k_htt_tx_dec_pending: 
> > num_pen: 63
> >  ksoftirqd/2-21[002] dns574.711922: wake_queue

ath10k wake_tx_queue issues

2018-05-15 Thread Niklas Cassel 
Hello mac80211 and ath10k people

Using ath10k, TX stops working when running iperf

[  3]  0.0- 1.0 sec  2.00 MBytes  16.8 Mbits/sec
[  3]  1.0- 2.0 sec  3.12 MBytes  26.2 Mbits/sec
[  3]  2.0- 3.0 sec  3.25 MBytes  27.3 Mbits/sec
[  3]  3.0- 4.0 sec   655 KBytes  5.36 Mbits/sec
[  3]  4.0- 5.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  5.0- 6.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  6.0- 7.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  7.0- 8.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  8.0- 9.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  9.0-10.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  0.0-10.3 sec  9.01 MBytes  7.32 Mbits/sec

The problem can be reproduced without specifying a send buffer size,
however, specifying a small send buffer helps to reproduce the problem faster.

What happens is that iperf gets -EAGAIN on write().
It continues to get -EAGAIN, even if iperf runs for e.g. 300 seconds.
The reason why we get -EAGAIN is because the send socket buffer is full
(iperf uses non-blocking I/O).

The problem is that the mac80211 wake_tx_queue callback never comes.

I guess the best way to describe this is to show my ftrace buffer:

 ksoftirqd/2-21[002] .ns474.711744: ath10k_htt_tx_dec_pending: 
num_pen: 60
 ksoftirqd/2-21[002] .ns374.711761: ath10k_mac_op_wake_tx_queue: 
txq: frame_cnt: 1 byte_cnt: 1534 f_txq: frame_cnt: 1 byte_cnt: 1534 num_pen: 60 
queue: 0
 ksoftirqd/2-21[002] .ns474.711765: ath10k_htt_tx_inc_pending: 
num_pen: 61
 ksoftirqd/2-21[002] .ns474.711781: ath10k_htt_tx_inc_pending: 
num_pen: 62
 ksoftirqd/2-21[002] .ns474.711787: ath10k_htt_tx_dec_pending: 
num_pen: 61
 ksoftirqd/2-21[002] .ns374.711803: ath10k_mac_op_wake_tx_queue: 
txq: frame_cnt: 1 byte_cnt: 1534 f_txq: frame_cnt: 1 byte_cnt: 1534 num_pen: 61 
queue: 0
 ksoftirqd/2-21[002] .ns474.711807: ath10k_htt_tx_inc_pending: 
num_pen: 62
 ksoftirqd/2-21[002] .ns474.711823: ath10k_htt_tx_inc_pending: 
num_pen: 63
 ksoftirqd/2-21[002] .ns474.711829: ath10k_htt_tx_dec_pending: 
num_pen: 62
 ksoftirqd/2-21[002] .ns374.711845: ath10k_mac_op_wake_tx_queue: 
txq: frame_cnt: 1 byte_cnt: 1534 f_txq: frame_cnt: 1 byte_cnt: 1534 num_pen: 62 
queue: 0
 ksoftirqd/2-21[002] .ns474.711849: ath10k_htt_tx_inc_pending: 
num_pen: 63
 ksoftirqd/2-21[002] .ns474.711865: ath10k_htt_tx_inc_pending: 
num_pen: 64
 ksoftirqd/2-21[002] dns574.711870: stop_queue: phy0 queue:0, 
reason:0
 ksoftirqd/2-21[002] dns574.711874: stop_queue: phy0 queue:1, 
reason:0
 ksoftirqd/2-21[002] dns574.711877: stop_queue: phy0 queue:2, 
reason:0
 ksoftirqd/2-21[002] dns574.711880: stop_queue: phy0 queue:3, 
reason:0
 ksoftirqd/2-21[002] dns574.711882: stop_queue: phy0 queue:4, 
reason:0
 ksoftirqd/2-21[002] dns574.711885: stop_queue: phy0 queue:5, 
reason:0
 ksoftirqd/2-21[002] dns574.711887: stop_queue: phy0 queue:6, 
reason:0
 ksoftirqd/2-21[002] dns574.711890: stop_queue: phy0 queue:7, 
reason:0
 ksoftirqd/2-21[002] dns574.711892: stop_queue: phy0 queue:8, 
reason:0
 ksoftirqd/2-21[002] dns574.711895: stop_queue: phy0 queue:9, 
reason:0
 ksoftirqd/2-21[002] dns574.711898: stop_queue: phy0 queue:10, 
reason:0
 ksoftirqd/2-21[002] dns574.711900: stop_queue: phy0 queue:11, 
reason:0
 ksoftirqd/2-21[002] dns574.711903: stop_queue: phy0 queue:12, 
reason:0
 ksoftirqd/2-21[002] dns574.711905: stop_queue: phy0 queue:13, 
reason:0
 ksoftirqd/2-21[002] dns574.711908: stop_queue: phy0 queue:14, 
reason:0
 ksoftirqd/2-21[002] dns574.711910: stop_queue: phy0 queue:15, 
reason:0
 ksoftirqd/2-21[002] .ns474.711917: ath10k_htt_tx_dec_pending: 
num_pen: 63
 ksoftirqd/2-21[002] dns574.711922: wake_queue: phy0 queue:0, 
reason:0
 ksoftirqd/2-21[002] dns574.711929: wake_queue: phy0 queue:15, 
reason:0
 ksoftirqd/2-21[002] .ns374.711948: ath10k_mac_op_wake_tx_queue: 
txq: frame_cnt: 1 byte_cnt: 1534 f_txq: frame_cnt: 1 byte_cnt: 1534 num_pen: 63 
queue: 0
 ksoftirqd/2-21[002] .ns474.711952: ath10k_htt_tx_inc_pending: 
num_pen: 64
 ksoftirqd/2-21[002] dns574.711956: stop_queue: phy0 queue:0, 
reason:0
 ksoftirqd/2-21[002] dns574.711959: stop_queue: phy0 queue:1, 
reason:0
 ksoftirqd/2-21[002] dns574.711962: stop_queue: phy0 queue:2, 
reason:0
 ksoftirqd/2-21[002] dns574.711964: stop_queue: phy0 queue:3, 
reason:0
 ksoftirqd/2-21[002] dns574.711967: stop_queue: phy0 queue:4, 
reason:0
 ksoftirqd/2-21[002] dns574.711969: stop_queue: phy0 queue:5, 
reason:0
 ksoftirqd/2-21[002] dns574.711972: stop_queue: phy0 queue:6, 
reason:0
 ksoftirqd/2-21[002] dns574.711974: stop_queue: phy0 queue:7, 
reason:0
 ksoftirqd/2-21[002] dns5