Re: [PATCH v1 1/1] USB: serial: nct_usb_serial: add support for Nuvoton USB adapter
From: Oliver Neukum
Date: Tue Jun 03 2025 - 07:59:16 EST
Hi,
On 03.06.25 05:20, hsyemail2@xxxxxxxxx wrote:
From: Sheng-Yuan Huang <syhuang3@xxxxxxxxxxx>
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@xxxxxxxxxxx>
---
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