Re: [PATCH v6] USB: serial: nct_usb_serial: add support for Nuvoton USB adapter
Sheng-Yuan Huang <[email protected]>
| Newsgroups | org.kernel.vger.linux-usb,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <CACW=pY69QdcZ8Su0t4e2Ws9kooEkeUV7MG=Z=YSWeLnF9bR+ww@mail.gmail.com> |
Hi Johan, Thanks for the review. Please see my replies below. > You never replied to my previous comments here: > > First, can you even control RTS when the device is in hardware flow > control mode? Thanks for raising this. I confirmed that the chip does not provide automatic hardware flow control, so I think the driver still needs to handle this explicitly. Based on that, I think the existing driver-side handling should be kept for now. > Second, I think you're supposed to just stop reading from the device > here to exert back pressure so that it in turn deasserts RTS. I am not sure that approach would work here, since the chip does not provide automatic hardware flow control. Best regards, Sheng-Yuan Huang > Johan Hovold <[email protected]> 於 2026年5月7日週四 上午12:14寫道： > > On Tue, Mar 17, 2026 at 02:14:46PM +0800, [email protected] wrote: > > From: Sheng-Yuan Huang <[email protected]> > > > > 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. > > > > This driver implements basic TTY operations. > > > > Signed-off-by: Sheng-Yuan Huang <[email protected]> > > --- > > > > v6: > > - Address review comments from Oliver Neukum > > - Thanks to Oliver Neukum and Johan Hovold for their review > > - Narrow the protected scope in nct_tiocmset_helper(): port_lock now > > protects only cached HCR updates, while command transmission is > > serialized separately. > > - Serialize the open transaction to prevent concurrent open calls > > from returning success before startup completes or before the open > > command is sent. > > - Serialize the close transaction to avoid resource races. > > Please include the changelog for all revisions here when resending. > > > drivers/usb/serial/Kconfig | 10 + > > drivers/usb/serial/Makefile | 1 + > > drivers/usb/serial/nct_usb_serial.c | 1455 +++++++++++++++++++++++++++ > > 3 files changed, 1466 insertions(+) > > create mode 100644 drivers/usb/serial/nct_usb_serial.c > > > > diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig > > index ef8d1c73c754..96a022bb9b21 100644 > > --- a/drivers/usb/serial/Kconfig > > +++ b/drivers/usb/serial/Kconfig > > @@ -443,6 +443,16 @@ config USB_SERIAL_NAVMAN > > To compile this driver as a module, choose M here: the > > module will be called navman. > > > > +config USB_SERIAL_NUV_MULTI_UART > > + tristate "USB Nuvoton Multi-Ports Serial Driver" > > + depends on USB_SERIAL > > + help > > + Say Y here if you want to use a Nuvoton Multi-Ports USB to > > + serial converter device > > + > > + To compile this driver as a module, choose M here: the > > + module will be called nct_usb_serial. > > + > > config USB_SERIAL_PL2303 > > tristate "USB Prolific 2303 Single Port Serial Driver" > > help > > diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile > > index c7bb1a88173e..c07919a52076 100644 > > --- a/drivers/usb/serial/Makefile > > +++ b/drivers/usb/serial/Makefile > > @@ -41,6 +41,7 @@ obj-$(CONFIG_USB_SERIAL_MOS7720) += mos7720.o > > obj-$(CONFIG_USB_SERIAL_MOS7840) += mos7840.o > > obj-$(CONFIG_USB_SERIAL_MXUPORT) += mxuport.o > > obj-$(CONFIG_USB_SERIAL_NAVMAN) += navman.o > > +obj-$(CONFIG_USB_SERIAL_NUV_MULTI_UART) += nct_usb_serial.o > > obj-$(CONFIG_USB_SERIAL_OMNINET) += omninet.o > > obj-$(CONFIG_USB_SERIAL_OPTICON) += opticon.o > > obj-$(CONFIG_USB_SERIAL_OPTION) += option.o > > diff --git a/drivers/usb/serial/nct_usb_serial.c b/drivers/usb/serial/nct_usb_serial.c > > new file mode 100644 > > index 000000000000..89dbfdc92136 > > --- /dev/null > > +++ b/drivers/usb/serial/nct_usb_serial.c > > @@ -0,0 +1,1455 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (C) 2024-2025 Nuvoton Corp > > + * Copyright (C) 2024-2025 Sheng-Yuan Huang <[email protected]> > > + * > > + * Nuvoton USB to serial adapter driver > > + * > > + * This device interface consists of one control endpoint for configuration, > > + * one bulk-out endpoint used for transmitting data for all serial ports, > > + * and one bulk-in endpoint for receiving data from all serial ports. > > + * The status of the ports may be reported via either an interrupt endpoint > > + * or the bulk-in endpoint, depending on the device configuration. > > + * > > + * The number of serial ports is configurable in firmware and reported to > > + * the driver via a vendor command at probe time. > > + * > > + * Host-device handshake sequence: > > + * 1. Host sends SET_INIT to initialize the device. > > + * 2. Host sends GET_PORTS_SUPPORT to query enabled ports (bitmask). > > + * 3. For each port open: Host sends SET_OPEN_PORT. > > + * 4. Host sends SET_CONFIG/SET_BAUD to configure UART parameters. > > + * 5. For each port close: Host sends SET_CLOSE_PORT. > > + */ > > + > > +#include <linux/bitops.h> > > I don't think this one is used. > > > +#include <linux/errno.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > > +#include <linux/moduleparam.h> > > Nor this one. > > > +#include <linux/serial.h> > > Nor this one? > > > +#include <linux/slab.h> > > +#include <linux/spinlock.h> > > +#include <linux/tty.h> > > > +#include <linux/tty_driver.h> > > +#include <linux/tty_port.h> > > These two are typically included via tty.h (and you don't need the first > one either way). > > > +#include <linux/tty_flip.h> > > > +#include <linux/uaccess.h> > > Same here. > > > +#include <linux/usb.h> > > +#include <linux/usb/serial.h> > > +#include <linux/kfifo.h> > > +#include <linux/mutex.h> > > + > > +#define NCT_WRITE_FIFO_SIZE 1024 > > + > > +#define NCT_VENDOR_ID 0x0416 > > +#define NCT_PRODUCT_ID 0x200b > > +#define NCT_USB_CLASS 0xff > > +#define NCT_USB_SUBCLASS 0x0 > > +#define NCT_USB_PROTOCOL 0x1 > > + > > +#define NCT_MAX_VENDOR_READ_SIZE 8 > > + > > +static const struct usb_device_id id_table[] = { > > + {USB_DEVICE_AND_INTERFACE_INFO(NCT_VENDOR_ID, NCT_PRODUCT_ID, > > + NCT_USB_CLASS, NCT_USB_SUBCLASS, > > + NCT_USB_PROTOCOL)}, > > I already asked you why you match on the interface class and protocol > here. Does the device have more than one interface? > > Please post the corresponding output of usb-devices. > > > + {} /* Terminating entry */ > > +}; > > + > > +MODULE_DEVICE_TABLE(usb, id_table); > > + > > +#define NCT_MAX_SEND_BULK_SIZE 128 > > This one is unused. What does it represent and should it be used instead > of removed? > > I also asked you about the meaning of the following defines in v4, but > you never replied: > > > +#define NCT_MAX_PACKAGE_SIZE 4096 /* The max size of one > > writing package */ > > +#define NCT_MAX_BULK_IN_SIZE 512 > > +#define NCT_MAX_BULK_OUT_SIZE 512 > > The last three are not used either. Might they become useful? Can you > expand on their meaning? > > > +/* The port does not exist in FW (for device status). */ > > +#define NCT_EMPTY_PORT 0xff > > + > > +/* Hardware configuration */ > > +#define NCT_MAX_NUM_COM_DEVICES 8 > > + > > +#define NCT_DEFAULT_BAUD 14 /* 115200 */ > > +static const unsigned int NCT_BAUD_SUP[] = { > > + /* It should be the same as FW's baud-rate table. */ > > + B0, B50, B75, B150, B300, B600, B1200, > > + B1800, B2400, B4800, B9600, B19200, B38400, B57600, > > + B115200, B230400, B460800, B921600, B1500000 > > +}; > > + > > +/* USB request */ > > +#define NCT_VENDOR_COM_READ_REQUEST_TYPE 0xc0 > > +#define NCT_VENDOR_COM_WRITE_REQUEST_TYPE 0x40 > > +#define NCT_VENDOR_COM_READ_REQUEST 0x01 > > +#define NCT_VENDOR_COM_WRITE_REQUEST 0x01 > > +/* Index definition */ > > +#define NCT_VCOM_INDEX_GLOBAL 0x0f > > + > > +/* 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 { > > + /* bits[3:0]: index, bits[11:4]: cmd, bits[15:12]: reserved */ > > + __le16 val; > > +}; > > Why you need nct_ctrl_msg at all? It looks like you just keep converting > back forth between host and le16 for no good reason. > > Can't you just use host order > > u16 cmd > > throughout and pass that to usb_control_msg_recv() in > nct_vendor_read/write() (and drop conversions in nct_build_cmd(), > nct_build_ctrl_msg() and nct_vendor_read/write())? > > > + > > +#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(((u16)cmd_code << NCT_CMD_CMD_SHIFT) | > > + (index & NCT_CMD_INDEX_MASK)); > > +} > > + > > +static inline u8 nct_get_cmd_index(__le16 val) > > +{ > > + return le16_to_cpu(val) & NCT_CMD_INDEX_MASK; > > +} > > This one is not used and can be dropped. > > > +static inline u8 nct_get_cmd_cmd(__le16 val) > > +{ > > + return (le16_to_cpu(val) & NCT_CMD_CMD_MASK) >> NCT_CMD_CMD_SHIFT; > > +} > > + > > +#define NCT_HDR_MAGIC 0xa5 > > +#define NCT_HDR_MAGIC2 0x5a > > +#define NCT_HDR_MAGIC_STATUS 0x5b > > + > > +struct nct_packet_header { > > + u8 magic; > > + u8 magic2; > > + __le16 len_and_idx; /* bits[3:0]: idx, bits[15:4]: len */ > > +} __packed; > > + > > +#define NCT_HDR_IDX_MASK 0x000f > > +#define NCT_HDR_LEN_MASK 0xfff0 > > +#define NCT_HDR_LEN_SHIFT 4 > > + > > +static inline void nct_set_hdr_idx_len(struct nct_packet_header *hdr, > > + u8 idx, u16 len) > > +{ > > + hdr->len_and_idx = cpu_to_le16((len << NCT_HDR_LEN_SHIFT) | > > + (idx & NCT_HDR_IDX_MASK)); > > +} > > + > > +static inline u8 nct_get_hdr_idx(const struct nct_packet_header *hdr) > > +{ > > + return le16_to_cpu(hdr->len_and_idx) & NCT_HDR_IDX_MASK; > > +} > > + > > +static inline u16 nct_get_hdr_len(const struct nct_packet_header *hdr) > > +{ > > + return (le16_to_cpu(hdr->len_and_idx) & NCT_HDR_LEN_MASK) >> > > + NCT_HDR_LEN_SHIFT; > > +} > > + > > +/* The definitions are for the fields 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 > > + > > +struct nct_ctrl_msg { > > + __le16 val; > > +}; > > And similar for nct_ctrl_msg. Why not used host order (u16 msg) > throughout and only keep the final conversion to LE in > nct_vendor_write()? > > > +#define NCT_CTRL_STOP_BIT_MASK 0x0001 > > +#define NCT_CTRL_PARITY_MASK 0x0006 > > +#define NCT_CTRL_PARITY_SHIFT 1 > > +#define NCT_CTRL_DATA_LEN_MASK 0x0018 > > +#define NCT_CTRL_DATA_LEN_SHIFT 3 > > +#define NCT_CTRL_FLOW_MASK 0x0060 > > +#define NCT_CTRL_FLOW_SHIFT 5 > > +#define NCT_CTRL_SPD_MASK 0x0f80 > > +#define NCT_CTRL_SPD_SHIFT 7 > > +#define NCT_CTRL_RESERVED_MASK 0xf000 > > +#define NCT_CTRL_RESERVED_SHIFT 12 > > + > > +static inline __le16 nct_build_ctrl_msg(u8 stop_bit, u8 parity, u8 data_len, > > + u8 flow, u8 spd) > > +{ > > + u16 val = 0; > > + > > + val |= (stop_bit & NCT_CTRL_STOP_BIT_MASK); > > + val |= ((parity & (NCT_CTRL_PARITY_MASK >> NCT_CTRL_PARITY_SHIFT)) << > > + NCT_CTRL_PARITY_SHIFT); > > + val |= ((data_len & > > + (NCT_CTRL_DATA_LEN_MASK >> NCT_CTRL_DATA_LEN_SHIFT)) << > > + NCT_CTRL_DATA_LEN_SHIFT); > > + val |= ((flow & (NCT_CTRL_FLOW_MASK >> NCT_CTRL_FLOW_SHIFT)) << > > + NCT_CTRL_FLOW_SHIFT); > > + val |= ((spd & (NCT_CTRL_SPD_MASK >> NCT_CTRL_SPD_SHIFT)) << > > + NCT_CTRL_SPD_SHIFT); > > If you shift the argument before you mask above you can drop the mask > shift. > > > + > > + return cpu_to_le16(val); > > +} > > + > > +#define NCT_USR_RDR 0x01 > > +#define NCT_USR_ORR 0x02 > > +#define NCT_USR_PBER 0x04 > > +#define NCT_USR_NSER 0x08 > > +#define NCT_USR_SBD 0x10 > > +#define NCT_USR_TBRE 0x20 > > +#define NCT_USR_TSRE 0x40 > > +#define NCT_USR_RFEI 0x80 > > +#define NCT_HSR_TCTS 0x01 > > +#define NCT_HSR_TDSR 0x02 > > +#define NCT_HSR_FERI 0x04 > > +#define NCT_HSR_TDCD 0x08 > > +#define NCT_HSR_CTS 0x10 > > +#define NCT_HSR_DSR 0x20 > > +#define NCT_HSR_RI 0x40 > > +#define NCT_HSR_DCD 0x80 > > +#define NCT_HCR_DTR 0x01 > > +#define NCT_HCR_RTS 0x02 > > + > > +#define NCT_UART_STATE_MSR_MASK (NCT_HSR_TCTS | NCT_HSR_TDSR | \ > > + NCT_HSR_TDCD | NCT_HSR_DCD) > > + > > +struct nct_port_status { > > + u8 index; > > + u8 usr; > > + u8 hsr; > > + u8 hcr; > > +}; > > + > > +struct nct_serial { > > + /* Protects the private data in structure 'nct_serial'. */ > > As I mentioned before, such comments doesn't really add any value so you > can drop it. > > > + spinlock_t serial_lock; > > + /* Serializes open/close startup-shutdown transactions. */ > > + struct mutex open_close_mutex; > > Just call it 'mutex'. You only have one. Or at least shorten it to > 'open_mutex'. > > > + unsigned int open_count; > > + > > + /* Reading data information */ > > + struct nct_tty_port *cur_port; > > + int cur_len; > > + > > + bool use_bulk_status; > > + u8 en_device_mask; > > + u8 last_assigned_hw_idx; > > + struct usb_endpoint_descriptor *bulk_out_ep; > > + struct usb_serial_port *port_by_hw_idx[NCT_MAX_NUM_COM_DEVICES]; > > +}; > > + > > +struct nct_tty_port { > > + u8 hw_idx; > > + u8 usr; > > + u8 hsr; > > + u8 hcr; > > + /* > > + * Flow control - stop writing data to device. > > + * 0: Write enable, 1: Stop writing > > + */ > > + bool flow_stop_wrt; > > + > > + /* Serializes modem-control transactions sent to the device. */ > > + struct mutex mctrl_mutex; > > + spinlock_t port_lock; /* Protects the port data. */ > > Please drop the comment, it adds no values. > > > + bool write_urb_in_use; > > +}; > > + > > +/* Functions */ > > + > > +/* Read from USB control pipe */ > > +static int nct_vendor_read(struct usb_interface *intf, > > + struct nct_vendor_cmd cmd, void *buf, > > + int size) > > +{ > > + struct usb_device *udev = interface_to_usbdev(intf); > > + int ret; > > + > > + if (size > NCT_MAX_VENDOR_READ_SIZE) > > + return -EINVAL; > > + > > + ret = usb_control_msg_recv(udev, 0, > > + NCT_VENDOR_COM_READ_REQUEST, > > + NCT_VENDOR_COM_READ_REQUEST_TYPE, > > + le16_to_cpu(cmd.val), > > + intf->cur_altsetting->desc.bInterfaceNumber, > > + buf, size, 100, GFP_KERNEL); > > + > > + return ret; > > +} > > + > > +static int nct_vendor_write(struct usb_interface *intf, > > + struct nct_vendor_cmd cmd, u16 val) > > +{ > > + struct device *dev = &intf->dev; > > + struct usb_device *udev = interface_to_usbdev(intf); > > + __le16 le_val = cpu_to_le16(val); > > + int ret; > > + > > + ret = usb_control_msg_send(udev, 0, > > + NCT_VENDOR_COM_WRITE_REQUEST, > > + NCT_VENDOR_COM_WRITE_REQUEST_TYPE, > > + le16_to_cpu(cmd.val), > > + intf->cur_altsetting->desc.bInterfaceNumber, > > + &le_val, sizeof(le_val), 100, GFP_KERNEL); > > + if (ret) > > + dev_err(dev, "failed to write [%04x]: %d\n", > > + nct_get_cmd_cmd(cmd.val), ret); > > + > > + return ret; > > +} > > + > > +/* > > + * Prepare write buffer by extracting data from core write FIFO and > > + * adding header > > + */ > > +static int nct_prepare_write_buffer(struct usb_serial_port *port, > > + void *dest, size_t size) > > +{ > > + unsigned int count; > > + struct nct_packet_header hdr; > > + struct nct_tty_port *tport = usb_get_serial_port_data(port); > > Nit: Please consider reording these reverse christmas tree style, which > tends to be easier to read (i.e. put the longest lines first). > > > + > > + if (size <= sizeof(hdr)) > > + return 0; > > The buffer will at least be as large as the endpoint max packet size so > this is not needed unless something is really broken. > > You can specify explicit buffer sizes in the driver structure to avoid > this check: > > .bulk_in_size > .bulk_out_size > > and the buffer can be larger than the endpoint (if the device can handle > it). > > If that's not enough, you can add sanity checks to probe. > > > + > > + /* Leave room for header */ > > + count = kfifo_out_locked(&port->write_fifo, dest + sizeof(hdr), > > + size - sizeof(hdr), &port->lock); > > + > > + if (!count) > > + return 0; > > Core has already made sure there is data in the fifo. > > > + > > + hdr.magic = NCT_HDR_MAGIC; > > + hdr.magic2 = NCT_HDR_MAGIC2; > > + nct_set_hdr_idx_len(&hdr, tport->hw_idx, count); > > + > > + /* > > + * Copy header into buffer (use memcpy to avoid aliasing/unaligned > > + * access). > > + */ > > + memcpy(dest, &hdr, sizeof(hdr)); > > And the buffer alignment is at least 8-byte aligned > (ARCH_KMALLOC_MINALIGN) so this is not needed either. > > > + > > + return count + sizeof(hdr); > > +} > > + > > +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); > > + for (i = 0; i < ARRAY_SIZE(NCT_BAUD_SUP); i++) { > > + if ((cflag & CBAUD) != NCT_BAUD_SUP[i]) > > + continue; > > + > > + spd = 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, "set index %d speed error\n", > > + index); > > Please add brackets for readability wherever you have multi-line > statements (as I mentioned before). > > In this case you could also just drop the line break as the resulting > line is short enough. > > > + else > > + *found = true; > > + > > + break; > > + } > > + > > + return spd; > > +} > > + > > +static void nct_set_termios(struct tty_struct *tty, > > + struct usb_serial_port *port, > > + const struct ktermios *old) > > +{ > > + struct nct_tty_port *tport = usb_get_serial_port_data(port); > > + struct usb_serial *serial = port->serial; > > + struct usb_interface *intf = serial->interface; > > + struct ktermios *termios = &tty->termios; > > + struct nct_ctrl_msg msg; > > + struct nct_vendor_cmd cmd; > > + u8 stop_bit; > > + u8 parity; > > + u8 data_len; > > + u8 flow; > > + bool baud_found = false; > > + speed_t old_baud; > > + > > + /* Device does not support CMSPAR. */ > > + termios->c_cflag &= ~CMSPAR; > > + > > + cmd.val = nct_build_cmd(NCT_VCOM_SET_CONFIG, tport->hw_idx); > > + > > + /* Set stop bit */ > > + if (C_CSTOPB(tty)) > > + stop_bit = NCT_VCOM_2_STOP_BITS; > > + else > > + stop_bit = NCT_VCOM_1_STOP_BIT; > > + > > + /* Set parity */ > > + if (C_PARENB(tty)) { > > + if (C_PARODD(tty)) > > + parity = NCT_VCOM_PARITY_ODD; > > + else > > + parity = NCT_VCOM_PARITY_EVEN; > > + } else { > > + parity = NCT_VCOM_PARITY_NONE; > > + } > > As I mentioned before you need to clear CMSPAR in the termios if you > don't support sticky parity. > > > + > > + /* Set data bit length */ > > + switch (C_CSIZE(tty)) { > > + case CS5: > > + data_len = NCT_VCOM_DL5; > > + break; > > + case CS6: > > + data_len = NCT_VCOM_DL6; > > + break; > > + case CS7: > > + data_len = NCT_VCOM_DL7; > > + break; > > + case CS8: > > + default: > > + data_len = NCT_VCOM_DL8; > > + break; > > + } > > + > > + /* Set flow control */ > > + if (C_CRTSCTS(tty)) > > + flow = NCT_VCOM_RTS_CTS; > > + else if (I_IXON(tty)) > > + flow = NCT_VCOM_XOFF; > > + else > > + flow = NCT_VCOM_DISABLE_FLOW_CTRL; > > + > > + tty_port_set_cts_flow(tty->port, C_CRTSCTS(tty)); > > I don't think you need to set this flag as the device appears to support > automatic hardware flow control (NCT_VCOM_RTS_CTS). More below. > > > + /* Create control message (spd=0: speed set separately via SET_BAUD) */ > > + msg.val = nct_build_ctrl_msg(stop_bit, parity, data_len, flow, 0); > > + nct_vendor_write(intf, cmd, le16_to_cpu(msg.val)); > > + > > + /* > > + * Set baud rate if speed changed. > > + * Always call tty_encode_baud_rate() to keep kernel's baud rate > > + * synchronized with the device, especially during initial open > > + * when the device uses its default baud rate. > > + */ > > + if (!old || old->c_ospeed != termios->c_ospeed) > > + nct_set_baud(intf, tport->hw_idx, termios->c_cflag, &baud_found); > > + > > + if (baud_found) { > > + tty_encode_baud_rate(tty, tty_get_baud_rate(tty), > > + tty_get_baud_rate(tty)); > > + } else { > > + if (old && tty_termios_baud_rate(old)) > > + old_baud = tty_termios_baud_rate(old); > > + else > > + old_baud = 115200; /* Sync with device default */ > > + > > + tty_encode_baud_rate(tty, old_baud, old_baud); > > + } > > +} > > + > > +static int nct_break(struct tty_struct *tty, int break_state) > > +{ > > + 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_vendor_cmd cmd; > > + > > + cmd.val = nct_build_cmd(NCT_VCOM_SILENT, tport->hw_idx); > > + > > + return nct_vendor_write(intf, cmd, 0); > > +} > > + > > +static int nct_tiocmset_helper(struct tty_struct *tty, unsigned int set, > > + unsigned int clear) > > Drop the "_helper" suffix. > > > +{ > > + 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; > > + u8 hcr; > > + int ret; > > + > > + mutex_lock(&tport->mctrl_mutex); > > + > > + /* > > + * port_lock serializes updates of the cached modem-control bits (hcr). > > + * The control transfer may sleep, so only the hcr read-modify-write is > > + * done under the spinlock. > > + */ > > + spin_lock_irq(&tport->port_lock); > > + hcr = tport->hcr; > > + > > + 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; > > + > > + tport->hcr = hcr; > > + spin_unlock_irq(&tport->port_lock); > > + > > + cmd.val = nct_build_cmd(NCT_VCOM_SET_HCR, tport->hw_idx); > > + msg.val = cpu_to_le16(hcr); > > + ret = nct_vendor_write(intf, cmd, le16_to_cpu(msg.val)); > > + mutex_unlock(&tport->mctrl_mutex); > > + > > + return ret; > > +} > > + > > +static int nct_tiocmget(struct tty_struct *tty) > > +{ > > + struct usb_serial_port *port = tty->driver_data; > > + struct nct_tty_port *tport = usb_get_serial_port_data(port); > > + unsigned int res; > > + u8 hcr, hsr; > > + > > + spin_lock_irq(&tport->port_lock); > > + hcr = tport->hcr; > > + hsr = tport->hsr; > > + spin_unlock_irq(&tport->port_lock); > > + res = ((hcr & NCT_HCR_DTR) ? TIOCM_DTR : 0) | > > + ((hcr & NCT_HCR_RTS) ? TIOCM_RTS : 0) | > > + ((hsr & NCT_HSR_CTS) ? TIOCM_CTS : 0) | > > + ((hsr & NCT_HSR_DSR) ? TIOCM_DSR : 0) | > > + ((hsr & NCT_HSR_RI) ? TIOCM_RI : 0) | > > + ((hsr & NCT_HSR_DCD) ? TIOCM_CD : 0); > > + > > + return res; > > +} > > + > > +static void nct_rx_throttle(struct tty_struct *tty) > > +{ > > + /* Handle RTS line for RTS/CTS flow control */ > > + if (C_CRTSCTS(tty)) > > + nct_tiocmset_helper(tty, 0, TIOCM_RTS); > > +} > > You never replied to my previous comments here: > > First, can you even control RTS when the device is in hardware > flow control mode? > > Second, I think you're supposed to just stop reading from the > device here to exert back pressure so that it in turn deasserts > RTS. > > But perhaps the latter does not work here as the bulk-in endpoint is > shared. > > > +static void nct_rx_unthrottle(struct tty_struct *tty) > > +{ > > + /* Handle RTS line for RTS/CTS flow control */ > > + if (C_CRTSCTS(tty)) > > + nct_tiocmset_helper(tty, TIOCM_RTS, 0); > > +} > > + > > +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); > > + unsigned int written; > > + int ret; > > + > > + /* Flow control */ > > + if (tty_port_cts_enabled(tty->port)) > > + if (tport->flow_stop_wrt) > > + return 0; > > Similarly here, you should be able to rely on the device to deassert RTS > when its buffers fill up, right? > > I apparently already asked you this in a comment to v4. > > Please make sure to address and/or respond to all comments before > sending another version. > > > + > > + if (!port) > > + return -ENXIO; > > How could port be NULL here? > > > + > > + /* Copy into core write FIFO */ > > + written = kfifo_in_locked(&port->write_fifo, buf, count, &port->lock); > > + > > Nit: drop the newline > > > + if (written) { > > + ret = usb_serial_generic_write_start(port, GFP_ATOMIC); > > + if (ret && ret != -EBUSY) > > + dev_err(&port->dev, > > + "usb_serial_generic_write_start failed: %d\n", > > + ret); > > + } > > + > > + return written; > > +} > > Without the flow control bits you should be able to use the generic > write implementation directly here (or possibly call it after checking). > > > + > > +static unsigned int nct_write_room(struct tty_struct *tty) > > +{ > > + struct usb_serial_port *port = tty->driver_data; > > + struct nct_tty_port *tport = usb_get_serial_port_data(port); > > + unsigned long flags; > > + unsigned int room; > > + > > + spin_lock_irqsave(&tport->port_lock, flags); > > + room = kfifo_avail(&port->write_fifo); > > + spin_unlock_irqrestore(&tport->port_lock, flags); > > + return room; > > +} > > Just use the generic implementation. > > > +static unsigned int nct_chars_in_buffer(struct tty_struct *tty) > > +{ > > + struct usb_serial_port *port = tty->driver_data; > > + unsigned int qlen; > > + > > + /* Get queued bytes in core write FIFO */ > > + qlen = kfifo_len(&port->write_fifo); > > + return qlen; > > +} > > Here too. > > > +/* > > + * Starts reads urb on all ports. It is to avoid potential issues caused by > > + * multiple ports being opened almost simultaneously. > > + * It must be called AFTER startup, with urbs initialized. > > + * Returns 0 if successful, non-zero error otherwise. > > + */ > > +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; > > + bool first_open = false; > > + > > + /* Caller must hold serial_priv->open_close_mutex. */ > > You can use lockdep_assert_held() to document this. > > > + > > + /* Start URBs on first open */ > > + spin_lock_irqsave(&serial_priv->serial_lock, flags); > > + if (serial_priv->open_count++ == 0) > > + first_open = true; > > + spin_unlock_irqrestore(&serial_priv->serial_lock, flags); > > Why do you take the serial_lock spinlock for this when you already hold > the open mutex? > > > + /* Only the first open submits read_urb and, if needed, interrupt_in_urb. */ > > + if (!first_open) > > + goto out_return; > > Just return directly (and skip ret initialisation above). > > > + > > + /* Start reading from bulk in endpoint */ > > + port = serial->port[0]; > > + ret = usb_submit_urb(port->read_urb, GFP_KERNEL); > > + if (ret) { > > + dev_err(&port->dev, "failed to submit read urb: %d\n", ret); > > + goto err_rollback; > > + } > > Why not use both URBs which is more efficient (i.e. > usb_serial_generic_submit_read_urbs())? > > > + > > + /* For getting status from interrupt-in */ > > + if (!serial_priv->use_bulk_status) { > > + /* 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, > > + "failed to submit interrupt urb: %d\n", > > + ret); > > + goto err_kill_read; > > + } > > + } > > + > > + ret = 0; > > + goto out_return; > > Just return explicit zero here. > > > + > > +err_kill_read: > > + usb_kill_urb(serial->port[0]->read_urb); > > +err_rollback: > > + spin_lock_irqsave(&serial_priv->serial_lock, flags); > > + if (serial_priv->open_count) > > + serial_priv->open_count--; > > + if (!serial_priv->open_count) { > > + serial_priv->cur_port = NULL; > > + serial_priv->cur_len = 0; > > + } > > + spin_unlock_irqrestore(&serial_priv->serial_lock, flags); > > + > > +out_return: > > + return ret; > > +} > > + > > +static void nct_shutdown_device(struct usb_serial *serial) > > +{ > > + struct nct_serial *serial_priv = usb_get_serial_data(serial); > > + unsigned long flags; > > + bool last_close = false; > > + > > + /* Caller must hold serial_priv->open_close_mutex. */ > > + > > + spin_lock_irqsave(&serial_priv->serial_lock, flags); > > + if (serial_priv->open_count) { > > + serial_priv->open_count--; > > + if (!serial_priv->open_count) { > > + last_close = true; > > + serial_priv->cur_port = NULL; > > + serial_priv->cur_len = 0; > > + } > > + } > > + spin_unlock_irqrestore(&serial_priv->serial_lock, flags); > > + > > + if (!last_close) > > + return; > > + > > + usb_kill_urb(serial->port[0]->read_urb); > > + if (!serial_priv->use_bulk_status) > > + usb_kill_urb(serial->port[0]->interrupt_in_urb); > > +} > > + > > +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; > > + int ret; > > + > > + if (!port->serial) > > + return -ENXIO; > > How could this happen? > > > + mutex_lock(&serial_priv->open_close_mutex); > > + > > + /* Be sure the device is started up */ > > + ret = nct_startup_device(port->serial); > > + if (ret) > > + goto out_unlock; > > + > > + cmd.val = nct_build_cmd(NCT_VCOM_SET_OPEN_PORT, tport->hw_idx); > > + msg.val = cpu_to_le16(0); > > + ret = nct_vendor_write(intf, cmd, le16_to_cpu(msg.val)); > > + if (ret) { > > + dev_err(&port->dev, "Failed to open port: %d\n", ret); > > + nct_shutdown_device(serial); > > + goto out_unlock; > > Nit: I prefer "err_unlock" to make it clear that this path is only used > on errors. > > > + } > > + > > + mutex_unlock(&serial_priv->open_close_mutex); > > + > > + wake_up_interruptible(&port->port.open_wait); > > This one should only be called on carrier detect changes. > > > + > > + /* > > + * Delay 1ms for firmware to configure hardware after opening the port. > > + * (Especially at high speed) > > + */ > > + usleep_range(1000, 2000); > > Nit: add a newline here. > > > + return 0; > > + > > +out_unlock: > > + mutex_unlock(&serial_priv->open_close_mutex); > > Nit: and here. > > > + return ret; > > +} > > + > > +static void nct_close(struct usb_serial_port *port) > > +{ > > + 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; > > + struct nct_ctrl_msg msg; > > + struct nct_vendor_cmd cmd; > > + > > + mutex_lock(&port->serial->disc_mutex); > > + mutex_lock(&serial_priv->open_close_mutex); > > + /* If disconnected, don't send the close-command to the firmware */ > > + if (port->serial->disconnected) > > + goto exit; > > Nit: "out_unlock" is more descriptive. > > > + > > + /* Send 'Close Port' to the device */ > > + cmd.val = nct_build_cmd(NCT_VCOM_SET_CLOSE_PORT, tport->hw_idx); > > + msg.val = cpu_to_le16(0); > > + nct_vendor_write(intf, cmd, le16_to_cpu(msg.val)); > > + > > +exit: > > + nct_shutdown_device(port->serial); > > + mutex_unlock(&serial_priv->open_close_mutex); > > + mutex_unlock(&port->serial->disc_mutex); > > +} > > + > > +static void nct_update_status(struct usb_serial *serial, unsigned char *data) > > +{ > > + struct nct_port_status *nps = (struct nct_port_status *)data; > > + struct usb_interface *intf = serial->interface; > > + struct nct_serial *serial_priv = usb_get_serial_data(serial); > > + struct nct_tty_port *tport; > > + struct tty_struct *tty; > > + struct usb_serial_port *port; > > + unsigned long flags; > > + > > + if (nps->index >= NCT_MAX_NUM_COM_DEVICES) { > > + if (nps->index != NCT_EMPTY_PORT) /* Un-used port */ > > + dev_warn(&intf->dev, "receive wrong H/W index\n"); > > + return; > > + } > > + if (!(nps->hsr & (NCT_UART_STATE_MSR_MASK | NCT_HSR_CTS))) > > + return; /* No state changes */ > > + > > + port = serial_priv->port_by_hw_idx[nps->index]; > > + if (!port) > > + return; > > + > > + tport = usb_get_serial_port_data(port); > > + if (!tport) > > + return; > > + > > + spin_lock_irqsave(&tport->port_lock, flags); > > + tport->usr = nps->usr; > > + tport->hsr = nps->hsr; > > + tport->hcr = nps->hcr; > > + spin_unlock_irqrestore(&tport->port_lock, flags); > > + > > + if (serial->disconnected) { > > + dev_err(&intf->dev, > > + "device disconnected, skip status update\n"); > > + return; > > + } > > + > > + tty = tty_port_tty_get(&port->port); > > + if (!tty) > > + return; /* The port has been closed. */ > > + > > + if (nps->hsr & NCT_UART_STATE_MSR_MASK) { > > + if (nps->hsr & NCT_HSR_DCD) { > > + struct tty_ldisc *ld = tty_ldisc_ref(tty); > > + > > + if (ld) { > > + if (ld->ops->dcd_change) > > + ld->ops->dcd_change(tty, 0x01); > > + tty_ldisc_deref(ld); > > + } > > + wake_up_interruptible(&tty->port->open_wait); > > + } > > Can't you use usb_serial_handle_dcd_change() here? > > > + } > > + > > + /* Flow control */ > > + if (tty_port_cts_enabled(&port->port)) { > > + if ((nps->hsr & NCT_HSR_CTS)) { > > + if (tport->flow_stop_wrt) > > + tport->flow_stop_wrt = false; > > + } else { > > + tport->flow_stop_wrt = true; > > + } > > + } > > So this may be needed either if the device stops transmitting when CTS > is deasserted. > > > + > > + tty_kref_put(tty); > > +} > > + > > +/* > > + * nct_usb_serial_read - process incoming bulk-in data. > > + * > > + * The device multiplexes all UART ports over a single bulk-in endpoint. > > + * Each data segment is prefixed with a 4-byte header: > > + * > > + * byte 0: magic (0xa5) > > + * byte 1: magic2 > > + * 0x5a = data packet > > + * 0x5b = status packet > > + * bytes 2-3: length and index (little-endian 16-bit) > > + * bits [3:0] = port index (1-based, valid range 1-7; 0 is reserved) > > + * bits [15:4] = payload length in bytes > > + * > > + * Multiple packets for different ports may be concatenated within a > > + * single URB. The driver allows a packet's payload to span across URB > > + * boundaries. In that case, the driver records the remaining length (cur_len) > > + * and target port (cur_port) to reassemble the data on subsequent URBs. > > + * > > + * Status packets (magic2 = 0x5b) carry an array of 4-byte per-port > > + * status structures instead of UART data: > > + * > > + * byte 0: port index (1-based; 0 = reserved, 0xff = unused entry) > > + * byte 1: UART status (USR) > > + * bit0: RX data ready > > + * bit1: overrun error > > + * bit2: parity error > > + * bit3: framing error > > + * bit4: break detected > > + * bit5: TX buffer empty > > + * bit6: TX shift register empty > > + * bit7: RX FIFO error > > + * byte 2: modem status (MSR) > > + * bit0: CTS change > > + * bit1: DSR change > > + * bit2: RI change > > + * bit3: DCD change > > + * bit4: CTS level > > + * bit5: DSR level > > + * bit6: RI level > > + * bit7: DCD level > > + * byte 3: modem control (MCR) > > + * bit0: DTR level > > + * bit1: RTS level > > + */ > > Thanks for adding this. > > > +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; > > + u8 hdr_idx; > > + 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 packet (i.e., it is not > > + * the remaining data without a header). > > + * The packet does not need reassembly at this point. > > + */ > > + for (i = 0; i < urb->actual_length; i++) { > > + if (i + sizeof(struct nct_packet_header) > > > + urb->actual_length) { > > + /* Not enough data for a header */ > > + spin_unlock_irqrestore(&serial_priv->serial_lock, > > + flags); > > + return; > > + } > > + > > + hdr = (struct nct_packet_header *)data; > > + /* Decode the header */ > > + > > + /* > > + * Status data is also transmitted via bulk-in > > + * pipe. > > + * Current FW reports a fixed 24-byte status payload. > > + */ > > + if (serial_priv->use_bulk_status && > > + 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; > > + if ((actual_len % sizeof(*nps)) != 0) { > > + dev_err(&intf->dev, > > + "bad status len: %d\n", > > + actual_len); > > + spin_unlock_irqrestore( > > + &serial_priv->serial_lock, > > + flags); > > + return; > > + } > > + > > + for (j = 0; j < (actual_len / sizeof(*nps)); > > + j++) { > > + nct_update_status(serial, > > + (unsigned char *)nps); > > Why do you cast to unsigned char * only to cast back in > nct_update_status() again? > > > + nps++; > > + } > > Looks like you fixed the bug I pointed out here (but it's not mentioned > in the change log) and tried to limit the line lengths by adding line > breaks (which doesn't necessarily improve readability), but the > indentation is still six levels. > > Did you try to look into adding helper functions to break the parsing up > and make it more readable? > > > + > > + spin_unlock_irqrestore(&serial_priv->serial_lock, > > + flags); > > + return; > > + } > > + > > + hdr_idx = nct_get_hdr_idx(hdr); > > + if (hdr->magic == NCT_HDR_MAGIC && > > + hdr->magic2 == NCT_HDR_MAGIC2 && > > + hdr_idx > 0 && > > + hdr_idx < NCT_MAX_NUM_COM_DEVICES && > > + nct_get_hdr_len(hdr) <= 512) > > + break; > > + > > + data++; > > + actual_len--; > > + if (!actual_len) { > > + dev_err(&intf->dev, > > + "decode serial packet size failed\n"); > > + spin_unlock_irqrestore(&serial_priv->serial_lock, > > + flags); > > + return; > > + } > > + } > > + /* > > + * Resolve TTY port by the idx in header and ensure it is valid. > > + * Record it for two purposes: > > + * (1) If the current packet payload is incomplete, the next URB > > + * continues payload bytes without a new header. > > + * (2) Route received bytes to the correct TTY port. > > + */ > > + port = serial_priv->port_by_hw_idx[nct_get_hdr_idx(hdr)]; > > + if (!port) { > > + dev_err(&intf->dev, > > + "decode serial packet index failed\n"); > > + spin_unlock_irqrestore(&serial_priv->serial_lock, flags); > > + return; > > + } > > + > > + tport = usb_get_serial_port_data(port); > > + if (!tport) { > > + 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 received this time belongs to a > > + * single packet payload segment (i.e. no next header in this URB). > > + */ > > + 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 receive the > > + * remaining data for this packet next time > > + */ > > + serial_priv->cur_port = tport; > > + } else { > > + /* > > + * The received data crosses packet boundaries (does not belong > > + * to the same header). We only handle data by > > + * the length in header. And we will handle > > + * another packet when 'goto "again" '. > > + */ > > + len = nct_get_hdr_len(hdr); > > + } > > + } else { /* Handling remaining data that crosses packet boundaries */ > > + 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 packet has been fully handled. > > + * Clear 'cur_port' as no additional data needs to be > > + * attached to the current packet. > > + */ > > + len = serial_priv->cur_len; > > + serial_priv->cur_port = NULL; > > + } > > + serial_priv->cur_len -= len; > > + } > > + spin_unlock_irqrestore(&serial_priv->serial_lock, flags); > > + 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 packets, so it needs to continue processing the next > > + * data packets. > > + */ > > + if (actual_len > 0) > > + goto again; > > +} > > + > > +static void nct_process_read_bulk(struct urb *urb) > > +{ > > + struct usb_serial_port *port = urb->context; > > nit: ^ double space > > > + struct nct_serial *serial_priv = usb_get_serial_data(port->serial); > > + bool stopped = false; > > + int status = urb->status; > > + int ret; > > + > > + switch (status) { > > + case 0: > > + nct_usb_serial_read(urb); > > + break; > > + case -ENOENT: > > + case -ECONNRESET: > > + case -ESHUTDOWN: > > + if (!(status == -ENOENT > > + && serial_priv > > serial_priv is not NULL here > > > + && serial_priv->open_count > > + && !port->serial->disconnected)) { > > + dev_dbg(&port->dev, "urb halted, status=%d\n", status); > > + stopped = true; > > Why did you add this? It should not be needed. > > > + } > > + break; > > + case -EPIPE: > > + dev_err(&port->dev, "urb stalled (EPIPE)\n"); > > + stopped = true; > > + break; > > + default: > > + dev_dbg(&port->dev, "nonzero urb status: %d\n", status); > > + break; > > + } > > + > > + if (stopped) > > + return; > > + > > + ret = usb_submit_urb(urb, GFP_ATOMIC); > > + if (ret != 0 && ret != -EPERM) > > + dev_err(&port->dev, > > + "failed resubmitting urb, ret=%d\n", > > + ret); > > Again, please use bracket around all blocks with multi-line statements > (throughout). > > And please use the following format consistently for errors > > "failed to ...: %d\n", ret > > (i.e. ": " instead of ", ret="). > > > +} > > + > > +static void nct_read_bulk_callback(struct urb *urb) > > +{ > > + struct usb_serial_port *port = urb->context; > > + > > + /* Do not resubmit if the urb is being killed */ > > + if (urb->status == -ESHUTDOWN) > > + return; > > Why check -ESHUTDOWN twice? > > I apparently already pointed this out. > > > + > > + if (!port || !port->serial) > > + return; > > Neither will be NULL. > > I already said this. > > Please don't ignore feedback, no one likes repeating themselves (and > especially not kernel maintainers). > > > + > > + if (port->serial->disconnected) > > + return; > > This should not be needed either. > > > + > > + /* Processing data */ > > + nct_process_read_bulk(urb); > > +} > > + > > +static int nct_calc_num_ports(struct usb_serial *serial, > > + struct usb_serial_endpoints *epds) > > +{ > > + struct nct_vendor_cmd cmd; > > + u8 buf[8]; > > Move these after the following struct declarations. > > > + struct nct_serial *serial_priv = usb_get_serial_data(serial); > > + struct usb_interface *intf = serial->interface; > > + struct usb_host_interface *iface_desc = intf->cur_altsetting; > > + struct usb_endpoint_descriptor *bulk_in = NULL; > > + struct usb_endpoint_descriptor *bulk_out = NULL; > > + struct usb_endpoint_descriptor *int_in = NULL; > > + int ret; > > + int i; > > + int num_ports; > > + > > + /* Send init command */ > > + cmd.val = nct_build_cmd(NCT_VCOM_SET_INIT, NCT_VCOM_INDEX_GLOBAL); > > + ret = nct_vendor_write(intf, cmd, 0); > > + if (ret) { > > + dev_err(&intf->dev, "set COM init error\n"); > > + return ret; > > + } > > + > > + /* Get ports' index supported by the device(/FW) */ > > + cmd.val = nct_build_cmd(NCT_VCOM_GET_PORTS_SUPPORT, NCT_VCOM_INDEX_GLOBAL); > > + ret = nct_vendor_read(intf, cmd, buf, 1); > > + if (ret) { > > + dev_err(&intf->dev, > > Line break not needed. > > > + "get COM port index error\n"); > > + return ret; > > + } > > + > > + if (!serial_priv) { > > + serial_priv = kzalloc_obj(*serial_priv, GFP_KERNEL); > > + if (!serial_priv) > > + return -ENOMEM; > > + spin_lock_init(&serial_priv->serial_lock); > > + mutex_init(&serial_priv->open_close_mutex); > > + usb_set_serial_data(serial, serial_priv); > > + } > > This does not belong here in calc_num_ports(), but if you really must > allocate memory you currently need to use devm_kzalloc() as otherwise it > may not be freed in all error paths. > > But its better to avoid that and defer allocation to attach() if > you can. > > > + serial_priv->en_device_mask = buf[0]; > > Can't you just store the mask in the serial port driver data > temporarily, and retrieve it in attach when you setup the driver data > (or just query the device again)? > > > + serial_priv->last_assigned_hw_idx = 0; /* Note: hw_idx is based on 1 */ > > + > > + for (i = 0, num_ports = 0; i < NCT_MAX_NUM_COM_DEVICES; i++) { > > + if (!(buf[0] & BIT(i))) > > + continue; /* The port is disabled */ > > + > > + num_ports++; > > + } > > + > > + for (i = 0; i < iface_desc->desc.bNumEndpoints; i++) { > > + struct usb_endpoint_descriptor *endpoint = > > + &iface_desc->endpoint[i].desc; > > + > > + if (!bulk_in && usb_endpoint_is_bulk_in(endpoint)) > > + bulk_in = endpoint; > > + if (!bulk_out && usb_endpoint_is_bulk_out(endpoint)) > > + bulk_out = endpoint; > > + if (!int_in && usb_endpoint_is_int_in(endpoint)) > > + int_in = endpoint; > > + } > > Use may be able to use usb_find_common_endpoints() here, but I don't > think you need it either since core will already have stored them in > epds->bulk_in[0], etc. > > > + > > + if (!bulk_in || !bulk_out) > > + return -ENODEV; > > You already set the driver num_bulk_out and num_bulk_in fields so core > would already have rejected any device which doesn't have at least that > many endpoints. > > > + > > + for (i = 0; i < num_ports; i++) { > > + epds->bulk_in[i] = bulk_in; > > The in endpoints do not need to be set for each port, just the bulk_out > one so that the write urbs are allocated. > > > + epds->bulk_out[i] = bulk_out; > > > + if (int_in && i == 0) > > + epds->interrupt_in[i] = int_in; > > No need for this either. > > > + } > > + epds->num_bulk_out = num_ports; > > + > > + return num_ports; > > +} > > + > > +static int nct_attach(struct usb_serial *serial) > > +{ > > + struct nct_serial *serial_priv = usb_get_serial_data(serial); > > + > > + if (!serial_priv) > > + return -ENODEV; > > + > > + serial_priv->use_bulk_status = true; > > + if (serial->port[0] && serial->port[0]->interrupt_in_urb) > > No need to check that port[0] exists (e.g. since you require at least > one endpoint). > > > + serial_priv->use_bulk_status = false; > > + > > + return 0; > > +} > > + > > +static int nct_port_probe(struct usb_serial_port *port) > > +{ > > + struct nct_tty_port *tport; > > + struct usb_serial *serial = port->serial; > > + struct nct_serial *serial_priv = usb_get_serial_data(serial); > > + unsigned long flags; > > + int i, ret; > > + unsigned int fifo_state; > > + > > + tport = kzalloc_obj(*tport, GFP_KERNEL); > > + if (!tport) > > + return -ENOMEM; > > + > > + /* Assign hw_idx */ > > + mutex_init(&tport->mctrl_mutex); > > + spin_lock_init(&tport->port_lock); > > + > > + spin_lock_irqsave(&serial_priv->serial_lock, flags); > > + for (i = serial_priv->last_assigned_hw_idx + 1; > > + i < NCT_MAX_NUM_COM_DEVICES; i++) { > > + if ((serial_priv->en_device_mask & (1 << i)) == 0) > > + continue; /* The port is disabled */ > > + > > + tport->hw_idx = i; > > + serial_priv->last_assigned_hw_idx = i; > > + break; > > + } > > + spin_unlock_irqrestore(&serial_priv->serial_lock, flags); > > It seems better to set up the port mapping for all ports once at > attach(). > > > + > > + usb_set_serial_port_data(port, tport); > > + > > + spin_lock_irqsave(&serial_priv->serial_lock, flags); > > + serial_priv->port_by_hw_idx[tport->hw_idx] = port; > > + spin_unlock_irqrestore(&serial_priv->serial_lock, flags); > > > + fifo_state = kfifo_initialized(&port->write_fifo); > > + > > + /* > > + * Keep the FIFO if it is already initialized. > > + */ > > + if (fifo_state) > > + return 0; > > + ret = kfifo_alloc(&port->write_fifo, NCT_WRITE_FIFO_SIZE, GFP_KERNEL); > > + if (ret) { > > + kfree(tport); > > + return ret; > > + } > > This should not be needed either as core allocates the write_fifo for > all ports with a bulk_out endpoint (which you just set up in > calc_num_ports()). > > > + > > + return 0; > > +} > > + > > +static void nct_interrupt_in_callback(struct urb *urb) > > +{ > > + struct usb_serial_port *port = urb->context; > > + int status = urb->status; > > + struct usb_serial *serial = port->serial; > > + unsigned char *data = urb->transfer_buffer; > > + int retval; > > + int i; > > + int actual_len = urb->actual_length; > > + struct nct_port_status *nps; > > Again, please try to approximate reverse xmas style declarations. > > > + > > + switch (status) { > > + case 0: > > + /* Success */ > > + if ((actual_len % sizeof(*nps)) != 0) > > + return; > > + > > + nps = (struct nct_port_status *)data; > > + for (i = 0; i < (actual_len / sizeof(*nps)); i++) { > > + nct_update_status(serial, (unsigned char *)nps); > > + nps++; > > + } > > Looks like this could share a helper function with the bulk_status > parsing? > > > + break; > > + > > + case -ECONNRESET: > > + case -ENOENT: > > + case -ESHUTDOWN: > > + /* This urb is terminated, clean up */ > > + return; > > + default: > > + break; > > + } > > Nit: add a newline > > > + retval = usb_submit_urb(urb, GFP_ATOMIC); > > + if (retval) > > + dev_err(&port->dev, "submit intr URB failed, ret=%d\n", > > + retval); > > +} > > + > > +static void nct_disconnect(struct usb_serial *serial) > > +{ > > + struct nct_serial *serial_priv = usb_get_serial_data(serial); > > + unsigned long flags; > > + > > + /* Reset status */ > > + spin_lock_irqsave(&serial_priv->serial_lock, flags); > > + serial_priv->open_count = 0; > > + serial_priv->cur_port = NULL; > > + serial_priv->cur_len = 0; > > + spin_unlock_irqrestore(&serial_priv->serial_lock, flags); > > The device is going away so there's no need to reset any state (and all > ports would already have been closed). > > > + > > +} > > + > > +static void nct_port_remove(struct usb_serial_port *port) > > +{ > > + struct nct_tty_port *priv = usb_get_serial_port_data(port); > > + struct nct_serial *serial_priv = usb_get_serial_data(port->serial); > > + unsigned long flags; > > + > > + if (priv) { > > priv is non-NULL here > > > + spin_lock_irqsave(&serial_priv->serial_lock, flags); > > + if (priv->hw_idx < NCT_MAX_NUM_COM_DEVICES) > > hw_idx should never be invalid here, right? > > > + serial_priv->port_by_hw_idx[priv->hw_idx] = NULL; > > + spin_unlock_irqrestore(&serial_priv->serial_lock, flags); > > + } > > + kfree(priv); > > +} > > + > > +static void nct_release(struct usb_serial *serial) > > +{ > > + struct nct_serial *serial_priv; > > + > > + serial_priv = usb_get_serial_data(serial); > > + kfree(serial_priv); > > +} > > + > > +static int nct_suspend(struct usb_serial *serial, pm_message_t message) > > +{ > > + struct nct_serial *serial_priv = usb_get_serial_data(serial); > > + int i; > > + > > + /* Stop all URBs */ > > + usb_kill_urb(serial->port[0]->read_urb); > > + for (i = 0; i < serial->type->num_ports; i++) > > + usb_kill_urb(serial->port[i]->write_urb); > > The generic implementation uses both write urbs. > > > + > > + if (!serial_priv->use_bulk_status) > > + usb_kill_urb(serial->port[0]->interrupt_in_urb); > > But you strictly don't need to implement suspend as you only use URBs > allocated by core, which core will kill for you on suspend. > > > + > > + return 0; > > +} > > + > > +static int nct_resume(struct usb_serial *serial) > > +{ > > + struct nct_serial *serial_priv = usb_get_serial_data(serial); > > + struct usb_interface *intf = serial->interface; > > + unsigned long flags; > > + int ret; > > + > > + /* Reset driver internal state */ > > + spin_lock_irqsave(&serial_priv->serial_lock, flags); > > + serial_priv->cur_port = NULL; > > + serial_priv->cur_len = 0; > > + spin_unlock_irqrestore(&serial_priv->serial_lock, flags); > > + > > + /* Resubmit URBs */ > > + if (!serial_priv->open_count) > > + return 0; > > + > > + ret = usb_submit_urb(serial->port[0]->read_urb, GFP_KERNEL); > > + if (ret) > > + dev_err(&intf->dev, "submit read URB failed, ret=%d\n", > > + ret); > > + > > + if (!serial_priv->use_bulk_status) { > > + ret = usb_submit_urb(serial->port[0]->interrupt_in_urb, > > + GFP_KERNEL); > > + if (ret) > > + dev_err(&intf->dev, > > + "submit interrupt URB failed, ret=%d\n", > > + ret); > > + } > > You may be able to use usb_serial_generic_resume() here instead of open > coding, which also takes care of restarting writes in case there is > still data in the fifos. > > At least if you are going to use both read urbs back to back. > > > + > > + return 0; > > +} > > + > > +static struct usb_serial_driver nct_usb_serial_device = { > > + .driver = { > > + .name = "nct_mtuart", > > + }, > > + .description = "Nuvoton USB to serial adapter", > > + .id_table = id_table, > > + .num_bulk_in = 1, > > + .num_bulk_out = 1, > > + .open = nct_open, > > + .close = nct_close, > > + .write = nct_serial_write, > > + .write_room = nct_write_room, > > + .write_bulk_callback = usb_serial_generic_write_bulk_callback, > > + .prepare_write_buffer = nct_prepare_write_buffer, > > + .read_bulk_callback = nct_read_bulk_callback, > > + .read_int_callback = nct_interrupt_in_callback, > > + .chars_in_buffer = nct_chars_in_buffer, > > + .throttle = nct_rx_throttle, > > + .unthrottle = nct_rx_unthrottle, > > + .attach = nct_attach, > > + .calc_num_ports = nct_calc_num_ports, > > + .set_termios = nct_set_termios, > > + .break_ctl = nct_break, > > + .tiocmget = nct_tiocmget, > > + .tiocmset = nct_tiocmset_helper, > > + .disconnect = nct_disconnect, > > + .release = nct_release, > > + .port_probe = nct_port_probe, > > + .port_remove = nct_port_remove, > > + .suspend = nct_suspend, > > + .resume = nct_resume, > > +}; > > + > > +static struct usb_serial_driver * const nct_serial_drivers[] = { > > + &nct_usb_serial_device, NULL > > +}; > > + > > +module_usb_serial_driver(nct_serial_drivers, id_table); > > +MODULE_DESCRIPTION("Nuvoton USB to serial adaptor driver"); > > +MODULE_AUTHOR("Sheng-Yuan Huang <[email protected]>"); > > +MODULE_LICENSE("GPL v2"); > > > + > > Nit: stray newline > > Again, I think this looks pretty good over all. > > Johan <https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail> 乾淨無病毒。www.avast.com <https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail> <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>