Message-ID: <3e1baa67-2624-4e5d-85a7-57b6cdf619e7@suse.com>
Date: Mon, 23 Jun 2025 12:32:49 +0200
From: Oliver Neukum <oneukum@...e.com>
To: hsyemail2@...il.com, Johan Hovold <johan@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
 Sheng-Yuan Huang <syhuang3@...oton.com>
Subject: Re: [PATCH v3 1/1] USB: serial: nct_usb_serial: add support for
 Nuvoton USB adapter

Hi,

On 23.06.25 09:17, hsyemail2@...il.com wrote:
> From: Sheng-Yuan Huang <syhuang3@...oton.com>
> 
> Add support for the Nuvoton USB-to-serial adapter, which provides
> multiple serial ports over a single USB interface.
> 
> The device exposes one control endpoint, one bulk-in endpoint, and
> one bulk-out endpoint for data transfer. Port status is reported via
> an interrupt-in or bulk-in endpoint, depending on device configuration.

A few issues left I am afraid.
Comments on them inline.

	Regards
		Oliver

> +/* Index definition */
> +#define NCT_VCOM_INDEX_0	0
> +#define NCT_VCOM_INDEX_1	1
> +#define NCT_VCOM_INDEX_2	2
> +#define NCT_VCOM_INDEX_3	3
> +#define NCT_VCOM_INDEX_4	4
> +#define NCT_VCOM_INDEX_5	5

Why? These make no sense.

> +#define NCT_VCOM_INDEX_GLOBAL	0xF
> +
> +/* Command */
> +#define NCT_VCOM_GET_NUM_PORTS		0
> +#define NCT_VCOM_GET_PORTS_SUPPORT	1
> +#define NCT_VCOM_GET_BAUD		2
> +#define NCT_VCOM_SET_INIT		3
> +#define NCT_VCOM_SET_CONFIG		4
> +#define NCT_VCOM_SET_BAUD		5
> +#define NCT_VCOM_SET_HCR		6
> +#define NCT_VCOM_SET_OPEN_PORT		7
> +#define NCT_VCOM_SET_CLOSE_PORT		8
> +#define NCT_VCOM_SILENT			9
> +/* Use bulk-in status instead of interrupt-in status */
> +#define NCT_VCON_SET_BULK_IN_STATUS	10
> +
> +struct nct_vendor_cmd {
> +	__le16 val;	/* bits[3:0]: index, bits[11:4]: cmd, bits[15:12]: reserved */
> +};
> +
> +#define NCT_CMD_INDEX_MASK	0x000F
> +#define NCT_CMD_CMD_MASK	0x0FF0
> +#define NCT_CMD_CMD_SHIFT	4
> +
> +static inline __le16 nct_build_cmd(__u8 cmd_code, __u8 index)
> +{
> +	return cpu_to_le16((cmd_code << NCT_CMD_CMD_SHIFT) | (index & NCT_CMD_INDEX_MASK));

This may be picking nits, but it seems to me that cmd_code is u8.
Hence cmd_code << NCT_CMD_CMD_SHIFT) would also be u8 and the operation
may overflow. You better cast cmd_code to u16.

> +static u16 nct_set_baud(struct usb_interface *intf, u16 index, unsigned int cflag, bool *found)
> +{
> +	struct nct_vendor_cmd cmd;
> +	struct nct_ctrl_msg msg;
> +	u16 i;
> +	u8 spd = NCT_DEFAULT_BAUD;
> +
> +	*found = false;
> +	cmd.val = nct_build_cmd(NCT_VCOM_SET_BAUD, index);
> +	dev_dbg(&intf->dev, "tty baud: 0x%X\n", (cflag & CBAUD));
> +	for (i = 0; i < ARRAY_SIZE(NCT_BAUD_SUP); i++) {
> +		if ((cflag & CBAUD) == NCT_BAUD_SUP[i]) {
> +			spd = i;
> +			dev_dbg(&intf->dev, "index %d set baud: NCT_BAUD_SUP[%d]=%d\n",
> +				index, spd, NCT_BAUD_SUP[i]);
> +			/*
> +			 * Create control message
> +			 * Note: The NCT_VCOM_SET_BAUD only set the baud rate
> +			 */
> +			msg.val = nct_build_ctrl_msg(0, 0, 0, 0, spd);
> +			if (nct_vendor_write(intf, cmd, le16_to_cpu(msg.val)))
> +				dev_err(&intf->dev, "%s - Set index: %d speed error\n",
> +					__func__, index);
> +			else
> +				*found = true;
> +
> +			break;
> +		}
> +	}
> +
> +	if (!*found)
> +		dev_warn(&intf->dev, "Unsupported baud rate 0x%X requested\n", (cflag & CBAUD));

This is problematic. There are two reasons for this to trigger

1. no match
2. IO error in nct_vendor_write()

If the second case happens you nevertheless claim the first cause
I'd just drop the warning. Better nothing than something misleading.
  
> +
> +	return spd;
> +}


> +static int nct_tiocmset_helper(struct tty_struct *tty, unsigned int set,
> +			       unsigned int clear)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +	struct nct_tty_port *tport = usb_get_serial_port_data(port);
> +	struct usb_serial *serial = port->serial;
> +	struct usb_interface *intf = serial->interface;
> +	struct nct_ctrl_msg msg;
> +	struct nct_vendor_cmd cmd;
> +	unsigned long flags;
> +	u8 hcr = 0;
> +
> +	if (set & TIOCM_RTS)
> +		hcr |= NCT_HCR_RTS;
> +	if (set & TIOCM_DTR)
> +		hcr |= NCT_HCR_DTR;
> +	if (clear & TIOCM_RTS)
> +		hcr &= ~NCT_HCR_RTS;
> +	if (clear & TIOCM_DTR)
> +		hcr &= ~NCT_HCR_DTR;
> +
> +	cmd.val = nct_build_cmd(NCT_VCOM_SET_HCR, tport->hw_idx);
> +	msg.val = cpu_to_le16(hcr);
> +
> +	spin_lock_irqsave(&tport->port_lock, flags);

No need for irqsave. A simple irq version will do.
Using irqsave is misleading, because we know that this
function can sleep.

> +	tport->hcr = hcr;
> +	spin_unlock_irqrestore(&tport->port_lock, flags);
> +
> +	dev_dbg(&intf->dev,
> +		"index/cmd/val(hcr)=%X, %X, %X [RTS=%X, DTR=%X]\n",
> +		nct_get_cmd_index(cmd.val), nct_get_cmd_cmd(cmd.val),
> +		le16_to_cpu(msg.val), hcr & NCT_HCR_RTS, hcr & NCT_HCR_DTR);
> +
> +	return nct_vendor_write(intf, cmd, le16_to_cpu(msg.val));
> +}


> +static int nct_serial_write_data(struct tty_struct *tty, struct usb_serial_port *port,
> +				 const unsigned char *buf, int count)
> +{
> +	int ret;
> +	unsigned long flags;
> +	struct nct_packet_header hdr;
> +	int wr_len;
> +	struct nct_tty_port *tport = usb_get_serial_port_data(port);
> +
> +	wr_len = min((unsigned int)count, NCT_MAX_SEND_BULK_SIZE - sizeof(hdr));
> +
> +	if (!wr_len)
> +		return 0;
> +
> +	spin_lock_irqsave(&tport->port_lock, flags);
> +

[..]

> +	/* Fill header */
> +	hdr.magic = NCT_HDR_MAGIC;
> +	hdr.magic2 = NCT_HDR_MAGIC2;
> +	nct_set_hdr_idx_len(&hdr, tport->hw_idx, wr_len); /* The 'hw_idx' is based on 1 */
> +
> +	/* Copy data */
> +	memcpy(port->write_urb->transfer_buffer + sizeof(hdr),
> +	       (const void *)buf, wr_len);
> +
> +	/* Filled urb data */
> +	memcpy(port->write_urb->transfer_buffer, (const void *)&hdr,
> +	       sizeof(hdr)); /* Copy header after filling all other fields */

You are copying the header in unconditionally ...

> +
> +	/* Set urb length(Total length) */
> +	port->write_urb->transfer_buffer_length = wr_len + sizeof(hdr);
> +
> +	port->write_urb->transfer_flags |= URB_ZERO_PACKET;
> +
> +	ret = usb_submit_urb(port->write_urb, GFP_ATOMIC);
> +	if (ret < 0) {
> +		dev_err(&port->dev,
> +			"%s: usb_submit_urb failed, ret=%d, hw_idx=%d\n",
> +			__func__, ret, tport->hw_idx);
> +	} else {
> +		tport->write_urb_in_use = true; /* Set it as busy */
> +		ret = wr_len + sizeof(hdr);
> +	}
> +
> +	spin_unlock_irqrestore(&tport->port_lock, flags);
> +
> +	if (ret > sizeof(hdr))
> +		ret = ret - sizeof(hdr);

... and here you check?

This needs an explanation. A very good explanation.

> +
> +	dev_dbg(&port->dev, "returning %d\n", ret);
> +	return ret;
> +}
> +
> +static int nct_serial_write(struct tty_struct *tty, struct usb_serial_port *port,
> +			    const unsigned char *buf, int count)
> +{
> +	struct nct_tty_port *tport = usb_get_serial_port_data(port);
> +
> +	if (!port) {
> +		pr_err("%s: port is NULL!\n", __func__);
> +		return -EIO;
> +	}
> +	if (!port->write_urb) {
> +		dev_err(&port->dev, "%s: write_urb not initialized!\n",	__func__);
> +		return -EIO;
> +	}
> +	if (!port->write_urb->transfer_buffer) {
> +		dev_err(&port->dev, "%s: transfer_buffer not initialized!\n", __func__);
> +		return -EIO;
> +	}

Can these errors really happen?

> +
> +	/* Flow control */
> +	if (tty_port_cts_enabled(tty->port))
> +		if (tport->flow_stop_wrt)
> +			return 0;
> +
> +	return nct_serial_write_data(tty, port, buf, count);
> +}


> +static int nct_open(struct tty_struct *tty, struct usb_serial_port *port)
> +{
> +	struct nct_vendor_cmd cmd;
> +	struct nct_ctrl_msg msg;
> +	struct nct_tty_port *tport = usb_get_serial_port_data(port);
> +	struct usb_serial *serial = port->serial;
> +	struct nct_serial *serial_priv = usb_get_serial_data(serial);
> +	struct usb_interface *intf = serial->interface;
> +
> +	if (!port->serial)
> +		return -ENXIO;
> +
> +	/* Allocate write_urb */
> +	if (!port->write_urb) {
> +		port->write_urb = usb_alloc_urb(0, GFP_KERNEL);
> +		if (!port->write_urb) {
> +			dev_err(&port->dev, "%s: Failed to allocate write URB\n", __func__);
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	/* Allocate bulk_out_buffer */
> +	port->write_urb->transfer_buffer = kmalloc(NCT_MAX_SEND_BULK_SIZE, GFP_KERNEL);

Can use kzalloc()

> +	if (!port->write_urb->transfer_buffer) {
> +		usb_free_urb(port->write_urb);
> +		port->write_urb = NULL;
> +		return -ENOMEM;
> +	}
> +
> +	/* Clear(init) buffer */
> +	memset(port->write_urb->transfer_buffer, 0, NCT_MAX_SEND_BULK_SIZE);
> +
> +	/* Set write_urb */
> +	usb_fill_bulk_urb(port->write_urb, serial->dev,
> +			  usb_sndbulkpipe(serial->dev, serial_priv->bulk_out_ep->bEndpointAddress),
> +			  port->write_urb->transfer_buffer, NCT_MAX_SEND_BULK_SIZE,
> +			  nct_write_bulk_callback, port);
> +
> +	/* Be sure the device is started up */
> +	if (nct_startup_device(port->serial) != 0)
> +		return -ENXIO;
> +
> +	cmd.val = nct_build_cmd(NCT_VCOM_SET_OPEN_PORT, tport->hw_idx);
> +	msg.val = cpu_to_le16(0);
> +	nct_vendor_write(intf, cmd, le16_to_cpu(msg.val));

This can fail.

> +	/*
> +	 * Delay 1ms for firmware to configure hardware after opening the port.
> +	 * (Especially at high speed)
> +	 */
> +	usleep_range(1000, 2000);
> +	return 0;
> +}
> +
> +static void nct_close(struct usb_serial_port *port)
> +{
> +	struct nct_tty_port *tport = usb_get_serial_port_data(port);
> +	unsigned long flags;
> +
> +	mutex_lock(&port->serial->disc_mutex);
> +	/* If disconnected, don't send the close-command to the firmware */
> +	if (port->serial->disconnected)

We are disconnected ...
> +		goto exit;
> +
> +	nct_serial_port_end(port);
> +
> +exit:
> +	/* Shutdown any outstanding bulk writes */
> +	usb_kill_urb(port->write_urb);

... so this is either useless, or a bug has already happened.

> +
> +	/* Free transfer_buffer */
> +	kfree(port->write_urb->transfer_buffer);
> +	port->write_urb->transfer_buffer = NULL;
> +
> +	if (tport) {
> +		spin_lock_irqsave(&tport->port_lock, flags);
> +		tport->write_urb_in_use = false;
> +		spin_unlock_irqrestore(&tport->port_lock, flags);
> +	}
> +
> +	mutex_unlock(&port->serial->disc_mutex);
> +}
> +

> +static void nct_usb_serial_read(struct urb *urb)
> +{
> +	struct usb_serial_port *port =  urb->context;
> +	struct usb_serial *serial = port->serial;
> +	struct usb_interface *intf = serial->interface;
> +	struct nct_serial *serial_priv = usb_get_serial_data(serial);
> +	struct nct_tty_port *tport;
> +	struct nct_packet_header *hdr = NULL;
> +	unsigned char *data = urb->transfer_buffer;
> +	int i, j;
> +	int actual_len = urb->actual_length;
> +	int len = 0;
> +	struct nct_port_status *nps;
> +	unsigned long flags;
> +
> +	if (!urb->actual_length)
> +		return;
> +
> +again:
> +	spin_lock_irqsave(&serial_priv->serial_lock, flags);
> +	tport = serial_priv->cur_port;
> +	if (!tport) {
> +		/*
> +		 * Handle a new data package (i.e., it is not
> +		 * the remaining data without a header).
> +		 * The package does not need to be combined this time.
> +		 */
> +
> +		for (i = 0; i < urb->actual_length; i++) {
> +			hdr = (struct nct_packet_header *)data;

How do you know that there is enough data for struct nct_packet_header
left?

> +			/* Decode the header */
> +
> +			if (serial_priv->status_trans_mode) {
> +				/*
> +				 * Status data is also transmitted via bulk-in
> +				 * pipe.
> +				 */
> +				if (hdr->magic == NCT_HDR_MAGIC &&
> +				    hdr->magic2 == NCT_HDR_MAGIC_STATUS &&
> +				    nct_get_hdr_len(hdr) == 24 && actual_len >= 28) {
> +					/*
> +					 * Notice: actual_len will be decreased,
> +					 * it is equal to urb->actual_length
> +					 * only at the beginning.
> +					 */
> +
> +					/*
> +					 * Status report.
> +					 * It should be a standalone package in
> +					 * one URB
> +					 */
> +					data += sizeof(struct nct_packet_header);
> +					actual_len -= sizeof(struct nct_packet_header);
> +
> +					nps = (struct nct_port_status *)data;
> +
> +					for (j = 0; j < actual_len - 4; j++) {
> +						nct_update_status(serial, (unsigned char *)nps);
> +						nps++;
> +					}
> +
> +					spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
> +					return;
> +				}
> +			}
> +
> +			if (hdr->magic == NCT_HDR_MAGIC &&
> +			    hdr->magic2 == NCT_HDR_MAGIC2 &&
> +			    nct_get_hdr_idx(hdr) <= NCT_MAX_NUM_COM_DEVICES &&
> +			    nct_get_hdr_len(hdr) <= 512)
> +				break;
> +
> +			data++;
> +			actual_len--;
> +			if (!actual_len) {
> +				dev_err(&intf->dev, "%s: Decode serial packet size failed.\n",
> +					__func__);
> +				spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
> +				return;
> +			}
> +		}
> +		/*
> +		 * Search tty port
> +		 * Search the tty device by the idx in header, and check if
> +		 * it is registered or opened.
> +		 * If it is, record them. The record will be used later for
> +		 * 2 purposes:
> +		 *   (1) If the current data package is incomplete, the following
> +		 *       incoming data will not include a header.
> +		 *   (2) To determine which device will be used for transmission.
> +		 */
> +		tport = NULL;
> +		for (i = 0; i < serial->type->num_ports; i++) {
> +			port = serial->port[i];
> +			tport = usb_get_serial_port_data(port);
> +			if (tport->hw_idx != nct_get_hdr_idx(hdr))
> +				continue;
> +
> +			break;
> +		}
> +		if (!tport) {
> +			dev_err(&intf->dev, "%s: Decode serial packet index failed.\n",	__func__);
> +			spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
> +			return;
> +		}
> +		/*
> +		 * Calculate the data length.
> +		 * Then, check if the length specified in the header matches
> +		 * the data length. If not, it indicates that the data we
> +		 * received spans across two (or more) packets.
> +		 */
> +		actual_len -= sizeof(struct nct_packet_header);
> +		data += sizeof(struct nct_packet_header);
> +		/* actual_len: the data length of the data we got this time */
> +		if (nct_get_hdr_len(hdr) > actual_len) {
> +			/*
> +			 * It means the length specified in the header (the
> +			 * custom header) is greater than the length of the
> +			 * data we received.
> +			 * Therefore, the data we received this time does not
> +			 * span across another packet (i.e. no new header).
> +			 */
> +			len = actual_len;
> +			/*
> +			 * cur_len: Record how many data does not handle yet
> +			 */
> +			serial_priv->cur_len = nct_get_hdr_len(hdr) - len;
> +			/*
> +			 * Record the current port. When we got remained data of
> +			 * the package next time
> +			 */
> +			serial_priv->cur_port = tport;
> +		} else {
> +			/*
> +			 * The data we got crosses packages(not belong
> +			 * to the same header). We only handle data by
> +			 * the length in header. And we will handle
> +			 * another package when 'goto "again" '.
> +			 */
> +			len = nct_get_hdr_len(hdr);
> +		}
> +	} else { /*  Handling the remained data which crosses package */
> +		if (serial_priv->cur_len > actual_len) {
> +			/*
> +			 * The unhandled part of the data exceeds the data we
> +			 * received this time. We only handle the data we
> +			 * have, expecting more data to be received later.
> +			 */
> +			len = actual_len;
> +		} else {
> +			/*
> +			 * This means the package has been fully handled.
> +			 * Clear 'cur_port' as no additional data needs to be
> +			 * attached to the current package.
> +			 */
> +			len = serial_priv->cur_len;
> +			serial_priv->cur_port = NULL;
> +		}
> +		serial_priv->cur_len -= len;
> +	}
> +	spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
> +	/*
> +	 * The per-character sysrq handling is too slow for fast devices,
> +	 * so we bypass it in the vast majority of cases where the USB serial is
> +	 * not a console.
> +	 */
> +	if (tport->sysrq) {
> +		for (i = 0; i < len; i++, data++)
> +			tty_insert_flip_char(&port->port, *data, TTY_NORMAL);
> +	} else {
> +		tty_insert_flip_string(&port->port, data, len);
> +		data += len;
> +	}
> +	/*
> +	 * Send data to the tty device (according to the port identified above).
> +	 */
> +	tty_flip_buffer_push(&port->port);
> +	actual_len -= len;
> +
> +	/*
> +	 * It means that the data we received this time contains two or
> +	 * more data packages, so it needs to continue processing the next
> +	 * data packages.
> +	 */
> +	if (actual_len > 0)
> +		goto again;
> +}

Powered by blists - more mailing lists