Message-ID: <YmI4I9MCLBheMyvr@kroah.com>
Date:   Fri, 22 Apr 2022 07:07:47 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Hongyu Xie <xy521521@...il.com>
Cc:     johan@...nel.org, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org, Hongyu Xie <xiehongyu1@...inos.cn>,
        stable@...r.kernel.org, "sheng . huang" <sheng.huang@...stech.com>,
        wangqi@...inos.cn, xiongxin@...inos.cn
Subject: Re: [RESEND PATCH -next] USB: serial: pl2303: implement reset_resume
 member

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@...inos.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@...r.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

Powered by blists - more mailing lists