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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAFBinCBMTOM-FMgENS-mrnV17HbKzhtPUd44_dDiwnD=+HVMWQ@mail.gmail.com>
Date: Sat, 26 Jul 2025 16:54:17 +0200
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

On Fri, Jul 25, 2025 at 12:07 PM Johan Hovold <johan@...nel.org> wrote:
>
> On Tue, Jul 15, 2025 at 11:20:33PM +0200, Martin Blumenstingl wrote:
>
> > On Mon, May 12, 2025 at 12:03 PM Johan Hovold <johan@...nel.org> wrote:
>
> > > The read urbs should be submitted at first open and stopped at last
> > > close to avoid wasting resources when no one is using the device.
> > >
> > > I know we have a few drivers that do not do this currently, but it
> > > shouldn't be that hard to get this right from the start.
>
> > If you're aware of an easy approach or you can recommend an existing
> > driver that implements the desired behavior then please let me know.
> >
> > The speciality about ch348 is that all ports share the RX/TX URBs.
> > My current idea is to implement this using a ref count (for the number
> > of open ports) and mutex for locking.
>
> Just use a mutex and integer (not refcount) to count the number of open
> ports. Submit the urbs on first open and stop them on last close.
>
> Not doing so, and instead submitting at attach(), means that the host
> controller will be wasting power by polling the endpoints continuously
> as long as the device is plugged in.
Thanks, my code wasn't miles off of f81534.c but I'm following that
more closely now.

[...]
> > I'm trying as you suggest:
> > - submit the URB synchronously for port N
> > - submit the URB synchronously for port N + 1
> > - ...
> >
> > This seems to work (using usb_bulk_msg). What doesn't work is
> > submitting URBs in parallel (this is what the vendor driver prevents
> > as well).
>
> No, the vendor driver tracks THRE per port
> (ttyport[portnum].write_empty) and allows writing to more than one port
> in parallel (e.g. releases the device write_lock while waiting for the
> transfer to complete).
>
> I thought the problem was that you could not submit another urb for the
> *same* port until the device buffer for that port had been emptied?
>
> This seems to be what the vendor driver is preventing.
I managed to get it to work now without any unnecessary waiting.
When I switched to just waiting for per-port THRE I accidentally
re-used the same URB (along with its buffer) for all ports. This of
course "corrupts" data, but it's my fault instead of the chip/firmware
causing it.
That's why I was referring to data corruption earlier.
Thanks for your persistence and for making me look at my code again
with a fresh mind.

> > > You should implement dtr_rts() as well.
>
> > This will be the first time we need the "package type" information as
> > CH348Q only supports CTS/RTS on the first four ports, for the last
> > four the signals aren't routed outside the package.
> > I need to see if I have other hardware with CTS/RTS pins to test this.
>
> Just connect a multimeter to the DTR and RTS pins and verify that they
> are asserted on open and deasserted on close after issuing those control
> requests (see ch9344_port_dtr_rts()).
Do I need to set anything special in termios for this to work?
The datasheet has a special note about DTR/TNOW (on page 8 for the CFG pin):
> Unified configuration: During power-on, if the CFG pin is
> at a high level or not connected, all DTRx/ TNOWx pins
> are configured to function as TNOW. CFG pin is low, all
> DTRx/ TNOWx pins are configured for DTR function.

On my test board the CFG pin is HIGH. From how I understand you, RTS
should at least change (even if DTR is in TNOW mode).
No matter what I do: both pins are always LOW (right after modprobe,
after opening the console, closing the console again, ...).
I even set up the vendor driver to test this: it's the same situation there.

If we need to make the DTR and RTS output something then the only way
I know of right now is to switch them to GPIO mode (I have code for
this but it's another ~300 lines patch on top of this).
So I'd like to not implement .dtr_rts and drop all CRTSCTS related code for now.


Best regards,
Martin