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
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
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
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.
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
From: [email protected]
> 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