[Buildroot] [PATCH v2] wpa_supplicant: Add NL80211 support option

[Buildroot] [PATCH v2] wpa_supplicant: Add NL80211 support option

[Jean-Baptiste Theou]

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.
---
 package/wpa_supplicant/Config.in         | 7 +++++++
 package/wpa_supplicant/wpa_supplicant.mk | 6 ++++++
 2 files changed, 13 insertions(+)

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_
-- 
2.0.3

[Buildroot] [PATCH v2] wpa_supplicant: Add NL80211 support option

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH v2] wpa_supplicant: Add NL80211 support option

[Nicolas Cavallari]

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.

[Buildroot] [PATCH v2] wpa_supplicant: Add NL80211 support option

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 1/2] wpa_supplicant: Add an explicit option to enable nl80211.

[Nicolas Cavallari]

Currently, nl80211 support is conditional with libnl being enabled,
using implicit dependencies.  This causes problems since it is not
obvious and wpa_supplicant without nl80211 isn't what most user expects.

If nl80211 isn't enabled, then buildroot only enables the wext driver,
which will only work if some deprecated kernel feature isn't left
disabled, or if using a outdated out-of-tree linux driver which doesn't
use the cfg80211 infrastructure.

This makes nl80211 support an explicit option, which
"select BR2_PACKAGE_LIBNL" accordingly.  To handle upgrades nicely, it
would have been nice to have "default y if BR2_PACKAGE_LIBNL", but
Kconfig treats this as a circular dependency.  So instead, this enables
the option by default, which is less worse than not enabling nl80211
when it was previously implicitly enabled.

Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
---
 package/wpa_supplicant/Config.in         | 16 ++++++++++++++++
 package/wpa_supplicant/wpa_supplicant.mk |  2 +-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/package/wpa_supplicant/Config.in b/package/wpa_supplicant/Config.in
index f32a867..1824f95 100644
--- a/package/wpa_supplicant/Config.in
+++ b/package/wpa_supplicant/Config.in
@@ -8,6 +8,22 @@ config BR2_PACKAGE_WPA_SUPPLICANT
 
 if BR2_PACKAGE_WPA_SUPPLICANT
 
+config BR2_PACKAGE_WPA_SUPPLICANT_NL80211
+	bool "Enable nl80211 support"
+	default y
+	select BR2_PACKAGE_LIBNL
+	help
+	  Enable support for nl80211.  This is the current wireless API for
+	  Linux, supported by all wireless drivers in vanilla Linux, but may
+	  not be supported by some out-of-tree Linux wireless drivers.
+	  wpa_supplicant will still fall back to using the Wireless Extensions
+	  (wext) API with these drivers.
+
+	  If this option is disabled, then only the deprecated wext API will
+	  be supported, with far less features.  Linux may supports using
+	  wext with modern drivers using a compatibility layer, but it must
+	  be enabled in the kernel configuration.
+
 config BR2_PACKAGE_WPA_SUPPLICANT_AP_SUPPORT
 	bool "Enable AP mode"
 	help
diff --git a/package/wpa_supplicant/wpa_supplicant.mk b/package/wpa_supplicant/wpa_supplicant.mk
index eb4278a..6e25280 100644
--- a/package/wpa_supplicant/wpa_supplicant.mk
+++ b/package/wpa_supplicant/wpa_supplicant.mk
@@ -32,7 +32,7 @@ WPA_SUPPLICANT_CONFIG_DISABLE = \
 # libnl-3 needs -lm (for rint) and -lpthread if linking statically
 # And library order matters hence stick -lnl-3 first since it's appended
 # in the wpa_supplicant Makefiles as in LIBS+=-lnl-3 ... thus failing
-ifeq ($(BR2_PACKAGE_LIBNL),y)
+ifeq ($(BR2_PACKAGE_WPA_SUPPLICANT_NL80211),y)
 ifeq ($(BR2_STATIC_LIBS),y)
 WPA_SUPPLICANT_LIBS += -lnl-3 -lm -lpthread
 endif
-- 
2.1.4

[Buildroot] [PATCH 2/2] wpa_supplicant: Have Hotspot 2.0 depend on nl80211

[Nicolas Cavallari]

Hotspot 2.0 uses ANQP whose implementation needs offchannel/
remain-on-channel capabilities from the driver, which only exist in the
nl80211 driver.  Compiling would work, but the feature would not work.
---
 package/wpa_supplicant/Config.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/package/wpa_supplicant/Config.in b/package/wpa_supplicant/Config.in
index 1824f95..c163bbc 100644
--- a/package/wpa_supplicant/Config.in
+++ b/package/wpa_supplicant/Config.in
@@ -40,6 +40,7 @@ config BR2_PACKAGE_WPA_SUPPLICANT_EAP
 
 config BR2_PACKAGE_WPA_SUPPLICANT_HOTSPOT
 	bool "Enable HS20"
+	depends on BR2_PACKAGE_WPA_SUPPLICANT_NL80211
 	help
 	  Enable Hotspot 2.0 and IEEE 802.11u interworking functionality.
 
-- 
2.1.4

[Buildroot] [PATCH 1/2] wpa_supplicant: Add an explicit option to enable nl80211.

[Baruch Siach]

Hi Nicolas,

On Thu, Jul 16, 2015 at 03:02:33PM +0200, Nicolas Cavallari wrote:
> Currently, nl80211 support is conditional with libnl being enabled,
> using implicit dependencies.  This causes problems since it is not
> obvious and wpa_supplicant without nl80211 isn't what most user expects.
> 
> If nl80211 isn't enabled, then buildroot only enables the wext driver,
> which will only work if some deprecated kernel feature isn't left
> disabled, or if using a outdated out-of-tree linux driver which doesn't
> use the cfg80211 infrastructure.
> 
> This makes nl80211 support an explicit option, which
> "select BR2_PACKAGE_LIBNL" accordingly.  To handle upgrades nicely, it
> would have been nice to have "default y if BR2_PACKAGE_LIBNL", but
> Kconfig treats this as a circular dependency.  So instead, this enables
> the option by default, which is less worse than not enabling nl80211
> when it was previously implicitly enabled.
> 
> Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
> ---
>  package/wpa_supplicant/Config.in         | 16 ++++++++++++++++
>  package/wpa_supplicant/wpa_supplicant.mk |  2 +-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/package/wpa_supplicant/Config.in b/package/wpa_supplicant/Config.in
> index f32a867..1824f95 100644
> --- a/package/wpa_supplicant/Config.in
> +++ b/package/wpa_supplicant/Config.in
> @@ -8,6 +8,22 @@ config BR2_PACKAGE_WPA_SUPPLICANT
>  
>  if BR2_PACKAGE_WPA_SUPPLICANT
>  
> +config BR2_PACKAGE_WPA_SUPPLICANT_NL80211
> +	bool "Enable nl80211 support"
> +	default y
> +	select BR2_PACKAGE_LIBNL

BR2_PACKAGE_LIBNL depends on BR2_TOOLCHAIN_HAS_THREADS. You need to propagate 
this dependency here, since Kconfig doesn't do that automatically. See 
http://nightly.buildroot.org/manual.html#depends-on-vs-select.

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[Buildroot] [PATCH 2/2] wpa_supplicant: Have Hotspot 2.0 depend on nl80211

[Baruch Siach]

Hi Nicolas,

On Thu, Jul 16, 2015 at 03:02:34PM +0200, Nicolas Cavallari wrote:
> Hotspot 2.0 uses ANQP whose implementation needs offchannel/
> remain-on-channel capabilities from the driver, which only exist in the
> nl80211 driver.  Compiling would work, but the feature would not work.

Your sign-off is missing.

>  package/wpa_supplicant/Config.in | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/package/wpa_supplicant/Config.in b/package/wpa_supplicant/Config.in
> index 1824f95..c163bbc 100644
> --- a/package/wpa_supplicant/Config.in
> +++ b/package/wpa_supplicant/Config.in
> @@ -40,6 +40,7 @@ config BR2_PACKAGE_WPA_SUPPLICANT_EAP
>  
>  config BR2_PACKAGE_WPA_SUPPLICANT_HOTSPOT
>  	bool "Enable HS20"
> +	depends on BR2_PACKAGE_WPA_SUPPLICANT_NL80211

Please add the BR2_PACKAGE_LIBNL dependency here as well.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[Buildroot] [PATCH 2/2] wpa_supplicant: Have Hotspot 2.0 depend on nl80211

[Thomas Petazzoni]

Dear Baruch Siach,

On Fri, 17 Jul 2015 09:41:01 +0300, Baruch Siach wrote:

> >  package/wpa_supplicant/Config.in | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/package/wpa_supplicant/Config.in b/package/wpa_supplicant/Config.in
> > index 1824f95..c163bbc 100644
> > --- a/package/wpa_supplicant/Config.in
> > +++ b/package/wpa_supplicant/Config.in
> > @@ -40,6 +40,7 @@ config BR2_PACKAGE_WPA_SUPPLICANT_EAP
> >  
> >  config BR2_PACKAGE_WPA_SUPPLICANT_HOTSPOT
> >  	bool "Enable HS20"
> > +	depends on BR2_PACKAGE_WPA_SUPPLICANT_NL80211
> 
> Please add the BR2_PACKAGE_LIBNL dependency here as well.

Not needed since it's a "depends on", so
BR2_PACKAGE_WPA_SUPPLICANT_HOTSPOT will anyway only be visible if
BR2_PACKAGE_WPA_SUPPLICANT_NL80211 is enabled, and if
BR2_PACKAGE_WPA_SUPPLICANT_NL80211 is enabled, then it has selected
BR2_PACKAGE_LIBNL.

Am I missing something?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 1/2] wpa_supplicant: Add an explicit option to enable nl80211.

[Nicolas Cavallari]

On 17/07/2015 08:39, Baruch Siach wrote:
> Hi Nicolas,
> 
> On Thu, Jul 16, 2015 at 03:02:33PM +0200, Nicolas Cavallari wrote:
>> Currently, nl80211 support is conditional with libnl being enabled,
>> using implicit dependencies.  This causes problems since it is not
>> obvious and wpa_supplicant without nl80211 isn't what most user expects.
>>
>> If nl80211 isn't enabled, then buildroot only enables the wext driver,
>> which will only work if some deprecated kernel feature isn't left
>> disabled, or if using a outdated out-of-tree linux driver which doesn't
>> use the cfg80211 infrastructure.
>>
>> This makes nl80211 support an explicit option, which
>> "select BR2_PACKAGE_LIBNL" accordingly.  To handle upgrades nicely, it
>> would have been nice to have "default y if BR2_PACKAGE_LIBNL", but
>> Kconfig treats this as a circular dependency.  So instead, this enables
>> the option by default, which is less worse than not enabling nl80211
>> when it was previously implicitly enabled.
>>
>> Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
>> ---
>>  package/wpa_supplicant/Config.in         | 16 ++++++++++++++++
>>  package/wpa_supplicant/wpa_supplicant.mk |  2 +-
>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/package/wpa_supplicant/Config.in b/package/wpa_supplicant/Config.in
>> index f32a867..1824f95 100644
>> --- a/package/wpa_supplicant/Config.in
>> +++ b/package/wpa_supplicant/Config.in
>> @@ -8,6 +8,22 @@ config BR2_PACKAGE_WPA_SUPPLICANT
>>  
>>  if BR2_PACKAGE_WPA_SUPPLICANT
>>  
>> +config BR2_PACKAGE_WPA_SUPPLICANT_NL80211
>> +	bool "Enable nl80211 support"
>> +	default y
>> +	select BR2_PACKAGE_LIBNL
> 
> BR2_PACKAGE_LIBNL depends on BR2_TOOLCHAIN_HAS_THREADS. You need to propagate 
> this dependency here, since Kconfig doesn't do that automatically. See 
> http://nightly.buildroot.org/manual.html#depends-on-vs-select.

libnl need threads ?

apparently, no it doesn't if configure is run with
--disable-pthreads.  But this is not supported by buildroot.

will respin accordingly.

[Buildroot] [PATCH 1/2 v2] wpa_supplicant: Add an explicit option to enable nl80211.

[Nicolas Cavallari]

Currently, nl80211 support is conditionnal with libnl being enabled,
using implicit dependencies.  This causes problems since it is not
obvious and wpa_supplicant without nl80211 isn't what most user expects.

If nl80211 isn't enabled, then buildroot only enables the wext driver,
which will only work if some deprecated kernel feature isn't left
disabled, or if using a outdated out-of-tree linux driver which doesn't
use the cfg80211 infrastructure.

This makes nl80211 support an explicit option, which
"select BR2_PACKAGE_LIBNL" accordingly.  To handle upgrades nicely, it
would have been nice to have "default y if BR2_PACKAGE_LIBNL", but
Kconfig treats this as a circular dependency.  So instead, this enables
the option by default, which is less worse than not enabling nl80211
when it was previously implicitely enabled.

Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
---
v2: Added depends on BR2_TOOLCHAIN_HAS_THREADS # libnl

 package/wpa_supplicant/Config.in         | 17 +++++++++++++++++
 package/wpa_supplicant/wpa_supplicant.mk |  2 +-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/package/wpa_supplicant/Config.in b/package/wpa_supplicant/Config.in
index f32a867..29dc4b9 100644
--- a/package/wpa_supplicant/Config.in
+++ b/package/wpa_supplicant/Config.in
@@ -8,6 +8,23 @@ config BR2_PACKAGE_WPA_SUPPLICANT
 
 if BR2_PACKAGE_WPA_SUPPLICANT
 
+config BR2_PACKAGE_WPA_SUPPLICANT_NL80211
+	bool "Enable nl80211 support"
+	default y
+	select BR2_PACKAGE_LIBNL
+	depends on BR2_TOOLCHAIN_HAS_THREADS # libnl
+	help
+	  Enable support for nl80211.  This is the current wireless API for
+	  Linux, supported by all wireless drivers in vanilla Linux, but may
+	  not be supported by some out-of-tree Linux wireless drivers.
+	  wpa_supplicant will still fall back to using the Wireless Extensions
+	  (wext) API with these drivers.
+
+	  If this option is disabled, then only the deprecated wext API will
+	  be supported, with far less features.  Linux may supports using
+	  wext with modern drivers using a compatibility layer, but it must
+	  be enabled in the kernel configuration.
+
 config BR2_PACKAGE_WPA_SUPPLICANT_AP_SUPPORT
 	bool "Enable AP mode"
 	help
diff --git a/package/wpa_supplicant/wpa_supplicant.mk b/package/wpa_supplicant/wpa_supplicant.mk
index eb4278a..6e25280 100644
--- a/package/wpa_supplicant/wpa_supplicant.mk
+++ b/package/wpa_supplicant/wpa_supplicant.mk
@@ -32,7 +32,7 @@ WPA_SUPPLICANT_CONFIG_DISABLE = \
 # libnl-3 needs -lm (for rint) and -lpthread if linking statically
 # And library order matters hence stick -lnl-3 first since it's appended
 # in the wpa_supplicant Makefiles as in LIBS+=-lnl-3 ... thus failing
-ifeq ($(BR2_PACKAGE_LIBNL),y)
+ifeq ($(BR2_PACKAGE_WPA_SUPPLICANT_NL80211),y)
 ifeq ($(BR2_STATIC_LIBS),y)
 WPA_SUPPLICANT_LIBS += -lnl-3 -lm -lpthread
 endif
-- 
2.1.4

[Buildroot] [PATCH 2/2 v2] wpa_supplicant: Have Hotspot 2.0 depend on nl80211

[Nicolas Cavallari]

Hotspot 2.0 uses ANQP whose implementation needs offchannel/
remain-on-channel capabilities from the driver, which only exist in the
nl80211 driver.  Compiling would work, but the feature would not work.

Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
---

v2: added signoff.
 package/wpa_supplicant/Config.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/package/wpa_supplicant/Config.in b/package/wpa_supplicant/Config.in
index 29dc4b9..22cf7f8 100644
--- a/package/wpa_supplicant/Config.in
+++ b/package/wpa_supplicant/Config.in
@@ -41,6 +41,7 @@ config BR2_PACKAGE_WPA_SUPPLICANT_EAP
 
 config BR2_PACKAGE_WPA_SUPPLICANT_HOTSPOT
 	bool "Enable HS20"
+	depends on BR2_PACKAGE_WPA_SUPPLICANT_NL80211
 	help
 	  Enable Hotspot 2.0 and IEEE 802.11u interworking functionality.
 
-- 
2.1.4

[Buildroot] [PATCH 2/2] wpa_supplicant: Have Hotspot 2.0 depend on nl80211

[Nicolas Cavallari]

On 17/07/2015 09:45, Thomas Petazzoni wrote:
> On Fri, 17 Jul 2015 09:41:01 +0300, Baruch Siach wrote:
>>>  config BR2_PACKAGE_WPA_SUPPLICANT_HOTSPOT
>>>  	bool "Enable HS20"
>>> +	depends on BR2_PACKAGE_WPA_SUPPLICANT_NL80211
>>
>> Please add the BR2_PACKAGE_LIBNL dependency here as well.
> 
> Not needed since it's a "depends on", so
> BR2_PACKAGE_WPA_SUPPLICANT_HOTSPOT will anyway only be visible if
> BR2_PACKAGE_WPA_SUPPLICANT_NL80211 is enabled, and if
> BR2_PACKAGE_WPA_SUPPLICANT_NL80211 is enabled, then it has selected
> BR2_PACKAGE_LIBNL.
> 
> Am I missing something?

I was thinking the same thing.

[Buildroot] [PATCH 2/2] wpa_supplicant: Have Hotspot 2.0 depend on nl80211

[Baruch Siach]

Hi Thomas,

On Fri, Jul 17, 2015 at 09:45:32AM +0200, Thomas Petazzoni wrote:
> On Fri, 17 Jul 2015 09:41:01 +0300, Baruch Siach wrote:
> > >  package/wpa_supplicant/Config.in | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/package/wpa_supplicant/Config.in b/package/wpa_supplicant/Config.in
> > > index 1824f95..c163bbc 100644
> > > --- a/package/wpa_supplicant/Config.in
> > > +++ b/package/wpa_supplicant/Config.in
> > > @@ -40,6 +40,7 @@ config BR2_PACKAGE_WPA_SUPPLICANT_EAP
> > >  
> > >  config BR2_PACKAGE_WPA_SUPPLICANT_HOTSPOT
> > >  	bool "Enable HS20"
> > > +	depends on BR2_PACKAGE_WPA_SUPPLICANT_NL80211
> > 
> > Please add the BR2_PACKAGE_LIBNL dependency here as well.
> 
> Not needed since it's a "depends on", so
> BR2_PACKAGE_WPA_SUPPLICANT_HOTSPOT will anyway only be visible if
> BR2_PACKAGE_WPA_SUPPLICANT_NL80211 is enabled, and if
> BR2_PACKAGE_WPA_SUPPLICANT_NL80211 is enabled, then it has selected
> BR2_PACKAGE_LIBNL.
> 
> Am I missing something?

Right. The dependency of hotspot on libnl is no apparent at all, though. IMO, 
having here

   select BR2_PACKAGE_WPA_SUPPLICANT_NL80211
   depends on BR2_TOOLCHAIN_HAS_THREADS # libnl

and maybe also a dependency comment, would be better.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[Buildroot] [1/2, v2] wpa_supplicant: Add an explicit option to enable nl80211.

[Gary Bisson]

Hi Nicolas,

On Fri, Jul 17, 2015 at 09:59:09AM +0200, Nicolas Cavallari wrote:
> Currently, nl80211 support is conditionnal with libnl being enabled,
> using implicit dependencies.  This causes problems since it is not
> obvious and wpa_supplicant without nl80211 isn't what most user expects.
> 
> If nl80211 isn't enabled, then buildroot only enables the wext driver,
> which will only work if some deprecated kernel feature isn't left
> disabled, or if using a outdated out-of-tree linux driver which doesn't
> use the cfg80211 infrastructure.
> 
> This makes nl80211 support an explicit option, which
> "select BR2_PACKAGE_LIBNL" accordingly.  To handle upgrades nicely, it
> would have been nice to have "default y if BR2_PACKAGE_LIBNL", but
> Kconfig treats this as a circular dependency.  So instead, this enables
> the option by default, which is less worse than not enabling nl80211
> when it was previously implicitely enabled.

Actually I like the fact that it is enabled by default. As you said,
most users now want NL80211.

Only one remark, why are there two spaces after every dots in your
commit logs. Is it a custom git hook? ;-)

Reviewed-by: Gary Bisson <gary.bisson@boundarydevices.com>
Tested-by: Gary Bisson <gary.bisson@boundarydevices.com>

Regards,
Gary

[Buildroot] [2/2, v2] wpa_supplicant: Have Hotspot 2.0 depend on nl80211

[Gary Bisson]

Hi Nicolas,

On Fri, Jul 17, 2015 at 09:59:10AM +0200, Nicolas Cavallari wrote:
> Hotspot 2.0 uses ANQP whose implementation needs offchannel/
> remain-on-channel capabilities from the driver, which only exist in the
> nl80211 driver.  Compiling would work, but the feature would not work.
> 
> Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
> ---
> 
> v2: added signoff.
>  package/wpa_supplicant/Config.in | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/package/wpa_supplicant/Config.in b/package/wpa_supplicant/Config.in
> index 29dc4b9..22cf7f8 100644
> --- a/package/wpa_supplicant/Config.in
> +++ b/package/wpa_supplicant/Config.in
> @@ -41,6 +41,7 @@ config BR2_PACKAGE_WPA_SUPPLICANT_EAP
>  
>  config BR2_PACKAGE_WPA_SUPPLICANT_HOTSPOT
>  	bool "Enable HS20"
> +	depends on BR2_PACKAGE_WPA_SUPPLICANT_NL80211
>  	help
>  	  Enable Hotspot 2.0 and IEEE 802.11u interworking functionality.
>

First I'm not sure on the procedure to test the Hotspot 2.0 but I think
we should at least add a comment like:

comment "HS20 needs NL80211 support"
	depends on !BR2_PACKAGE_WPA_SUPPLICANT_NL80211

Because right now if you unselect NL80211 WPA option the HS20 just
disappears.

Or maybe selecting NL80211 would be even better if absolutely required
at runtime.

Regards,
Gary

[Buildroot] [2/2, v2] wpa_supplicant: Have Hotspot 2.0 depend on nl80211

[Nicolas Cavallari]

On 17/09/2015 17:31, Gary Bisson wrote:
> Hi Nicolas,
> 
> On Fri, Jul 17, 2015 at 09:59:10AM +0200, Nicolas Cavallari wrote:
>> Hotspot 2.0 uses ANQP whose implementation needs offchannel/
>> remain-on-channel capabilities from the driver, which only exist in the
>> nl80211 driver.  Compiling would work, but the feature would not work.
>>
>> Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
>> ---
>>
>> v2: added signoff.
>>  package/wpa_supplicant/Config.in | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/package/wpa_supplicant/Config.in b/package/wpa_supplicant/Config.in
>> index 29dc4b9..22cf7f8 100644
>> --- a/package/wpa_supplicant/Config.in
>> +++ b/package/wpa_supplicant/Config.in
>> @@ -41,6 +41,7 @@ config BR2_PACKAGE_WPA_SUPPLICANT_EAP
>>  
>>  config BR2_PACKAGE_WPA_SUPPLICANT_HOTSPOT
>>  	bool "Enable HS20"
>> +	depends on BR2_PACKAGE_WPA_SUPPLICANT_NL80211
>>  	help
>>  	  Enable Hotspot 2.0 and IEEE 802.11u interworking functionality.
>>
> 
> First I'm not sure on the procedure to test the Hotspot 2.0 but I think
> we should at least add a comment like:
> 
> comment "HS20 needs NL80211 support"
> 	depends on !BR2_PACKAGE_WPA_SUPPLICANT_NL80211
> 
> Because right now if you unselect NL80211 WPA option the HS20 just
> disappears.
> 
> Or maybe selecting NL80211 would be even better if absolutely required
> at runtime.


The more i think of it, the more I feel like the current behavior is
acceptable as is and this patch is unnecessary.

Sure, using a vanilla wpa_supplicant, the features needed by Hotspot 2.0
are only implemented by the nl80211 driver, but there may be other
non-vanilla drivers added by out-of-tree patches which also implement it.

These patches may be in a buildroot global patch directory.  We
shouldn't force nl80211 to be enabled to enable Hotspot 2.0 because this
is the only possibility that we know.

[Buildroot] [PATCH 1/2] wpa_supplicant: Add an explicit option to enable nl80211.

[Thomas Petazzoni]

Dear Nicolas Cavallari,

On Thu, 16 Jul 2015 15:02:33 +0200, Nicolas Cavallari wrote:
> Currently, nl80211 support is conditional with libnl being enabled,
> using implicit dependencies.  This causes problems since it is not
> obvious and wpa_supplicant without nl80211 isn't what most user expects.
> 
> If nl80211 isn't enabled, then buildroot only enables the wext driver,
> which will only work if some deprecated kernel feature isn't left
> disabled, or if using a outdated out-of-tree linux driver which doesn't
> use the cfg80211 infrastructure.
> 
> This makes nl80211 support an explicit option, which
> "select BR2_PACKAGE_LIBNL" accordingly.  To handle upgrades nicely, it
> would have been nice to have "default y if BR2_PACKAGE_LIBNL", but
> Kconfig treats this as a circular dependency.  So instead, this enables
> the option by default, which is less worse than not enabling nl80211
> when it was previously implicitly enabled.
> 
> Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
> ---
>  package/wpa_supplicant/Config.in         | 16 ++++++++++++++++
>  package/wpa_supplicant/wpa_supplicant.mk |  2 +-
>  2 files changed, 17 insertions(+), 1 deletion(-)

I've applied after doing some minor fixes:

    [Thomas:
     - rewrap Config.in help text
     - add comment about thread dependency.]

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [2/2, v2] wpa_supplicant: Have Hotspot 2.0 depend on nl80211

[Yann E. MORIN]

Nicolas, All,

On 2015-09-17 19:21 +0200, Nicolas Cavallari spake thusly:
> On 17/09/2015 17:31, Gary Bisson wrote:
> > Hi Nicolas,
> > 
> > On Fri, Jul 17, 2015 at 09:59:10AM +0200, Nicolas Cavallari wrote:
> >> Hotspot 2.0 uses ANQP whose implementation needs offchannel/
> >> remain-on-channel capabilities from the driver, which only exist in the
> >> nl80211 driver.  Compiling would work, but the feature would not work.
> >>
> >> Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
> >> ---
> >>
> >> v2: added signoff.
> >>  package/wpa_supplicant/Config.in | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/package/wpa_supplicant/Config.in b/package/wpa_supplicant/Config.in
> >> index 29dc4b9..22cf7f8 100644
> >> --- a/package/wpa_supplicant/Config.in
> >> +++ b/package/wpa_supplicant/Config.in
> >> @@ -41,6 +41,7 @@ config BR2_PACKAGE_WPA_SUPPLICANT_EAP
> >>  
> >>  config BR2_PACKAGE_WPA_SUPPLICANT_HOTSPOT
> >>  	bool "Enable HS20"
> >> +	depends on BR2_PACKAGE_WPA_SUPPLICANT_NL80211
> >>  	help
> >>  	  Enable Hotspot 2.0 and IEEE 802.11u interworking functionality.
> >>
> > 
> > First I'm not sure on the procedure to test the Hotspot 2.0 but I think
> > we should at least add a comment like:
> > 
> > comment "HS20 needs NL80211 support"
> > 	depends on !BR2_PACKAGE_WPA_SUPPLICANT_NL80211
> > 
> > Because right now if you unselect NL80211 WPA option the HS20 just
> > disappears.
> > 
> > Or maybe selecting NL80211 would be even better if absolutely required
> > at runtime.
> 
> 
> The more i think of it, the more I feel like the current behavior is
> acceptable as is and this patch is unnecessary.
> 
> Sure, using a vanilla wpa_supplicant, the features needed by Hotspot 2.0
> are only implemented by the nl80211 driver, but there may be other
> non-vanilla drivers added by out-of-tree patches which also implement it.
> 
> These patches may be in a buildroot global patch directory.  We
> shouldn't force nl80211 to be enabled to enable Hotspot 2.0 because this
> is the only possibility that we know.

So, given your feedback, I'm marking this patch as "Rejected" in our
Patchwork.

Thanks! :-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'