| Message ID | 20210116164828.40545-1-marex@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,V2] net: ks8851: Fix mixed module/builtin build | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | fail | Patch does not apply to net-next |
| netdev/tree_selection | success | Clearly marked for net-next |
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
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 ?
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
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
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.
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.
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
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)
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(-)