[2.6 patch] drivers/isdn/gigaset/: build asyncdata.o into the gigaset module

[2.6 patch] drivers/isdn/gigaset/: build asyncdata.o into the gigaset module

[Adrian Bunk]

This patch fixes the following compile error:

<--  snip  -->

...
  LD      drivers/isdn/gigaset/built-in.o
drivers/isdn/gigaset/ser_gigaset.o: In function `gigaset_m10x_send_skb':
(.text+0xe50): multiple definition of `gigaset_m10x_send_skb'
drivers/isdn/gigaset/usb_gigaset.o:(.text+0x0): first defined here
drivers/isdn/gigaset/ser_gigaset.o: In function `gigaset_m10x_input':
(.text+0x1121): multiple definition of `gigaset_m10x_input'
drivers/isdn/gigaset/usb_gigaset.o:(.text+0x2d1): first defined here
make[4]: *** [drivers/isdn/gigaset/built-in.o] Error 1

<--  snip  -->

Signed-off-by: Adrian Bunk <bunk@stusta.de>

---

 drivers/isdn/gigaset/Makefile    |    6 +++---
 drivers/isdn/gigaset/asyncdata.c |    2 ++
 2 files changed, 5 insertions(+), 3 deletions(-)

--- linux-2.6.20-mm1/drivers/isdn/gigaset/Makefile.old	2007-02-15 19:35:30.000000000 +0100
+++ linux-2.6.20-mm1/drivers/isdn/gigaset/Makefile	2007-02-15 19:35:46.000000000 +0100
@@ -1,7 +1,7 @@
-gigaset-y := common.o interface.o proc.o ev-layer.o i4l.o
-usb_gigaset-y := usb-gigaset.o asyncdata.o
+gigaset-y := common.o interface.o proc.o ev-layer.o i4l.o asyncdata.o
+usb_gigaset-y := usb-gigaset.o
 bas_gigaset-y := bas-gigaset.o isocdata.o
-ser_gigaset-y := ser-gigaset.o asyncdata.o
+ser_gigaset-y := ser-gigaset.o
 
 obj-$(CONFIG_GIGASET_M105) += usb_gigaset.o gigaset.o
 obj-$(CONFIG_GIGASET_BASE) += bas_gigaset.o gigaset.o
--- linux-2.6.20-mm1/drivers/isdn/gigaset/asyncdata.c.old	2007-02-15 19:36:14.000000000 +0100
+++ linux-2.6.20-mm1/drivers/isdn/gigaset/asyncdata.c	2007-02-15 19:37:08.000000000 +0100
@@ -444,6 +444,7 @@
 		atomic_set(&inbuf->head, head);
 	}
 }
+EXPORT_SYMBOL_GPL(gigaset_m10x_input);
 
 
 /* == data output ========================================================== */
@@ -591,3 +592,4 @@
 
 	return len;	/* ok so far */
 }
+EXPORT_SYMBOL_GPL(gigaset_m10x_send_skb);

Re: [2.6 patch] drivers/isdn/gigaset/: build asyncdata.o into the gigaset module

[Tilman Schmidt]

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

Am 15.02.2007 22:56 schrieb Adrian Bunk:
> This patch fixes the following compile error:
> 
> <--  snip  -->
> 
> ...
>   LD      drivers/isdn/gigaset/built-in.o
> drivers/isdn/gigaset/ser_gigaset.o: In function `gigaset_m10x_send_skb':
> (.text+0xe50): multiple definition of `gigaset_m10x_send_skb'
> drivers/isdn/gigaset/usb_gigaset.o:(.text+0x0): first defined here
> drivers/isdn/gigaset/ser_gigaset.o: In function `gigaset_m10x_input':
> (.text+0x1121): multiple definition of `gigaset_m10x_input'
> drivers/isdn/gigaset/usb_gigaset.o:(.text+0x2d1): first defined here
> make[4]: *** [drivers/isdn/gigaset/built-in.o] Error 1

How did you manage to produce that error? I have never encountered it.

Thanks,
Tilman

-- 
Tilman Schmidt                          E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 253 bytes --]

Re: [2.6 patch] drivers/isdn/gigaset/: build asyncdata.o into the gigaset module

[Adrian Bunk]

On Fri, Feb 16, 2007 at 01:17:19AM +0100, Tilman Schmidt wrote:
> Am 15.02.2007 22:56 schrieb Adrian Bunk:
> > This patch fixes the following compile error:
> > 
> > <--  snip  -->
> > 
> > ...
> >   LD      drivers/isdn/gigaset/built-in.o
> > drivers/isdn/gigaset/ser_gigaset.o: In function `gigaset_m10x_send_skb':
> > (.text+0xe50): multiple definition of `gigaset_m10x_send_skb'
> > drivers/isdn/gigaset/usb_gigaset.o:(.text+0x0): first defined here
> > drivers/isdn/gigaset/ser_gigaset.o: In function `gigaset_m10x_input':
> > (.text+0x1121): multiple definition of `gigaset_m10x_input'
> > drivers/isdn/gigaset/usb_gigaset.o:(.text+0x2d1): first defined here
> > make[4]: *** [drivers/isdn/gigaset/built-in.o] Error 1
> 
> How did you manage to produce that error? I have never encountered it.

CONFIG_ISDN_DRV_GIGASET=y
CONFIG_GIGASET_BASE=y
CONFIG_GIGASET_M105=y
CONFIG_GIGASET_M101=y
CONFIG_GIGASET_DEBUG=y
CONFIG_GIGASET_UNDOCREQ=y

It might depend on the binutils version.

> Thanks,
> Tilman

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Kbuild problem (was: [2.6 patch] drivers/isdn/gigaset/: build asyncdata.o into the gigaset module)

[Tilman Schmidt]

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

Am 16.02.2007 09:33 schrieb Adrian Bunk:
>>> ...
>>>   LD      drivers/isdn/gigaset/built-in.o
>>> drivers/isdn/gigaset/ser_gigaset.o: In function `gigaset_m10x_send_skb':
>>> (.text+0xe50): multiple definition of `gigaset_m10x_send_skb'
>>> drivers/isdn/gigaset/usb_gigaset.o:(.text+0x0): first defined here
>>> drivers/isdn/gigaset/ser_gigaset.o: In function `gigaset_m10x_input':
>>> (.text+0x1121): multiple definition of `gigaset_m10x_input'
>>> drivers/isdn/gigaset/usb_gigaset.o:(.text+0x2d1): first defined here
>>> make[4]: *** [drivers/isdn/gigaset/built-in.o] Error 1

> CONFIG_ISDN_DRV_GIGASET=y
> CONFIG_GIGASET_BASE=y
> CONFIG_GIGASET_M105=y
> CONFIG_GIGASET_M101=y

Ah, thanks, I can see what's happening now. When both the M101 and M105
drivers are built into the kernel, asyncdata.o gets linked in twice, via

	usb_gigaset-y := usb-gigaset.o asyncdata.o
	obj-$(CONFIG_GIGASET_M105) += usb_gigaset.o gigaset.o

and

	ser_gigaset-y := ser-gigaset.o asyncdata.o
	obj-$(CONFIG_GIGASET_M101) += ser_gigaset.o gigaset.o

The assertion in Documentation/kbuild/makefiles.txt:

        The order of files in $(obj-y) is significant.  Duplicates in
        the lists are allowed: the first instance will be linked into
        built-in.o and succeeding instances will be ignored.

doesn't work out in this case because asyncdata.o isn't added directly
to $(obj-y), but as part of usb_gigaset.o and ser_gigaset.o.

I'm not quite happy with your solution, though. I'd prefer a
Makefile which builds modular usb_gigaset.ko and/or ser_gigaset.ko
like the present one (including asyncdata.o), but when linking
usb-gigaset.o and ser-gigaset.o into the kernel includes asyncdata.o
only once. Trouble is, I don't know how to express that in Kbuild.

Any ideas?

Thanks,
Tilman

-- 
Tilman Schmidt                          E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 253 bytes --]

Re: Kbuild problem (was: [2.6 patch] drivers/isdn/gigaset/: build asyncdata.o into the gigaset module)

[Adrian Bunk]

On Sat, Feb 17, 2007 at 01:04:33AM +0100, Tilman Schmidt wrote:
> Am 16.02.2007 09:33 schrieb Adrian Bunk:
> >>> ...
> >>>   LD      drivers/isdn/gigaset/built-in.o
> >>> drivers/isdn/gigaset/ser_gigaset.o: In function `gigaset_m10x_send_skb':
> >>> (.text+0xe50): multiple definition of `gigaset_m10x_send_skb'
> >>> drivers/isdn/gigaset/usb_gigaset.o:(.text+0x0): first defined here
> >>> drivers/isdn/gigaset/ser_gigaset.o: In function `gigaset_m10x_input':
> >>> (.text+0x1121): multiple definition of `gigaset_m10x_input'
> >>> drivers/isdn/gigaset/usb_gigaset.o:(.text+0x2d1): first defined here
> >>> make[4]: *** [drivers/isdn/gigaset/built-in.o] Error 1
> 
> > CONFIG_ISDN_DRV_GIGASET=y
> > CONFIG_GIGASET_BASE=y
> > CONFIG_GIGASET_M105=y
> > CONFIG_GIGASET_M101=y
> 
> Ah, thanks, I can see what's happening now. When both the M101 and M105
> drivers are built into the kernel, asyncdata.o gets linked in twice, via
> 
> 	usb_gigaset-y := usb-gigaset.o asyncdata.o
> 	obj-$(CONFIG_GIGASET_M105) += usb_gigaset.o gigaset.o
> 
> and
> 
> 	ser_gigaset-y := ser-gigaset.o asyncdata.o
> 	obj-$(CONFIG_GIGASET_M101) += ser_gigaset.o gigaset.o
> 
> The assertion in Documentation/kbuild/makefiles.txt:
> 
>         The order of files in $(obj-y) is significant.  Duplicates in
>         the lists are allowed: the first instance will be linked into
>         built-in.o and succeeding instances will be ignored.
> 
> doesn't work out in this case because asyncdata.o isn't added directly
> to $(obj-y), but as part of usb_gigaset.o and ser_gigaset.o.
> 
> I'm not quite happy with your solution, though. I'd prefer a
> Makefile which builds modular usb_gigaset.ko and/or ser_gigaset.ko
> like the present one (including asyncdata.o), but when linking
> usb-gigaset.o and ser-gigaset.o into the kernel includes asyncdata.o
> only once. Trouble is, I don't know how to express that in Kbuild.
> 
> Any ideas?

CONFIG_GIGASET_M105=y, CONFIG_GIGASET_M101=m and similar problems might 
make this quite tricky.

Shared functionality simply doesn't belong into any of the affected 
modules, but to a different place where all users can access it.

> Thanks,
> Tilman

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: Kbuild problem

[Tilman Schmidt]

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

Am 17.02.2007 11:52 schrieb Adrian Bunk:
> On Sat, Feb 17, 2007 at 01:04:33AM +0100, Tilman Schmidt wrote:
>> [...] I'd prefer a
>> Makefile which builds modular usb_gigaset.ko and/or ser_gigaset.ko
>> like the present one (including asyncdata.o), but when linking
>> usb-gigaset.o and ser-gigaset.o into the kernel includes asyncdata.o
>> only once. Trouble is, I don't know how to express that in Kbuild.
>>
>> Any ideas?
> 
> CONFIG_GIGASET_M105=y, CONFIG_GIGASET_M101=m and similar problems might 
> make this quite tricky.

I am no judge of that. All I know about the possibilities of Kbuild
is what the files in Documentation/kbuild tell me. From browsing
through other kernel Makefiles it would appear that there are more
possibilities though, such as if/then/else constructs.

> Shared functionality simply doesn't belong into any of the affected 
> modules, but to a different place where all users can access it.

Alright, then so be it. But that raises another question:
asyncdata.o is only needed for M105 and M101, not for the base
driver. How do I express in Kbuild that asyncdata.o is to be added
to gigaset-y only if CONFIG_GIGASET_M105 and CONFIG_GIGASET_M101
are not both 'n'?

Thanks,
Tilman

-- 
Tilman Schmidt                          E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 253 bytes --]

Re: Kbuild problem

[Kai Germaschewski]

On Sat, 17 Feb 2007, Tilman Schmidt wrote:

> Alright, then so be it. But that raises another question:
> asyncdata.o is only needed for M105 and M101, not for the base
> driver. How do I express in Kbuild that asyncdata.o is to be added
> to gigaset-y only if CONFIG_GIGASET_M105 and CONFIG_GIGASET_M101
> are not both 'n'?

The way this is typically done is via Kconfig. Add a config option 
ASYNCDATA (actually something more descriptive/specific would be better), 
add a "select ASYNCDATA" to the config options for m101 and m105, and then 
you can use CONFIG_ASYNCDATA to decide whether to add asyncdata.o in the 
Makefile.

--Kai

Re: Kbuild problem

[Adrian Bunk]

On Sun, Feb 18, 2007 at 12:22:10AM -0500, Kai Germaschewski wrote:
> On Sat, 17 Feb 2007, Tilman Schmidt wrote:
> 
> > Alright, then so be it. But that raises another question:
> > asyncdata.o is only needed for M105 and M101, not for the base
> > driver. How do I express in Kbuild that asyncdata.o is to be added
> > to gigaset-y only if CONFIG_GIGASET_M105 and CONFIG_GIGASET_M101
> > are not both 'n'?
> 
> The way this is typically done is via Kconfig. Add a config option 
> ASYNCDATA (actually something more descriptive/specific would be better), 
> add a "select ASYNCDATA" to the config options for m101 and m105, and then 
> you can use CONFIG_ASYNCDATA to decide whether to add asyncdata.o in the 
> Makefile.

One disadvantage of this approach is that in a kernel with 
CONFIG_GIGASET_BASE=y, you can't later compile and load the usb_gigaset 
or ser_gigaset modules without rebooting since they require a change to 
the kernel image.

> --Kai

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: Kbuild problem

[Tilman Schmidt]

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

Adrian Bunk schrieb:
> On Sun, Feb 18, 2007 at 12:22:10AM -0500, Kai Germaschewski wrote:
>> On Sat, 17 Feb 2007, Tilman Schmidt wrote:
>> 
>> > asyncdata.o is only needed for M105 and M101, not for the base
>> > driver. How do I express in Kbuild that asyncdata.o is to be added
>> > to gigaset-y only if CONFIG_GIGASET_M105 and CONFIG_GIGASET_M101
>> > are not both 'n'?
>> 
>> The way this is typically done is via Kconfig. Add a config option 
>> ASYNCDATA (actually something more descriptive/specific would be better), 
>> add a "select ASYNCDATA" to the config options for m101 and m105, and then 
>> you can use CONFIG_ASYNCDATA to decide whether to add asyncdata.o in the 
>> Makefile.
> 
> One disadvantage of this approach is that in a kernel with 
> CONFIG_GIGASET_BASE=y, you can't later compile and load the usb_gigaset 
> or ser_gigaset modules without rebooting since they require a change to 
> the kernel image.

You've got a point there. So linking asyncdata.o into the modules that
need it, as it is currently done, would perhaps be better after all?
The original problem (asyncdata.o linked in twice, causing duplicate
symbol definitions) could be fixed with this (admittedly somewhat
awkward) change to the Makefile (build tested):

--- linux-2.6.20-mm1-work/drivers/isdn/gigaset/Makefile 2007-02-20 13:39:44.000000000 +0100
+++ u/drivers/isdn/gigaset/Makefile     2007-02-20 13:38:58.000000000 +0100
@@ -1,7 +1,10 @@
 gigaset-y := common.o interface.o proc.o ev-layer.o i4l.o
 usb_gigaset-y := usb-gigaset.o asyncdata.o
 bas_gigaset-y := bas-gigaset.o isocdata.o
-ser_gigaset-y := ser-gigaset.o asyncdata.o
+ser_gigaset-y := ser-gigaset.o
+ifneq ($(CONFIG_GIGASET_M101)$(CONFIG_GIGASET_M105),yy)
+ser_gigaset-y += asyncdata.o
+endif

 obj-$(CONFIG_GIGASET_M105) += usb_gigaset.o gigaset.o
 obj-$(CONFIG_GIGASET_BASE) += bas_gigaset.o gigaset.o

The alternative would be to always link asyncdata.o into the gigaset
module whether it's needed or not. "size asyncdata.o" says:
   text    data     bss     dec     hex filename
   4200       0       0    4200    1068 asyncdata.o
which appears tolerable.

Opinions?

-- 
Tilman Schmidt                    E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: Kbuild problem

[Joerg Dorchain]

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

On Tue, Feb 20, 2007 at 02:56:27PM +0100, Tilman Schmidt wrote:
> > One disadvantage of this approach is that in a kernel with 
> > CONFIG_GIGASET_BASE=y, you can't later compile and load the usb_gigaset 
> > or ser_gigaset modules without rebooting since they require a change to 
> > the kernel image.
> 
> You've got a point there. So linking asyncdata.o into the modules that
> need it, as it is currently done, would perhaps be better after all?

From the architectural point of view, what odds against making it a
module of its own? Dependancies are resolved by modprobe, so users should
be fine.
There are other library-like parts in the kernel where an object is
built statically when at least one needing driver is static, a module
when all nedding drivers are modules, or not at all but appears are a
config option when no in-kernel driver needs it.

> 
> The alternative would be to always link asyncdata.o into the gigaset
> module whether it's needed or not. "size asyncdata.o" says:
>    text    data     bss     dec     hex filename
>    4200       0       0    4200    1068 asyncdata.o
> which appears tolerable.

Ugly. But I've seen worse.

Bye,

Joerg

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Kbuild problem

[Adrian Bunk]

On Tue, Feb 20, 2007 at 02:56:27PM +0100, Tilman Schmidt wrote:
> Adrian Bunk schrieb:
> > On Sun, Feb 18, 2007 at 12:22:10AM -0500, Kai Germaschewski wrote:
> >> On Sat, 17 Feb 2007, Tilman Schmidt wrote:
> >> 
> >> > asyncdata.o is only needed for M105 and M101, not for the base
> >> > driver. How do I express in Kbuild that asyncdata.o is to be added
> >> > to gigaset-y only if CONFIG_GIGASET_M105 and CONFIG_GIGASET_M101
> >> > are not both 'n'?
> >> 
> >> The way this is typically done is via Kconfig. Add a config option 
> >> ASYNCDATA (actually something more descriptive/specific would be better), 
> >> add a "select ASYNCDATA" to the config options for m101 and m105, and then 
> >> you can use CONFIG_ASYNCDATA to decide whether to add asyncdata.o in the 
> >> Makefile.
> > 
> > One disadvantage of this approach is that in a kernel with 
> > CONFIG_GIGASET_BASE=y, you can't later compile and load the usb_gigaset 
> > or ser_gigaset modules without rebooting since they require a change to 
> > the kernel image.
> 
> You've got a point there. So linking asyncdata.o into the modules that
> need it, as it is currently done, would perhaps be better after all?
> The original problem (asyncdata.o linked in twice, causing duplicate
> symbol definitions) could be fixed with this (admittedly somewhat
> awkward) change to the Makefile (build tested):
> 
> --- linux-2.6.20-mm1-work/drivers/isdn/gigaset/Makefile 2007-02-20 13:39:44.000000000 +0100
> +++ u/drivers/isdn/gigaset/Makefile     2007-02-20 13:38:58.000000000 +0100
> @@ -1,7 +1,10 @@
>  gigaset-y := common.o interface.o proc.o ev-layer.o i4l.o
>  usb_gigaset-y := usb-gigaset.o asyncdata.o
>  bas_gigaset-y := bas-gigaset.o isocdata.o
> -ser_gigaset-y := ser-gigaset.o asyncdata.o
> +ser_gigaset-y := ser-gigaset.o
> +ifneq ($(CONFIG_GIGASET_M101)$(CONFIG_GIGASET_M105),yy)
> +ser_gigaset-y += asyncdata.o
> +endif

What happens with CONFIG_GIGASET_M101=y, CONFIG_GIGASET_M105=m ?

>  obj-$(CONFIG_GIGASET_M105) += usb_gigaset.o gigaset.o
>  obj-$(CONFIG_GIGASET_BASE) += bas_gigaset.o gigaset.o
> 
> The alternative would be to always link asyncdata.o into the gigaset
> module whether it's needed or not. "size asyncdata.o" says:
>    text    data     bss     dec     hex filename
>    4200       0       0    4200    1068 asyncdata.o
> which appears tolerable.
> 
> Opinions?

I'm usually someone who likes to avoid including unneeded code in the 
kernel, but in this case I'd say KISS - and build it always into the 
gigaset module.

> Tilman Schmidt

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: Kbuild problem

[Tilman Schmidt]

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

Am 20.02.2007 15:59 schrieb Adrian Bunk:
> I'm usually someone who likes to avoid including unneeded code in the 
> kernel, but in this case I'd say KISS - and build it always into the 
> gigaset module.

There seems to be a clear majority for this solution. So KISS it
will be. Thanks for all the advice. I'll do a new patch.

-- 
Tilman Schmidt                          E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 253 bytes --]