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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, Apr 11, 2022 at 03:02:18PM +0300, 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 serial port controllers. For runtime PM, we need a way to find
> the serial ports for each serial port controller device.
> 
> The serial core manages ports. Each serial controller can have multiple
> ports. As serial core has no struct device, and the serial port device
> drivers have their own driver data, we cannot currently start making
> use of serial core generic data easily without changing all the serial
> port device drivers.
> 
> We could consider adding a serial core specific struct device. It would
> be a child of the serial port device, and would allow us eventually to use
> device_links to add generic runtime PM calls for example. But as the serial
> core layer is not a device driver, driver specific features would need to
> be added, and are probably not justified for a virtual device.
> 
> Considering the above, let's improve the serial core layer so we can
> manage the serial port controllers better. Let's register the controllers
> with the serial core layer in addition to the serial ports.
> 
> To find the serial ports for a controller based on struct device, let's
> add a new data structure for a serial_controller. Let's add the registered
> devices into a radix_tree so we can look up the controller easily even
> with many controllers registered. This allows us to keep track of the
> runtime PM state for each serial port controller device.
> 
> 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 just want to 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. For most cases, we just enable runtime PM and keep the
> runtime PM usage count until all the serial controller ports are
> unregistered. For drivers implementing runtime PM, we just keep track of
> the configuration.
> 
> The serial core layer has the following use cases to deal with:
> 
> - 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
> 
> - 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
> 
> - 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.

...

> +struct serial_controller {
> +	struct uart_driver *drv;		/* For port specific uart_state */

> +	struct mutex lock;			/* For changing enabled_count */
> +	int enabled_count;			/* Enable count for runtime PM */

Wondering if we may use kref instead which will check for saturation as well.

> +	unsigned long implements_pm_runtime:1;
> +	unsigned long supports_autosuspend:1;
> +};

...

> +		WARN_ON(port->supports_autosuspend !=
> +			controller->supports_autosuspend);

One line?


...

> +	controller = kzalloc(sizeof(*controller), GFP_KERNEL);
> +	if (!controller)
> +		return -ENOMEM;
> +
> +	mutex_init(&controller->lock);
> +	controller->drv = drv;
> +	controller->supports_autosuspend = port->supports_autosuspend;
> +	port->state->controller = controller;
> +
> +	return radix_tree_insert(&serial_core_devices, idx, controller);

Hmm... Memory leak at error?


...

> +	if (!idx)
> +		return;

Do you really need this?

> +	controller = radix_tree_lookup(&serial_core_devices, idx);
> +	if (!controller)
> +		return;

...and/or this?

> +	controller = radix_tree_delete(&serial_core_devices, idx);
> +	if (!controller)
> +		return;

...

> +	struct device *dev = port->dev;

I would split assignment to be closer to first user...

...

...somewhere here.

> +	if (!dev)
> +		return 0;

...

> +	struct serial_controller *controller;
> +	struct device *dev = port->dev;
> +	struct uart_state *state;

> +	if (!dev)
> +		return;

Ditto.

...

>  	unsigned char		hub6;			/* this should be in the 8250 driver */
>  	unsigned char		suspended;
>  	unsigned char		console_reinit;
> +	unsigned long		supports_autosuspend:1;

Hmm... Maybe use unsigned char and convert all of them to something else if needed?

-- 
With Best Regards,
Andy Shevchenko