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

From: Oliver Neukum
Date: Mon Oct 24 2022 - 05:40:30 EST




On 21.10.22 15:37, Corentin Labbe wrote:
The CH348 is an USB octo port serial adapter.
This patch adds support for it.


Hi,

comments inline.

+struct ch348_rxbuf {
+ u8 port;
+ u8 length;
+ u8 data[];
+} __packed;
+
+#define CH348_RX_PORT_CHUNK_LENGTH 32
+#define CH348_RX_PORT_MAX_LENGTH 30
+
+struct ch348_txbuf {
+ u8 port;
+ __le16 length;
+ u8 data[];
+} __packed;
+

You know how long data will be. Why leave it unspecified?

+static void ch348_process_read_urb(struct urb *urb)
+{
+ struct usb_serial_port *port = urb->context;
+ struct ch348 *ch348 = usb_get_serial_data(port->serial);
+ u8 *buffer = urb->transfer_buffer, *end;
+ unsigned int portnum, usblen;
+ struct ch348_rxbuf *rxb;
+
+ if (!urb->actual_length) {

That check needs to be for < 2 or you can process garbage.

+ dev_warn(&port->dev, "%s:%d empty rx buffer\n", __func__, __LINE__);
+ return;
+ }
+
+ end = buffer + urb->actual_length;
+
+ for (; buffer < end; buffer += CH348_RX_PORT_CHUNK_LENGTH) {
+ rxb = (struct ch348_rxbuf *)buffer;
+ portnum = rxb->port;
+ if (portnum >= CH348_MAXPORT) {
+ dev_warn(&port->dev, "%s:%d invalid port %d\n",
+ __func__, __LINE__, portnum);
+ break;
+ }
+
+ usblen = rxb->length;
+ if (usblen > 30) {

You have defined a nummerical constant for that. Use it.

+ dev_warn(&port->dev, "%s:%d invalid length %d for port %d\n",
+ __func__, __LINE__, usblen, portnum);
+ break;
+ }
+
+ port = ch348->ttyport[portnum].port;
+ tty_insert_flip_string(&port->port, rxb->data, usblen);
+ tty_flip_buffer_push(&port->port);
+ port->icount.rx += usblen;
+ usb_serial_debug_data(&port->dev, __func__, usblen, rxb->data);
+ }
+}
+
+static int ch348_prepare_write_buffer(struct usb_serial_port *port, void *dest, size_t size)
+{
+ struct ch348_txbuf *rxt = dest;
+ const size_t txhdrsize = offsetof(struct ch348_txbuf, data);

What is that? This is a constant.

+ int count;
+
+ count = kfifo_out_locked(&port->write_fifo, rxt->data,
+ size - txhdrsize, &port->lock);
+
+ rxt->port = port->port_number;
+ rxt->length = cpu_to_le16(count);
+
+ return count + txhdrsize;
+}

+static void ch348_set_termios(struct tty_struct *tty, struct usb_serial_port *port,
+ const struct ktermios *termios_old)
+{
+ struct ch348 *ch348 = usb_get_serial_data(port->serial);
+ int portnum = port->port_number;
+ struct ktermios *termios = &tty->termios;
+ int ret, sent;
+ __le32 dwDTERate;

OK it is LE.

+ u8 bCharFormat;
+ struct ch348_initbuf *buffer;
+
+ if (termios_old && !tty_termios_hw_change(&tty->termios, termios_old))
+ return;
+
+ buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
+ if (!buffer) {
+ if (termios_old)
+ tty->termios = *termios_old;
+ return;
+ }
+
+ dwDTERate = tty_get_baud_rate(tty);

This is speed_t, in other words unsigned int without specified endianness.

+ /* test show no success on low baud and datasheet said it is not supported */
+ if (dwDTERate < 1200)
+ dwDTERate = DEFAULT_BAUD_RATE;
+ /* datasheet said it is not supported */
+ if (dwDTERate > 6000000)
+ dwDTERate = 6000000;

You are comparing to constants in native endianness.

+
+ bCharFormat = termios->c_cflag & CSTOPB ? 2 : 1;
+
+ buffer->bParityType = termios->c_cflag & PARENB ?
+ (termios->c_cflag & PARODD ? 1 : 2) +
+ (termios->c_cflag & CMSPAR ? 2 : 0) : 0;
+
+ switch (termios->c_cflag & CSIZE) {
+ case CS5:
+ buffer->bDataBits = 5;
+ break;
+ case CS6:
+ buffer->bDataBits = 6;
+ break;
+ case CS7:
+ buffer->bDataBits = 7;
+ break;
+ case CS8:
+ default:
+ buffer->bDataBits = 8;
+ break;
+ }
+ buffer->cmd = CMD_WB_E | (portnum & 0x0F);
+ buffer->reg = R_INIT;
+ buffer->port = portnum;
+ buffer->dwDTERate = cpu_to_be32(le32_to_cpu(dwDTERate));

So it is native, not LE?

[..]

+
+static int ch348_fixup_port_bulk_in(struct ch348 *ch348, struct usb_serial_port *port)
+{
+ int i;
+
+ /* Already Initialized */
+ if (port->bulk_in_size) {

BTW, shouldn't these be unsigned int?
[..]

Regards
Oliver