On Mon, Dec 19, 2022 at 10:53:55AM -0800, Aloka Dixit wrote:
> On 12/18/2022 7:24 AM, Jouni Malinen wrote:
> > On Wed, Sep 15, 2021 at 07:54:35PM -0700, Aloka Dixit wrote:
> > > diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> > > +static int ieee80211_set_ap_mbssid_options(struct ieee80211_sub_if_data *sdata,
> > > + struct cfg80211_mbssid_config params)
> >
> > While that does not really break behavior, why is that params argument
> > passed by value instead of by reference? I see no point in copying
> > struct cfg80211_mbssid_config members for this call since the function
> > is only reading the value.
>
> Hi Jouni, the only reason for value instead of reference is that this
> function does not need to change anything in 'params'. I didn't understand
> your question. Are you suggesting moving the assignments to
> ieee80211_start_ap() instead of this separate function?
I would use a constant pointer (const struct cfg80211_mbssid_config
*params) to avoid the need to copy the full contents of that struct
whenever calling the function.
Maybe the following patch is a clearer way of showing what I was
thinking of (and testing with):
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index e30b2bdb8f01..b0abd99f006e 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -138,7 +138,7 @@ static int ieee80211_set_mon_options(struct ieee80211_sub_if_data *sdata,
}
static int ieee80211_set_ap_mbssid_options(struct ieee80211_sub_if_data *sdata,
- struct cfg80211_mbssid_config params,
+ const struct cfg80211_mbssid_config *params,
struct ieee80211_bss_conf *link_conf)
{
struct ieee80211_sub_if_data *tx_sdata;
@@ -148,10 +148,10 @@ static int ieee80211_set_ap_mbssid_options(struct ieee80211_sub_if_data *sdata,
link_conf->nontransmitted = false;
link_conf->ema_ap = false;
- if (sdata->vif.type != NL80211_IFTYPE_AP || !params.tx_wdev)
+ if (sdata->vif.type != NL80211_IFTYPE_AP || !params->tx_wdev)
return -EINVAL;
- tx_sdata = IEEE80211_WDEV_TO_SUB_IF(params.tx_wdev);
+ tx_sdata = IEEE80211_WDEV_TO_SUB_IF(params->tx_wdev);
if (!tx_sdata)
return -EINVAL;
@@ -160,9 +160,9 @@ static int ieee80211_set_ap_mbssid_options(struct ieee80211_sub_if_data *sdata,
} else {
sdata->vif.mbssid_tx_vif = &tx_sdata->vif;
link_conf->nontransmitted = true;
- link_conf->bssid_index = params.index;
+ link_conf->bssid_index = params->index;
}
- if (params.ema)
+ if (params->ema)
link_conf->ema_ap = true;
return 0;
@@ -1268,10 +1268,17 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
if (sdata->vif.type == NL80211_IFTYPE_AP &&
params->mbssid_config.tx_wdev) {
err = ieee80211_set_ap_mbssid_options(sdata,
- params->mbssid_config,
+ ¶ms->mbssid_config,
link_conf);
if (err)
return err;
+ } else {
+ /* FIX: Is this the correct thing to do here and under which
+ * conditions? At least ema_ap needs to be cleared for AP mode
+ * if mbssid_config.tx_wdev is not set. */
+ link_conf->bssid_index = 0;
+ link_conf->nontransmitted = false;
+ link_conf->ema_ap = false;
}
mutex_lock(&local->mtx);
> > This cleanup is important, but it is done only here in this helper
> > function..
> > And that is the only place where the help function is called and this
> > happens only under the params->mbssid_config.tx_wdev condition. In other
> > words, those bssid_index/nontransmitted/ema_ap values are not cleared in
> > all cases. This results in issue when the bss_conf (link_conf in the
> > current kernel snapshot) is left in the previous mbssid configuration.
> >
>
> Will send a patch to fix this part.
Thanks.
> Please let me know regarding the first question above so that I can include
> that in the same patch.
That should not be in the same patch since it is just
cleanup/optimization while the not clearing parameters in some cases is
a visible bug that should be fixed on its own first.
--
Jouni Malinen PGP id EFC895FA
