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] [thread-next>] [day] [month] [year] [list]

Message-ID: <2025071631-thesaurus-blissful-58f3@gregkh>
Date: Wed, 16 Jul 2025 09:44:27 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Cc: Johan Hovold <johan@...nel.org>, Corentin Labbe <clabbe@...libre.com>,
	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 Tue, Jul 15, 2025 at 11:20:33PM +0200, Martin Blumenstingl wrote:
> Hi Johan,
> 
> I'm excluding comments that are clear to me in this reply.
> 
> On Mon, May 12, 2025 at 12:03 PM Johan Hovold <johan@...nel.org> wrote:
> [...]
> > > +     if (ret) {
> > > +             dev_err(&port->serial->dev->dev,
> > > +                     "Failed to configure UART_MCR, err=%d\n", ret);
> > > +             return ret;
> > > +     }
> >
> > 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.

How do you know if a port is "open" or not and keep track of them all?
Trying to manage that is a pain and a refcount shouldn't need locking if
you use the proper refcount_t type in a sane way.

Try to keep it simple please.

> > With this implementation writing data continuously to one port will
> > starve the others.
> >
> > The vendor implementation appears to write to more than one port in
> > parallel and track THRE per port which would avoid the starvation issue
> > and should also be much more efficient.
> >
> > Just track THRE per port and only submit the write urb when it the
> > transmitter is empty or when it becomes empty.
> 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).

Why would submitting urbs in parallel not work?  Is the device somehow
broken and can't accept multiple requests at the same time?

thanks,

greg k-h