[PATCH] serial/8250: remove comment about schedule_timeout

[PATCH] serial/8250: remove comment about schedule_timeout

[Thadeu Lima de Souza Cascardo]

Ted T'so has added the function size_fifo in 1999 for the 2.3 series
[1], a long time ago.

During the 2.5 cycle, Russell King has restructured the serial drivers
and, in that process, has suggested using schedule_timeout instead of
mdelay in size_fifo. [2]

It was only at 2.6.7 that Greg Kroah-Hartman added the msleep function
to the core kernel, as people were starting to duplicate it. [3]

However, as size_fifo is called under a spinlock from the autoconfig
function, we might not use msleep here, so removing that comment is the
appropriate thing to do.

[1] http://lkml.iu.edu/hypermail/linux/kernel/9908.3/1229.html
[2] https://git.kernel.org/cgit/linux/kernel/git/tglx/history.git/commit/?id=33c0d1b0c3ebb61243d9b19ce70d9063acff2aac
[3] https://git.kernel.org/cgit/linux/kernel/git/tglx/history.git/commit/?id=8365c315507fe10925bb3281d74444fe02935b25

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: linux-serial@vger.kernel.org
Cc: Russell King <rmk+kernel@armlinux.org.uk>
Cc: Theodore Ts'o <tytso@mit.edu>
---
 drivers/tty/serial/8250/8250_port.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 1731b98d2471..645daf551fdf 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -796,7 +796,7 @@ static int size_fifo(struct uart_8250_port *up)
 	serial_out(up, UART_LCR, 0x03);
 	for (count = 0; count < 256; count++)
 		serial_out(up, UART_TX, count);
-	mdelay(20);/* FIXME - schedule_timeout */
+	mdelay(20);
 	for (count = 0; (serial_in(up, UART_LSR) & UART_LSR_DR) &&
 	     (count < 256); count++)
 		serial_in(up, UART_RX);
-- 
2.11.0

Re: [PATCH] serial/8250: remove comment about schedule_timeout

[One Thousand Gnomes]

On Thu,  5 Jan 2017 13:48:40 -0200
Thadeu Lima de Souza Cascardo <cascardo@canonical.com> wrote:

> Ted T'so has added the function size_fifo in 1999 for the 2.3 series
> [1], a long time ago.
> 
> During the 2.5 cycle, Russell King has restructured the serial drivers
> and, in that process, has suggested using schedule_timeout instead of
> mdelay in size_fifo. [2]
> 
> It was only at 2.6.7 that Greg Kroah-Hartman added the msleep function
> to the core kernel, as people were starting to duplicate it. [3]
> 
> However, as size_fifo is called under a spinlock from the autoconfig
> function, we might not use msleep here, so removing that comment is the
> appropriate thing to do.

No.. it's still a flaw in the driver that we can't use msleep here. It's
one of those things that people want to know if the probe locking ever
gets restructured.

Alan

Re: [PATCH] serial/8250: remove comment about schedule_timeout

[Thadeu Lima de Souza Cascardo]

On Thu, Jan 05, 2017 at 04:40:16PM +0000, One Thousand Gnomes wrote:
> On Thu,  5 Jan 2017 13:48:40 -0200
> Thadeu Lima de Souza Cascardo <cascardo@canonical.com> wrote:
> 
> > Ted T'so has added the function size_fifo in 1999 for the 2.3 series
> > [1], a long time ago.
> > 
> > During the 2.5 cycle, Russell King has restructured the serial drivers
> > and, in that process, has suggested using schedule_timeout instead of
> > mdelay in size_fifo. [2]
> > 
> > It was only at 2.6.7 that Greg Kroah-Hartman added the msleep function
> > to the core kernel, as people were starting to duplicate it. [3]
> > 
> > However, as size_fifo is called under a spinlock from the autoconfig
> > function, we might not use msleep here, so removing that comment is the
> > appropriate thing to do.
> 
> No.. it's still a flaw in the driver that we can't use msleep here. It's
> one of those things that people want to know if the probe locking ever
> gets restructured.
> 
> Alan

Okay. At first, I wanted to simply use msleep there, then realized there
was the spinlock. I thought I got a good writeup of the history that was
not in git yet, maybe we should just add a better comment and more
details in the commit log. How about this as the comment?

Cascardo.

---
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 1731b98d2471..51efbfcfb922 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -796,7 +796,8 @@ static int size_fifo(struct uart_8250_port *up)
 	serial_out(up, UART_LCR, 0x03);
 	for (count = 0; count < 256; count++)
 		serial_out(up, UART_TX, count);
-	mdelay(20);/* FIXME - schedule_timeout */
+	/* FIXME - use msleep when probe locking is restructured */
+	mdelay(20);
 	for (count = 0; (serial_in(up, UART_LSR) & UART_LSR_DR) &&
 	     (count < 256); count++)
 		serial_in(up, UART_RX);

Re: [PATCH] serial/8250: remove comment about schedule_timeout

[Theodore Ts'o]

On Thu, Jan 05, 2017 at 03:17:45PM -0200, Thadeu Lima de Souza Cascardo wrote:
> Okay. At first, I wanted to simply use msleep there, then realized there
> was the spinlock. I thought I got a good writeup of the history that was
> not in git yet, maybe we should just add a better comment and more
> details in the commit log. How about this as the comment?

Could you look at fixing this, instead?  I was taking a look at the
code, and it so very.... 1990's.  It looks like it was written during
the days before SMP, and then locking was smashed in a very
blunt-force way ---- probably because that's how it happened.  :-)

What we should **really** be doing with autoconfiguration, is instead
of using the spinlock to prevent it from being accessed, is to lock
out any attempt to transmit from the top-half, and remove it from the
linked list of ports to prevent the interrupt handler from accessing
it, make sure that any active interrupt handler has exited (and you
can use the spinlock for the preceeding bits), but then we can safely
do the autoconfiguration without having the deal with the spinlock at
all, afterwards.

It should't be *that* hard to just fix it for real.

Cheers,

						- Ted