| Message ID | 20231127191952.123587-1-Chaitanya.Tata@nordicsemi.no |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [1/2] De-couple MBO and WNM | expand |
On Tue, Nov 28, 2023 at 12:49:52AM +0530, Chaitanya Tata wrote: > MBO is mandatory for WFA certification but WNM is not, so, in order to > have a lean build for WFA certification only, de-couple WNM from MBO so > that it can be compiled out. > > WNM is still auto-selected automatically when MBO is enabled, this > behaviour is unchanged. I'm not sure what this is trying to achieve.. As far as the claim about WNM not being needed for MBO is concerned, the Wi-Fi Agile Multiband specification mandate STA to support WNM-Notification Request frame. > diff --git a/wpa_supplicant/mbo.c b/wpa_supplicant/mbo.c > +#ifdef CONFIG_WNM > static void wpas_mbo_send_wnm_notification(struct wpa_supplicant *wpa_s, > const u8 *data, size_t len) And as far as adding CONFIG_WNM in mbo.c is concerned, this would have no impact since this file is included only if CONFIG_MBO=y is defined and if CONFIG_MBO is defined, then so will CONFIG_WNM.
On Wed, Dec 6, 2023 at 4:16 PM Jouni Malinen <j@w1.fi> wrote: > > On Tue, Nov 28, 2023 at 12:49:52AM +0530, Chaitanya Tata wrote: > > MBO is mandatory for WFA certification but WNM is not, so, in order to > > have a lean build for WFA certification only, de-couple WNM from MBO so > > that it can be compiled out. > > > > WNM is still auto-selected automatically when MBO is enabled, this > > behaviour is unchanged. > > I'm not sure what this is trying to achieve.. As far as the claim about > WNM not being needed for MBO is concerned, the Wi-Fi Agile Multiband > specification mandate STA to support WNM-Notification Request frame. The idea is to only include the support for that Notification request frame and disable the rest of WNM to save memory (wnm_sta.o is ~24K text). > > > diff --git a/wpa_supplicant/mbo.c b/wpa_supplicant/mbo.c > > +#ifdef CONFIG_WNM > > static void wpas_mbo_send_wnm_notification(struct wpa_supplicant *wpa_s, > > const u8 *data, size_t len) > > And as far as adding CONFIG_WNM in mbo.c is concerned, this would have > no impact since this file is included only if CONFIG_MBO=y is defined > and if CONFIG_MBO is defined, then so will CONFIG_WNM. Yes, I don't want to change this behaviour, but if someone wants to save memory then WNM can be disabled and MBO would still work. Of course, this patch is wrong, I will push a v2 that properly fixes the issue.
On Sun, Dec 10, 2023 at 12:21 AM Krishna Chaitanya <chaitanya.mgit@gmail.com> wrote: > > On Wed, Dec 6, 2023 at 4:16 PM Jouni Malinen <j@w1.fi> wrote: > > > > On Tue, Nov 28, 2023 at 12:49:52AM +0530, Chaitanya Tata wrote: > > > MBO is mandatory for WFA certification but WNM is not, so, in order to > > > have a lean build for WFA certification only, de-couple WNM from MBO so > > > that it can be compiled out. > > > > > > WNM is still auto-selected automatically when MBO is enabled, this > > > behaviour is unchanged. > > > > I'm not sure what this is trying to achieve.. As far as the claim about > > WNM not being needed for MBO is concerned, the Wi-Fi Agile Multiband > > specification mandate STA to support WNM-Notification Request frame. > The idea is to only include the support for that Notification request frame > and disable the rest of WNM to save memory (wnm_sta.o is ~24K text). > > > > > diff --git a/wpa_supplicant/mbo.c b/wpa_supplicant/mbo.c > > > +#ifdef CONFIG_WNM > > > static void wpas_mbo_send_wnm_notification(struct wpa_supplicant *wpa_s, > > > const u8 *data, size_t len) > > > > And as far as adding CONFIG_WNM in mbo.c is concerned, this would have > > no impact since this file is included only if CONFIG_MBO=y is defined > > and if CONFIG_MBO is defined, then so will CONFIG_WNM. > Yes, I don't want to change this behaviour, but if someone wants to save > memory then WNM can be disabled and MBO would still work. > > Of course, this patch is wrong, I will push a v2 that properly fixes the issue. Looks like, we need to separate out BTM code to a separate file btm.c and include it for both MBO and WNM (NEED_BTM=y). This way we can get rid of rest of the WNM. WDYT?
On Sun, Dec 10, 2023 at 12:52:04AM +0530, Krishna Chaitanya wrote: > Looks like, we need to separate out BTM code to a separate file btm.c > and include > it for both MBO and WNM (NEED_BTM=y). This way we can get rid of rest of the > WNM. WDYT? It does not have to be moved to a separate file, but it would seem reasonable to be able to pick a subset of wnm_sta.c when CONFIG_MBO=y without having to force CONFIG_WNM=y. For example, wnm_sta.c could be included into the build if either CONFIG_WNM=y or CONFIG_MBO=y and then within wnm_sta.c, some of the functions would be included based on CONFIG_WNM and some base on either CONFIG_WNM or CONFIG_MBO. And well, there are already couple of parts there using CONFIG_MBO on its own.
On Sun, Dec 10, 2023 at 2:09 PM Jouni Malinen <j@w1.fi> wrote: > > On Sun, Dec 10, 2023 at 12:52:04AM +0530, Krishna Chaitanya wrote: > > Looks like, we need to separate out BTM code to a separate file btm.c > > and include > > it for both MBO and WNM (NEED_BTM=y). This way we can get rid of rest of the > > WNM. WDYT? > > It does not have to be moved to a separate file, but it would seem > reasonable to be able to pick a subset of wnm_sta.c when CONFIG_MBO=y > without having to force CONFIG_WNM=y. For example, wnm_sta.c could be > included into the build if either CONFIG_WNM=y or CONFIG_MBO=y and then > within wnm_sta.c, some of the functions would be included based on > CONFIG_WNM and some base on either CONFIG_WNM or CONFIG_MBO. And well, > there are already couple of parts there using CONFIG_MBO on its own. Thanks. Yes, I started doing the same, except I moved all BTM functions to btm.c to make it clearer, but yeah they can be in wnm_sta.c itself.
diff --git a/wpa_supplicant/mbo.c b/wpa_supplicant/mbo.c index 59b15daf6..bc30145a9 100644 --- a/wpa_supplicant/mbo.c +++ b/wpa_supplicant/mbo.c @@ -271,6 +271,7 @@ int wpas_mbo_ie(struct wpa_supplicant *wpa_s, u8 *buf, size_t len, } +#ifdef CONFIG_WNM static void wpas_mbo_send_wnm_notification(struct wpa_supplicant *wpa_s, const u8 *data, size_t len) { @@ -309,6 +310,7 @@ static void wpas_mbo_send_wnm_notification(struct wpa_supplicant *wpa_s, wpabuf_free(buf); } +#endif /* CONFIG_WNM */ static void wpas_mbo_non_pref_chan_changed(struct wpa_supplicant *wpa_s) @@ -320,8 +322,10 @@ static void wpas_mbo_non_pref_chan_changed(struct wpa_supplicant *wpa_s) return; wpas_mbo_non_pref_chan_attrs(wpa_s, buf, 1); +#ifdef CONFIG_WNM wpas_mbo_send_wnm_notification(wpa_s, wpabuf_head_u8(buf), wpabuf_len(buf)); +#endif /* CONFIG_WNM */ wpas_update_mbo_connect_params(wpa_s); wpabuf_free(buf); } @@ -512,6 +516,7 @@ void wpas_mbo_ie_trans_req(struct wpa_supplicant *wpa_s, const u8 *mbo_ie, wpa_printf(MSG_DEBUG, "MBO: Station does not support Cellular data connection"); break; +#ifdef CONFIG_WNM case MBO_ATTR_ID_TRANSITION_REASON: if (elen != 1) goto fail; @@ -540,6 +545,7 @@ void wpas_mbo_ie_trans_req(struct wpa_supplicant *wpa_s, const u8 *mbo_ie, } break; +#endif /* CONFIG_WNM */ case MBO_ATTR_ID_AP_CAPA_IND: case MBO_ATTR_ID_NON_PREF_CHAN_REPORT: case MBO_ATTR_ID_CELL_DATA_CAPA: @@ -563,9 +569,11 @@ void wpas_mbo_ie_trans_req(struct wpa_supplicant *wpa_s, const u8 *mbo_ie, wpa_msg(wpa_s, MSG_INFO, MBO_CELL_PREFERENCE "preference=%u", *cell_pref); +#ifdef CONFIG_WNM if (wpa_s->wnm_mbo_trans_reason_present) wpa_msg(wpa_s, MSG_INFO, MBO_TRANSITION_REASON "reason=%u", wpa_s->wnm_mbo_transition_reason); +#endif if (disallowed_sec && wpa_s->current_bss) wpa_bss_tmp_disallow(wpa_s, wpa_s->current_bss->bssid, @@ -611,7 +619,9 @@ void wpas_mbo_update_cell_capa(struct wpa_supplicant *wpa_s, u8 mbo_cell_capa) cell_capa[5] = MBO_ATTR_ID_CELL_DATA_CAPA; cell_capa[6] = mbo_cell_capa; +#ifdef CONFIG_WNM wpas_mbo_send_wnm_notification(wpa_s, cell_capa, 7); +#endif /* CONFIG_WNM */ wpa_supplicant_set_default_scan_ies(wpa_s); wpas_update_mbo_connect_params(wpa_s); }
MBO is mandatory for WFA certification but WNM is not, so, in order to have a lean build for WFA certification only, de-couple WNM from MBO so that it can be compiled out. WNM is still auto-selected automatically when MBO is enabled, this behaviour is unchanged. Signed-off-by: Chaitanya Tata <Chaitanya.Tata@nordicsemi.no> --- wpa_supplicant/mbo.c | 10 ++++++++++ 1 file changed, 10 insertions(+)