From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f42.google.com (mail-wm1-f42.google.com [209.85.128.42]) (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 39C9A3ACA41 for ; Mon, 20 Jul 2026 14:58:20 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.42 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784559501; cv=none; b=UnUkmJQwvchok/5Vbo8CdXI8p+P4jVC/5yVM0by95NYkbiMsLIRVcRq7qc7EH2NnUh+n8RWn0QaaJl+99FgOZuGF8AM/HlfFZ3RP7dbaspMKl89fRZb6TxIdu9SpiWm42bhJSBFnHNzvzf6nBpbY3jYtgNFSDh2vFNcbRhcfDd0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784559501; c=relaxed/simple; bh=jzFQWnseWWxoyN1ShXRYtH2cU8pOwjWhXzAAGBHZ/o4=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=gf0WNuXeafIxmfj3t8zc30taEfc/hp+60cgUA7Am1kA20GfZi7R8ljDf0mMieQ6j7kYkaPMTflFvjKgtDQInGREiqryH07xZy5V53iON/heElmH3HR6k/UqFuGaaJN04gyGyuHGSjXzZNZtoOZgc0C7vbxcKHVZV6Ru0U03PQUU= 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=Qxa7InTI; arc=none smtp.client-ip=209.85.128.42 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="Qxa7InTI" Received: by mail-wm1-f42.google.com with SMTP id 5b1f17b1804b1-4955de8797cso6664755e9.3 for ; Mon, 20 Jul 2026 07:58:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1784559498; x=1785164298; darn=vger.kernel.org; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:from:to:cc:subject :date:message-id:reply-to:content-type; bh=b0HRbfmTok9CVSMITXJl4LP8yNtyhGJvNwreiAeO/rE=; b=Qxa7InTId4jxsi8L7wDj+tJXo2LRH9jkCs0AEiFJq/9XpGp+4Mufh96ShRko1JvwNu JjRDK2uBIz8S12cMRHUaeRAS791j7SlzpHHAFz1ICATh5zoUjqMils+HL6t+V69TUPP7 +crIZ4ZHXNx4BBp225A1QpygzqY3lA6bj/NpQCPA1zGPB/J01w9jJYJKUzyF799xHswz 0VeXTy9Z8OBAdIeREZwN4DwJPFID2DRtKosTIq+OqUXtvtBR/rLZ+rYcmpW/InFHt9Ln 8oLbZbHBtqW04GezqBmjVWF01zGKSIxtqiksWoXs3GdNN30cz2O0hl6fCLZ2bvinqmHj nITw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1784559498; x=1785164298; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=b0HRbfmTok9CVSMITXJl4LP8yNtyhGJvNwreiAeO/rE=; b=oCeabblB6M9tRxTkTYn07k8VJiRm/oyVxD1xR2UUsMpzl3kCQ74JUqAgdFXykdq3oY lxFDMKLW7NXfaqrBQsAtreEFMYNaQCfxbrd5u1dmjP63abZFXgTK1qMKCK4BhHoxGUa/ FW8JVNR0omIIPv01HY9u5eSrzk7p91oV2v1njlzxPl9XAAUhBpwc5aY9W1GJS4cKEAkG peam+R+P0qr1+99tKVr4EvCy6qsuWAq6ZIF2CCCU+uKdW4r71pDczKWZW3SPOCwRTo9x 7Qxos9Tf8fctawBA8bITlu8Th3rpD4XJ6VIEHqskUv+gsf9OXQCCVVDRwFN8UbRahI4i 6rWg== X-Forwarded-Encrypted: i=1; AHgh+Rr55b82bwrVamQK5KT4p3OLiqzjQpyHOaIvhXsJ4QbIvjLwgE26oXwS8TQNpnk3Gr5ofNAGLlWGlTV2E/Y=@vger.kernel.org X-Gm-Message-State: AOJu0Yxk7LzNEkydttcewqv/4Cwp0PTVTYq8yqL8UP3bndBNbn+V+UJE MPcoN6OQ6raM9DX6rTMGk8BCPewtzkGB57oKDex6cI3Fad/sbxX9ARUD4/26FcAUjSg= X-Gm-Gg: AfdE7cmsZPjy3xzUzyphV6uDkaxYqdiaYZvllCZwglqj5B7WjGgGqtVxFF1eVlvem7R dnHeBWhzk3FMekfm/T1kg4ZZ6WTpbcoaJ1Id6HWAWRGQa3Ubi+S6VXmGilZ4EEZP5aHvBhzgCdQ M8ddQgpliW35QVECns8yY5sbaqqbl5ncPX3y2AV8RxEsM/ynFkBVXNvAQUYx4LL2hCeXeZTzy9C LgVMWpy98Un9p0bPf7aIrMLjOG/wHO4glhjLhHOgJjPIqkgAhBJYN2W6HsEvNL1HixRYgLjFEdL ysxClHrObgTnAqrc+m9Bha/Kpgp5LgpTpPCnmo7fmU0LcN1BF3FxqleycFha4mI+LOolFn2eAlB J57w53hrr6nLeOh8A6HXKQFXCSZJgdEh+aVodcTDDO8Gt0vSiXFfvsuSzDuKE949OjHIpm5QKuG o5RKn/ X-Received: by 2002:a05:600d:644d:10b0:495:5de0:d87 with SMTP id 5b1f17b1804b1-4955de00e16mr44746845e9.36.1784559498351; Mon, 20 Jul 2026 07:58:18 -0700 (PDT) Received: from pathway.suse.cz ([176.114.240.130]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-49549a47de7sm292714245e9.7.2026.07.20.07.58.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 20 Jul 2026 07:58:18 -0700 (PDT) Date: Mon, 20 Jul 2026 16:58:16 +0200 From: Petr Mladek To: John Ogness Cc: Greg Kroah-Hartman , Jiri Slaby , Andy Shevchenko , linux-kernel@vger.kernel.org, Ilpo =?iso-8859-1?Q?J=E4rvinen?= , Andy Shevchenko , Hugo Villeneuve , Osama Abdelkader , Stepan Ionichev , Kees Cook , Xin Zhao , Fushuai Wang , Yunhui Cui , Jacques Nilo , linux-serial@vger.kernel.org Subject: Re: [PATCH tty v6 1/2] serial: 8250: Switch to nbcon console, take 2 Message-ID: References: <20260720103242.7265-1-john.ogness@linutronix.de> <20260720103242.7265-2-john.ogness@linutronix.de> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260720103242.7265-2-john.ogness@linutronix.de> On Mon 2026-07-20 12:38:35, John Ogness wrote: > Implement the necessary callbacks to switch the 8250 console driver > to perform as an nbcon console. > > Add implementations for the nbcon console callbacks: > > ->write_atomic() > ->write_thread() > ->device_lock() > ->device_unlock() > > and add CON_NBCON to the initial @flags. > > All hardware access in the callbacks is within unsafe sections. > The ->write_atomic() and ->write_thread() callbacks allow safe > handover/takeover per byte and add a preceding newline if they > take over from another context mid-line. > > For the ->write_atomic() callback, a new irq_work is used to defer > modem control since it may be called from a context that does not > allow waking up tasks. During suspend/resume the irq_work is not > used as this has been shown to cause suspend problems for some > hardware. Upon resume, any pending modem control is performed. > > Note: A new __serial8250_clear_IER() is introduced for direct > clearing of UART_IER during console writing (which may not be > holding the port lock for atomic printing). This allows restoring > a lockdep check to serial8250_clear_IER() in a follow-up commit. > > --- a/drivers/tty/serial/8250/8250_core.c > +++ b/drivers/tty/serial/8250/8250_core.c > @@ -584,6 +609,9 @@ void serial8250_suspend_port(int line) > struct uart_8250_port *up = &serial8250_ports[line]; > struct uart_port *port = &up->port; > > + /* No irq_work may be queued when suspending. */ > + up->avoid_modem_status_work = true; Do we need to synchronize this against serial8250_console_write() where this flag is checked, please? My understanding is that we should be on the safe side. Otherwise there might be bigger problems. I believe that this is called after both console_suspend_all() and console_suspend(uport->cons). The later makes sure that the console is not longer used even when @console_suspend_enabled is false. And these functions even call synchronize_srcu(&console_srcu). This might even answer the question from Sashiko AI whether we should flush the related irq_work() here, see https://sashiko.dev/#/patchset/20260720103242.7265-1-john.ogness%40linutronix.de That said, I am not sure about RT_PREEMPT. AFAIK, it handles IRQs in a kthread. In this case, synchronize_srcu() would not make sure that the irq_work was procceed. Note that Sashiko AI suggests that we might need to flush the irq_work even in serial8250_console_exit(). I guess that the situation is the same there. It is called after synchronize_srcu()... > + > if (!console_suspend_enabled && uart_console(port) && > port->type != PORT_8250) { > unsigned char canary = 0xa5; > @@ -620,6 +648,12 @@ void serial8250_resume_port(int line) > port->uartclk = 921600*16; > } > uart_resume_port(&serial8250_reg, port); > + > + /* irq_work allowed again. Handle MSR now if pending. */ > + up->avoid_modem_status_work = false; > + guard(uart_port_lock_irqsave)(port); > + if (uart_console(port) && up->msr_saved_flags) > + serial8250_modem_status(up); I would use scoped_guard() to make the scope clear. Something like: scoped_guard(uart_port_lock_irqsave, port) { if (uart_console(port) && up->msr_saved_flags) serial8250_modem_status(up); } Motivation: The guard() is pretty hidden. It can easily get overlooked when people add more code at the end of this function. Wait, this should not be needed if we make sure that the work was flushed in serial8250_suspend_port(). > } > EXPORT_SYMBOL(serial8250_resume_port); > > --- a/drivers/tty/serial/8250/8250_port.c > +++ b/drivers/tty/serial/8250/8250_port.c > @@ -3286,39 +3329,57 @@ static void serial8250_console_fifo_write(struct uart_8250_port *up, > * Allow timeout for each byte written since the caller will only wait > * for UART_LSR_BOTH_EMPTY using the timeout of a single character > */ > - serial8250_fifo_wait_for_lsr_thre(up, tx_count); > + serial8250_fifo_wait_for_lsr_thre(up, wctxt, tx_count); > +} > + > +static void serial8250_console_byte_write(struct uart_8250_port *up, > + struct nbcon_write_context *wctxt) > +{ > + struct uart_port *port = &up->port; > + const char *s = wctxt->outbuf; > + const char *end = s + wctxt->len; > + > + /* > + * Write out the message. If a handover or takeover occurs, writing > + * must be aborted since wctxt->outbuf and wctxt->len are no longer > + * valid. > + */ > + while (s != end) { > + if (!nbcon_enter_unsafe(wctxt)) > + return; > + > + uart_console_write(port, s++, 1, serial8250_console_wait_putchar); > + > + nbcon_exit_unsafe(wctxt); > + } > } > > /* > - * Print a string to the serial port trying not to disturb > - * any possible real use of the port... > + * Print a string to the serial port trying not to disturb > + * any possible real use of the port... > * > - * The console_lock must be held when we get here. > - * > - * Doing runtime PM is really a bad idea for the kernel console. > - * Thus, we assume the function is called when device is powered up. > + * Doing runtime PM is really a bad idea for the kernel console. > + * Thus, assume it is called when device is powered up. > */ > -void serial8250_console_write(struct uart_8250_port *up, const char *s, > - unsigned int count) > +void serial8250_console_write(struct uart_8250_port *up, > + struct nbcon_write_context *wctxt, > + bool is_atomic) > { > struct uart_8250_em485 *em485 = up->em485; > struct uart_port *port = &up->port; > - unsigned long flags; > - unsigned int ier, use_fifo; > - int locked = 1; > - > - touch_nmi_watchdog(); > + unsigned int ier; > + bool use_fifo; > > - if (oops_in_progress) > - locked = uart_port_trylock_irqsave(port, &flags); > - else > - uart_port_lock_irqsave(port, &flags); > + if (!nbcon_enter_unsafe(wctxt)) > + return; > > /* > - * First save the IER then disable the interrupts > + * First, save the IER, then disable the interrupts. The special > + * variant to clear the IER is used because console printing may > + * occur without holding the port lock. I would make the comment more clear when it might happen and if it is safe. Something like: * First, save the IER, then disable the interrupts. The special * variant to clear the IER is used because an emergency and panic * console printing is synchronized only by nbcon context without * holding the port lock. > */ > ier = serial_port_in(port, UART_IER); > - serial8250_clear_IER(up); > + __serial8250_clear_IER(up); > > /* check scratch reg to see if port powered off during system sleep */ > if (up->canary && (up->canary != serial_port_in(port, UART_SCR))) { > @@ -3332,6 +3393,18 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s, > mdelay(port->rs485.delay_rts_before_send); > } > > + /* If ownership was lost, no writing is allowed */ > + if (!nbcon_can_proceed(wctxt)) > + goto skip_write; > + > + /* > + * If console printer did not fully output the previous line, it must > + * have been handed or taken over. Insert a newline in order to > + * maintain clean output. > + */ > + if (!up->console_line_ended) > + uart_console_write(port, "\n", 1, serial8250_console_wait_putchar); > + > use_fifo = (up->capabilities & UART_CAP_FIFO) && > /* > * BCM283x requires to check the fifo > @@ -3352,10 +3425,23 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s, > */ > !uart_console_hwflow_active(&up->port); > > + nbcon_exit_unsafe(wctxt); > + > if (likely(use_fifo)) > - serial8250_console_fifo_write(up, s, count); > + serial8250_console_fifo_write(up, wctxt); > else > - uart_console_write(port, s, count, serial8250_console_wait_putchar); > + serial8250_console_byte_write(up, wctxt); > +skip_write: > + /* > + * If ownership was lost, this context must reacquire ownership and > + * re-enter the unsafe section in order to perform final actions > + * (such as re-enabling interrupts). > + */ > + if (!nbcon_can_proceed(wctxt)) { This should be: if (!nbcon_enter_unsafe(wctxt)) or even better: while (!nbcon_enter_unsafe(wctxt)) nbcon_reacquire_nobuf(wctxt); Otherwise, we would not be in the unsafe_context when nbcon_can_proceed() succeeded. Note: I have missed this. It was actually found by Sashiko... > + do { > + nbcon_reacquire_nobuf(wctxt); > + } while (!nbcon_enter_unsafe(wctxt)); > + } > > /* > * Finally, wait for transmitter to become empty Otherwise, it looks good to me. Best Regards, Petr