| Message ID | 20240502100728.7914-1-mans@mansr.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | USB: serial: use tty_port_register_device_serdev | expand |
On Thu, May 02, 2024 at 11:07:28AM +0100, Mans Rullgard wrote: > Use tty_port_register_device_serdev() so that usb-serial devices > can be used as serdev controllers. I'm afraid it's not that easy. The reason serdev is not enabled for usb-serial is that there's currently no support for handling hotplug in serdev. The device can go away from under you at any time and then you'd crash the kernel. Johan
Johan Hovold <johan@kernel.org> writes: > On Thu, May 02, 2024 at 11:07:28AM +0100, Mans Rullgard wrote: >> Use tty_port_register_device_serdev() so that usb-serial devices >> can be used as serdev controllers. > > I'm afraid it's not that easy. The reason serdev is not enabled for > usb-serial is that there's currently no support for handling hotplug in > serdev. The device can go away from under you at any time and then you'd > crash the kernel. Oh, that's unfortunate. Regular serial ports can go away too, though, and that seems to be handled fine. What am I missing?
On Thu, May 02, 2024 at 11:45:44AM +0100, Måns Rullgård wrote: > Johan Hovold <johan@kernel.org> writes: > > > On Thu, May 02, 2024 at 11:07:28AM +0100, Mans Rullgard wrote: > >> Use tty_port_register_device_serdev() so that usb-serial devices > >> can be used as serdev controllers. > > > > I'm afraid it's not that easy. The reason serdev is not enabled for > > usb-serial is that there's currently no support for handling hotplug in > > serdev. The device can go away from under you at any time and then you'd > > crash the kernel. > > Oh, that's unfortunate. Regular serial ports can go away too, though, > and that seems to be handled fine. What am I missing? How is it handled? Normal serial ports can go away but in practice, it's a rare occurance, and usually people use serdev for devices where the ports can not be removed (i.e. internal connections). thanks, greg k-h
Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes: > On Thu, May 02, 2024 at 11:45:44AM +0100, Måns Rullgård wrote: >> Johan Hovold <johan@kernel.org> writes: >> >> > On Thu, May 02, 2024 at 11:07:28AM +0100, Mans Rullgard wrote: >> >> Use tty_port_register_device_serdev() so that usb-serial devices >> >> can be used as serdev controllers. >> > >> > I'm afraid it's not that easy. The reason serdev is not enabled for >> > usb-serial is that there's currently no support for handling hotplug in >> > serdev. The device can go away from under you at any time and then you'd >> > crash the kernel. >> >> Oh, that's unfortunate. Regular serial ports can go away too, though, >> and that seems to be handled fine. What am I missing? > > How is it handled? Normal serial ports can go away but in practice, > it's a rare occurance, and usually people use serdev for devices where > the ports can not be removed (i.e. internal connections). If I unbind a regular serial port from its driver using sysfs, a serdev device defined in a device tree gets removed as expected. Binding the serial port makes everything come back again. I fail to see any problem here. If there is one, you'll have to be less evasive in explaining what it is.
On Thu, May 02, 2024 at 02:24:41PM +0100, Måns Rullgård wrote: > Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes: > > > On Thu, May 02, 2024 at 11:45:44AM +0100, Måns Rullgård wrote: > >> Johan Hovold <johan@kernel.org> writes: > >> > >> > On Thu, May 02, 2024 at 11:07:28AM +0100, Mans Rullgard wrote: > >> >> Use tty_port_register_device_serdev() so that usb-serial devices > >> >> can be used as serdev controllers. > >> > > >> > I'm afraid it's not that easy. The reason serdev is not enabled for > >> > usb-serial is that there's currently no support for handling hotplug in > >> > serdev. The device can go away from under you at any time and then you'd > >> > crash the kernel. > >> > >> Oh, that's unfortunate. Regular serial ports can go away too, though, > >> and that seems to be handled fine. What am I missing? > > > > How is it handled? Normal serial ports can go away but in practice, > > it's a rare occurance, and usually people use serdev for devices where > > the ports can not be removed (i.e. internal connections). > > If I unbind a regular serial port from its driver using sysfs, a serdev > device defined in a device tree gets removed as expected. Binding the > serial port makes everything come back again. I fail to see any problem > here. If there is one, you'll have to be less evasive in explaining > what it is. Try yanking a usb-serial device out with this patch applied and see what happens. I'm pretty sure serdev will not handle that well, just like if you yank out a pci serial device while it is being used. Doing bind/unbind is not a "surprise" removal, but a nice orderly one :) If this does now work, nice, but I haven't seen the changes to serdev to make this happen, I wonder what changed... thanks, greg k-h
Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes: > On Thu, May 02, 2024 at 02:24:41PM +0100, Måns Rullgård wrote: >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes: >> >> > On Thu, May 02, 2024 at 11:45:44AM +0100, Måns Rullgård wrote: >> >> Johan Hovold <johan@kernel.org> writes: >> >> >> >> > On Thu, May 02, 2024 at 11:07:28AM +0100, Mans Rullgard wrote: >> >> >> Use tty_port_register_device_serdev() so that usb-serial devices >> >> >> can be used as serdev controllers. >> >> > >> >> > I'm afraid it's not that easy. The reason serdev is not enabled for >> >> > usb-serial is that there's currently no support for handling hotplug in >> >> > serdev. The device can go away from under you at any time and then you'd >> >> > crash the kernel. >> >> >> >> Oh, that's unfortunate. Regular serial ports can go away too, though, >> >> and that seems to be handled fine. What am I missing? >> > >> > How is it handled? Normal serial ports can go away but in practice, >> > it's a rare occurance, and usually people use serdev for devices where >> > the ports can not be removed (i.e. internal connections). >> >> If I unbind a regular serial port from its driver using sysfs, a serdev >> device defined in a device tree gets removed as expected. Binding the >> serial port makes everything come back again. I fail to see any problem >> here. If there is one, you'll have to be less evasive in explaining >> what it is. > > Try yanking a usb-serial device out with this patch applied and see what > happens. I'm pretty sure serdev will not handle that well, just like if > you yank out a pci serial device while it is being used. Doing > bind/unbind is not a "surprise" removal, but a nice orderly one :) > > If this does now work, nice, but I haven't seen the changes to serdev to > make this happen, I wonder what changed... Turns out I missed one change that is needed for unplugging to be handled: --- a/drivers/usb/serial/bus.c +++ b/drivers/usb/serial/bus.c @@ -91,7 +91,7 @@ static void usb_serial_device_remove(struct device *dev) autopm_err = usb_autopm_get_interface(port->serial->interface); minor = port->minor; - tty_unregister_device(usb_serial_tty_driver, minor); + tty_port_unregister_device(&port->port, usb_serial_tty_driver, minor); driver = port->serial->type; if (driver->port_remove) With this additional change, yanking (shorting the data lines; the thing is soldered) the usb-serial converter works, although a couple of warnings are printed: [ 28.678301] usb 1-1: USB disconnect, device number 2 [ 28.683695] ttyUSB ttyUSB0: tty_hangup: tty->count(1) != (#fd's(0) + #kopen's(0)) [ 28.691516] ftdi_sio ttyUSB0: error from flowcontrol urb [ 28.759056] ttyUSB ttyUSB0: tty_port_close_start: tty->count = 1 port count = 0 [ 28.772531] ftdi_sio ttyUSB0: FTDI USB Serial Device converter now disconnected from ttyUSB0 [ 28.781346] ftdi_sio 1-1:1.0: device disconnected Where should I go looking for the cause of these?
Hi Mans, kernel test robot noticed the following build errors: [auto build test ERROR on johan-usb-serial/usb-next] [also build test ERROR on johan-usb-serial/usb-linus usb/usb-testing usb/usb-next usb/usb-linus tty/tty-testing tty/tty-next tty/tty-linus linus/master v6.9-rc6 next-20240502] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Mans-Rullgard/USB-serial-use-tty_port_register_device_serdev/20240502-180923 base: https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git usb-next patch link: https://lore.kernel.org/r/20240502100728.7914-1-mans%40mansr.com patch subject: [PATCH] USB: serial: use tty_port_register_device_serdev config: s390-randconfig-r081-20240503 (https://download.01.org/0day-ci/archive/20240503/202405030657.vgUKnyOZ-lkp@intel.com/config) compiler: s390-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240503/202405030657.vgUKnyOZ-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202405030657.vgUKnyOZ-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/usb/serial/bus.c: In function 'usb_serial_device_probe': >> drivers/usb/serial/bus.c:53:19: error: too few arguments to function 'tty_port_register_device_serdev' 53 | tty_dev = tty_port_register_device_serdev(&port->port, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from include/linux/tty.h:11, from drivers/usb/serial/bus.c:10: include/linux/tty_port.h:150:16: note: declared here 150 | struct device *tty_port_register_device_serdev(struct tty_port *port, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ vim +/tty_port_register_device_serdev +53 drivers/usb/serial/bus.c 31 32 static int usb_serial_device_probe(struct device *dev) 33 { 34 struct usb_serial_port *port = to_usb_serial_port(dev); 35 struct usb_serial_driver *driver; 36 struct device *tty_dev; 37 int retval = 0; 38 int minor; 39 40 /* make sure suspend/resume doesn't race against port_probe */ 41 retval = usb_autopm_get_interface(port->serial->interface); 42 if (retval) 43 return retval; 44 45 driver = port->serial->type; 46 if (driver->port_probe) { 47 retval = driver->port_probe(port); 48 if (retval) 49 goto err_autopm_put; 50 } 51 52 minor = port->minor; > 53 tty_dev = tty_port_register_device_serdev(&port->port, 54 usb_serial_tty_driver, 55 minor, dev); 56 if (IS_ERR(tty_dev)) { 57 retval = PTR_ERR(tty_dev); 58 goto err_port_remove; 59 } 60 61 usb_autopm_put_interface(port->serial->interface); 62 63 dev_info(&port->serial->dev->dev, 64 "%s converter now attached to ttyUSB%d\n", 65 driver->description, minor); 66 67 return 0; 68 69 err_port_remove: 70 if (driver->port_remove) 71 driver->port_remove(port); 72 err_autopm_put: 73 usb_autopm_put_interface(port->serial->interface); 74 75 return retval; 76 } 77
diff --git a/drivers/usb/serial/bus.c b/drivers/usb/serial/bus.c index 6c812d01b37d..34a1f355f17f 100644 --- a/drivers/usb/serial/bus.c +++ b/drivers/usb/serial/bus.c @@ -50,8 +50,9 @@ static int usb_serial_device_probe(struct device *dev) } minor = port->minor; - tty_dev = tty_port_register_device(&port->port, usb_serial_tty_driver, - minor, dev); + tty_dev = tty_port_register_device_serdev(&port->port, + usb_serial_tty_driver, + minor, dev); if (IS_ERR(tty_dev)) { retval = PTR_ERR(tty_dev); goto err_port_remove;
Use tty_port_register_device_serdev() so that usb-serial devices can be used as serdev controllers. Signed-off-by: Mans Rullgard <mans@mansr.com> --- drivers/usb/serial/bus.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)