| Message ID | 20200929193327.GA13987@ls3530.fritz.box (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: serial: ftdi_sio: Fix serial port stall after resume | expand |
On Tue, Sep 29, 2020 at 09:33:27PM +0200, Helge Deller wrote: > With a 4-port serial USB HUB with FT232BM chips the serial ports stop > working after a software suspend/resume cycle. > Rewriting the latency timer during the resume phase fixes it. Hmm. This sounds weird. Why would the latency timer make such a difference? > Cc: stable@vger.kernel.org > Signed-off-by: Helge Deller <deller@gmx.de> > > diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c > index 9823bb424abd..8ee6cf6215c1 100644 > --- a/drivers/usb/serial/ftdi_sio.c > +++ b/drivers/usb/serial/ftdi_sio.c > @@ -1092,6 +1092,7 @@ static u32 ftdi_232bm_baud_base_to_divisor(int baud, int base); > static u32 ftdi_232bm_baud_to_divisor(int baud); > static u32 ftdi_2232h_baud_base_to_divisor(int baud, int base); > static u32 ftdi_2232h_baud_to_divisor(int baud); > +static int ftdi_reset_resume(struct usb_serial *serial); > > static struct usb_serial_driver ftdi_sio_device = { > .driver = { > @@ -1122,6 +1123,7 @@ static struct usb_serial_driver ftdi_sio_device = { > .set_termios = ftdi_set_termios, > .break_ctl = ftdi_break_ctl, > .tx_empty = ftdi_tx_empty, > + .reset_resume = ftdi_reset_resume, > }; > > static struct usb_serial_driver * const serial_drivers[] = { > @@ -2379,6 +2381,16 @@ static int ftdi_stmclite_probe(struct usb_serial *serial) > return 0; > } > > +static int ftdi_reset_resume(struct usb_serial *serial) > +{ > + struct usb_serial_port *port = serial->port[0]; > + > + if (tty_port_initialized(&port->port)) > + write_latency_timer(port); Why are you only doing this for open ports? > + > + return usb_serial_generic_resume(serial); > +} And if the device has been reset there may need to reconfigured the termios settings for open ports. Could you expand a bit on what the problem is here? Johan
On 10/8/20 5:21 PM, Johan Hovold wrote: > On Tue, Sep 29, 2020 at 09:33:27PM +0200, Helge Deller wrote: >> With a 4-port serial USB HUB with FT232BM chips the serial ports stop >> working after a software suspend/resume cycle. >> Rewriting the latency timer during the resume phase fixes it. > > Hmm. This sounds weird. Why would the latency timer make such a > difference? Actually, I don't know. My assumption is, that by setting the latency timer, the USB transmissions somehow starts again. Maybe the integrated HUB is involved as well? I'm no expert on this driver, the only thing I did is to compare what is being configured when the device is detected and initialized and when what happens when it's waked up. Setting the latency timer seemed to me to be the only difference, and it actually worked then. But I agree, it seems weird. If you have other ideas, I'm happy to test. >> Cc: stable@vger.kernel.org >> Signed-off-by: Helge Deller <deller@gmx.de> >> >> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c >> index 9823bb424abd..8ee6cf6215c1 100644 >> --- a/drivers/usb/serial/ftdi_sio.c >> +++ b/drivers/usb/serial/ftdi_sio.c >> @@ -1092,6 +1092,7 @@ static u32 ftdi_232bm_baud_base_to_divisor(int baud, int base); >> static u32 ftdi_232bm_baud_to_divisor(int baud); >> static u32 ftdi_2232h_baud_base_to_divisor(int baud, int base); >> static u32 ftdi_2232h_baud_to_divisor(int baud); >> +static int ftdi_reset_resume(struct usb_serial *serial); >> >> static struct usb_serial_driver ftdi_sio_device = { >> .driver = { >> @@ -1122,6 +1123,7 @@ static struct usb_serial_driver ftdi_sio_device = { >> .set_termios = ftdi_set_termios, >> .break_ctl = ftdi_break_ctl, >> .tx_empty = ftdi_tx_empty, >> + .reset_resume = ftdi_reset_resume, >> }; >> >> static struct usb_serial_driver * const serial_drivers[] = { >> @@ -2379,6 +2381,16 @@ static int ftdi_stmclite_probe(struct usb_serial *serial) >> return 0; >> } >> >> +static int ftdi_reset_resume(struct usb_serial *serial) >> +{ >> + struct usb_serial_port *port = serial->port[0]; >> + >> + if (tty_port_initialized(&port->port)) >> + write_latency_timer(port); > > Why are you only doing this for open ports? I more or less copied it from another driver.... >> + >> + return usb_serial_generic_resume(serial); >> +} > > And if the device has been reset there may need to reconfigured the > termios settings for open ports. > > Could you expand a bit on what the problem is here? My testcase is pretty simple: 1. I use e.g. "minicom -D /dev/ttyUSB2". Serial connection works. 2. I exit minicom. 3. I suspend the workstation: "systemctl suspend" 4. I wake up the machine and wait a few seconds. 5. I start "minicom -D /dev/ttyUSB2" again. No transfers on the serial port. With my patch the minicom serial communications does work. Another way to wake up the connection is to rmmod the driver and insmod it again. Helge
On Thu, Oct 08, 2020 at 08:16:02PM +0200, Helge Deller wrote: > On 10/8/20 5:21 PM, Johan Hovold wrote: > > On Tue, Sep 29, 2020 at 09:33:27PM +0200, Helge Deller wrote: > >> With a 4-port serial USB HUB with FT232BM chips the serial ports stop > >> working after a software suspend/resume cycle. > >> Rewriting the latency timer during the resume phase fixes it. > >> +static int ftdi_reset_resume(struct usb_serial *serial) > >> +{ > >> + struct usb_serial_port *port = serial->port[0]; > >> + > >> + if (tty_port_initialized(&port->port)) > >> + write_latency_timer(port); > > > > Why are you only doing this for open ports? > > I more or less copied it from another driver.... > > >> + > >> + return usb_serial_generic_resume(serial); > >> +} > > > > And if the device has been reset there may need to reconfigured the > > termios settings for open ports. > > > > Could you expand a bit on what the problem is here? > > My testcase is pretty simple: > 1. I use e.g. "minicom -D /dev/ttyUSB2". Serial connection works. > 2. I exit minicom. > 3. I suspend the workstation: "systemctl suspend" > 4. I wake up the machine and wait a few seconds. > 5. I start "minicom -D /dev/ttyUSB2" again. No transfers on the serial port. > > With my patch the minicom serial communications does work. > Another way to wake up the connection is to rmmod the driver and > insmod it again. Weird indeed. If you exit minicom before suspend and no other process is keeping the port open, then that write_latency_timer() above would never be executed. Could you enable some debugging and provide a dmesg log from a test cycle (open/close minicom, suspend/resume, open minicom)? echo file usb-serial.c +p > /sys/kernel/debug/dynamic_debug/control Johan
On 10/27/20 10:00 AM, Johan Hovold wrote: > On Thu, Oct 08, 2020 at 08:16:02PM +0200, Helge Deller wrote: >> On 10/8/20 5:21 PM, Johan Hovold wrote: >>> On Tue, Sep 29, 2020 at 09:33:27PM +0200, Helge Deller wrote: >>>> With a 4-port serial USB HUB with FT232BM chips the serial ports stop >>>> working after a software suspend/resume cycle. >>>> Rewriting the latency timer during the resume phase fixes it. > >>>> +static int ftdi_reset_resume(struct usb_serial *serial) >>>> +{ >>>> + struct usb_serial_port *port = serial->port[0]; >>>> + >>>> + if (tty_port_initialized(&port->port)) >>>> + write_latency_timer(port); >>> >>> Why are you only doing this for open ports? >> >> I more or less copied it from another driver.... >> >>>> + >>>> + return usb_serial_generic_resume(serial); >>>> +} >>> >>> And if the device has been reset there may need to reconfigured the >>> termios settings for open ports. >>> >>> Could you expand a bit on what the problem is here? >> >> My testcase is pretty simple: >> 1. I use e.g. "minicom -D /dev/ttyUSB2". Serial connection works. >> 2. I exit minicom. >> 3. I suspend the workstation: "systemctl suspend" >> 4. I wake up the machine and wait a few seconds. >> 5. I start "minicom -D /dev/ttyUSB2" again. No transfers on the serial port. >> >> With my patch the minicom serial communications does work. >> Another way to wake up the connection is to rmmod the driver and >> insmod it again. > > Weird indeed. If you exit minicom before suspend and no other process is > keeping the port open, then that write_latency_timer() above would never > be executed. > > Could you enable some debugging and provide a dmesg log from a test > cycle (open/close minicom, suspend/resume, open minicom)? > > echo file usb-serial.c +p > /sys/kernel/debug/dynamic_debug/control I enabled the debugging and tried a few times, but somehow I can not reproduce the issue any longer. So, please drop my patch for now. Thanks! Helge
On Wed, Oct 28, 2020 at 03:54:35PM +0100, Helge Deller wrote: > On 10/27/20 10:00 AM, Johan Hovold wrote: > > On Thu, Oct 08, 2020 at 08:16:02PM +0200, Helge Deller wrote: > >> My testcase is pretty simple: > >> 1. I use e.g. "minicom -D /dev/ttyUSB2". Serial connection works. > >> 2. I exit minicom. > >> 3. I suspend the workstation: "systemctl suspend" > >> 4. I wake up the machine and wait a few seconds. > >> 5. I start "minicom -D /dev/ttyUSB2" again. No transfers on the serial port. > >> > >> With my patch the minicom serial communications does work. > >> Another way to wake up the connection is to rmmod the driver and > >> insmod it again. > > > > Weird indeed. If you exit minicom before suspend and no other process is > > keeping the port open, then that write_latency_timer() above would never > > be executed. > > > > Could you enable some debugging and provide a dmesg log from a test > > cycle (open/close minicom, suspend/resume, open minicom)? > > > > echo file usb-serial.c +p > /sys/kernel/debug/dynamic_debug/control > > I enabled the debugging and tried a few times, but somehow I can not > reproduce the issue any longer. > > So, please drop my patch for now. Good to hear the problem's gone. :) Let us know if you run into it again. Johan
diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c index 9823bb424abd..8ee6cf6215c1 100644 --- a/drivers/usb/serial/ftdi_sio.c +++ b/drivers/usb/serial/ftdi_sio.c @@ -1092,6 +1092,7 @@ static u32 ftdi_232bm_baud_base_to_divisor(int baud, int base); static u32 ftdi_232bm_baud_to_divisor(int baud); static u32 ftdi_2232h_baud_base_to_divisor(int baud, int base); static u32 ftdi_2232h_baud_to_divisor(int baud); +static int ftdi_reset_resume(struct usb_serial *serial); static struct usb_serial_driver ftdi_sio_device = { .driver = { @@ -1122,6 +1123,7 @@ static struct usb_serial_driver ftdi_sio_device = { .set_termios = ftdi_set_termios, .break_ctl = ftdi_break_ctl, .tx_empty = ftdi_tx_empty, + .reset_resume = ftdi_reset_resume, }; static struct usb_serial_driver * const serial_drivers[] = { @@ -2379,6 +2381,16 @@ static int ftdi_stmclite_probe(struct usb_serial *serial) return 0; } +static int ftdi_reset_resume(struct usb_serial *serial) +{ + struct usb_serial_port *port = serial->port[0]; + + if (tty_port_initialized(&port->port)) + write_latency_timer(port); + + return usb_serial_generic_resume(serial); +} + static int ftdi_sio_port_remove(struct usb_serial_port *port) { struct ftdi_private *priv = usb_get_serial_port_data(port);
With a 4-port serial USB HUB with FT232BM chips the serial ports stop working after a software suspend/resume cycle. Rewriting the latency timer during the resume phase fixes it. Cc: stable@vger.kernel.org Signed-off-by: Helge Deller <deller@gmx.de>