| Message ID | 20220419065408.2461091-1-xy521521@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND,-next] USB: serial: pl2303: implement reset_resume member | expand |
Hi greg, On 2022/4/22 00:45, Greg KH wrote: > On Tue, Apr 19, 2022 at 02:54:08PM +0800, Hongyu Xie wrote: >> From: Hongyu Xie <xiehongyu1@kylinos.cn> >> >> pl2303.c doesn't have reset_resume for hibernation. >> So needs_binding will be set to 1 duiring hibernation. >> usb_forced_unbind_intf will be called, and the port minor >> will be released (x in ttyUSBx). > > Please use the full 72 columns that you are allowed in a changelog text. > > >> It works fine if you have only one USB-to-serial device. >> Assume you have 2 USB-to-serial device, nameing A and B. >> A gets a smaller minor(ttyUSB0), B gets a bigger one. >> And start to hibernate. When your PC is in hibernation, >> unplug device A. Then wake up your PC by pressing the >> power button. After waking up the whole system, device >> B gets ttyUSB0. This will casuse a problem if you were >> using those to ports(like opened two minicom process) >> before hibernation. >> So member reset_resume is needed in usb_serial_driver >> pl2303_device. > > If you want persistent device naming, use the symlinks that udev creates > for your for all your serial devices. Never rely on the number of a USB > to serial device. Let me put it this way. Assume you need to record messages output from two machines using 2 USB-to-serial devices(naming A and B, and A is on USB1-port3, B is on USB1-port4) opened by two minicom process. The setting for A in minicom would be like: "A - Serial Device : /dev/ttyUSB0" The setting for B in minicom would be like: "A - Serial Device : /dev/ttyUSB1" Then start to hibernate on your computer. When your PC is in hibernation, unplug A. After waking up your computer, "/dev/ttyUSB0" would be released first, then allocated to B. The minicom process used to record outputs from A is now recording B's outputs. The minicom process used to record outputs from B is now recording nothing, because "/dev/ttyUSB1" is not exist anymore. That's the problem I've been talking about. And I don't think using symlinks will solve this problem. > >> Codes in pl2303_reset_resume are borrowed from pl2303_open. >> >> As a matter of fact, all driver under drivers/usb/serial >> has the same problem except ch341.c. >> >> Cc: stable@vger.kernel.org > > How does this meet the stable kernel rule requirements? It would be a > new feature if it were accepted, right? It's not a new feature at all. struct usb_serial_driver already has a member name reset_resume, there is no implementation in pl2303.c yet. And ch341.c has one(ch341_reset_resume()), that why I said "all driver under drivers/usb/serial has the same problem except ch341.c" > > > thanks, > > greg k-h
On Fri, Apr 22, 2022 at 10:35:59AM +0800, Hongyu Xie wrote: > > Hi greg, > On 2022/4/22 00:45, Greg KH wrote: > > On Tue, Apr 19, 2022 at 02:54:08PM +0800, Hongyu Xie wrote: > > > From: Hongyu Xie <xiehongyu1@kylinos.cn> > > > > > > pl2303.c doesn't have reset_resume for hibernation. > > > So needs_binding will be set to 1 duiring hibernation. > > > usb_forced_unbind_intf will be called, and the port minor > > > will be released (x in ttyUSBx). > > > > Please use the full 72 columns that you are allowed in a changelog text. > > > > > > > It works fine if you have only one USB-to-serial device. > > > Assume you have 2 USB-to-serial device, nameing A and B. > > > A gets a smaller minor(ttyUSB0), B gets a bigger one. > > > And start to hibernate. When your PC is in hibernation, > > > unplug device A. Then wake up your PC by pressing the > > > power button. After waking up the whole system, device > > > B gets ttyUSB0. This will casuse a problem if you were > > > using those to ports(like opened two minicom process) > > > before hibernation. > > > So member reset_resume is needed in usb_serial_driver > > > pl2303_device. > > > > If you want persistent device naming, use the symlinks that udev creates > > for your for all your serial devices. Never rely on the number of a USB > > to serial device. > Let me put it this way. Assume you need to record messages output from two > machines using 2 USB-to-serial devices(naming A and B, and A is on > USB1-port3, B is on USB1-port4) opened by two minicom process. > The setting for A in minicom would be like: > "A - Serial Device : /dev/ttyUSB0" > The setting for B in minicom would be like: > "A - Serial Device : /dev/ttyUSB1" > Then start to hibernate on your computer. When your PC is in > hibernation, unplug A. After waking up your computer, "/dev/ttyUSB0" > would be released first, then allocated to B. The minicom process used > to record outputs from A is now recording B's outputs. The minicom > process used to record outputs from B is now recording nothing, because > "/dev/ttyUSB1" is not exist anymore. That's the problem I've been > talking about. And I don't think using symlinks will solve this problem. Yes, symlinks will solve the issue, that is what they are there for. Look in /dev/serial/ for a persistent name for them that allows you to uniquely open the correct device if they can be described. Using /dev/ttyUSBX is almost never the correct thing to do. > > > Codes in pl2303_reset_resume are borrowed from pl2303_open. > > > > > > As a matter of fact, all driver under drivers/usb/serial > > > has the same problem except ch341.c. > > > > > > Cc: stable@vger.kernel.org > > > > How does this meet the stable kernel rule requirements? It would be a > > new feature if it were accepted, right? > It's not a new feature at all. struct usb_serial_driver already has a > member name reset_resume, there is no implementation in pl2303.c yet. > And ch341.c has one(ch341_reset_resume()), that why I said "all driver > under drivers/usb/serial has the same problem except ch341.c" Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for what is valid stable kernel changes. thanks, greg k-h
Hi greg On 2022/4/22 13:07, Greg KH wrote: > On Fri, Apr 22, 2022 at 10:35:59AM +0800, Hongyu Xie wrote: >> >> Hi greg, >> On 2022/4/22 00:45, Greg KH wrote: >>> On Tue, Apr 19, 2022 at 02:54:08PM +0800, Hongyu Xie wrote: >>>> From: Hongyu Xie <xiehongyu1@kylinos.cn> >>>> >>>> pl2303.c doesn't have reset_resume for hibernation. >>>> So needs_binding will be set to 1 duiring hibernation. >>>> usb_forced_unbind_intf will be called, and the port minor >>>> will be released (x in ttyUSBx). >>> >>> Please use the full 72 columns that you are allowed in a changelog text. >>> >>> >>>> It works fine if you have only one USB-to-serial device. >>>> Assume you have 2 USB-to-serial device, nameing A and B. >>>> A gets a smaller minor(ttyUSB0), B gets a bigger one. >>>> And start to hibernate. When your PC is in hibernation, >>>> unplug device A. Then wake up your PC by pressing the >>>> power button. After waking up the whole system, device >>>> B gets ttyUSB0. This will casuse a problem if you were >>>> using those to ports(like opened two minicom process) >>>> before hibernation. >>>> So member reset_resume is needed in usb_serial_driver >>>> pl2303_device. >>> >>> If you want persistent device naming, use the symlinks that udev creates >>> for your for all your serial devices. Never rely on the number of a USB >>> to serial device. >> Let me put it this way. Assume you need to record messages output from two >> machines using 2 USB-to-serial devices(naming A and B, and A is on >> USB1-port3, B is on USB1-port4) opened by two minicom process. >> The setting for A in minicom would be like: >> "A - Serial Device : /dev/ttyUSB0" >> The setting for B in minicom would be like: >> "A - Serial Device : /dev/ttyUSB1" >> Then start to hibernate on your computer. When your PC is in >> hibernation, unplug A. After waking up your computer, "/dev/ttyUSB0" >> would be released first, then allocated to B. The minicom process used >> to record outputs from A is now recording B's outputs. The minicom >> process used to record outputs from B is now recording nothing, because >> "/dev/ttyUSB1" is not exist anymore. That's the problem I've been >> talking about. And I don't think using symlinks will solve this problem. > > Yes, symlinks will solve the issue, that is what they are there for. > Look in /dev/serial/ for a persistent name for them that allows you to > uniquely open the correct device if they can be described. Using > /dev/ttyUSBX is almost never the correct thing to do. Thanks for letting me know this. This patch is useless. I still have one more question though, since using /dev/ttyUSBX is almost never the correct thing to do, what is /dev/ttyUSBx used for then? > >>>> Codes in pl2303_reset_resume are borrowed from pl2303_open. >>>> >>>> As a matter of fact, all driver under drivers/usb/serial >>>> has the same problem except ch341.c. >>>> >>>> Cc: stable@vger.kernel.org >>> >>> How does this meet the stable kernel rule requirements? It would be a >>> new feature if it were accepted, right? >> It's not a new feature at all. struct usb_serial_driver already has a >> member name reset_resume, there is no implementation in pl2303.c yet. >> And ch341.c has one(ch341_reset_resume()), that why I said "all driver >> under drivers/usb/serial has the same problem except ch341.c" > > Please read: > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html > for what is valid stable kernel changes. > > thanks, > > greg k-h thanks, Hongyu Xie
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c index 88b284d61681..7cc05123b88c 100644 --- a/drivers/usb/serial/pl2303.c +++ b/drivers/usb/serial/pl2303.c @@ -1218,6 +1218,53 @@ static void pl2303_process_read_urb(struct urb *urb) tty_flip_buffer_push(&port->port); } +static int pl2303_configure(struct usb_serial *serial, struct pl2303_serial_private *priv) +{ + struct usb_serial_port *port = serial->port[0]; + + if (priv->quirks & PL2303_QUIRK_LEGACY) { + usb_clear_halt(serial->dev, port->write_urb->pipe); + usb_clear_halt(serial->dev, port->read_urb->pipe); + } else { + /* reset upstream data pipes */ + if (priv->type == &pl2303_type_data[TYPE_HXN]) + pl2303_vendor_write(serial, PL2303_HXN_RESET_REG, + PL2303_HXN_RESET_UPSTREAM_PIPE | + PL2303_HXN_RESET_DOWNSTREAM_PIPE); + else { + pl2303_vendor_write(serial, 8, 0); + pl2303_vendor_write(serial, 9, 0); + } + } + return 0; +} + +static int pl2303_reset_resume(struct usb_serial *serial) +{ + struct usb_serial_port *port = serial->port[0]; + struct pl2303_serial_private *priv = usb_get_serial_port_data(port); + struct tty_struct *tty = tty_port_tty_get(&port->port); + int ret; + + /* reconfigure pl2303 serial port after bus-reset */ + pl2303_configure(serial, priv); + + /* Setup termios */ + if (tty) + pl2303_set_termios(tty, port, NULL); + + if (tty_port_initialized(&port->port)) { + ret = usb_submit_urb(port->interrupt_in_urb, GFP_NOIO); + if (ret) { + dev_err(&port->dev, "failed to submit interrupt urb: %d\n", + ret); + return ret; + } + } + + return usb_serial_generic_resume(serial); +} + static struct usb_serial_driver pl2303_device = { .driver = { .owner = THIS_MODULE, @@ -1246,6 +1293,7 @@ static struct usb_serial_driver pl2303_device = { .release = pl2303_release, .port_probe = pl2303_port_probe, .port_remove = pl2303_port_remove, + .reset_resume = pl2303_reset_resume, }; static struct usb_serial_driver * const serial_drivers[] = {