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: <CAFBinCAt1DevnggWJdzBzh3X1Yfb0ScZXYsgkrA1cGrUmfXVwg@mail.gmail.com>
Date: Mon, 15 Dec 2025 03:10:29 +0100
From: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To: Johan Hovold <johan@...nel.org>
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

Hi Johan,

On Mon, Dec 1, 2025 at 3:10 PM Johan Hovold <johan@...nel.org> wrote:
[...]
> > 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.
Doing so results in:
- lots of UART_LSR_OE
- RTS stays LOW (pulled to GND)

UART_LSR_OE increasing seems correct as far as I understand this.
RTS being LOW is wrong and I cannot manage to get ch348 to pull it to HIGH.

I did some more research and found that ch348 implements UART_IIR_MSI
and provides a fully standard compatible UART_MSR.
This is either triggered by a status change on the pins (UART_MSR
delta bits and the actual status bits), or by requesting an update
using the VEN_R command (UART_MSR status bits only, no delta bits).

In a very simple test-case I've used jumper cables on port #0 of ch348:
- RX and TX connected together
- CTS and RTS connected together

If I remove the jumper between CTS and RTS I get:
  ch348 ttyUSB0: got MSR = 0x01 // jumper removed
  ch348 ttyUSB0: got MSR = 0x11 // jumper connected again
  ch348 ttyUSB0: got MSR = 0x01 // jumper removed again

So the hardware does register the change.

Earlier I thought I found a fix: I had the values for
R_C4_HW_FLOW_CONTROL_OFF and R_C4_HW_FLOW_CONTROL_ON swapped.
That however didn't fix it.

My current work can be found here: [0]
If you also don't have any further ideas then I'll drop the whole
RTS/CTS code for now so the ch348 driver can finally make it into
Linux 6.20

> And in the other direction, verify that writes are buffered after you
> deassert RTS manually on the other end. That should be easier.
This seems to work: if I pull CTS up then ch348 stops sending data

> > 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.
In the meantime I found out why I had trouble with the DTR signal on port 1.
It was a user(space) error. I've been using [1] for some of my tests
and it has a bug where it would clear c_cflag HUPCL [2], which
prevents the kernel from turning DTR off on port close.

[...]
> 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.
I saw this and I'm switching over to use this as it simplifies my code.

> > 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()).
I started implementing the endpoint mapping in calc_num_ports and it
simplified the code.


Best regards,
Martin


[0] https://github.com/xdarklight/ch348/blob/v9-prep-20251214/ch348.c
[1] https://github.com/cbrake/linux-serial-test/
[2] https://github.com/cbrake/linux-serial-test/pull/70