Message ID | 1407514672-6231-1-git-send-email-jtheou@adeneo-embedded.us |
---|---|
State | Rejected |
Headers | show |
Dear Jean-Baptiste Theou, On Fri, 8 Aug 2014 09:17:52 -0700, Jean-Baptiste Theou wrote: > When you select wpa_supplicant, having an visual indication about the > support or not of NL80211 is important. > > And even if libnl is available, you may want to disable the support of > NL80211 inside wpa_supplicant. You forgot your Signed-off-by here. But we anyway have more comments below. > +config BR2_PACKAGE_WPA_SUPPLICANT_NL80211 > + bool "Enable NL80211" > + default y if BR2_PACKAGE_LIBNL > + select BR2_PACKAGE_LIBNL > + help > + Enable support for NL80211. In which cases would you want to *not* have NL80211 support if you already have libnl enabled? What is the reason/use-case? If the NL80211 support in wpa_supplicant itself adds a significant binary size to the filesystem, then it might be useful to have an option such as the one you propose. But in this case, you should include some numbers in your commit log. Also, your patch keeps the automatic dependency handling of libnl, and adds explicitly dependency handling on top of it, which doesn't make a lot of sense: it should really be either one or the other solution, not both. You can also see slide 146 and following of http://free-electrons.com/doc/training/buildroot/buildroot-slides.pdf for more details about automatic or explicit handling of optional dependencies. In the mean time, we'll mark your patch as Rejected in patchwork. Don't hesitate to resubmit a patch with a better explanation, and a cleanup of automatic vs. explicit handling of the libnl dependency. Thanks! Thomas
On 12/07/2015 19:25, Thomas Petazzoni wrote: > Dear Jean-Baptiste Theou, > > On Fri, 8 Aug 2014 09:17:52 -0700, Jean-Baptiste Theou wrote: >> When you select wpa_supplicant, having an visual indication about the >> support or not of NL80211 is important. >> >> And even if libnl is available, you may want to disable the support of >> NL80211 inside wpa_supplicant. > > You forgot your Signed-off-by here. But we anyway have more comments > below. > >> +config BR2_PACKAGE_WPA_SUPPLICANT_NL80211 >> + bool "Enable NL80211" >> + default y if BR2_PACKAGE_LIBNL >> + select BR2_PACKAGE_LIBNL >> + help >> + Enable support for NL80211. > > In which cases would you want to *not* have NL80211 support if you > already have libnl enabled? What is the reason/use-case? The author made it clear that it is not the main reason. The main reason is that wpa_supplicant's usefulness is pretty reduced if nl80211 is not enabled; With a default kernel configuration, only nl80211 is supported since the deprecated wext compatibility is disabled by default, so you may even end up with a wpa_supplicant binary that can not manage any wifi device if you forgot about enabling libnl to have nl80211. And wpa_supplicant is not entirely useless without nl80211 either: you may want to only use the wired driver to do 802.1x on Ethernet networks, or you may only need the wext driver because you are using some old/unmaintained out-of-tree linux driver that only knows about wext. In either case, it is useful to have an option to enable nl80211, and I would even suggest it to be enabled by default (and removing the automatic dependency, of course). I'm quite sure that some features which are already optional (such as HS20 or WPS) are useless/dead code without nl80211.
Dear Nicolas Cavallari, On Mon, 13 Jul 2015 11:24:28 +0200, Nicolas Cavallari wrote: > >> +config BR2_PACKAGE_WPA_SUPPLICANT_NL80211 > >> + bool "Enable NL80211" > >> + default y if BR2_PACKAGE_LIBNL > >> + select BR2_PACKAGE_LIBNL > >> + help > >> + Enable support for NL80211. > > > > In which cases would you want to *not* have NL80211 support if you > > already have libnl enabled? What is the reason/use-case? > > The author made it clear that it is not the main reason. The main > reason is that wpa_supplicant's usefulness is pretty reduced if > nl80211 is not enabled; > > With a default kernel configuration, only nl80211 is supported since > the deprecated wext compatibility is disabled by default, so you may > even end up with a wpa_supplicant binary that can not manage any wifi > device if you forgot about enabling libnl to have nl80211. > > And wpa_supplicant is not entirely useless without nl80211 either: you > may want to only use the wired driver to do 802.1x on Ethernet > networks, or you may only need the wext driver because you are using > some old/unmaintained out-of-tree linux driver that only knows about wext. > > In either case, it is useful to have an option to enable nl80211, and > I would even suggest it to be enabled by default (and removing the > automatic dependency, of course). I'm quite sure that some features > which are already optional (such as HS20 or WPS) are useless/dead code > without nl80211. Then fair enough: what we need is a patch that adds an option as suggested by the original author, but also remove the "automatic optional dependency" handling that is currently in wpa_supplicant.mk. Would you be willing to work on such a patch? Thanks for the feedback! Thomas
diff --git a/package/wpa_supplicant/Config.in b/package/wpa_supplicant/Config.in index 81c61ac..1d43acd 100644 --- a/package/wpa_supplicant/Config.in +++ b/package/wpa_supplicant/Config.in @@ -22,6 +22,13 @@ config BR2_PACKAGE_WPA_SUPPLICANT_EAP help Enable support for EAP. +config BR2_PACKAGE_WPA_SUPPLICANT_NL80211 + bool "Enable NL80211" + default y if BR2_PACKAGE_LIBNL + select BR2_PACKAGE_LIBNL + help + Enable support for NL80211. + config BR2_PACKAGE_WPA_SUPPLICANT_HOTSPOT bool "Enable HS20" help diff --git a/package/wpa_supplicant/wpa_supplicant.mk b/package/wpa_supplicant/wpa_supplicant.mk index 0ca2ce5..4ea2a3a 100644 --- a/package/wpa_supplicant/wpa_supplicant.mk +++ b/package/wpa_supplicant/wpa_supplicant.mk @@ -42,6 +42,12 @@ else WPA_SUPPLICANT_CONFIG_DISABLE += CONFIG_DRIVER_NL80211 endif +ifeq ($(BR2_PACKAGE_WPA_SUPPLICANT_NL80211),y) + WPA_SUPPLICANT_CONFIG_ENABLE += CONFIG_DRIVER_NL80211 +else + WPA_SUPPLICANT_CONFIG_DISABLE += CONFIG_DRIVER_NL80211 +endif + # Trailing underscore on purpose to not enable CONFIG_EAPOL_TEST ifeq ($(BR2_PACKAGE_WPA_SUPPLICANT_EAP),y) WPA_SUPPLICANT_CONFIG_ENABLE += CONFIG_EAP_