Message-ID: <CAFBinCA8cMP3o483c40RjHkMAEt4RCmL6uCTTk5DPmrNVN6_NQ@mail.gmail.com>
Date: Wed, 16 Jul 2025 11:31:49 +0200
From: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To: Greg KH <gregkh@...uxfoundation.org>
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 Wed, Jul 16, 2025 at 10:57 AM Greg KH <gregkh@...uxfoundation.org> wrote:
>
> On Wed, Jul 16, 2025 at 10:28:22AM +0200, Martin Blumenstingl wrote:
> > On Wed, Jul 16, 2025 at 9:44 AM Greg KH <gregkh@...uxfoundation.org> wrote:
> > >
> > > 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.
> > I'm currently refcounting from usb_serial_driver.{open,close}
> > The additional challenge is that I need to open two URBs at the right
> > time to "avoid wasting resources". At least in my head I can't make it
> > work without an additional lock.
>
> Urbs really aren't all that large of a "resource", so don't worry about
> that.  Get it working properly first before attempting to care about
> small buffers like this :)
I understood that this is a requirement from Johan, he wrote (on May
12th in this thread):
> 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.

The original approach was to submit these URBs in .attach (e.g. during
probe time) and kill them in .detach

> > The following is a simplified/pseudo-code version of what I have at
> > the moment (which is called from my ch348_open):
> > static int ch348_submit_urbs(struct usb_serial *serial)
> > {
> >   int ret = 0;
> >
> >   mutex_lock(&ch348->manage_urbs_lock);
> >
> >   if (refcount_read(&ch348->num_open_ports))
> >     goto out_increment_refcount;
> >
> >   ret = usb_serial_generic_open(NULL, serial_rx);
> >   if (ret)
> >     goto out_unlock;
> >
> >   ret = usb_serial_generic_open(NULL, status);
> >   if (ret) {
> >     usb_serial_generic_close(serial_rx); /* undo the first
> > usb_serial_generic_open */
> >     goto out_unlock;
> >   }
>
> That's odd, why use NULL for a tty device?  Ah, we don't even use it for
> anything anymore, maybe that should be fixed...
usb_serial_generic_open() doesn't use the tty internally - which is
why passing NULL doesn't matter now (but who knows what's going to
happen in future, so better move away from it).

> Anyway, just submit the urbs, why use usb_serial_generic_* at all for
> the status urb?  That's not normal.
I can either submit the urbs myself or use usb_serial_generic_submit_read_urbs()

> And are you trying to only have one set of urbs out for any port being
> opened (i.e. you only have one control, one read, and one write urb for
> the whole device, and the port info are multiplexed over these urbs?  Or
> do you have one endpoint per port?)
CH348 provides up to 8 serial ports using these four endpoints, so
multiplexing is going on:
- one bulk out for TX (see struct ch348_txbuf)
- one bulk in for RX (see struct ch348_rxbuf)
- one bulk out for CONFIG handling (see struct ch348_config_buf)
- one bulk in for STATUS handling (see struct ch348_status_entry)

> If you are sharing endpoints, try looking at one of the other usb-serial
> drivers that do this today, like io_edgeport.c, that has had shared
> endpoints for 25 years, it's not a new thing :)
My understanding is that io_edgeport is submits the URBs that are
shared across ports outside of .open/.close.
So this will be a question for Johan: am I still good with the
original approach - or can you convince Greg that a different approach
is better?

[...]
> > > > > 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?
> > I don't know the reason behind this.
> > These are requests to the same bulk out endpoint. When submitting
> > multiple serial TX requests at the same time then some of the data is
> > lost / corrupted.
> >
> > The vendor driver basically does this in their write function (very
> > much simplified, see [0] for their original code):
> >   spin_lock_irqsave(&ch9344->write_lock, flags);
> >   usb_submit_urb(wb->urb, GFP_ATOMIC); /* part of ch9344_start_wb */
> >   spin_unlock_irqrestore(&ch9344->write_lock, flags);
>
> that's crazy, why the timeout logic in there?  This is a write function,
> submit the data to the device and get out of the way, that's all that
> should be needed here.
>From what I've seen during my tests:
- we fire the URB with TX data
- the device receives it and puts the data into a per-port TX buffer
- it indicates that it's done processing the URB
- the TX buffer is then written out (the host can move on do something else)
- the device signals to the host (via the STATUS endpoint) that it has
written out all data from the TX buffer

With that timeout logic my understanding is that they're trying to
catch cases where the last step (STATUS signal) did not work (for
whatever reason) so that the host can continue sending more data.

> > If you have any suggestions: please tell me - I'm happy to try them out!
>
> Try keeping it simple first, the vendor driver looks like it was written
> by someone who was paid per line of code :)
The original approach when upstreaming the ch348 driver was just to
submit TX URBs (without any custom code, just letting usb-serial core
handle it).
Corentin and Nicolas even wrote test programs to help reproduce issues
we've seen with the initial driver versions.
In other words: I don't know how to simplify our (to be upstreamed)
version without breaking something :-(

Also I'm not paid for this driver (at all - that also includes payment
per line of code), so there's no incentive there ;-).


Best regards,
Martin

Powered by blists - more mailing lists