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

[Martin Blumenstingl <martin.blumenstingl@googlemail.com>]

[...]
> > >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.
The usb-serial framework queues up data in a kfifo (with max size = PAGE_SIZE).
After step 3 (CH348 indicates to the host that it has ingested the
URB) we can continue sending data for another port - that's what I'm
working on for v9.

> > > > 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?
TX data was truncated/corrupted. There's a recent-ish commit in the
vendor driver [0] which suggests that parallel (they're calling it
"independent upload on multiple serial ports") is possible starting
with chip revision 0x8a but I cannot test this (since my test board
has a CH348Q revision 0x86).


Best regards,
Martin


[0] https://github.com/WCHSoftGroup/ch9344ser_linux/commit/875d57b8c6a7dd3f4359eb9adfe3779e2bbb0ac1