Re: [tty:tty-testing 19/19] legacy_serial.c:undefined reference to `fsl8250_handle_irq'

Hello,

On Fri, Jun 02, 2023 at 10:27:52AM +0800, kernel test robot wrote:

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
head:   3a3d09a9ee0ef5b417d6bdf8486a4da2bef06dc3
commit: 3a3d09a9ee0ef5b417d6bdf8486a4da2bef06dc3 [19/19] serial: 8250: Apply FSL workarounds also without SERIAL_8250_CONSOLE
config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20230602/202306021041.qbRZZenE-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?id=3a3d09a9ee0ef5b417d6bdf8486a4da2bef06dc3
        git remote add tty https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git
        git fetch --no-tags tty tty-testing
        git checkout 3a3d09a9ee0ef5b417d6bdf8486a4da2bef06dc3

For the new readers, this is about

	https://lore.kernel.org/r/20230531083230.2702181-1-u.kleine-koenig@pengutronix.de (local)

Greg already dropped it from his tty-testing tree. Thanks and sorry for
this second breakage. :-\

        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=powerpc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot [off-list ref]
| Closes: https://lore.kernel.org/oe-kbuild-all/202306021041.qbRZZenE-lkp@intel.com/ (local)

All errors (new ones prefixed by >>):

   powerpc-linux-ld: arch/powerpc/kernel/legacy_serial.o: in function `serial_dev_init':

quoted

quoted

legacy_serial.c:(.init.text+0x46a): undefined reference to `fsl8250_handle_irq'
powerpc-linux-ld: legacy_serial.c:(.init.text+0x472): undefined reference to `fsl8250_handle_irq'

Urgs, this is ugly. Arch code uses a function from the 8250 driver
introduced in commit 9deaa53ac7fa ("serial: add irq handler for
Freescale 16550 errata.").

So the problematic case is SERIAL_8250=m which results in
fsl8250_handle_irq being in a module.

diff --git a/arch/powerpc/kernel/legacy_serial.c b/arch/powerpc/kernel/legacy_serial.c
index c9ad12461d44..ad9f15902abb 100644
--- a/arch/powerpc/kernel/legacy_serial.c
+++ b/arch/powerpc/kernel/legacy_serial.c

@@ -510,8 +510,12 @@ static void __init fixup_port_irq(int index,
 
 #ifdef CONFIG_SERIAL_8250_FSL
 	if (of_device_is_compatible(np, "fsl,ns16550")) {
-		port->handle_irq = fsl8250_handle_irq;
-		port->has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE);
+		if (IS_REACHABLE(CONFIG_SERIAL_8250)) {
+			port->handle_irq = fsl8250_handle_irq;
+			port->has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE);
+		} else {
+			pr_warn("Not activating fsl workarounds for 8250 port %d\n", index);
+		}
 	}
 #endif
 }

would work. The warning would trigger in cases where before the port
just silently used the default irq handler and so the FSL bug isn't
workarounded[1]. If the warning isn't wanted, it could be simplified to:

#if IS_REACHABLE(CONFIG_SERIAL_8250)
	if (of_device_is_compatible(np, "fsl,ns16550")) {
		...
	}
#endif

But I wonder if in the presence of

        if (IS_ENABLED(CONFIG_SERIAL_8250_FSL) &&
            (of_device_is_compatible(np, "fsl,ns16550") ||
             of_device_is_compatible(np, "fsl,16550-FIFO64"))) {
                port->handle_irq = fsl8250_handle_irq;
                port->has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE);
        }

in of_platform_serial_setup() the code in
arch/powerpc/kernel/legacy_serial.c can just be dropped instead?!

Best regards
Uwe

[1] Of course this won't happen because the help text of SERIAL_8250
clearly indicates that =m isn't a safe choice in the presence of
"non-standard serial ports". So the issue is purly theoretic.

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |