2020-05-19 11:12:33

by Chi-Hsien Lin

[permalink] [raw]
Subject: [PATCH 3/4] 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


This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.


2020-05-19 11:19:49

by Arend van Spriel

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

On 5/19/2020 1:09 PM, Chi-Hsien Lin wrote:
> 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;
> + }
> + }
> +

I prefer to keep this code path as flat as possible so I prefer no
monitoring of firmware commands issued by user-space. Maybe another
approach would be to set the carrier on a firmware event?

Regards,
Arend

2020-05-21 09:28:32

by Chi-Hsien Lin

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



On 05/19/2020 7:18, Arend Van Spriel wrote:
> On 5/19/2020 1:09 PM, Chi-Hsien Lin wrote:
>> 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;
>> + }
>> + }
>> +
>
> I prefer to keep this code path as flat as possible so I prefer no
> monitoring of firmware commands issued by user-space. Maybe another
> approach would be to set the carrier on a firmware event?

Event based implementation would be like below:
1. BRCMF_E_IF -> brcmf_fweh_handle_if_event: Change iftype to STA or AP
2. BRCMF_E_LINK -> brcmf_notify_connect_status: Set net carrier on or
off only in AP mode

Such mechanism could run into race conditions when driver init (triggers
BRCMF_E_IF with STA role) and start_ap (triggers BRCMF_E_IF with AP
role) happens close enough. If hostapd sets key before BRCMF_E_IF for AP
role comes back (iftype is still NL80211_IFTYPE_STATION),
nl80211_key_allowed() fails with -NOLINK. This is observed when start_ap
happens right after insmod, or when hostapd disable/enable happens.


>
> Regards,
> Arend
>