| Message ID | 20180730203351.11260-1-chunkeey@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | John Crispin |
| Headers | show |
| Series | [OpenWrt-Devel] kernel: modules: fix kmod-regmap | expand |
On 30/07/18 22:33, Christian Lamparter wrote: > This patch fixes the a compile issue that was triggered by > apm821xx/sata when kmod-regmap was selected. > > The CONFIG_REGMAP is declared in drivers/base/regmap/Kconfig > as type "bool" and not "tristate". Hence the symbol should > never be set to module, as this confuses the #if CONFIG_REGMAP > guards in include/linux/regmap.h: > > |.../drivers/regulator/core.c:4041: undefined reference to `dev_get_regmap' > |.../drivers/regulator/core.c:4042: undefined reference to `dev_get_regmap' > |.../drivers/regulator/core.c:4044: undefined reference to `dev_get_regmap' > |.../drivers/regulator/helpers.o: In function `regulator_is_enabled_regmap': > |.../drivers/regulator/helpers.c:36: undefined reference to `regmap_read' > |... i started a test build 2 minutes ago to figure this one out :-) thanks ! > Signed-off-by: Christian Lamparter <chunkeey@gmail.com> > --- > package/kernel/linux/modules/other.mk | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/package/kernel/linux/modules/other.mk b/package/kernel/linux/modules/other.mk > index 7e18a21db3..dd037cf86c 100644 > --- a/package/kernel/linux/modules/other.mk > +++ b/package/kernel/linux/modules/other.mk > @@ -718,7 +718,7 @@ define KernelPackage/regmap > SUBMENU:=$(OTHER_MENU) > TITLE:=Generic register map support > DEPENDS:=+kmod-lib-lzo +kmod-i2c-core > - KCONFIG:=CONFIG_REGMAP \ > + KCONFIG:=CONFIG_REGMAP=y \ > CONFIG_REGMAP_MMIO \ > CONFIG_REGMAP_SPI \ > CONFIG_REGMAP_I2C \
On 30 July 2018 at 22:35, John Crispin <john@phrozen.org> wrote: > > > On 30/07/18 22:33, Christian Lamparter wrote: >> >> This patch fixes the a compile issue that was triggered by >> apm821xx/sata when kmod-regmap was selected. >> >> The CONFIG_REGMAP is declared in drivers/base/regmap/Kconfig >> as type "bool" and not "tristate". Hence the symbol should >> never be set to module, as this confuses the #if CONFIG_REGMAP >> guards in include/linux/regmap.h: >> >> |.../drivers/regulator/core.c:4041: undefined reference to >> `dev_get_regmap' >> |.../drivers/regulator/core.c:4042: undefined reference to >> `dev_get_regmap' >> |.../drivers/regulator/core.c:4044: undefined reference to >> `dev_get_regmap' >> |.../drivers/regulator/helpers.o: In function >> `regulator_is_enabled_regmap': >> |.../drivers/regulator/helpers.c:36: undefined reference to `regmap_read' >> |... > > i started a test build 2 minutes ago to figure this one out :-) thanks ! This is actually the wrong fix and papers over an issue in one of our local patches. We intentionally allow regmap to be built as a module, see https://github.com/openwrt/openwrt/blob/master/target/linux/generic/hack-4.14/259-regmap_dynamic.patch So there are likely some additional EXPORT_SYMBOL_GPL()s required instead. Regards Jonas
On Wednesday, August 1, 2018 3:21:00 PM CEST Jonas Gorski wrote: > On 30 July 2018 at 22:35, John Crispin <john@phrozen.org> wrote: > > On 30/07/18 22:33, Christian Lamparter wrote: > >> > >> This patch fixes the a compile issue that was triggered by > >> apm821xx/sata when kmod-regmap was selected. > >> > >> The CONFIG_REGMAP is declared in drivers/base/regmap/Kconfig > >> as type "bool" and not "tristate". Hence the symbol should > >> never be set to module, as this confuses the #if CONFIG_REGMAP > >> guards in include/linux/regmap.h: > >> > >> |.../drivers/regulator/core.c:4041: undefined reference to > >> `dev_get_regmap' > >> |.../drivers/regulator/core.c:4042: undefined reference to > >> `dev_get_regmap' > >> |.../drivers/regulator/core.c:4044: undefined reference to > >> `dev_get_regmap' > >> |.../drivers/regulator/helpers.o: In function > >> `regulator_is_enabled_regmap': > >> |.../drivers/regulator/helpers.c:36: undefined reference to `regmap_read' > >> |... > > > > i started a test build 2 minutes ago to figure this one out :-) thanks ! > > This is actually the wrong fix and papers over an issue in one of our > local patches. > > We intentionally allow regmap to be built as a module, see > > https://github.com/openwrt/openwrt/blob/master/target/linux/generic/hack-4.14/259-regmap_dynamic.patch > > So there are likely some additional EXPORT_SYMBOL_GPL()s required instead. Thanks for FYI, I missed this patch completely. I found that the issue lies in the upstream regulator core code. It is using these regmap's functions. And the CONFIG_REGULATOR symbol that pulls in the core's code is sadly a bool menuconfig option... so CONFIG_REGMAP pretty much becomes a requirement/select for the REGULATOR. --- diff --git a/target/linux/generic/hack-4.14/259-regmap_dynamic.patch b/target/linux/generic/hack-4.14/259-regmap_dynamic.patch index 1c6e78df30..35803c3181 100644 --- a/target/linux/generic/hack-4.14/259-regmap_dynamic.patch +++ b/target/linux/generic/hack-4.14/259-regmap_dynamic.patch @@ -9,8 +9,9 @@ Signed-off-by: Felix Fietkau <nbd@nbd.name> drivers/base/regmap/Kconfig | 15 ++++++++++----- drivers/base/regmap/Makefile | 12 ++++++++---- drivers/base/regmap/regmap.c | 3 +++ + drivers/regulator/Kconfig | 1 + include/linux/regmap.h | 2 +- - 4 files changed, 22 insertions(+), 10 deletions(-) + 5 files changed, 23 insertions(+), 10 deletions(-) --- a/drivers/base/regmap/Kconfig +++ b/drivers/base/regmap/Kconfig @@ -96,6 +97,15 @@ Signed-off-by: Felix Fietkau <nbd@nbd.name> postcore_initcall(regmap_initcall); + +MODULE_LICENSE("GPL"); +--- a/drivers/regulator/Kconfig ++++ b/drivers/regulator/Kconfig +@@ -1,5 +1,6 @@ + menuconfig REGULATOR + bool "Voltage and Current Regulator Support" ++ select REGMAP + help + Generic Voltage and Current Regulator support. + --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -139,7 +139,7 @@ struct reg_sequence { --- or alternatively, I could just add the CONFIG_REGMAP=y (This is possible thanks to the changes in 259-regmap_dynamic.patch) to the apm821xx/sata config: --- diff --git a/target/linux/apm821xx/sata/config-default b/target/linux/apm821xx/sata/config-default index 5af8c338f5..c267e7cddc 100644 --- a/target/linux/apm821xx/sata/config-default +++ b/target/linux/apm821xx/sata/config-default @@ -45,5 +45,6 @@ CONFIG_PPC_EARLY_DEBUG_44x=y # CONFIG_PPC_EARLY_DEBUG_MEMCONS is not set CONFIG_PPC_EARLY_DEBUG_44x_PHYSHIGH=0x4 CONFIG_PPC_EARLY_DEBUG_44x_PHYSLOW=0xef600300 +CONFIG_REGMAP=y CONFIG_REGULATOR=y CONFIG_REGULATOR_FIXED_VOLTAGE=y --- What is prefered? I like the first patch more, since it would avoid such a error in the future. But it does add to the pile of hacks. Regards, Christian
On 1 August 2018 at 22:44, Christian Lamparter <chunkeey@gmail.com> wrote: > On Wednesday, August 1, 2018 3:21:00 PM CEST Jonas Gorski wrote: >> On 30 July 2018 at 22:35, John Crispin <john@phrozen.org> wrote: >> > On 30/07/18 22:33, Christian Lamparter wrote: >> >> >> >> This patch fixes the a compile issue that was triggered by >> >> apm821xx/sata when kmod-regmap was selected. >> >> >> >> The CONFIG_REGMAP is declared in drivers/base/regmap/Kconfig >> >> as type "bool" and not "tristate". Hence the symbol should >> >> never be set to module, as this confuses the #if CONFIG_REGMAP >> >> guards in include/linux/regmap.h: >> >> >> >> |.../drivers/regulator/core.c:4041: undefined reference to >> >> `dev_get_regmap' >> >> |.../drivers/regulator/core.c:4042: undefined reference to >> >> `dev_get_regmap' >> >> |.../drivers/regulator/core.c:4044: undefined reference to >> >> `dev_get_regmap' >> >> |.../drivers/regulator/helpers.o: In function >> >> `regulator_is_enabled_regmap': >> >> |.../drivers/regulator/helpers.c:36: undefined reference to `regmap_read' >> >> |... >> > >> > i started a test build 2 minutes ago to figure this one out :-) thanks ! >> >> This is actually the wrong fix and papers over an issue in one of our >> local patches. >> >> We intentionally allow regmap to be built as a module, see >> >> https://github.com/openwrt/openwrt/blob/master/target/linux/generic/hack-4.14/259-regmap_dynamic.patch >> >> So there are likely some additional EXPORT_SYMBOL_GPL()s required instead. > > Thanks for FYI, I missed this patch completely. > > I found that the issue lies in the upstream regulator core code. > It is using these regmap's functions. And the CONFIG_REGULATOR symbol that pulls > in the core's code is sadly a bool menuconfig option... so CONFIG_REGMAP pretty > much becomes a requirement/select for the REGULATOR. This is one of the more trickier variants, it optionally supports regmap thanks to the stubs provided if regmap is disabled - which breaks if you compile regmap as a module. I expect this to happen more in the future. This is a bit tricky to solve. We can't just generally stub out the regmap stuff unless compiled in, as a lot of regulators depend on it. A proper fix is likely a bit more work (as usual). > > --- > diff --git a/target/linux/generic/hack-4.14/259-regmap_dynamic.patch b/target/linux/generic/hack-4.14/259-regmap_dynamic.patch > index 1c6e78df30..35803c3181 100644 > --- a/target/linux/generic/hack-4.14/259-regmap_dynamic.patch > +++ b/target/linux/generic/hack-4.14/259-regmap_dynamic.patch > @@ -9,8 +9,9 @@ Signed-off-by: Felix Fietkau <nbd@nbd.name> > drivers/base/regmap/Kconfig | 15 ++++++++++----- > drivers/base/regmap/Makefile | 12 ++++++++---- > drivers/base/regmap/regmap.c | 3 +++ > + drivers/regulator/Kconfig | 1 + > include/linux/regmap.h | 2 +- > - 4 files changed, 22 insertions(+), 10 deletions(-) > + 5 files changed, 23 insertions(+), 10 deletions(-) > > --- a/drivers/base/regmap/Kconfig > +++ b/drivers/base/regmap/Kconfig > @@ -96,6 +97,15 @@ Signed-off-by: Felix Fietkau <nbd@nbd.name> > postcore_initcall(regmap_initcall); > + > +MODULE_LICENSE("GPL"); > +--- a/drivers/regulator/Kconfig > ++++ b/drivers/regulator/Kconfig > +@@ -1,5 +1,6 @@ > + menuconfig REGULATOR > + bool "Voltage and Current Regulator Support" > ++ select REGMAP This will cause REGMAP to be included regardless if needed or not, but would be acceptable (at least to me) as an interim build fix (most targets using REGULATORs likely won't be part of the 4/32 gang anyway). The correct fix is likely to a) add this REGMAP select to all REGULATOR drivers depending on/using it (this can probably even be upstreamed) b) (OpenWrt only) Change the usage in regulator/core.c and regulator/helper.c to work like REGMAP is disabled if it is build as a module. b) is to allow building REGMAP as =m even when REGULATOR is enabled, if no REGMAP requiring REGULATOR driver is enabled. regards Jonas
On Thursday, August 2, 2018 11:43:50 AM CEST Jonas Gorski wrote: > On 1 August 2018 at 22:44, Christian Lamparter <chunkeey@gmail.com> wrote: >> On Wednesday, August 1, 2018 3:21:00 PM CEST Jonas Gorski wrote: >>> On 30/07/18 22:33, Christian Lamparter wrote: >>>> >>>> This patch fixes the a compile issue that was triggered by >>>> apm821xx/sata when kmod-regmap was selected. >>>> >>>> The CONFIG_REGMAP is declared in drivers/base/regmap/Kconfig >>>> as type "bool" and not "tristate". Hence the symbol should >>>> never be set to module, as this confuses the #if CONFIG_REGMAP >>>> guards in include/linux/regmap.h: >>> This is actually the wrong fix and papers over an issue in one of our >>> local patches. >>> [...] >>> >>> We intentionally allow regmap to be built as a module, see >>> >>> https://github.com/openwrt/openwrt/blob/master/target/linux/generic/hack-4.14/259-regmap_dynamic.patch >>> >>> So there are likely some additional EXPORT_SYMBOL_GPL()s required instead. >> Thanks for FYI, I missed this patch completely. >> >> I found that the issue lies in the upstream regulator core code. >> It is using these regmap's functions. And the CONFIG_REGULATOR symbol that pulls >> in the core's code is sadly a bool menuconfig option... so CONFIG_REGMAP pretty >> much becomes a requirement/select for the REGULATOR. > > This is one of the more trickier variants, it optionally supports > regmap thanks to the stubs provided if regmap is disabled - which > breaks if you compile regmap as a module. I expect this to happen > more in the future. From what I can tell, this did already happend in the past as well. https://github.com/openwrt/archive/commit/5e76bea8b4d45b87ea237bc19bcf8a2ac2ce9a42 Sadly, there's no explanation in the commit message. I guess it was a different time back then. Anyway, I can tell from my previous post, that the issue can fixed in the same way (i.e. adding CONFIG_REGMAP=y to the sata/config-default and a revert of the kmod-regmap patch). > This is a bit tricky to solve. We can't just generally stub out the > regmap stuff unless compiled in, as a lot of regulators depend on it. Right, it's becoming very tricky in OpenWrt patch context. In vanilla linux it's pretty straight forward, since it does not allow regmap to be a module. And I don't think upstream will be changing its stance on a regmap module. From their POV "in upstream everything works as intended" and regmap is a critical part of many subsystems that "you want anyway (tm)". Which is sort of true too, but more to this later. > A proper fix is likely a bit more work (as usual). This begs the question if a proper fix for a hack makes sense or not. It's definitely easier to follow kirkwood target and just add the CONFIG_REGMAP=y to the config of apm821xx/sata and revert the kmod-regmap patch. >> diff --git a/target/linux/generic/hack-4.14/259-regmap_dynamic.patch b/target/linux/generic/hack-4.14/259-regmap_dynamic.patch >> index 1c6e78df30..35803c3181 100644 >> --- a/target/linux/generic/hack-4.14/259-regmap_dynamic.patch >> +++ b/target/linux/generic/hack-4.14/259-regmap_dynamic.patch >> @@ -96,6 +97,15 @@ Signed-off-by: Felix Fietkau <nbd@nbd.name> >> postcore_initcall(regmap_initcall); >> + >> +MODULE_LICENSE("GPL"); >> +--- a/drivers/regulator/Kconfig >> ++++ b/drivers/regulator/Kconfig >> +@@ -1,5 +1,6 @@ >> + menuconfig REGULATOR >> + bool "Voltage and Current Regulator Support" >> ++ select REGMAP > > This will cause REGMAP to be included regardless if needed or not, but > would be acceptable (at least to me) as an interim build fix (most > targets using REGULATORs likely won't be part of the 4/32 gang > anyway). Yes exactly that is the idea. I did make that patch with ar71xx + tiny and ramips rt288x/rt305x in mind. The situation is a little bit different with ath79-tiny as its config-4.14 already sets: CONFIG_REGULATOR=y CONFIG_REGULATOR_FIXED_VOLTAGE=y And it inherits the CONFIG_REGMAP=y from the main ath79 config. If the REGMAP=y is only needed there because of the REGULATOR_FIXED_VOLTAGE (which does not need it), then it could make sense to extend the hack for this special case. In order to keep the kernel image as small as possible. (see below) > The correct fix is likely to > > a) add this REGMAP select to all REGULATOR drivers depending on/using > it (this can probably even be upstreamed) From looking at the upstream Kconfigs, I think this is pretty much already the case. The drivers for the i2c jellybean regulator parts select REGMAP_I2C. The remaining regulators drivers are usually part of a SoC and have a indirect dependency/select to another symbol that selects REGMAP (these are mostly syscon/mfd symbols and CONFIG_SYSCON pulls in REGMAP_MMIO). So I guess this will only become a problem is regulator kmods are added. > b) (OpenWrt only) Change the usage in regulator/core.c and > regulator/helper.c to work like REGMAP is disabled if it is > build as a module. --- diff --git a/package/kernel/linux/modules/other.mk b/package/kernel/linux/modules/other.mk index dd037cf86c..7e18a21db3 100644 --- a/package/kernel/linux/modules/other.mk +++ b/package/kernel/linux/modules/other.mk @@ -718,7 +718,7 @@ define KernelPackage/regmap SUBMENU:=$(OTHER_MENU) TITLE:=Generic register map support DEPENDS:=+kmod-lib-lzo +kmod-i2c-core - KCONFIG:=CONFIG_REGMAP=y \ + KCONFIG:=CONFIG_REGMAP \ CONFIG_REGMAP_MMIO \ CONFIG_REGMAP_SPI \ CONFIG_REGMAP_I2C \ diff --git a/target/linux/generic/hack-4.14/259-regmap_dynamic.patch b/target/linux/generic/hack-4.14/259-regmap_dynamic.patch index 1c6e78df30..458b7c35a1 100644 --- a/target/linux/generic/hack-4.14/259-regmap_dynamic.patch +++ b/target/linux/generic/hack-4.14/259-regmap_dynamic.patch @@ -103,7 +103,7 @@ Signed-off-by: Felix Fietkau <nbd@nbd.name> }) -#ifdef CONFIG_REGMAP -+#if IS_ENABLED(CONFIG_REGMAP) ++#if IS_REACHABLE(CONFIG_REGMAP) enum regmap_endian { /* Unspecified -> 0 -> Backwards compatible default */ --- I think this should do just that. To quote IS_REACHABLE help text:" IS_REACHABLE(CONFIG_FOO) evaluates to 1 if the currently compiled code can call a function defined in code compiled based on CONFIG_FOO. This is similar to IS_ENABLED(), but returns false when invoked from built-in code when CONFIG_FOO is set to 'm'." and it should combine the best of both worlds. (And maybe make it possible to disable the REGMAP=y from ath79-tiny. Any testers?) > b) is to allow building REGMAP as =m even when REGULATOR is enabled, > if no REGMAP requiring REGULATOR driver is enabled. I was more thinking about detangling just the REGULATOR_FIXED_VOLTAGE driver from the REGULATOR framework. So it can be built without the regulator core and regmap. I did play around with #ifdefing but nah, this turns out to become a much bigger fragile hack, that nobody is going to maintain. Regards, Christian Regards, Christian
On 3 August 2018 at 18:00, Christian Lamparter <chunkeey@gmail.com> wrote: > diff --git a/package/kernel/linux/modules/other.mk b/package/kernel/linux/modules/other.mk > index dd037cf86c..7e18a21db3 100644 > --- a/package/kernel/linux/modules/other.mk > +++ b/package/kernel/linux/modules/other.mk > @@ -718,7 +718,7 @@ define KernelPackage/regmap > SUBMENU:=$(OTHER_MENU) > TITLE:=Generic register map support > DEPENDS:=+kmod-lib-lzo +kmod-i2c-core > - KCONFIG:=CONFIG_REGMAP=y \ > + KCONFIG:=CONFIG_REGMAP \ > CONFIG_REGMAP_MMIO \ > CONFIG_REGMAP_SPI \ > CONFIG_REGMAP_I2C \ > diff --git a/target/linux/generic/hack-4.14/259-regmap_dynamic.patch b/target/linux/generic/hack-4.14/259-regmap_dynamic.patch > index 1c6e78df30..458b7c35a1 100644 > --- a/target/linux/generic/hack-4.14/259-regmap_dynamic.patch > +++ b/target/linux/generic/hack-4.14/259-regmap_dynamic.patch > @@ -103,7 +103,7 @@ Signed-off-by: Felix Fietkau <nbd@nbd.name> > }) > > -#ifdef CONFIG_REGMAP > -+#if IS_ENABLED(CONFIG_REGMAP) > ++#if IS_REACHABLE(CONFIG_REGMAP) > > enum regmap_endian { > /* Unspecified -> 0 -> Backwards compatible default */ > --- > > I think this should do just that. To quote IS_REACHABLE help text:" > IS_REACHABLE(CONFIG_FOO) evaluates to 1 if the currently compiled > code can call a function defined in code compiled based on CONFIG_FOO. > This is similar to IS_ENABLED(), but returns false when invoked from > built-in code when CONFIG_FOO is set to 'm'." > > and it should combine the best of both worlds. (And maybe make it > possible to disable the REGMAP=y from ath79-tiny. Any testers?) Ooooooh, I didn't know that one exists! Nice find! Yes, that should fix it in a clean way without introducing more hacks. I really like this one! Regards Jonas
diff --git a/package/kernel/linux/modules/other.mk b/package/kernel/linux/modules/other.mk index 7e18a21db3..dd037cf86c 100644 --- a/package/kernel/linux/modules/other.mk +++ b/package/kernel/linux/modules/other.mk @@ -718,7 +718,7 @@ define KernelPackage/regmap SUBMENU:=$(OTHER_MENU) TITLE:=Generic register map support DEPENDS:=+kmod-lib-lzo +kmod-i2c-core - KCONFIG:=CONFIG_REGMAP \ + KCONFIG:=CONFIG_REGMAP=y \ CONFIG_REGMAP_MMIO \ CONFIG_REGMAP_SPI \ CONFIG_REGMAP_I2C \
This patch fixes the a compile issue that was triggered by apm821xx/sata when kmod-regmap was selected. The CONFIG_REGMAP is declared in drivers/base/regmap/Kconfig as type "bool" and not "tristate". Hence the symbol should never be set to module, as this confuses the #if CONFIG_REGMAP guards in include/linux/regmap.h: |.../drivers/regulator/core.c:4041: undefined reference to `dev_get_regmap' |.../drivers/regulator/core.c:4042: undefined reference to `dev_get_regmap' |.../drivers/regulator/core.c:4044: undefined reference to `dev_get_regmap' |.../drivers/regulator/helpers.o: In function `regulator_is_enabled_regmap': |.../drivers/regulator/helpers.c:36: undefined reference to `regmap_read' |... Signed-off-by: Christian Lamparter <chunkeey@gmail.com> --- package/kernel/linux/modules/other.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)