21285 serial driver deadlock(?) fix

21285 serial driver deadlock(?) fix

[Steve Moskovchenko]

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>

21285 serial driver deadlock(?) fix

[Russell King - ARM Linux]

On Mon, Dec 28, 2009 at 05:27:06PM -0500, Steve Moskovchenko wrote:
> 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.

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

> 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.

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.

21285 serial driver deadlock(?) fix

[Simon Kagstrom]

On Mon, 28 Dec 2009 17:27:06 -0500
Steve Moskovchenko <stevenm86@gmail.com> wrote:

> 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.

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

21285 serial driver deadlock(?) fix

[Simon Kagstrom]

(Added the list to To: as well)

On Tue, 5 Jan 2010 21:37:42 +0300
Steve M <stevenm86@gmail.com> wrote:

> I know little of the serial layer, but I am not certain that it is a good
> idea to put _nosync directly into the stop_tx function, since this is a
> function whose pointer is given out to the upper layer. We may only want the
> nosync behavior from within the ISR, but waiting for the ISR to complete in
> other cases may be more appropriate.

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

21285 serial driver deadlock(?) fix

[Steve M]

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.

On Mon, Jan 11, 2010 at 12:57 PM, Simon Kagstrom <
simon.kagstrom@netinsight.net> wrote:

> (Added the list to To: as well)
>
> On Tue, 5 Jan 2010 21:37:42 +0300
> Steve M <stevenm86@gmail.com> wrote:
>
> > I know little of the serial layer, but I am not certain that it is a good
> > idea to put _nosync directly into the stop_tx function, since this is a
> > function whose pointer is given out to the upper layer. We may only want
> the
> > nosync behavior from within the ISR, but waiting for the ISR to complete
> in
> > other cases may be more appropriate.
>
> 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
>
-------------- 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>

21285 serial driver deadlock(?) fix

[Simon Kagstrom]

On Tue, 12 Jan 2010 20:12:36 +0300
Steve M <stevenm86@gmail.com> wrote:

> 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.

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