Message-ID: <2ff4dd60-7579-40ce-a4e5-3ad846659f9c@suse.com>
Date: Tue, 3 Jun 2025 13:57:39 +0200
From: Oliver Neukum <oneukum@...e.com>
To: hsyemail2@...il.com, Johan Hovold <johan@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
 Sheng-Yuan Huang <syhuang3@...oton.com>
Subject: Re: [PATCH v1 1/1] USB: serial: nct_usb_serial: add support for
 Nuvoton USB adapter

Hi,

On 03.06.25 05:20, 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.

I am afraid there are a few issue that will not to be addressed
before this can be merged.

> This driver implements basic TTY operations.
> 
> Signed-off-by: Sheng-Yuan Huang <syhuang3@...oton.com>
> ---
>   drivers/usb/serial/nct_usb_serial.c | 1523 +++++++++++++++++++++++++++
>   1 file changed, 1523 insertions(+)
>   create mode 100644 drivers/usb/serial/nct_usb_serial.c
> 
> diff --git a/drivers/usb/serial/nct_usb_serial.c b/drivers/usb/serial/nct_usb_serial.c
> new file mode 100644
> index 000000000000..424c604229b3
> --- /dev/null
> +++ b/drivers/usb/serial/nct_usb_serial.c

> +/* Index definition */
> +enum {
> +	NCT_VCOM_INDEX_0 = 0,
> +	NCT_VCOM_INDEX_1,
> +	NCT_VCOM_INDEX_2,
> +	NCT_VCOM_INDEX_3,
> +	NCT_VCOM_INDEX_4,
> +	NCT_VCOM_INDEX_5,
> +	NCT_VCOM_INDEX_GLOBAL = 0xF,
> +};

What use is this? A number is a number.

> +/* Command */
> +enum {
> +	NCT_VCOM_GET_NUM_PORTS = 0,
> +	NCT_VCOM_GET_PORTS_SUPPORT,
> +	NCT_VCOM_GET_BAUD,
> +	NCT_VCOM_SET_INIT,
> +	NCT_VCOM_SET_CONFIG,
> +	NCT_VCOM_SET_BAUD,
> +	NCT_VCOM_SET_HCR,
> +	NCT_VCOM_SET_OPEN_PORT,
> +	NCT_VCOM_SET_CLOSE_PORT,
> +	NCT_VCOM_SILENT,
> +	/* Use bulk-in status instead of interrupt-in status */
> +	NCT_VCON_SET_BULK_IN_STATUS,
> +};

No. This is an abuse of enumeration. These are just commands that
happen to use the number space consecutively. These need to be
defines.

> +union nct_vendor_cmd {
> +	struct pkg0 {
> +		u16 index:4;
> +		u16 cmd:8;
> +	} p;
> +	u16 val;
> +} __packed;

This definition is an endianness bug waiting to happen.
If this goes over the wire, it has a defined endianness,
which needs to be declared.

> +#define NCT_HDR_MAGIC		0xA5
> +#define NCT_HDR_MAGIC2		0x5A
> +#define NCT_HDR_MAGIC_STATUS	0x5B
> +
> +struct nct_packet_header {
> +	unsigned int magic:8;
> +	unsigned int magic2:8;
> +	unsigned int idx:4;
> +	unsigned int len:12;
> +} __packed;

Again endianness.

> +/* The definitions are for the feilds of nct_ctrl_msg */
> +#define NCT_VCOM_1_STOP_BIT		0
> +#define NCT_VCOM_2_STOP_BITS		1
> +#define NCT_VCOM_PARITY_NONE		0
> +#define NCT_VCOM_PARITY_ODD		1
> +#define NCT_VCOM_PARITY_EVEN		2
> +#define NCT_VCOM_DL5			0
> +#define NCT_VCOM_DL6			1
> +#define NCT_VCOM_DL7			2
> +#define NCT_VCOM_DL8			3
> +#define NCT_VCOM_DISABLE_FLOW_CTRL	0
> +#define NCT_VCOM_XOFF			1
> +#define NCT_VCOM_RTS_CTS		2
> +union nct_ctrl_msg {
> +	struct pkg1 {
> +		u16 stop_bit:1;
> +		u16 parity:2;
> +		u16 data_len:2;
> +		u16 flow:2;
> +		u16 spd:5;
> +		u16 reserved:4;
> +	} p;
> +	u16 val;
> +} __packed;

At the risk of repeating myself: endianness

> +
> +/* Read from USB control pipe */
> +static int nct_vendor_read(struct usb_interface *intf, union nct_vendor_cmd cmd,
> +			   unsigned char *buf, int size)
> +{
> +	struct device *dev = &intf->dev;
> +	struct usb_device *udev = interface_to_usbdev(intf);
> +	u8 *tmp_buf;
> +	int res;
> +
> +	tmp_buf = kmalloc(NCT_MAX_VENDOR_READ_SIZE, GFP_KERNEL);
> +	if (!tmp_buf)
> +		return -ENOMEM;
> +
> +	if (size > NCT_MAX_VENDOR_READ_SIZE)
> +		dev_err(dev, NCT_DRVNAME ": %s - failed to read [%04x]: over size %d\n",
> +			__func__, cmd.p.cmd, size);

And you just go on and overwrite kernel memory?
If you test for plausibility, do something with the result.


> +static int nct_vendor_write(struct usb_interface *intf, union nct_vendor_cmd cmd, u16 val)
> +{
> +	struct device *dev = &intf->dev;
> +	struct usb_device *udev = interface_to_usbdev(intf);
> +	int res;
> +	u8 *buf_val;

Why is this u8* ?
It should be le16*

> +	buf_val = kmalloc(2, GFP_KERNEL);
> +	if (!buf_val)
> +		return -ENOMEM;
> +
> +	/* Copy data to the buffer for sending */
> +	buf_val[0] = val & 0xff;
> +	buf_val[1] = (val >> 8) & 0xff;

We have macros for that.

> +static u16 nct_set_baud(struct usb_interface *intf, u16 index, unsigned int cflag)
> +{
> +	union nct_ctrl_msg msg;
> +	union nct_vendor_cmd cmd;
> +	u16 i;
> +
> +	msg.val = 0;
> +	cmd.p.cmd = NCT_VCOM_SET_BAUD;
> +	msg.p.spd = NCT_DEFAULT_BAUD;
> +	cmd.p.index = index;
> +	dev_dbg(&intf->dev, NCT_DRVNAME ": %s tty baud: 0x%X\n", __func__,
> +		(cflag & CBAUD));
> +	for (i = 0; i < ARRAY_SIZE(NCT_BAUD_SUP); i++) {
> +		if ((cflag & CBAUD) == NCT_BAUD_SUP[i]) {
> +			msg.p.spd = i;
> +			dev_dbg(&intf->dev,
> +				NCT_DRVNAME ": %s index %d set baud: NCT_BAUD_SUP[%d]=%d\n",
> +				__func__, cmd.p.index, msg.p.spd, NCT_BAUD_SUP[i]);
> +			if (nct_vendor_write(intf, cmd, msg.val))
> +				dev_err(&intf->dev,
> +					NCT_DRVNAME ": %s - Set index: %d speed error\n",
> +					__func__, cmd.p.index);
> +
> +			break;
> +		}

If nothing matches, you do nothing?
> +	}
> +
> +	return msg.p.spd;

So errors are ignored?


> +static int nct_serial_tiocmset(struct tty_struct *tty, unsigned int set,
> +			       unsigned int clear)
> +{
> +	return nct_tiocmset_helper(tty, set, clear);
> +}

Why? Does this function do anything useful?

> +static void nct_rx_throttle(struct tty_struct *tty)
> +{
> +	unsigned int set;
> +	unsigned int clear = 0;

Why?

> +
> +	/* If we are implementing RTS/CTS, control that line */
> +	if (C_CRTSCTS(tty)) {
> +		set = 0;
> +		clear = TIOCM_RTS;
> +		nct_tiocmset_helper(tty, set, clear);
> +	}
> +}
> +
> +static void nct_rx_unthrottle(struct tty_struct *tty)
> +{
> +	unsigned int set;
> +	unsigned int clear = 0;

Why?

> +	/* If we are implementing RTS/CTS, control that line */
> +	if (C_CRTSCTS(tty)) {
> +		set = 0;
> +		set |= TIOCM_RTS;
> +		nct_tiocmset_helper(tty, set, clear);
> +	}
> +}
> +
> +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);
> +
> +	if (tport->write_urb_in_use) {
> +		spin_unlock_irqrestore(&tport->port_lock, flags);
> +		return 0;
> +	}
> +
> +	/* Fill header */
> +	hdr.magic = NCT_HDR_MAGIC;
> +	hdr.magic2 = NCT_HDR_MAGIC2;
> +	hdr.idx = tport->hw_idx; /* The 'hw_idx' is based on 1 */

Endianness.

> +
> +	/* Copy data */
> +	memcpy(port->write_urb->transfer_buffer + sizeof(hdr),
> +	       (const void *)buf, wr_len);
> +
> +	hdr.len = wr_len; /* File filed 'len' of header */

Endiannes

> +static int nct_startup_device(struct usb_serial *serial)
> +{
> +	int ret = 0;
> +	struct nct_serial *serial_priv = usb_get_serial_data(serial);
> +	struct usb_serial_port *port;
> +	unsigned long flags;
> +
> +	/* Be sure this happens exactly once */
> +	spin_lock_irqsave(&serial_priv->serial_lock, flags);
> +
> +	if (serial_priv->device_init) {
> +		spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
> +		return 0;
> +	}
> +	serial_priv->device_init = true;
> +	spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
> +
> +	/* Start reading from bulk in endpoint */
> +	port = serial->port[0];
> +	if (!port->read_urb)
> +		dev_dbg(&port->dev, NCT_DRVNAME ": %s: port->read_urb is null, index=%d\n",
> +			__func__, 0);
> +
> +	ret = usb_submit_urb(port->read_urb, GFP_KERNEL);
> +	if (ret)
> +		dev_err(&port->dev,
> +			NCT_DRVNAME ": %s: usb_submit_urb failed, ret=%d, port=%d\n",
> +			__func__, ret, 0);

Error handling?
> +
> +	/* For getting status from interrupt-in */
> +	if (!serial_priv->status_trans_mode) {
> +		/* Start reading from interrupt pipe */
> +		port = serial->port[0];
> +		ret = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL);
> +		if (ret)
> +			dev_err(&port->dev,
> +				NCT_DRVNAME ": %s: usb_submit_urb(intr) failed, ret=%d, port=%d\n",
> +				__func__, ret, 0);
> +	}
> +	return ret;
> +}
> +
> +static void nct_serial_port_end(struct usb_serial_port *port)
> +{
> +	struct nct_tty_port *tport = usb_get_serial_port_data(port);
> +	struct usb_serial *serial = port->serial;
> +	struct usb_interface *intf = serial->interface;
> +	union nct_ctrl_msg msg;
> +	union nct_vendor_cmd cmd;
> +
> +	/* Send 'Close Port' to the device */
> +	cmd.p.index = (u16)tport->hw_idx;
> +	cmd.p.cmd = NCT_VCOM_SET_CLOSE_PORT;

Endianness


> +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;
> +			/* 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 &&
> +				    hdr->len == 24 && actual_len >= 28) {

Endianness

> +					/*
> +					 * 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 &&
> +			    hdr->idx <= NCT_MAX_NUM_COM_DEVICES &&
> +			    hdr->len <= 512)
> +				break;

Endianness

	Regards
		Oliver

Powered by blists - more mailing lists