[PATCH 0/3] wimax: Kbuild / rfkill-build fixes

[PATCH 0/3] wimax: Kbuild / rfkill-build fixes

[Inaky Perez-Gonzalez]

These three patches fix issues reported by Randy Dunlap on the
linux-next tree for Jan 6:

http://lkml.org/lkml/2009/1/6/340

The issue fixed by patch #3 wasn't reported on the emails but was
uncovered while discussing the fixes.

Inaky Perez-Gonzalez (3):
  wimax: fix '#ifdef CONFIG_BUG' layout to avoid warning
  wimax: fix kconfig interactions with rfkill and input layers
  wimax: testing for rfkill support should also test for
    CONFIG_RFKILL_MODULE

 net/wimax/Kconfig     |   14 ++++++++++++++
 net/wimax/id-table.c  |   10 ++++++----
 net/wimax/op-rfkill.c |    2 +-
 3 files changed, 21 insertions(+), 5 deletions(-)

[PATCH 1/3] wimax: fix '#ifdef CONFIG_BUG' layout to avoid warning

[Inaky Perez-Gonzalez]

Reported by Randy Dunlap:

> Also, this warning needs to be fixed:
>
> linux-next-20090106/net/wimax/id-table.c:133: warning: ISO C90
> forbids mixed declarations and code

Move the return on #defined(CONFIG_BUG) below the variable
declarations so it doesn't violate ISO C90.

On wimax_id_table_release() we want to do a debug check if CONFIG_BUG
is enabled. However, we also want the debug code to be always compiled
to ensure there is no bitrot. It will be optimized out by the compiler
when CONFIG_BUG is disabled.

Added a note to the function header stating this.

Signed-off-by: Inaky Perez-Gonzalez <inaky@linux.intel.com>
---
 net/wimax/id-table.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/wimax/id-table.c b/net/wimax/id-table.c
index d3b8855..d6e0e8f 100644
--- a/net/wimax/id-table.c
+++ b/net/wimax/id-table.c
@@ -123,15 +123,17 @@ void wimax_id_table_rm(struct wimax_dev *wimax_dev)
 /*
  * Release the gennetlink family id / mapping table
  *
- * On debug, verify that the table is empty upon removal.
+ * On debug, verify that the table is empty upon removal. We want the
+ * code always compiled, to ensure it doesn't bit rot. It will be
+ * compiled out if CONFIG_BUG is disabled.
  */
 void wimax_id_table_release(void)
 {
-#ifndef CONFIG_BUG
-	return;
-#endif
 	struct wimax_dev *wimax_dev;
 
+#ifdef CONFIG_BUG
+	return;
+#endif
 	spin_lock(&wimax_id_table_lock);
 	list_for_each_entry(wimax_dev, &wimax_id_table, id_table_node) {
 		printk(KERN_ERR "BUG: %s wimax_dev %p ifindex %d not cleared\n",
-- 
1.5.6.5

Re: [PATCH 1/3] wimax: fix '#ifdef CONFIG_BUG' layout to avoid warning

[Ilpo Järvinen]

On Tue, 6 Jan 2009, Inaky Perez-Gonzalez wrote:

> Reported by Randy Dunlap:
> 
> > Also, this warning needs to be fixed:
> >
> > linux-next-20090106/net/wimax/id-table.c:133: warning: ISO C90
> > forbids mixed declarations and code
> 
> Move the return on #defined(CONFIG_BUG) below the variable
> declarations so it doesn't violate ISO C90.
> 
> On wimax_id_table_release() we want to do a debug check if CONFIG_BUG
> is enabled. However, we also want the debug code to be always compiled
> to ensure there is no bitrot.

I hope this kind of solution won't add some warnings? Besides, this seems 
rather strange reasoning as CONFIG_BUG is mostly enabled anyway?

> It will be optimized out by the compiler
> when CONFIG_BUG is disabled.
> 
> Added a note to the function header stating this.
> 
> Signed-off-by: Inaky Perez-Gonzalez <inaky@linux.intel.com>
> ---
>  net/wimax/id-table.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/net/wimax/id-table.c b/net/wimax/id-table.c
> index d3b8855..d6e0e8f 100644
> --- a/net/wimax/id-table.c
> +++ b/net/wimax/id-table.c
> @@ -123,15 +123,17 @@ void wimax_id_table_rm(struct wimax_dev *wimax_dev)
>  /*
>   * Release the gennetlink family id / mapping table
>   *
> - * On debug, verify that the table is empty upon removal.
> + * On debug, verify that the table is empty upon removal. We want the
> + * code always compiled, to ensure it doesn't bit rot. It will be
> + * compiled out if CONFIG_BUG is disabled.
>   */
>  void wimax_id_table_release(void)
>  {
> -#ifndef CONFIG_BUG
> -	return;
> -#endif
>  	struct wimax_dev *wimax_dev;
>  
> +#ifdef CONFIG_BUG

Did you perhaps mean ifndef here??? :-)

> +	return;
> +#endif
>  	spin_lock(&wimax_id_table_lock);
>  	list_for_each_entry(wimax_dev, &wimax_id_table, id_table_node) {
>  		printk(KERN_ERR "BUG: %s wimax_dev %p ifindex %d not cleared\n",
> 

-- 
 i.

Re: [PATCH 1/3] wimax: fix '#ifdef CONFIG_BUG' layout to avoid warning

[Inaky Perez-Gonzalez]

On Wednesday 07 January 2009, Ilpo Järvinen wrote:
> On Tue, 6 Jan 2009, Inaky Perez-Gonzalez wrote:
> > Reported by Randy Dunlap:
> > > Also, this warning needs to be fixed:
> > >
> > > linux-next-20090106/net/wimax/id-table.c:133: warning: ISO C90
> > > forbids mixed declarations and code
> >
> > Move the return on #defined(CONFIG_BUG) below the variable
> > declarations so it doesn't violate ISO C90.
> >
> > On wimax_id_table_release() we want to do a debug check if CONFIG_BUG
> > is enabled. However, we also want the debug code to be always compiled
> > to ensure there is no bitrot.
>
> I hope this kind of solution won't add some warnings? Besides, this seems
> rather strange reasoning as CONFIG_BUG is mostly enabled anyway?

Well, it is legal code -- short of 'if (1) return'. It doesn't warn (and 
it should not).

> > -#ifndef CONFIG_BUG
> > -	return;
> > -#endif
> >  	struct wimax_dev *wimax_dev;
> >
> > +#ifdef CONFIG_BUG
>
> Did you perhaps mean ifndef here??? :-)

Sigh ... you are right ... good thing I triple checked.

Sending updated patch series.

Thanks,

-- 
Inaky

Re: [PATCH 1/3] wimax: fix '#ifdef CONFIG_BUG' layout to avoid warning

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1107 bytes --]

On Wed, 7 Jan 2009, Inaky Perez-Gonzalez wrote:

> On Wednesday 07 January 2009, Ilpo Järvinen wrote:
> > On Tue, 6 Jan 2009, Inaky Perez-Gonzalez wrote:
> > > Reported by Randy Dunlap:
> > > > Also, this warning needs to be fixed:
> > > >
> > > > linux-next-20090106/net/wimax/id-table.c:133: warning: ISO C90
> > > > forbids mixed declarations and code
> > >
> > > Move the return on #defined(CONFIG_BUG) below the variable
> > > declarations so it doesn't violate ISO C90.
> > >
> > > On wimax_id_table_release() we want to do a debug check if CONFIG_BUG
> > > is enabled. However, we also want the debug code to be always compiled
> > > to ensure there is no bitrot.
> >
> > I hope this kind of solution won't add some warnings? Besides, this seems
> > rather strange reasoning as CONFIG_BUG is mostly enabled anyway?
> 
> Well, it is legal code -- short of 'if (1) return'. It doesn't warn (and 
> it should not).

Obviously, but I was concerned on the other lines than that 
particular one, e.g., gcc might think that wimax_dev is unused 
variable and emit a warning or along those lines...?


-- 
 i.

Re: [PATCH 1/3] wimax: fix '#ifdef CONFIG_BUG' layout to avoid warning

[Inaky Perez-Gonzalez]

On Wednesday 07 January 2009, Ilpo Järvinen wrote:
> On Wed, 7 Jan 2009, Inaky Perez-Gonzalez wrote:
> > On Wednesday 07 January 2009, Ilpo Järvinen wrote:
> > > On Tue, 6 Jan 2009, Inaky Perez-Gonzalez wrote:
> > > > Reported by Randy Dunlap:
> > > > > Also, this warning needs to be fixed:
> > > > >
> > > > > linux-next-20090106/net/wimax/id-table.c:133: warning: ISO C90
> > > > > forbids mixed declarations and code
> > > >
> > > > Move the return on #defined(CONFIG_BUG) below the variable
> > > > declarations so it doesn't violate ISO C90.
> > > >
> > > > On wimax_id_table_release() we want to do a debug check if CONFIG_BUG
> > > > is enabled. However, we also want the debug code to be always
> > > > compiled to ensure there is no bitrot.
> > >
> > > I hope this kind of solution won't add some warnings? Besides, this
> > > seems rather strange reasoning as CONFIG_BUG is mostly enabled anyway?
> >
> > Well, it is legal code -- short of 'if (1) return'. It doesn't warn (and
> > it should not).
>
> Obviously, but I was concerned on the other lines than that
> particular one, e.g., gcc might think that wimax_dev is unused
> variable and emit a warning or along those lines...?

Ah, I see -- no, it won't. [disclaimer: not know much about compiler 
optimization] In theory, as we were saying, it works just as in a case 
where you have

int somevar;

if (1)
   return;

somevar = call_some_func();

with 1 being the result of a compile time evaluation. The compiler sees
that somevar is being used, but the code path is never executed, so everything
gets dumped.

If it ever did, it'd be a matter of changing that return to an if (1) return.
It'd look uglier though.

-- 
Inaky

[PATCH 2/3] wimax: fix kconfig interactions with rfkill and input layers

[Inaky Perez-Gonzalez]

WiMAX can work without RFKILL, but it was missing a check to make sure
RFKILL is not being made a module with wimax compiled into the
kernel. This caused failed builds in s390, where CONFIG_INPUT is
always off.

When RFKILL is enabled, the code uses the input layer to report
hardware switch changes; thus, if RFKILL is enabled, INPUT has to be
too. It also needs to display some message when INPUT is disabled that
explains why WiMAX is not selectable.

(issues found by Randy Dunlap in the linux-next tree).

Signed-off-by: Inaky Perez-Gonzalez <inaky@linux.intel.com>
---
 net/wimax/Kconfig |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/net/wimax/Kconfig b/net/wimax/Kconfig
index 0bdbb69..18495cd 100644
--- a/net/wimax/Kconfig
+++ b/net/wimax/Kconfig
@@ -1,9 +1,23 @@
 #
 # WiMAX LAN device configuration
 #
+# Note the ugly 'depends on' on WIMAX: that disallows RFKILL to be a
+# module if WIMAX is to be linked in. The WiMAX code is done in such a
+# way that it doesn't require and explicit dependency on RFKILL in
+# case an embedded system wants to rip it out.
+#
+# As well, enablement of the RFKILL code means we need the INPUT layer
+# support to inject events coming from hw rfkill switches. That
+# dependency could be killed if input.h provided appropiate means to
+# work when input is disabled.
+
+comment "WiMAX Wireless Broadband support requires CONFIG_INPUT enabled"
+	depends on INPUT = n && RFKILL != n
 
 menuconfig WIMAX
 	tristate "WiMAX Wireless Broadband support"
+	depends on (y && RFKILL != m) || m
+	depends on (INPUT && RFKILL != n) || RFKILL = n
 	help
 
 	  Select to configure support for devices that provide
-- 
1.5.6.5

[PATCH 3/3] wimax: testing for rfkill support should also test for CONFIG_RFKILL_MODULE

[Inaky Perez-Gonzalez]

Current WiMAX rfkill code is missing the case where rfkill is compiled
in as modules and works only when rfkill is compiled in. This is not
correct. Fixed to test for CONFIG_RFKILL or CONFIG_RKILL_MODULE.

Signed-off-by: Inaky Perez-Gonzalez <inaky@linux.intel.com>
---
 net/wimax/op-rfkill.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/wimax/op-rfkill.c b/net/wimax/op-rfkill.c
index 8745bac..2b75aee 100644
--- a/net/wimax/op-rfkill.c
+++ b/net/wimax/op-rfkill.c
@@ -71,7 +71,7 @@
 #define D_SUBMODULE op_rfkill
 #include "debug-levels.h"
 
-#ifdef CONFIG_RFKILL
+#if defined(CONFIG_RFKILL) || defined(CONFIG_RFKILL_MODULE)
 
 
 /**
-- 
1.5.6.5