On 06.09.22 10:29, Corentin Labbe wrote:
> The CH348 is an USB octo port serial adapter.
> This patch adds support for it.
Hi,
thanks for the driver. Unfortunately there are a few issues.
Comments inline.
Regards
Oliver
> +
> +#define CH348_CTO_D 0x01
> +#define CH348_CTO_R 0x02
> +
> +#define CH348_CTI_C 0x10
> +#define CH348_CTI_DSR 0x20
> +#define CH348_CTI_R 0x40
> +#define CH348_CTI_DCD 0x80
> +
> +#define CH348_LO 0x02
> +#define CH348_LP 0x04
> +#define CH348_LF 0x08
> +#define CH348_LB 0x10
> +
> +#define CMD_W_R 0xC0
> +#define CMD_W_BR 0x80
> +
> +#define CMD_WB_E 0x90
> +#define CMD_RB_E 0xC0
> +
> +#define M_NOR 0x00
> +#define M_HF 0x03
> +
> +#define R_MOD 0x97
> +#define R_IO_D 0x98
> +#define R_IO_O 0x99
> +#define R_IO_I 0x9b
> +#define R_TM_O 0x9c
> +#define R_INIT 0xa1
> +
> +#define R_C1 0x01
> +#define R_C2 0x02
> +#define R_C4 0x04
> +#define R_C5 0x06
> +
> +#define R_II_B1 0x06
> +#define R_II_B2 0x02
> +#define R_II_B3 0x00
> +
> +#define CH348_RX_PORTNUM_OFF 0
> +#define CH348_RX_LENGTH_OFF 1
> +#define CH348_RX_DATA_OFF 2
> +
> +#define CH348_RX_PORT_CHUNK_LENGTH 32
> +#define CH348_RX_PORT_MAX_LENGTH 30
> +
> +#define CH348_TX_PORTNUM_OFF 0
> +#define CH348_TX_LENGTH0_OFF 1
> +#define CH348_TX_LENGTH1_OFF 2
> +#define CH348_TX_DATA_OFF 3
This is, well, horrible. If the device uses a defined package
to transmit data to and from the host, please define a data
structure for it.
An array of u8 and offsets defined as preprocessor constants
is not the clean way to do this.
> +/* Some values came from vendor tree, and we have no meaning for them, this
> + * function simply use them.
> + */
> +static int ch348_do_magic(struct ch348 *ch348, int portnum, u8 action, u8 reg, u8 control)
> +{
> + int ret = 0, len;
> + u8 *buffer;
This should probably also be a defined data structure.
> +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;
> +
> + if (!urb->actual_length) {
> + 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) {
What happens if you get a partial chunk?
> + portnum = buffer[CH348_RX_PORTNUM_OFF];
> + if (portnum >= CH348_MAXPORT) {
> + dev_warn(&port->dev, "%s:%d invalid port %d\n",
> + __func__, __LINE__, portnum);
> + break;
> + }
> +
> + usblen = buffer[CH348_RX_LENGTH_OFF];
> + if (usblen > 30) {
> + 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, &buffer[CH348_RX_DATA_OFF], usblen);
> + tty_flip_buffer_push(&port->port);
> + port->icount.rx += usblen;
> + usb_serial_debug_data(&port->dev, __func__, usblen, &buffer[CH348_RX_DATA_OFF]);
> + }
> +}
> +
> +static int ch348_prepare_write_buffer(struct usb_serial_port *port, void *dest, size_t size)
> +{
> + u8 *buf = dest;
> + int count;
> +
> + count = kfifo_out_locked(&port->write_fifo, buf + CH348_TX_DATA_OFF,
> + size - CH348_TX_DATA_OFF,
> + &port->lock);
> +
> + buf[CH348_TX_PORTNUM_OFF] = port->port_number;
> + buf[CH348_TX_LENGTH0_OFF] = count;
> + buf[CH348_TX_LENGTH1_OFF] = count >> 8;
We have macros for this conversion.
> +static void ch348_status_read_bulk_callback(struct urb *urb)
> +{
> + struct ch348 *ch348 = urb->context;
> + u8 *data = urb->transfer_buffer;
> + unsigned int len = urb->actual_length;
> + int ret;
> +
> + switch (urb->status) {
> + case 0:
> + /* success */
> + break;
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + /* this urb is terminated, clean up */
> + dev_dbg(&ch348->udev->dev, "%s - urb shutting down: %d\n",
> + __func__, urb->status);
> + return;
> + default:
> + dev_dbg(&ch348->udev->dev, "%s - nonzero urb status: %d\n",
> + __func__, urb->status);
> + goto exit;
> + }
> +
> + usb_serial_debug_data(&ch348->udev->dev, __func__, len, data);
> + ch348_update_status(ch348, data, len);
> +
> +exit:
> + ret = usb_submit_urb(urb, GFP_ATOMIC);
> + if (ret && urb->status == 0) {
Why check urb->status?
> + dev_err(&ch348->udev->dev, "%s - usb_submit_urb failed: %d\n",
> + __func__, ret);
> + }
> +}
> +
> +static int ch348_allocate_status_read(struct ch348 *ch348, struct usb_endpoint_descriptor *epd)
> +{
> + int buffer_size = usb_endpoint_maxp(epd);
> +
> + ch348->status_read_urb = usb_alloc_urb(0, GFP_KERNEL);
> + if (!ch348->status_read_urb)
> + return -ENOMEM;
> + ch348->status_read_buffer = kmalloc(buffer_size, GFP_KERNEL);
> + if (!ch348->status_read_buffer)
> + return -ENOMEM;
> +
> + usb_fill_bulk_urb(ch348->status_read_urb, ch348->udev,
> + ch348->statusrx_endpoint, ch348->status_read_buffer,
> + buffer_size, ch348_status_read_bulk_callback, ch348);
> +
> + return 0;
> +}
> +
> +static void ch348_release(struct usb_serial *serial)
> +{
> + struct ch348 *ch348 = usb_get_serial_data(serial);
> +
> + usb_kill_urb(ch348->status_read_urb);
> + usb_free_urb(ch348->status_read_urb);
> +}
> +
> +static int ch348_probe(struct usb_serial *serial, const struct usb_device_id *id)
> +{
> + struct usb_interface *data_interface;
> + struct usb_endpoint_descriptor *epcmdwrite = NULL;
> + struct usb_endpoint_descriptor *epstatusread = NULL;
> + struct usb_endpoint_descriptor *epread = NULL;
> + struct usb_endpoint_descriptor *epwrite = NULL;
> + struct usb_device *usb_dev = serial->dev;
> + struct ch348 *ch348;
> + int ret;
> +
> + data_interface = usb_ifnum_to_if(usb_dev, 0);
> +
> + epread = &data_interface->cur_altsetting->endpoint[0].desc;
> + epwrite = &data_interface->cur_altsetting->endpoint[1].desc;
> + epstatusread = &data_interface->cur_altsetting->endpoint[2].desc;
> + epcmdwrite = &data_interface->cur_altsetting->endpoint[3].desc;
Please check that these endpoints exist.
> +
> + ch348 = devm_kzalloc(&serial->dev->dev, sizeof(*ch348), GFP_KERNEL);
> + if (!ch348)
> + return -ENOMEM;
> +
> + usb_set_serial_data(serial, ch348);
> +
> + ch348->readsize = usb_endpoint_maxp(epread);
> + ch348->writesize = usb_endpoint_maxp(epwrite);
> + ch348->udev = serial->dev;
> +
> + spin_lock_init(&ch348->status_lock);
> +
> + ch348->rx_endpoint = usb_rcvbulkpipe(usb_dev, epread->bEndpointAddress);
> + ch348->tx_endpoint = usb_sndbulkpipe(usb_dev, epwrite->bEndpointAddress);
> + ch348->cmdtx_endpoint = usb_sndbulkpipe(usb_dev, epcmdwrite->bEndpointAddress);
> + ch348->statusrx_endpoint = usb_rcvbulkpipe(usb_dev, epstatusread->bEndpointAddress);
> +
> + ret = ch348_allocate_status_read(ch348, epstatusread);
> + if (ret)
> + return ret;
This leaks an URB in the error case. Either fix it up here or in
ch348_allocate_status_read().
