Message-ID: <2025071613-ethics-component-e56d@gregkh>
Date: Wed, 16 Jul 2025 10:57: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 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 :)

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

Anyway, just submit the urbs, why use usb_serial_generic_* at all for
the status urb?  That's not normal.

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

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

> out_increment_refcount:
>   refcount_inc(&ch348->num_open_ports);
> 
> out_unlock:
>   mutex_unlock(&ch348->manage_urbs_lock);
> 
>   return ret;
> }
> 
> My understanding is that usb-serial core does not provide any locking
> around .open/.close.

Nor should it, these are independent per-port.

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

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

good luck!

greg k-h

Powered by blists - more mailing lists