2020-09-14 03:17:31

by Wright Feng

[permalink] [raw]
Subject: [PATCH v2 0/3] brcmfmac: SoftAP creation and dcmd buffer size changes

Allow SoftAP creation via vendor command from test tool; also update
dcmd buffer size settings for new firmware.

Changes in v2:
- Fix sparse check error in v1
- Remove one patch due to duplicate
- Add more comment in "brcmfmac: increase dcmd max control buffer
size" patch

Kurt Lee (1):
brcmfmac: set net carrier on via test tool for AP mode

Double Lo (2):
brcmfmac: support virtual interface creation from firmware
brcmfmac: increase dcmd max control buffer size

.../broadcom/brcm80211/brcmfmac/bcdc.c | 4 +-
.../broadcom/brcm80211/brcmfmac/cfg80211.c | 50 +++++++++++++++++--
.../broadcom/brcm80211/brcmfmac/vendor.c | 9 ++++
3 files changed, 58 insertions(+), 5 deletions(-)

--
2.25.0


2020-09-14 03:17:32

by Wright Feng

[permalink] [raw]
Subject: [PATCH v2 1/3] brcmfmac: support virtual interface creation from firmware

From: "Double Lo" <[email protected]>

Allow interface creation via IF_ADD event from firmware.

Signed-off-by: Double Lo <[email protected]>
---
.../broadcom/brcm80211/brcmfmac/cfg80211.c | 50 +++++++++++++++++--
1 file changed, 46 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 5d99771c3f64..c0177e121526 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -230,6 +230,9 @@ struct parsed_vndr_ies {
struct parsed_vndr_ie_info ie_info[VNDR_IE_PARSE_LIMIT];
};

+#define WLC_E_IF_ROLE_STA 0 /* Infra STA */
+#define WLC_E_IF_ROLE_AP 1 /* Access Point */
+
static u8 nl80211_band_to_fwil(enum nl80211_band band)
{
switch (band) {
@@ -5535,8 +5538,10 @@ void brcmf_cfg80211_free_netdev(struct net_device *ndev)
ifp = netdev_priv(ndev);
vif = ifp->vif;

- if (vif)
+ if (vif) {
brcmf_free_vif(vif);
+ ifp->vif = NULL;
+ }
}

static bool brcmf_is_linkup(struct brcmf_cfg80211_vif *vif,
@@ -6102,6 +6107,9 @@ static s32 brcmf_notify_vif_event(struct brcmf_if *ifp,
struct brcmf_if_event *ifevent = (struct brcmf_if_event *)data;
struct brcmf_cfg80211_vif_event *event = &cfg->vif_event;
struct brcmf_cfg80211_vif *vif;
+ enum nl80211_iftype iftype = NL80211_IFTYPE_UNSPECIFIED;
+ bool vif_pend = false;
+ int err;

brcmf_dbg(TRACE, "Enter: action %u flags %u ifidx %u bsscfgidx %u\n",
ifevent->action, ifevent->flags, ifevent->ifidx,
@@ -6114,9 +6122,30 @@ static s32 brcmf_notify_vif_event(struct brcmf_if *ifp,
switch (ifevent->action) {
case BRCMF_E_IF_ADD:
/* waiting process may have timed out */
- if (!cfg->vif_event.vif) {
- spin_unlock(&event->vif_event_lock);
- return -EBADF;
+ if (!vif) {
+ /* handle IF_ADD event from firmware */
+ vif_pend = true;
+ if (ifevent->role == WLC_E_IF_ROLE_STA)
+ iftype = NL80211_IFTYPE_STATION;
+ else if (ifevent->role == WLC_E_IF_ROLE_AP)
+ iftype = NL80211_IFTYPE_AP;
+ else
+ vif_pend = false;
+
+ if (vif_pend) {
+ spin_unlock(&event->vif_event_lock);
+ vif = brcmf_alloc_vif(cfg, iftype);
+ if (IS_ERR(vif)) {
+ brcmf_err("Role:%d failed to alloc vif\n",
+ ifevent->role);
+ return PTR_ERR(vif);
+ }
+ spin_lock(&event->vif_event_lock);
+ } else {
+ brcmf_err("Invalid Role:%d\n", ifevent->role);
+ spin_unlock(&event->vif_event_lock);
+ return -EBADF;
+ }
}

ifp->vif = vif;
@@ -6126,6 +6155,19 @@ static s32 brcmf_notify_vif_event(struct brcmf_if *ifp,
ifp->ndev->ieee80211_ptr = &vif->wdev;
SET_NETDEV_DEV(ifp->ndev, wiphy_dev(cfg->wiphy));
}
+
+ if (vif_pend) {
+ err = brcmf_net_attach(ifp, false);
+ if (err) {
+ brcmf_err("netdevice register failed with err:%d\n",
+ err);
+ brcmf_free_vif(vif);
+ free_netdev(ifp->ndev);
+ }
+ spin_unlock(&event->vif_event_lock);
+ return err;
+ }
+
spin_unlock(&event->vif_event_lock);
wake_up(&event->vif_wq);
return 0;
--
2.25.0

2020-09-14 03:20:13

by Wright Feng

[permalink] [raw]
Subject: [PATCH v2 3/3] brcmfmac: increase dcmd max control buffer size

From: "Double Lo" <[email protected]>

Add the padding round up value in the max control buffer
size to match firmware configuration for new chips.

Signed-off-by: Double Lo <[email protected]>
Signed-off-by: Chi-Hsien Lin <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
index 2c95a08a5871..705130cf27d5 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
@@ -87,6 +87,8 @@ struct brcmf_proto_bcdc_header {
* plus any space that might be needed
* for bus alignment padding.
*/
+#define ROUND_UP_MARGIN 2048
+
struct brcmf_bcdc {
u16 reqid;
u8 bus_header[BUS_HEADER_LEN];
@@ -471,7 +473,7 @@ int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr)

drvr->hdrlen += BCDC_HEADER_LEN + BRCMF_PROT_FW_SIGNAL_MAX_TXBYTES;
drvr->bus_if->maxctl = BRCMF_DCMD_MAXLEN +
- sizeof(struct brcmf_proto_bcdc_dcmd);
+ sizeof(struct brcmf_proto_bcdc_dcmd) + ROUND_UP_MARGIN;
return 0;

fail:
--
2.25.0

2020-09-14 03:20:13

by Wright Feng

[permalink] [raw]
Subject: [PATCH v2 2/3] brcmfmac: set net carrier on via test tool for AP mode

From: Kurt Lee <[email protected]>

In manufacturing line, test tool may be used to enable SoftAP. Such
SoftAP can't pass traffic because netif carrier is off by default. To
allow such use case, let brcmfmac parse ioctl cmd, and then set iftype
to ap mode and report netif_carrier_on to upper layer.

Signed-off-by: Kurt Lee <[email protected]>
Signed-off-by: Chi-Hsien Lin <[email protected]>
---
.../net/wireless/broadcom/brcm80211/brcmfmac/vendor.c | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c
index d07e7c7355d9..5edf5ac1167a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c
@@ -64,6 +64,15 @@ static int brcmf_cfg80211_vndr_cmds_dcmd_handler(struct wiphy *wiphy,
*(char *)(dcmd_buf + len) = '\0';
}

+ if (cmdhdr->cmd == BRCMF_C_SET_AP) {
+ if (*(int *)(dcmd_buf) == 1) {
+ ifp->vif->wdev.iftype = NL80211_IFTYPE_AP;
+ brcmf_net_setcarrier(ifp, true);
+ } else {
+ ifp->vif->wdev.iftype = NL80211_IFTYPE_STATION;
+ }
+ }
+
if (cmdhdr->set)
ret = brcmf_fil_cmd_data_set(ifp, cmdhdr->cmd, dcmd_buf,
ret_len);
--
2.25.0

2020-09-14 08:24:29

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] brcmfmac: set net carrier on via test tool for AP mode

Wright Feng <[email protected]> writes:

> From: Kurt Lee <[email protected]>
>
> In manufacturing line, test tool may be used to enable SoftAP. Such
> SoftAP can't pass traffic because netif carrier is off by default. To
> allow such use case, let brcmfmac parse ioctl cmd, and then set iftype
> to ap mode and report netif_carrier_on to upper layer.

nl80211 does not use ioctl(), so the commit log is misleading.

> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c
> @@ -64,6 +64,15 @@ static int brcmf_cfg80211_vndr_cmds_dcmd_handler(struct wiphy *wiphy,
> *(char *)(dcmd_buf + len) = '\0';
> }
>
> + if (cmdhdr->cmd == BRCMF_C_SET_AP) {
> + if (*(int *)(dcmd_buf) == 1) {
> + ifp->vif->wdev.iftype = NL80211_IFTYPE_AP;
> + brcmf_net_setcarrier(ifp, true);
> + } else {
> + ifp->vif->wdev.iftype = NL80211_IFTYPE_STATION;
> + }
> + }

We now have rules for the vendor API:

https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api

As the BRCMF_VNDR_CMDS_DCMD just seems to be a simple pipe between the
user space and the firmware I'm leaning towards that we should just
remove support for that command from brcmfmac.

And besides, for factory tests you should be using NL80211_CMD_TESTMODE.
The vendor API is meant for normal mode usage.


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

2020-09-14 08:42:45

by Arend van Spriel

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] brcmfmac: set net carrier on via test tool for AP mode

On 9/14/2020 10:20 AM, Kalle Valo wrote:
> Wright Feng <[email protected]> writes:
>
>> From: Kurt Lee <[email protected]>
>>
>> In manufacturing line, test tool may be used to enable SoftAP. Such
>> SoftAP can't pass traffic because netif carrier is off by default. To
>> allow such use case, let brcmfmac parse ioctl cmd, and then set iftype
>> to ap mode and report netif_carrier_on to upper layer.
>
> nl80211 does not use ioctl(), so the commit log is misleading.
>
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c
>> @@ -64,6 +64,15 @@ static int brcmf_cfg80211_vndr_cmds_dcmd_handler(struct wiphy *wiphy,
>> *(char *)(dcmd_buf + len) = '\0';
>> }
>>
>> + if (cmdhdr->cmd == BRCMF_C_SET_AP) {
>> + if (*(int *)(dcmd_buf) == 1) {
>> + ifp->vif->wdev.iftype = NL80211_IFTYPE_AP;
>> + brcmf_net_setcarrier(ifp, true);
>> + } else {
>> + ifp->vif->wdev.iftype = NL80211_IFTYPE_STATION;
>> + }
>> + }
>
> We now have rules for the vendor API:
>
> https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api
>
> As the BRCMF_VNDR_CMDS_DCMD just seems to be a simple pipe between the
> user space and the firmware I'm leaning towards that we should just
> remove support for that command from brcmfmac.
>
> And besides, for factory tests you should be using NL80211_CMD_TESTMODE.
> The vendor API is meant for normal mode usage.

We talked about this API before. This command used to be a TESTMODE
command, but with the introduction of vendor commands in cfg80211 I
changed that. My reasons being that 1) it helps me in cfg80211 callback
development, and 2) (some of) our customers were not accepting the need
for a separate driver (build) for manufacturing. My intent has always
been to keep this opaque for the driver so I am opposed to patches like
this that are interpreting the opaque blob. The firmware could simply
generate a CARRIER event and the driver can act on that.

Regards,
Arend


Attachments:
smime.p7s (4.08 kB)
S/MIME Cryptographic Signature