calling context when writing to tty_driver

calling context when writing to tty_driver

[Martin Diehl]

Hi,

just hitting another "sleeping on semaphore from illegal context" issue
with 2.5.39. Happened on down() in either usbserial->write_room() or
usbserial->write(), when invoked from bh context.

Some grepping reveals no documentation of calling context requirements
for those driver calls and existing serial code seems to be happy with bh
context. Therefore I'm wondering whether it is permitted to call from
don't-sleep context?

Since write_room() is usually called immediately before write()'ing stuff
to the driver it would be a good idea to keep them both callable from bh,
IMHO. For example tty_ldisc->write_wakeup() might probably want to issue
write_room() followed by write().

Currently, usbserial calls write_wakeup() from bh (on OUT urb completion)
but needs process context for write_room() and write(). My impression is
the whole point of write_room() is to find out how many data can be
accepted by the write() - if write() would be allowed to sleep it could
just block to deal with any amount of data.

TIA for any insight.

Martin

Re: calling context when writing to tty_driver

[David McIlwraith]

Semaphores may sleep - therefore, they cannot be used from a 'non-sleep'
context.

Sincerely,
David McIlwraith quack@bigpond.net.au

----- Original Message -----
From: "Martin Diehl" <lists@mdiehl.de>
To: <linux-kernel@vger.kernel.org>
Cc: "Greg KH" <greg@kroah.com>
Sent: Tuesday, October 01, 2002 8:37 PM
Subject: calling context when writing to tty_driver


>
> Hi,
>
> just hitting another "sleeping on semaphore from illegal context" issue
> with 2.5.39. Happened on down() in either usbserial->write_room() or
> usbserial->write(), when invoked from bh context.
>
> Some grepping reveals no documentation of calling context requirements
> for those driver calls and existing serial code seems to be happy with bh
> context. Therefore I'm wondering whether it is permitted to call from
> don't-sleep context?
>
> Since write_room() is usually called immediately before write()'ing stuff
> to the driver it would be a good idea to keep them both callable from bh,
> IMHO. For example tty_ldisc->write_wakeup() might probably want to issue
> write_room() followed by write().
>
> Currently, usbserial calls write_wakeup() from bh (on OUT urb completion)
> but needs process context for write_room() and write(). My impression is
> the whole point of write_room() is to find out how many data can be
> accepted by the write() - if write() would be allowed to sleep it could
> just block to deal with any amount of data.
>
> TIA for any insight.
>
> Martin
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: calling context when writing to tty_driver

[Martin Diehl]

On Tue, 1 Oct 2002, David McIlwraith wrote:

> Semaphores may sleep - therefore, they cannot be used from a 'non-sleep'
> context.

Yes, sure. Sorry if I wasn't clear enough - the point is whether those
tty_driver write/write_room() calls are allowed to sleep or not. If yes,
the usbserial implementation is right and it is impossible to do further
writing directly from write_wakeup() callback (which would be really bad
IMHO) - if not, usbserial needs to avoid the down() somehow.

Martin

Re: calling context when writing to tty_driver

[David McIlwraith]

Spinlocks *could* be used in place, if this is the case. Having not examined
the code, I don't know the implementation specifics.

----- Original Message -----
From: "Martin Diehl" <lists@mdiehl.de>
To: "David McIlwraith" <quack@bigpond.net.au>
Cc: "Martin Diehl" <lists@mdiehl.de>; <linux-kernel@vger.kernel.org>
Sent: Tuesday, October 01, 2002 9:28 PM
Subject: Re: calling context when writing to tty_driver


> On Tue, 1 Oct 2002, David McIlwraith wrote:
>
> > Semaphores may sleep - therefore, they cannot be used from a 'non-sleep'
> > context.
>
> Yes, sure. Sorry if I wasn't clear enough - the point is whether those
> tty_driver write/write_room() calls are allowed to sleep or not. If yes,
> the usbserial implementation is right and it is impossible to do further
> writing directly from write_wakeup() callback (which would be really bad
> IMHO) - if not, usbserial needs to avoid the down() somehow.
>
> Martin
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: calling context when writing to tty_driver

[Greg KH]

On Tue, Oct 01, 2002 at 12:37:42PM +0200, Martin Diehl wrote:
> 
> Hi,
> 
> just hitting another "sleeping on semaphore from illegal context" issue
> with 2.5.39. Happened on down() in either usbserial->write_room() or
> usbserial->write(), when invoked from bh context.

Can you send me the whole backtrace?  I'm curious as to who is calling
those functions from bh context.

> Some grepping reveals no documentation of calling context requirements
> for those driver calls and existing serial code seems to be happy with bh
> context. Therefore I'm wondering whether it is permitted to call from
> don't-sleep context?

I don't know.

> Since write_room() is usually called immediately before write()'ing stuff
> to the driver it would be a good idea to keep them both callable from bh,
> IMHO. For example tty_ldisc->write_wakeup() might probably want to issue
> write_room() followed by write().
> 
> Currently, usbserial calls write_wakeup() from bh (on OUT urb completion)
> but needs process context for write_room() and write(). My impression is
> the whole point of write_room() is to find out how many data can be
> accepted by the write() - if write() would be allowed to sleep it could
> just block to deal with any amount of data.

Making write() block for any amount of data would increase the
complexity of the drivers.  What should probably be done is convert the
usb-serial drivers to use the new serial core, but I don't have the time
to do this work right now.  Any takers?

thanks,

greg k-h

Re: calling context when writing to tty_driver

[Russell King]

On Tue, Oct 01, 2002 at 11:34:00AM -0700, Greg KH wrote:
> Making write() block for any amount of data would increase the
> complexity of the drivers.  What should probably be done is convert the
> usb-serial drivers to use the new serial core, but I don't have the time
> to do this work right now.  Any takers?

It needs some changes to the serial core first.  I'm in 2.5 catchup mode
at the moment, I've also got stuff outstanding for 2.4.19 at present, so
I've _no_ idea when I'll be able to do the necessary changes on my side.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: calling context when writing to tty_driver

[Martin Diehl]

On Tue, 1 Oct 2002, Greg KH wrote:

> > just hitting another "sleeping on semaphore from illegal context" issue
> > with 2.5.39. Happened on down() in either usbserial->write_room() or
> > usbserial->write(), when invoked from bh context.
> 
> Can you send me the whole backtrace?  I'm curious as to who is calling
> those functions from bh context.

It's from an intermediate state of my attempt to rewrite the irda ldisc
(irtty) to make it working with the new serial driver and usbserial as
well. It's all working with serial (uart) which doesn't sleep neither in
write_room() nor write() - except when called from userland.

With usbserial however port->sem is acquired in both cases. It gets
invoked from non-sleep context in two cases:

1) start of transmission (frame comes from network layer):

	netdev->hard_start_xmit() - under xmit-lock
		- takes some spinlock_bh to serialize with write_wakeup()
		- calls tty_driver->write_room() and tty_driver->write()

2) sending more data after serial driver has transmitted some bytes:

	tty_driver in bh context (urb-complete for usbserial)
	calls ldisc->write_wakeup()
		which uses tty_driver->write_room() / write()
		to send the next few bytes from the frame

The backtrace below is from case 1. In both cases it would be extremely
painful to defer all the real work to process context - then we could as
well nuke write_wakeup() and write_room() and make tty_driver->write()
just blocking.

Agreed, there aren't many users of write_wakeup but it seems they all rely
on case 2 above (at least): ppp, n_hdlc, slip, x25, input/serio to give a
few examples. Well, they are probably always used on top of serial without
problem - but I'd expect the same issue might already hit bluetooth (i.e.
hci_ldisc) when running over usbserial.

> > Currently, usbserial calls write_wakeup() from bh (on OUT urb completion)
> > but needs process context for write_room() and write(). My impression is
> > the whole point of write_room() is to find out how many data can be
> > accepted by the write() - if write() would be allowed to sleep it could
> > just block to deal with any amount of data.
> 
> Making write() block for any amount of data would increase the
> complexity of the drivers.  What should probably be done is convert the
> usb-serial drivers to use the new serial core, but I don't have the time
> to do this work right now.  Any takers?

Given the above mentioned observations with other write_wakeup() users I
tend to assume my approach to call write_room()/write() from non-sleep
context couldn't be that wrong. And it works with existing serial driver
(old and new one).

Another question/suggestion: do we need to acquire port->sem in usbserial?
Couldn't this be done with a spinlock - at least when called from_user?
If we agree serial drivers shouldn't sleep in write_room()/write() my
impression is this needs to be addressed somehow, regardless whether
usbserial uses the new serial core or not. Anybody tried this with a
bluetooth dongle over usbserial?

Martin

----------------------------------------

backtrace from 2.5.39:

 Sleeping function called from illegal context at /mnt/disk/kernel/v2.5.39-md/include/asm/semaphore.h
 c02bbc94 c011aa62 c024f160 cc8d0780 00000077 ffffffea cc8ce500 cc8d0780 
        00000077 c55c4000 c95f3c00 c02bbd58 cbcf42e4 cc8c60b7 c55c4000 00000000 
        c5d43000 0000001c c95f3c00 c95f3c00 cbcf42e4 cc8c1377 c95f3c00 c5d43000 
 Call Trace
  [__might_sleep+66/71]__might_sleep+0x42/0x47
  [<c011aa62>]__might_sleep+0x42/0x47
  [<cc8d0780>].LC4+0x0/0x100 [usbserial]
  [<cc8ce500>]serial_write+0x80/0x130 [usbserial]
  [<cc8d0780>].LC4+0x0/0x100 [usbserial]
  [<cc8c60b7>]irtty_do_write+0x57/0x60 [sir_tty]
  [<cc8c1377>]sirdev_do_write+0x67/0x88 [irda_sir]
  [<cc8c193b>]sirdev_hard_xmit+0x25f/0x2f0 [irda_sir]
  [__wake_up_common+47/80]__wake_up_common+0x2f/0x50
  [<c0118adf>]__wake_up_common+0x2f/0x50
  [default_wake_function+33/64]default_wake_function+0x21/0x40
  [<c0118a91>]default_wake_function+0x21/0x40
  [qdisc_restart+160/608]qdisc_restart+0xa0/0x260
  [<c0209080>]qdisc_restart+0xa0/0x260
  [dev_queue_xmit+270/1088]dev_queue_xmit+0x10e/0x440
  [<c0200aae>]dev_queue_xmit+0x10e/0x440
  [alloc_skb+232/480]alloc_skb+0xe8/0x1e0
  [<c01fd348>]alloc_skb+0xe8/0x1e0
  [<cc899a02>]irlap_send_discovery_xid_frame+0x312/0x320 [irda]

Re: calling context when writing to tty_driver

[Paul Mackerras]

Greg KH writes:

> On Tue, Oct 01, 2002 at 12:37:42PM +0200, Martin Diehl wrote:
> > 
> > Hi,
> > 
> > just hitting another "sleeping on semaphore from illegal context" issue
> > with 2.5.39. Happened on down() in either usbserial->write_room() or
> > usbserial->write(), when invoked from bh context.
> 
> Can you send me the whole backtrace?  I'm curious as to who is calling
> those functions from bh context.

PPP will do that.

Paul.

Re: calling context when writing to tty_driver

[Greg KH]

On Tue, Oct 01, 2002 at 11:10:34PM +0200, Martin Diehl wrote:
> 
> Another question/suggestion: do we need to acquire port->sem in usbserial?
> Couldn't this be done with a spinlock - at least when called from_user?

It used to be a spinlock, but too many drivers did bad things with the
spinlock held, so I changed it to a semaphore so they could sleep while
it is held.  I think in 2.5, all of the nasty drivers can be easily
fixed (the usb core now can be told not to sleep when submitting an
urb), so this might be able to be changed back to a spinlock.

> If we agree serial drivers shouldn't sleep in write_room()/write() my
> impression is this needs to be addressed somehow, regardless whether
> usbserial uses the new serial core or not. Anybody tried this with a
> bluetooth dongle over usbserial?

I don't know, do we agree that you can't sleep in those functions?  If
so, I'll look into fixing the usbserial drivers up.

thanks,

greg k-h

Re: calling context when writing to tty_driver

[Paul Mackerras]

Greg KH writes:

> On Tue, Oct 01, 2002 at 11:10:34PM +0200, Martin Diehl wrote:
> > If we agree serial drivers shouldn't sleep in write_room()/write() my
> > impression is this needs to be addressed somehow, regardless whether
> > usbserial uses the new serial core or not. Anybody tried this with a
> > bluetooth dongle over usbserial?
> 
> I don't know, do we agree that you can't sleep in those functions?  If
> so, I'll look into fixing the usbserial drivers up.

I really think that write and write_room shouldn't be allowed to
sleep.  If they can sleep it will cause much grief for PPP, since the
PPP start_xmit function does get called in softirq context, and in the
common case where you are doing PPP over a serial port, that will
ultimately end up in a call to the serial port's write routine.  If we
can't call the write routine from softirq context, that will mean we
will have to have some sort of helper thread (whether that is keventd
or something else), and that will introduce extra unnecessary latency
when you are using a serial port where the write routine doesn't ever
block (which is all of them except usbserial, at the moment).

I would have thought that the normal tty line discipline would impose
the same requirement.  You type a character, it gets put in a flip
buffer and processed later in softirq context.  That processing says
"we're supposed to echo this character" so it will call the serial
port's write routine - in softirq context, if I am not mistaken.

Regards,
Paul.

Re: calling context when writing to tty_driver

[Martin Diehl]

On Thu, 3 Oct 2002, Paul Mackerras wrote:

> > > If we agree serial drivers shouldn't sleep in write_room()/write() my
> > > impression is this needs to be addressed somehow, regardless whether
> > > usbserial uses the new serial core or not. Anybody tried this with a
> > > bluetooth dongle over usbserial?
> > 
> > I don't know, do we agree that you can't sleep in those functions?  If
> > so, I'll look into fixing the usbserial drivers up.
> 
> I really think that write and write_room shouldn't be allowed to
> sleep.  If they can sleep it will cause much grief for PPP, since the
> PPP start_xmit function does get called in softirq context, and in the
> common case where you are doing PPP over a serial port, that will
> ultimately end up in a call to the serial port's write routine.  If we
> can't call the write routine from softirq context, that will mean we

... would see tons (basically 1+ per frame) of this:

usb.c: registered new driver serial
usbserial.c: USB Serial support registered for Generic
usbserial.c: USB Serial Driver core v1.6
usbserial.c: USB Serial support registered for PL-2303
usbserial.c: PL-2303 converter detected
usbserial.c: PL-2303 converter now attached to ttyUSB0 (or usb/tts/0 for devfs)
usb.c: registered new driver pl2303
pl2303.c: Prolific PL2303 USB to serial adaptor driver v0.9
CSLIP: code copyright 1989 Regents of the University of California
PPP generic driver version 2.4.2
Sleeping function called from illegal context at
/mnt/disk/kernel/v2.5.39-md/include/asm/semaphore.h:119
c72afe8c c011aa62 c024f160 cc88f780 00000077 ffffffea cc88d500 cc88f780 
       00000077 0000002d c921b000 00000000 c834a000 cc8a4e3c c834a000 00000000 
       c921b0ac 0000002d 00000001 00000060 00000246 cbfbdd04 000000c0 c921b000 
Call Trace:
 [<c011aa62>]__might_sleep+0x42/0x47
 [<cc88f780>].LC4+0x0/0x100 [usbserial]
 [<cc88d500>]serial_write+0x80/0x130 [usbserial]
 [<cc88f780>].LC4+0x0/0x100 [usbserial]
 [<cc8a4e3c>]ppp_async_push+0xcc/0x270 [ppp_async]
 [<cc8a4d59>]ppp_async_send+0x39/0x50 [ppp_async]
 [<cc89dd55>]ppp_channel_push+0x105/0x390 [ppp_generic]
 [<c01fd348>]alloc_skb+0xe8/0x1e0
 [<cc89c42f>]ppp_write+0x16f/0x180 [ppp_generic]
 [<c014b277>]vfs_write+0xb7/0x140
 [<c014b368>]sys_write+0x28/0x40
 [<c01077d7>]syscall_call+0x7/0xb

My initial question for the irda ldisc was basically to find out whether 
the assumption about write/write_room should not sleep was reasonable.
Looks like I'd better stay with this approach for now. The hope is it will 
continue to work with serial - and with usbserial probably later ;-)

Martin