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

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

[Marek Vasut]

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 passing THIS_MODULE as argument to ks8851_probe_common(),
ks8851_register_mdiobus(), and ultimately __mdiobus_register() in the
ks8851_common.c.

Fixes: ef3631220d2b ("net: ks8851: Register MDIO bus and the internal PHY")
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Lukas Wunner <lukas@wunner.de>
---
V2: Pass the THIS_MODULE into ks8851_common.c
---
 drivers/net/ethernet/micrel/ks8851.h        | 2 +-
 drivers/net/ethernet/micrel/ks8851_common.c | 9 +++++----
 drivers/net/ethernet/micrel/ks8851_par.c    | 2 +-
 drivers/net/ethernet/micrel/ks8851_spi.c    | 2 +-
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.h b/drivers/net/ethernet/micrel/ks8851.h
index ef13929036cf..037138fc6cb4 100644
--- a/drivers/net/ethernet/micrel/ks8851.h
+++ b/drivers/net/ethernet/micrel/ks8851.h
@@ -428,7 +428,7 @@ struct ks8851_net {
 };
 
 int ks8851_probe_common(struct net_device *netdev, struct device *dev,
-			int msg_en);
+			int msg_en, struct module *owner);
 int ks8851_remove_common(struct device *dev);
 int ks8851_suspend(struct device *dev);
 int ks8851_resume(struct device *dev);
diff --git a/drivers/net/ethernet/micrel/ks8851_common.c b/drivers/net/ethernet/micrel/ks8851_common.c
index f1996787bba5..88303ba4869d 100644
--- a/drivers/net/ethernet/micrel/ks8851_common.c
+++ b/drivers/net/ethernet/micrel/ks8851_common.c
@@ -1104,7 +1104,8 @@ int ks8851_resume(struct device *dev)
 }
 #endif
 
-static int ks8851_register_mdiobus(struct ks8851_net *ks, struct device *dev)
+static int ks8851_register_mdiobus(struct ks8851_net *ks, struct device *dev,
+				   struct module *owner)
 {
 	struct phy_device *phy_dev;
 	struct mii_bus *mii_bus;
@@ -1122,7 +1123,7 @@ static int ks8851_register_mdiobus(struct ks8851_net *ks, struct device *dev)
 	mii_bus->phy_mask = ~((u32)BIT(0));
 	snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
 
-	ret = mdiobus_register(mii_bus);
+	ret = __mdiobus_register(mii_bus, owner);
 	if (ret)
 		goto err_mdiobus_register;
 
@@ -1149,7 +1150,7 @@ static void ks8851_unregister_mdiobus(struct ks8851_net *ks)
 }
 
 int ks8851_probe_common(struct net_device *netdev, struct device *dev,
-			int msg_en)
+			int msg_en, struct module *owner)
 {
 	struct ks8851_net *ks = netdev_priv(netdev);
 	unsigned cider;
@@ -1224,7 +1225,7 @@ int ks8851_probe_common(struct net_device *netdev, struct device *dev,
 
 	dev_info(dev, "message enable is %d\n", msg_en);
 
-	ret = ks8851_register_mdiobus(ks, dev);
+	ret = ks8851_register_mdiobus(ks, dev, owner);
 	if (ret)
 		goto err_mdio;
 
diff --git a/drivers/net/ethernet/micrel/ks8851_par.c b/drivers/net/ethernet/micrel/ks8851_par.c
index 3bab0cb2b1a5..d6fc53d3efbb 100644
--- a/drivers/net/ethernet/micrel/ks8851_par.c
+++ b/drivers/net/ethernet/micrel/ks8851_par.c
@@ -324,7 +324,7 @@ static int ks8851_probe_par(struct platform_device *pdev)
 
 	netdev->irq = platform_get_irq(pdev, 0);
 
-	return ks8851_probe_common(netdev, dev, msg_enable);
+	return ks8851_probe_common(netdev, dev, msg_enable, THIS_MODULE);
 }
 
 static int ks8851_remove_par(struct platform_device *pdev)
diff --git a/drivers/net/ethernet/micrel/ks8851_spi.c b/drivers/net/ethernet/micrel/ks8851_spi.c
index 4ec7f1615977..9fbb7a548580 100644
--- a/drivers/net/ethernet/micrel/ks8851_spi.c
+++ b/drivers/net/ethernet/micrel/ks8851_spi.c
@@ -451,7 +451,7 @@ static int ks8851_probe_spi(struct spi_device *spi)
 
 	netdev->irq = spi->irq;
 
-	return ks8851_probe_common(netdev, dev, msg_enable);
+	return ks8851_probe_common(netdev, dev, msg_enable, THIS_MODULE);
 }
 
 static int ks8851_remove_spi(struct spi_device *spi)
-- 
2.29.2

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

[Arnd Bergmann]

On Sat, Jan 16, 2021 at 5:48 PM Marek Vasut <marex@denx.de> 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 passing THIS_MODULE as argument to ks8851_probe_common(),
> ks8851_register_mdiobus(), and ultimately __mdiobus_register() in the
> ks8851_common.c.
>
> Fixes: ef3631220d2b ("net: ks8851: Register MDIO bus and the internal PHY")
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Lukas Wunner <lukas@wunner.de>

I don't really like this version, as it does not actually solve the problem of
linking the same object file into both vmlinux and a loadable module, which
can have all kinds of side-effects besides that link failure you saw.

If you want to avoid exporting all those symbols, a simpler hack would
be to '#include "ks8851_common.c" from each of the two files, which
then always duplicates the contents (even when both are built-in), but
at least builds the file the correct way.

       Arnd

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

[Marek Vasut]

On 1/16/21 6:04 PM, Arnd Bergmann wrote:
> On Sat, Jan 16, 2021 at 5:48 PM Marek Vasut <marex@denx.de> 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 passing THIS_MODULE as argument to ks8851_probe_common(),
>> ks8851_register_mdiobus(), and ultimately __mdiobus_register() in the
>> ks8851_common.c.
>>
>> Fixes: ef3631220d2b ("net: ks8851: Register MDIO bus and the internal PHY")
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Andrew Lunn <andrew@lunn.ch>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>> Cc: Jakub Kicinski <kuba@kernel.org>
>> Cc: Lukas Wunner <lukas@wunner.de>
> 
> I don't really like this version, as it does not actually solve the problem of
> linking the same object file into both vmlinux and a loadable module, which
> can have all kinds of side-effects besides that link failure you saw.
> 
> If you want to avoid exporting all those symbols, a simpler hack would
> be to '#include "ks8851_common.c" from each of the two files, which
> then always duplicates the contents (even when both are built-in), but
> at least builds the file the correct way.

That's the same as V1, isn't it ?

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

[Arnd Bergmann]

On Sat, Jan 16, 2021 at 6:56 PM Marek Vasut <marex@denx.de> wrote:
> On 1/16/21 6:04 PM, Arnd Bergmann wrote:
> > On Sat, Jan 16, 2021 at 5:48 PM Marek Vasut <marex@denx.de> wrote:
>
> > I don't really like this version, as it does not actually solve the problem of
> > linking the same object file into both vmlinux and a loadable module, which
> > can have all kinds of side-effects besides that link failure you saw.
> >
> > If you want to avoid exporting all those symbols, a simpler hack would
> > be to '#include "ks8851_common.c" from each of the two files, which
> > then always duplicates the contents (even when both are built-in), but
> > at least builds the file the correct way.
>
> That's the same as V1, isn't it ?

Ah, I had not actually looked at the original submission, but yes, that
was slightly better than v2, provided you make all symbols static to
avoid the new link error.

I still think that having three modules and exporting the symbols from
the common part as Heiner Kallweit suggested would be the best
way to do it.

        Arnd

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

[Lukas Wunner]

On Sat, Jan 16, 2021 at 08:26:22PM +0100, Arnd Bergmann wrote:
> On Sat, Jan 16, 2021 at 6:56 PM Marek Vasut <marex@denx.de> wrote:
> > On 1/16/21 6:04 PM, Arnd Bergmann wrote:
> > > On Sat, Jan 16, 2021 at 5:48 PM Marek Vasut <marex@denx.de> wrote:
> >
> > > I don't really like this version, as it does not actually solve the problem of
> > > linking the same object file into both vmlinux and a loadable module, which
> > > can have all kinds of side-effects besides that link failure you saw.
> > >
> > > If you want to avoid exporting all those symbols, a simpler hack would
> > > be to '#include "ks8851_common.c" from each of the two files, which
> > > then always duplicates the contents (even when both are built-in), but
> > > at least builds the file the correct way.
> >
> > That's the same as V1, isn't it ?
> 
> Ah, I had not actually looked at the original submission, but yes, that
> was slightly better than v2, provided you make all symbols static to
> avoid the new link error.
> 
> I still think that having three modules and exporting the symbols from
> the common part as Heiner Kallweit suggested would be the best
> way to do it.

FWIW I'd prefer V1 (the #include approach) as it allows going back to
using static inlines for register access.  That's what we had before
7a552c850c45.

It seems unlikely that a system uses both, the parallel *and* the SPI
variant of the ks8851.  So the additional memory necessary because of
code duplication wouldn't matter in practice.

Thanks,

Lukas

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

[Marek Vasut]

On 1/16/21 9:39 PM, Lukas Wunner wrote:
> On Sat, Jan 16, 2021 at 08:26:22PM +0100, Arnd Bergmann wrote:
>> On Sat, Jan 16, 2021 at 6:56 PM Marek Vasut <marex@denx.de> wrote:
>>> On 1/16/21 6:04 PM, Arnd Bergmann wrote:
>>>> On Sat, Jan 16, 2021 at 5:48 PM Marek Vasut <marex@denx.de> wrote:
>>>
>>>> I don't really like this version, as it does not actually solve the problem of
>>>> linking the same object file into both vmlinux and a loadable module, which
>>>> can have all kinds of side-effects besides that link failure you saw.
>>>>
>>>> If you want to avoid exporting all those symbols, a simpler hack would
>>>> be to '#include "ks8851_common.c" from each of the two files, which
>>>> then always duplicates the contents (even when both are built-in), but
>>>> at least builds the file the correct way.
>>>
>>> That's the same as V1, isn't it ?
>>
>> Ah, I had not actually looked at the original submission, but yes, that
>> was slightly better than v2, provided you make all symbols static to
>> avoid the new link error.
>>
>> I still think that having three modules and exporting the symbols from
>> the common part as Heiner Kallweit suggested would be the best
>> way to do it.
> 
> FWIW I'd prefer V1 (the #include approach) as it allows going back to
> using static inlines for register access.  That's what we had before
> 7a552c850c45.
> 
> It seems unlikely that a system uses both, the parallel *and* the SPI
> variant of the ks8851.  So the additional memory necessary because of
> code duplication wouldn't matter in practice.

I have a board with both options populated on my desk, sorry.

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

[Heiner Kallweit]

On 16.01.2021 22:25, Marek Vasut wrote:
> On 1/16/21 9:39 PM, Lukas Wunner wrote:
>> On Sat, Jan 16, 2021 at 08:26:22PM +0100, Arnd Bergmann wrote:
>>> On Sat, Jan 16, 2021 at 6:56 PM Marek Vasut <marex@denx.de> wrote:
>>>> On 1/16/21 6:04 PM, Arnd Bergmann wrote:
>>>>> On Sat, Jan 16, 2021 at 5:48 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>>> I don't really like this version, as it does not actually solve the problem of
>>>>> linking the same object file into both vmlinux and a loadable module, which
>>>>> can have all kinds of side-effects besides that link failure you saw.
>>>>>
>>>>> If you want to avoid exporting all those symbols, a simpler hack would
>>>>> be to '#include "ks8851_common.c" from each of the two files, which
>>>>> then always duplicates the contents (even when both are built-in), but
>>>>> at least builds the file the correct way.
>>>>
>>>> That's the same as V1, isn't it ?
>>>
>>> Ah, I had not actually looked at the original submission, but yes, that
>>> was slightly better than v2, provided you make all symbols static to
>>> avoid the new link error.
>>>
>>> I still think that having three modules and exporting the symbols from
>>> the common part as Heiner Kallweit suggested would be the best
>>> way to do it.
>>
>> FWIW I'd prefer V1 (the #include approach) as it allows going back to
>> using static inlines for register access.  That's what we had before
>> 7a552c850c45.
>>
>> It seems unlikely that a system uses both, the parallel *and* the SPI
>> variant of the ks8851.  So the additional memory necessary because of
>> code duplication wouldn't matter in practice.
> 
> I have a board with both options populated on my desk, sorry.

Making the common part a separate module shouldn't be that hard.
AFAICS it would just take:
- export 4 functions from common
- extend Kconfig
- extend Makefile
One similar configuration that comes to my mind and could be used as
template is SPI_FSL_LIB.

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

[Arnd Bergmann]

On Sat, Jan 16, 2021 at 10:41 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
> >>
> >> It seems unlikely that a system uses both, the parallel *and* the SPI
> >> variant of the ks8851.  So the additional memory necessary because of
> >> code duplication wouldn't matter in practice.
> >
> > I have a board with both options populated on my desk, sorry.
>
> Making the common part a separate module shouldn't be that hard.
> AFAICS it would just take:
> - export 4 functions from common
> - extend Kconfig
> - extend Makefile
> One similar configuration that comes to my mind and could be used as
> template is SPI_FSL_LIB.

There is no need to even change Kconfig, just simplify the Makefile to

obj-$(CONFIG_KS8851) += ks8851_common.o ks8851_spi.o
obj-$(CONFIG_KS8851_MLL) += ks8851_common.o ks8851_par.o

This will do the right thing and build ks8851_common.ko into
vmlinux if at least one of the two front-ends is built-in, and
otherwise build it at a loadable module if there is another
module using it.

         Arnd