Re: [PATCH 0/3] USB-Serial serdev support

[Johan Hovold <johan@kernel.org>]

On Thu, Mar 13, 2025 at 08:40:44PM +0100, Marco Felsch wrote:
> On 25-03-11, Johan Hovold wrote:
> > On Tue, Sep 17, 2024 at 06:49:48AM +0200, Marco Felsch wrote:
> > > On 24-09-09, Johan Hovold wrote:
> > > > On Wed, Aug 07, 2024 at 04:08:47PM +0200, Marco Felsch wrote:
> > > > > this patchset is based on Johan's patches [1] but dropped the need of
> > > > > the special 'serial' of-node [2].
> > > > 
> > > > That's great that you found and referenced my proof-of-concept patches,
> > > > but it doesn't seem like you tried to understand why this hasn't been
> > > > merged yet.
> > 
> > > > First, as the commit message you refer to below explain, we need some
> > > > way to describe multiport controllers. Just dropping the 'serial' node
> > > > does not make that issue go away.
> > > 
> > > Sorry for asking but isn't the current OF abstraction [1] enough? As far
> > > as I understood we can describe the whole USB tree within OF. I used [1]
> > > and the this patchset to describe the following hierarchy:
> > > 
> > >  usb-root -> usb-hub port-1 -> usb-serial interface-0 -> serial
> > >                                                          bt-module
> > > 
> > > [1] Documentation/devicetree/bindings/usb/usb-device.yaml
> > 
> > Again, you still need to consider devices with multiple serial ports
> > (and they do not always map neatly to one port per interface either).
> 
> We use a dual-port FTDI and our USB tree looks as followed:

> interface-0 is used for the bt-module which is served by the serdev
> driver.
> 
> interface-1 is used by an userspace driver which makes use of the
> /dev/ttyUSB1 port.
>
> So we do have the multiple serial ports use-case already. Can you please
> explain what I miss?

There are other USB serial devices that support multiple ports over a
single USB interface. The DT bindings need to account for that case as
well, and that probably means we should not be describing the interfaces
at all but rather the physical ports.

> > > > Second, and more importantly, you do not address the main obstacle for
> > > > enabling serdev for USB serial which is that the serdev cannot handle
> > > > hotplugging.
> > > 
> > > Hotplugging is a good point but out-of-scope IMHO (at least for now)
> > > since the current serdev implementation rely on additional firmware
> > > information e.g OF node to be present. E.g. if the above mentioned setup
> > > would connect the "serial bt-module" directly to the UART port you still
> > > need an OF node to bind the serdev driver. If the node isn't present
> > > user-space would need to do the hci handling.
> > 
> > There's nothing preventing you from adding a devicetree node for a USB
> > device that can be unplugged.
> 
> I see and I have to admit that I didn't test this :/ But since you
> pointed it out I tested it now!
> 
> So as explained, our USB tree looks as above and our DTS looks like the
> one in the cover letter. Of course I run on an embedded system but the
> USB FTDI based module is powered by the VBUS of the hub. Therefore I
> ran the test by disabling the downstream port which in turn disabled the
> VBUS supply. This should come very close to a physical unplug event.

You will also see the following kind of warnings in the logs:

ttyUSB ttyUSB0: tty_hangup: tty->count(1) != (#fd's(0) + #kopen's(0))
ttyUSB ttyUSB0: tty_port_close_start: tty->count = 1 port count = 0

which are due to the fact that serdev does not support hangups which are
used during teardown of USB serial ports.

> > > So from my POV the serdev abstraction is for manufacturers which make
> > > use of "onboard" usb-devices which are always present at the same USB
> > > tree location. Serdev is not made for general purpose USB ports (yet)
> > > where a user can plug-in all types of USB devices.
> > 
> > Right, but someone need to make sure that serdev can handle devices
> > going away first as nothing is currently preventing that from happening.
> 
> Can you please check my above tests? Maybe I do miss something but for
> me it looks like it's working. Looking forwards for your input.

I skimmed the code to verify that the issue is still there, and even
forward ported my patches to confirm that that you would still see the
port count warnings that indicate that something is amiss.

Johan