Re: [PATCH 8/11] usbserial: pl2303: Ports tty functions.

[Greg KH <gregkh@suse.de>]

On Tue, Jun 06, 2006 at 10:10:58AM -0300, Luiz Fernando N. Capitulino wrote:
> On Tue, 6 Jun 2006 00:34:41 -0700
> Greg KH <gregkh@suse.de> wrote:
> 
> | On Sat, Jun 03, 2006 at 07:19:17PM -0300, Luiz Fernando N. Capitulino wrote:
> | > On Fri, 2 Jun 2006 15:44:35 -0700
> | > Greg KH <gregkh@suse.de> wrote:
> | > 
> | > | On Fri, Jun 02, 2006 at 03:41:21PM -0700, Pete Zaitcev wrote:
> | > | > On Fri, 2 Jun 2006 13:50:14 -0700, Greg KH <gregkh@suse.de> wrote:
> | > | > > On Fri, Jun 02, 2006 at 12:03:14AM -0300, Luiz Fernando N.Capitulino wrote:
> | > | > 
> | > | > > >   2. The new pl2303's set_termios() can (still) sleep. Serial Core's
> | > | > > >      documentation says that that method must not sleep, but I couldn't find
> | > | > > >      where in the Serial Core code it's called in atomic context. So, is this
> | > | > > >      still true? Isn't the Serial Core's documentation out of date?
> | > | > > 
> | > | > > If this is true then we should just stop the port right now, as the USB
> | > | > > devices can not handle this.  They need to be able to sleep to
> | > | > > accomplish this functionality.
> | > | > > 
> | > | > > Russell, is this a requirement of the serial layer?  Why?
> | > | > 
> | > | > Shouldn't it be all right to schedule the change at the moment of
> | > | > that call and have it happen later? Resisting a temptation to abuse
> | > | > keventd and schedule_work and using a tasklet may help with latency
> | > | > enough to make this tolerable.
> | > | 
> | > | Some devices require more than one usb message to set all of the proper
> | > | termios bits in the device.  Creating a way to queue them up and fire
> | > | them off later, and handle errors if something happened in the middle,
> | > | after we told userspace the termios change succeeded, might get quite
> | > | messy :(
> | > 
> | >  But set_termios() returns nothing, and look what termios
> | > man page says about tcsetattr() return value:
> | > 
> | > """
> | > Note that tcsetattr() returns success if any of the requested changes could
> | > be successfully carried out. Therefore, when making multiple changes it may be
> | > necessary to follow this call with a further call to tcgetattr() to check that
> | > all changes have been performed successfully.
> | > """
> | 
> | Good point, I forgot about that.
> | 
> | >  Also, why do they need to sleep? Did you note that my version of
> | > set_mctrl() is atomic?
> | 
> | Yes, that looks "atomic" in a way, but when the function returns, the
> | value is not really set.  It only happens some time in the future when
> | the urb completes (and hopefully it works, no retry is allowed...)
> | 
> | So it might be a bit "racy" :)
> 
>  Oh, that's true.
> 
>  Is it acceptable?

I don't think so.

> The hardware is capable to queue URBs, right?

Yes it is.

thanks,

greg k-h