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
