new locking in change_termios breaks USB serial drivers

new locking in change_termios breaks USB serial drivers

[Al Borchers]

2.6.9-rc3 changes the locking in the tty_ioctl.c function
change_termios().  It gets a spin_lock_irqsave(&tty_termios_lock,...)
before calling the tty driver's set_termios function.

This means that the drivers' set_termios functions cannot sleep.

Unfortunately, many USB serial drivers' set_termios functions
send an urb to change the termios settings and sleep waiting for
it to complete.

I just looked quickly, but it seems belkin_sa.c, digi_acceleport.c,
ftdi_sio.c, io_ti.c, kl5usb105.c, mct_u232.c, pl2303.c, and whiteheat.c
all sleep in their set_termios functions.

If this locking in change_termios() stays, we are going to have to
fix set_termios in all of these drivers.  I am updating io_ti.c right
now.

-- Al

Re: new locking in change_termios breaks USB serial drivers

[Alan Cox]

On Gwe, 2004-10-01 at 11:40, Al Borchers wrote:
> Unfortunately, many USB serial drivers' set_termios functions
> send an urb to change the termios settings and sleep waiting for
> it to complete.
> 
> I just looked quickly, but it seems belkin_sa.c, digi_acceleport.c,
> ftdi_sio.c, io_ti.c, kl5usb105.c, mct_u232.c, pl2303.c, and whiteheat.c
> all sleep in their set_termios functions.
> 
> If this locking in change_termios() stays, we are going to have to
> fix set_termios in all of these drivers.  I am updating io_ti.c right
> now.

How much of a problem is this, would it make more sense to make the
termios locking also include a semaphore to serialize driver side events
and not the spin lock ?

We need some kind of locking there otherwise multiple parallel termios
setters resulting in truely strange occurences because driver authors
don't think about 64 parallel executions of ->change_termios()

I can switch the lock around if you want.

Alan

Re: new locking in change_termios breaks USB serial drivers

[Al Borchers]

Alan Cox wrote:
> How much of a problem is this, would it make more sense to make the
> termios locking also include a semaphore to serialize driver side events
> and not the spin lock ?

Its a design decision for the tty layer.  You should choose whatever is
best there and the drivers will have to adapt.

I don't know how many tty drivers have assumed that set_termios can sleep,
like the USB serial drivers have.  If that is an implicit part of tty API
that other drivers depend on, then, if possible, it seems much better to keep
the API the same and continue to allow set_termios to sleep.

I think the USB serial drivers can just queue up urbs to the device
with commands to set the termios settings and return without waiting
for those urbs to complete.  There are potential synchronization issues,
however.  The termios settings might go into different USB queues than
the data, and so it is possible that data sent immediately after a
set_termios might get to the device before the new termios settings.

To correctly support TCSETAW/TCSETSW the USB serial drivers would have to
have two different versions of set_termios--a non sleeping one to be called
through the tty API and a sleeping one to use with TCSETAW/TCSETSW ioctls
so the ioctl would not return until the settings were guaranteed to have
taken effect.  Not many USB serial drivers support TCSETAW/TCSETSW now.

-- Al

Re: new locking in change_termios breaks USB serial drivers

[Alan Cox]

On Gwe, 2004-10-01 at 17:24, Al Borchers wrote:
> Its a design decision for the tty layer.  You should choose whatever is
> best there and the drivers will have to adapt.

>From a tty layer I don't think there is a motivation to enforce no
sleep. Hopefully nobody has a reason to need to fiddle with termios
data in their IRQ handlers ?

> To correctly support TCSETAW/TCSETSW the USB serial drivers would have to
> have two different versions of set_termios--a non sleeping one to be called

Providing the driver isnt sticking its nose into ->ioctl the tty layer
core already correctly handles TCSETAW for you because it uses
tty_wait_until_sent before issuing the change. You don't have to deal
with that providing you've implemented driver->chars_in_buffer, and
if neccessary ->wait_until_sent.

In a waiting case the driver will get

	->chars_in_buffer
		until it returns zero
	->wait_until_sent
	->change_termios

which serializes with respect to the one writer. If you have a writer
during a termios change by another well tough luck, you lose and I've
no intention of changing that behaviour unless someone cites a standard
requiring it.

Re: [linux-usb-devel] Re: new locking in change_termios breaks USB serial drivers

[Al Borchers]

Alan Cox wrote:
> In a waiting case the driver will get
> 
> 	->chars_in_buffer
> 		until it returns zero
> 	->wait_until_sent
> 	->change_termios
>
> which serializes with respect to the one writer. If you have a writer
> during a termios change by another well tough luck, you lose and I've
> no intention of changing that behaviour unless someone cites a standard
> requiring it.

Right, of course.

My comments about TCSETAW just muddled the issue.  Scratch those.

The problem is that a non-sleeping USB serial set_termios has to
return before it can be sure the termios settings have taken effect.
With USB we can send an urb to the device telling it to change termios
settings, but without waiting for the urb callback we don't know
that the device has received the command and actually changed the
settings.

So data written immediately following the set_termios, in the same
process, might possibly be sent out the serial port before the
termios changes have taken effect.

There are several solutions:

* The tty layer could use a semaphore so the USB serial set_termios could
   sleep until the new termios settings have taken effect before returning.

* The USB serial driver could hold off sending data to the device after a
   non-sleeping set_termios until it knew the settings had taken effect in
   the device.

* Maybe in practice this is not a problem--we can just say "set_termios
   for USB serial devices will change the termios settings as soon as
   possible, but there is a slight chance data written immediately after
   set termios might go out under the previous termios settings".

* Or ... other suggestions?

-- Al

Re: [linux-usb-devel] Re: new locking in change_termios breaks USB serial drivers

[Alan Cox]

On Gwe, 2004-10-01 at 19:14, Al Borchers wrote:
> * The tty layer could use a semaphore so the USB serial set_termios could
>    sleep until the new termios settings have taken effect before returning.

This seems to be the right choice and is the change I will implement.

Re: [linux-usb-devel] Re: new locking in change_termios breaks USB serial drivers

[Al Borchers]

Alan Cox wrote:
> On Gwe, 2004-10-01 at 19:14, Al Borchers wrote:
>>* The tty layer could use a semaphore so the USB serial set_termios could
>>   sleep until the new termios settings have taken effect before returning.
> 
> This seems to be the right choice and is the change I will implement.

Great!  Thanks.

-- Al

RE: new locking in change_termios breaks USB serial drivers

[Stuart MacDonald]

From: linux-kernel-owner@vger.kernel.org 
> Its a design decision for the tty layer.  You should choose 
> whatever is
> best there and the drivers will have to adapt.

I agree.

..Stu

Re: new locking in change_termios breaks USB serial drivers

[Greg KH]

On Fri, Oct 01, 2004 at 12:36:09PM +0100, Alan Cox wrote:
> On Gwe, 2004-10-01 at 11:40, Al Borchers wrote:
> > Unfortunately, many USB serial drivers' set_termios functions
> > send an urb to change the termios settings and sleep waiting for
> > it to complete.
> > 
> > I just looked quickly, but it seems belkin_sa.c, digi_acceleport.c,
> > ftdi_sio.c, io_ti.c, kl5usb105.c, mct_u232.c, pl2303.c, and whiteheat.c
> > all sleep in their set_termios functions.
> > 
> > If this locking in change_termios() stays, we are going to have to
> > fix set_termios in all of these drivers.  I am updating io_ti.c right
> > now.
> 
> How much of a problem is this, would it make more sense to make the
> termios locking also include a semaphore to serialize driver side events
> and not the spin lock ?

It would make the usb-serial drivers much simpler if this was turned
into a semaphore.

> We need some kind of locking there otherwise multiple parallel termios
> setters resulting in truely strange occurences because driver authors
> don't think about 64 parallel executions of ->change_termios()
> 
> I can switch the lock around if you want.

I'm all for it, if the tty core isn't messing with any line settings
from interrupts.

thanks,

greg k-h