[RESEND,-next] USB: serial: pl2303: implement reset_resume member

Commit Message

Hongyu Xie April 19, 2022, 6:54 a.m. UTC

From: Hongyu Xie <[e-mail]>

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).
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.
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: [e-mail]
Signed-off-by: Hongyu Xie <[e-mail]>
Reported-by: sheng.huang <[e-mail]>
---
 drivers/usb/serial/pl2303.c | 48 +++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

Comments

Greg KH April 21, 2022, 4:45 p.m. UTC | #1

On Tue, Apr 19, 2022 at 02:54:08PM +0800, Hongyu Xie wrote:
> From: Hongyu Xie <[e-mail]>
> 
> 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.

> 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: [e-mail]

How does this meet the stable kernel rule requirements?  It would be a
new feature if it were accepted, right?


thanks,

greg k-h

Hongyu Xie April 22, 2022, 2:35 a.m. UTC | #2

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 <[e-mail]>
>>
>> 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: [e-mail]
> 
> 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

Greg KH April 22, 2022, 5:07 a.m. UTC | #3

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 <[e-mail]>
> > > 
> > > 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: [e-mail]
> > 
> > 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

Hongyu Xie April 22, 2022, 6:42 a.m. UTC | #4

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 <[e-mail]>
>>>>
>>>> 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: [e-mail]
>>>
>>> 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

Greg KH April 22, 2022, 7:36 a.m. UTC | #5

On Fri, Apr 22, 2022 at 02:42:41PM +0800, Hongyu Xie wrote:
> 
> 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 <[e-mail]>
> > > > > 
> > > > > 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.

It's not useless, I'm just saying that you should not be relying on
ttyUSBX nodes to ever be persistent.

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

That is the name that the kernel gives them, as it has to pick
something.  Same for all kernel device names, the persistence rules live
in userspace, not in the kernel.

Now if this is a valid feature or not for the driver that's a totally
different question that I will let Johan review.  I was just pointing
out a way for you to resolve your issue today without any kernel changes
needed at all.

thanks,

greg k-h

Patch

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[] = {