diff mbox series

[3/4] brcmfmac: set net carrier on via test tool for AP mode

Message ID 20200519110951.88998-4-chi-hsien.lin@cypress.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series brcmfmac: SoftAP creation and dcmd buffer size changes | expand

Commit Message

Chi-Hsien Lin May 19, 2020, 11:09 a.m. UTC
From: Kurt Lee <kurt.lee@cypress.com>

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 <kurt.lee@cypress.com>
Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
---
 .../net/wireless/broadcom/brcm80211/brcmfmac/vendor.c    | 9 +++++++++
 1 file changed, 9 insertions(+)

--
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.

Comments

Arend van Spriel May 19, 2020, 11:18 a.m. UTC | #1
On 5/19/2020 1:09 PM, Chi-Hsien Lin wrote:
> From: Kurt Lee <kurt.lee@cypress.com>
> 
> 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 <kurt.lee@cypress.com>
> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
> ---
>   .../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
Chi-Hsien Lin May 21, 2020, 9:27 a.m. UTC | #2
On 05/19/2020 7:18, Arend Van Spriel wrote:
> On 5/19/2020 1:09 PM, Chi-Hsien Lin wrote:
>> From: Kurt Lee <kurt.lee@cypress.com>
>>
>> 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 <kurt.lee@cypress.com>
>> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
>> ---
>>    .../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
>
diff mbox series

Patch

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);