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

[Andy Shevchenko]

On Wed, Nov 23, 2022 at 10:28:24AM +0200, 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.
> 
> The serial layer has a few challenges to deal with:
> 
> 1. The serial port mapping to a physical serial port controller device
>    is currently not easily available after the physical serial controller
>    struct device gets registered uart_add_one_port() time
> 
> 2. The serial port device drivers have their own driver data. So we cannot
>    currently start making use of serial core generic data easily without
>    changing all the serial port device drivers
> 
> To find the serial ports for a controller based on struct device, let's
> add a new data structure for a serial_controller. On registering a port,
> we can use the drv->state array to find the associated serial port
> controller and initialize the serial core controller.
> 
> As some serial port device drivers enable runtime PM in their probe before
> registering with the serial core layer, and some do not enable runtime PM
> at all currently, we need check the state in the serial core layer on
> uart_port_startup(). We need to also consider that a serial port device
> may have multiple ports.
> 
> Initially we enable runtime PM for all the serial port controller devices.
> This allows us to add runtime PM calls and properly handle any errors
> without a need for serial layer specific runtime PM wrapper functions.
> 
> After this patch no functional changes for the serial port device drivers
> are intended. We just enable runtime PM and keep the runtime PM usage
> count until all the serial controller ports are unregistered. For drivers
> implementing PM runtime, we just keep track of the runtime PM
> configuration.
> 
> For the serial port drivers, the serial core layer has the following use
> cases to deal with:
> 
> 1. If a serial port device driver does not implement runtime PM, the
>    device state is set to active state, and the runtime PM usage count
>    is kept until the last port for a device is unregistered
> 
> 2. If a serial port device driver implements runtime PM, the runtime PM
>    usage count is kept until the last port for the device is unregistered
> 
> 3. If a serial port device driver implements runtime PM autosuspend,
>    autosuspend is not prevented. This currently gets set only for the
>    8250_omap driver to keep runtime PM working for it
> 
> For system suspend, things should be mostly detached from the runtime PM.
> The serial port device drivers may call pm_runtime_force_suspend() and
> pm_runtime_force_resume() as needed.

...

> +static struct serial_controller *to_controller(struct uart_port *port)

Perhaps to_serial_controller()? Or to_serial_ctrl() ?

> +{
> +	if (!port->dev)
> +		return NULL;
> +
> +	return port->state->controller;
> +}

...

> +/*
> + * Starts runtime PM for the serial controller device if not already started
> + * by the serial port driver. Called from uart_add_one_port() with port_mutex
> + * held.

Perhaps we may utilize lockdep_assert() here?

> + */

Also maybe might_sleep() can be added where it's appropriate?

...

> +static int serial_core_register_port(struct uart_port *port,
> +				     struct uart_driver *drv)
> +{
> +	struct serial_controller *controller;
> +	bool allocated = false;
> +	int ret;
> +
> +	if (!port->dev)
> +		return 0;
> +
> +	controller = serial_core_find_controller(drv, port->dev);
> +	if (!controller) {

> +		controller = kzalloc(sizeof(*controller), GFP_KERNEL);
> +		if (!controller)
> +			return -ENOMEM;
> +
> +		controller->drv = drv;
> +		controller->dev = port->dev;
> +		controller->supports_autosuspend = port->supports_autosuspend;
> +		allocated = true;

Maybe split this to a serial_core_controller_alloc()?

> +	}
> +
> +	port->state->controller = controller;
> +	WARN_ON(port->supports_autosuspend != controller->supports_autosuspend);
> +
> +	ret = serial_core_pm_runtime_start(port);
> +	if (ret < 0)
> +		goto err_free;
> +
> +	return 0;
> +
> +err_free:
> +	port->state->controller = NULL;
> +	if (allocated)
> +		kfree(controller);
> +
> +	return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko