In a randconfig with CONFIG_SERIAL_CPM=m and
CONFIG_PPC_EARLY_DEBUG_CPM=y, there is a build error:
ERROR: modpost: "udbg_putc" [drivers/tty/serial/cpm_uart/cpm_uart.ko] undefined!
The build can be fixed by exporting "udbg_putc" in udbg.c.
OTOH, maybe something like this is more appropriate in
arch/powerpc/Kconfig.debug?
config PPC_EARLY_DEBUG_CPM
bool "Early serial debugging for Freescale CPM-based serial ports"
- depends on SERIAL_CPM
+ depends on SERIAL_CPM=y
Signed-off-by: Randy Dunlap <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Nicholas Piggin <[email protected]>
Cc: Christophe Leroy <[email protected]>
Cc: [email protected]
Cc: Arnd Bergmann <[email protected]>
Cc: "Pali Rohár" <[email protected]>
---
arch/powerpc/kernel/udbg.c | 1 +
1 file changed, 1 insertion(+)
diff -- a/arch/powerpc/kernel/udbg.c b/arch/powerpc/kernel/udbg.c
--- a/arch/powerpc/kernel/udbg.c
+++ b/arch/powerpc/kernel/udbg.c
@@ -14,6 +14,7 @@
#include <asm/udbg.h>
void (*udbg_putc)(char c);
+EXPORT_SYMBOL(udbg_putc);
void (*udbg_flush)(void);
int (*udbg_getc)(void);
int (*udbg_getc_poll)(void);
On Friday 12 May 2023 22:58:04 Randy Dunlap wrote:
> In a randconfig with CONFIG_SERIAL_CPM=m and
> CONFIG_PPC_EARLY_DEBUG_CPM=y, there is a build error:
> ERROR: modpost: "udbg_putc" [drivers/tty/serial/cpm_uart/cpm_uart.ko] undefined!
>
> The build can be fixed by exporting "udbg_putc" in udbg.c.
>
> OTOH, maybe something like this is more appropriate in
> arch/powerpc/Kconfig.debug?
>
> config PPC_EARLY_DEBUG_CPM
> bool "Early serial debugging for Freescale CPM-based serial ports"
> - depends on SERIAL_CPM
> + depends on SERIAL_CPM=y
>
> Signed-off-by: Randy Dunlap <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: Nicholas Piggin <[email protected]>
> Cc: Christophe Leroy <[email protected]>
> Cc: [email protected]
> Cc: Arnd Bergmann <[email protected]>
> Cc: "Pali Rohár" <[email protected]>
> ---
> arch/powerpc/kernel/udbg.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff -- a/arch/powerpc/kernel/udbg.c b/arch/powerpc/kernel/udbg.c
> --- a/arch/powerpc/kernel/udbg.c
> +++ b/arch/powerpc/kernel/udbg.c
> @@ -14,6 +14,7 @@
> #include <asm/udbg.h>
>
> void (*udbg_putc)(char c);
> +EXPORT_SYMBOL(udbg_putc);
> void (*udbg_flush)(void);
> int (*udbg_getc)(void);
> int (*udbg_getc_poll)(void);
Hello! I do not think that it is a good idea to export udbg functions
for kernel modules. I have quickly looked at the cpm_uart driver file
drivers/tty/serial/cpm_uart/cpm_uart_core.c and it looks like that once
udbg from it is registered then there is no code for unregistering it.
So I have feeling that compiling cpm_uart driver as module should not be
allowed when CONFIG_SERIAL_CPM_CONSOLE is enabled, and early debug
should depend on CONFIG_CONSOLE_POLL || CONFIG_SERIAL_CPM_CONSOLE.
Any other opinion?
On 5/13/23 16:05, Pali Rohár wrote:
> On Saturday 13 May 2023 15:53:19 Randy Dunlap wrote:
>> Hi Pali,
>>
>> On 5/13/23 02:13, Pali Rohár wrote:
>>> On Friday 12 May 2023 22:58:04 Randy Dunlap wrote:
>>>> In a randconfig with CONFIG_SERIAL_CPM=m and
>>>> CONFIG_PPC_EARLY_DEBUG_CPM=y, there is a build error:
>>>> ERROR: modpost: "udbg_putc" [drivers/tty/serial/cpm_uart/cpm_uart.ko] undefined!
>>>>
>>>> The build can be fixed by exporting "udbg_putc" in udbg.c.
>>>>
>>>> OTOH, maybe something like this is more appropriate in
>>>> arch/powerpc/Kconfig.debug?
>>>>
>>>> config PPC_EARLY_DEBUG_CPM
>>>> bool "Early serial debugging for Freescale CPM-based serial ports"
>>>> - depends on SERIAL_CPM
>>>> + depends on SERIAL_CPM=y
>>>>
>>>> Signed-off-by: Randy Dunlap <[email protected]>
>>>> Cc: Michael Ellerman <[email protected]>
>>>> Cc: Nicholas Piggin <[email protected]>
>>>> Cc: Christophe Leroy <[email protected]>
>>>> Cc: [email protected]
>>>> Cc: Arnd Bergmann <[email protected]>
>>>> Cc: "Pali Rohár" <[email protected]>
>>>> ---
>>>> arch/powerpc/kernel/udbg.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff -- a/arch/powerpc/kernel/udbg.c b/arch/powerpc/kernel/udbg.c
>>>> --- a/arch/powerpc/kernel/udbg.c
>>>> +++ b/arch/powerpc/kernel/udbg.c
>>>> @@ -14,6 +14,7 @@
>>>> #include <asm/udbg.h>
>>>>
>>>> void (*udbg_putc)(char c);
>>>> +EXPORT_SYMBOL(udbg_putc);
>>>> void (*udbg_flush)(void);
>>>> int (*udbg_getc)(void);
>>>> int (*udbg_getc_poll)(void);
>>>
>>> Hello! I do not think that it is a good idea to export udbg functions
>>> for kernel modules. I have quickly looked at the cpm_uart driver file
>>
>> Sure, that was just a conversation starter.
>>
>>> drivers/tty/serial/cpm_uart/cpm_uart_core.c and it looks like that once
>>> udbg from it is registered then there is no code for unregistering it.
>>> So I have feeling that compiling cpm_uart driver as module should not be
>>> allowed when CONFIG_SERIAL_CPM_CONSOLE is enabled, and early debug
>>> should depend on CONFIG_CONSOLE_POLL || CONFIG_SERIAL_CPM_CONSOLE.
>>
>> The other change above also fixes the build error: (Option 2)
>>
>>>> config PPC_EARLY_DEBUG_CPM
>>>> bool "Early serial debugging for Freescale CPM-based serial ports"
>>>> - depends on SERIAL_CPM
>>>> + depends on SERIAL_CPM=y
>>
>>
>> Also, making SERIAL_CPM (cpm_uart driver) be restricted to not allow it
>> as a module when SERIAL_CPM_CONSOLE is enabled [how does one express that
>> in Kconfig language?] will cause a circular dependency since SERIAL_CPM_CONSOLE
>> depends on SERIAL_CPM=y. It looks like you are suggesting reversing the
>> dependencies.
>
> I see that SERIAL_CPM_CONSOLE has already "depends on SERIAL_CPM=y".
> Am I right that this already disallow compiling cpm_uart driver as
> module when SERIAL_CPM_CONSOLE is enabled?
No, it just prevents SERIAL_CPM_CONSOLE from being set when
SERIAL_CPM=m.
>>> Any other opinion?
>>
>> Yes, please. Otherwise I prefer my option 2.
>>
>> --
>> ~Randy
>
> Option 2 looks like a better solution.
Thanks.
--
~Randy
Hi Pali,
On 5/13/23 02:13, Pali Rohár wrote:
> On Friday 12 May 2023 22:58:04 Randy Dunlap wrote:
>> In a randconfig with CONFIG_SERIAL_CPM=m and
>> CONFIG_PPC_EARLY_DEBUG_CPM=y, there is a build error:
>> ERROR: modpost: "udbg_putc" [drivers/tty/serial/cpm_uart/cpm_uart.ko] undefined!
>>
>> The build can be fixed by exporting "udbg_putc" in udbg.c.
>>
>> OTOH, maybe something like this is more appropriate in
>> arch/powerpc/Kconfig.debug?
>>
>> config PPC_EARLY_DEBUG_CPM
>> bool "Early serial debugging for Freescale CPM-based serial ports"
>> - depends on SERIAL_CPM
>> + depends on SERIAL_CPM=y
>>
>> Signed-off-by: Randy Dunlap <[email protected]>
>> Cc: Michael Ellerman <[email protected]>
>> Cc: Nicholas Piggin <[email protected]>
>> Cc: Christophe Leroy <[email protected]>
>> Cc: [email protected]
>> Cc: Arnd Bergmann <[email protected]>
>> Cc: "Pali Rohár" <[email protected]>
>> ---
>> arch/powerpc/kernel/udbg.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff -- a/arch/powerpc/kernel/udbg.c b/arch/powerpc/kernel/udbg.c
>> --- a/arch/powerpc/kernel/udbg.c
>> +++ b/arch/powerpc/kernel/udbg.c
>> @@ -14,6 +14,7 @@
>> #include <asm/udbg.h>
>>
>> void (*udbg_putc)(char c);
>> +EXPORT_SYMBOL(udbg_putc);
>> void (*udbg_flush)(void);
>> int (*udbg_getc)(void);
>> int (*udbg_getc_poll)(void);
>
> Hello! I do not think that it is a good idea to export udbg functions
> for kernel modules. I have quickly looked at the cpm_uart driver file
Sure, that was just a conversation starter.
> drivers/tty/serial/cpm_uart/cpm_uart_core.c and it looks like that once
> udbg from it is registered then there is no code for unregistering it.
> So I have feeling that compiling cpm_uart driver as module should not be
> allowed when CONFIG_SERIAL_CPM_CONSOLE is enabled, and early debug
> should depend on CONFIG_CONSOLE_POLL || CONFIG_SERIAL_CPM_CONSOLE.
The other change above also fixes the build error: (Option 2)
>> config PPC_EARLY_DEBUG_CPM
>> bool "Early serial debugging for Freescale CPM-based serial ports"
>> - depends on SERIAL_CPM
>> + depends on SERIAL_CPM=y
Also, making SERIAL_CPM (cpm_uart driver) be restricted to not allow it
as a module when SERIAL_CPM_CONSOLE is enabled [how does one express that
in Kconfig language?] will cause a circular dependency since SERIAL_CPM_CONSOLE
depends on SERIAL_CPM=y. It looks like you are suggesting reversing the
dependencies.
> Any other opinion?
Yes, please. Otherwise I prefer my option 2.
--
~Randy
On Saturday 13 May 2023 15:53:19 Randy Dunlap wrote:
> Hi Pali,
>
> On 5/13/23 02:13, Pali Rohár wrote:
> > On Friday 12 May 2023 22:58:04 Randy Dunlap wrote:
> >> In a randconfig with CONFIG_SERIAL_CPM=m and
> >> CONFIG_PPC_EARLY_DEBUG_CPM=y, there is a build error:
> >> ERROR: modpost: "udbg_putc" [drivers/tty/serial/cpm_uart/cpm_uart.ko] undefined!
> >>
> >> The build can be fixed by exporting "udbg_putc" in udbg.c.
> >>
> >> OTOH, maybe something like this is more appropriate in
> >> arch/powerpc/Kconfig.debug?
> >>
> >> config PPC_EARLY_DEBUG_CPM
> >> bool "Early serial debugging for Freescale CPM-based serial ports"
> >> - depends on SERIAL_CPM
> >> + depends on SERIAL_CPM=y
> >>
> >> Signed-off-by: Randy Dunlap <[email protected]>
> >> Cc: Michael Ellerman <[email protected]>
> >> Cc: Nicholas Piggin <[email protected]>
> >> Cc: Christophe Leroy <[email protected]>
> >> Cc: [email protected]
> >> Cc: Arnd Bergmann <[email protected]>
> >> Cc: "Pali Rohár" <[email protected]>
> >> ---
> >> arch/powerpc/kernel/udbg.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff -- a/arch/powerpc/kernel/udbg.c b/arch/powerpc/kernel/udbg.c
> >> --- a/arch/powerpc/kernel/udbg.c
> >> +++ b/arch/powerpc/kernel/udbg.c
> >> @@ -14,6 +14,7 @@
> >> #include <asm/udbg.h>
> >>
> >> void (*udbg_putc)(char c);
> >> +EXPORT_SYMBOL(udbg_putc);
> >> void (*udbg_flush)(void);
> >> int (*udbg_getc)(void);
> >> int (*udbg_getc_poll)(void);
> >
> > Hello! I do not think that it is a good idea to export udbg functions
> > for kernel modules. I have quickly looked at the cpm_uart driver file
>
> Sure, that was just a conversation starter.
>
> > drivers/tty/serial/cpm_uart/cpm_uart_core.c and it looks like that once
> > udbg from it is registered then there is no code for unregistering it.
> > So I have feeling that compiling cpm_uart driver as module should not be
> > allowed when CONFIG_SERIAL_CPM_CONSOLE is enabled, and early debug
> > should depend on CONFIG_CONSOLE_POLL || CONFIG_SERIAL_CPM_CONSOLE.
>
> The other change above also fixes the build error: (Option 2)
>
> >> config PPC_EARLY_DEBUG_CPM
> >> bool "Early serial debugging for Freescale CPM-based serial ports"
> >> - depends on SERIAL_CPM
> >> + depends on SERIAL_CPM=y
>
>
> Also, making SERIAL_CPM (cpm_uart driver) be restricted to not allow it
> as a module when SERIAL_CPM_CONSOLE is enabled [how does one express that
> in Kconfig language?] will cause a circular dependency since SERIAL_CPM_CONSOLE
> depends on SERIAL_CPM=y. It looks like you are suggesting reversing the
> dependencies.
I see that SERIAL_CPM_CONSOLE has already "depends on SERIAL_CPM=y".
Am I right that this already disallow compiling cpm_uart driver as
module when SERIAL_CPM_CONSOLE is enabled?
> > Any other opinion?
>
> Yes, please. Otherwise I prefer my option 2.
>
> --
> ~Randy
Option 2 looks like a better solution.