Message-ID: <2025071639-annotate-huddle-0e11@gregkh>
Date: Wed, 16 Jul 2025 12:00:50 +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 Wed, Jul 16, 2025 at 11:31:49AM +0200, Martin Blumenstingl wrote:
> 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

Ok, that's fine, but as you are multiplexing stuff, so reference counts
are going to get complex.  I'll defer to Johan, but this seems messy.

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

Yes it does.

sorry about that, I misunderstood Johan's review comments here.  I'll
defer to him.


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

Why can't the host just keep sending data?  Only "stall" if you don't
have an active urb to send?  Or just keep creating new urbs for every
send transaction and then destroying them when finished?  That way the
data gets queued up in the kernel (have a max able to be allocated so
you don't create a DoS accidentally), and you should be ok.  I think
some of the other drivers do this, or used to in the past.

> > > 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).

Ah, and what happened with that version?  Did it not work?

thanks,

greg k-h

Powered by blists - more mailing lists