Re: [PATCH RFC 0/11] usbserial: Serial Core port.

Re: [PATCH RFC 0/11] usbserial: Serial Core port.

[Pete Zaitcev]

On Fri, 02 Jun 2006 00:03:06 -0300, "Luiz Fernando N.Capitulino" <lcapitulino@mandriva.com.br> wrote:

This looks interesting, although I do not know if it buys us much.
The code seems sane at first view. The private lock inside pl2303
saves you from the most obvious races.

>  The tests I've done so far weren't anything serious: as the mobile supports a
> AT command set, I have used the ones (with minicom) which transfers more data.
> Of course that I also did module load/unload tests, tried to disconnect the
> device while it's transfering data and so on.

Next, it would be nice to test if PPP works, and if getty and shell work
(with getty driving the USB-to-serial adapter).

> +static void serial_send_xchar(struct uart_port *port, char ch)
> +{
> +	USBSERIAL_PORT->serial->type->uart_ops->send_xchar(port, ch);
>  }

I think you just inherited a mistake in usb-serial design. It attempts
to act as an adaptation layer (like, say, USB core itself) instead of
a library like libata. Why can't the UART framework call pl2303?

Also this meaningless obfuscation has to go:

> +#define USBSERIAL_PORT ((struct usb_serial_port *)port)
> +static void pl2303_start_tx(struct uart_port *port)
> +{
> +	struct usb_serial_port *usp = USBSERIAL_PORT;

Greetings,
-- Pete

Re: [PATCH RFC 0/11] usbserial: Serial Core port.

[David Woodhouse]

On Thu, 2006-06-01 at 23:48 -0700, Pete Zaitcev wrote:
> 
> >  The tests I've done so far weren't anything serious: as the mobile supports a
> > AT command set, I have used the ones (with minicom) which transfers more data.
> > Of course that I also did module load/unload tests, tried to disconnect the
> > device while it's transfering data and so on.
> 
> Next, it would be nice to test if PPP works, and if getty and shell work
> (with getty driving the USB-to-serial adapter).

xmodem is a good test -- better than PPP because it stresses the
buffering in a way which PPP won't. Log into a remote system, try
sending and receiving files with xmodem.

-- 
dwmw2

Re: [PATCH RFC 0/11] usbserial: Serial Core port.

[Luiz Fernando N. Capitulino]

On Fri, 02 Jun 2006 11:03:29 +0100
David Woodhouse <dwmw2@infradead.org> wrote:

| On Thu, 2006-06-01 at 23:48 -0700, Pete Zaitcev wrote:
| > 
| > >  The tests I've done so far weren't anything serious: as the mobile supports a
| > > AT command set, I have used the ones (with minicom) which transfers more data.
| > > Of course that I also did module load/unload tests, tried to disconnect the
| > > device while it's transfering data and so on.
| > 
| > Next, it would be nice to test if PPP works, and if getty and shell work
| > (with getty driving the USB-to-serial adapter).
| 
| xmodem is a good test -- better than PPP because it stresses the
| buffering in a way which PPP won't. Log into a remote system, try
| sending and receiving files with xmodem.

 Unfortunally my cables only connects devices to the computers
(ie, I can't connect two computers).

 Don't know if exists a Prolific USB <-> DB9, but if it does, would
be good if someone could make the test. Seems easy to do.

-- 
Luiz Fernando N. Capitulino

Re: [PATCH RFC 0/11] usbserial: Serial Core port.

[David Woodhouse]

On Fri, 2006-06-02 at 10:45 -0300, Luiz Fernando N. Capitulino wrote:
>  Unfortunally my cables only connects devices to the computers
> (ie, I can't connect two computers).
> 
>  Don't know if exists a Prolific USB <-> DB9, but if it does, would
> be good if someone could make the test. Seems easy to do. 

What manner of device does it connect? I'm sure I've seen GSM phones
which seem to be something like pl2302 when you connect to them with
USB... and with a phone, you can dial up to a remote system and use
xmodem.

You also get to test the flow control, since if you're using a 9600 baud
dialup connection and your 'serial' link is faster than that, it'll need
to be throttled.

-- 
dwmw2

Re: [PATCH RFC 0/11] usbserial: Serial Core port.

[Luiz Fernando N. Capitulino]

On Fri, 02 Jun 2006 14:54:06 +0100
David Woodhouse <dwmw2@infradead.org> wrote:

| On Fri, 2006-06-02 at 10:45 -0300, Luiz Fernando N. Capitulino wrote:
| >  Unfortunally my cables only connects devices to the computers
| > (ie, I can't connect two computers).
| > 
| >  Don't know if exists a Prolific USB <-> DB9, but if it does, would
| > be good if someone could make the test. Seems easy to do. 
| 
| What manner of device does it connect? I'm sure I've seen GSM phones
| which seem to be something like pl2302 when you connect to them with
| USB... and with a phone, you can dial up to a remote system and use
| xmodem.

 Oh, got it now. Yes, it's a GSM cell phone.

 Now I'm looking for a modem to dial for, but it will be easy to get.

| You also get to test the flow control, since if you're using a 9600 baud
| dialup connection and your 'serial' link is faster than that, it'll need
| to be throttled.

 Okay.

 Thanks a lot for the suggestions.

-- 
Luiz Fernando N. Capitulino

Re: [PATCH RFC 0/11] usbserial: Serial Core port.

[Pete Zaitcev]

On Fri, 02 Jun 2006 11:03:29 +0100, David Woodhouse <dwmw2@infradead.org> wrote:
> On Thu, 2006-06-01 at 23:48 -0700, Pete Zaitcev wrote:
> > 
> > >  The tests I've done so far weren't anything serious: as the mobile supports a
> > > AT command set, I have used the ones (with minicom) which transfers more data.
> > > Of course that I also did module load/unload tests, tried to disconnect the
> > > device while it's transfering data and so on.
> > 
> > Next, it would be nice to test if PPP works, and if getty and shell work
> > (with getty driving the USB-to-serial adapter).
> 
> xmodem is a good test -- better than PPP because it stresses the
> buffering in a way which PPP won't. Log into a remote system, try
> sending and receiving files with xmodem.

I understand. My intent was different, however. One of the bigger sticking
points for usb-serial was its interaction with line disciplines, which are
notorious for looping back and requesting writes from callbacks
(e.g. h_hdlc.c). They are also sensitive to drivers lying about the
amount of free space in their FIFOs. This is something you never test
when driving a serial port from an application, no matter how cleverly
written.

-- Pete

Re: [PATCH RFC 0/11] usbserial: Serial Core port.

[Luiz Fernando N. Capitulino]

On Fri, 2 Jun 2006 15:47:23 -0700
Pete Zaitcev <zaitcev@redhat.com> wrote:

| On Fri, 02 Jun 2006 11:03:29 +0100, David Woodhouse <dwmw2@infradead.org> wrote:
| > On Thu, 2006-06-01 at 23:48 -0700, Pete Zaitcev wrote:
| > > 
| > > >  The tests I've done so far weren't anything serious: as the mobile supports a
| > > > AT command set, I have used the ones (with minicom) which transfers more data.
| > > > Of course that I also did module load/unload tests, tried to disconnect the
| > > > device while it's transfering data and so on.
| > > 
| > > Next, it would be nice to test if PPP works, and if getty and shell work
| > > (with getty driving the USB-to-serial adapter).
| > 
| > xmodem is a good test -- better than PPP because it stresses the
| > buffering in a way which PPP won't. Log into a remote system, try
| > sending and receiving files with xmodem.
| 
| I understand. My intent was different, however. One of the bigger sticking
| points for usb-serial was its interaction with line disciplines, which are
| notorious for looping back and requesting writes from callbacks
| (e.g. h_hdlc.c). They are also sensitive to drivers lying about the
| amount of free space in their FIFOs. This is something you never test
| when driving a serial port from an application, no matter how cleverly
| written.

 Okay, I've made some of the tests suggested by you.

 I connected to my computer an USRobotics external modem (which uses the
standard serial driver) and the cell phone I have available for the tests
(which uses the pl2303 USB-Serial driver).

  In all the tests the modem was configured to answer the calls, and the
cell phone was configured to dial to the modem (my home's number).

  The reason for that setup is economic: my cell phone has some additional
credits (read free minutes) which I can use w/o spending money.

  Thus, we have something like this:

            Phone call from the cell phone
            /- <- - <- - - <- - - - <- - \
            |        ------------        |
            \-------o| computer |o-------/
  modem (phone line) ------------ GSM cell phone (pl2303)

   (I know, I know. I will never try to be an artist)

1. xmodem transfer test

 I transferred the following picture (48K):

   http://www.cpu.eti.br/pics/monkeys.jpg

 through minicom using the xmodem protocol with both USB-Serial versions: the
current one and (my) Serial Core port.

 Unfortunatally, I could only make one transfer for each USB-Serial version.

 Results:

   1.1 Current USB-Serial version

      o Received picture:
        http://www.cpu.eti.br/pics/usbserial-curr/monkeys.jpg

      o Got two TIMEOUT errors

      o md5 check didn't match

      o minicom's summary:

         20060604 12:20:19 Bytes Sent:  47488   BPS:161
         20060604 12:20:30 Disconnected (0:05:37)

  1.2 Serial Core USB-Serial version

      o Received picture:
        http://www.cpu.eti.br/pics/usbserial-sc/monkeys.jpg
 
      o Got seven TIMEOUT errors

      o md5 check didn't match

      o minicom's summary:

        20060604 12:08:34 Bytes Sent:  47488   BPS:113
	20060604 12:08:48 Disconnected (0:07:44)

 In both cases there is no visible problem in the picture and both md5 checks
failed. The only significant difference is the speed.

 However, note that this V0.0 of the port can be improved in many ways
(take a look at the pl2303's start_tx() method, the do-while() loop
isn't necessary, for example).

2. Remote login

 I've setup a remote serial console using mgetty. And, for both USB-Serial
versions, I could log, execute some commands and edit a file.

 I didn't notice any problem.

3. Conclusion

 IMHO, these tests can only be used to show that the USB-Serial Serial Core
port is feasible.

 Unfortunatally this is a very expensive test environment, and I can't use
it for development. The best one would be to have a USB<->DB9 cable..

PS: I didn't translate the picture message because I'm afraid I can't
make it as funny as it's in pt_BR.

-- 
Luiz Fernando N. Capitulino

Re: [PATCH RFC 0/11] usbserial: Serial Core port.

[Pete Zaitcev]

On Sun, 4 Jun 2006 20:12:23 -0300, "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:

> | I understand. My intent was different, however. One of the bigger sticking
> | points for usb-serial was its interaction with line disciplines, which are
> | notorious for looping back and requesting writes from callbacks
> | (e.g. h_hdlc.c). They are also sensitive to drivers lying about the
> | amount of free space in their FIFOs. This is something you never test
> | when driving a serial port from an application, no matter how cleverly
> | written.

>   In all the tests the modem was configured to answer the calls, and the
> cell phone was configured to dial to the modem (my home's number).

This is exactly backwards, and so it tests different code paths.
The line discipline is involved into driving a cooked mode port,
e.g. the one where getty is.

Running uploads and downloads with things like xmodem is a good
test of hardware flow control, so someone will have to do it too.

>  Unfortunatally this is a very expensive test environment, and I can't use
> it for development. The best one would be to have a USB<->DB9 cable..

PL-2303 already has a DB-9, you actually you need a DB-9-to-DB-9
Null Modem (cross-over) cable.

Anyway, I do not expect pl2303 failing this test, mind. It's more
of a problem for simpler devices.

-- Pete

Re: [PATCH RFC 0/11] usbserial: Serial Core port.

[Luiz Fernando N. Capitulino]

On Sun, 4 Jun 2006 16:24:53 -0700
Pete Zaitcev <zaitcev@redhat.com> wrote:

| On Sun, 4 Jun 2006 20:12:23 -0300, "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:
| 
| > | I understand. My intent was different, however. One of the bigger sticking
| > | points for usb-serial was its interaction with line disciplines, which are
| > | notorious for looping back and requesting writes from callbacks
| > | (e.g. h_hdlc.c). They are also sensitive to drivers lying about the
| > | amount of free space in their FIFOs. This is something you never test
| > | when driving a serial port from an application, no matter how cleverly
| > | written.
| 
| >   In all the tests the modem was configured to answer the calls, and the
| > cell phone was configured to dial to the modem (my home's number).
| 
| This is exactly backwards, and so it tests different code paths.
| The line discipline is involved into driving a cooked mode port,
| e.g. the one where getty is.

 I was going to try it last night and realized that my cell phone
can't answer data calls. :((

-- 
Luiz Fernando N. Capitulino

Re: [PATCH RFC 0/11] usbserial: Serial Core port.

[David Woodhouse]

On Mon, 2006-06-05 at 09:14 -0300, Luiz Fernando N. Capitulino wrote:
>  I was going to try it last night and realized that my cell phone
> can't answer data calls. :(( 

Often you have to contact your provider and ask them to give you a 'data
number', then you can answer calls to _that_ number.

Using v.110 instead of analogue data calls may also work, but you need
an ISDN landline to call it from for that (or another mobile).

-- 
dwmw2

Re: [PATCH RFC 0/11] usbserial: Serial Core port.

[Luiz Fernando N. Capitulino]

 Hi Pete,

On Thu, 1 Jun 2006 23:48:33 -0700
Pete Zaitcev <zaitcev@redhat.com> wrote:

| On Fri, 02 Jun 2006 00:03:06 -0300, "Luiz Fernando N.Capitulino" <lcapitulino@mandriva.com.br> wrote:
| 
| This looks interesting, although I do not know if it buys us much.

 Yeah, it will be a lot of work to port all the USB-Serial drivers to the
new interface. We have to discuss if it really pays off.

 IMHO, yes, it does. Currently, USB-Serial is duplicating code and solutions
from the Serial Core implementation. From the kernel POV, we're maintaining
two interfaces to solve (almost) the same problem.

 Killing one of the interfaces would make people concetrate in only one
of them.

| The code seems sane at first view. The private lock inside pl2303
| saves you from the most obvious races.

 But I wonder why I couldn't just use the Serial Core's spinlock for
that.

| >  The tests I've done so far weren't anything serious: as the mobile supports a
| > AT command set, I have used the ones (with minicom) which transfers more data.
| > Of course that I also did module load/unload tests, tried to disconnect the
| > device while it's transfering data and so on.
| 
| Next, it would be nice to test if PPP works, and if getty and shell work
| (with getty driving the USB-to-serial adapter).

 Hmmmm. I'll have to buy a new simcard for that (I can't use GPRS with the
one I have), then it will have to wait some days.

 Would be good to get some help here. :)

| > +static void serial_send_xchar(struct uart_port *port, char ch)
| > +{
| > +	USBSERIAL_PORT->serial->type->uart_ops->send_xchar(port, ch);
| >  }
| 
| I think you just inherited a mistake in usb-serial design. It attempts
| to act as an adaptation layer (like, say, USB core itself) instead of
| a library like libata. Why can't the UART framework call pl2303?

 Good point.

 In my first version of the port, I added additional code to handle
the multi-port thing. When it worked, I realized that that code wasn't
necessary: just registering each port with the Serial Core seems
enough.

 Then yes, I think we can do it.

 Will think more about that and try a new version in the weekend.

 Well, for some reason the patchset e-mails didn't reach LKML.
That's strange, because our replies are going there.

 Either, git-send-email has a bug or I did something wrong.

 Anyways, I just uploaded the patchset to:

http://distro2.conectiva.com.br/~lcapitulino/patches/usbserial/2.6.17-rc5/

-- 
Luiz Fernando N. Capitulino

Re: [PATCH RFC 0/11] usbserial: Serial Core port.

[Greg KH]

On Fri, Jun 02, 2006 at 12:03:06AM -0300, Luiz Fernando N.Capitulino wrote:
> 
>  Hi folks.
> 
>  This patch series is my first attempt to port the USB-Serial layer to the
> Serial Core API. Currently USB-Serial uses the TTY layer directly, duplicating
> code and solutions from the Serial Core implementation.
> 
>  The final (ported) USB-Serial code is simpler and cleaner. Now I'd like to know
> whether I'm doing it right or not.
> 
>  Note that this is a work in progress though. I've only ported the USB-Serial
> core and one of its drivers, the pl2303 one.
> 
>  Most of my questions and design decisions are adressed in the patches, please
> refer to them for details.

Nice first cut at this.  But please try to also convert 2 other drivers
at the same time to make sure that the model is right.  I'd suggest the
io_edgeport and the funsoft drivers.  io_edgeport because it is very
complex in that it doesn't share a single bulk in/out pair for every
port, but multiplexes them all through one pipe.  And funsoft because we
want to still be able to write usb-serial drivers that are this simple.

I agree with most of Pete's comments and like the general idea of moving
the usb-serial core to be more like libata (provider of helper
functions, not getting in the middle of everything).  I'll wait for your
next cut to provide specific code comments.

thanks,

greg k-h

Re: [PATCH RFC 0/11] usbserial: Serial Core port.

[Luiz Fernando N. Capitulino]

On Fri, 2 Jun 2006 13:48:39 -0700
Greg KH <gregkh@suse.de> wrote:

| On Fri, Jun 02, 2006 at 12:03:06AM -0300, Luiz Fernando N.Capitulino wrote:
| > 
| >  Hi folks.
| > 
| >  This patch series is my first attempt to port the USB-Serial layer to the
| > Serial Core API. Currently USB-Serial uses the TTY layer directly, duplicating
| > code and solutions from the Serial Core implementation.
| > 
| >  The final (ported) USB-Serial code is simpler and cleaner. Now I'd like to know
| > whether I'm doing it right or not.
| > 
| >  Note that this is a work in progress though. I've only ported the USB-Serial
| > core and one of its drivers, the pl2303 one.
| > 
| >  Most of my questions and design decisions are adressed in the patches, please
| > refer to them for details.
| 
| Nice first cut at this.  But please try to also convert 2 other drivers
| at the same time to make sure that the model is right.  I'd suggest the
| io_edgeport and the funsoft drivers.  io_edgeport because it is very
| complex in that it doesn't share a single bulk in/out pair for every
| port, but multiplexes them all through one pipe.  And funsoft because we
| want to still be able to write usb-serial drivers that are this simple.

 I'd love to do that but, unfortunatally, USB-Serial cables are too
expensive in Brazil (and I have no sure if I can find these ones in
Curitiba).

 The Prolific 2303 cables I have were lent by Mandriva.

 Couldn't someone send these cables for me in Brazil? I can send they
back when the port is finished.

| I agree with most of Pete's comments and like the general idea of moving
| the usb-serial core to be more like libata (provider of helper
| functions, not getting in the middle of everything).  I'll wait for your
| next cut to provide specific code comments.

 Hope to have something to show in the next days.

-- 
Luiz Fernando N. Capitulino

Re: [PATCH RFC 0/11] usbserial: Serial Core port.

[Greg KH]

On Sat, Jun 03, 2006 at 07:03:52PM -0300, Luiz Fernando N. Capitulino wrote:
> On Fri, 2 Jun 2006 13:48:39 -0700
> Greg KH <gregkh@suse.de> wrote:
> 
> | On Fri, Jun 02, 2006 at 12:03:06AM -0300, Luiz Fernando N.Capitulino wrote:
> | > 
> | >  Hi folks.
> | > 
> | >  This patch series is my first attempt to port the USB-Serial layer to the
> | > Serial Core API. Currently USB-Serial uses the TTY layer directly, duplicating
> | > code and solutions from the Serial Core implementation.
> | > 
> | >  The final (ported) USB-Serial code is simpler and cleaner. Now I'd like to know
> | > whether I'm doing it right or not.
> | > 
> | >  Note that this is a work in progress though. I've only ported the USB-Serial
> | > core and one of its drivers, the pl2303 one.
> | > 
> | >  Most of my questions and design decisions are adressed in the patches, please
> | > refer to them for details.
> | 
> | Nice first cut at this.  But please try to also convert 2 other drivers
> | at the same time to make sure that the model is right.  I'd suggest the
> | io_edgeport and the funsoft drivers.  io_edgeport because it is very
> | complex in that it doesn't share a single bulk in/out pair for every
> | port, but multiplexes them all through one pipe.  And funsoft because we
> | want to still be able to write usb-serial drivers that are this simple.
> 
>  I'd love to do that but, unfortunatally, USB-Serial cables are too
> expensive in Brazil (and I have no sure if I can find these ones in
> Curitiba).

No need to test fully, if it builds, I can test the io_edgeport driver,
and the funsoft one is pretty much a "nothing" driver.

thanks,

greg k-h

Re: [PATCH RFC 0/11] usbserial: Serial Core port.

[Luiz Fernando N. Capitulino]

(CC'ing Pete too).

On Tue, 6 Jun 2006 00:29:53 -0700
Greg KH <gregkh@suse.de> wrote:

| On Sat, Jun 03, 2006 at 07:03:52PM -0300, Luiz Fernando N. Capitulino wrote:
| > On Fri, 2 Jun 2006 13:48:39 -0700
| > Greg KH <gregkh@suse.de> wrote:
| > 
| > | On Fri, Jun 02, 2006 at 12:03:06AM -0300, Luiz Fernando N.Capitulino wrote:
| > | > 
| > | >  Hi folks.
| > | > 
| > | >  This patch series is my first attempt to port the USB-Serial layer to the
| > | > Serial Core API. Currently USB-Serial uses the TTY layer directly, duplicating
| > | > code and solutions from the Serial Core implementation.
| > | > 
| > | >  The final (ported) USB-Serial code is simpler and cleaner. Now I'd like to know
| > | > whether I'm doing it right or not.
| > | > 
| > | >  Note that this is a work in progress though. I've only ported the USB-Serial
| > | > core and one of its drivers, the pl2303 one.
| > | > 
| > | >  Most of my questions and design decisions are adressed in the patches, please
| > | > refer to them for details.
| > | 
| > | Nice first cut at this.  But please try to also convert 2 other drivers
| > | at the same time to make sure that the model is right.  I'd suggest the
| > | io_edgeport and the funsoft drivers.  io_edgeport because it is very
| > | complex in that it doesn't share a single bulk in/out pair for every
| > | port, but multiplexes them all through one pipe.  And funsoft because we
| > | want to still be able to write usb-serial drivers that are this simple.
| > 
| >  I'd love to do that but, unfortunatally, USB-Serial cables are too
| > expensive in Brazil (and I have no sure if I can find these ones in
| > Curitiba).
| 
| No need to test fully, if it builds, I can test the io_edgeport driver,
| and the funsoft one is pretty much a "nothing" driver.

 Okay.

 Mandriva just supplied an USB-Serial cable wich is handled by the
ftdio_sio driver. I'll port this one first, because it will allow us
to make the missing tests.

 BTW, I'll also postpone the 'libata-like' design change, because
it doesn't seem to as easy as it looked at first (note that that
design change will not impact ported drivers too much, since most of
its code are Serial Core's methods).

-- 
Luiz Fernando N. Capitulino

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

[Greg KH]

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?

thanks,

greg k-h

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

[Pete Zaitcev]

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.

I'm sure that a generic mechanism to drive asynchronous usb_control_msg
is going to be required as well for this project. The pl2303 is just
lucky to avoid it.

-- Pete

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

[Greg KH]

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 :(

thanks,

greg k-h

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

[Luiz Fernando N. Capitulino]

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

 Also, why do they need to sleep? Did you note that my version of
set_mctrl() is atomic?

-- 
Luiz Fernando N. Capitulino

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

[Greg KH]

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" :)

thanks,

greg k-h

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

[Alan Cox]

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

For the tty layer at least this was fixed to be semaphore locked and I
think this is now a docs error. It was fixed precisely because the USB
people couldn't resolve termios setup without sleeping.

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

[Luiz Fernando N. Capitulino]

On Tue, 06 Jun 2006 10:23:28 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> 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?
| 
| For the tty layer at least this was fixed to be semaphore locked and I
| think this is now a docs error. It was fixed precisely because the USB
| people couldn't resolve termios setup without sleeping.

 Russell, could you confirm if this is valid for the Serial Core
layer?

-- 
Luiz Fernando N. Capitulino

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

[Luiz Fernando N. Capitulino]

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? The hardware is capable to queue URBs, right?

-- 
Luiz Fernando N. Capitulino

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

[Greg KH]

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