[PATCH] staging/wlags49_h2: Fix build error when CONFIG_SYSFS is

[PATCH] staging/wlags49_h2: Fix build error when CONFIG_SYSFS is

[Javier Martinez Canillas]

Hello,

Didn't get any feedback so resending the patch.

I got a wlags49_h2 driver build error in linux-next when CONFIG_SYSFS is not set.

CC [M]  drivers/staging/wlags49_h2/wl_cs.o
In file included from drivers/staging/wlags49_h2/wl_cs.c:104:
drivers/staging/wlags49_h2/wl_sysfs.h: In function ‘register_wlags_sysfs’:
drivers/staging/wlags49_h2/wl_sysfs.h:5: error: parameter name omitted
drivers/staging/wlags49_h2/wl_sysfs.h: In function ‘unregister_wlags_sysfs’:
drivers/staging/wlags49_h2/wl_sysfs.h:6: error: parameter name omitted
make[1]: *** [drivers/staging/wlags49_h2/wl_cs.o] Error 1
make: *** [_module_drivers/staging/wlags49_h2] Error 2

This is due a wrong function definition (it does not include parameters names). 

Current patch solves the issue.

Thanks a lot

Signed-off-by: Javier Martinez Canillas <martinez.javier@gmail.com>
---
 drivers/staging/wlags49_h2/wl_sysfs.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/wlags49_h2/wl_sysfs.h
b/drivers/staging/wlags49_h2/wl_sysfs.h
index 6d96d03..fa658c3 100644
--- a/drivers/staging/wlags49_h2/wl_sysfs.h
+++ b/drivers/staging/wlags49_h2/wl_sysfs.h
@@ -2,6 +2,6 @@
 extern void register_wlags_sysfs(struct net_device *);
 extern void unregister_wlags_sysfs(struct net_device *);
 #else
-static void register_wlags_sysfs(struct net_device *) { return; };
-static void unregister_wlags_sysfs(struct net_device *) { return; };
+static inline void register_wlags_sysfs(struct net_device *net) { }
+static inline void unregister_wlags_sysfs(struct net_device *net) { }
 #endif
--
1.7.0.4

Re: [PATCH] staging/wlags49_h2: Fix build error when CONFIG_SYSFS

[Dan Carpenter]

On Thu, Jun 17, 2010 at 01:17:44AM -0400, Javier Martinez Canillas wrote:
> Hello,
> 
> Didn't get any feedback so resending the patch.
> 

The patch is ok.  I'm pretty sure that it will get merged eventually.

To be honest, it wouldn't surprise me if it didn't get merged until
2.6.36.  It fixes a build error yes, but it's also a staging thing so
there might not be a rush on it.

regards,
dan carpenter

Re: [PATCH] staging/wlags49_h2: Fix build error when CONFIG_SYSFS is

[Javier Martinez Canillas]

>> The patch is ok.  I'm pretty sure that it will get merged eventually.
>
> To be honest, it wouldn't surprise me if it didn't get merged until
> 2.6.36.  It fixes a build error yes, but it's also a staging thing so
> there might not be a rush on it.
>

Thank you very much for the feedback Dan.

Best regards,

-----------------------------------------
Javier Martínez Canillas
+595 981 88 66 58

Re: [PATCH] staging/wlags49_h2: Fix build error when CONFIG_SYSFS

[Henk de Groot]

Javier,

I assumed Greg KH already added this last version of your patch to the 
queue for the staging tree. Anyway I think it's fine. The only problem I 
see is a possible complained from the compiler because of an argument 
that's defined but never used. The only way around that is to avoid the 
call to the function in the first place by using compiler directives in 
the code (or use a #define but that solution was already rejected). 
Disabling the call to the function in the code may even save some more 
overhead and make the memory footprint smaller.

Kind regards,

Henk.

Op 17-6-2010 7:17, Javier Martinez Canillas schreef:
> Hello,
>
> Didn't get any feedback so resending the patch.
>
> I got a wlags49_h2 driver build error in linux-next when CONFIG_SYSFS is not set.
>
> CC [M]  drivers/staging/wlags49_h2/wl_cs.o
> In file included from drivers/staging/wlags49_h2/wl_cs.c:104:
> drivers/staging/wlags49_h2/wl_sysfs.h: In function ‘register_wlags_sysfs’:
> drivers/staging/wlags49_h2/wl_sysfs.h:5: error: parameter name omitted
> drivers/staging/wlags49_h2/wl_sysfs.h: In function ‘unregister_wlags_sysfs’:
> drivers/staging/wlags49_h2/wl_sysfs.h:6: error: parameter name omitted
> make[1]: *** [drivers/staging/wlags49_h2/wl_cs.o] Error 1
> make: *** [_module_drivers/staging/wlags49_h2] Error 2
>
> This is due a wrong function definition (it does not include parameters names).
>
> Current patch solves the issue.
>
> Thanks a lot
>
> Signed-off-by: Javier Martinez Canillas<martinez.javier@gmail.com>
> ---
>   drivers/staging/wlags49_h2/wl_sysfs.h |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/wlags49_h2/wl_sysfs.h
> b/drivers/staging/wlags49_h2/wl_sysfs.h
> index 6d96d03..fa658c3 100644
> --- a/drivers/staging/wlags49_h2/wl_sysfs.h
> +++ b/drivers/staging/wlags49_h2/wl_sysfs.h
> @@ -2,6 +2,6 @@
>   extern void register_wlags_sysfs(struct net_device *);
>   extern void unregister_wlags_sysfs(struct net_device *);
>   #else
> -static void register_wlags_sysfs(struct net_device *) { return; };
> -static void unregister_wlags_sysfs(struct net_device *) { return; };
> +static inline void register_wlags_sysfs(struct net_device *net) { }
> +static inline void unregister_wlags_sysfs(struct net_device *net) { }
>   #endif
> --
> 1.7.0.4
>
>
>
>    

Re: [PATCH] staging/wlags49_h2: Fix build error when CONFIG_SYSFS is

[Javier Martinez Canillas]

Hank,

Thank you very much for your feedback.

On Thu, Jun 17, 2010 at 1:23 PM, Henk de Groot <henk.de.groot@hetnet.nl> wrote:
> The only problem I see is a
> possible complained from the compiler because of an argument that's defined
> but never used. The only way around that is to avoid the call to the
> function in the first place by using compiler directives in the code (or use
> a #define but that solution was already rejected). Disabling the call to the
> function in the code may even save some more overhead and make the memory
> footprint smaller.

I thought about using #ifdefs in the code too but then read in
Documentation/SubmittingPatches:

2) #ifdefs are ugly

Code cluttered with ifdefs is difficult to read and maintain.  Don't do
it. Instead, put your ifdefs in a header, and conditionally define
'static inline'

So I discard this option. Do you want me to do it anyway and send a
new patch? I'm willing to solve it the right way.

Thanks a lot.

Best regards,

-----------------------------------------
Javier Martínez Canillas
+595 981 88 66 58
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] staging/wlags49_h2: Fix build error when CONFIG_SYSFS

[Dan Carpenter]

On Thu, Jun 17, 2010 at 07:23:32PM +0200, Henk de Groot wrote:
> Javier,
>
> I assumed Greg KH already added this last version of your patch to the  
> queue for the staging tree. Anyway I think it's fine. The only problem I  
> see is a possible complained from the compiler because of an argument  
> that's defined but never used.

Gcc never complains about arguments that aren't used.  You often need to
do this so that your function matches how a function pointer is defined.

I also tested this.  To disable CONFIG_SYSFS do a "make allnoconfig"
then enable CONFIG_EMBEDDED, then you can disable CONFIG_SYSFS.  It's
deliberately a pain in the arse I suppose.

It's not a huge deal to have a #define either, but Joe Perches is right
that the current fix really is the better one.

regards,
dan carpenter

Re: [PATCH] staging/wlags49_h2: Fix build error when CONFIG_SYSFS

[Henk de Groot]

Hello Javier,

Op 17-6-2010 19:46, Javier Martinez Canillas schreef:
> 2) #ifdefs are ugly
>
> Code cluttered with ifdefs is difficult to read and maintain.  Don't do
> it. Instead, put your ifdefs in a header, and conditionally define
> 'static inline'
>    

That's very true. The wlags49_h2 driver is full of them and it would 
improve readability  the code to clean it up.

> So I discard this option. Do you want me to do it anyway and send a
> new patch? I'm willing to solve it the right way.
>    

Please do not listen to me. Your patch is fine. I'm not a regular kernel 
hacker so I don't know the rules either. Its more likely you know more 
about it than me already.

Of course I have idea's about the code and how to write it but I believe 
it is more important to have a unified standard across the kernel. I'm 
not seasoned enough to comment what's right or wrong with respect to the 
established coding standard. For example my style is to always put {} in 
the body of an "if" but I learned that its against the kernel coding 
standard if the body is a single statement. And that has to take have 
preference over my own ideas to get uniform code.

I know this driver does not keep up with the standards (yet?), 
especially the HCF library part (the files that do not start with a wl_ 
prefix). It might even be generated by some design tool because Agere 
did some strange things with structures and its very hard to read an 
follow as it is.

Anyway, please keep the patch as is, unless somebody with much more 
kernel knowledge comes along and tells us how it was supposed to be 
done. And I think they already said its fine now.

Kind regards,

Henk.