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
