From: Carlos Martín <carlos@cmartin.tk>

From: Carlos Martín <carlos@cmartin.tk>

[Carlos Martin]

[PATCH] acxsm: Fix Kconfig option check

This check never actually worked because CONFIG_ACX_{ACX,USB} are
tristate. With Adrian Bunk's patch to the Kconfig, this works with the
_BOOL hidden Kconfig options.
Also update error message adding that this shouldn't happen anymore.

Signed-off-by: Carlos Martin <carlos@cmartin.tk>
---

 acx_struct.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/acx_struct.h b/acx_struct.h
index 93495e0..3237974 100644
--- a/acx_struct.h
+++ b/acx_struct.h
@@ -105,8 +105,9 @@ enum { acx_debug = 0 };
 #define DEVTYPE_PCI		0
 #define DEVTYPE_USB		1
 
-#if !defined(CONFIG_ACX_PCI) && !defined(CONFIG_ACX_USB)
+#if !defined(CONFIG_ACX_PCI_BOOL) && !defined(CONFIG_ACX_USB_BOOL)
 #error Driver must include PCI and/or USB support. You selected neither.
+#error This really should not happen anymore. Please report.
 #endif
 
 #if defined(CONFIG_ACX_PCI)



-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid\x103432&bid#0486&dat\x121642

Re: From: Carlos Martín <carlos@cmartin.tk>

[Carlos Martín]

Oops, sorry about that, I'll send a properly formed e-mail. I'm still managing 
to get stuff wrong with git-send-email.perl

   cmn
-- 
Carlos Martín Nieto    |   http://www.cmartin.tk
Hobbyist programmer    |


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid\x103432&bid#0486&dat\x121642

Re: From: Carlos Martц╜n <carlos@cmartin.tk>

[Denis Vlasenko]

On Sunday 19 February 2006 00:35, Carlos Martin wrote:
> [PATCH] acxsm: Fix Kconfig option check
> 
> This check never actually worked because CONFIG_ACX_{ACX,USB} are
> tristate. With Adrian Bunk's patch to the Kconfig, this works with the
> _BOOL hidden Kconfig options.
> Also update error message adding that this shouldn't happen anymore.

Carlos, I didn't apply Adrian's patch to my tree.

I am not sure we want to support every imaginable .config.
Realistic goal is:
* allnoconfig must compile
* allyesconfig must compile
* allmodconfig must compile

Adrian's patch makes code more obfuscated and it still has one
corner case (when one choice is 'y' and other is 'm')

If you disagree, please explain why do you want Adrian's patch
applied.
--
vda


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642

Re: From: Carlos Martц╜n <carlos@cmartin.tk>

[Carlos Martín]

On Monday 20 February 2006 16:30, Denis Vlasenko wrote:
> On Sunday 19 February 2006 00:35, Carlos Martin wrote:
> > [PATCH] acxsm: Fix Kconfig option check
> > 
> > This check never actually worked because CONFIG_ACX_{ACX,USB} are
> > tristate. With Adrian Bunk's patch to the Kconfig, this works with the
> > _BOOL hidden Kconfig options.
> > Also update error message adding that this shouldn't happen anymore.
> 
> Carlos, I didn't apply Adrian's patch to my tree.
> 
> I am not sure we want to support every imaginable .config.
> Realistic goal is:
> * allnoconfig must compile
> * allyesconfig must compile
> * allmodconfig must compile
Which doesn't work with your #error condition.

> 
> Adrian's patch makes code more obfuscated and it still has one
> corner case (when one choice is 'y' and other is 'm')
> 
> If you disagree, please explain why do you want Adrian's patch
> applied.

It at least compiles. Your tree doesn't compile. It always tells me that I 
didn't choose either USB or PCI support which is incorrect.

I've just tested this and CONFIG_ACX_{PCI,USB} only get defined if they are 
set to Y. If they're set to M, it doesn't, so it spews out the warning.

I don't like Adrian's patch, and I don't think it's the right way to do it, 
but it's what we have that works. I'll try to think of something a bit more 
elegant meanwhile.

   cmn
-- 
Carlos Martín Nieto    |   http://www.cmartin.tk
Hobbyist programmer    |


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid\x103432&bid#0486&dat\x121642

Re: From: Carlos Martц╜n <carlos@cmartin.tk>

[Denis Vlasenko]

[-- Attachment #1: Type: text/plain, Size: 971 bytes --]

On Monday 20 February 2006 20:56, Carlos Martín wrote:
> On Monday 20 February 2006 16:30, Denis Vlasenko wrote:
> > On Sunday 19 February 2006 00:35, Carlos Martin wrote:
> > > [PATCH] acxsm: Fix Kconfig option check
> > > 
> > > This check never actually worked because CONFIG_ACX_{ACX,USB} are
> > > tristate. With Adrian Bunk's patch to the Kconfig, this works with the
> > > _BOOL hidden Kconfig options.
> > > Also update error message adding that this shouldn't happen anymore.
> > 
> > Carlos, I didn't apply Adrian's patch to my tree.
> > 
> > I am not sure we want to support every imaginable .config.
> > Realistic goal is:
> > * allnoconfig must compile
> > * allyesconfig must compile
> > * allmodconfig must compile
> Which doesn't work with your #error condition.

allmodconfig for acx is:

CONFIG_ACX=m
CONFIG_ACX_PCI=y
CONFIG_ACX_USB=y

and it compiles for me just fine. Please send me your .config.
Mine is attached.
--
vda

[-- Attachment #2: .config.bz2 --]
[-- Type: application/x-bzip2, Size: 9085 bytes --]

Re: From: Carlos Martц╜n <carlos@cmartin.tk>

[Carlos Martín]

On Tuesday 21 February 2006 07:17, Denis Vlasenko wrote:
> 
> allmodconfig for acx is:
> 
> CONFIG_ACX=m
> CONFIG_ACX_PCI=y
> CONFIG_ACX_USB=y
> 
> and it compiles for me just fine. Please send me your .config.
> Mine is attached.

My bad, sorry. I forgot to copy the Kconfig file over, so I was getting 
tristate ACX_{PCI,USB} instead of bools.

I had

CONFIG_ACX=m
CONFIG_ACX_PCI=m
CONFIG_ACX_PCI_BOOL=y
CONFIG_ACX_USB=m
CONFIG_ACX_USB_BOOL=y

So of course it was failing. Once I copied Kconfig over, all goes according to 
plan.

   cmn
-- 
Carlos Martín Nieto    |   http://www.cmartin.tk
Hobbyist programmer    |


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid\x103432&bid#0486&dat\x121642

Re: From: Carlos Mart??n <carlos@cmartin.tk>

[Christoph Hellwig]

On Sat, Feb 18, 2006 at 11:35:21PM +0100, Carlos Martin wrote:
> [PATCH] acxsm: Fix Kconfig option check
> 
> This check never actually worked because CONFIG_ACX_{ACX,USB} are
> tristate. With Adrian Bunk's patch to the Kconfig, this works with the
> _BOOL hidden Kconfig options.
> Also update error message adding that this shouldn't happen anymore.

All the fixes discussed so far are not very nice at all.  The right
fix is the following:


 - split the module into three:
	acx-common.ko
	acx-pci.ko
	acx-usb.ko

 - make CONFIG_ACX_PCI and CONFIG_ACX_USB user-visible tristate
   varibles in the Kconfig.
 - build acx-common.ko when either of those is selected, with the
   following makefile:

---- snip ----
acx-common-y		+= wlan.o conv.o ioctl.o common.o
acx-pci-y		+= pci.o
acx-usb-y		+= usb.o

obj-$(CONFIG_ACX_PCI)	+= acx-common.o acx-pci.o
obj-$(CONFIG_ACX_USB)	+= acx-common.o acx-usb.o
---- snip ----

 - kill the IS_PCI/IS_USB macros and add a acx_operations structure that
   handles the different hardware without branches all over and allows
   the hw-specific code to be in separate modules.


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642

Re: From: Carlos Mart??n <carlos@cmartin.tk>

[Carlos Martín]

On Tuesday 21 February 2006 20:26, Christoph Hellwig wrote:
> On Sat, Feb 18, 2006 at 11:35:21PM +0100, Carlos Martin wrote:
> > [PATCH] acxsm: Fix Kconfig option check
> > 
> > This check never actually worked because CONFIG_ACX_{ACX,USB} are
> > tristate. With Adrian Bunk's patch to the Kconfig, this works with the
> > _BOOL hidden Kconfig options.
> > Also update error message adding that this shouldn't happen anymore.
> 
> All the fixes discussed so far are not very nice at all.  The right
> fix is the following:
> 
> 
>  - split the module into three:
> 	acx-common.ko
> 	acx-pci.ko
> 	acx-usb.ko

I don't really like this method. It's not for any real reason though, so I'll 
go with it if people want it. I've used this for the rt2x00 driver by loading 
it manually, which was really annoying, but I suppose when it's properly 
installed it works well.

> 
>  - make CONFIG_ACX_PCI and CONFIG_ACX_USB user-visible tristate
>    varibles in the Kconfig.
>  - build acx-common.ko when either of those is selected, with the
>    following makefile:
> 
> ---- snip ----
> acx-common-y		+= wlan.o conv.o ioctl.o common.o
> acx-pci-y		+= pci.o
> acx-usb-y		+= usb.o
> 
> obj-$(CONFIG_ACX_PCI)	+= acx-common.o acx-pci.o
> obj-$(CONFIG_ACX_USB)	+= acx-common.o acx-usb.o

This is how we had it before, which leads to having a lot of the same code on 
both modules. The unified driver is not much larger so it was made this way, 
and that's where all the problem comes from.

Wouldn't this lead to duplicated function definitions at link time if both are 
compiled-in? From your module split above I understood you wanted acx-common 
to be another module, but here I see it goes into the modules.

> ---- snip ----
> 
>  - kill the IS_PCI/IS_USB macros and add a acx_operations structure that
>    handles the different hardware without branches all over and allows
>    the hw-specific code to be in separate modules.

There aren't that many IS_{PCI,USB} uses and most if not all are justified 
(extra step for one case). It might be a good idea to do that for the 
IS_ACX{100,111} macros instead of calling the generic function which then 
calls the chip-specific one.

   cmn
-- 
Carlos Martín Nieto    |   http://www.cmartin.tk
Hobbyist programmer    |


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid\x103432&bid#0486&dat\x121642

Re: From: Carlos Mart??n <carlos@cmartin.tk>

[Christoph Hellwig]

On Tue, Feb 21, 2006 at 09:24:23PM +0100, Carlos Mart?n wrote:
> > acx-common-y		+= wlan.o conv.o ioctl.o common.o
> > acx-pci-y		+= pci.o
> > acx-usb-y		+= usb.o
> > 
> > obj-$(CONFIG_ACX_PCI)	+= acx-common.o acx-pci.o
> > obj-$(CONFIG_ACX_USB)	+= acx-common.o acx-usb.o
> 
> This is how we had it before, which leads to having a lot of the same code on 
> both modules. The unified driver is not much larger so it was made this way, 
> and that's where all the problem comes from.
> Wouldn't this lead to duplicated function definitions at link time if both are 
> compiled-in? From your module split above I understood you wanted acx-common 
> to be another module, but here I see it goes into the modules.
> 

No.  The above makefile fragment builds three modules: acx-common.o,
acx-pci.o and acx-usb.o as mentioned above.  The magic here is that with
that makefile fragment is that the kbuild systems builds acx-common.o if
either CONFIG_ACX_PCI or CONFIG_ACX_USB is set, and even makes sure to
do the right thing if either is builtin.  There is not code duplication
at all.

> > ---- snip ----
> > 
> >  - kill the IS_PCI/IS_USB macros and add a acx_operations structure that
> >    handles the different hardware without branches all over and allows
> >    the hw-specific code to be in separate modules.
> 
> There aren't that many IS_{PCI,USB} uses and most if not all are justified 
> (extra step for one case). It might be a good idea to do that for the 
> IS_ACX{100,111} macros instead of calling the generic function which then 
> calls the chip-specific one.

The important bit is that you need the pointers with the above module
spit, because you can't call usb- or pci-specific routines from acx-common.ko


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642

Re: From: Carlos Mart??n <carlos@cmartin.tk>

[Carlos Martín]

On Tuesday 21 February 2006 21:32, Christoph Hellwig wrote:
> On Tue, Feb 21, 2006 at 09:24:23PM +0100, Carlos Mart?n wrote:
> > Wouldn't this lead to duplicated function definitions at link time if both 
are 
> > compiled-in? From your module split above I understood you wanted 
acx-common 
> > to be another module, but here I see it goes into the modules.
> > 
> 
> No.  The above makefile fragment builds three modules: acx-common.o,
> acx-pci.o and acx-usb.o as mentioned above.  The magic here is that with
> that makefile fragment is that the kbuild systems builds acx-common.o if
> either CONFIG_ACX_PCI or CONFIG_ACX_USB is set, and even makes sure to
> do the right thing if either is builtin.  There is not code duplication
> at all.

Then all is good.

> 
> > > ---- snip ----
> > > 
> > >  - kill the IS_PCI/IS_USB macros and add a acx_operations structure that
> > >    handles the different hardware without branches all over and allows
> > >    the hw-specific code to be in separate modules.
> > 
> > There aren't that many IS_{PCI,USB} uses and most if not all are justified 
> > (extra step for one case). It might be a good idea to do that for the 
> > IS_ACX{100,111} macros instead of calling the generic function which then 
> > calls the chip-specific one.
> 
> The important bit is that you need the pointers with the above module
> spit, because you can't call usb- or pci-specific routines from 
> acx-common.ko 

Yes, I realise that (unless you export them, but I don't think we want that). 
I've started this, but I think it'll probably be next week before I have time 
to really work on it.

This approach is probably better even if the driver is unified. Pointer 
dereferences are cheaper than branches/jumping, aren't they?

   cmn
-- 
Carlos Martín Nieto    |   http://www.cmartin.tk
Hobbyist programmer    |


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid\x103432&bid#0486&dat\x121642

Re: From: Carlos Mart??n <carlos@cmartin.tk>

[Christoph Hellwig]

On Tue, Feb 21, 2006 at 10:03:58PM +0100, Carlos Mart?n wrote:
> > The important bit is that you need the pointers with the above module
> > spit, because you can't call usb- or pci-specific routines from 
> > acx-common.ko 
> 
> Yes, I realise that (unless you export them, but I don't think we want that). 

even that wouldn't work with current module because the usb and pci modules
call into the common code and thus we'd have recursive module depency.

> This approach is probably better even if the driver is unified. Pointer 
> dereferences are cheaper than branches/jumping, aren't they?

It shouldn't matter these days as cpus have nice branch prediction.


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642

Re: From: Carlos Mart??n <carlos@cmartin.tk>

[Carlos Martín]

On Wednesday 22 February 2006 15:36, Christoph Hellwig wrote:
> On Tue, Feb 21, 2006 at 10:03:58PM +0100, Carlos Mart?n wrote:
> > > The important bit is that you need the pointers with the above module
> > > spit, because you can't call usb- or pci-specific routines from 
> > > acx-common.ko 
> > 
> > Yes, I realise that (unless you export them, but I don't think we want 
that). 
> 
> even that wouldn't work with current module because the usb and pci modules
> call into the common code and thus we'd have recursive module depency.

And you may not build both modules, so acx-common.ko would never be satisfied. 
Function pointers all the way then.

> 
> > This approach is probably better even if the driver is unified. Pointer 
> > dereferences are cheaper than branches/jumping, aren't they?
> 
> It shouldn't matter these days as cpus have nice branch prediction.

I remember reading P4s have problems with this because they had really long 
branch pipes (the proper name escapes me at the moment) and if they got it 
wrong, it took quite a bit of time (in CPU terms, of course) to flush them. I 
don't use them personally, but I believe the trend is to do that.

   cmn
-- 
Carlos Martín Nieto    |   http://www.cmartin.tk
Hobbyist programmer    |


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid\x103432&bid#0486&dat\x121642