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

[Johan Hovold <johan@kernel.org>]

Hi Martin (and Corentin),

and sorry about the late reply here.

On Sun, Aug 25, 2024 at 12:56:17PM +0200, Martin Blumenstingl wrote:
> On Tue, Jul 23, 2024 at 6:13 PM Johan Hovold <johan@kernel.org> wrote:

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

Ok, thanks for confirming, sounds like that's how the device works then.

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

It would only be to really confirm that this is how the vendor protocol
and device works. Your call.

> [...]
> > > > > +             * 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.

Right. I think the io_edgeport driver maintains some kind of TxCredits.
I guess that's related, but not sure how relevant that is here (and you
probably shouldn't based anything on that old driver directly anyway).

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

Not sure what Corentin has been preparing, but yeah, you need some kind
of deferred mechanism to make write() non-blocking and hold off sending
more data to the device until you're sure there's room in its buffers. I
guess a workqueue should do fine.

Johan