diff mbox series

[-next,1/2] brcmfmac: Support multiple AP interfaces and fix STA disconnection issue

Message ID 20220722122956.841786-2-alvin@pqrs.dk
State New
Headers show
Series brcmfmac: AP STA concurrency patches from Cypress/Infineon | expand

Commit Message

Alvin Šipraga July 22, 2022, 12:29 p.m. UTC 
From: Soontak Lee <soontak.lee@cypress.com>

Support multiple AP interfaces for STA + AP + AP usecase.
And fix STA disconnection when deactivating AP interface.

Signed-off-by: Soontak Lee <soontak.lee@cypress.com>
Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 48 +++++++++++++++----
 .../broadcom/brcm80211/brcmfmac/cfg80211.h    |  1 +
 .../broadcom/brcm80211/brcmfmac/common.c      |  5 ++
 3 files changed, 44 insertions(+), 10 deletions(-)

Comments

Franky Lin Aug. 10, 2022, 9:32 p.m. UTC | #1 
On Fri, Jul 22, 2022 at 5:30 AM Alvin Šipraga <alvin@pqrs.dk> wrote:
>
> From: Soontak Lee <soontak.lee@cypress.com>
>
> Support multiple AP interfaces for STA + AP + AP usecase.

AFAIK, Broadcom's fullmac firmware doesn't support such 2AP + 1STA use case.

> And fix STA disconnection when deactivating AP interface.
>
> Signed-off-by: Soontak Lee <soontak.lee@cypress.com>
> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ---
>  .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 48 +++++++++++++++----
>  .../broadcom/brcm80211/brcmfmac/cfg80211.h    |  1 +
>  .../broadcom/brcm80211/brcmfmac/common.c      |  5 ++
>  3 files changed, 44 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index 3ae6779fe153..856fd5516ddf 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -4747,6 +4747,7 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev,
>                   settings->inactivity_timeout);
>         dev_role = ifp->vif->wdev.iftype;
>         mbss = ifp->vif->mbss;
> +       brcmf_dbg(TRACE, "mbss %s\n", mbss ? "enabled" : "disabled");
>
>         /* store current 11d setting */
>         if (brcmf_fil_cmd_int_get(ifp, BRCMF_C_GET_REGULATORY,
> @@ -4961,6 +4962,9 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev,
>         if ((err) && (!mbss)) {
>                 brcmf_set_mpc(ifp, 1);
>                 brcmf_configure_arp_nd_offload(ifp, true);
> +       } else {
> +               cfg->num_softap++;
> +               brcmf_dbg(TRACE, "Num of SoftAP %u\n", cfg->num_softap);
>         }
>         return err;
>  }
> @@ -4975,6 +4979,7 @@ static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, struct net_device *ndev,
>         s32 err;
>         struct brcmf_fil_bss_enable_le bss_enable;
>         struct brcmf_join_params join_params;
> +       s32 apsta = 0;
>
>         brcmf_dbg(TRACE, "Enter\n");
>
> @@ -4983,6 +4988,27 @@ static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, struct net_device *ndev,
>                 /* first to make sure they get processed by fw. */
>                 msleep(400);
>
> +               cfg->num_softap--;
> +
> +               /* Clear bss configuration and SSID */
> +               bss_enable.bsscfgidx = cpu_to_le32(ifp->bsscfgidx);
> +               bss_enable.enable = cpu_to_le32(0);
> +               err = brcmf_fil_iovar_data_set(ifp, "bss", &bss_enable,
> +                                              sizeof(bss_enable));
> +               if (err < 0)
> +                       brcmf_err("bss_enable config failed %d\n", err);
> +
> +               memset(&join_params, 0, sizeof(join_params));
> +               err = brcmf_fil_cmd_data_set(ifp, BRCMF_C_SET_SSID,
> +                                            &join_params, sizeof(join_params));
> +               if (err < 0)
> +                       bphy_err(drvr, "SET SSID error (%d)\n", err);
> +
> +               if (cfg->num_softap) {
> +                       brcmf_dbg(TRACE, "Num of SoftAP %u\n", cfg->num_softap);
> +                       return 0;
> +               }
> +
>                 if (profile->use_fwauth != BIT(BRCMF_PROFILE_FWAUTH_NONE)) {
>                         if (profile->use_fwauth & BIT(BRCMF_PROFILE_FWAUTH_PSK))
>                                 brcmf_set_pmk(ifp, NULL, 0);
> @@ -5000,17 +5026,18 @@ static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, struct net_device *ndev,
>                 if (ifp->bsscfgidx == 0)
>                         brcmf_fil_iovar_int_set(ifp, "closednet", 0);
>
> -               memset(&join_params, 0, sizeof(join_params));
> -               err = brcmf_fil_cmd_data_set(ifp, BRCMF_C_SET_SSID,
> -                                            &join_params, sizeof(join_params));
> -               if (err < 0)
> -                       bphy_err(drvr, "SET SSID error (%d)\n", err);
> -               err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_DOWN, 1);
> -               if (err < 0)
> -                       bphy_err(drvr, "BRCMF_C_DOWN error %d\n", err);
> -               err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_AP, 0);
> +               err = brcmf_fil_iovar_int_get(ifp, "apsta", &apsta);
>                 if (err < 0)
> -                       bphy_err(drvr, "setting AP mode failed %d\n", err);
> +                       brcmf_err("wl apsta failed (%d)\n", err);
> +
> +               if (!apsta) {
> +                       err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_DOWN, 1);
> +                       if (err < 0)
> +                               bphy_err(drvr, "BRCMF_C_DOWN error %d\n", err);
> +                       err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_AP, 0);
> +                       if (err < 0)
> +                               bphy_err(drvr, "Set AP mode error %d\n", err);
> +               }
>                 if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MBSS))
>                         brcmf_fil_iovar_int_set(ifp, "mbss", 0);
>                 brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_REGULATORY,
> @@ -7641,6 +7668,7 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
>
>         cfg->wiphy = wiphy;
>         cfg->pub = drvr;
> +       cfg->num_softap = 0;
>         init_vif_event(&cfg->vif_event);
>         INIT_LIST_HEAD(&cfg->vif_list);
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
> index e90a30808c22..e4ebc2fa6ebb 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
> @@ -371,6 +371,7 @@ struct brcmf_cfg80211_info {
>         struct brcmf_cfg80211_wowl wowl;
>         struct brcmf_pno_info *pno;
>         u8 ac_priority[MAX_8021D_PRIO];
> +       u8 num_softap;
>  };
>
>  /**
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> index fe01da9e620d..83e023a22f9b 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> @@ -303,6 +303,11 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp)
>                 brcmf_dbg(INFO, "CLM version = %s\n", clmver);
>         }
>
> +       /* set apsta */
> +       err = brcmf_fil_iovar_int_set(ifp, "apsta", 1);
> +       if (err)
> +               brcmf_info("failed setting apsta, %d\n", err);
> +

I do not understand why entering apsta mode by default. The mode is
supposed to be enabled only when an AP interface is created in
brcmf_cfg80211_start_ap. I think one of the side effects of apsta mode
is that memory footprint significantly increases. It should remain
disabled for STA only mode (which is the major use case) for better
performance.

Regards,
- Franky

>         /* set mpc */
>         err = brcmf_fil_iovar_int_set(ifp, "mpc", 1);
>         if (err) {
> --
> 2.37.0
>
Alvin Šipraga Aug. 17, 2022, 8:50 a.m. UTC | #2 
Hi Franky,

On Wed, Aug 10, 2022 at 02:32:06PM -0700, Franky Lin wrote:
> On Fri, Jul 22, 2022 at 5:30 AM Alvin Šipraga <alvin@pqrs.dk> wrote:
> >
> > From: Soontak Lee <soontak.lee@cypress.com>
> >
> > Support multiple AP interfaces for STA + AP + AP usecase.
> 
> AFAIK, Broadcom's fullmac firmware doesn't support such 2AP + 1STA use case.

Thanks for the clarification. The series should be ignored by Kalle
then.

> 
> > And fix STA disconnection when deactivating AP interface.
> >
> > Signed-off-by: Soontak Lee <soontak.lee@cypress.com>
> > Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
> > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> > ---
> >  .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 48 +++++++++++++++----
> >  .../broadcom/brcm80211/brcmfmac/cfg80211.h    |  1 +
> >  .../broadcom/brcm80211/brcmfmac/common.c      |  5 ++
> >  3 files changed, 44 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> > index 3ae6779fe153..856fd5516ddf 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> > @@ -4747,6 +4747,7 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev,
> >                   settings->inactivity_timeout);
> >         dev_role = ifp->vif->wdev.iftype;
> >         mbss = ifp->vif->mbss;
> > +       brcmf_dbg(TRACE, "mbss %s\n", mbss ? "enabled" : "disabled");
> >
> >         /* store current 11d setting */
> >         if (brcmf_fil_cmd_int_get(ifp, BRCMF_C_GET_REGULATORY,
> > @@ -4961,6 +4962,9 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev,
> >         if ((err) && (!mbss)) {
> >                 brcmf_set_mpc(ifp, 1);
> >                 brcmf_configure_arp_nd_offload(ifp, true);
> > +       } else {
> > +               cfg->num_softap++;
> > +               brcmf_dbg(TRACE, "Num of SoftAP %u\n", cfg->num_softap);
> >         }
> >         return err;
> >  }
> > @@ -4975,6 +4979,7 @@ static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, struct net_device *ndev,
> >         s32 err;
> >         struct brcmf_fil_bss_enable_le bss_enable;
> >         struct brcmf_join_params join_params;
> > +       s32 apsta = 0;
> >
> >         brcmf_dbg(TRACE, "Enter\n");
> >
> > @@ -4983,6 +4988,27 @@ static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, struct net_device *ndev,
> >                 /* first to make sure they get processed by fw. */
> >                 msleep(400);
> >
> > +               cfg->num_softap--;
> > +
> > +               /* Clear bss configuration and SSID */
> > +               bss_enable.bsscfgidx = cpu_to_le32(ifp->bsscfgidx);
> > +               bss_enable.enable = cpu_to_le32(0);
> > +               err = brcmf_fil_iovar_data_set(ifp, "bss", &bss_enable,
> > +                                              sizeof(bss_enable));
> > +               if (err < 0)
> > +                       brcmf_err("bss_enable config failed %d\n", err);
> > +
> > +               memset(&join_params, 0, sizeof(join_params));
> > +               err = brcmf_fil_cmd_data_set(ifp, BRCMF_C_SET_SSID,
> > +                                            &join_params, sizeof(join_params));
> > +               if (err < 0)
> > +                       bphy_err(drvr, "SET SSID error (%d)\n", err);
> > +
> > +               if (cfg->num_softap) {
> > +                       brcmf_dbg(TRACE, "Num of SoftAP %u\n", cfg->num_softap);
> > +                       return 0;
> > +               }
> > +
> >                 if (profile->use_fwauth != BIT(BRCMF_PROFILE_FWAUTH_NONE)) {
> >                         if (profile->use_fwauth & BIT(BRCMF_PROFILE_FWAUTH_PSK))
> >                                 brcmf_set_pmk(ifp, NULL, 0);
> > @@ -5000,17 +5026,18 @@ static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, struct net_device *ndev,
> >                 if (ifp->bsscfgidx == 0)
> >                         brcmf_fil_iovar_int_set(ifp, "closednet", 0);
> >
> > -               memset(&join_params, 0, sizeof(join_params));
> > -               err = brcmf_fil_cmd_data_set(ifp, BRCMF_C_SET_SSID,
> > -                                            &join_params, sizeof(join_params));
> > -               if (err < 0)
> > -                       bphy_err(drvr, "SET SSID error (%d)\n", err);
> > -               err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_DOWN, 1);
> > -               if (err < 0)
> > -                       bphy_err(drvr, "BRCMF_C_DOWN error %d\n", err);
> > -               err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_AP, 0);
> > +               err = brcmf_fil_iovar_int_get(ifp, "apsta", &apsta);
> >                 if (err < 0)
> > -                       bphy_err(drvr, "setting AP mode failed %d\n", err);
> > +                       brcmf_err("wl apsta failed (%d)\n", err);
> > +
> > +               if (!apsta) {
> > +                       err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_DOWN, 1);
> > +                       if (err < 0)
> > +                               bphy_err(drvr, "BRCMF_C_DOWN error %d\n", err);
> > +                       err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_AP, 0);
> > +                       if (err < 0)
> > +                               bphy_err(drvr, "Set AP mode error %d\n", err);
> > +               }
> >                 if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MBSS))
> >                         brcmf_fil_iovar_int_set(ifp, "mbss", 0);
> >                 brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_REGULATORY,
> > @@ -7641,6 +7668,7 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
> >
> >         cfg->wiphy = wiphy;
> >         cfg->pub = drvr;
> > +       cfg->num_softap = 0;
> >         init_vif_event(&cfg->vif_event);
> >         INIT_LIST_HEAD(&cfg->vif_list);
> >
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
> > index e90a30808c22..e4ebc2fa6ebb 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
> > @@ -371,6 +371,7 @@ struct brcmf_cfg80211_info {
> >         struct brcmf_cfg80211_wowl wowl;
> >         struct brcmf_pno_info *pno;
> >         u8 ac_priority[MAX_8021D_PRIO];
> > +       u8 num_softap;
> >  };
> >
> >  /**
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> > index fe01da9e620d..83e023a22f9b 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> > @@ -303,6 +303,11 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp)
> >                 brcmf_dbg(INFO, "CLM version = %s\n", clmver);
> >         }
> >
> > +       /* set apsta */
> > +       err = brcmf_fil_iovar_int_set(ifp, "apsta", 1);
> > +       if (err)
> > +               brcmf_info("failed setting apsta, %d\n", err);
> > +
> 
> I do not understand why entering apsta mode by default. The mode is
> supposed to be enabled only when an AP interface is created in
> brcmf_cfg80211_start_ap. I think one of the side effects of apsta mode
> is that memory footprint significantly increases. It should remain
> disabled for STA only mode (which is the major use case) for better
> performance.

By better performance, do you just mean "lower chance of memory
exhaustion"? If so, surely the firmware would be designed such that it
doesn't run out of memory under the advertised use-cases (STA, AP+STA
etc.), regardless of the current apsta setting?

I would hope that somebody from Infineon could chime in here, as I am
not privy to the FW design.

Kind regards,
Alvin

> 
> Regards,
> - Franky
> 
> >         /* set mpc */
> >         err = brcmf_fil_iovar_int_set(ifp, "mpc", 1);
> >         if (err) {
> > --
> > 2.37.0
> >
> 
> -- 
> This electronic communication and the information and any files transmitted 
> with it, or attached to it, are confidential and are intended solely for 
> the use of the individual or entity to whom it is addressed and may contain 
> information that is confidential, legally privileged, protected by privacy 
> laws, or otherwise restricted from disclosure to anyone else. If you are 
> not the intended recipient or the person responsible for delivering the 
> e-mail to the intended recipient, you are hereby notified that any use, 
> copying, distributing, dissemination, forwarding, printing, or copying of 
> this e-mail is strictly prohibited. If you received this e-mail in error, 
> please return the e-mail to the sender, delete it from your computer, and 
> destroy any printed copy of it.
Franky Lin Aug. 22, 2022, 5:12 p.m. UTC | #3 
Hi Alvin,

On Wed, Aug 17, 2022 at 1:50 AM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote:
>
> Hi Franky,
>
> On Wed, Aug 10, 2022 at 02:32:06PM -0700, Franky Lin wrote:
> > On Fri, Jul 22, 2022 at 5:30 AM Alvin Šipraga <alvin@pqrs.dk> wrote:
> > >
> > > From: Soontak Lee <soontak.lee@cypress.com>
> > >
> > > Support multiple AP interfaces for STA + AP + AP usecase.
> >
> > AFAIK, Broadcom's fullmac firmware doesn't support such 2AP + 1STA use case.
>
> Thanks for the clarification. The series should be ignored by Kalle
> then.
>
> >
> > > And fix STA disconnection when deactivating AP interface.
> > >
> > > Signed-off-by: Soontak Lee <soontak.lee@cypress.com>
> > > Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
> > > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > > Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> > > ---
> > >  .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 48 +++++++++++++++----
> > >  .../broadcom/brcm80211/brcmfmac/cfg80211.h    |  1 +
> > >  .../broadcom/brcm80211/brcmfmac/common.c      |  5 ++
> > >  3 files changed, 44 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> > > index 3ae6779fe153..856fd5516ddf 100644
> > > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> > > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> > > @@ -4747,6 +4747,7 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev,
> > >                   settings->inactivity_timeout);
> > >         dev_role = ifp->vif->wdev.iftype;
> > >         mbss = ifp->vif->mbss;
> > > +       brcmf_dbg(TRACE, "mbss %s\n", mbss ? "enabled" : "disabled");
> > >
> > >         /* store current 11d setting */
> > >         if (brcmf_fil_cmd_int_get(ifp, BRCMF_C_GET_REGULATORY,
> > > @@ -4961,6 +4962,9 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev,
> > >         if ((err) && (!mbss)) {
> > >                 brcmf_set_mpc(ifp, 1);
> > >                 brcmf_configure_arp_nd_offload(ifp, true);
> > > +       } else {
> > > +               cfg->num_softap++;
> > > +               brcmf_dbg(TRACE, "Num of SoftAP %u\n", cfg->num_softap);
> > >         }
> > >         return err;
> > >  }
> > > @@ -4975,6 +4979,7 @@ static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, struct net_device *ndev,
> > >         s32 err;
> > >         struct brcmf_fil_bss_enable_le bss_enable;
> > >         struct brcmf_join_params join_params;
> > > +       s32 apsta = 0;
> > >
> > >         brcmf_dbg(TRACE, "Enter\n");
> > >
> > > @@ -4983,6 +4988,27 @@ static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, struct net_device *ndev,
> > >                 /* first to make sure they get processed by fw. */
> > >                 msleep(400);
> > >
> > > +               cfg->num_softap--;
> > > +
> > > +               /* Clear bss configuration and SSID */
> > > +               bss_enable.bsscfgidx = cpu_to_le32(ifp->bsscfgidx);
> > > +               bss_enable.enable = cpu_to_le32(0);
> > > +               err = brcmf_fil_iovar_data_set(ifp, "bss", &bss_enable,
> > > +                                              sizeof(bss_enable));
> > > +               if (err < 0)
> > > +                       brcmf_err("bss_enable config failed %d\n", err);
> > > +
> > > +               memset(&join_params, 0, sizeof(join_params));
> > > +               err = brcmf_fil_cmd_data_set(ifp, BRCMF_C_SET_SSID,
> > > +                                            &join_params, sizeof(join_params));
> > > +               if (err < 0)
> > > +                       bphy_err(drvr, "SET SSID error (%d)\n", err);
> > > +
> > > +               if (cfg->num_softap) {
> > > +                       brcmf_dbg(TRACE, "Num of SoftAP %u\n", cfg->num_softap);
> > > +                       return 0;
> > > +               }
> > > +
> > >                 if (profile->use_fwauth != BIT(BRCMF_PROFILE_FWAUTH_NONE)) {
> > >                         if (profile->use_fwauth & BIT(BRCMF_PROFILE_FWAUTH_PSK))
> > >                                 brcmf_set_pmk(ifp, NULL, 0);
> > > @@ -5000,17 +5026,18 @@ static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, struct net_device *ndev,
> > >                 if (ifp->bsscfgidx == 0)
> > >                         brcmf_fil_iovar_int_set(ifp, "closednet", 0);
> > >
> > > -               memset(&join_params, 0, sizeof(join_params));
> > > -               err = brcmf_fil_cmd_data_set(ifp, BRCMF_C_SET_SSID,
> > > -                                            &join_params, sizeof(join_params));
> > > -               if (err < 0)
> > > -                       bphy_err(drvr, "SET SSID error (%d)\n", err);
> > > -               err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_DOWN, 1);
> > > -               if (err < 0)
> > > -                       bphy_err(drvr, "BRCMF_C_DOWN error %d\n", err);
> > > -               err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_AP, 0);
> > > +               err = brcmf_fil_iovar_int_get(ifp, "apsta", &apsta);
> > >                 if (err < 0)
> > > -                       bphy_err(drvr, "setting AP mode failed %d\n", err);
> > > +                       brcmf_err("wl apsta failed (%d)\n", err);
> > > +
> > > +               if (!apsta) {
> > > +                       err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_DOWN, 1);
> > > +                       if (err < 0)
> > > +                               bphy_err(drvr, "BRCMF_C_DOWN error %d\n", err);
> > > +                       err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_AP, 0);
> > > +                       if (err < 0)
> > > +                               bphy_err(drvr, "Set AP mode error %d\n", err);
> > > +               }
> > >                 if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MBSS))
> > >                         brcmf_fil_iovar_int_set(ifp, "mbss", 0);
> > >                 brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_REGULATORY,
> > > @@ -7641,6 +7668,7 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
> > >
> > >         cfg->wiphy = wiphy;
> > >         cfg->pub = drvr;
> > > +       cfg->num_softap = 0;
> > >         init_vif_event(&cfg->vif_event);
> > >         INIT_LIST_HEAD(&cfg->vif_list);
> > >
> > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
> > > index e90a30808c22..e4ebc2fa6ebb 100644
> > > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
> > > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
> > > @@ -371,6 +371,7 @@ struct brcmf_cfg80211_info {
> > >         struct brcmf_cfg80211_wowl wowl;
> > >         struct brcmf_pno_info *pno;
> > >         u8 ac_priority[MAX_8021D_PRIO];
> > > +       u8 num_softap;
> > >  };
> > >
> > >  /**
> > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> > > index fe01da9e620d..83e023a22f9b 100644
> > > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> > > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> > > @@ -303,6 +303,11 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp)
> > >                 brcmf_dbg(INFO, "CLM version = %s\n", clmver);
> > >         }
> > >
> > > +       /* set apsta */
> > > +       err = brcmf_fil_iovar_int_set(ifp, "apsta", 1);
> > > +       if (err)
> > > +               brcmf_info("failed setting apsta, %d\n", err);
> > > +
> >
> > I do not understand why entering apsta mode by default. The mode is
> > supposed to be enabled only when an AP interface is created in
> > brcmf_cfg80211_start_ap. I think one of the side effects of apsta mode
> > is that memory footprint significantly increases. It should remain
> > disabled for STA only mode (which is the major use case) for better
> > performance.
>
> By better performance, do you just mean "lower chance of memory
> exhaustion"? If so, surely the firmware would be designed such that it
> doesn't run out of memory under the advertised use-cases (STA, AP+STA
> etc.), regardless of the current apsta setting?

I think some packet related buffers will be adjusted for apsta mode so
the sta mode performance will hurt because there is less buffer to
use.

Another significant impact I am sure about is some power saving
features will be turned off once apsta mode is enabled. So the chip
will drain more power even the AP interface is not created.

Regards,
- Franky


>
> I would hope that somebody from Infineon could chime in here, as I am
> not privy to the FW design.
>
> Kind regards,
> Alvin
>
> >
> > Regards,
> > - Franky
> >
> > >         /* set mpc */
> > >         err = brcmf_fil_iovar_int_set(ifp, "mpc", 1);
> > >         if (err) {
> > > --
> > > 2.37.0
> > >
> >
> > --
> > This electronic communication and the information and any files transmitted
> > with it, or attached to it, are confidential and are intended solely for
> > the use of the individual or entity to whom it is addressed and may contain
> > information that is confidential, legally privileged, protected by privacy
> > laws, or otherwise restricted from disclosure to anyone else. If you are
> > not the intended recipient or the person responsible for delivering the
> > e-mail to the intended recipient, you are hereby notified that any use,
> > copying, distributing, dissemination, forwarding, printing, or copying of
> > this e-mail is strictly prohibited. If you received this e-mail in error,
> > please return the e-mail to the sender, delete it from your computer, and
> > destroy any printed copy of it.
Alvin Šipraga Aug. 23, 2022, 9:03 a.m. UTC | #4 
Hi Franky,

On Mon, Aug 22, 2022 at 10:12:14AM -0700, Franky Lin wrote:
> On Wed, Aug 17, 2022 at 1:50 AM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote:
> > On Wed, Aug 10, 2022 at 02:32:06PM -0700, Franky Lin wrote:
> > > On Fri, Jul 22, 2022 at 5:30 AM Alvin Šipraga <alvin@pqrs.dk> wrote:
> > > >  /**
> > > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> > > > index fe01da9e620d..83e023a22f9b 100644
> > > > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> > > > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> > > > @@ -303,6 +303,11 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp)
> > > >                 brcmf_dbg(INFO, "CLM version = %s\n", clmver);
> > > >         }
> > > >
> > > > +       /* set apsta */
> > > > +       err = brcmf_fil_iovar_int_set(ifp, "apsta", 1);
> > > > +       if (err)
> > > > +               brcmf_info("failed setting apsta, %d\n", err);
> > > > +
> > >
> > > I do not understand why entering apsta mode by default. The mode is
> > > supposed to be enabled only when an AP interface is created in
> > > brcmf_cfg80211_start_ap. I think one of the side effects of apsta mode
> > > is that memory footprint significantly increases. It should remain
> > > disabled for STA only mode (which is the major use case) for better
> > > performance.
> >
> > By better performance, do you just mean "lower chance of memory
> > exhaustion"? If so, surely the firmware would be designed such that it
> > doesn't run out of memory under the advertised use-cases (STA, AP+STA
> > etc.), regardless of the current apsta setting?
> 
> I think some packet related buffers will be adjusted for apsta mode so
> the sta mode performance will hurt because there is less buffer to
> use.
> 
> Another significant impact I am sure about is some power saving
> features will be turned off once apsta mode is enabled. So the chip
> will drain more power even the AP interface is not created.

OK. And this apsta mode seems only to be used for STA + P2P mode in the
upstream driver's current form. But doesn't the driver also support AP +
STA mode ordinarily? Would apsta=1 not be necessary for that use-case?
Of course I assume you can only answer me for Broadcom chipsets.

Trying to understand whether to drop this whole series or whether a
modified version can be suitably upstreamed.

Kind regards,
Alvin

> 
> Regards,
> - Franky
diff mbox series

Patch

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 3ae6779fe153..856fd5516ddf 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -4747,6 +4747,7 @@  brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev,
 		  settings->inactivity_timeout);
 	dev_role = ifp->vif->wdev.iftype;
 	mbss = ifp->vif->mbss;
+	brcmf_dbg(TRACE, "mbss %s\n", mbss ? "enabled" : "disabled");
 
 	/* store current 11d setting */
 	if (brcmf_fil_cmd_int_get(ifp, BRCMF_C_GET_REGULATORY,
@@ -4961,6 +4962,9 @@  brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev,
 	if ((err) && (!mbss)) {
 		brcmf_set_mpc(ifp, 1);
 		brcmf_configure_arp_nd_offload(ifp, true);
+	} else {
+		cfg->num_softap++;
+		brcmf_dbg(TRACE, "Num of SoftAP %u\n", cfg->num_softap);
 	}
 	return err;
 }
@@ -4975,6 +4979,7 @@  static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, struct net_device *ndev,
 	s32 err;
 	struct brcmf_fil_bss_enable_le bss_enable;
 	struct brcmf_join_params join_params;
+	s32 apsta = 0;
 
 	brcmf_dbg(TRACE, "Enter\n");
 
@@ -4983,6 +4988,27 @@  static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, struct net_device *ndev,
 		/* first to make sure they get processed by fw. */
 		msleep(400);
 
+		cfg->num_softap--;
+
+		/* Clear bss configuration and SSID */
+		bss_enable.bsscfgidx = cpu_to_le32(ifp->bsscfgidx);
+		bss_enable.enable = cpu_to_le32(0);
+		err = brcmf_fil_iovar_data_set(ifp, "bss", &bss_enable,
+					       sizeof(bss_enable));
+		if (err < 0)
+			brcmf_err("bss_enable config failed %d\n", err);
+
+		memset(&join_params, 0, sizeof(join_params));
+		err = brcmf_fil_cmd_data_set(ifp, BRCMF_C_SET_SSID,
+					     &join_params, sizeof(join_params));
+		if (err < 0)
+			bphy_err(drvr, "SET SSID error (%d)\n", err);
+
+		if (cfg->num_softap) {
+			brcmf_dbg(TRACE, "Num of SoftAP %u\n", cfg->num_softap);
+			return 0;
+		}
+
 		if (profile->use_fwauth != BIT(BRCMF_PROFILE_FWAUTH_NONE)) {
 			if (profile->use_fwauth & BIT(BRCMF_PROFILE_FWAUTH_PSK))
 				brcmf_set_pmk(ifp, NULL, 0);
@@ -5000,17 +5026,18 @@  static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, struct net_device *ndev,
 		if (ifp->bsscfgidx == 0)
 			brcmf_fil_iovar_int_set(ifp, "closednet", 0);
 
-		memset(&join_params, 0, sizeof(join_params));
-		err = brcmf_fil_cmd_data_set(ifp, BRCMF_C_SET_SSID,
-					     &join_params, sizeof(join_params));
-		if (err < 0)
-			bphy_err(drvr, "SET SSID error (%d)\n", err);
-		err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_DOWN, 1);
-		if (err < 0)
-			bphy_err(drvr, "BRCMF_C_DOWN error %d\n", err);
-		err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_AP, 0);
+		err = brcmf_fil_iovar_int_get(ifp, "apsta", &apsta);
 		if (err < 0)
-			bphy_err(drvr, "setting AP mode failed %d\n", err);
+			brcmf_err("wl apsta failed (%d)\n", err);
+
+		if (!apsta) {
+			err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_DOWN, 1);
+			if (err < 0)
+				bphy_err(drvr, "BRCMF_C_DOWN error %d\n", err);
+			err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_AP, 0);
+			if (err < 0)
+				bphy_err(drvr, "Set AP mode error %d\n", err);
+		}
 		if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MBSS))
 			brcmf_fil_iovar_int_set(ifp, "mbss", 0);
 		brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_REGULATORY,
@@ -7641,6 +7668,7 @@  struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
 
 	cfg->wiphy = wiphy;
 	cfg->pub = drvr;
+	cfg->num_softap = 0;
 	init_vif_event(&cfg->vif_event);
 	INIT_LIST_HEAD(&cfg->vif_list);
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
index e90a30808c22..e4ebc2fa6ebb 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
@@ -371,6 +371,7 @@  struct brcmf_cfg80211_info {
 	struct brcmf_cfg80211_wowl wowl;
 	struct brcmf_pno_info *pno;
 	u8 ac_priority[MAX_8021D_PRIO];
+	u8 num_softap;
 };
 
 /**
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index fe01da9e620d..83e023a22f9b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -303,6 +303,11 @@  int brcmf_c_preinit_dcmds(struct brcmf_if *ifp)
 		brcmf_dbg(INFO, "CLM version = %s\n", clmver);
 	}
 
+	/* set apsta */
+	err = brcmf_fil_iovar_int_set(ifp, "apsta", 1);
+	if (err)
+		brcmf_info("failed setting apsta, %d\n", err);
+
 	/* set mpc */
 	err = brcmf_fil_iovar_int_set(ifp, "mpc", 1);
 	if (err) {