[PATCH V2 0/6] brcmfmac: flow control and p2p fix series

[PATCH V2 0/6] brcmfmac: flow control and p2p fix series

[Chi-Hsien Lin]

Fix WFA certification failures due to flow control credit issues.
Also fix an issue with a varialbe (dwell time) that is not properly 
initialized.

Chnages since V1:
 - Remove unnecessary casting from patch 03/06
 - Correct a s-o-b email error in patch 03/06 commit message

Amar Shankar (1):
  brcmfmac: reserve 2 credits for host tx control path

Chung-Hsien Hsu (1):
  brcmfmac: update tx status flags to sync with firmware

Double Lo (1):
  brcmfmac: fix throughput zero stalls on PM 1 mode due to credit map

Jia-Shyr Chuang (1):
  brcmfmac: increase message buffer size for control packets

Joseph Chuang (1):
  brcmfmac: initialize the requested dwell time

Raveendran Somu (1):
  brcmfmac: allow credit borrowing for all access categories

 .../broadcom/brcm80211/brcmfmac/cfg80211.c    |  2 +-
 .../broadcom/brcm80211/brcmfmac/fwsignal.c    | 68 +++++++++++++------
 .../broadcom/brcm80211/brcmfmac/msgbuf.c      |  3 +-
 .../broadcom/brcm80211/brcmfmac/p2p.c         |  5 +-
 .../broadcom/brcm80211/brcmfmac/sdio.c        | 19 +++++-
 5 files changed, 67 insertions(+), 30 deletions(-)

-- 
2.25.0

[PATCH V2 1/6] brcmfmac: allow credit borrowing for all access categories

[Chi-Hsien Lin]

From: Raveendran Somu <raveendran.somu@cypress.com>

Current credit borrowing allows only the access category BE to
borrow the credits. This change is to fix the credit borrowing
logic, to make borrowing available for all access categories
and also to borrow only from the lower categories. This fixes WFA
802.11n certs 5.2.27 failures.

Signed-off-by: Raveendran Somu <raveendran.somu@cypress.com>
Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com>
Signed-off-by: Chung-Hsien Hsu <stanley.hsu@cypress.com>
Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
---
 .../broadcom/brcm80211/brcmfmac/fwsignal.c    | 61 +++++++++++--------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
index 09701262330d..d7c36af4b566 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
@@ -383,6 +383,7 @@ struct brcmf_fws_mac_descriptor {
 };
 
 #define BRCMF_FWS_HANGER_MAXITEMS	3072
+#define BRCMF_BORROW_RATIO			3
 
 /**
  * enum brcmf_fws_hanger_item_state - state of hanger item.
@@ -479,7 +480,8 @@ struct brcmf_fws_info {
 	u32 fifo_enqpkt[BRCMF_FWS_FIFO_COUNT];
 	int fifo_credit[BRCMF_FWS_FIFO_COUNT];
 	int init_fifo_credit[BRCMF_FWS_FIFO_COUNT];
-	int credits_borrowed[BRCMF_FWS_FIFO_AC_VO + 1];
+	int credits_borrowed[BRCMF_FWS_FIFO_AC_VO + 1]
+		[BRCMF_FWS_FIFO_AC_VO + 1];
 	int deq_node_pos[BRCMF_FWS_FIFO_COUNT];
 	u32 fifo_credit_map;
 	u32 fifo_delay_map;
@@ -1185,13 +1187,11 @@ static void brcmf_fws_return_credits(struct brcmf_fws_info *fws,
 	if (!credits)
 		return;
 
-	fws->fifo_credit_map |= 1 << fifo;
-
-	if ((fifo == BRCMF_FWS_FIFO_AC_BE) &&
-	    (fws->credits_borrowed[0])) {
+	if (fifo > BRCMF_FWS_FIFO_AC_BK &&
+	    fifo <= BRCMF_FWS_FIFO_AC_VO) {
 		for (lender_ac = BRCMF_FWS_FIFO_AC_VO; lender_ac >= 0;
 		     lender_ac--) {
-			borrowed = &fws->credits_borrowed[lender_ac];
+			borrowed = &fws->credits_borrowed[fifo][lender_ac];
 			if (*borrowed) {
 				fws->fifo_credit_map |= (1 << lender_ac);
 				fifo_credit = &fws->fifo_credit[lender_ac];
@@ -1208,7 +1208,11 @@ static void brcmf_fws_return_credits(struct brcmf_fws_info *fws,
 		}
 	}
 
-	fws->fifo_credit[fifo] += credits;
+	if (credits) {
+		fws->fifo_credit[fifo] += credits;
+		fws->fifo_credit_map |= 1 << fifo;
+	}
+
 	if (fws->fifo_credit[fifo] > fws->init_fifo_credit[fifo])
 		fws->fifo_credit[fifo] = fws->init_fifo_credit[fifo];
 
@@ -2005,27 +2009,31 @@ static void brcmf_fws_rollback_toq(struct brcmf_fws_info *fws,
 	}
 }
 
-static int brcmf_fws_borrow_credit(struct brcmf_fws_info *fws)
+static int brcmf_fws_borrow_credit(struct brcmf_fws_info *fws,
+				   int highest_lender_ac, int borrower_ac,
+				   bool borrow_all)
 {
-	int lender_ac;
+	int lender_ac, borrow_limit = 0;
 
-	if (time_after(fws->borrow_defer_timestamp, jiffies)) {
-		fws->fifo_credit_map &= ~(1 << BRCMF_FWS_FIFO_AC_BE);
-		return -ENAVAIL;
-	}
+	for (lender_ac = 0; lender_ac <= highest_lender_ac; lender_ac++) {
 
-	for (lender_ac = 0; lender_ac <= BRCMF_FWS_FIFO_AC_VO; lender_ac++) {
-		if (fws->fifo_credit[lender_ac] > 0) {
-			fws->credits_borrowed[lender_ac]++;
+		if (!borrow_all)
+			borrow_limit =
+			  fws->init_fifo_credit[lender_ac] / BRCMF_BORROW_RATIO;
+		else
+			borrow_limit = 0;
+
+		if (fws->fifo_credit[lender_ac] > borrow_limit) {
+			fws->credits_borrowed[borrower_ac][lender_ac]++;
 			fws->fifo_credit[lender_ac]--;
 			if (fws->fifo_credit[lender_ac] == 0)
 				fws->fifo_credit_map &= ~(1 << lender_ac);
-			fws->fifo_credit_map |= (1 << BRCMF_FWS_FIFO_AC_BE);
+			fws->fifo_credit_map |= (1 << borrower_ac);
 			brcmf_dbg(DATA, "borrow credit from: %d\n", lender_ac);
 			return 0;
 		}
 	}
-	fws->fifo_credit_map &= ~(1 << BRCMF_FWS_FIFO_AC_BE);
+	fws->fifo_credit_map &= ~(1 << borrower_ac);
 	return -ENAVAIL;
 }
 
@@ -2216,9 +2224,10 @@ static void brcmf_fws_dequeue_worker(struct work_struct *worker)
 			}
 			continue;
 		}
-		while ((fws->fifo_credit[fifo] > 0) ||
+
+		while ((fws->fifo_credit[fifo]) ||
 		       ((!fws->bcmc_credit_check) &&
-			(fifo == BRCMF_FWS_FIFO_BCMC))) {
+				(fifo == BRCMF_FWS_FIFO_BCMC))) {
 			skb = brcmf_fws_deq(fws, fifo);
 			if (!skb)
 				break;
@@ -2228,10 +2237,14 @@ static void brcmf_fws_dequeue_worker(struct work_struct *worker)
 			if (fws->bus_flow_blocked)
 				break;
 		}
-		if ((fifo == BRCMF_FWS_FIFO_AC_BE) &&
-		    (fws->fifo_credit[fifo] <= 0) &&
-		    (!fws->bus_flow_blocked)) {
-			while (brcmf_fws_borrow_credit(fws) == 0) {
+
+		if (fifo >= BRCMF_FWS_FIFO_AC_BE &&
+		    fifo <= BRCMF_FWS_FIFO_AC_VO &&
+		    fws->fifo_credit[fifo] == 0 &&
+		    !fws->bus_flow_blocked) {
+			while (brcmf_fws_borrow_credit(fws,
+						       fifo - 1, fifo,
+						       true) == 0) {
 				skb = brcmf_fws_deq(fws, fifo);
 				if (!skb) {
 					brcmf_fws_return_credits(fws, fifo, 1);
-- 
2.25.0

Re: [PATCH V2 1/6] brcmfmac: allow credit borrowing for all access categories

[Kalle Valo]

Chi-Hsien Lin <chi-hsien.lin@cypress.com> wrote:

> From: Raveendran Somu <raveendran.somu@cypress.com>
> 
> Current credit borrowing allows only the access category BE to
> borrow the credits. This change is to fix the credit borrowing
> logic, to make borrowing available for all access categories
> and also to borrow only from the lower categories. This fixes WFA
> 802.11n certs 5.2.27 failures.
> 
> Signed-off-by: Raveendran Somu <raveendran.somu@cypress.com>
> Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com>
> Signed-off-by: Chung-Hsien Hsu <stanley.hsu@cypress.com>
> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>

6 patches applied to wireless-drivers-next.git, thanks.

683608bde030 brcmfmac: allow credit borrowing for all access categories
fc4aa125974d brcmfmac: increase message buffer size for control packets
b41c232d3366 brcmfmac: reserve 2 credits for host tx control path
d843246ee761 brcmfmac: update tx status flags to sync with firmware
bbf7ae3dcb42 brcmfmac: fix throughput zero stalls on PM 1 mode due to credit map
ad96bc27032c brcmfmac: initialize the requested dwell time

-- 
https://patchwork.kernel.org/patch/11598099/

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

[PATCH V2 2/6] brcmfmac: increase message buffer size for control packets

[Chi-Hsien Lin]

From: Jia-Shyr Chuang <joseph.chuang@cypress.com>

In wifi firmware, max length of IOCTL/IOVAR buffer size is 8192.
Increase the message buffer max size same as wifi firmware for control
packets so return buffers can come back.

Signed-off-by: Soontak Lee <soontak.lee@cypress.com>
Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com>
Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
index 8bb4f1fa790e..f1a20db8daab 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
@@ -54,6 +54,7 @@
 #define BRCMF_IOCTL_REQ_PKTID			0xFFFE
 
 #define BRCMF_MSGBUF_MAX_PKT_SIZE		2048
+#define BRCMF_MSGBUF_MAX_CTL_PKT_SIZE           8192
 #define BRCMF_MSGBUF_RXBUFPOST_THRESHOLD	32
 #define BRCMF_MSGBUF_MAX_IOCTLRESPBUF_POST	8
 #define BRCMF_MSGBUF_MAX_EVENTBUF_POST		8
@@ -1028,7 +1029,7 @@ brcmf_msgbuf_rxbuf_ctrl_post(struct brcmf_msgbuf *msgbuf, bool event_buf,
 		rx_bufpost = (struct msgbuf_rx_ioctl_resp_or_event *)ret_ptr;
 		memset(rx_bufpost, 0, sizeof(*rx_bufpost));
 
-		skb = brcmu_pkt_buf_get_skb(BRCMF_MSGBUF_MAX_PKT_SIZE);
+		skb = brcmu_pkt_buf_get_skb(BRCMF_MSGBUF_MAX_CTL_PKT_SIZE);
 
 		if (skb == NULL) {
 			bphy_err(drvr, "Failed to alloc SKB\n");
-- 
2.25.0

[PATCH V2 3/6] brcmfmac: reserve 2 credits for host tx control path

[Chi-Hsien Lin]

From: Amar Shankar <amsr@cypress.com>

It is observed that sometimes when sdiod is low in tx credits in low
rssi scenarios, the data path consumes all sdiod rx all credits and
there is no sdiod rx credit available for control path causing host
and card to go out of sync resulting in link loss between host and
card. So in order to prevent it some credits are reserved for control
path.

Note that TXCTL_CREDITS can't be larger than the firmware default
credit update threshold 2; otherwise there will be a deadlock for both
side waiting for each other.

Signed-off-by: Amar Shankar <amsr@cypress.com>
Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com>
Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
---
 .../broadcom/brcm80211/brcmfmac/sdio.c        | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index ce6f15284277..4da40436b4ab 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -635,6 +635,8 @@ static const struct brcmf_firmware_mapping brcmf_sdio_fwnames[] = {
 	BRCMF_FW_ENTRY(CY_CC_43012_CHIP_ID, 0xFFFFFFFF, 43012)
 };
 
+#define TXCTL_CREDITS	2
+
 static void pkt_align(struct sk_buff *p, int len, int align)
 {
 	uint datalign;
@@ -648,8 +650,16 @@ static void pkt_align(struct sk_buff *p, int len, int align)
 /* To check if there's window offered */
 static bool data_ok(struct brcmf_sdio *bus)
 {
-	return (u8)(bus->tx_max - bus->tx_seq) != 0 &&
-	       ((u8)(bus->tx_max - bus->tx_seq) & 0x80) == 0;
+	/* Reserve TXCTL_CREDITS credits for txctl */
+	return (bus->tx_max - bus->tx_seq) > TXCTL_CREDITS &&
+	       ((bus->tx_max - bus->tx_seq) & 0x80) == 0;
+}
+
+/* To check if there's window offered */
+static bool txctl_ok(struct brcmf_sdio *bus)
+{
+	return (bus->tx_max - bus->tx_seq) != 0 &&
+	       ((bus->tx_max - bus->tx_seq) & 0x80) == 0;
 }
 
 static int
@@ -2655,7 +2665,7 @@ static void brcmf_sdio_dpc(struct brcmf_sdio *bus)
 	brcmf_sdio_clrintr(bus);
 
 	if (bus->ctrl_frame_stat && (bus->clkstate == CLK_AVAIL) &&
-	    data_ok(bus)) {
+	    txctl_ok(bus)) {
 		sdio_claim_host(bus->sdiodev->func1);
 		if (bus->ctrl_frame_stat) {
 			err = brcmf_sdio_tx_ctrlframe(bus,  bus->ctrl_frame_buf,
@@ -2663,6 +2673,9 @@ static void brcmf_sdio_dpc(struct brcmf_sdio *bus)
 			bus->ctrl_frame_err = err;
 			wmb();
 			bus->ctrl_frame_stat = false;
+			if (err)
+				brcmf_err("sdio ctrlframe tx failed err=%d\n",
+					  err);
 		}
 		sdio_release_host(bus->sdiodev->func1);
 		brcmf_sdio_wait_event_wakeup(bus);
-- 
2.25.0

Re: [PATCH V2 3/6] brcmfmac: reserve 2 credits for host tx control path

[Dmitry Osipenko]

10.06.2020 18:21, Chi-Hsien Lin пишет:
> From: Amar Shankar <amsr@cypress.com>
> 
> It is observed that sometimes when sdiod is low in tx credits in low
> rssi scenarios, the data path consumes all sdiod rx all credits and
> there is no sdiod rx credit available for control path causing host
> and card to go out of sync resulting in link loss between host and
> card. So in order to prevent it some credits are reserved for control
> path.
> 
> Note that TXCTL_CREDITS can't be larger than the firmware default
> credit update threshold 2; otherwise there will be a deadlock for both
> side waiting for each other.
> 
> Signed-off-by: Amar Shankar <amsr@cypress.com>
> Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com>
> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
> ---
>  .../broadcom/brcm80211/brcmfmac/sdio.c        | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index ce6f15284277..4da40436b4ab 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -635,6 +635,8 @@ static const struct brcmf_firmware_mapping brcmf_sdio_fwnames[] = {
>  	BRCMF_FW_ENTRY(CY_CC_43012_CHIP_ID, 0xFFFFFFFF, 43012)
>  };
>  
> +#define TXCTL_CREDITS	2
> +
>  static void pkt_align(struct sk_buff *p, int len, int align)
>  {
>  	uint datalign;
> @@ -648,8 +650,16 @@ static void pkt_align(struct sk_buff *p, int len, int align)
>  /* To check if there's window offered */
>  static bool data_ok(struct brcmf_sdio *bus)
>  {
> -	return (u8)(bus->tx_max - bus->tx_seq) != 0 &&
> -	       ((u8)(bus->tx_max - bus->tx_seq) & 0x80) == 0;
> +	/* Reserve TXCTL_CREDITS credits for txctl */
> +	return (bus->tx_max - bus->tx_seq) > TXCTL_CREDITS &&
> +	       ((bus->tx_max - bus->tx_seq) & 0x80) == 0;
> +}
> +
> +/* To check if there's window offered */
> +static bool txctl_ok(struct brcmf_sdio *bus)
> +{
> +	return (bus->tx_max - bus->tx_seq) != 0 &&
> +	       ((bus->tx_max - bus->tx_seq) & 0x80) == 0;
>  }
>  
>  static int
> @@ -2655,7 +2665,7 @@ static void brcmf_sdio_dpc(struct brcmf_sdio *bus)
>  	brcmf_sdio_clrintr(bus);
>  
>  	if (bus->ctrl_frame_stat && (bus->clkstate == CLK_AVAIL) &&
> -	    data_ok(bus)) {
> +	    txctl_ok(bus)) {
>  		sdio_claim_host(bus->sdiodev->func1);
>  		if (bus->ctrl_frame_stat) {
>  			err = brcmf_sdio_tx_ctrlframe(bus,  bus->ctrl_frame_buf,
> @@ -2663,6 +2673,9 @@ static void brcmf_sdio_dpc(struct brcmf_sdio *bus)
>  			bus->ctrl_frame_err = err;
>  			wmb();
>  			bus->ctrl_frame_stat = false;
> +			if (err)
> +				brcmf_err("sdio ctrlframe tx failed err=%d\n",
> +					  err);
>  		}
>  		sdio_release_host(bus->sdiodev->func1);
>  		brcmf_sdio_wait_event_wakeup(bus);
> 

Hello,

This patch causes a severe WiFi performance regression on BCM4329.
Please fix or revert this patch, thanks in advance.

Before this patch:
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec  17.2 MBytes  14.4 Mbits/sec    0             sender
[  5]   0.00-10.04  sec  16.9 MBytes  14.1 Mbits/sec
receiver


After this patch:
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec  1.05 MBytes   881 Kbits/sec    3             sender
[  5]   0.00-14.01  sec   959 KBytes   561 Kbits/sec
receiver

Re: [PATCH V2 3/6] brcmfmac: reserve 2 credits for host tx control path

[Kalle Valo]

Dmitry Osipenko <digetx@gmail.com> writes:

> 10.06.2020 18:21, Chi-Hsien Lin пишет:
>> From: Amar Shankar <amsr@cypress.com>
>> 
>> It is observed that sometimes when sdiod is low in tx credits in low
>> rssi scenarios, the data path consumes all sdiod rx all credits and
>> there is no sdiod rx credit available for control path causing host
>> and card to go out of sync resulting in link loss between host and
>> card. So in order to prevent it some credits are reserved for control
>> path.
>> 
>> Note that TXCTL_CREDITS can't be larger than the firmware default
>> credit update threshold 2; otherwise there will be a deadlock for both
>> side waiting for each other.
>> 
>> Signed-off-by: Amar Shankar <amsr@cypress.com>
>> Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com>
>> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>

[...]

> This patch causes a severe WiFi performance regression on BCM4329.
> Please fix or revert this patch, thanks in advance.
>
> Before this patch:
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bitrate         Retr
> [  5]   0.00-10.00  sec  17.2 MBytes  14.4 Mbits/sec    0             sender
> [  5]   0.00-10.04  sec  16.9 MBytes  14.1 Mbits/sec
> receiver
>
>
> After this patch:
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bitrate         Retr
> [  5]   0.00-10.00  sec  1.05 MBytes   881 Kbits/sec    3             sender
> [  5]   0.00-14.01  sec   959 KBytes   561 Kbits/sec
> receiver

Can someone please send a revert patch (with the explanation above) if a
fix is not quickly found? The commit id is:

commit b41c232d33666191a1db11befc0f040fcbe664e9
Author:     Amar Shankar <amsr@cypress.com>
AuthorDate: Wed Jun 10 10:21:03 2020 -0500
Commit:     Kalle Valo <kvalo@codeaurora.org>
CommitDate: Tue Jul 14 12:46:43 2020 +0300

    brcmfmac: reserve 2 credits for host tx control path

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

Re: [PATCH V2 3/6] brcmfmac: reserve 2 credits for host tx control path

[Dmitry Osipenko]

04.08.2020 14:08, Kalle Valo пишет:
> Dmitry Osipenko <digetx@gmail.com> writes:
> 
>> 10.06.2020 18:21, Chi-Hsien Lin пишет:
>>> From: Amar Shankar <amsr@cypress.com>
>>>
>>> It is observed that sometimes when sdiod is low in tx credits in low
>>> rssi scenarios, the data path consumes all sdiod rx all credits and
>>> there is no sdiod rx credit available for control path causing host
>>> and card to go out of sync resulting in link loss between host and
>>> card. So in order to prevent it some credits are reserved for control
>>> path.
>>>
>>> Note that TXCTL_CREDITS can't be larger than the firmware default
>>> credit update threshold 2; otherwise there will be a deadlock for both
>>> side waiting for each other.
>>>
>>> Signed-off-by: Amar Shankar <amsr@cypress.com>
>>> Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com>
>>> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
> 
> [...]
> 
>> This patch causes a severe WiFi performance regression on BCM4329.
>> Please fix or revert this patch, thanks in advance.
>>
>> Before this patch:
>> - - - - - - - - - - - - - - - - - - - - - - - - -
>> [ ID] Interval           Transfer     Bitrate         Retr
>> [  5]   0.00-10.00  sec  17.2 MBytes  14.4 Mbits/sec    0             sender
>> [  5]   0.00-10.04  sec  16.9 MBytes  14.1 Mbits/sec
>> receiver
>>
>>
>> After this patch:
>> - - - - - - - - - - - - - - - - - - - - - - - - -
>> [ ID] Interval           Transfer     Bitrate         Retr
>> [  5]   0.00-10.00  sec  1.05 MBytes   881 Kbits/sec    3             sender
>> [  5]   0.00-14.01  sec   959 KBytes   561 Kbits/sec
>> receiver
> 
> Can someone please send a revert patch (with the explanation above) if a
> fix is not quickly found? The commit id is:
> 
> commit b41c232d33666191a1db11befc0f040fcbe664e9
> Author:     Amar Shankar <amsr@cypress.com>
> AuthorDate: Wed Jun 10 10:21:03 2020 -0500
> Commit:     Kalle Valo <kvalo@codeaurora.org>
> CommitDate: Tue Jul 14 12:46:43 2020 +0300
> 
>     brcmfmac: reserve 2 credits for host tx control path
> 

Hello Kalle,

I'll send the revert if nobody will stand up to address the problem in a
two weeks, thanks.

Re: [PATCH V2 3/6] brcmfmac: reserve 2 credits for host tx control path

[Kalle Valo]

Dmitry Osipenko <digetx@gmail.com> writes:

> 04.08.2020 14:08, Kalle Valo пишет:
>> Dmitry Osipenko <digetx@gmail.com> writes:
>> 
>>> 10.06.2020 18:21, Chi-Hsien Lin пишет:
>>>> From: Amar Shankar <amsr@cypress.com>
>>>>
>>>> It is observed that sometimes when sdiod is low in tx credits in low
>>>> rssi scenarios, the data path consumes all sdiod rx all credits and
>>>> there is no sdiod rx credit available for control path causing host
>>>> and card to go out of sync resulting in link loss between host and
>>>> card. So in order to prevent it some credits are reserved for control
>>>> path.
>>>>
>>>> Note that TXCTL_CREDITS can't be larger than the firmware default
>>>> credit update threshold 2; otherwise there will be a deadlock for both
>>>> side waiting for each other.
>>>>
>>>> Signed-off-by: Amar Shankar <amsr@cypress.com>
>>>> Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com>
>>>> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
>> 
>> [...]
>> 
>>> This patch causes a severe WiFi performance regression on BCM4329.
>>> Please fix or revert this patch, thanks in advance.
>>>
>>> Before this patch:
>>> - - - - - - - - - - - - - - - - - - - - - - - - -
>>> [ ID] Interval           Transfer     Bitrate         Retr
>>> [  5]   0.00-10.00  sec  17.2 MBytes  14.4 Mbits/sec    0             sender
>>> [  5]   0.00-10.04  sec  16.9 MBytes  14.1 Mbits/sec
>>> receiver
>>>
>>>
>>> After this patch:
>>> - - - - - - - - - - - - - - - - - - - - - - - - -
>>> [ ID] Interval           Transfer     Bitrate         Retr
>>> [  5]   0.00-10.00  sec  1.05 MBytes   881 Kbits/sec    3             sender
>>> [  5]   0.00-14.01  sec   959 KBytes   561 Kbits/sec
>>> receiver
>> 
>> Can someone please send a revert patch (with the explanation above) if a
>> fix is not quickly found? The commit id is:
>> 
>> commit b41c232d33666191a1db11befc0f040fcbe664e9
>> Author:     Amar Shankar <amsr@cypress.com>
>> AuthorDate: Wed Jun 10 10:21:03 2020 -0500
>> Commit:     Kalle Valo <kvalo@codeaurora.org>
>> CommitDate: Tue Jul 14 12:46:43 2020 +0300
>> 
>>     brcmfmac: reserve 2 credits for host tx control path
>> 
>
> Hello Kalle,
>
> I'll send the revert if nobody will stand up to address the problem in a
> two weeks, thanks.

Thanks. Then I should be able to get the revert to v5.9 so that the
release won't be broken. (v5.8 is unaffected)

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

Re: [PATCH V2 3/6] brcmfmac: reserve 2 credits for host tx control path

[Chi-Hsien Lin]

On 8/5/2020 1:22 AM, Kalle Valo wrote:
> Dmitry Osipenko <digetx@gmail.com> writes:
>
>> 04.08.2020 14:08, Kalle Valo пишет:
>>> Dmitry Osipenko <digetx@gmail.com> writes:
>>>
>>>> 10.06.2020 18:21, Chi-Hsien Lin пишет:
>>>>> From: Amar Shankar <amsr@cypress.com>
>>>>>
>>>>> It is observed that sometimes when sdiod is low in tx credits in low
>>>>> rssi scenarios, the data path consumes all sdiod rx all credits and
>>>>> there is no sdiod rx credit available for control path causing host
>>>>> and card to go out of sync resulting in link loss between host and
>>>>> card. So in order to prevent it some credits are reserved for control
>>>>> path.
>>>>>
>>>>> Note that TXCTL_CREDITS can't be larger than the firmware default
>>>>> credit update threshold 2; otherwise there will be a deadlock for both
>>>>> side waiting for each other.
>>>>>
>>>>> Signed-off-by: Amar Shankar <amsr@cypress.com>
>>>>> Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com>
>>>>> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
>>> [...]
>>>
>>>> This patch causes a severe WiFi performance regression on BCM4329.
>>>> Please fix or revert this patch, thanks in advance.
>>>>
>>>> Before this patch:
>>>> - - - - - - - - - - - - - - - - - - - - - - - - -
>>>> [ ID] Interval           Transfer     Bitrate         Retr
>>>> [  5]   0.00-10.00  sec  17.2 MBytes  14.4 Mbits/sec    0             sender
>>>> [  5]   0.00-10.04  sec  16.9 MBytes  14.1 Mbits/sec
>>>> receiver
>>>>
>>>>
>>>> After this patch:
>>>> - - - - - - - - - - - - - - - - - - - - - - - - -
>>>> [ ID] Interval           Transfer     Bitrate         Retr
>>>> [  5]   0.00-10.00  sec  1.05 MBytes   881 Kbits/sec    3             sender
>>>> [  5]   0.00-14.01  sec   959 KBytes   561 Kbits/sec
>>>> receiver
>>> Can someone please send a revert patch (with the explanation above) if a
>>> fix is not quickly found? The commit id is:
>>>
>>> commit b41c232d33666191a1db11befc0f040fcbe664e9
>>> Author:     Amar Shankar <amsr@cypress.com>
>>> AuthorDate: Wed Jun 10 10:21:03 2020 -0500
>>> Commit:     Kalle Valo <kvalo@codeaurora.org>
>>> CommitDate: Tue Jul 14 12:46:43 2020 +0300
>>>
>>>      brcmfmac: reserve 2 credits for host tx control path
>>>
>> Hello Kalle,
>>
>> I'll send the revert if nobody will stand up to address the problem in a
>> two weeks, thanks.
> Thanks. Then I should be able to get the revert to v5.9 so that the
> release won't be broken. (v5.8 is unaffected)

Dmitry/Kalle,

We'll take a look and revert/fix it in a few days.

Re: [PATCH V2 3/6] brcmfmac: reserve 2 credits for host tx control path

[Dmitry Osipenko]

05.08.2020 11:24, Chi-Hsien Lin пишет:
> 
> On 8/5/2020 1:22 AM, Kalle Valo wrote:
>> Dmitry Osipenko <digetx@gmail.com> writes:
>>
>>> 04.08.2020 14:08, Kalle Valo пишет:
>>>> Dmitry Osipenko <digetx@gmail.com> writes:
>>>>
>>>>> 10.06.2020 18:21, Chi-Hsien Lin пишет:
>>>>>> From: Amar Shankar <amsr@cypress.com>
>>>>>>
>>>>>> It is observed that sometimes when sdiod is low in tx credits in low
>>>>>> rssi scenarios, the data path consumes all sdiod rx all credits and
>>>>>> there is no sdiod rx credit available for control path causing host
>>>>>> and card to go out of sync resulting in link loss between host and
>>>>>> card. So in order to prevent it some credits are reserved for control
>>>>>> path.
>>>>>>
>>>>>> Note that TXCTL_CREDITS can't be larger than the firmware default
>>>>>> credit update threshold 2; otherwise there will be a deadlock for
>>>>>> both
>>>>>> side waiting for each other.
>>>>>>
>>>>>> Signed-off-by: Amar Shankar <amsr@cypress.com>
>>>>>> Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com>
>>>>>> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
>>>> [...]
>>>>
>>>>> This patch causes a severe WiFi performance regression on BCM4329.
>>>>> Please fix or revert this patch, thanks in advance.
>>>>>
>>>>> Before this patch:
>>>>> - - - - - - - - - - - - - - - - - - - - - - - - -
>>>>> [ ID] Interval           Transfer     Bitrate         Retr
>>>>> [  5]   0.00-10.00  sec  17.2 MBytes  14.4 Mbits/sec   
>>>>> 0             sender
>>>>> [  5]   0.00-10.04  sec  16.9 MBytes  14.1 Mbits/sec
>>>>> receiver
>>>>>
>>>>>
>>>>> After this patch:
>>>>> - - - - - - - - - - - - - - - - - - - - - - - - -
>>>>> [ ID] Interval           Transfer     Bitrate         Retr
>>>>> [  5]   0.00-10.00  sec  1.05 MBytes   881 Kbits/sec   
>>>>> 3             sender
>>>>> [  5]   0.00-14.01  sec   959 KBytes   561 Kbits/sec
>>>>> receiver
>>>> Can someone please send a revert patch (with the explanation above)
>>>> if a
>>>> fix is not quickly found? The commit id is:
>>>>
>>>> commit b41c232d33666191a1db11befc0f040fcbe664e9
>>>> Author:     Amar Shankar <amsr@cypress.com>
>>>> AuthorDate: Wed Jun 10 10:21:03 2020 -0500
>>>> Commit:     Kalle Valo <kvalo@codeaurora.org>
>>>> CommitDate: Tue Jul 14 12:46:43 2020 +0300
>>>>
>>>>      brcmfmac: reserve 2 credits for host tx control path
>>>>
>>> Hello Kalle,
>>>
>>> I'll send the revert if nobody will stand up to address the problem in a
>>> two weeks, thanks.
>> Thanks. Then I should be able to get the revert to v5.9 so that the
>> release won't be broken. (v5.8 is unaffected)
> 
> Dmitry/Kalle,
> 
> We'll take a look and revert/fix it in a few days.

Good to know, thank you.

Re: [PATCH V2 3/6] brcmfmac: reserve 2 credits for host tx control path

[Wright Feng]

Dmitry Osipenko 於 8/5/2020 10:01 PM 寫道:
> 05.08.2020 11:24, Chi-Hsien Lin пишет:
>>
>> On 8/5/2020 1:22 AM, Kalle Valo wrote:
>>> Dmitry Osipenko <digetx@gmail.com> writes:
>>>
>>>> 04.08.2020 14:08, Kalle Valo пишет:
>>>>> Dmitry Osipenko <digetx@gmail.com> writes:
>>>>>
>>>>>> 10.06.2020 18:21, Chi-Hsien Lin пишет:
>>>>>>> From: Amar Shankar <amsr@cypress.com>
>>>>>>>
>>>>>>> It is observed that sometimes when sdiod is low in tx credits in low
>>>>>>> rssi scenarios, the data path consumes all sdiod rx all credits and
>>>>>>> there is no sdiod rx credit available for control path causing host
>>>>>>> and card to go out of sync resulting in link loss between host and
>>>>>>> card. So in order to prevent it some credits are reserved for control
>>>>>>> path.
>>>>>>>
>>>>>>> Note that TXCTL_CREDITS can't be larger than the firmware default
>>>>>>> credit update threshold 2; otherwise there will be a deadlock for
>>>>>>> both
>>>>>>> side waiting for each other.
>>>>>>>
>>>>>>> Signed-off-by: Amar Shankar <amsr@cypress.com>
>>>>>>> Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com>
>>>>>>> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
>>>>> [...]
>>>>>
>>>>>> This patch causes a severe WiFi performance regression on BCM4329.
>>>>>> Please fix or revert this patch, thanks in advance.
>>>>>>
>>>>>> Before this patch:
>>>>>> - - - - - - - - - - - - - - - - - - - - - - - - -
>>>>>> [ ID] Interval           Transfer     Bitrate         Retr
>>>>>> [  5]   0.00-10.00  sec  17.2 MBytes  14.4 Mbits/sec
>>>>>> 0             sender
>>>>>> [  5]   0.00-10.04  sec  16.9 MBytes  14.1 Mbits/sec
>>>>>> receiver
>>>>>>
>>>>>>
>>>>>> After this patch:
>>>>>> - - - - - - - - - - - - - - - - - - - - - - - - -
>>>>>> [ ID] Interval           Transfer     Bitrate         Retr
>>>>>> [  5]   0.00-10.00  sec  1.05 MBytes   881 Kbits/sec
>>>>>> 3             sender
>>>>>> [  5]   0.00-14.01  sec   959 KBytes   561 Kbits/sec
>>>>>> receiver
>>>>> Can someone please send a revert patch (with the explanation above)
>>>>> if a
>>>>> fix is not quickly found? The commit id is:
>>>>>
>>>>> commit b41c232d33666191a1db11befc0f040fcbe664e9
>>>>> Author:     Amar Shankar <amsr@cypress.com>
>>>>> AuthorDate: Wed Jun 10 10:21:03 2020 -0500
>>>>> Commit:     Kalle Valo <kvalo@codeaurora.org>
>>>>> CommitDate: Tue Jul 14 12:46:43 2020 +0300
>>>>>
>>>>>       brcmfmac: reserve 2 credits for host tx control path
>>>>>
>>>> Hello Kalle,
>>>>
>>>> I'll send the revert if nobody will stand up to address the problem in a
>>>> two weeks, thanks.
>>> Thanks. Then I should be able to get the revert to v5.9 so that the
>>> release won't be broken. (v5.8 is unaffected)
>>
>> Dmitry/Kalle,
>>
>> We'll take a look and revert/fix it in a few days.
> 
> Good to know, thank you.
> 
Hi Dmitry,
We have a fix for this issue. But we got the different test result 
numbers from yours, so would you help us verify the same with
following patch in your setup?
With your confirmation, I can make sure we see the same issue.

---
  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 9 +++++++--
  1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index e8712ad3ac45..ea07bb1bfe27 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -664,8 +664,13 @@ static void pkt_align(struct sk_buff *p, int len, 
int align)
  /* To check if there's window offered */
  static bool data_ok(struct brcmf_sdio *bus)
  {
-	/* Reserve TXCTL_CREDITS credits for txctl */
-	return (bus->tx_max - bus->tx_seq) > TXCTL_CREDITS &&
+	u8 tx_rsv = 0;
+
+	/* Reserve TXCTL_CREDITS credits for txctl when it is ready to send */
+	if (bus->ctrl_frame_stat)
+		tx_rsv = TXCTL_CREDITS;
+
+	return (bus->tx_max - bus->tx_seq) > tx_rsv &&
  	       ((bus->tx_max - bus->tx_seq) & 0x80) == 0;
  }

-- 

Here is our 4329 TX throughput result.

Without the fix:
Interval           Transfer     Bandwidth       Retr
0.00-10.00  sec  36.4 MBytes  30.6 Mbits/sec  receiver
With the fix:
Interval           Transfer     Bandwidth       Retr
0.00-10.00  sec  47.9 MBytes  40.2 Mbits/sec  receiver

Re: [PATCH V2 3/6] brcmfmac: reserve 2 credits for host tx control path

[Dmitry Osipenko]

11.08.2020 11:35, Wright Feng пишет:
..
> Hi Dmitry,
> We have a fix for this issue. But we got the different test result
> numbers from yours, so would you help us verify the same with
> following patch in your setup?
> With your confirmation, I can make sure we see the same issue.
> 
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index e8712ad3ac45..ea07bb1bfe27 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -664,8 +664,13 @@ static void pkt_align(struct sk_buff *p, int len,
> int align)
>  /* To check if there's window offered */
>  static bool data_ok(struct brcmf_sdio *bus)
>  {
> -    /* Reserve TXCTL_CREDITS credits for txctl */
> -    return (bus->tx_max - bus->tx_seq) > TXCTL_CREDITS &&
> +    u8 tx_rsv = 0;
> +
> +    /* Reserve TXCTL_CREDITS credits for txctl when it is ready to send */
> +    if (bus->ctrl_frame_stat)
> +        tx_rsv = TXCTL_CREDITS;
> +
> +    return (bus->tx_max - bus->tx_seq) > tx_rsv &&
>             ((bus->tx_max - bus->tx_seq) & 0x80) == 0;
>  }

Hello, Wright!

I tried this patch and it doesn't fix the problem.

Could you please tell what firmware you're using?

I'm using stock firmware that comes from the linux-firmware package and
this NVRAM [1].

[1]
https://github.com/grate-driver/linux-firmware/blob/master/brcm/brcmfmac4329-sdio.acer%2Cpicasso.txt

 brcmfmac: brcmf_fw_alloc_request: using brcm/brcmfmac4329-sdio for chip
BCM4329/3
 brcmfmac: brcmf_c_process_clm_blob: no clm_blob available (err=-2),
device may have limited channels available
 brcmfmac: brcmf_c_preinit_dcmds: Firmware: BCM4329/3 wl0: Sep  2 2011
14:48:19 version 4.220.48

It also interesting that you're getting 40Mbit/s. I never managed to get
that speed on 4329 using upstream brcmfmac driver, but was getting it
using downstream BCMDHD driver. At some point I even tried to figure out
what's the difference which makes the upstream driver to have
significantly lower throughput than downstream and had older BCMDHD
ported to a recent upstream kernel, unfortunately I couldn't spot
anything obvious. Having BCMDHD and brcmfmac back-to-back on the same
hardware, the same kernel and using same firmware (IIRC), the BCMDHD was
more than x2 faster. I also remember that I had to enforce HT mode on AP
in order to get speeds over 15Mbit/s using BCMDHD, which made me to
conclude that upstream brcmfmac driver just doesn't support that HT mode
for 4329, but now I'm seeing yours iperf results and it makes me wonder..

Re: [PATCH V2 3/6] brcmfmac: reserve 2 credits for host tx control path

[Wright Feng]

Dmitry Osipenko 於 8/11/2020 10:54 PM 寫道:
> 11.08.2020 11:35, Wright Feng пишет:
> ..
>> Hi Dmitry,
>> We have a fix for this issue. But we got the different test result
>> numbers from yours, so would you help us verify the same with
>> following patch in your setup?
>> With your confirmation, I can make sure we see the same issue.
>>
>> ---
>>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> index e8712ad3ac45..ea07bb1bfe27 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> @@ -664,8 +664,13 @@ static void pkt_align(struct sk_buff *p, int len,
>> int align)
>>   /* To check if there's window offered */
>>   static bool data_ok(struct brcmf_sdio *bus)
>>   {
>> -    /* Reserve TXCTL_CREDITS credits for txctl */
>> -    return (bus->tx_max - bus->tx_seq) > TXCTL_CREDITS &&
>> +    u8 tx_rsv = 0;
>> +
>> +    /* Reserve TXCTL_CREDITS credits for txctl when it is ready to send */
>> +    if (bus->ctrl_frame_stat)
>> +        tx_rsv = TXCTL_CREDITS;
>> +
>> +    return (bus->tx_max - bus->tx_seq) > tx_rsv &&
>>              ((bus->tx_max - bus->tx_seq) & 0x80) == 0;
>>   }
> 
> Hello, Wright!
> 
> I tried this patch and it doesn't fix the problem.
> 
> Could you please tell what firmware you're using?
> 
> I'm using stock firmware that comes from the linux-firmware package and
> this NVRAM [1].
> 
> [1]
> https://github.com/grate-driver/linux-firmware/blob/master/brcm/brcmfmac4329-sdio.acer%2Cpicasso.txt
> 
>   brcmfmac: brcmf_fw_alloc_request: using brcm/brcmfmac4329-sdio for chip
> BCM4329/3
>   brcmfmac: brcmf_c_process_clm_blob: no clm_blob available (err=-2),
> device may have limited channels available
>   brcmfmac: brcmf_c_preinit_dcmds: Firmware: BCM4329/3 wl0: Sep  2 2011
> 14:48:19 version 4.220.48
> 
> It also interesting that you're getting 40Mbit/s. I never managed to get
> that speed on 4329 using upstream brcmfmac driver, but was getting it
> using downstream BCMDHD driver. At some point I even tried to figure out
> what's the difference which makes the upstream driver to have
> significantly lower throughput than downstream and had older BCMDHD
> ported to a recent upstream kernel, unfortunately I couldn't spot
> anything obvious. Having BCMDHD and brcmfmac back-to-back on the same
> hardware, the same kernel and using same firmware (IIRC), the BCMDHD was
> more than x2 faster. I also remember that I had to enforce HT mode on AP
> in order to get speeds over 15Mbit/s using BCMDHD, which made me to
> conclude that upstream brcmfmac driver just doesn't support that HT mode
> for 4329, but now I'm seeing yours iperf results and it makes me wonder..
> 
Hi Dmitry,

The last time the drivers I used is v5.4 and I was not able to see low 
throughput issue as you saw.
At this time, I changed the base to tag wt-2020-0727 and I am able to 
reproduce the issue as you did.
TX throughput with wt-2020-07-27 FMAC is 190 Kbits/sec


The root cause should be that tx_max and tx_seq are circle positive 
numbers, it should not use ">" to check if it still exists available TX 
credit.
With the solution below, I am able to get the normal throughput.
TX throughput with wt-2020-07-27+patch FMAC is 40.0 Mbits/sec

Regarding another case about 40Mbit/s, I am using 4329b0(4329/2) chip to 
verify the throughput because we are not able to find 4329b1(4329/3) 
which is very old product around 10 years ago.
The firmware I am using is the same version but the build is for 4329b0. 
(private internal build - 4.220.48). My host platform is x86_64 with 4 
cores on Linux kernel 4.12. I will try your NVRAM when I have time to 
see if I can find anything.

---
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index e8712ad..50c8107 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -664,9 +664,14 @@ static void pkt_align(struct sk_buff *p, int len, 
int align)
  /* To check if there's window offered */
  static bool data_ok(struct brcmf_sdio *bus)
  {
-	/* Reserve TXCTL_CREDITS credits for txctl */
-	return (bus->tx_max - bus->tx_seq) > TXCTL_CREDITS &&
-	       ((bus->tx_max - bus->tx_seq) & 0x80) == 0;
+	u8 tx_rsv = 0;
+
+	/* Reserve TXCTL_CREDITS credits for txctl when it is ready to send */
+	if (bus->ctrl_frame_stat)
+		tx_rsv = TXCTL_CREDITS;
+
+	return (bus->tx_max - bus->tx_seq - tx_rsv) != 0 &&
+	       ((bus->tx_max - bus->tx_seq - tx_rsv) & 0x80) == 0;
  }

  /* To check if there's window offered */
---

Re: [PATCH V2 3/6] brcmfmac: reserve 2 credits for host tx control path

[Dmitry Osipenko]

12.08.2020 07:03, Wright Feng пишет:
...
> Hi Dmitry,
> 
> The last time the drivers I used is v5.4 and I was not able to see low
> throughput issue as you saw.
> At this time, I changed the base to tag wt-2020-0727 and I am able to
> reproduce the issue as you did.
> TX throughput with wt-2020-07-27 FMAC is 190 Kbits/sec
> 
> 
> The root cause should be that tx_max and tx_seq are circle positive
> numbers, it should not use ">" to check if it still exists available TX
> credit.
> With the solution below, I am able to get the normal throughput.
> TX throughput with wt-2020-07-27+patch FMAC is 40.0 Mbits/sec
> 
> Regarding another case about 40Mbit/s, I am using 4329b0(4329/2) chip to
> verify the throughput because we are not able to find 4329b1(4329/3)
> which is very old product around 10 years ago.
> The firmware I am using is the same version but the build is for 4329b0.
> (private internal build - 4.220.48). My host platform is x86_64 with 4
> cores on Linux kernel 4.12. I will try your NVRAM when I have time to
> see if I can find anything.
> 
> ---
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index e8712ad..50c8107 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -664,9 +664,14 @@ static void pkt_align(struct sk_buff *p, int len,
> int align)
>  /* To check if there's window offered */
>  static bool data_ok(struct brcmf_sdio *bus)
>  {
> -    /* Reserve TXCTL_CREDITS credits for txctl */
> -    return (bus->tx_max - bus->tx_seq) > TXCTL_CREDITS &&
> -           ((bus->tx_max - bus->tx_seq) & 0x80) == 0;
> +    u8 tx_rsv = 0;
> +
> +    /* Reserve TXCTL_CREDITS credits for txctl when it is ready to send */
> +    if (bus->ctrl_frame_stat)
> +        tx_rsv = TXCTL_CREDITS;
> +
> +    return (bus->tx_max - bus->tx_seq - tx_rsv) != 0 &&
> +           ((bus->tx_max - bus->tx_seq - tx_rsv) & 0x80) == 0;
>  }
> 
>  /* To check if there's window offered */
> ---

Wright, thank you very much for the patch! It fixes the problem!

Tested-by: Dmitry Osipenko <digetx@gmail.com>

The 4329/3 is indeed an older chip, but it's also an "old" device (Acer
A500 tablet from 2011/12) that I'm using. Upstream v5.9 kernel just got
support for the A500. There are quite a lot of other older devices with
4329/3 in a wild that are still very usable if user can wipe off ancient
Android and put a modern Linux distro on them. Today that A500 tablet is
still rocking hard running a modern upstream kernel, opensource drivers
and KDE Plasma 5. The 15Mbit is a good enough speed for a lot of things,
but of course 40Mbit will be better. Would be great if you could try to
help with improving the speed :) Please feel free to contact me at any
time if you'll have patches to try!

Re: [PATCH V2 3/6] brcmfmac: reserve 2 credits for host tx control path

[Wright Feng]

Dmitry Osipenko 於 8/12/2020 10:22 PM 寫道:
> 12.08.2020 07:03, Wright Feng пишет:
> ...
>> Hi Dmitry,
>>
>> The last time the drivers I used is v5.4 and I was not able to see low
>> throughput issue as you saw.
>> At this time, I changed the base to tag wt-2020-0727 and I am able to
>> reproduce the issue as you did.
>> TX throughput with wt-2020-07-27 FMAC is 190 Kbits/sec
>>
>>
>> The root cause should be that tx_max and tx_seq are circle positive
>> numbers, it should not use ">" to check if it still exists available TX
>> credit.
>> With the solution below, I am able to get the normal throughput.
>> TX throughput with wt-2020-07-27+patch FMAC is 40.0 Mbits/sec
>>
>> Regarding another case about 40Mbit/s, I am using 4329b0(4329/2) chip to
>> verify the throughput because we are not able to find 4329b1(4329/3)
>> which is very old product around 10 years ago.
>> The firmware I am using is the same version but the build is for 4329b0.
>> (private internal build - 4.220.48). My host platform is x86_64 with 4
>> cores on Linux kernel 4.12. I will try your NVRAM when I have time to
>> see if I can find anything.
>>
>> ---
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> index e8712ad..50c8107 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> @@ -664,9 +664,14 @@ static void pkt_align(struct sk_buff *p, int len,
>> int align)
>>   /* To check if there's window offered */
>>   static bool data_ok(struct brcmf_sdio *bus)
>>   {
>> -    /* Reserve TXCTL_CREDITS credits for txctl */
>> -    return (bus->tx_max - bus->tx_seq) > TXCTL_CREDITS &&
>> -           ((bus->tx_max - bus->tx_seq) & 0x80) == 0;
>> +    u8 tx_rsv = 0;
>> +
>> +    /* Reserve TXCTL_CREDITS credits for txctl when it is ready to send */
>> +    if (bus->ctrl_frame_stat)
>> +        tx_rsv = TXCTL_CREDITS;
>> +
>> +    return (bus->tx_max - bus->tx_seq - tx_rsv) != 0 &&
>> +           ((bus->tx_max - bus->tx_seq - tx_rsv) & 0x80) == 0;
>>   }
>>
>>   /* To check if there's window offered */
>> ---
> 
> Wright, thank you very much for the patch! It fixes the problem!
> 
> Tested-by: Dmitry Osipenko <digetx@gmail.com>
> 
> The 4329/3 is indeed an older chip, but it's also an "old" device (Acer
> A500 tablet from 2011/12) that I'm using. Upstream v5.9 kernel just got
> support for the A500. There are quite a lot of other older devices with
> 4329/3 in a wild that are still very usable if user can wipe off ancient
> Android and put a modern Linux distro on them. Today that A500 tablet is
> still rocking hard running a modern upstream kernel, opensource drivers
> and KDE Plasma 5. The 15Mbit is a good enough speed for a lot of things,
> but of course 40Mbit will be better. Would be great if you could try to
> help with improving the speed :) Please feel free to contact me at any
> time if you'll have patches to try!
> 
Dmitry,
Thanks for the verification. The patch will be submitted to upstream 
within two days.
And with regards to low THP problem on Acer A500 tablet, I will create 
another mail thread to discuss with you.

Re: [PATCH V2 3/6] brcmfmac: reserve 2 credits for host tx control path

[Dmitry Osipenko]

13.08.2020 06:03, Wright Feng пишет:
..
>> Wright, thank you very much for the patch! It fixes the problem!
>>
>> Tested-by: Dmitry Osipenko <digetx@gmail.com>
>>
>> The 4329/3 is indeed an older chip, but it's also an "old" device (Acer
>> A500 tablet from 2011/12) that I'm using. Upstream v5.9 kernel just got
>> support for the A500. There are quite a lot of other older devices with
>> 4329/3 in a wild that are still very usable if user can wipe off ancient
>> Android and put a modern Linux distro on them. Today that A500 tablet is
>> still rocking hard running a modern upstream kernel, opensource drivers
>> and KDE Plasma 5. The 15Mbit is a good enough speed for a lot of things,
>> but of course 40Mbit will be better. Would be great if you could try to
>> help with improving the speed :) Please feel free to contact me at any
>> time if you'll have patches to try!
>>
> Dmitry,
> Thanks for the verification. The patch will be submitted to upstream
> within two days.
> And with regards to low THP problem on Acer A500 tablet, I will create
> another mail thread to discuss with you.

Thank you!

[PATCH V2 4/6] brcmfmac: update tx status flags to sync with firmware

[Chi-Hsien Lin]

From: Chung-Hsien Hsu <stanley.hsu@cypress.com>

There is a mismatch of tx status flag values between host and firmware.
It makes the host mistake the flags and have incorrect behavior of credit
returns. So update the flags to sync with the firmware ones.

Signed-off-by: Chung-Hsien Hsu <stanley.hsu@cypress.com>
Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
---
 .../wireless/broadcom/brcm80211/brcmfmac/fwsignal.c    | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
index d7c36af4b566..ce8c6ceafc58 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
@@ -323,6 +323,10 @@ struct brcmf_skbuff_cb {
  *	firmware suppress the packet as device is already in PS mode.
  * @BRCMF_FWS_TXSTATUS_FW_TOSSED:
  *	firmware tossed the packet.
+ * @BRCMF_FWS_TXSTATUS_FW_DISCARD_NOACK:
+ *	firmware tossed the packet after retries.
+ * @BRCMF_FWS_TXSTATUS_FW_SUPPRESS_ACKED:
+ *	firmware wrongly reported suppressed previously, now fixing to acked.
  * @BRCMF_FWS_TXSTATUS_HOST_TOSSED:
  *	host tossed the packet.
  */
@@ -331,6 +335,8 @@ enum brcmf_fws_txstatus {
 	BRCMF_FWS_TXSTATUS_CORE_SUPPRESS,
 	BRCMF_FWS_TXSTATUS_FW_PS_SUPPRESS,
 	BRCMF_FWS_TXSTATUS_FW_TOSSED,
+	BRCMF_FWS_TXSTATUS_FW_DISCARD_NOACK,
+	BRCMF_FWS_TXSTATUS_FW_SUPPRESS_ACKED,
 	BRCMF_FWS_TXSTATUS_HOST_TOSSED
 };
 
@@ -1455,6 +1461,10 @@ brcmf_fws_txs_process(struct brcmf_fws_info *fws, u8 flags, u32 hslot,
 		remove_from_hanger = false;
 	} else if (flags == BRCMF_FWS_TXSTATUS_FW_TOSSED)
 		fws->stats.txs_tossed += compcnt;
+	else if (flags == BRCMF_FWS_TXSTATUS_FW_DISCARD_NOACK)
+		fws->stats.txs_discard += compcnt;
+	else if (flags == BRCMF_FWS_TXSTATUS_FW_SUPPRESS_ACKED)
+		fws->stats.txs_discard += compcnt;
 	else if (flags == BRCMF_FWS_TXSTATUS_HOST_TOSSED)
 		fws->stats.txs_host_tossed += compcnt;
 	else
-- 
2.25.0

[PATCH V2 5/6] brcmfmac: fix throughput zero stalls on PM 1 mode due to credit map

[Chi-Hsien Lin]

From: Double Lo <double.lo@cypress.com>

This patch move the credit map setting to right place to avoid
brcmf_fws_return_credits() return without setting the credit map.
It fix the thoughput zero stalls issue in softAP mode when STA
using PM 1 mode.

Signed-off-by: Double Lo <double.lo@cypress.com>
Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
index ce8c6ceafc58..ab4526223a93 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
@@ -1193,6 +1193,8 @@ static void brcmf_fws_return_credits(struct brcmf_fws_info *fws,
 	if (!credits)
 		return;
 
+	fws->fifo_credit_map |= 1 << fifo;
+
 	if (fifo > BRCMF_FWS_FIFO_AC_BK &&
 	    fifo <= BRCMF_FWS_FIFO_AC_VO) {
 		for (lender_ac = BRCMF_FWS_FIFO_AC_VO; lender_ac >= 0;
@@ -1216,7 +1218,6 @@ static void brcmf_fws_return_credits(struct brcmf_fws_info *fws,
 
 	if (credits) {
 		fws->fifo_credit[fifo] += credits;
-		fws->fifo_credit_map |= 1 << fifo;
 	}
 
 	if (fws->fifo_credit[fifo] > fws->init_fifo_credit[fifo])
-- 
2.25.0

[PATCH V2 6/6] brcmfmac: initialize the requested dwell time

[Chi-Hsien Lin]

From: Joseph Chuang <joseph.chuang@cypress.com>

Commit 9c29da3f4e7e
("brcmfmac: Fix P2P Group Formation failure via Go-neg method") did not
initialize requested_dwell properly, resulting in an always-false dwell
time overflow check. Fix it by setting the correct requested_dwell
value.

Fixes: 4905432b28b7 ("brcmfmac: Fix P2P Group Formation failure via Go-neg method")
Signed-off-by: Joseph Chuang <joseph.chuang@cypress.com>
Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +-
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c      | 5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index a757abd7a599..bddd23240609 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -5129,7 +5129,7 @@ brcmf_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
 					      &freq);
 		chan_nr = ieee80211_frequency_to_channel(freq);
 		af_params->channel = cpu_to_le32(chan_nr);
-
+		af_params->dwell_time = cpu_to_le32(params->wait);
 		memcpy(action_frame->data, &buf[DOT11_MGMT_HDR_LEN],
 		       le16_to_cpu(action_frame->len));
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
index d2795dc17c46..debd887e159e 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
@@ -1700,7 +1700,7 @@ static s32 brcmf_p2p_pub_af_tx(struct brcmf_cfg80211_info *cfg,
 	return err;
 }
 
-static bool brcmf_p2p_check_dwell_overflow(s32 requested_dwell,
+static bool brcmf_p2p_check_dwell_overflow(u32 requested_dwell,
 					   unsigned long dwell_jiffies)
 {
 	if ((requested_dwell & CUSTOM_RETRY_MASK) &&
@@ -1738,8 +1738,7 @@ bool brcmf_p2p_send_action_frame(struct brcmf_cfg80211_info *cfg,
 	unsigned long dwell_jiffies = 0;
 	bool dwell_overflow = false;
 
-	s32 requested_dwell = af_params->dwell_time;
-
+	u32 requested_dwell = le32_to_cpu(af_params->dwell_time);
 	action_frame = &af_params->action_frame;
 	action_frame_len = le16_to_cpu(action_frame->len);
 
-- 
2.25.0

Re: [PATCH V2 6/6] brcmfmac: initialize the requested dwell time

[Chi-Hsien Lin]

On 06/10/2020 11:21, Chi-Hsien Lin wrote:
> From: Joseph Chuang <joseph.chuang@cypress.com>
> 
> Commit 9c29da3f4e7e
> ("brcmfmac: Fix P2P Group Formation failure via Go-neg method") did not
> initialize requested_dwell properly, resulting in an always-false dwell
> time overflow check. Fix it by setting the correct requested_dwell
> value.
> 
> Fixes: 4905432b28b7 ("brcmfmac: Fix P2P Group Formation failure via Go-neg method")
> Signed-off-by: Joseph Chuang <joseph.chuang@cypress.com>
> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>

Kalle,

This issue is reported by kernel test robot. Please kindly add below tag 
during commit.

Reported-by: kernel test robot <lkp@intel.com>



> ---
>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +-
>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c      | 5 ++---
>   2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index a757abd7a599..bddd23240609 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -5129,7 +5129,7 @@ brcmf_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
>   					      &freq);
>   		chan_nr = ieee80211_frequency_to_channel(freq);
>   		af_params->channel = cpu_to_le32(chan_nr);
> -
> +		af_params->dwell_time = cpu_to_le32(params->wait);
>   		memcpy(action_frame->data, &buf[DOT11_MGMT_HDR_LEN],
>   		       le16_to_cpu(action_frame->len));
>   
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
> index d2795dc17c46..debd887e159e 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
> @@ -1700,7 +1700,7 @@ static s32 brcmf_p2p_pub_af_tx(struct brcmf_cfg80211_info *cfg,
>   	return err;
>   }
>   
> -static bool brcmf_p2p_check_dwell_overflow(s32 requested_dwell,
> +static bool brcmf_p2p_check_dwell_overflow(u32 requested_dwell,
>   					   unsigned long dwell_jiffies)
>   {
>   	if ((requested_dwell & CUSTOM_RETRY_MASK) &&
> @@ -1738,8 +1738,7 @@ bool brcmf_p2p_send_action_frame(struct brcmf_cfg80211_info *cfg,
>   	unsigned long dwell_jiffies = 0;
>   	bool dwell_overflow = false;
>   
> -	s32 requested_dwell = af_params->dwell_time;
> -
> +	u32 requested_dwell = le32_to_cpu(af_params->dwell_time);
>   	action_frame = &af_params->action_frame;
>   	action_frame_len = le16_to_cpu(action_frame->len);
>   
> 

Re: [PATCH V2 6/6] brcmfmac: initialize the requested dwell time

[Kalle Valo]

Chi-Hsien Lin <chi-hsien.lin@cypress.com> wrote:

> From: Joseph Chuang <joseph.chuang@cypress.com>
> 
> Commit 9c29da3f4e7e
> ("brcmfmac: Fix P2P Group Formation failure via Go-neg method") did not
> initialize requested_dwell properly, resulting in an always-false dwell
> time overflow check. Fix it by setting the correct requested_dwell
> value.
> 
> Fixes: 4905432b28b7 ("brcmfmac: Fix P2P Group Formation failure via Go-neg method")

The commit id is wrong, this should be:

Fixes: 9c29da3f4e7e ("brcmfmac: Fix P2P Group Formation failure via Go-neg method")

-- 
https://patchwork.kernel.org/patch/11598109/

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