| Message ID | 20140304.141003.1969905483115684548.davem@davemloft.net |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
Hi, David! On 04.03.2014 23:10, David Miller wrote: > From: Kirill Tkhai <tkhai@yandex.ru> > Date: Fri, 28 Feb 2014 18:51:23 +0400 > >> This should be fixed like that's done for 8250 >> [patch drivers-serial-cleanup-locking-for-rt.patch] > > Like the patch below? Exactly this. > Is this transformation legitimate outside of the -rt tree? I wish the commit > messages for the 8250 change was more verbose, explaining exactly why this > is needed. > > And if it's needed, all the sparc serial/console drivers need this too. port->lock is mutex, so we should not disable interrupts before we are doing spin_lock(). In RT spin_trylock_irqsave() does not disable irqs. The patch is made to be good for both RT and !RT cases, so everything is ok! Really I don't know, why the original patch is not in upstream ***shrugging***. It's from 2009, everything is possible. > diff --git a/drivers/tty/serial/sunhv.c b/drivers/tty/serial/sunhv.c > index cf86e72..dc697ce 100644 > --- a/drivers/tty/serial/sunhv.c > +++ b/drivers/tty/serial/sunhv.c > @@ -433,13 +433,10 @@ static void sunhv_console_write_paged(struct console *con, const char *s, unsign > unsigned long flags; > int locked = 1; > > - local_irq_save(flags); > - if (port->sysrq) { > - locked = 0; > - } else if (oops_in_progress) { > - locked = spin_trylock(&port->lock); > - } else > - spin_lock(&port->lock); > + if (port->sysrq || oops_in_progress) > + locked = spin_trylock_irqsave(&port->lock, flags); > + else > + spin_lock_irqsave(&port->lock, flags); > > while (n > 0) { > unsigned long ra = __pa(con_write_page); > @@ -470,8 +467,7 @@ static void sunhv_console_write_paged(struct console *con, const char *s, unsign > } > > if (locked) > - spin_unlock(&port->lock); > - local_irq_restore(flags); > + spin_unlock_irqrestore(&port->lock, flags); > } > > static inline void sunhv_console_putchar(struct uart_port *port, char c) > @@ -492,7 +488,10 @@ static void sunhv_console_write_bychar(struct console *con, const char *s, unsig > unsigned long flags; > int i, locked = 1; > > - local_irq_save(flags); > + if (port->sysrq || oops_in_progress) > + locked = spin_trylock_irqsave(&port->lock, flags); > + else > + spin_lock_irqsave(&port->lock, flags); > if (port->sysrq) { > locked = 0; > } else if (oops_in_progress) { > @@ -507,8 +506,7 @@ static void sunhv_console_write_bychar(struct console *con, const char *s, unsig > } > > if (locked) > - spin_unlock(&port->lock); > - local_irq_restore(flags); > + spin_unlock_irqrestore(&port->lock, flags); > } > > static struct console sunhv_console = { > -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* David Miller | 2014-03-04 14:10:03 [-0500]: >> This should be fixed like that's done for 8250 >> [patch drivers-serial-cleanup-locking-for-rt.patch] > >Like the patch below? The patch looks good. >Is this transformation legitimate outside of the -rt tree? I wish the commit >messages for the 8250 change was more verbose, explaining exactly why this >is needed. Looking at it, it looks like the transformation is legitimate in vanila, too. In -RT the spin_lock_irqsave() does not spin but sleep if the lock is taken. Before that, local_irq_save() is invoked which disables interrupts even on -RT. That is why you might_sleep() triggers a warning here. In the ->sysrq and oops_in_progress case it is save to trylock the lock i.e. this is what we do now anyway except for ->sysrq where we assume that the lock is already taken. The spin_lock_irqsave() grabs the lock and disables the interrupts on vanila (the same behavior) and on -RT it won't disable interrupts. >And if it's needed, all the sparc serial/console drivers need this too. Let me forward the 8250 to GregKH and see how he reacts. Would be nice if it is another patch that gets removed from that queue… Sebastian -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/tty/serial/sunhv.c b/drivers/tty/serial/sunhv.c index cf86e72..dc697ce 100644 --- a/drivers/tty/serial/sunhv.c +++ b/drivers/tty/serial/sunhv.c @@ -433,13 +433,10 @@ static void sunhv_console_write_paged(struct console *con, const char *s, unsign unsigned long flags; int locked = 1; - local_irq_save(flags); - if (port->sysrq) { - locked = 0; - } else if (oops_in_progress) { - locked = spin_trylock(&port->lock); - } else - spin_lock(&port->lock); + if (port->sysrq || oops_in_progress) + locked = spin_trylock_irqsave(&port->lock, flags); + else + spin_lock_irqsave(&port->lock, flags); while (n > 0) { unsigned long ra = __pa(con_write_page); @@ -470,8 +467,7 @@ static void sunhv_console_write_paged(struct console *con, const char *s, unsign } if (locked) - spin_unlock(&port->lock); - local_irq_restore(flags); + spin_unlock_irqrestore(&port->lock, flags); } static inline void sunhv_console_putchar(struct uart_port *port, char c) @@ -492,7 +488,10 @@ static void sunhv_console_write_bychar(struct console *con, const char *s, unsig unsigned long flags; int i, locked = 1; - local_irq_save(flags); + if (port->sysrq || oops_in_progress) + locked = spin_trylock_irqsave(&port->lock, flags); + else + spin_lock_irqsave(&port->lock, flags); if (port->sysrq) { locked = 0; } else if (oops_in_progress) { @@ -507,8 +506,7 @@ static void sunhv_console_write_bychar(struct console *con, const char *s, unsig } if (locked) - spin_unlock(&port->lock); - local_irq_restore(flags); + spin_unlock_irqrestore(&port->lock, flags); } static struct console sunhv_console = {