diff mbox series

[net-next] net: ks8851: Fix mixed module/builtin build

Message ID 20210115134239.126152-1-marex@denx.de (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: ks8851: Fix mixed module/builtin build | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 1 maintainers not CCed: davem@davemloft.net
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 2 this patch: 2
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 25 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 2 this patch: 2
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Marek Vasut Jan. 15, 2021, 1:42 p.m. UTC
When either the SPI or PAR variant is compiled as module AND the other
variant is compiled as built-in, the following build error occurs:

arm-linux-gnueabi-ld: drivers/net/ethernet/micrel/ks8851_common.o: in function `ks8851_probe_common':
ks8851_common.c:(.text+0x1564): undefined reference to `__this_module'

Fix this by including the ks8851_common.c in both ks8851_spi.c and
ks8851_par.c. The DEBUG macro is defined in ks8851_common.c, so it
does not have to be defined again.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Lukas Wunner <lukas@wunner.de>
---
NOTE: Maybe there is a better way?
---
 drivers/net/ethernet/micrel/Makefile     | 4 ++--
 drivers/net/ethernet/micrel/ks8851_par.c | 2 +-
 drivers/net/ethernet/micrel/ks8851_spi.c | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

Comments

Andrew Lunn Jan. 15, 2021, 3 p.m. UTC | #1
On Fri, Jan 15, 2021 at 02:42:39PM +0100, Marek Vasut wrote:
> When either the SPI or PAR variant is compiled as module AND the other
> variant is compiled as built-in, the following build error occurs:
> 
> arm-linux-gnueabi-ld: drivers/net/ethernet/micrel/ks8851_common.o: in function `ks8851_probe_common':
> ks8851_common.c:(.text+0x1564): undefined reference to `__this_module'
> 
> Fix this by including the ks8851_common.c in both ks8851_spi.c and
> ks8851_par.c. The DEBUG macro is defined in ks8851_common.c, so it
> does not have to be defined again.

DEBUG should not be defined for production code. So i would remove it
altogether.

There is kconfig'ury you can use to make them both the same. But i'm
not particularly good with it.

    Andrew
Marek Vasut Jan. 15, 2021, 3:05 p.m. UTC | #2
On 1/15/21 4:00 PM, Andrew Lunn wrote:
> On Fri, Jan 15, 2021 at 02:42:39PM +0100, Marek Vasut wrote:
>> When either the SPI or PAR variant is compiled as module AND the other
>> variant is compiled as built-in, the following build error occurs:
>>
>> arm-linux-gnueabi-ld: drivers/net/ethernet/micrel/ks8851_common.o: in function `ks8851_probe_common':
>> ks8851_common.c:(.text+0x1564): undefined reference to `__this_module'
>>
>> Fix this by including the ks8851_common.c in both ks8851_spi.c and
>> ks8851_par.c. The DEBUG macro is defined in ks8851_common.c, so it
>> does not have to be defined again.
> 
> DEBUG should not be defined for production code. So i would remove it
> altogether.
> 
> There is kconfig'ury you can use to make them both the same. But i'm
> not particularly good with it.

We had discussion about this module/builtin topic in ks8851 before, so I 
was hoping someone might provide a better suggestion.
Andrew Lunn Jan. 15, 2021, 3:59 p.m. UTC | #3
On Fri, Jan 15, 2021 at 04:05:57PM +0100, Marek Vasut wrote:
> On 1/15/21 4:00 PM, Andrew Lunn wrote:
> > On Fri, Jan 15, 2021 at 02:42:39PM +0100, Marek Vasut wrote:
> > > When either the SPI or PAR variant is compiled as module AND the other
> > > variant is compiled as built-in, the following build error occurs:
> > > 
> > > arm-linux-gnueabi-ld: drivers/net/ethernet/micrel/ks8851_common.o: in function `ks8851_probe_common':
> > > ks8851_common.c:(.text+0x1564): undefined reference to `__this_module'
> > > 
> > > Fix this by including the ks8851_common.c in both ks8851_spi.c and
> > > ks8851_par.c. The DEBUG macro is defined in ks8851_common.c, so it
> > > does not have to be defined again.
> > 
> > DEBUG should not be defined for production code. So i would remove it
> > altogether.
> > 
> > There is kconfig'ury you can use to make them both the same. But i'm
> > not particularly good with it.
> 
> We had discussion about this module/builtin topic in ks8851 before, so I was
> hoping someone might provide a better suggestion.

Try Arnd Bergmann. He is good with this sort of thing.

    Andrew
Marek Vasut Jan. 15, 2021, 4:08 p.m. UTC | #4
On 1/15/21 4:59 PM, Andrew Lunn wrote:
> On Fri, Jan 15, 2021 at 04:05:57PM +0100, Marek Vasut wrote:
>> On 1/15/21 4:00 PM, Andrew Lunn wrote:
>>> On Fri, Jan 15, 2021 at 02:42:39PM +0100, Marek Vasut wrote:
>>>> When either the SPI or PAR variant is compiled as module AND the other
>>>> variant is compiled as built-in, the following build error occurs:
>>>>
>>>> arm-linux-gnueabi-ld: drivers/net/ethernet/micrel/ks8851_common.o: in function `ks8851_probe_common':
>>>> ks8851_common.c:(.text+0x1564): undefined reference to `__this_module'
>>>>
>>>> Fix this by including the ks8851_common.c in both ks8851_spi.c and
>>>> ks8851_par.c. The DEBUG macro is defined in ks8851_common.c, so it
>>>> does not have to be defined again.
>>>
>>> DEBUG should not be defined for production code. So i would remove it
>>> altogether.
>>>
>>> There is kconfig'ury you can use to make them both the same. But i'm
>>> not particularly good with it.
>>
>> We had discussion about this module/builtin topic in ks8851 before, so I was
>> hoping someone might provide a better suggestion.
> 
> Try Arnd Bergmann. He is good with this sort of thing.

Adding to CC.
Heiner Kallweit Jan. 15, 2021, 5:22 p.m. UTC | #5
On 15.01.2021 16:59, Andrew Lunn wrote:
> On Fri, Jan 15, 2021 at 04:05:57PM +0100, Marek Vasut wrote:
>> On 1/15/21 4:00 PM, Andrew Lunn wrote:
>>> On Fri, Jan 15, 2021 at 02:42:39PM +0100, Marek Vasut wrote:
>>>> When either the SPI or PAR variant is compiled as module AND the other
>>>> variant is compiled as built-in, the following build error occurs:
>>>>
>>>> arm-linux-gnueabi-ld: drivers/net/ethernet/micrel/ks8851_common.o: in function `ks8851_probe_common':
>>>> ks8851_common.c:(.text+0x1564): undefined reference to `__this_module'
>>>>
>>>> Fix this by including the ks8851_common.c in both ks8851_spi.c and
>>>> ks8851_par.c. The DEBUG macro is defined in ks8851_common.c, so it
>>>> does not have to be defined again.
>>>
>>> DEBUG should not be defined for production code. So i would remove it
>>> altogether.
>>>
>>> There is kconfig'ury you can use to make them both the same. But i'm
>>> not particularly good with it.
>>
>> We had discussion about this module/builtin topic in ks8851 before, so I was
>> hoping someone might provide a better suggestion.
> 
> Try Arnd Bergmann. He is good with this sort of thing.
> 
>     Andrew
> .
> 
I'd say make ks8851_common.c a separate module. Then, if one of SPI / PAR
is built in, ks8851_common needs to be built in too. To do so you'd have
export all symbols from ks8851_common that you want to use in SPI /PAR.
kernel test robot Jan. 15, 2021, 7:51 p.m. UTC | #6
Hi Marek,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Marek-Vasut/net-ks8851-Fix-mixed-module-builtin-build/20210115-215024
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 1d9f03c0a15fa01aa14fb295cbc1236403fceb0b
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/e11965ba009b3247ecbbb87730c724405eae8753
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Marek-Vasut/net-ks8851-Fix-mixed-module-builtin-build/20210115-215024
        git checkout e11965ba009b3247ecbbb87730c724405eae8753
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   mips-linux-ld: drivers/net/ethernet/micrel/ks8851_par.o: in function `ks8851_resume':
>> ks8851_par.c:(.text.ks8851_resume+0x0): multiple definition of `ks8851_resume'; drivers/net/ethernet/micrel/ks8851_spi.o:ks8851_spi.c:(.text.ks8851_resume+0x0): first defined here
   mips-linux-ld: drivers/net/ethernet/micrel/ks8851_par.o: in function `ks8851_suspend':
>> ks8851_par.c:(.text.ks8851_suspend+0x0): multiple definition of `ks8851_suspend'; drivers/net/ethernet/micrel/ks8851_spi.o:ks8851_spi.c:(.text.ks8851_suspend+0x0): first defined here
   mips-linux-ld: drivers/net/ethernet/micrel/ks8851_par.o: in function `ks8851_probe_common':
>> ks8851_par.c:(.text.ks8851_probe_common+0x0): multiple definition of `ks8851_probe_common'; drivers/net/ethernet/micrel/ks8851_spi.o:ks8851_spi.c:(.text.ks8851_probe_common+0x0): first defined here
   mips-linux-ld: drivers/net/ethernet/micrel/ks8851_par.o: in function `ks8851_remove_common':
>> ks8851_par.c:(.text.ks8851_remove_common+0x0): multiple definition of `ks8851_remove_common'; drivers/net/ethernet/micrel/ks8851_spi.o:ks8851_spi.c:(.text.ks8851_remove_common+0x0): first defined here

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot Jan. 15, 2021, 7:51 p.m. UTC | #7
Hi Marek,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Marek-Vasut/net-ks8851-Fix-mixed-module-builtin-build/20210115-215024
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 1d9f03c0a15fa01aa14fb295cbc1236403fceb0b
config: riscv-allyesconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/e11965ba009b3247ecbbb87730c724405eae8753
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Marek-Vasut/net-ks8851-Fix-mixed-module-builtin-build/20210115-215024
        git checkout e11965ba009b3247ecbbb87730c724405eae8753
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   riscv64-linux-ld: drivers/net/ethernet/micrel/ks8851_par.o: in function `ks8851_remove_common':
>> (.text+0x469a): multiple definition of `ks8851_remove_common'; drivers/net/ethernet/micrel/ks8851_spi.o:(.text+0x47a4): first defined here
   riscv64-linux-ld: drivers/net/ethernet/micrel/ks8851_par.o: in function `ks8851_probe_common':
>> (.text+0x360c): multiple definition of `ks8851_probe_common'; drivers/net/ethernet/micrel/ks8851_spi.o:(.text+0x3570): first defined here

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Arnd Bergmann Jan. 15, 2021, 9:36 p.m. UTC | #8
On Fri, Jan 15, 2021 at 6:24 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
> On 15.01.2021 16:59, Andrew Lunn wrote:
> > On Fri, Jan 15, 2021 at 04:05:57PM +0100, Marek Vasut wrote:
> >> On 1/15/21 4:00 PM, Andrew Lunn wrote:
> >>> On Fri, Jan 15, 2021 at 02:42:39PM +0100, Marek Vasut wrote:
> >>>> When either the SPI or PAR variant is compiled as module AND the other
> >>>> variant is compiled as built-in, the following build error occurs:
> >>>>
> >>>> arm-linux-gnueabi-ld: drivers/net/ethernet/micrel/ks8851_common.o: in function `ks8851_probe_common':
> >>>> ks8851_common.c:(.text+0x1564): undefined reference to `__this_module'
> >>>>
> >>>> Fix this by including the ks8851_common.c in both ks8851_spi.c and
> >>>> ks8851_par.c. The DEBUG macro is defined in ks8851_common.c, so it
> >>>> does not have to be defined again.
> >>>
> >>> DEBUG should not be defined for production code. So i would remove it
> >>> altogether.
> >>>
> >>> There is kconfig'ury you can use to make them both the same. But i'm
> >>> not particularly good with it.
> >>
> >> We had discussion about this module/builtin topic in ks8851 before, so I was
> >> hoping someone might provide a better suggestion.
> >
> > Try Arnd Bergmann. He is good with this sort of thing.
> >
> I'd say make ks8851_common.c a separate module. Then, if one of SPI / PAR
> is built in, ks8851_common needs to be built in too. To do so you'd have
> export all symbols from ks8851_common that you want to use in SPI /PAR.

Yes, that should work, as long the common module does not reference
any symbols from the other two modules (it normally wouldn't), and all
globals in the common one are exported.

You can also link everything into a single module, but then you have
to deal with registering two device_driver structures from a single
init function, which would undo some of cleanup.

       Arnd
Marek Vasut Jan. 16, 2021, 4:49 p.m. UTC | #9
On 1/15/21 10:36 PM, Arnd Bergmann wrote:
> On Fri, Jan 15, 2021 at 6:24 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>> On 15.01.2021 16:59, Andrew Lunn wrote:
>>> On Fri, Jan 15, 2021 at 04:05:57PM +0100, Marek Vasut wrote:
>>>> On 1/15/21 4:00 PM, Andrew Lunn wrote:
>>>>> On Fri, Jan 15, 2021 at 02:42:39PM +0100, Marek Vasut wrote:
>>>>>> When either the SPI or PAR variant is compiled as module AND the other
>>>>>> variant is compiled as built-in, the following build error occurs:
>>>>>>
>>>>>> arm-linux-gnueabi-ld: drivers/net/ethernet/micrel/ks8851_common.o: in function `ks8851_probe_common':
>>>>>> ks8851_common.c:(.text+0x1564): undefined reference to `__this_module'
>>>>>>
>>>>>> Fix this by including the ks8851_common.c in both ks8851_spi.c and
>>>>>> ks8851_par.c. The DEBUG macro is defined in ks8851_common.c, so it
>>>>>> does not have to be defined again.
>>>>>
>>>>> DEBUG should not be defined for production code. So i would remove it
>>>>> altogether.
>>>>>
>>>>> There is kconfig'ury you can use to make them both the same. But i'm
>>>>> not particularly good with it.
>>>>
>>>> We had discussion about this module/builtin topic in ks8851 before, so I was
>>>> hoping someone might provide a better suggestion.
>>>
>>> Try Arnd Bergmann. He is good with this sort of thing.
>>>
>> I'd say make ks8851_common.c a separate module. Then, if one of SPI / PAR
>> is built in, ks8851_common needs to be built in too. To do so you'd have
>> export all symbols from ks8851_common that you want to use in SPI /PAR.
> 
> Yes, that should work, as long the common module does not reference
> any symbols from the other two modules (it normally wouldn't), and all
> globals in the common one are exported.
> 
> You can also link everything into a single module, but then you have
> to deal with registering two device_driver structures from a single
> init function, which would undo some of cleanup.

Maybe just passing THIS_MODULE around is even better way to do it, I 
CCed you on the V2, [PATCH net-next V2] net: ks8851: Fix mixed 
module/builtin build .
diff mbox series

Patch

diff --git a/drivers/net/ethernet/micrel/Makefile b/drivers/net/ethernet/micrel/Makefile
index 5cc00d22c708..1aedfe0ecaf1 100644
--- a/drivers/net/ethernet/micrel/Makefile
+++ b/drivers/net/ethernet/micrel/Makefile
@@ -5,7 +5,7 @@ 
 
 obj-$(CONFIG_KS8842) += ks8842.o
 obj-$(CONFIG_KS8851) += ks8851.o
-ks8851-objs = ks8851_common.o ks8851_spi.o
+ks8851-objs = ks8851_spi.o
 obj-$(CONFIG_KS8851_MLL) += ks8851_mll.o
-ks8851_mll-objs = ks8851_common.o ks8851_par.o
+ks8851_mll-objs = ks8851_par.o
 obj-$(CONFIG_KSZ884X_PCI) += ksz884x.o
diff --git a/drivers/net/ethernet/micrel/ks8851_par.c b/drivers/net/ethernet/micrel/ks8851_par.c
index 3bab0cb2b1a5..e9131cc8a57b 100644
--- a/drivers/net/ethernet/micrel/ks8851_par.c
+++ b/drivers/net/ethernet/micrel/ks8851_par.c
@@ -8,7 +8,7 @@ 
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#define DEBUG
+#include "ks8851_common.c"
 
 #include <linux/interrupt.h>
 #include <linux/module.h>
diff --git a/drivers/net/ethernet/micrel/ks8851_spi.c b/drivers/net/ethernet/micrel/ks8851_spi.c
index 4ec7f1615977..53779d66f747 100644
--- a/drivers/net/ethernet/micrel/ks8851_spi.c
+++ b/drivers/net/ethernet/micrel/ks8851_spi.c
@@ -8,7 +8,7 @@ 
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#define DEBUG
+#include "ks8851_common.c"
 
 #include <linux/interrupt.h>
 #include <linux/module.h>