Hi,
This has arrived in linux-next and causes boot warnings, see below.
On 25/05/2023 12:30, Tony Lindgren wrote:
> We want to enable runtime PM for serial port device drivers in a generic
> way. To do this, we want to have the serial core layer manage the
> registered physical serial controller devices.
>
> To manage serial controllers, let's set up a struct bus and struct device
> for the serial core controller as suggested by Greg and Jiri. The serial
> core controller devices are children of the physical serial port device.
> The serial core controller device is needed to support multiple different
> kind of ports connected to single physical serial port device.
>
> Let's also set up a struct device for the serial core port. The serial
> core port instances are children of the serial core controller device.
>
> With the serial core port device we can now flush pending TX on the
> runtime PM resume as suggested by Johan.
>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Suggested-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Suggested-by: Jiri Slaby <jirislaby@xxxxxxxxxx>
> Suggested-by: Johan Hovold <johan@xxxxxxxxxx>
> Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
...
>
> /**
> - * uart_remove_one_port - detach a driver defined port structure
> + * serial_core_remove_one_port - detach a driver defined port structure
> * @drv: pointer to the uart low level driver structure for this port
> * @uport: uart port structure for this port
> *
> @@ -3153,20 +3170,16 @@ EXPORT_SYMBOL(uart_add_one_port);
> *
> * This unhooks (and hangs up) the specified port structure from the core
> * driver. No further calls will be made to the low-level code for this port.
> + * Caller must hold port_mutex.
> */
> -void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> +static void serial_core_remove_one_port(struct uart_driver *drv,
> + struct uart_port *uport)
> {
> struct uart_state *state = drv->state + uport->line;
> struct tty_port *port = &state->port;
> struct uart_port *uart_port;
> struct tty_struct *tty;
>
> - mutex_lock(&port_mutex);
serial_core_remove_one_port() no longer takes port_mutex (caller must
hold it)...
> -
> - /*
> - * Mark the port "dead" - this prevents any opens from
> - * succeeding while we shut down the port.
> - */
> mutex_lock(&port->mutex);
> uart_port = uart_port_check(state);
> if (uart_port != uport)
> @@ -3177,7 +3190,6 @@ void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> mutex_unlock(&port->mutex);
> goto out;
> }
> - uport->flags |= UPF_DEAD;
> mutex_unlock(&port->mutex);
>
> /*
> @@ -3209,6 +3221,7 @@ void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> * Indicate that there isn't a port here anymore.
> */
> uport->type = PORT_UNKNOWN;
> + uport->port_dev = NULL;
>
> mutex_lock(&port->mutex);
> WARN_ON(atomic_dec_return(&state->refcount) < 0);
> @@ -3218,7 +3231,6 @@ void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> out:
> mutex_unlock(&port_mutex);
... but it still drops it at the end of the function.
> }
> -EXPORT_SYMBOL(uart_remove_one_port);
...
> +/*
> + * Find a registered serial core controller device if one exists. Returns
> + * the first device matching the ctrl_id. Caller must hold port_mutex.
> + */
> +static struct serial_ctrl_device *serial_core_ctrl_find(struct uart_driver *drv,
> + struct device *phys_dev,
> + int ctrl_id)
> +{
> + struct uart_state *state;
> + int i;
> +
> + lockdep_assert_held(&port_mutex);
This function must be called with port_mutex held, but...
> +
> + for (i = 0; i < drv->nr; i++) {
> + state = drv->state + i;
> + if (!state->uart_port || !state->uart_port->port_dev)
> + continue;
> +
> + if (state->uart_port->dev == phys_dev &&
> + state->uart_port->ctrl_id == ctrl_id)
> + return serial_core_get_ctrl_dev(state->uart_port->port_dev);
> + }
> +
> + return NULL;
> +}
...
> +/*
> + * Removes a serial core port device, and the related serial core controller
> + * device if the last instance.
> + */
> +void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port)
> +{
> + struct device *phys_dev = port->dev;
> + struct serial_port_device *port_dev = port->port_dev;
> + struct serial_ctrl_device *ctrl_dev = serial_core_get_ctrl_dev(port_dev);
> + int ctrl_id = port->ctrl_id;
> +
> + mutex_lock(&port_mutex);
We take port_mutex here...
> +
> + port->flags |= UPF_DEAD;
> +
> + serial_core_remove_one_port(drv, port);
> +
> + /* Note that struct uart_port *port is no longer valid at this point */
> + serial_base_port_device_remove(port_dev);
serial_base_port_device_remove() then drops it...
> +
> + /* Drop the serial core controller device if no ports are using it */
> + if (!serial_core_ctrl_find(drv, phys_dev, ctrl_id))
serial_core_ctrl_find() complains that it's not held.
> + serial_base_ctrl_device_remove(ctrl_dev);
> +
> + mutex_unlock(&port_mutex);
And we attempt to unlock it when it's not held.
I haven't studied this change in detail, but I assume the bug is that
serial_base_port_device_remove() shouldn't be dropping port_mutex. The
below hack gets my board booting again.
Thanks,
Steve
Hack fix:
----8<----
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 29bd5ede0b25..044e4853341a 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3234,8 +3234,7 @@ static void serial_core_remove_one_port(struct uart_driver *drv,
wait_event(state->remove_wait, !atomic_read(&state->refcount));
state->uart_port = NULL;
mutex_unlock(&port->mutex);
-out:
- mutex_unlock(&port_mutex);
+out:;
}
/**
