Message-ID: <772a05f5f3c1b35864b80bf78429b843bdf14ee9.camel@linux.ibm.com>
Subject: Re: [niks:has_ioport_v3] [tty] aa0652d7f1:
 BUG:kernel_NULL_pointer_dereference,address
From:   Niklas Schnelle <schnelle@linux.ibm.com>
To:     Arnd Bergmann <arnd@kernel.org>,
        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>
Date:   Wed, 08 Mar 2023 15:03:55 +0100
In-Reply-To: <c85f0735-6c85-4a6e-ad07-66493783c58e@app.fastmail.com>
References: <202301051008.a8468257-yujie.liu@intel.com>
         <e211f932-77c3-427a-859a-d846598524ed@app.fastmail.com>
         <2e79ebb0ef62e550231c3daf46c62cd60d78acbc.camel@linux.ibm.com>
         <c85f0735-6c85-4a6e-ad07-66493783c58e@app.fastmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Wed, 2023-03-08 at 13:21 +0100, Arnd Bergmann wrote:
> On Wed, Mar 8, 2023, at 12:24, Niklas Schnelle wrote:
> > On Thu, 2023-01-05 at 09:03 +0100, Arnd Bergmann wrote:
> >=20
> > 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:
> >=20
> > @@ -519,9 +534,14 @@ static void set_io_from_upio(struct uart_port *p)
> >  #endif
> >=20
> >         default:
> > +#ifdef CONFIG_HAS_IOPORT
> >                 p->serial_in =3D io_serial_in;
> >                 p->serial_out =3D io_serial_out;
> >                 break;
> > +#else
> > +               WARN(1, "Unsupported UART type \"io\"\n");
> > +               return;
> > +#endif
> >         }
>=20
> 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
>=20
>        default:
>                p->serial_in =3D IS_ENABLED(CONFIG_HAS_IOPORT) ?
>                       io_serial_in : no_serial_in;
>                p->serial_out =3D IS_ENABLED(CONFIG_HAS_IOPORT) ?
>                       io_serial_out : no_serial_out;

Sadly the IS_ENABLED() plus ternary still gives me an undeclared
identifier error for io_serial_in(). So I think we need the more ugly
#ifdef. With that I hope it would then not crash even if one might be
left without any console at all.

>=20
> 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
>=20
> static struct plat_serial8250_port serial_platform_data[] =3D {
>         {
>                 .iobase         =3D 0x3f8, /* NULL pointer */
>                 .irq            =3D IRQ_ISA_UART,
>                 .uartclk        =3D 1843200,
>         /* default   .iotype         =3D UPIO_PORT, */
>         },
>=20
> so I think an empty function plus a warning is best here.

So in the above case .iotype is implicitly set to 0 which is UPIO_PORT
so I think one could argue it is selected, no? Not sure how picking
UPIO_MEM as default would look like then. One thing we could do though
is make the switch/case more regular like so:

...
#ifdef CONFIG_HAS_IOPORT
	case UPIO_PORT:
		p->serial_in =3D io_serial_in;
		p->serial_out =3D io_serial_out;
		break;
#endif

	default:
		WARN(1, "Unsupported UART type %x\n", p->iotype);
		p->serial_in =3D no_serial_in;
		p->serial_out =3D no_serial_out;
	}
...

That way we would have to always define no_serial_in() /
no_serial_out() but would also gracefully handle when p->iotype is set
to some other value and it looks relatively clean.