Re: pm.c:undefined reference to `i8042_command'

Re: pm.c:undefined reference to `i8042_command'

[Randy Dunlap]

Hi,


On 12/4/23 04:02, kernel test robot wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head:   33cc938e65a98f1d29d0a18403dbbee050dcad9a
> commit: 7b4537199a4a8480b8c3ba37a2d44765ce76cd9b kbuild: link symbol CRCs at final link, removing CONFIG_MODULE_REL_CRCS
> date:   1 year, 6 months ago
> config: mips-randconfig-r001-20211012 (https://download.01.org/0day-ci/archive/20231204/202312041909.lwhcU35R-lkp@intel.com/config)
> compiler: mips64el-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231204/202312041909.lwhcU35R-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202312041909.lwhcU35R-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>    mips64el-linux-ld: arch/mips/loongson2ef/lemote-2f/pm.o: in function `setup_wakeup_events':
>>> pm.c:(.text+0x118): undefined reference to `i8042_command'
>>> mips64el-linux-ld: pm.c:(.text+0x154): undefined reference to `i8042_command'


How do we feel about this?
I suppose that an ARCH or mach or board should know what it requires.


---
 arch/mips/loongson2ef/Kconfig |    3 +++
 1 file changed, 3 insertions(+)

diff -- a/arch/mips/loongson2ef/Kconfig b/arch/mips/loongson2ef/Kconfig
--- a/arch/mips/loongson2ef/Kconfig
+++ b/arch/mips/loongson2ef/Kconfig
@@ -40,6 +40,9 @@ config LEMOTE_MACH2F
 	select ARCH_HAS_PHYS_TO_DMA
 	select ARCH_MIGHT_HAVE_PC_PARPORT
 	select ARCH_MIGHT_HAVE_PC_SERIO
+	select INPUT
+	select SERIO
+	select SERIO_I8042
 	select BOARD_SCACHE
 	select BOOT_ELF32
 	select CEVT_R4K if ! MIPS_EXTERNAL_TIMER


-- 
~Randy

Re: pm.c:undefined reference to `i8042_command'

[Arnd Bergmann]

On Wed, Dec 6, 2023, at 06:24, Randy Dunlap wrote:
>> All errors (new ones prefixed by >>):
>> 
>>    mips64el-linux-ld: arch/mips/loongson2ef/lemote-2f/pm.o: in function `setup_wakeup_events':
>>>> pm.c:(.text+0x118): undefined reference to `i8042_command'
>>>> mips64el-linux-ld: pm.c:(.text+0x154): undefined reference to `i8042_command'
>
>
> How do we feel about this?
> I suppose that an ARCH or mach or board should know what it requires.
>
>
> ---
>  arch/mips/loongson2ef/Kconfig |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff -- a/arch/mips/loongson2ef/Kconfig b/arch/mips/loongson2ef/Kconfig
> --- a/arch/mips/loongson2ef/Kconfig
> +++ b/arch/mips/loongson2ef/Kconfig
> @@ -40,6 +40,9 @@ config LEMOTE_MACH2F
>  	select ARCH_HAS_PHYS_TO_DMA
>  	select ARCH_MIGHT_HAVE_PC_PARPORT
>  	select ARCH_MIGHT_HAVE_PC_SERIO
> +	select INPUT
> +	select SERIO
> +	select SERIO_I8042
>  	select BOARD_SCACHE
>  	select BOOT_ELF32

I think it's bad style to force-select an optional subsystem.
How about making the entire file optional? It seems that there
are already __weak functions in its place.

--- a/arch/mips/loongson2ef/lemote-2f/Makefile
+++ b/arch/mips/loongson2ef/lemote-2f/Makefile
@@ -8,5 +8,6 @@ obj-y += clock.o machtype.o irq.o reset.o dma.o ec_kb3310b.o
 #
 # Suspend Support
 #
-
+ifdef CONFIG_SERIO_I8042
 obj-$(CONFIG_SUSPEND) += pm.o
+endif

     Arnd

Re: pm.c:undefined reference to `i8042_command'

[Randy Dunlap]

On 12/5/23 22:52, Arnd Bergmann wrote:
> On Wed, Dec 6, 2023, at 06:24, Randy Dunlap wrote:
>>> All errors (new ones prefixed by >>):
>>>
>>>    mips64el-linux-ld: arch/mips/loongson2ef/lemote-2f/pm.o: in function `setup_wakeup_events':
>>>>> pm.c:(.text+0x118): undefined reference to `i8042_command'
>>>>> mips64el-linux-ld: pm.c:(.text+0x154): undefined reference to `i8042_command'
>>
>>
>> How do we feel about this?
>> I suppose that an ARCH or mach or board should know what it requires.
>>
>>
>> ---
>>  arch/mips/loongson2ef/Kconfig |    3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff -- a/arch/mips/loongson2ef/Kconfig b/arch/mips/loongson2ef/Kconfig
>> --- a/arch/mips/loongson2ef/Kconfig
>> +++ b/arch/mips/loongson2ef/Kconfig
>> @@ -40,6 +40,9 @@ config LEMOTE_MACH2F
>>  	select ARCH_HAS_PHYS_TO_DMA
>>  	select ARCH_MIGHT_HAVE_PC_PARPORT
>>  	select ARCH_MIGHT_HAVE_PC_SERIO
>> +	select INPUT
>> +	select SERIO
>> +	select SERIO_I8042
>>  	select BOARD_SCACHE
>>  	select BOOT_ELF32
> 
> I think it's bad style to force-select an optional subsystem.
> How about making the entire file optional? It seems that there
> are already __weak functions in its place.

Yes, I agree in general.

Hopefully the maintainer will opine your suggestion.
Jiaxun?

Thanks.

> 
> --- a/arch/mips/loongson2ef/lemote-2f/Makefile
> +++ b/arch/mips/loongson2ef/lemote-2f/Makefile
> @@ -8,5 +8,6 @@ obj-y += clock.o machtype.o irq.o reset.o dma.o ec_kb3310b.o
>  #
>  # Suspend Support
>  #
> -
> +ifdef CONFIG_SERIO_I8042
>  obj-$(CONFIG_SUSPEND) += pm.o
> +endif
> 
>      Arnd

-- 
~Randy

Re: pm.c:undefined reference to `i8042_command'

[Jiaxun Yang]

在2023年12月6日十二月 下午5:26，Randy Dunlap写道：
> On 12/5/23 22:52, Arnd Bergmann wrote:
>> On Wed, Dec 6, 2023, at 06:24, Randy Dunlap wrote:
>>>> All errors (new ones prefixed by >>):
>>>>
>>>>    mips64el-linux-ld: arch/mips/loongson2ef/lemote-2f/pm.o: in function `setup_wakeup_events':
>>>>>> pm.c:(.text+0x118): undefined reference to `i8042_command'
>>>>>> mips64el-linux-ld: pm.c:(.text+0x154): undefined reference to `i8042_command'
>>>
>>>
>>> How do we feel about this?
>>> I suppose that an ARCH or mach or board should know what it requires.
>>>
>>>
>>> ---
>>>  arch/mips/loongson2ef/Kconfig |    3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff -- a/arch/mips/loongson2ef/Kconfig b/arch/mips/loongson2ef/Kconfig
>>> --- a/arch/mips/loongson2ef/Kconfig
>>> +++ b/arch/mips/loongson2ef/Kconfig
>>> @@ -40,6 +40,9 @@ config LEMOTE_MACH2F
>>>  	select ARCH_HAS_PHYS_TO_DMA
>>>  	select ARCH_MIGHT_HAVE_PC_PARPORT
>>>  	select ARCH_MIGHT_HAVE_PC_SERIO
>>> +	select INPUT
>>> +	select SERIO
>>> +	select SERIO_I8042
>>>  	select BOARD_SCACHE
>>>  	select BOOT_ELF32
>> 
>> I think it's bad style to force-select an optional subsystem.
>> How about making the entire file optional? It seems that there
>> are already __weak functions in its place.
>
> Yes, I agree in general.
>
> Hopefully the maintainer will opine your suggestion.
> Jiaxun?

LGTM, PM is not an essential function here.
Arnd, do you mind making it a patch?

Thanks
- Jiaxun

>
> Thanks.
>
>> 
>> --- a/arch/mips/loongson2ef/lemote-2f/Makefile
>> +++ b/arch/mips/loongson2ef/lemote-2f/Makefile
>> @@ -8,5 +8,6 @@ obj-y += clock.o machtype.o irq.o reset.o dma.o ec_kb3310b.o
>>  #
>>  # Suspend Support
>>  #
>> -
>> +ifdef CONFIG_SERIO_I8042
>>  obj-$(CONFIG_SUSPEND) += pm.o
>> +endif
>> 
>>      Arnd
>
> -- 
> ~Randy

-- 
- Jiaxun

Re: pm.c:undefined reference to `i8042_command'

[Arnd Bergmann]

On Thu, Dec 7, 2023, at 01:21, Jiaxun Yang wrote:
> 在2023年12月6日十二月 下午5:26，Randy Dunlap写道：
>> On 12/5/23 22:52, Arnd Bergmann wrote:
>>> On Wed, Dec 6, 2023, at 06:24, Randy Dunlap wrote:

>>> I think it's bad style to force-select an optional subsystem.
>>> How about making the entire file optional? It seems that there
>>> are already __weak functions in its place.
>>
>> Yes, I agree in general.
>>
>> Hopefully the maintainer will opine your suggestion.
>> Jiaxun?
>
> LGTM, PM is not an essential function here.
> Arnd, do you mind making it a patch?
>

I just tried it out and my version doesn't actually work since
we still end up building the file with SERIO_I82042=m.

We could still make it work by checking for serio being
built-in here, but in the end I think that's worse than
Randy's patch, so let's just go with his original version.

      Arnd

Re: pm.c:undefined reference to `i8042_command'

[Randy Dunlap]

On 12/14/23 11:54, Arnd Bergmann wrote:
> On Thu, Dec 7, 2023, at 01:21, Jiaxun Yang wrote:
>> 在2023年12月6日十二月 下午5:26，Randy Dunlap写道：
>>> On 12/5/23 22:52, Arnd Bergmann wrote:
>>>> On Wed, Dec 6, 2023, at 06:24, Randy Dunlap wrote:
> 
>>>> I think it's bad style to force-select an optional subsystem.
>>>> How about making the entire file optional? It seems that there
>>>> are already __weak functions in its place.
>>>
>>> Yes, I agree in general.
>>>
>>> Hopefully the maintainer will opine your suggestion.
>>> Jiaxun?
>>
>> LGTM, PM is not an essential function here.
>> Arnd, do you mind making it a patch?
>>
> 
> I just tried it out and my version doesn't actually work since
> we still end up building the file with SERIO_I82042=m.
> 
> We could still make it work by checking for serio being
> built-in here, but in the end I think that's worse than
> Randy's patch, so let's just go with his original version.
> 
>       Arnd

I think that we should look for yet another alternative then.
I don't believe that adding "select INPUT" is a good idea.
It would be the only "select INPUT" in the entire kernel tree.


thanks.
-- 
#Randy
https://people.kernel.org/tglx/notes-about-netiquette
https://subspace.kernel.org/etiquette.html