linux-kernel - Re: [PATCH v8 1/2] usb: serial: add support for CH348

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aS2hxeBR-tptevYd@hovoldconsulting.com>
Date: Mon, 1 Dec 2025 15:10:13 +0100
From: Johan Hovold <johan@...nel.org>
To: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Cc: Corentin Labbe <clabbe@...libre.com>, gregkh@...uxfoundation.org,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
	david@...t.cz
Subject: Re: [PATCH v8 1/2] usb: serial: add support for CH348

On Sat, Nov 29, 2025 at 04:59:36PM +0100, Martin Blumenstingl wrote:
> On Wed, Aug 27, 2025 at 12:08 PM Johan Hovold <johan@...nel.org> wrote:
> >
> > On Mon, Aug 04, 2025 at 11:35:35PM +0200, Martin Blumenstingl wrote:
> > > On Mon, Aug 4, 2025 at 2:32 PM Johan Hovold <johan@...nel.org> wrote:

> > The host controller should split the buffer, but apparently this crashes
> > the device firmware.

> I think having a 512 - 3 (TX header) byte limitation for the TX buffer
> is acceptable for the first upstream version of this driver.

It's probably even required given how the firmware behaves.

> > > I think I made it work, sort of.
> > > It's a bit annoying because of code I don't understand. It seems that
> > > R_4 has the following settings:
> > > 0x00 DTR off
> > > 0x01 DTR on
> > > 0x10 RTS off
> > > 0x11 RTS on
> > > 0x08 activate (used during port initialization)
> > > 0x50 HW flow on
> > > 0x51 no RTS / HW flow off
> > >
> > > That said, poking 0x00, 0x01, 0x10 and 0x11 by themselves didn't do much.
> > > One also has to write 0x06 to the per-port VEN_R register.
> > > The vendor driver only does that in .set_termios, which I call
> > > questionable until someone calls me out on this and is willing to
> > > share a good reason why that's a good idea ;-)
> > >
> > > However, I'm unable to control the RTS line of port 1. It works for
> > > port 0, port 2 and 3 but not for port 1.
> > > Ports 4-7 don't have the TNOW/DTR and RTS lines routed outside the
> > > package, so I can't test these.
> >
> > Sounds like good progress. Have you made sure HW flow isn't just enabled
> > by default on port 1 or similar?

> Unfortunately I don't know how to read the HW flow control state from
> the hardware.
> Do you have any suggestions, how I can test HW flow control (after
> manually enabling it for a port)?

You can try disabling reading from the device (e.g. never submit the
read urbs) and see if the RTS is deasserted when the buffer fills up.

And in the other direction, verify that writes are buffered after you
deassert RTS manually on the other end. That should be easier.

> In case I can't easily figure it out: would you also accept a driver
> that doesn't support RTS/CTS for its initial version?

It's good to at least be able to control DTR/RST at open/close (i.e.
implement dtr_rts()) so that you can communicate when the other end
has hw flow enabled. Sound like you're really close to doing so.

> On another note: after walking away from the driver for some time I
> came back to day and spotted a comment in usb serial core's
> usb-serial.c:

> > /* we don't use num_ports here because some devices have more endpoint pairs than ports */

> With that I'm now setting num_bulk_out to 8 (number of ports) + 1 (for
> the config endpoint).

You should not be able to probe() if you set num_bulk_out higher than
the actual number of bulk out endpoints the device has as core verifies
that during probe.

You can (should) set num_ports higher (e.g. indirectly via
calc_num_ports()) for devices that mux data for multiple ports over
a shared endpoint (like this device, iirc).

mxuport and a couple of other drivers implements such a scheme.

> This, together with delaying the call to
> usb_serial_generic_write_bulk_callback() until we receive
> UART_IIR_THRI allowed me to get rid of the workqueue and re-use a lot
> more code from the USB serial core.

For writing if you need to wait for THRE per port then it may be best to
just use a driver specific write implementation (using a single urb per
port). That should be more readable/maintainable.

You can still let core allocate the urb and writer buffer for you (by
providing the endpoint mapping in calc_num_ports()).

Johan