diff mbox series

[1/2] De-couple MBO and WNM

Message ID 20231127191952.123587-1-Chaitanya.Tata@nordicsemi.no
State Rejected
Headers show
Series [1/2] De-couple MBO and WNM | expand

Commit Message

Chaitanya Tata Nov. 27, 2023, 7:19 p.m. UTC
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(+)

Comments

Jouni Malinen Dec. 6, 2023, 10:45 a.m. UTC | #1
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.
Chaitanya Tata Dec. 9, 2023, 6:51 p.m. UTC | #2
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.
Chaitanya Tata Dec. 9, 2023, 7:22 p.m. UTC | #3
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?
Jouni Malinen Dec. 10, 2023, 8:39 a.m. UTC | #4
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.
Chaitanya Tata Dec. 10, 2023, 10:21 a.m. UTC | #5
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 mbox series

Patch

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