On Mon, Jul 06, 2020 at 05:36:56PM +0100, Marc Zyngier wrote:
> On 2020-07-06 15:35, Andy Shevchenko wrote:
> > The comment near to uart_port_spin_lock_init() says:
> >
> > Ensure that the serial console lock is initialised early.
> > If this port is a console, then the spinlock is already initialised.
> >
> > and there is nothing about enabled or disabled consoles. The commit
> > a3cb39d258ef ("serial: core: Allow detach and attach serial device
> > for console") made a change, which follows the comment, and also to
> > prevent reinitialisation of the lock in use, when user detaches and
> > attaches back the same console device. But this change discovers
> > another issue, that uart_add_one_port() tries to access a spin lock
> > that now may be uninitialised. This happens when a driver expects
> > the serial core to register a console on its behalf. In this case
> > we must initialise a spin lock before use.
...
> > +static inline void uart_any_port_spin_lock_init(struct uart_port *port)
>
> nit: __uart_port_spin_lock_init() looks like a better name to me
> (as a primitive of uart_port_spin_lock_init). You can also drop
> the inline which doesn't mean much these days (and this isn't
> a hot path).
Actually I was thinking about that name, but decided to go without underscores
(b/c some developers like this approach, some like that one). I'm fine with
your proposal nevertheless.
> > + /*
> > + * If this driver supports console, and it hasn't been
> > + * successfully registered yet, initialise spin lock for it.
> > + */
> > + if (port->cons && !(port->cons->flags & CON_ENABLED))
> > + uart_any_port_spin_lock_init(port);
> > +
> > /*
> > * Ensure that the modem control lines are de-activated.
> > * keep the DTR setting that is set in uart_set_options()
>
> Otherwise looks OK to me (having tested an earlier version).
> With the above addressed:
>
> Acked-by: Marc Zyngier <maz@xxxxxxxxxx>
Thanks!
--
With Best Regards,
Andy Shevchenko
