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

[Oliver Neukum <oneukum@xxxxxxxx>]

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