From: Carlos M. <ca...@cm...> - 2006-02-18 22:34:09
|
[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 <ca...@cm...> --- 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 =3D 0 }; #define DEVTYPE_PCI 0 #define DEVTYPE_USB 1 =20 -#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 =20 #if defined(CONFIG_ACX_PCI) |
From: Carlos <ca...@cm...> - 2006-02-18 22:37:05
|
Oops, sorry about that, I'll send a properly formed e-mail. I'm still manag= ing=20 to get stuff wrong with git-send-email.perl cmn =2D-=20 Carlos Mart=EDn Nieto | http://www.cmartin.tk Hobbyist programmer | |
From: Denis V. <vd...@il...> - 2006-02-20 15:31:23
|
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 |
From: Carlos <ca...@cm...> - 2006-02-20 18:54:36
|
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 > >=20 > > 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. >=20 > Carlos, I didn't apply Adrian's patch to my tree. >=20 > 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. >=20 > Adrian's patch makes code more obfuscated and it still has one > corner case (when one choice is 'y' and other is 'm') >=20 > 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= =20 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= =20 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,= =20 but it's what we have that works. I'll try to think of something a bit more= =20 elegant meanwhile. cmn =2D-=20 Carlos Mart=EDn Nieto | http://www.cmartin.tk Hobbyist programmer | |
From: Denis V. <vd...@il...> - 2006-02-21 06:18:24
Attachments:
.config.bz2
|
On Monday 20 February 2006 20:56, Carlos Mart=EDn 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 > > >=20 > > > 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. > >=20 > > Carlos, I didn't apply Adrian's patch to my tree. > >=20 > > 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=3Dm CONFIG_ACX_PCI=3Dy CONFIG_ACX_USB=3Dy and it compiles for me just fine. Please send me your .config. Mine is attached. =2D- vda |
From: Carlos <ca...@cm...> - 2006-02-21 14:30:41
|
On Tuesday 21 February 2006 07:17, Denis Vlasenko wrote: >=20 > allmodconfig for acx is: >=20 > CONFIG_ACX=3Dm > CONFIG_ACX_PCI=3Dy > CONFIG_ACX_USB=3Dy >=20 > 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=20 tristate ACX_{PCI,USB} instead of bools. I had CONFIG_ACX=3Dm CONFIG_ACX_PCI=3Dm CONFIG_ACX_PCI_BOOL=3Dy CONFIG_ACX_USB=3Dm CONFIG_ACX_USB_BOOL=3Dy So of course it was failing. Once I copied Kconfig over, all goes according= to=20 plan. cmn =2D-=20 Carlos Mart=C3=ADn Nieto | http://www.cmartin.tk Hobbyist programmer | |
From: Christoph H. <hc...@in...> - 2006-02-21 19:26:11
|
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. |
From: Carlos <ca...@cm...> - 2006-02-21 20:22:52
|
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 > >=20 > > 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. >=20 > All the fixes discussed so far are not very nice at all. The right > fix is the following: >=20 >=20 > - 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=20 go with it if people want it. I've used this for the rt2x00 driver by loadi= ng=20 it manually, which was really annoying, but I suppose when it's properly=20 installed it works well. >=20 > - 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: >=20 > ---- snip ---- > acx-common-y +=3D wlan.o conv.o ioctl.o common.o > acx-pci-y +=3D pci.o > acx-usb-y +=3D usb.o >=20 > obj-$(CONFIG_ACX_PCI) +=3D acx-common.o acx-pci.o > obj-$(CONFIG_ACX_USB) +=3D acx-common.o acx-usb.o This is how we had it before, which leads to having a lot of the same code = on=20 both modules. The unified driver is not much larger so it was made this way= ,=20 and that's where all the problem comes from. Wouldn't this lead to duplicated function definitions at link time if both = are=20 compiled-in? From your module split above I understood you wanted acx-commo= n=20 to be another module, but here I see it goes into the modules. > ---- snip ---- >=20 > - 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= =20 (extra step for one case). It might be a good idea to do that for the=20 IS_ACX{100,111} macros instead of calling the generic function which then=20 calls the chip-specific one. cmn =2D-=20 Carlos Mart=EDn Nieto | http://www.cmartin.tk Hobbyist programmer | |
From: Christoph H. <hc...@in...> - 2006-02-21 20:32:24
|
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 |
From: Carlos <ca...@cm...> - 2006-02-21 21:02:26
|
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 b= oth=20 are=20 > > compiled-in? From your module split above I understood you wanted=20 acx-common=20 > > to be another module, but here I see it goes into the modules. > >=20 >=20 > 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. >=20 > > > ---- snip ---- > > >=20 > > > - kill the IS_PCI/IS_USB macros and add a acx_operations structure t= hat > > > handles the different hardware without branches all over and allows > > > the hw-specific code to be in separate modules. > >=20 > > There aren't that many IS_{PCI,USB} uses and most if not all are justif= ied=20 > > (extra step for one case). It might be a good idea to do that for the=20 > > IS_ACX{100,111} macros instead of calling the generic function which th= en=20 > > calls the chip-specific one. >=20 > 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=20 > acx-common.ko=20 Yes, I realise that (unless you export them, but I don't think we want that= ).=20 I've started this, but I think it'll probably be next week before I have ti= me=20 to really work on it. This approach is probably better even if the driver is unified. Pointer=20 dereferences are cheaper than branches/jumping, aren't they? cmn =2D-=20 Carlos Mart=EDn Nieto | http://www.cmartin.tk Hobbyist programmer | |
From: Christoph H. <hc...@in...> - 2006-02-22 14:36:19
|
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. |
From: Carlos <ca...@cm...> - 2006-02-22 15:04:53
|
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=20 > > > acx-common.ko=20 > >=20 > > Yes, I realise that (unless you export them, but I don't think we want= =20 that).=20 >=20 > even that wouldn't work with current module because the usb and pci modul= es > 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 satisfi= ed.=20 =46unction pointers all the way then. >=20 > > This approach is probably better even if the driver is unified. Pointer= =20 > > dereferences are cheaper than branches/jumping, aren't they? >=20 > 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= =20 branch pipes (the proper name escapes me at the moment) and if they got it= =20 wrong, it took quite a bit of time (in CPU terms, of course) to flush them.= I=20 don't use them personally, but I believe the trend is to do that. cmn =2D-=20 Carlos Mart=EDn Nieto | http://www.cmartin.tk Hobbyist programmer | |