From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) (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 019EE3CE485 for ; Fri, 15 May 2026 10:21:20 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.41 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778840484; cv=none; b=BMwfmSkKiZrhg/Ibiu4eO0SebqU+n6b3LLbrouitjLDoRm3YIwivUxH0y+XYy6/o3V4J9O9brBZOCVdTuUhOxe7TTjBjJco2+Yi8qznsXeqWwpWp/ylXZlP3vl+KJ3qB1SvRA7gZYeVWxygbi0RJZ7EQmEj3Sq8y5B136uHqJ1U= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778840484; c=relaxed/simple; bh=EsfVrBZrJsQs/Tx4mfI8R8eRm6q5uajg1tGl9pwd0JM=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=QeZBYIFP/9DL/zeAiu7YnNRHgTphVQU1O8MnZQP0LWrkr0kniBIzphbKKC4PDqgn3mEsLZGJnTQxg/GZQvcj6wfwEMZZOo7EQtAMdYVRC/pHBCguBBecvoKosBexeOqMNXjodhtAT8CxvRA+BtG2KTDOU1WbgR1Mo9C4pSRCy0g= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=W8SRfXK/; arc=none smtp.client-ip=209.85.128.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="W8SRfXK/" Received: by mail-wm1-f41.google.com with SMTP id 5b1f17b1804b1-488a88aeec9so99634255e9.2 for ; Fri, 15 May 2026 03:21:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1778840479; x=1779445279; 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=HkvhJ/9wP8zr+IJruUAJhSjjyH2HxiZyy6n7D1GMZRw=; b=W8SRfXK/Kgq8GrQxYKwInjZCMRtWX3qIIyuOU/TIduZrBVlynZkhp5ZU+blJ/qI02T z3PGRwTdWZKlwo79lWPPPJ3Ydm5O+MIexA/CBKOs5qm+GUDR+2YiyULKXdVDlIXvEqcI Ku+zyVGq5V8hViIboN3xLKPhkYIProkyGwcnH0rvPeSsS1HHoIztaCdT3W9h5i2SMs6B fqE3zI5CYj6QlipmoMqxuQQ5C6m8cDOg1W0wZleZsDikm/p6naXhM4+UJxbCCvcMul3O GWKEi9EWeIAUgFrbZyuDnT++yIt0N1NNHf8xY7AiWLy1XmXmnSH/E7MKziF92edtNcDV OQAA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1778840479; x=1779445279; 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=HkvhJ/9wP8zr+IJruUAJhSjjyH2HxiZyy6n7D1GMZRw=; b=RDrmeGHQkLfIkOTjdF5Gtsxs58nG+R9MibFLvX6njNzmhmcdb9HlOBCJXHK9mtLFpq 8FFubQqgocwIIE+B6Fq2q72oO5npiG3FYIHJ/iCzBkyivvKpOtUqfCJf95I4vWtUc+ad 21exxX2IzM3rNIiRor76B++sV+TcarkAshX6lTh16M1vkHDufzpg+IkqyLRFhEyasNXS Ejw+aeZzpolS8nn03aUhb2+9hO+NangshqSkkVMn5OmGEC+Pa/o3dK/ylSi+bRcl/qDK WfekCs0xwlkfCztjnBlp1Xv7vu33j3KsHF+JGtIuuNsCVWOO+uNvZuU6B7qWE2ubMt+B +mQw== X-Forwarded-Encrypted: i=1; AFNElJ9ejApu3z+6WHdiJKHKkwhLAjql84qOdPrwKydz2PhxCRFuBu7MTOf+fso162r+i2r0/hp7g3B+tiq/Cmg=@vger.kernel.org X-Gm-Message-State: AOJu0YzJrzp5O2VdC92vVSeDImhad32u5JBW6Z1FhFeKEZF2fRzTkyav b10UX7WiuP/mQSYAa2LgQO6ihbn7zOMLb1gEYtpXe1TJmHUNP7rhJBM7mNywcaEX07c= X-Gm-Gg: Acq92OEkMblWyUe0prFRolj22DvNv4cKL3ZrrH5dgwIWEyU7WQbZ0pkBbNTGxXHT9ED Yc0mwRbXVafSBjw6eAAe9HzDjVv4xYnDxq7acQ/YUyDQMmPVYumZpLEhPqvHpjZVZ5OcVjtWoKR YBipdVy+OwQlQM7Nb7Rlseiz9ddLRnOuO0WuFa27keHNRqci52dDg1Wm/XpLAcOsXlgVFHDULHO r3j808JTHt8bgKrHBUrtiDlaexuvADIOLgjKOVD6ZJ0PVnw6L2WB/7obceE7s0odAECuHhKkgwn f51Eti+UgjRedM5Aq+Ip1fgkplVE5DwXKKPpB4OHPBYze0N7fBk210ttULWPDt/O77ha/rtRnIX 8f81GclN+zZPjHvaIN7Z9LHPP8kae7EpHwRUlpcXh4/91YZNxZoB1E2FX8DUqVE2AwOW0r6Kdh4 jd0nOeZ2X8nuLAwCGJQxxs6V+b23dWMQ5m X-Received: by 2002:a05:600c:46cf:b0:48e:8974:c377 with SMTP id 5b1f17b1804b1-48fe6626ba1mr44036975e9.29.1778840479456; Fri, 15 May 2026 03:21:19 -0700 (PDT) Received: from [10.11.12.109] ([79.115.63.228]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-48fe4c834besm52284855e9.3.2026.05.15.03.21.18 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 15 May 2026 03:21:19 -0700 (PDT) Message-ID: Date: Fri, 15 May 2026 13:21:15 +0300 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 tty v4 2/6] serial: Replace driver usage of UPF_CONS_FLOW To: John Ogness , Krzysztof Kozlowski , Greg Kroah-Hartman , Jiri Slaby , Alim Akhtar Cc: Andy Shevchenko , linux-kernel@vger.kernel.org, "David S. Miller" , =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= , Andy Shevchenko , Thomas Fourier , Kees Cook , linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, sparclinux@vger.kernel.org, Peter Griffin , =?UTF-8?Q?Andr=C3=A9_Draszik?= , Alexey Klimov , Juan Yescas References: <20260506121606.5805-1-john.ogness@linutronix.de> <20260506121606.5805-3-john.ogness@linutronix.de> <87lddvy2dl.fsf@jogness.linutronix.de> <1a5abd2e-e9ab-4a48-94c2-5e082f57adde@kernel.org> <87wlx56rcc.fsf@jogness.linutronix.de> Content-Language: en-US From: Tudor Ambarus In-Reply-To: <87wlx56rcc.fsf@jogness.linutronix.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 5/15/26 10:53 AM, John Ogness wrote: > On 2026-05-13, Krzysztof Kozlowski wrote: >>> (This email is particularly directed at the Samsung folks.) >>> >>> Responding to Sashiko: >>> >>> https://sashiko.dev/#/patchset/20260506121606.5805-1-john.ogness%40linutronix.de >>> >>> On 2026-05-06, Sashiko wrote: >>>>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c >>>>> index e27806bf2cf3e..2f94fc798cffb 100644 >>>>> --- a/drivers/tty/serial/samsung_tty.c >>>>> +++ b/drivers/tty/serial/samsung_tty.c >>>>> @@ -319,7 +319,7 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port) >>>>> ourport->tx_enabled = 0; >>>>> ourport->tx_in_progress = 0; >>>>> >>>>> - if (port->flags & UPF_CONS_FLOW) >>>>> + if (uart_cons_flow_enabled(port)) >>>>> s3c24xx_serial_rx_enable(port); >>>> >>>> This isn't a new bug introduced by this patch, but does this code >>>> result in a recursive spinlock acquisition and deadlock when console >>>> flow control is enabled? >>>> >>>> The serial core framework calls port->ops->stop_tx() with the >>>> port->lock spinlock already held. When uart_cons_flow_enabled(port) >>>> evaluates to true, s3c24xx_serial_stop_tx() invokes >>>> s3c24xx_serial_rx_enable(). >>>> >>>> Looking at s3c24xx_serial_rx_enable(), it unconditionally attempts to >>>> acquire the exact same lock again: >>>> >>>> static void s3c24xx_serial_rx_enable(struct uart_port *port) >>>> { >>>> struct s3c24xx_uart_port *ourport = to_ourport(port); >>>> unsigned long flags; >>>> ... >>>> uart_port_lock_irqsave(port, &flags); >>>> ... >>>> } >>>> >>>> Because spinlocks are not recursive in the kernel, it appears this >>>> sequence would cause an immediate deadlock on the same CPU. >>> >>> Although out of scope for my series, the reported issue is real. Perhaps >>> something for Krzysztof or Alim to look into. >>> >>>>> @@ -493,7 +493,7 @@ static void s3c24xx_serial_start_tx(struct uart_port *port) >>>>> struct tty_port *tport = &port->state->port; >>>>> >>>>> if (!ourport->tx_enabled) { >>>>> - if (port->flags & UPF_CONS_FLOW) >>>>> + if (uart_cons_flow_enabled(port)) >>>>> s3c24xx_serial_rx_disable(port); >>>> >>>> Similarly here, s3c24xx_serial_start_tx() is also called with >>>> port->lock held, and s3c24xx_serial_rx_disable() will attempt to >>>> acquire port->lock again: >>>> >>>> static void s3c24xx_serial_rx_disable(struct uart_port *port) >>>> { >>>> struct s3c24xx_uart_port *ourport = to_ourport(port); >>>> unsigned long flags; >>>> ... >>>> uart_port_lock_irqsave(port, &flags); >>>> ... >>>> } >>>> >>>> Could this pre-existing locking issue in the samsung_tty driver be >>>> addressed so that the rx enable/disable helpers do not try to take the >>>> port lock when it is already held by the caller? >>> >>> Also legitimate. But out of scope for my series. >> >> >> Thanks for letting us know. Deadlock did not happen so far, so something >> is missing in Sashiko's report. :) > > Nothing is missing. I am guessing you never use console flow > control. The deadlock is clearly visible: > > ->stop_tx() (always called with the port locked) > s3c24xx_serial_stop_tx() > s3c24xx_serial_rx_enable() > uart_port_lock_irqsave() (DEADLOCK!) > Right. The lock acquisitions in the rx helper functions are redundant and shall be removed. The serial core framework invokes the .stop_tx() and .start_tx() callbacks with the port->lock spinlock already held. Furthermore, all internal driver paths that invoke stop_tx/start_tx also acquire port->lock prior to calling them. However, s3c24xx_serial_rx_enable() and s3c24xx_serial_rx_disable() unconditionally attempt to acquire port->lock again using uart_port_lock_irqsave(). Since kernel spinlocks are not recursive, this causes a deadlock on the same CPU when console flow control is engaged. Just removing the redundant lock acquisitions shall fix it. I'll prepare a patch. diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c index e27806bf2cf3..17cd5bb100b1 100644 --- a/drivers/tty/serial/samsung_tty.c +++ b/drivers/tty/serial/samsung_tty.c +++ b/drivers/tty/serial/samsung_tty.c @@ -245,12 +245,9 @@ static bool s3c24xx_serial_txempty_nofifo(const struct uart_port *port) static void s3c24xx_serial_rx_enable(struct uart_port *port) { struct s3c24xx_uart_port *ourport = to_ourport(port); - unsigned long flags; int count = 10000; u32 ucon, ufcon; - uart_port_lock_irqsave(port, &flags); - while (--count && !s3c24xx_serial_txempty_nofifo(port)) udelay(100); @@ -263,23 +260,18 @@ static void s3c24xx_serial_rx_enable(struct uart_port *port) wr_regl(port, S3C2410_UCON, ucon); ourport->rx_enabled = 1; - uart_port_unlock_irqrestore(port, flags); } static void s3c24xx_serial_rx_disable(struct uart_port *port) { struct s3c24xx_uart_port *ourport = to_ourport(port); - unsigned long flags; u32 ucon; - uart_port_lock_irqsave(port, &flags); - ucon = rd_regl(port, S3C2410_UCON); ucon &= ~S3C2410_UCON_RXIRQMODE; wr_regl(port, S3C2410_UCON, ucon); ourport->rx_enabled = 0; - uart_port_unlock_irqrestore(port, flags); } static void s3c24xx_serial_stop_tx(struct uart_port *port)