[PATCH 0/3] Add AP isolate support and two modules parameters

[PATCH 0/3] Add AP isolate support and two modules parameters

[Wright Feng]

Add AP isolate support and two module parameters("eap_restrict" and
"sdio_wq_highpri").

Wright Feng (3):
  brcmfmac: support AP isolation
  brcmfmac: make firmware eap_restrict a module parameter
  brcmfmac: make setting SDIO workqueue WQ_HIGHPRI a module parameter

 .../broadcom/brcm80211/brcmfmac/cfg80211.c         | 30 ++++++++++++++++++++++
 .../wireless/broadcom/brcm80211/brcmfmac/common.c  | 10 ++++++++
 .../wireless/broadcom/brcm80211/brcmfmac/common.h  |  4 +++
 .../wireless/broadcom/brcm80211/brcmfmac/sdio.c    | 22 ++++++++++------
 4 files changed, 58 insertions(+), 8 deletions(-)

-- 
2.1.0

[PATCH 1/3] brcmfmac: support AP isolate

[Wright Feng]

Hostap daemon has a parameter "ap_isolate" which is used to prevent
low-level bridging of frames between associated stations in the BSS.
For driver side, we add cfg80211 ops method change_bss to support
setting AP isolate from user space.

Signed-off-by: Wright Feng <wright.feng@cypress.com>
Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
---
 .../wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index a2328d3..eb49900 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -5376,6 +5376,26 @@ static int brcmf_cfg80211_del_pmk(struct wiphy *wiphy, struct net_device *dev,
 	return brcmf_set_pmk(ifp, NULL, 0);
 }
 
+static int
+brcmf_cfg80211_change_bss(struct wiphy *wiphy, struct net_device *dev,
+			  struct bss_parameters *params)
+{
+	struct brcmf_if *ifp;
+	int ret = 0;
+	u32 ap_isolate;
+
+	brcmf_dbg(TRACE, "Enter\n");
+	ifp = netdev_priv(dev);
+	if (params->ap_isolate >= 0) {
+		ap_isolate = (u32)params->ap_isolate;
+		ret = brcmf_fil_iovar_int_set(ifp, "ap_isolate", ap_isolate);
+		if (ret < 0)
+			brcmf_err("ap_isolate iovar failed: ret=%d\n", ret);
+	}
+
+	return ret;
+}
+
 static struct cfg80211_ops brcmf_cfg80211_ops = {
 	.add_virtual_intf = brcmf_cfg80211_add_iface,
 	.del_virtual_intf = brcmf_cfg80211_del_iface,
@@ -5421,6 +5441,7 @@ static struct cfg80211_ops brcmf_cfg80211_ops = {
 	.update_connect_params = brcmf_cfg80211_update_conn_params,
 	.set_pmk = brcmf_cfg80211_set_pmk,
 	.del_pmk = brcmf_cfg80211_del_pmk,
+	.change_bss = brcmf_cfg80211_change_bss,
 };
 
 struct cfg80211_ops *brcmf_cfg80211_get_ops(struct brcmf_mp_device *settings)
-- 
2.1.0

[PATCH 2/3] brcmfmac: make firmware eap_restrict a module parameter

[Wright Feng]

When eap_restrict is enabled, firmware will toss non-802.1x frames from
tx/rx data path if station not yet authorized.
Internal firmware eap_restrict is disabled by default. This patch makes
it possible to enable firmware eap_restrict by specifying
eap_restrict=1 as module parameter.

Signed-off-by: Wright Feng <wright.feng@cypress.com>
Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 9 +++++++++
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c   | 5 +++++
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h   | 2 ++
 3 files changed, 16 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index eb49900..07a231c 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -6942,6 +6942,7 @@ static s32 brcmf_config_dongle(struct brcmf_cfg80211_info *cfg)
 	struct wireless_dev *wdev;
 	struct brcmf_if *ifp;
 	s32 power_mode;
+	s32 eap_restrict;
 	s32 err = 0;
 
 	if (cfg->dongle_up)
@@ -6966,6 +6967,14 @@ static s32 brcmf_config_dongle(struct brcmf_cfg80211_info *cfg)
 	err = brcmf_dongle_roam(ifp);
 	if (err)
 		goto default_conf_out;
+
+	eap_restrict = ifp->drvr->settings->eap_restrict;
+	if (eap_restrict) {
+		err = brcmf_fil_iovar_int_set(ifp, "eap_restrict",
+					      eap_restrict);
+		if (err)
+			brcmf_info("eap_restrict error (%d)\n", err);
+	}
 	err = brcmf_cfg80211_change_iface(wdev->wiphy, ndev, wdev->iftype,
 					  NULL);
 	if (err)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index dec25e4..cda6bef 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -67,6 +67,10 @@ static int brcmf_iapp_enable;
 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_eap_restrict;
+module_param_named(eap_restrict, brcmf_eap_restrict, int, 0400);
+MODULE_PARM_DESC(eap_restrict, "Block non-802.1X frames until auth finished");
+
 #ifdef DEBUG
 /* always succeed brcmf_bus_started() */
 static int brcmf_ignore_probe_fail;
@@ -413,6 +417,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->eap_restrict = !!brcmf_eap_restrict;
 #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 144cf45..059f09c 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h
@@ -37,6 +37,7 @@ extern struct brcmf_mp_global_t brcmf_mp_global;
  * @feature_disable: Feature_disable bitmask.
  * @fcmode: FWS flow control.
  * @roamoff: Firmware roaming off?
+ * @eap_restrict: Not allow data tx/rx until 802.1X auth succeeds
  * @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.
@@ -47,6 +48,7 @@ struct brcmf_mp_device {
 	int		fcmode;
 	bool		roamoff;
 	bool		iapp;
+	bool		eap_restrict;
 	bool		ignore_probe_fail;
 	struct brcmfmac_pd_cc *country_codes;
 	const char	*board_type;
-- 
2.1.0

[PATCH 3/3] brcmfmac: make setting SDIO workqueue WQ_HIGHPRI a module parameter

[Wright Feng]

With setting sdio_wq_highpri=1 in module parameters, tasks submitted to
SDIO workqueue will put at the head of the queue and run immediately.
This parameter is for getting higher TX/RX throughput with SDIO bus.

Signed-off-by: Wright Feng <wright.feng@cypress.com>
Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
---
 .../wireless/broadcom/brcm80211/brcmfmac/common.c  |  5 +++++
 .../wireless/broadcom/brcm80211/brcmfmac/common.h  |  2 ++
 .../wireless/broadcom/brcm80211/brcmfmac/sdio.c    | 22 ++++++++++++++--------
 3 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index cda6bef..3cf0e74 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -71,6 +71,10 @@ static int brcmf_eap_restrict;
 module_param_named(eap_restrict, brcmf_eap_restrict, int, 0400);
 MODULE_PARM_DESC(eap_restrict, "Block non-802.1X frames until auth finished");
 
+static int brcmf_sdio_wq_highpri;
+module_param_named(sdio_wq_highpri, brcmf_sdio_wq_highpri, int, 0);
+MODULE_PARM_DESC(sdio_wq_highpri, "Set SDIO workqueue to high priority");
+
 #ifdef DEBUG
 /* always succeed brcmf_bus_started() */
 static int brcmf_ignore_probe_fail;
@@ -418,6 +422,7 @@ struct brcmf_mp_device *brcmf_get_module_param(struct device *dev,
 	settings->roamoff = !!brcmf_roamoff;
 	settings->iapp = !!brcmf_iapp_enable;
 	settings->eap_restrict = !!brcmf_eap_restrict;
+	settings->sdio_wq_highpri = !!brcmf_sdio_wq_highpri;
 #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 059f09c..0cb39b6 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h
@@ -38,6 +38,7 @@ extern struct brcmf_mp_global_t brcmf_mp_global;
  * @fcmode: FWS flow control.
  * @roamoff: Firmware roaming off?
  * @eap_restrict: Not allow data tx/rx until 802.1X auth succeeds
+ * @sdio_wq_highpri: Tasks submitted to SDIO workqueue will run immediately.
  * @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.
@@ -49,6 +50,7 @@ struct brcmf_mp_device {
 	bool		roamoff;
 	bool		iapp;
 	bool		eap_restrict;
+	bool		sdio_wq_highpri;
 	bool		ignore_probe_fail;
 	struct brcmfmac_pd_cc *country_codes;
 	const char	*board_type;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 3a08252..885e8bd 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -4342,9 +4342,21 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev)
 	bus->txminmax = BRCMF_TXMINMAX;
 	bus->tx_seq = SDPCM_SEQ_WRAP - 1;
 
+	/* attempt to attach to the dongle */
+	if (!(brcmf_sdio_probe_attach(bus))) {
+		brcmf_err("brcmf_sdio_probe_attach failed\n");
+		goto fail;
+	}
+
 	/* single-threaded workqueue */
-	wq = alloc_ordered_workqueue("brcmf_wq/%s", WQ_MEM_RECLAIM,
-				     dev_name(&sdiodev->func1->dev));
+	if (sdiodev->settings->sdio_wq_highpri) {
+		wq = alloc_workqueue("brcmf_wq/%s",
+				     WQ_HIGHPRI | WQ_MEM_RECLAIM | WQ_UNBOUND,
+				     1, dev_name(&sdiodev->func1->dev));
+	} else {
+		wq = alloc_ordered_workqueue("brcmf_wq/%s", WQ_MEM_RECLAIM,
+					     dev_name(&sdiodev->func1->dev));
+	}
 	if (!wq) {
 		brcmf_err("insufficient memory to create txworkqueue\n");
 		goto fail;
@@ -4353,12 +4365,6 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev)
 	INIT_WORK(&bus->datawork, brcmf_sdio_dataworker);
 	bus->brcmf_wq = wq;
 
-	/* attempt to attach to the dongle */
-	if (!(brcmf_sdio_probe_attach(bus))) {
-		brcmf_err("brcmf_sdio_probe_attach failed\n");
-		goto fail;
-	}
-
 	spin_lock_init(&bus->rxctl_lock);
 	spin_lock_init(&bus->txq_lock);
 	init_waitqueue_head(&bus->ctrl_wait);
-- 
2.1.0

Re: [PATCH 2/3] brcmfmac: make firmware eap_restrict a module parameter

[Arend Van Spriel]

On 3/19/2020 8:53 AM, Wright Feng wrote:
> When eap_restrict is enabled, firmware will toss non-802.1x frames from
> tx/rx data path if station not yet authorized.
> Internal firmware eap_restrict is disabled by default. This patch makes
> it possible to enable firmware eap_restrict by specifying
> eap_restrict=1 as module parameter.

What is the reason for not having this toss behavior as default? Don't 
see much reason for having the module parameter.

> Signed-off-by: Wright Feng <wright.feng@cypress.com>
> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
> ---
>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 9 +++++++++
>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c   | 5 +++++
>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h   | 2 ++
>   3 files changed, 16 insertions(+)

Re: [PATCH 1/3] brcmfmac: support AP isolate

[Arend Van Spriel]

On 3/19/2020 8:53 AM, Wright Feng wrote:
> Hostap daemon has a parameter "ap_isolate" which is used to prevent
> low-level bridging of frames between associated stations in the BSS.
> For driver side, we add cfg80211 ops method change_bss to support
> setting AP isolate from user space.
> 
> Signed-off-by: Wright Feng <wright.feng@cypress.com>
> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
> ---
>   .../wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index a2328d3..eb49900 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c

[...]

> @@ -5421,6 +5441,7 @@ static struct cfg80211_ops brcmf_cfg80211_ops = {
>   	.update_connect_params = brcmf_cfg80211_update_conn_params,
>   	.set_pmk = brcmf_cfg80211_set_pmk,
>   	.del_pmk = brcmf_cfg80211_del_pmk,
> +	.change_bss = brcmf_cfg80211_change_bss,

maybe only add this when firmware support "ap_isolate"?

Regards,
Arend

Re: [PATCH 3/3] brcmfmac: make setting SDIO workqueue WQ_HIGHPRI a module parameter

[Arend Van Spriel]

+ Tejun - regarding alloc_workqueue usage

On 3/19/2020 8:53 AM, Wright Feng wrote:
> With setting sdio_wq_highpri=1 in module parameters, tasks submitted to
> SDIO workqueue will put at the head of the queue and run immediately.
> This parameter is for getting higher TX/RX throughput with SDIO bus.
> 
> Signed-off-by: Wright Feng <wright.feng@cypress.com>
> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
> ---
>   .../wireless/broadcom/brcm80211/brcmfmac/common.c  |  5 +++++
>   .../wireless/broadcom/brcm80211/brcmfmac/common.h  |  2 ++
>   .../wireless/broadcom/brcm80211/brcmfmac/sdio.c    | 22 ++++++++++++++--------
>   3 files changed, 21 insertions(+), 8 deletions(-)
> 

[...]

> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index 3a08252..885e8bd 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -4342,9 +4342,21 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev)
>   	bus->txminmax = BRCMF_TXMINMAX;
>   	bus->tx_seq = SDPCM_SEQ_WRAP - 1;
>   
> +	/* attempt to attach to the dongle */
> +	if (!(brcmf_sdio_probe_attach(bus))) {
> +		brcmf_err("brcmf_sdio_probe_attach failed\n");
> +		goto fail;
> +	}
> +
>   	/* single-threaded workqueue */
> -	wq = alloc_ordered_workqueue("brcmf_wq/%s", WQ_MEM_RECLAIM,
> -				     dev_name(&sdiodev->func1->dev));
> +	if (sdiodev->settings->sdio_wq_highpri) {
> +		wq = alloc_workqueue("brcmf_wq/%s",
> +				     WQ_HIGHPRI | WQ_MEM_RECLAIM | WQ_UNBOUND,
> +				     1, dev_name(&sdiodev->func1->dev));

So two things changed, 1) WQ_HIGHPRI flag added *and* 2) use 
allow_workqueue basically dropping __WQ_ORDERED. Not sure which one 
contributes to the behavior described in the commit message.

Regards,
Arend

> +	} else {
> +		wq = alloc_ordered_workqueue("brcmf_wq/%s", WQ_MEM_RECLAIM,
> +					     dev_name(&sdiodev->func1->dev));
> +	}
>   	if (!wq) {
>   		brcmf_err("insufficient memory to create txworkqueue\n");
>   		goto fail;

Re: [PATCH 1/3] brcmfmac: support AP isolate

[Wright Feng]

Arend Van Spriel 於 3/19/2020 4:48 PM 寫道:
> On 3/19/2020 8:53 AM, Wright Feng wrote:
>> Hostap daemon has a parameter "ap_isolate" which is used to prevent
>> low-level bridging of frames between associated stations in the BSS.
>> For driver side, we add cfg80211 ops method change_bss to support
>> setting AP isolate from user space.
>>
>> Signed-off-by: Wright Feng <wright.feng@cypress.com>
>> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
>> ---
>>   .../wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 21 
>> +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git 
>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c 
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> index a2328d3..eb49900 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> 
> [...]
> 
>> @@ -5421,6 +5441,7 @@ static struct cfg80211_ops brcmf_cfg80211_ops = {
>>       .update_connect_params = brcmf_cfg80211_update_conn_params,
>>       .set_pmk = brcmf_cfg80211_set_pmk,
>>       .del_pmk = brcmf_cfg80211_del_pmk,
>> +    .change_bss = brcmf_cfg80211_change_bss,
> 
> maybe only add this when firmware support "ap_isolate"?
Sounds great, will do it in V2.
> 
> Regards,
> Arend

Re: [PATCH 2/3] brcmfmac: make firmware eap_restrict a module parameter

[Wright Feng]

Arend Van Spriel 於 3/19/2020 4:28 PM 寫道:
> On 3/19/2020 8:53 AM, Wright Feng wrote:
>> When eap_restrict is enabled, firmware will toss non-802.1x frames from
>> tx/rx data path if station not yet authorized.
>> Internal firmware eap_restrict is disabled by default. This patch makes
>> it possible to enable firmware eap_restrict by specifying
>> eap_restrict=1 as module parameter.
> 
> What is the reason for not having this toss behavior as default? Don't 
> see much reason for having the module parameter.

The eap_restrict feature in most of firmwares are disabled as default, 
and refer to our experience, using eap_restrict increases roam time for 
associations in some cases.
So what we do in this patch is not changing the default firmware 
behavior but still give users a way to enable eap_resrtict feature.

>> Signed-off-by: Wright Feng <wright.feng@cypress.com>
>> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
>> ---
>>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 9 
>> +++++++++
>>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c   | 5 +++++
>>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h   | 2 ++
>>   3 files changed, 16 insertions(+)

Re: [PATCH 3/3] brcmfmac: make setting SDIO workqueue WQ_HIGHPRI a module parameter

[Wright Feng]

Arend Van Spriel 於 3/19/2020 4:55 PM 寫道:
> + Tejun - regarding alloc_workqueue usage
> 
> On 3/19/2020 8:53 AM, Wright Feng wrote:
>> With setting sdio_wq_highpri=1 in module parameters, tasks submitted to
>> SDIO workqueue will put at the head of the queue and run immediately.
>> This parameter is for getting higher TX/RX throughput with SDIO bus.
>>
>> Signed-off-by: Wright Feng <wright.feng@cypress.com>
>> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
>> ---
>>   .../wireless/broadcom/brcm80211/brcmfmac/common.c  |  5 +++++
>>   .../wireless/broadcom/brcm80211/brcmfmac/common.h  |  2 ++
>>   .../wireless/broadcom/brcm80211/brcmfmac/sdio.c    | 22 
>> ++++++++++++++--------
>>   3 files changed, 21 insertions(+), 8 deletions(-)
>>
> 
> [...]
> 
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c 
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> index 3a08252..885e8bd 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> @@ -4342,9 +4342,21 @@ struct brcmf_sdio *brcmf_sdio_probe(struct 
>> brcmf_sdio_dev *sdiodev)
>>       bus->txminmax = BRCMF_TXMINMAX;
>>       bus->tx_seq = SDPCM_SEQ_WRAP - 1;
>> +    /* attempt to attach to the dongle */
>> +    if (!(brcmf_sdio_probe_attach(bus))) {
>> +        brcmf_err("brcmf_sdio_probe_attach failed\n");
>> +        goto fail;
>> +    }
>> +
>>       /* single-threaded workqueue */
>> -    wq = alloc_ordered_workqueue("brcmf_wq/%s", WQ_MEM_RECLAIM,
>> -                     dev_name(&sdiodev->func1->dev));
>> +    if (sdiodev->settings->sdio_wq_highpri) {
>> +        wq = alloc_workqueue("brcmf_wq/%s",
>> +                     WQ_HIGHPRI | WQ_MEM_RECLAIM | WQ_UNBOUND,
>> +                     1, dev_name(&sdiodev->func1->dev));
> 
> So two things changed, 1) WQ_HIGHPRI flag added *and* 2) use 
> allow_workqueue basically dropping __WQ_ORDERED. Not sure which one 
> contributes to the behavior described in the commit message.

The combination of Unbound and max_active==1 implies ordered, so I 
suppose we don't need to set __WQ_ORDERED bit in flags.

> Regards,
> Arend
> 
>> +    } else {
>> +        wq = alloc_ordered_workqueue("brcmf_wq/%s", WQ_MEM_RECLAIM,
>> +                         dev_name(&sdiodev->func1->dev));
>> +    }
>>       if (!wq) {
>>           brcmf_err("insufficient memory to create txworkqueue\n");
>>           goto fail;
> 

Re: [PATCH 3/3] brcmfmac: make setting SDIO workqueue WQ_HIGHPRI a module parameter

[Arend Van Spriel]

On 3/20/2020 9:08 AM, Wright Feng wrote:
> 
> 
> Arend Van Spriel 於 3/19/2020 4:55 PM 寫道:
>> + Tejun - regarding alloc_workqueue usage
>>
>> On 3/19/2020 8:53 AM, Wright Feng wrote:
>>> With setting sdio_wq_highpri=1 in module parameters, tasks submitted to
>>> SDIO workqueue will put at the head of the queue and run immediately.
>>> This parameter is for getting higher TX/RX throughput with SDIO bus.
>>>
>>> Signed-off-by: Wright Feng <wright.feng@cypress.com>
>>> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
>>> ---
>>>   .../wireless/broadcom/brcm80211/brcmfmac/common.c  |  5 +++++
>>>   .../wireless/broadcom/brcm80211/brcmfmac/common.h  |  2 ++
>>>   .../wireless/broadcom/brcm80211/brcmfmac/sdio.c    | 22 
>>> ++++++++++++++--------
>>>   3 files changed, 21 insertions(+), 8 deletions(-)
>>>
>>
>> [...]
>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c 
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> index 3a08252..885e8bd 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> @@ -4342,9 +4342,21 @@ struct brcmf_sdio *brcmf_sdio_probe(struct 
>>> brcmf_sdio_dev *sdiodev)
>>>       bus->txminmax = BRCMF_TXMINMAX;
>>>       bus->tx_seq = SDPCM_SEQ_WRAP - 1;
>>> +    /* attempt to attach to the dongle */
>>> +    if (!(brcmf_sdio_probe_attach(bus))) {
>>> +        brcmf_err("brcmf_sdio_probe_attach failed\n");
>>> +        goto fail;
>>> +    }
>>> +
>>>       /* single-threaded workqueue */
>>> -    wq = alloc_ordered_workqueue("brcmf_wq/%s", WQ_MEM_RECLAIM,
>>> -                     dev_name(&sdiodev->func1->dev));
>>> +    if (sdiodev->settings->sdio_wq_highpri) {
>>> +        wq = alloc_workqueue("brcmf_wq/%s",
>>> +                     WQ_HIGHPRI | WQ_MEM_RECLAIM | WQ_UNBOUND,
>>> +                     1, dev_name(&sdiodev->func1->dev));
>>
>> So two things changed, 1) WQ_HIGHPRI flag added *and* 2) use 
>> allow_workqueue basically dropping __WQ_ORDERED. Not sure which one 
>> contributes to the behavior described in the commit message.
> 
> The combination of Unbound and max_active==1 implies ordered, so I 
> suppose we don't need to set __WQ_ORDERED bit in flags.

My reason for asking was the idea to only determine flags in the 
if-statement and have following by one alloc_wq call, ie.:

wq_flags = WQ_MEM_RECLAIM;
if (sdio_wq_highpri)
	wq_flags |= WQ_HIGHPRI
wq = alloc_ordered_workqueue(..., wq_flags,...);

Regards,
Arend

Re: [PATCH 3/3] brcmfmac: make setting SDIO workqueue WQ_HIGHPRI a module parameter

[Wright Feng]

Arend Van Spriel 於 3/20/2020 4:18 PM 寫道:
> On 3/20/2020 9:08 AM, Wright Feng wrote:
>>
>>
>> Arend Van Spriel 於 3/19/2020 4:55 PM 寫道:
>>> + Tejun - regarding alloc_workqueue usage
>>>
>>> On 3/19/2020 8:53 AM, Wright Feng wrote:
>>>> With setting sdio_wq_highpri=1 in module parameters, tasks submitted to
>>>> SDIO workqueue will put at the head of the queue and run immediately.
>>>> This parameter is for getting higher TX/RX throughput with SDIO bus.
>>>>
>>>> Signed-off-by: Wright Feng <wright.feng@cypress.com>
>>>> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
>>>> ---
>>>>   .../wireless/broadcom/brcm80211/brcmfmac/common.c  |  5 +++++
>>>>   .../wireless/broadcom/brcm80211/brcmfmac/common.h  |  2 ++
>>>>   .../wireless/broadcom/brcm80211/brcmfmac/sdio.c    | 22 
>>>> ++++++++++++++--------
>>>>   3 files changed, 21 insertions(+), 8 deletions(-)
>>>>
>>>
>>> [...]
>>>
>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c 
>>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>> index 3a08252..885e8bd 100644
>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>> @@ -4342,9 +4342,21 @@ struct brcmf_sdio *brcmf_sdio_probe(struct 
>>>> brcmf_sdio_dev *sdiodev)
>>>>       bus->txminmax = BRCMF_TXMINMAX;
>>>>       bus->tx_seq = SDPCM_SEQ_WRAP - 1;
>>>> +    /* attempt to attach to the dongle */
>>>> +    if (!(brcmf_sdio_probe_attach(bus))) {
>>>> +        brcmf_err("brcmf_sdio_probe_attach failed\n");
>>>> +        goto fail;
>>>> +    }
>>>> +
>>>>       /* single-threaded workqueue */
>>>> -    wq = alloc_ordered_workqueue("brcmf_wq/%s", WQ_MEM_RECLAIM,
>>>> -                     dev_name(&sdiodev->func1->dev));
>>>> +    if (sdiodev->settings->sdio_wq_highpri) {
>>>> +        wq = alloc_workqueue("brcmf_wq/%s",
>>>> +                     WQ_HIGHPRI | WQ_MEM_RECLAIM | WQ_UNBOUND,
>>>> +                     1, dev_name(&sdiodev->func1->dev));
>>>
>>> So two things changed, 1) WQ_HIGHPRI flag added *and* 2) use 
>>> allow_workqueue basically dropping __WQ_ORDERED. Not sure which one 
>>> contributes to the behavior described in the commit message.
>>
>> The combination of Unbound and max_active==1 implies ordered, so I 
>> suppose we don't need to set __WQ_ORDERED bit in flags.
> 
> My reason for asking was the idea to only determine flags in the 
> if-statement and have following by one alloc_wq call, ie.:
> 
> wq_flags = WQ_MEM_RECLAIM;
> if (sdio_wq_highpri)
>      wq_flags |= WQ_HIGHPRI
> wq = alloc_ordered_workqueue(..., wq_flags,...);
> 
Yes, I also want to do that so, but the comment in 
inclues/linux/workqueue.h shows that
"@flags: WQ_* flags (only WQ_FREEZABLE and WQ_MEM_RECLAIM are meaningful)"
and "__WQ_ORDERED" and "__WQ_ORDERED_EXPLICITI" are for workqueue 
internal use.

That's why I set WQ_HIGHPRI by alloc_workqueue which is also seen in 
other wireless drivers.

> Regards,
> Arend

Re: [PATCH 2/3] brcmfmac: make firmware eap_restrict a module parameter

[Kalle Valo]

Wright Feng <wright.feng@cypress.com> writes:

> Arend Van Spriel 於 3/19/2020 4:28 PM 寫道:
>> On 3/19/2020 8:53 AM, Wright Feng wrote:
>>> When eap_restrict is enabled, firmware will toss non-802.1x frames from
>>> tx/rx data path if station not yet authorized.
>>> Internal firmware eap_restrict is disabled by default. This patch makes
>>> it possible to enable firmware eap_restrict by specifying
>>> eap_restrict=1 as module parameter.
>>
>> What is the reason for not having this toss behavior as default?
>> Don't see much reason for having the module parameter.
>
> The eap_restrict feature in most of firmwares are disabled as default,
> and refer to our experience, using eap_restrict increases roam time
> for associations in some cases.

What are these these cases exactly?

> So what we do in this patch is not changing the default firmware
> behavior but still give users a way to enable eap_resrtict feature.

You should have mentioned this (ie. answer the "Why?" part) in the
commit log in the first place.

But I don't like adding module parameters unless with really good
reasons. And in this case there's no proper documentation when and how a
user should use the module parameter so this is nowhere near a proper
justifiction.

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

Re: [PATCH 3/3] brcmfmac: make setting SDIO workqueue WQ_HIGHPRI a module parameter

[Kalle Valo]

Wright Feng <wright.feng@cypress.com> writes:

> With setting sdio_wq_highpri=1 in module parameters, tasks submitted to
> SDIO workqueue will put at the head of the queue and run immediately.
> This parameter is for getting higher TX/RX throughput with SDIO bus.

Why would someone want to disable this? Like in patch 2, please avoid
adding new module parameters as much as possible.

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

Re: [PATCH 3/3] brcmfmac: make setting SDIO workqueue WQ_HIGHPRI a module parameter

[Tejun Heo]

Hello,

On Fri, Mar 20, 2020 at 04:08:47PM +0800, Wright Feng wrote:
> > So two things changed, 1) WQ_HIGHPRI flag added *and* 2) use
> > allow_workqueue basically dropping __WQ_ORDERED. Not sure which one
> > contributes to the behavior described in the commit message.
> 
> The combination of Unbound and max_active==1 implies ordered, so I suppose
> we don't need to set __WQ_ORDERED bit in flags.

It doesn't on NUMA setups. If you need ordered execution, you need the flag.

Thanks.

-- 
tejun

Re: [PATCH 3/3] brcmfmac: make setting SDIO workqueue WQ_HIGHPRI a module parameter

[Wright Feng]

Tejun Heo 於 3/25/2020 2:23 AM 寫道:
> Hello,
> 
> On Fri, Mar 20, 2020 at 04:08:47PM +0800, Wright Feng wrote:
>>> So two things changed, 1) WQ_HIGHPRI flag added *and* 2) use
>>> allow_workqueue basically dropping __WQ_ORDERED. Not sure which one
>>> contributes to the behavior described in the commit message.
>>
>> The combination of Unbound and max_active==1 implies ordered, so I suppose
>> we don't need to set __WQ_ORDERED bit in flags.
> 
> It doesn't on NUMA setups. If you need ordered execution, you need the flag.
> 
> Thanks.
>
Hi Tejun,
In kernel/workqueue.c, it helps set __WQ_ORDERED flag for the 
condition(Unbound && max_active == 1) to keep the previous behavior to 
avoid subtle breakages on NUMA.
Does it mean we don't have to set the flags in alloc_workqueue? or I 
misunderstand something?
If that's incorrect, would you please give me a hint how to set 
__WQ_ORDERED(internal use flag) and WQ_HIGHPRI flags at the same time?

---
/*
  * Unbound && max_active == 1 used to imply ordered, which is no
  * longer the case on NUMA machines due to per-node pools.  While
  * alloc_ordered_workqueue() is the right way to create an ordered
  * workqueue, keep the previous behavior to avoid subtle breakages
  * on NUMA.
  */
if ((flags & WQ_UNBOUND) && max_active == 1)
     flags |= __WQ_ORDERED;
---

Regards,
Wright

Re: [PATCH 3/3] brcmfmac: make setting SDIO workqueue WQ_HIGHPRI a module parameter

[Tejun Heo]

On Wed, Mar 25, 2020 at 12:29:44PM +0800, Wright Feng wrote:
> If that's incorrect, would you please give me a hint how to set
> __WQ_ORDERED(internal use flag) and WQ_HIGHPRI flags at the same time?

Wouldn't alloc_ordered_workqueue(NAME, WQ_HIGHPRI, ...) do what you want?

Thanks.

-- 
tejun

Re: [PATCH 3/3] brcmfmac: make setting SDIO workqueue WQ_HIGHPRI a module parameter

[Arend Van Spriel]

On March 25, 2020 3:08:18 PM Tejun Heo <tj@kernel.org> wrote:

> On Wed, Mar 25, 2020 at 12:29:44PM +0800, Wright Feng wrote:
>> If that's incorrect, would you please give me a hint how to set
>> __WQ_ORDERED(internal use flag) and WQ_HIGHPRI flags at the same time?
>
> Wouldn't alloc_ordered_workqueue(NAME, WQ_HIGHPRI, ...) do what you want?

That was my initial suggestion. Can WQ_HIGHPRI be used together with 
WQ_MEMRECLAIM?

Regards,
Arend

Re: [PATCH 3/3] brcmfmac: make setting SDIO workqueue WQ_HIGHPRI a module parameter

[Wright Feng]

Arend Van Spriel 於 3/25/2020 10:53 PM 寫道:
> On March 25, 2020 3:08:18 PM Tejun Heo <tj@kernel.org> wrote:
> 
>> On Wed, Mar 25, 2020 at 12:29:44PM +0800, Wright Feng wrote:
>>> If that's incorrect, would you please give me a hint how to set
>>> __WQ_ORDERED(internal use flag) and WQ_HIGHPRI flags at the same time?
>>
>> Wouldn't alloc_ordered_workqueue(NAME, WQ_HIGHPRI, ...) do what you want?
> 
> That was my initial suggestion. Can WQ_HIGHPRI be used together with 
> WQ_MEMRECLAIM?
> 
> Regards,
> Arend
I was trying do that, but the comment of alloc_oredered_workqueue shows 
that only WQ_FREEZABLE and WQ_MEM_RECLAIM are meaningful, so...

I will measure the throughput with "alloc_ordered_workqueue(NAME, 
WQ_HIGHPRI, ...)" to see if WQ_HIGHPRI works with 
alloc_ordered_workqueue. Thanks for the suggestion.

---
/**
  * alloc_ordered_workqueue - allocate an ordered workqueue
  * @fmt: printf format for the name of the workqueue
  * @flags: WQ_* flags (only WQ_FREEZABLE and WQ_MEM_RECLAIM are meaningful)
  * @args...: args for @fmt
  *
...
  */
---

Regards,
Wright

Re: [PATCH 3/3] brcmfmac: make setting SDIO workqueue WQ_HIGHPRI a module parameter

[Tejun Heo]

On Wed, Mar 25, 2020 at 03:53:43PM +0100, Arend Van Spriel wrote:
> On March 25, 2020 3:08:18 PM Tejun Heo <tj@kernel.org> wrote:
> 
> > On Wed, Mar 25, 2020 at 12:29:44PM +0800, Wright Feng wrote:
> > > If that's incorrect, would you please give me a hint how to set
> > > __WQ_ORDERED(internal use flag) and WQ_HIGHPRI flags at the same time?
> > 
> > Wouldn't alloc_ordered_workqueue(NAME, WQ_HIGHPRI, ...) do what you want?
> 
> That was my initial suggestion. Can WQ_HIGHPRI be used together with
> WQ_MEMRECLAIM?

Yeah, they shouldn't interfere with each other.

Thanks.

-- 
tejun

Re: [PATCH 3/3] brcmfmac: make setting SDIO workqueue WQ_HIGHPRI a module parameter

[Tejun Heo]

Hello,

On Wed, Mar 25, 2020 at 11:06:33PM +0800, Wright Feng wrote:
> I was trying do that, but the comment of alloc_oredered_workqueue shows that
> only WQ_FREEZABLE and WQ_MEM_RECLAIM are meaningful, so...
> 
> I will measure the throughput with "alloc_ordered_workqueue(NAME,
> WQ_HIGHPRI, ...)" to see if WQ_HIGHPRI works with alloc_ordered_workqueue.
> Thanks for the suggestion.
> 
> ---
> /**
>  * alloc_ordered_workqueue - allocate an ordered workqueue
>  * @fmt: printf format for the name of the workqueue
>  * @flags: WQ_* flags (only WQ_FREEZABLE and WQ_MEM_RECLAIM are meaningful)
>  * @args...: args for @fmt

Yeah, I think the comment is outdated. If it doesn't work as expected, please
let me know.

-- 
tejun

Re: [PATCH 3/3] brcmfmac: make setting SDIO workqueue WQ_HIGHPRI a module parameter

[Wright Feng]

Tejun Heo 於 3/25/2020 11:12 PM 寫道:
> Hello,
> 
> On Wed, Mar 25, 2020 at 11:06:33PM +0800, Wright Feng wrote:
>> I was trying do that, but the comment of alloc_oredered_workqueue shows that
>> only WQ_FREEZABLE and WQ_MEM_RECLAIM are meaningful, so...
>>
>> I will measure the throughput with "alloc_ordered_workqueue(NAME,
>> WQ_HIGHPRI, ...)" to see if WQ_HIGHPRI works with alloc_ordered_workqueue.
>> Thanks for the suggestion.
>>
>> ---
>> /**
>>   * alloc_ordered_workqueue - allocate an ordered workqueue
>>   * @fmt: printf format for the name of the workqueue
>>   * @flags: WQ_* flags (only WQ_FREEZABLE and WQ_MEM_RECLAIM are meaningful)
>>   * @args...: args for @fmt
> 
> Yeah, I think the comment is outdated. If it doesn't work as expected, please
> let me know.
> 
It works as expected. With alloc_oredered_workqueue(..., WQ_HIGHPRI, 
...), the nice value is -20 and throughput result with 43455(11ac) on 1 
core 1.6 Ghz platform is
Without WQ_HIGGPRI TX/RX: 293/301 (mbps)
With    WQ_HIGHPRI TX/RX: 293/321 (mbps)

Regards,
Wright

Re: [PATCH 3/3] brcmfmac: make setting SDIO workqueue WQ_HIGHPRI a module parameter

[Tejun Heo]

On Fri, Mar 27, 2020 at 05:14:29PM +0800, Wright Feng wrote:
> 
> 
> Tejun Heo 於 3/25/2020 11:12 PM 寫道:
> > Hello,
> > 
> > On Wed, Mar 25, 2020 at 11:06:33PM +0800, Wright Feng wrote:
> > > I was trying do that, but the comment of alloc_oredered_workqueue shows that
> > > only WQ_FREEZABLE and WQ_MEM_RECLAIM are meaningful, so...
> > > 
> > > I will measure the throughput with "alloc_ordered_workqueue(NAME,
> > > WQ_HIGHPRI, ...)" to see if WQ_HIGHPRI works with alloc_ordered_workqueue.
> > > Thanks for the suggestion.
> > > 
> > > ---
> > > /**
> > >   * alloc_ordered_workqueue - allocate an ordered workqueue
> > >   * @fmt: printf format for the name of the workqueue
> > >   * @flags: WQ_* flags (only WQ_FREEZABLE and WQ_MEM_RECLAIM are meaningful)
> > >   * @args...: args for @fmt
> > 
> > Yeah, I think the comment is outdated. If it doesn't work as expected, please
> > let me know.
> > 
> It works as expected. With alloc_oredered_workqueue(..., WQ_HIGHPRI, ...),
> the nice value is -20 and throughput result with 43455(11ac) on 1 core 1.6
> Ghz platform is
> Without WQ_HIGGPRI TX/RX: 293/301 (mbps)
> With    WQ_HIGHPRI TX/RX: 293/321 (mbps)

Will update the comment. Thanks.

-- 
tejun