Re: [PATCH v12 1/1] serial: core: Start managing serial controllers to enable runtime PM

[Steven Price <steven.price@arm.com>]

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@linux.intel.com>
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Suggested-by: Jiri Slaby <jirislaby@kernel.org>
> Suggested-by: Johan Hovold <johan@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

...

>  
>  /**
> - * 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:;
 }
 
 /**