Message-ID: <CAFBinCATe+RXHz6Cy9cbo=vYL+qm_kz1qDTB8oL775xdgk=TYg@mail.gmail.com>
Date: Sun, 25 Aug 2024 12:56:17 +0200
From: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To: Johan Hovold <johan@...nel.org>
Cc: Corentin Labbe <clabbe@...libre.com>, gregkh@...uxfoundation.org, 
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org, david@...t.cz
Subject: Re: [PATCH 1/1 v7] usb: serial: add support for CH348

Hi Johan,

On Tue, Jul 23, 2024 at 6:13 PM Johan Hovold <johan@...nel.org> wrote:
[...]
> > > Do you need to include any copyrights from the vendor driver? It looks
> > > like at least some of the code below was copied from somewhere (but
> > > perhaps not that much).
>
> > If you - or someone else - have any advice on this I'd be happy to go with that!
>
> If you copy code directly (even if you clean it up after) you should
> include it, but not necessarily if you just use it for reference for the
> protocol.
>
> It doesn't hurt mentioning anyway when you add the reference to the
> vendor driver, for example:
>
>         Based on the XXX driver:
>
>                 https://...
>
>         Copyright YYY
Thanks, I'll include that in the next version.

[...]
> > For slow speeds I never receive the "Transmitter holding register
> > empty" interrupt/signal when using the full TX buffer.
> > It's not that the interrupt/signal is late - it just never arrives.
> > I don't know why that is (whether the firmware tries to keep things
> > "fair" for other ports, ...) though.
>
> Perhaps you can run some isolated experiments if you haven't already.
> Submitting just a single URB with say 128, 512 or 1024 bytes of data and
> see when/if you ever receive a transmitter holding empty interrupt.
>
> How does the vendor driver handle this? Does it really wait for the THRE
> interrupt before submitting more data?
The vendor driver:
- first acquires a per-device (not per port) write_lock [0]
- then waits for the (per-device, not per port) write buffer to be empty [1]
- and only then submits more data to be transmitted [2]

> You could try increasing the buffer size to 2k and see how much is
> received on the other end if you submit one URB (e.g. does the hardware
> just drop the last 1k of data when the device fifo is full).
I have not tried this yet but if still relevant (after the info about
the THRE interrupt) then I can try it and share the results.

[...]
> > > > +             * If we ingest more data then usb_serial_generic_write() will
> > > > +             * internally try to process as much data as possible with any
> > > > +             * number of URBs without giving us the chance to wait in
> > > > +             * between transfers.
> > >
> > > If the hardware really works this way, then perhaps you should not use
> > > the generic write implementation. Just maintain a single urb per port
> > > and don't submit it until the device fifo is empty.
>
> > I tried to avoid having to copy & paste (which then also means having
> > to maintain it down the line) most of the generic write
> > implementation.
> > This whole dance with waiting for the "Transmitter holding register
> > empty" by the way was the reason why parts of the transmit buffer got
> > lost, see the report from Nicolas in v6 [1]
>
> I understand, but the generic implementation is not a good fit here as
> it actively tries to make sure the device buffers are always full (e.g.
> by using two URBs back-to-back).
>
> If you can't find a way to make the hardware behave properly then a
> custom implementation using a single URBs is preferable over trying
> to limit the generic implementation like you did here. Perhaps bits can
> be reused anyway (e.g. chars_in_buffer if you use the write fifo).
I cannot find any other usb-serial driver which uses this pattern.
Most devices seem to be happy to take more data once they trigger the
write_bulk_callback but not ch348.

If there's any other (even if it's not a usb-serial) driver that I can
use as a reference implementation for your suggestion?
I'm not sure whether to use a dedicated kthread, single threaded workqueue, ...

> > > > +static struct usb_serial_driver ch348_device = {
> > > > +     .driver = {
> > > > +             .owner = THIS_MODULE,
> > > > +             .name = "ch348",
> > > > +     },
> > > > +     .id_table =             ch348_ids,
> > > > +     .num_ports =            CH348_MAXPORT,
> > > > +     .num_bulk_in =          1,
> > > > +     .num_bulk_out =         1,
> > >
> > > Set both of these to 2 so that core verifies that you have all four
> > > endpoints.
>
> > I will have to test this because I thought that:
> > - using 2 here makes usb-serial allocate an URB as well and by default
> > assign it to the first and second port
> > - usb-serial should not touch the second bulk in / bulk out endpoint
> > (as they're entirely vendor / chip specific)
>
> Setting these two should make core make sure that the endpoints exist,
> and by default they will be assigned to the first and second port, but
> you can override that calc_num_endpoints() (as you already do).
>
> For the second IN EP, you could even let core allocate the URB and use
> that instead of doing so manually (e.g. by submitting yourself or using
> the generic read implementation as mxuport does).
Thanks for the hint - I have tried this and it indeed simplifies the code!


Best regards,
Martin


[0] https://github.com/WCHSoftGroup/ch9344ser_linux/blob/d4fc95fb1cca9962384ca88b0007df8738ae5829/driver/ch9344.c#L1100
[1] https://github.com/WCHSoftGroup/ch9344ser_linux/blob/d4fc95fb1cca9962384ca88b0007df8738ae5829/driver/ch9344.c#L1152-L1156
[2] https://github.com/WCHSoftGroup/ch9344ser_linux/blob/d4fc95fb1cca9962384ca88b0007df8738ae5829/driver/ch9344.c#L1166

Powered by blists - more mailing lists