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

[Corentin LABBE <clabbe@baylibre.com>]

Le Mon, Oct 24, 2022 at 11:40:16AM +0200, Oliver Neukum a écrit :
> 
> 
> 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?
> 

Hello

Thanks for your review, I will fix them all.

> [..]
> 
> > +
> > +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?

I dont understand what you mean here.

Regards