From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f50.google.com (mail-wm1-f50.google.com [209.85.128.50]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C86A6371CF1 for ; Wed, 4 Mar 2026 11:08:02 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.50 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772622484; cv=none; b=omW5ClFPeWP8iye2t6jjCGDgG+net3MvDZd3ucVBGe7P2/DcbQSG1w30Wkn7beI9+TbVspf3aAT7z/0gse8aZyxGu8lnMGIfzBXgCLeYKGbX/U6moDR82626J+GxoNEcAoF1wotUTvm8OfijI5fn6sDqXL12E+0dUtBxKfNCVF4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772622484; c=relaxed/simple; bh=MqR618vUskxcz9JqcOuaxzLICh5XKOW1wOrrjJ073Q0=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=NsyyLduOyUZtkPhxPoa1FFB0/9yGF74SMKsrNwLO2WW5xE5+Bc7miOX2mdsVQOJ3nQRmYY8ureInzHBRh7yzCd5vqNuuzKMMBJZFlL/9nzzvtTjK/zpwHdaTnsCqLskWbgHbBtmNrRPBbtvtyothCrnGBgx7NhSZCGMZ/LI3560= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=suse.com; spf=pass smtp.mailfrom=suse.com; dkim=pass (2048-bit key) header.d=suse.com header.i=@suse.com header.b=YC2DJm5L; arc=none smtp.client-ip=209.85.128.50 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=suse.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=suse.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=suse.com header.i=@suse.com header.b="YC2DJm5L" Received: by mail-wm1-f50.google.com with SMTP id 5b1f17b1804b1-4807068eacbso55861065e9.2 for ; Wed, 04 Mar 2026 03:08:02 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1772622481; x=1773227281; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=XTucieT2brvHlyNLxWqi/FWnFz/6i/21uMod3Iz4R3A=; b=YC2DJm5LSLHcGO2N2TTvbQmdRYr86ndVQ+6QJm36iupRYwB0VC5ansexQnKPXpoYQ+ xjyKbwhjsuEKNssqaqVgU68CxenZvJ0AHTn/2ihza1srxpcxLa/6hyLiz+XgfW0WMsdK mFMzwhY7eaCyZSjNOHZHO3BP1tbssxXmP0loXjV+EANs9hBLVpRj7hOrsum5pW+21TYH mIC7MHuh2eeUoXKNkZvKmObqbbZLba0Ysr0H2hNrn4FpYre9EJZVVerirq4SMLpX7t3R Q2LOQfLNY1lkh+onyGncM7LX+329zihcj2BqPQN4DTxtfji14IG5u/k/OZOu2zQ/0Hrc y2EQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1772622481; x=1773227281; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=XTucieT2brvHlyNLxWqi/FWnFz/6i/21uMod3Iz4R3A=; b=vYRjBezyEY0d7M7cE3XIQxPqKWEqxV/XqOzys/+s3+TsOf2GTU12PZ41ChiZxuOKi5 6BeBQQ3k4k4kBiGZJ7FwXep05DABbfEfGZ9DWcc3MWOf4PiNBWhX93uWysXQf3HyOkIY 6OVB/hSgz46ajMMcY1VXlozX8wUtfNFc3ejBo+LTCMQVgjICTFE8PKFnSVyg1GUr719a vxLG9VQpfBO82xHz70sR2DrqtqsxYjmB433i09cCWjtCSbMXNFOxm1R5SCc23S8lH5u1 N25ymniLQ6KqMn9VcTfjyP8ohOX0QUPgdrCoklXuh+49j80W/lyEtqlhYGYZnNesD7jj cMjw== X-Gm-Message-State: AOJu0YzpHlhkw5XIAe14ASgA7RWOQe7jDiKXBFpSOJ81UFXj0W8n4iN7 egMKX1uLSGDY2rRVIXinR1MFgEGxTQAESj07rm+kZYh3DqvC5lsS/OLoNd5Tmlt1P6c= X-Gm-Gg: ATEYQzyWSjKs9a9cPQhNVSjvpeQTPDOTZ883w/yJ0NR3VDD69h9sky/ZtHyxOvSKgvh w0/v3uUxon0DroevlFdxGWd9ovFvmiwo16vzotz9/SoKl/tYsME4ftZYObE8wTGOdtOqzuJggXf DPwhMKV9nx+9/BhByX/nrRTOiTyaZttXSwIk9sJtJwGn1JSEFag/AvuogLQhiuDmGwrqytuLL57 ABhIlhbFdS39jW3gn4O0Y7S+c9Xg8velsKzQC/EqgiS0/n6yszZ7pDl7pxnnn6XaFW5fuNRiEy8 GazX0NleQbs4VszXT9xeCVo+cvAZlnQvFIz+XeOIxWg4omQ2VCzyCbcc5+zfx1NjdD7FACNu7P0 rjhJ/R5NUozeUGch8AH704bCF8RyUjUi4mM3Py0EfMwWWMKFNe70LQOQ9YZ2pAp5PLaNiY82Gbs yHgjvzCsAwTf4HeHXgCCiFCo2rNw4YQzoxl4BNaZqOgcqDEYbSmnGdQ2pN6y3wbFPFDR00 X-Received: by 2002:a05:600c:3b29:b0:483:7980:4687 with SMTP id 5b1f17b1804b1-48519899381mr28044505e9.17.1772622480990; Wed, 04 Mar 2026 03:08:00 -0800 (PST) Received: from ?IPV6:2a02:3033:26c:d55c:3b10:49f7:caf4:851f? ([2a02:3033:26c:d55c:3b10:49f7:caf4:851f]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4851acfe884sm10583145e9.7.2026.03.04.03.08.00 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 04 Mar 2026 03:08:00 -0800 (PST) Message-ID: Date: Wed, 4 Mar 2026 12:07:59 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5] USB: serial: nct_usb_serial: add support for Nuvoton USB adapter To: hsyemail2@gmail.com, Johan Hovold , Greg Kroah-Hartman Cc: linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org, Sheng-Yuan Huang References: <20260304080929.10179-1-syhuang3@nuvoton.com> Content-Language: en-US From: Oliver Neukum In-Reply-To: <20260304080929.10179-1-syhuang3@nuvoton.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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