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
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 <[email protected]>
Signed-off-by: Chi-hsien Lin <[email protected]>
---
.../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
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 <[email protected]>
Signed-off-by: Chi-hsien Lin <[email protected]>
---
.../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
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 <[email protected]>
Signed-off-by: Chi-hsien Lin <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Chi-hsien Lin <[email protected]>
> ---
> 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(+)
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 <[email protected]>
> Signed-off-by: Chi-hsien Lin <[email protected]>
> ---
> .../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
+ 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 <[email protected]>
> Signed-off-by: Chi-hsien Lin <[email protected]>
> ---
> .../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;
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 <[email protected]>
>> Signed-off-by: Chi-hsien Lin <[email protected]>
>> ---
>> .../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
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 <[email protected]>
>> Signed-off-by: Chi-hsien Lin <[email protected]>
>> ---
>> 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(+)
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 <[email protected]>
>> Signed-off-by: Chi-hsien Lin <[email protected]>
>> ---
>> .../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;
>
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 <[email protected]>
>>> Signed-off-by: Chi-hsien Lin <[email protected]>
>>> ---
>>> .../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
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 <[email protected]>
>>>> Signed-off-by: Chi-hsien Lin <[email protected]>
>>>> ---
>>>> .../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
Wright Feng <[email protected]> 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
Wright Feng <[email protected]> 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
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
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
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
On March 25, 2020 3:08:18 PM Tejun Heo <[email protected]> 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
Arend Van Spriel 於 3/25/2020 10:53 PM 寫道:
> On March 25, 2020 3:08:18 PM Tejun Heo <[email protected]> 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
On Wed, Mar 25, 2020 at 03:53:43PM +0100, Arend Van Spriel wrote:
> On March 25, 2020 3:08:18 PM Tejun Heo <[email protected]> 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
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
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
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