[PATCH 0/3] brcmfmac: throughput enhancement for SDIO and flow control mode

[PATCH 0/3] brcmfmac: throughput enhancement for SDIO and flow control mode

[Wright Feng]

These are for throughput enhancement with SDIO bus or with flow control
mode enabled, and also introuce a new module parameter to enhance TX
throughput as well.

Wright Feng (3):
  brcmfmac: calling skb_orphan before sending skb to SDIO bus
  brcmfmac: add credit numbers updating support
  brcmfmac: make firmware frameburst mode a module parameter

 .../broadcom/brcm80211/brcmfmac/cfg80211.c         |  7 +++++++
 .../wireless/broadcom/brcm80211/brcmfmac/common.c  |  5 +++++
 .../wireless/broadcom/brcm80211/brcmfmac/common.h  |  2 ++
 .../wireless/broadcom/brcm80211/brcmfmac/fwil.h    |  1 +
 .../broadcom/brcm80211/brcmfmac/fwsignal.c         | 23 ++++++++++++++--------
 .../wireless/broadcom/brcm80211/brcmfmac/sdio.c    |  1 +
 6 files changed, 31 insertions(+), 8 deletions(-)

-- 
1.9.1

[PATCH 1/3] brcmfmac: calling skb_orphan before sending skb to SDIO bus

[Wright Feng]

Linux 3.6 introduces TSQ which has a per socket threshold for TCP Tx
packets to reduce latency. In fcmode 1 and fcmode 2, host driver enqueues
skb in hanger and TCP doesn't push new skb until host frees the skb when
receiving fwstatus event. So using skb_orphan before sending skb to bus
will make the skb removing the ownership of socket. With this patch, we
are able to get better throughput in fcmode 1 and fcmode 2.

Tested 43455 TCP throughput in 20 MHz bandwidth with / without this patch.
fcmode 0: 59.5 / 59.6 (mbps)
fcmode 1: 59.3 / 23.4 (mbps)
fcmode 2: 59.6 / 21.5 (mbps)

Signed-off-by: Wright Feng <wright.feng@cypress.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index b2e1ab5..519b25d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -2298,6 +2298,7 @@ static uint brcmf_sdio_sendfromq(struct brcmf_sdio *bus, uint maxframes)
 					      &prec_out);
 			if (pkt == NULL)
 				break;
+			skb_orphan(pkt);
 			__skb_queue_tail(&pktq, pkt);
 		}
 		spin_unlock_bh(&bus->txq_lock);
-- 
1.9.1

[PATCH 2/3] brcmfmac: add credit numbers updating support

[Wright Feng]

The credit numbers are static and tunable per chip in firmware side.
However the credit number may be changed that is based on packet pool
length and will send BRCMF_E_FIFO_CREDIT_MAP event to notify host driver
updates the credit numbers during interface up.
The purpose of this patch is making host driver has ability of updating
the credit numbers when receiving the BRCMF_E_FIFO_CREDIT_MAP event.

Signed-off-by: Wright Feng <wright.feng@cypress.com>
---
 .../broadcom/brcm80211/brcmfmac/fwsignal.c         | 23 ++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
index f3cbf78..e0910c5 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
@@ -511,6 +511,7 @@ struct brcmf_fws_info {
 	struct work_struct fws_dequeue_work;
 	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 deq_node_pos[BRCMF_FWS_FIFO_COUNT];
 	u32 fifo_credit_map;
@@ -1237,6 +1238,9 @@ static void brcmf_fws_return_credits(struct brcmf_fws_info *fws,
 	}
 
 	fws->fifo_credit[fifo] += credits;
+	if (fws->fifo_credit[fifo] > fws->init_fifo_credit[fifo])
+		fws->fifo_credit[fifo] = fws->init_fifo_credit[fifo];
+
 }
 
 static void brcmf_fws_schedule_deq(struct brcmf_fws_info *fws)
@@ -1595,19 +1599,21 @@ static int brcmf_fws_notify_credit_map(struct brcmf_if *ifp,
 		brcmf_err("event payload too small (%d)\n", e->datalen);
 		return -EINVAL;
 	}
-	if (fws->creditmap_received)
-		return 0;
 
 	fws->creditmap_received = true;
 
 	brcmf_dbg(TRACE, "enter: credits %pM\n", credits);
 	brcmf_fws_lock(fws);
 	for (i = 0; i < ARRAY_SIZE(fws->fifo_credit); i++) {
-		if (*credits)
+		fws->fifo_credit[i] += credits[i] - fws->init_fifo_credit[i];
+		fws->init_fifo_credit[i] = credits[i];
+		if (fws->fifo_credit[i] > 0)
 			fws->fifo_credit_map |= 1 << i;
 		else
 			fws->fifo_credit_map &= ~(1 << i);
-		fws->fifo_credit[i] = *credits++;
+		if (fws->fifo_credit[i] < 0)
+			brcmf_err("fifo_credit[%d] value is negative(%d)\n",
+				  i, fws->fifo_credit[i]);
 	}
 	brcmf_fws_schedule_deq(fws);
 	brcmf_fws_unlock(fws);
@@ -2013,7 +2019,7 @@ static int brcmf_fws_borrow_credit(struct brcmf_fws_info *fws)
 	}
 
 	for (lender_ac = 0; lender_ac <= BRCMF_FWS_FIFO_AC_VO; lender_ac++) {
-		if (fws->fifo_credit[lender_ac]) {
+		if (fws->fifo_credit[lender_ac] > 0) {
 			fws->credits_borrowed[lender_ac]++;
 			fws->fifo_credit[lender_ac]--;
 			if (fws->fifo_credit[lender_ac] == 0)
@@ -2210,8 +2216,9 @@ static void brcmf_fws_dequeue_worker(struct work_struct *worker)
 			}
 			continue;
 		}
-		while ((fws->fifo_credit[fifo]) || ((!fws->bcmc_credit_check) &&
-		       (fifo == BRCMF_FWS_FIFO_BCMC))) {
+		while ((fws->fifo_credit[fifo] > 0) ||
+		       ((!fws->bcmc_credit_check) &&
+			(fifo == BRCMF_FWS_FIFO_BCMC))) {
 			skb = brcmf_fws_deq(fws, fifo);
 			if (!skb)
 				break;
@@ -2222,7 +2229,7 @@ static void brcmf_fws_dequeue_worker(struct work_struct *worker)
 				break;
 		}
 		if ((fifo == BRCMF_FWS_FIFO_AC_BE) &&
-		    (fws->fifo_credit[fifo] == 0) &&
+		    (fws->fifo_credit[fifo] <= 0) &&
 		    (!fws->bus_flow_blocked)) {
 			while (brcmf_fws_borrow_credit(fws) == 0) {
 				skb = brcmf_fws_deq(fws, fifo);
-- 
1.9.1

[PATCH 3/3] brcmfmac: make firmware frameburst mode a module parameter

[Wright Feng]

This patch is for adding a new module parameter "frameburst".
With setting "frameburst=1" in module parameters, firmware frameburst mode
will be enabled. The feature can enable per-packet framebursting in
firmware side and get higher TX throughput in High Throughput(HT) mode.

Signed-off-by: Wright Feng <wright.feng@cypress.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 7 +++++++
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c   | 5 +++++
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h   | 2 ++
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.h     | 1 +
 4 files changed, 15 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 230a378..4a05d3f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -6638,6 +6638,13 @@ static s32 brcmf_config_dongle(struct brcmf_cfg80211_info *cfg)
 
 	brcmf_configure_arp_nd_offload(ifp, true);
 
+	if (ifp->drvr->settings->frameburst) {
+		err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_FAKEFRAG, 1);
+			if (err)
+				brcmf_info("setting frameburst mode failed\n");
+		brcmf_dbg(INFO, "frameburst mode enabled\n");
+	}
+
 	cfg->dongle_up = true;
 default_conf_out:
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index 94044a7..0ad4c31 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -78,6 +78,10 @@
 module_param_named(iapp, brcmf_iapp_enable, int, 0);
 MODULE_PARM_DESC(iapp, "Enable partial support for the obsoleted Inter-Access Point Protocol");
 
+static int brcmf_frameburst;
+module_param_named(frameburst, brcmf_frameburst, int, 0);
+MODULE_PARM_DESC(frameburst, "Enable firmware frameburst feature");
+
 #ifdef DEBUG
 /* always succeed brcmf_bus_started() */
 static int brcmf_ignore_probe_fail;
@@ -419,6 +423,7 @@ struct brcmf_mp_device *brcmf_get_module_param(struct device *dev,
 	settings->fcmode = brcmf_fcmode;
 	settings->roamoff = !!brcmf_roamoff;
 	settings->iapp = !!brcmf_iapp_enable;
+	settings->frameburst = !!brcmf_frameburst;
 #ifdef DEBUG
 	settings->ignore_probe_fail = !!brcmf_ignore_probe_fail;
 #endif
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h
index a34642c..b919752 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h
@@ -47,6 +47,7 @@ struct brcmf_mp_global_t {
  * @feature_disable: Feature_disable bitmask.
  * @fcmode: FWS flow control.
  * @roamoff: Firmware roaming off?
+ * @frameburst: Firmware frame burst mode.
  * @ignore_probe_fail: Ignore probe failure.
  * @country_codes: If available, pointer to struct for translating country codes
  * @bus: Bus specific platform data. Only SDIO at the mmoment.
@@ -57,6 +58,7 @@ struct brcmf_mp_device {
 	int		fcmode;
 	bool		roamoff;
 	bool		iapp;
+	bool		frameburst;
 	bool		ignore_probe_fail;
 	struct brcmfmac_pd_cc *country_codes;
 	union {
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.h
index 63b1287..b6b183b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.h
@@ -80,6 +80,7 @@
 #define BRCMF_C_SCB_DEAUTHENTICATE_FOR_REASON	201
 #define BRCMF_C_SET_ASSOC_PREFER		205
 #define BRCMF_C_GET_VALID_CHANNELS		217
+#define BRCMF_C_SET_FAKEFRAG			219
 #define BRCMF_C_GET_KEY_PRIMARY			235
 #define BRCMF_C_SET_KEY_PRIMARY			236
 #define BRCMF_C_SET_SCAN_PASSIVE_TIME		258
-- 
1.9.1

Re: [PATCH 1/3] brcmfmac: calling skb_orphan before sending skb to SDIO bus

[Franky Lin]

On Mon, Oct 29, 2018 at 3:27 AM Wright Feng <Wright.Feng@cypress.com> wrote:
>
> Linux 3.6 introduces TSQ which has a per socket threshold for TCP Tx
> packets to reduce latency. In fcmode 1 and fcmode 2, host driver enqueues
> skb in hanger and TCP doesn't push new skb until host frees the skb when
> receiving fwstatus event. So using skb_orphan before sending skb to bus
> will make the skb removing the ownership of socket. With this patch, we
> are able to get better throughput in fcmode 1 and fcmode 2.
>
> Tested 43455 TCP throughput in 20 MHz bandwidth with / without this patch.
> fcmode 0: 59.5 / 59.6 (mbps)
> fcmode 1: 59.3 / 23.4 (mbps)
> fcmode 2: 59.6 / 21.5 (mbps)
>
> Signed-off-by: Wright Feng <wright.feng@cypress.com>
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index b2e1ab5..519b25d 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -2298,6 +2298,7 @@ static uint brcmf_sdio_sendfromq(struct brcmf_sdio *bus, uint maxframes)
>                                               &prec_out);
>                         if (pkt == NULL)
>                                 break;
> +                       skb_orphan(pkt);

TSQ allows device driver to tweak for a deeper queue now. [1]. We
should use that instead of orphaning the packet before handing over to
firmware which would remove the bufferbloat protection by TSQ.

Thanks,
-Franky

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3a9b76fd0db9f

>                         __skb_queue_tail(&pktq, pkt);
>                 }
>                 spin_unlock_bh(&bus->txq_lock);
> --
> 1.9.1
>

Re: [PATCH 2/3] brcmfmac: add credit numbers updating support

[Arend van Spriel]

On 10/29/2018 11:27 AM, Wright Feng wrote:
> The credit numbers are static and tunable per chip in firmware side.
> However the credit number may be changed that is based on packet pool
> length and will send BRCMF_E_FIFO_CREDIT_MAP event to notify host driver
> updates the credit numbers during interface up.
> The purpose of this patch is making host driver has ability of updating
> the credit numbers when receiving the BRCMF_E_FIFO_CREDIT_MAP event.

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Wright Feng <wright.feng@cypress.com>
> ---
>  .../broadcom/brcm80211/brcmfmac/fwsignal.c         | 23 ++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
> index f3cbf78..e0910c5 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c

[...]

> @@ -1595,19 +1599,21 @@ static int brcmf_fws_notify_credit_map(struct brcmf_if *ifp,
>  		brcmf_err("event payload too small (%d)\n", e->datalen);
>  		return -EINVAL;
>  	}
> -	if (fws->creditmap_received)
> -		return 0;
>
>  	fws->creditmap_received = true;

I think the creditmap_received struct member is no longer needed.

>  	brcmf_dbg(TRACE, "enter: credits %pM\n", credits);
>  	brcmf_fws_lock(fws);
>  	for (i = 0; i < ARRAY_SIZE(fws->fifo_credit); i++) {
> -		if (*credits)
> +		fws->fifo_credit[i] += credits[i] - fws->init_fifo_credit[i];
> +		fws->init_fifo_credit[i] = credits[i];
> +		if (fws->fifo_credit[i] > 0)
>  			fws->fifo_credit_map |= 1 << i;
>  		else
>  			fws->fifo_credit_map &= ~(1 << i);
> -		fws->fifo_credit[i] = *credits++;
> +		if (fws->fifo_credit[i] < 0)
> +			brcmf_err("fifo_credit[%d] value is negative(%d)\n",
> +				  i, fws->fifo_credit[i]);

This looks like it should not happen so maybe warrants a WARN or WARN_ONCE?

>  	}
>  	brcmf_fws_schedule_deq(fws);
>  	brcmf_fws_unlock(fws);

Re: [PATCH 3/3] brcmfmac: make firmware frameburst mode a module parameter

[Arend van Spriel]

On 10/29/2018 11:27 AM, Wright Feng wrote:
> This patch is for adding a new module parameter "frameburst".
> With setting "frameburst=1" in module parameters, firmware frameburst mode
> will be enabled. The feature can enable per-packet framebursting in
> firmware side and get higher TX throughput in High Throughput(HT) mode.

I am not sure about every firmware image, but in recent branches here I 
see firmware enables frameburst by default. So not sure about the 
motivation to add this. You could also consider adding this to the 
nl80211 api. I am sure other vendors have similar features.

Regards,
Arend

> Signed-off-by: Wright Feng <wright.feng@cypress.com>
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 7 +++++++
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c   | 5 +++++
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h   | 2 ++
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.h     | 1 +
>  4 files changed, 15 insertions(+)

Re: [PATCH 1/3] brcmfmac: calling skb_orphan before sending skb to SDIO bus

[Wright Feng]

On 2018/10/30 上午 02:50, Franky Lin wrote:
> On Mon, Oct 29, 2018 at 3:27 AM Wright Feng <Wright.Feng@cypress.com> wrote:
>>
>> Linux 3.6 introduces TSQ which has a per socket threshold for TCP Tx
>> packets to reduce latency. In fcmode 1 and fcmode 2, host driver enqueues
>> skb in hanger and TCP doesn't push new skb until host frees the skb when
>> receiving fwstatus event. So using skb_orphan before sending skb to bus
>> will make the skb removing the ownership of socket. With this patch, we
>> are able to get better throughput in fcmode 1 and fcmode 2.
>>
>> Tested 43455 TCP throughput in 20 MHz bandwidth with / without this patch.
>> fcmode 0: 59.5 / 59.6 (mbps)
>> fcmode 1: 59.3 / 23.4 (mbps)
>> fcmode 2: 59.6 / 21.5 (mbps)
>>
>> Signed-off-by: Wright Feng <wright.feng@cypress.com>
>> ---
>>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> index b2e1ab5..519b25d 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> @@ -2298,6 +2298,7 @@ static uint brcmf_sdio_sendfromq(struct brcmf_sdio *bus, uint maxframes)
>>                                                &prec_out);
>>                          if (pkt == NULL)
>>                                  break;
>> +                       skb_orphan(pkt);
> 
> TSQ allows device driver to tweak for a deeper queue now. [1]. We
> should use that instead of orphaning the packet before handing over to
> firmware which would remove the bufferbloat protection by TSQ.
> 
> Thanks,
> -Franky
> 
Since the aggregation problem has been fixed by setting sk_pacing_shift,
I will submit v2 without this one.
Thanks for the information.

-Wright
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3a9b76fd0db9f
> 
>>                          __skb_queue_tail(&pktq, pkt);
>>                  }
>>                  spin_unlock_bh(&bus->txq_lock);
>> --
>> 1.9.1
>>

Re: [PATCH 3/3] brcmfmac: make firmware frameburst mode a module parameter

[Wright Feng]

On 2018/10/30 下午 07:33, Arend van Spriel wrote:
> On 10/29/2018 11:27 AM, Wright Feng wrote:
>> This patch is for adding a new module parameter "frameburst".
>> With setting "frameburst=1" in module parameters, firmware frameburst 
>> mode
>> will be enabled. The feature can enable per-packet framebursting in
>> firmware side and get higher TX throughput in High Throughput(HT) mode.
> 
> I am not sure about every firmware image, but in recent branches here I 
> see firmware enables frameburst by default. So not sure about the 
> motivation to add this. You could also consider adding this to the 
> nl80211 api. I am sure other vendors have similar features.
> 
> Regards,
> Arend
> 
The newer firmware may set firmware by default, but Some
firmwares in upstream do not, like 7.35.349.28 for 4354-sdio.

Before adding new API in ops and ATTR in nl80211, I think keeping the
frameburst module parameter is good for user to enhance TX throughput.
And I will find time to add new API after finishing my internal task.

-Wright
>> Signed-off-by: Wright Feng <wright.feng@cypress.com>
>> ---
>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 7 +++++++
>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c   | 5 +++++
>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h   | 2 ++
>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.h     | 1 +
>>  4 files changed, 15 insertions(+)
> 

Re: [PATCH 2/3] brcmfmac: add credit numbers updating support

[Wright Feng]

On 2018/10/30 下午 07:04, Arend van Spriel wrote:
> On 10/29/2018 11:27 AM, Wright Feng wrote:
>> The credit numbers are static and tunable per chip in firmware side.
>> However the credit number may be changed that is based on packet pool
>> length and will send BRCMF_E_FIFO_CREDIT_MAP event to notify host driver
>> updates the credit numbers during interface up.
>> The purpose of this patch is making host driver has ability of updating
>> the credit numbers when receiving the BRCMF_E_FIFO_CREDIT_MAP event.
> 
> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>> Signed-off-by: Wright Feng <wright.feng@cypress.com>
>> ---
>>  .../broadcom/brcm80211/brcmfmac/fwsignal.c         | 23 
>> ++++++++++++++--------
>>  1 file changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git 
>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c 
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>> index f3cbf78..e0910c5 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
> 
> [...]
> 
>> @@ -1595,19 +1599,21 @@ static int brcmf_fws_notify_credit_map(struct 
>> brcmf_if *ifp,
>>          brcmf_err("event payload too small (%d)\n", e->datalen);
>>          return -EINVAL;
>>      }
>> -    if (fws->creditmap_received)
>> -        return 0;
>>
>>      fws->creditmap_received = true;
> 
> I think the creditmap_received struct member is no longer needed.
I will keep this because we still need it for checking whether flow
control is active or not.
> 
>>      brcmf_dbg(TRACE, "enter: credits %pM\n", credits);
>>      brcmf_fws_lock(fws);
>>      for (i = 0; i < ARRAY_SIZE(fws->fifo_credit); i++) {
>> -        if (*credits)
>> +        fws->fifo_credit[i] += credits[i] - fws->init_fifo_credit[i];
>> +        fws->init_fifo_credit[i] = credits[i];
>> +        if (fws->fifo_credit[i] > 0)
>>              fws->fifo_credit_map |= 1 << i;
>>          else
>>              fws->fifo_credit_map &= ~(1 << i);
>> -        fws->fifo_credit[i] = *credits++;
>> +        if (fws->fifo_credit[i] < 0)
>> +            brcmf_err("fifo_credit[%d] value is negative(%d)\n",
>> +                  i, fws->fifo_credit[i]);
> 
> This looks like it should not happen so maybe warrants a WARN or WARN_ONCE?
I will replace those 2 lines with WARN_ONCE.
Thanks for the suggestion.

-Wright
> 
>>      }
>>      brcmf_fws_schedule_deq(fws);
>>      brcmf_fws_unlock(fws);
> 

Re: [PATCH 1/3] brcmfmac: calling skb_orphan before sending skb to SDIO bus

[Franky Lin]

On Thu, Nov 1, 2018 at 8:08 PM Wright Feng <Wright.Feng@cypress.com> wrote:
>
>
>
> On 2018/10/30 上午 02:50, Franky Lin wrote:
> > On Mon, Oct 29, 2018 at 3:27 AM Wright Feng <Wright.Feng@cypress.com> wrote:
> >>
> >> Linux 3.6 introduces TSQ which has a per socket threshold for TCP Tx
> >> packets to reduce latency. In fcmode 1 and fcmode 2, host driver enqueues
> >> skb in hanger and TCP doesn't push new skb until host frees the skb when
> >> receiving fwstatus event. So using skb_orphan before sending skb to bus
> >> will make the skb removing the ownership of socket. With this patch, we
> >> are able to get better throughput in fcmode 1 and fcmode 2.
> >>
> >> Tested 43455 TCP throughput in 20 MHz bandwidth with / without this patch.
> >> fcmode 0: 59.5 / 59.6 (mbps)
> >> fcmode 1: 59.3 / 23.4 (mbps)
> >> fcmode 2: 59.6 / 21.5 (mbps)
> >>
> >> Signed-off-by: Wright Feng <wright.feng@cypress.com>
> >> ---
> >>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> >> index b2e1ab5..519b25d 100644
> >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> >> @@ -2298,6 +2298,7 @@ static uint brcmf_sdio_sendfromq(struct brcmf_sdio *bus, uint maxframes)
> >>                                                &prec_out);
> >>                          if (pkt == NULL)
> >>                                  break;
> >> +                       skb_orphan(pkt);
> >
> > TSQ allows device driver to tweak for a deeper queue now. [1]. We
> > should use that instead of orphaning the packet before handing over to
> > firmware which would remove the bufferbloat protection by TSQ.
> >
> > Thanks,
> > -Franky
> >
> Since the aggregation problem has been fixed by setting sk_pacing_shift,
> I will submit v2 without this one.
> Thanks for the information.

No the problem is not fixed. The patch just provides an interface
sk_pacing_shift_update so the device driver can tweak the scale to
allow more packets being queued. You need to call
sk_pacing_shift_update somewhere in brcmfmac.

Thanks,
-Franky
>
> -Wright
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3a9b76fd0db9f
> >
> >>                          __skb_queue_tail(&pktq, pkt);
> >>                  }
> >>                  spin_unlock_bh(&bus->txq_lock);
> >> --
> >> 1.9.1
> >>

Re: [PATCH 1/3] brcmfmac: calling skb_orphan before sending skb to SDIO bus

[Wright Feng]

Franky Lin 於 11/3/2018 3:51 AM 寫道:
> On Thu, Nov 1, 2018 at 8:08 PM Wright Feng <Wright.Feng@cypress.com> wrote:
>>
>>
>>
>> On 2018/10/30 上午 02:50, Franky Lin wrote:
>>> On Mon, Oct 29, 2018 at 3:27 AM Wright Feng <Wright.Feng@cypress.com> wrote:
>>>>
>>>> Linux 3.6 introduces TSQ which has a per socket threshold for TCP Tx
>>>> packets to reduce latency. In fcmode 1 and fcmode 2, host driver enqueues
>>>> skb in hanger and TCP doesn't push new skb until host frees the skb when
>>>> receiving fwstatus event. So using skb_orphan before sending skb to bus
>>>> will make the skb removing the ownership of socket. With this patch, we
>>>> are able to get better throughput in fcmode 1 and fcmode 2.
>>>>
>>>> Tested 43455 TCP throughput in 20 MHz bandwidth with / without this patch.
>>>> fcmode 0: 59.5 / 59.6 (mbps)
>>>> fcmode 1: 59.3 / 23.4 (mbps)
>>>> fcmode 2: 59.6 / 21.5 (mbps)
>>>>
>>>> Signed-off-by: Wright Feng <wright.feng@cypress.com>
>>>> ---
>>>>    drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>> index b2e1ab5..519b25d 100644
>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>> @@ -2298,6 +2298,7 @@ static uint brcmf_sdio_sendfromq(struct brcmf_sdio *bus, uint maxframes)
>>>>                                                 &prec_out);
>>>>                           if (pkt == NULL)
>>>>                                   break;
>>>> +                       skb_orphan(pkt);
>>>
>>> TSQ allows device driver to tweak for a deeper queue now. [1]. We
>>> should use that instead of orphaning the packet before handing over to
>>> firmware which would remove the bufferbloat protection by TSQ.
>>>
>>> Thanks,
>>> -Franky
>>>
>> Since the aggregation problem has been fixed by setting sk_pacing_shift,
>> I will submit v2 without this one.
>> Thanks for the information.
> 
> No the problem is not fixed. The patch just provides an interface
> sk_pacing_shift_update so the device driver can tweak the scale to
> allow more packets being queued. You need to call
> sk_pacing_shift_update somewhere in brcmfmac.
> 
> Thanks,
> -Franky
We are still implementing the function of setting sk_pacing_shift in
brcmfmac, so the patch will be submitted in another patch series once
we have done the fully tests.

-Wright
>>
>> -Wright
>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3a9b76fd0db9f
>>>
>>>>                           __skb_queue_tail(&pktq, pkt);
>>>>                   }
>>>>                   spin_unlock_bh(&bus->txq_lock);
>>>> --
>>>> 1.9.1
>>>>