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

[Greg KH <gregkh@linuxfoundation.org>]

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@kernel.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