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

[Franky Lin <franky.lin@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 10690 bytes --]

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.

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4203 bytes --]