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

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

On Sat, Jan 16, 2021 at 5:48 PM Marek Vasut [off-list ref] 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

On 1/16/21 6:04 PM, Arnd Bergmann wrote:

On Sat, Jan 16, 2021 at 5:48 PM Marek Vasut [off-list ref] wrote:

quoted

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 ?

On Sat, Jan 16, 2021 at 6:56 PM Marek Vasut [off-list ref] wrote:

On 1/16/21 6:04 PM, Arnd Bergmann wrote:

quoted

On Sat, Jan 16, 2021 at 5:48 PM Marek Vasut [off-list ref] wrote:

quoted

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

On Sat, Jan 16, 2021 at 08:26:22PM +0100, Arnd Bergmann wrote:

On Sat, Jan 16, 2021 at 6:56 PM Marek Vasut [off-list ref] wrote:

quoted

On 1/16/21 6:04 PM, Arnd Bergmann wrote:

quoted

On Sat, Jan 16, 2021 at 5:48 PM Marek Vasut [off-list ref] wrote:

quoted

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

On 1/16/21 9:39 PM, Lukas Wunner wrote:

On Sat, Jan 16, 2021 at 08:26:22PM +0100, Arnd Bergmann wrote:

quoted

On Sat, Jan 16, 2021 at 6:56 PM Marek Vasut [off-list ref] wrote:

quoted

On 1/16/21 6:04 PM, Arnd Bergmann wrote:

quoted

On Sat, Jan 16, 2021 at 5:48 PM Marek Vasut [off-list ref] wrote:

quoted

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.

On 16.01.2021 22:25, Marek Vasut wrote:

On 1/16/21 9:39 PM, Lukas Wunner wrote:

quoted

On Sat, Jan 16, 2021 at 08:26:22PM +0100, Arnd Bergmann wrote:

quoted

On Sat, Jan 16, 2021 at 6:56 PM Marek Vasut [off-list ref] wrote:

quoted

On 1/16/21 6:04 PM, Arnd Bergmann wrote:

quoted

On Sat, Jan 16, 2021 at 5:48 PM Marek Vasut [off-list ref] wrote:

quoted

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.

On Sat, Jan 16, 2021 at 10:41 PM Heiner Kallweit [off-list ref] wrote:

quoted

quoted

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