[PATCH] USB: serial: use tty_port_register_device_serdev

[PATCH] USB: serial: use tty_port_register_device_serdev

[Mans Rullgard]

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(-)

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;
-- 
2.45.0

Re: [PATCH] USB: serial: use tty_port_register_device_serdev

[Johan Hovold]

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

Re: [PATCH] USB: serial: use tty_port_register_device_serdev

[Måns Rullgård]

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?

-- 
Måns Rullgård

Re: [PATCH] USB: serial: use tty_port_register_device_serdev

[Greg Kroah-Hartman]

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

Re: [PATCH] USB: serial: use tty_port_register_device_serdev

[Måns Rullgård]

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.

-- 
Måns Rullgård

Re: [PATCH] USB: serial: use tty_port_register_device_serdev

[Greg Kroah-Hartman]

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

Re: [PATCH] USB: serial: use tty_port_register_device_serdev

[Måns Rullgård]

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?

-- 
Måns Rullgård

Re: [PATCH] USB: serial: use tty_port_register_device_serdev

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki