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@xxxxxxxxxx> 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
