21285 serial driver deadlock(?) fix

Hello

I've got a board with a StrongARM SA-110 and a 21285 "footbrtidge" on it. The kernel's 21285 serial driver works for kernel messages but not for I/O from userspace.

My kernel experience is pretty limited, but I believe the problem lies in drivers/serial/21285.c.

As I understand it:
serial21285_tx_chars looks like an ISR, which likes to call serial21285_stop_tx every once in a while. serial21285_stop_tx, in turn, calls disable_irq(), which waits for the current instance of the ISR to finish before disabling that interrupt. But, calling this from within an ISR results in deadlock and makes the puppy very sad.

I've modified the ISR to call a different version of stop_tx, which uses disable_irq_nosync() instead. This gave me a working console.

Another issue with the driver is the baud rate calculation, which seems broken for me. I had to comment out the lines that write to the BRG register (not in this patch), but this may be a result of my board using a different clock rate than a real EBSA285. Maybe I will look into that later.

I have attached the relevant patch. I am also using this with my previous CONFIG_MMU patch and a minor modification to ignore the machine detection, or else my kernel wouldn't boot at all.

What do you guys think?

Thanks
Steve

-------------- next part --------------
A non-text attachment was scrubbed...
Name: footbridge-serial-deadlock.patch
Type: text/x-patch
Size: 777 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20091228/2b57160b/attachment.bin>

That's right - however, I'm not sure whether the stop_rx also needs to be a _nosync version.

Works for me. Could be that your 21285 is clocked at a different rate, in which case the 21285 timers are also going to be wrong. You need to specify the 21285 clock rate to the booting kernel via ATAGs or the "mem_fclk_21285=" command line option.

There is no way for the kernel to autodetect this; it must be passed to the kernel in some manner from something like a boot loader.

I sent a similar patch some weeks ago:

http://www.spinics.net/lists/arm-kernel/msg77006.html

which fixes the same problem for me. It would be good to get at least one of these in I think. My patch also uses _nosync for stop_rx.

// Simon

(Added the list to To: as well)

Well, I think serial21285_stop_tx also needs _nosync since it too is called from an interrupt handler:

static irqreturn_t serial21285_tx_chars(int irq, void *dev_id)

  {
	[...]
	if (uart_circ_empty(xmit))
		serial21285_stop_tx(port);

}

Of course, we could make an unsynced version of the stop_tx/stop_rx which is called from the local interrupt handlers, and a synchronous one which gets called from the structure.

I don't know if the function pointers in uart_ops would run into some problem with the _nosync versions? I haven't seen any problems during my tests here with them at least.

// Simon

I was worried about the regular UART ops wanting a synced version, so I made a nonsynced version specifically to be called from the interrupt handler. I am attaching that patch for reference.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100112/c904b24d/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: footbridge-serial-deadlock.patch
Type: application/octet-stream
Size: 776 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100112/c904b24d/attachment.obj>

I'm fine with your patch as well as long as the board doesn't hang at bootup :-).

Anyway, I belive mine was already applied to Russells git tree:

http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=5874/1

so I think you'd have to rebase your patch against that and submit it to the ARM patch system.

// Simon