Re: [PATCH] serial: 8250: clear RX trigger callbacks when removing a port

[James Hilliard]

On Tue, Oct 27, 2026 at 12:09 AM Jiri Slaby <jirislaby@xxxxxxxxxx> wrote:
>
> On 24. 10. 26, 22:05, James Hilliard wrote:
> > serial8250_unregister_port() removes the device supplied by a driver and
> > normally re-registers the persistent slot as an unknown legacy port. The
> > slot retains its RX trigger callbacks and sysfs attribute group, so the
> > replacement rx_trig_bytes attribute can still call the removed driver's
> > code. Unloading that driver leaves the callbacks pointing into freed
> > module text.
> >
> > Clear both callbacks after uart_remove_one_port() has drained the old
> > sysfs users and before registering the fallback port. Its RX trigger
> > attribute then uses the generic handlers, which reject a port without
> > FIFO capabilities.
> >
> > Also copy the callbacks unconditionally during registration. A replacement
> > port without custom handlers must use the generic implementation instead
> > of inheriting callbacks from the previous owner of the slot.
>
> I don't understand why this is a problem only for the two callbacks.
> Care to elaborate?

It isn't limited to those two callbacks. The specific path here is the
rx_trig_bytes attribute recreated for the fallback PORT_UNKNOWN port.
Its show/store handlers call get_rxtrig/set_rxtrig directly, without
requiring an opened or initialized port. The FIFO capability checks are
inside the generic handlers, so retaining a custom callback bypasses
those checks.

The other callbacks have different protections: serial8250_init_port()
clears pm, for example, and uart_port_startup() skips startup for
PORT_UNKNOWN. Those protections do not cover subsequent slot reuse.

In that replacement case, startup and set_termios are also copied only
when the new driver supplies them, and serial8250_set_defaults() does
not reset them. They can therefore retain the previous owner's handlers
too.

This patch addresses the direct sysfs exposure introduced by the
referenced commit, including replacement of these callbacks when the
slot is reused. The other retained callbacks need a broader cleanup of
the replacement path as well.

>
> > Fixes: 8a6c543cf8b3 ("serial: 8250: allow UART drivers to override rx_trig_bytes handling")
> > Signed-off-by: James Hilliard <james.hilliard1@xxxxxxxxx>
> > ---
> >   drivers/tty/serial/8250/8250_core.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> > index 42319843cb5b..c638e10c43e2 100644
> > --- a/drivers/tty/serial/8250/8250_core.c
> > +++ b/drivers/tty/serial/8250/8250_core.c
> > @@ -830,10 +830,8 @@ static int __serial8250_register_8250_port(struct uart_8250_port *uart,
> >               uart->port.get_divisor = up->port.get_divisor;
> >       if (up->port.set_divisor)
> >               uart->port.set_divisor = up->port.set_divisor;
> > -     if (up->port.get_rxtrig)
> > -             uart->port.get_rxtrig = up->port.get_rxtrig;
> > -     if (up->port.set_rxtrig)
> > -             uart->port.set_rxtrig = up->port.set_rxtrig;
> > +     uart->port.get_rxtrig = up->port.get_rxtrig;
> > +     uart->port.set_rxtrig = up->port.set_rxtrig;
> >       if (up->port.startup)
> >               uart->port.startup = up->port.startup;
> >       if (up->port.shutdown)
> > @@ -953,6 +951,9 @@ void serial8250_unregister_port(struct uart_8250_port *uart)
> >       }
> >
> >       uart_remove_one_port(&serial8250_reg, &uart->port);
> > +     /* The fallback port must use the generic RX trigger handlers. */
> > +     uart->port.get_rxtrig = NULL;
> > +     uart->port.set_rxtrig = NULL;
> >       if (serial8250_isa_devs) {
> >               uart->port.flags &= ~UPF_BOOT_AUTOCONF;
> >               uart->port.type = PORT_UNKNOWN;
>
> thanks,
> --
> js
> suse labs