Feedback-ID: i36794607:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-206-g57c8fdedf8-fm-20230227.001-g57c8fded
Mime-Version: 1.0
Message-Id: <c85f0735-6c85-4a6e-ad07-66493783c58e@app.fastmail.com>
In-Reply-To: <2e79ebb0ef62e550231c3daf46c62cd60d78acbc.camel@linux.ibm.com>
References: <202301051008.a8468257-yujie.liu@intel.com>
 <e211f932-77c3-427a-859a-d846598524ed@app.fastmail.com>
 <2e79ebb0ef62e550231c3daf46c62cd60d78acbc.camel@linux.ibm.com>
Date:   Wed, 08 Mar 2023 13:21:48 +0100
From:   "Arnd Bergmann" <arnd@kernel.org>
To:     "Niklas Schnelle" <schnelle@linux.ibm.com>,
        "kernel test robot" <yujie.liu@intel.com>
Cc:     oe-lkp@lists.linux.dev, "kernel test robot" <lkp@intel.com>,
        linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
        "Heiko Carstens" <hca@linux.ibm.com>
Subject: Re: [niks:has_ioport_v3] [tty] aa0652d7f1:
 BUG:kernel_NULL_pointer_dereference,address
Content-Type: text/plain
Precedence: bulk

On Wed, Mar 8, 2023, at 12:24, Niklas Schnelle wrote:
> On Thu, 2023-01-05 at 09:03 +0100, Arnd Bergmann wrote:
>
> Yes that makes sense, it's clearly not correct to put the default case
> inside CONFIG_SERIAL_8250_RT288X. What do you think about going with
> something like:
>
> @@ -519,9 +534,14 @@ static void set_io_from_upio(struct uart_port *p)
>  #endif
>
>         default:
> +#ifdef CONFIG_HAS_IOPORT
>                 p->serial_in = io_serial_in;
>                 p->serial_out = io_serial_out;
>                 break;
> +#else
> +               WARN(1, "Unsupported UART type \"io\"\n");
> +               return;
> +#endif
>         }

I think we have to ensure that ->serial_in() always points
to some function that doesn't immediately panic, though that
could be an empty dummy like

       default:
               p->serial_in = IS_ENABLED(CONFIG_HAS_IOPORT) ?
                      io_serial_in : no_serial_in;
               p->serial_out = IS_ENABLED(CONFIG_HAS_IOPORT) ?
                      io_serial_out : no_serial_out;

Ideally we'd make mem_serial_in() the default function
and only use io_serial_in() when UPIO_PORT is selected,
but that still causes a NULL pointer dereference when
a platform initializes a 8250 like

static struct plat_serial8250_port serial_platform_data[] = {
        {
                .iobase         = 0x3f8, /* NULL pointer */
                .irq            = IRQ_ISA_UART,
                .uartclk        = 1843200,
        /* default   .iotype         = UPIO_PORT, */
        },

so I think an empty function plus a warning is best here.

> I've pushed a version with the above change rebased on v6.3-rc1 to my
> git.kernel.org repository and will do some more testing before I can
> hopefully send this out for review and make some progress on this.
> Meanwhile the original problem is now the only thing preventing clean
> Werror builds on clang for s390 as far as I understand.

Thanks a lot! Let's hope we can manage to get this merged at last.

    Arnd