headers
Anton Vorontsov | 1 Jul 2008 15:43

[PATCH] serial: 8250: fix shared interrupts issues with SMP and RT kernels

With SMP kernels _irqsave spinlock disables only local interrupts, while
the shared serial interrupt could be assigned to the CPU that is not
currently starting up the serial port.

This might cause issues because serial8250_startup() routine issues
IRQ-triggering operations before registering the port in the IRQ chain
(though, this is fine to do and done explicitly because we don't want to
process any interrupts on the port startup).

With RT kernels and preemptable hardirqs, _irqsave spinlock does not
disable local hardirqs, and the bug could be reproduced much easily:

$ cat /dev/ttyS0 &
$ cat /dev/ttyS1
irq 42: nobody cared (try booting with the "irqpoll" option)
Call Trace:
[C0475EB0] [C0008A98] show_stack+0x4c/0x1ac (unreliable)
[C0475EF0] [C004BBD4] __report_bad_irq+0x34/0xb8
[C0475F10] [C004BD38] note_interrupt+0xe0/0x308
[C0475F50] [C004B09C] thread_simple_irq+0xdc/0x104
[C0475F70] [C004B3FC] do_irqd+0x338/0x3c8
[C0475FC0] [C00398E0] kthread+0xf8/0x100
[C0475FF0] [C0011FE0] original_kernel_thread+0x44/0x60
handlers:
[<c02112c4>] (serial8250_interrupt+0x0/0x138)
Disabling IRQ #42

After this, all serial ports on the given IRQ are non-functional.

To fix the issue we should explicitly disable shared IRQ before
(Continue reading)

Permalink | Reply | 0 comments |
headers
Alan Cox | 1 Jul 2008 15:43
Picon

Re: [PATCH] serial: 8250: fix shared interrupts issues with SMP and RT kernels

> > again, please let the -rt maintainers sort out which patches need to be 
> > propagated to upstream maintainers.
> 
> This appears to be not only RT issue though. In theory, this can be

Agreed - RT is showing up a real bug here.

> triggered on SMP also. Thanks to Daniel Walker for pointing this out.

It looks correct to me except that you cannot use spin_lock/disable_irq
in that way safely. You must always disable_irq before taking the lock,
or prove it is safe and use disable_irq_nosync

The reason:
		CPU#0 spin_lock_... [taken]
		CPU#1 IRQ
		CPU#1 spin_lock [waits]
		CPU#0 disable_irq (deadlock)

Note that is also not generally safe to do

		disable IRQ on device
		spin_lock
		disable_irq

because IRQ propogation occurs asynchronously to PCI bus traffic even on
PC class systems (especially Pentium-PII era boxes with SMP). You can
disable the device IRQ and still have an IRQ 'in flight' that arrives
afterwards.
(Continue reading)

Permalink | Reply | 0 comments |
headers
Anton Vorontsov | 1 Jul 2008 17:02

[PATCH v2] serial: 8250: fix shared interrupts issues with SMP and RT kernels

With SMP kernels _irqsave spinlock disables only local interrupts, while
the shared serial interrupt could be assigned to the CPU that is not
currently starting up the serial port.

This might cause issues because serial8250_startup() routine issues
IRQ-triggering operations before registering the port in the IRQ chain
(though, this is fine to do and done explicitly because we don't want to
process any interrupts on the port startup).

With RT kernels and preemptable hardirqs, _irqsave spinlock does not
disable local hardirqs, and the bug could be reproduced much easily:

$ cat /dev/ttyS0 &
$ cat /dev/ttyS1
irq 42: nobody cared (try booting with the "irqpoll" option)
Call Trace:
[C0475EB0] [C0008A98] show_stack+0x4c/0x1ac (unreliable)
[C0475EF0] [C004BBD4] __report_bad_irq+0x34/0xb8
[C0475F10] [C004BD38] note_interrupt+0xe0/0x308
[C0475F50] [C004B09C] thread_simple_irq+0xdc/0x104
[C0475F70] [C004B3FC] do_irqd+0x338/0x3c8
[C0475FC0] [C00398E0] kthread+0xf8/0x100
[C0475FF0] [C0011FE0] original_kernel_thread+0x44/0x60
handlers:
[<c02112c4>] (serial8250_interrupt+0x0/0x138)
Disabling IRQ #42

After this, all serial ports on the given IRQ are non-functional.

To fix the issue we should explicitly disable shared IRQ before
(Continue reading)

Permalink | Reply | 0 comments |
headers
Kumar Gala | 2 Jul 2008 09:46

Re: [PATCH] cpm_uart: Support uart_wait_until_sent()


On Jun 26, 2008, at 6:55 AM, Laurent Pinchart wrote:

> Set port->fifosize to the software FIFO size, and update the port  
> timeout
> when the baud rate is modified. SCC ports have an optional 32 byte  
> hardware
> FIFO which is currently not taken into account, as there is no  
> documented way
> to check when the FIFO becomes empty.
>
> Signed-off-by: Laurent Pinchart <laurentp <at> cse-semaphore.com>
> ---
> drivers/serial/cpm_uart/cpm_uart_core.c |    6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)

I spoke to soon about being able to apply this.  Can you respin this  
against my powerpc-next tree.  Also, I had troubles applying this due  
to mailer formatting issues.

- k

> diff --git a/drivers/serial/cpm_uart/cpm_uart_core.c b/drivers/ 
> serial/cpm_uart/cpm_uart_core.c
> index a19dc7e..151cad2 100644
> --- a/drivers/serial/cpm_uart/cpm_uart_core.c
> +++ b/drivers/serial/cpm_uart/cpm_uart_core.c
>  <at>  <at>  -547,6 +547,11  <at>  <at>  static void cpm_uart_set_termios(struct  
> uart_port *port,
> 	}
(Continue reading)

Permalink | Reply | 0 comments |
headers
Laurent Pinchart | 2 Jul 2008 10:58

[PATCHv2] cpm_uart: Support uart_wait_until_sent()

Set port->fifosize to the software FIFO size, and update the port timeout
when the baud rate is modified. SCC ports have an optional 32 byte hardware
FIFO which is currently not taken into account, as there is no documented way
to check when the FIFO becomes empty.

Signed-off-by: Laurent Pinchart <laurentp <at> cse-semaphore.com>
---
 drivers/serial/cpm_uart/cpm_uart_core.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/serial/cpm_uart/cpm_uart_core.c b/drivers/serial/cpm_uart/cpm_uart_core.c
index a19dc7e..151cad2 100644
--- a/drivers/serial/cpm_uart/cpm_uart_core.c
+++ b/drivers/serial/cpm_uart/cpm_uart_core.c
 <at>  <at>  -608,6 +608,11  <at>  <at>  static void cpm_uart_set_termios(struct uart_port *port,
 	}

 	/*
+	 * Update the timeout
+	 */
+	uart_update_timeout(port, termios->c_cflag, baud);
+
+	/*
 	 * Set up parity check flag
 	 */
 #define RELEVANT_IFLAG(iflag) (iflag & (IGNBRK|BRKINT|IGNPAR|PARMRK|INPCK))
 <at>  <at>  -1068,6 +1073,7  <at>  <at>  int cpm_uart_drv_get_platform_data(struct platform_device *pdev, int is_con)
 	pinfo->port.type = PORT_CPM;
 	pinfo->port.ops = &cpm_uart_pops,
 	pinfo->port.iotype = UPIO_MEM;
(Continue reading)

Permalink | Reply | 0 comments |
headers
Kumar Gala | 2 Jul 2008 18:13

Re: [PATCHv2] cpm_uart: Support uart_wait_until_sent()


On Jul 2, 2008, at 3:58 AM, Laurent Pinchart wrote:

> Set port->fifosize to the software FIFO size, and update the port  
> timeout
> when the baud rate is modified. SCC ports have an optional 32 byte  
> hardware
> FIFO which is currently not taken into account, as there is no  
> documented way
> to check when the FIFO becomes empty.
>
> Signed-off-by: Laurent Pinchart <laurentp <at> cse-semaphore.com>
> ---
> drivers/serial/cpm_uart/cpm_uart_core.c |    6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)

applied.

- k
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Permalink | Reply | 0 comments |
headers
Anton Vorontsov | 9 Jul 2008 21:29

Re: [PATCH v2] serial: 8250: fix shared interrupts issues with SMP and RT kernels

On Tue, Jul 01, 2008 at 07:02:54PM +0400, Anton Vorontsov wrote:
[...]
> Signed-off-by: Anton Vorontsov <avorontsov <at> ru.mvista.com>
> ---
> 
> On Tue, Jul 01, 2008 at 02:43:53PM +0100, Alan Cox wrote:
[...]
> > It looks correct to me except that you cannot use spin_lock/disable_irq
> > in that way safely. You must always disable_irq before taking the lock,
> > or prove it is safe and use disable_irq_nosync
> > 
> > The reason:
> > 		CPU#0 spin_lock_... [taken]
> > 		CPU#1 IRQ
> > 		CPU#1 spin_lock [waits]
> > 		CPU#0 disable_irq (deadlock)
> 
> This deadlock possibility is interesting by itself, thanks for
> mentioning it.
> 
> But this can't happen here. IRQ will not grab the up->port.lock,
> because port isn't registered in the 8250 IRQ handling chain (yet).

Alan, are there any issues with the proof or the patch itself?

Thanks,

> As for _nosync, probably this is good idea indeed, and should be safe
> AFAICS.
>
(Continue reading)

Permalink | Reply | 0 comments |
headers
Alan Cox | 9 Jul 2008 21:21
Picon

Re: [PATCH v2] serial: 8250: fix shared interrupts issues with SMP and RT kernels

> > But this can't happen here. IRQ will not grab the up->port.lock,
> > because port isn't registered in the 8250 IRQ handling chain (yet).
> 
> Alan, are there any issues with the proof or the patch itself?

The logic looks fine, the analysis looks fine. 

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Permalink | Reply | 0 comments |
headers
Axel Hosemann | 10 Jul 2008 01:16
Picon

Suggested patch for linux

Dear Sirs,
the support of the serial interface by linux systems nowadays suffers 
from the disadvantage, that hardware handshake is only supported for the 
full duplex mode. That means, that the RTS signal is getting always 
asserted immediately after the serial was opened. So the RTS signal 
under linux, as far as I understood it, carries exactly the same 
information as the DTR line. That means, that there is no half-duplex 
behaviour provided by linux.  A half-duplex  application as e.g. RS-485 
controllers or radio modems  are depending on a different behaviour: The 
serial port asserts DTR after it was opened, but RTS must only be 
asserted, when data has been written to the tx buffer for transmission. 
Stimulated by  the RTS signal, the DCE will then engage the channel for 
transmission and release it, after the RTS has been deasserted when the 
buffer had been sent. This behaviour is up to now not supported by the 
linux serial drivers.
Therefore I make a suggestion for adding the half-duplex support for 
linux kernels by the attached patch, which I  created and tested on a 
UART 16550 compliant  hardware and additionally by means of a ftdi usb 
to serial port adaptor.  The patch was compiled and tested with an 
UBUNTU 2.6.24-19-generic environment.
If you see a chance, to add this functionality for future linux kernels, 
I would be interested in further discussions about this patch.

Regards

Axel Hosemann
Bergstrasse 6f
CH 5644 Auw
Attachment (serialHalfDuplex.patch): text/x-patch, 11 KiB
(Continue reading)

Permalink | Reply | 19 comments |
headers
Alan Cox | 10 Jul 2008 10:59
Picon

Re: Suggested patch for linux

> Stimulated by  the RTS signal, the DCE will then engage the channel for 
> transmission and release it, after the RTS has been deasserted when the 
> buffer had been sent. This behaviour is up to now not supported by the 
> linux serial drivers.

Actually you can drive this behaviour from user space which is what
software like scarabd has done for many years.

> If you see a chance, to add this functionality for future linux kernels, 
> I would be interested in further discussions about this patch.

The big problem is that the kernel does not know what is a "transmission"
it just sees a series of writes to the device. In many cases the
multi-drop or radio systems also need the caller to wait for a
transmission slot either by beacon, by timing or by monitoring the
carrier detect to avoid collisions.

That seems to best be done in user space as the algorithms are quite
variable and some are complex.

Do we really benefit from having this in kernel ?

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Permalink | Reply | 17 comments |

Gmane