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

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

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

[...]
> > +                     /*
> > +                      * Writing larger buffers can take longer than the
> > +                      * hardware allows before discarding the write buffer.
> > +                      * Limit the transfer size in such cases.
> > +                      * These values have been found by empirical testing.
> > +                      */
> > +                     max_bytes = 128;
>
> This is a potential buffer overflow if a (malicious) device has
> endpoints smaller than this (use min()).
For endpoints smaller than CH348_TX_HDRSIZE we'll also be in trouble.
Validating against CH348_TX_HDRSIZE size here doesn't make much sense
to me (as we'd never be able to progress). I think I should validate
it in ch348_attach() instead - what do you think?

> > +             else
> > +                     /*
> > +                      * Only ingest as many bytes as we can transfer with
> > +                      * one URB at a time keeping the TX header in mind.
> > +                      */
> > +                     max_bytes = hw_tx_port->bulk_out_size - CH348_TX_HDRSIZE;
> > +
> > +             count = kfifo_out_locked(&port->write_fifo, rxt->data,
> > +                                      max_bytes, &port->lock);
> > +             if (count)
> > +                     break;
> > +     }
>
> 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).

[...]
> > +     if (!wait_for_completion_timeout(&ch348->txbuf_completion,
> > +                                      msecs_to_jiffies(CH348_CMD_TIMEOUT)))
> > +             dev_err_console(port,
> > +                             "Failed to wait for TX buffer to be fully written out\n");
>
> This would also avoid blocking for extended periods of time on the
> system work queue.
I'm moving this to a timer instead.

If you have any comments on the (new) TX logic then please let me know.

[...]
> You should implement dtr_rts() as well.
This will be the first time we need the "package type" information as
CH348Q only supports CTS/RTS on the first four ports, for the last
four the signals aren't routed outside the package.
I need to see if I have other hardware with CTS/RTS pins to test this.
If not: can we omit hardware flow control in the first upstream
version?


Best regards,
Martin