Re: [PATCH v5] USB: serial: nct_usb_serial: add support for Nuvoton USB adapter

[Oliver Neukum <oneukum@suse.com>]

Hi,

thank you for the submission.

On 04.03.26 09:09, hsyemail2@gmail.com wrote:

> +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;
> +	u8 hcr;
> +
> +	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;
> +	cmd.val = nct_build_cmd(NCT_VCOM_SET_HCR, tport->hw_idx);
> +	msg.val = cpu_to_le16(hcr);
> +	spin_unlock_irq(&tport->port_lock);

What exactly are you locking with that spinlock against?
You are keeping it held after setting tport->hcr

> +	return nct_vendor_write(intf, cmd, le16_to_cpu(msg.val));
> +}

> + *  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;
> +	struct nct_serial *serial_priv = usb_get_serial_data(serial);
> +	struct usb_serial_port *port;
> +	unsigned long flags;
> +	bool first_open = false;
> +
> +	/* 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);

And here we have a problem. At this time a concurrent opener
can run and read open_count
> +
> +	/* Only the first open submits read_urb and, if needed, interrupt_in_urb. */
> +	if (!first_open)
> +		return 0;

That means that a concurrent opener can return here, without the URB
having been submitted.

> +	/* 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;

Here you handle errors.

> +	}
> +
> +	/* 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;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_kill_read:
> +	usb_kill_urb(serial->port[0]->read_urb);
> +err_rollback:
> +	spin_lock_irqsave(&serial_priv->serial_lock, flags);

Taking the lock again

> +	if (serial_priv->open_count)
> +		serial_priv->open_count--;

Too late

> +	if (!serial_priv->open_count) {
> +		serial_priv->cur_port = NULL;
> +		serial_priv->cur_len = 0;
> +	}
> +	spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
> +	return ret;
> +}

If a second call to open() races with a primary open() that fails,
we'll end up with the first open() failing, as it should, but the
second one succeeds, although it also has to fail with an error return.

It seems to me that the obvious fix is to add a mutex that needs to be held
throughout nct_startup_device() and nct_shutdown_device()


> +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 usb_interface *intf = serial->interface;
> +	int ret;
> +
> +	if (!port->serial)
> +		return -ENXIO;
> +
> +	/* 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);
> +	ret = nct_vendor_write(intf, cmd, le16_to_cpu(msg.val));

Likewise. If two calls to open() are racing, the second one will
return before you send NCT_VCOM_SET_OPEN_PORT to the device.

> +	if (ret) {
> +		dev_err(&port->dev, "Failed to open port: %d\n", ret);
> +		nct_shutdown_device(serial);
> +		return ret;
> +	}
> +
> +	wake_up_interruptible(&port->port.open_wait);
> +
> +	/*
> +	 * Delay 1ms for firmware to configure hardware after opening the port.
> +	 * (Especially at high speed)
> +	 */
> +	usleep_range(1000, 2000);
> +	return 0;
> +}

	Regards
		Oliver