All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Helmut Grohne <helmut.grohne@intenta.de>
Cc: Jiri Slaby <jslaby@suse.com>,
	Michal Simek <michal.simek@xilinx.com>,
	Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>,
	Jan Kiszka <jan.kiszka@web.de>,
	linux-serial@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] tty: xilinx_uartps: Really fix id assignment
Date: Fri, 10 Jul 2020 14:45:35 +0200	[thread overview]
Message-ID: <20200710124535.GA1584780@kroah.com> (raw)
In-Reply-To: <20200709074849.GA28968@laureti-dev>

On Thu, Jul 09, 2020 at 09:48:53AM +0200, Helmut Grohne wrote:
> The problems started with the revert (18cc7ac8a28e28). The
> cdns_uart_console.index is statically assigned -1. When the port is
> registered, Linux assigns consecutive numbers to it. It turned out that
> when using ttyPS1 as console, the index is not updated as we are reusing
> the same cdns_uart_console instance for multiple ports. When registering
> ttyPS0, it gets updated from -1 to 0, but when registering ttyPS1, it
> already is 0 and not updated.
> 
> That led to 2ae11c46d5fdc4. It assigns the index prior to registering
> the uart_driver once. Unfortunately, that ended up breaking the
> situation where the probe order does not match the id order. When using
> the same device tree for both uboot and linux, it is important that the
> serial0 alias points to the console. So some boards reverse those
> aliases. This was reported by Jan Kiszka. The proposed fix was reverting
> the index assignment and going back to the previous iteration.
> 
> However such a reversed assignement (serial0 -> uart1, serial1 -> uart0)
> was already partially broken by the revert (18cc7ac8a28e28). While the
> ttyPS device works, the kmsg connection is already broken and kernel
> messages go missing. Reverting the id assignment does not fix this.
> 
> >From the xilinx_uartps driver pov (after reverting the refactoring
> commits), there can be only one console. This manifests in static
> variables console_pprt and cdns_uart_console. These variables are not
> properly linked and can go out of sync. The cdns_uart_console.index is
> important for uart_add_one_port. We call that function for each port -
> one of which hopefully is the console. If it isn't, the CON_ENABLED flag
> is not set and console_port is cleared. The next cdns_uart_probe call
> then tries to register the next port using that same cdns_uart_console.
> 
> It is important that console_port and cdns_uart_console (and its index
> in particular) stay in sync. The index assignment implemented by
> Shubhrajyoti Datta is correct in principle. It just may have to happen a
> second time if the first cdns_uart_probe call didn't encounter the
> console device. And we shouldn't change the index once the console uart
> is registered.
> 
> Reported-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Reported-by: Jan Kiszka <jan.kiszka@web.de>
> Link: https://lore.kernel.org/linux-serial/f4092727-d8f5-5f91-2c9f-76643aace993@siemens.com/
> Fixes: 18cc7ac8a28e28 ("Revert "serial: uartps: Register own uart console and driver structures"")
> Fixes: 2ae11c46d5fdc4 ("tty: xilinx_uartps: Fix missing id assignment to the console")
> Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
> ---
>  drivers/tty/serial/xilinx_uartps.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

What tree/branch is this against?  It doesn't seem to apply to my
tty-linus branch which is where I would think it should go to, right?

thanks,

greg k-h

WARNING: multiple messages have this Message-ID (diff)
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Helmut Grohne <helmut.grohne@intenta.de>
Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>,
	Michal Simek <michal.simek@xilinx.com>,
	Jan Kiszka <jan.kiszka@web.de>,
	linux-serial@vger.kernel.org, Jiri Slaby <jslaby@suse.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] tty: xilinx_uartps: Really fix id assignment
Date: Fri, 10 Jul 2020 14:45:35 +0200	[thread overview]
Message-ID: <20200710124535.GA1584780@kroah.com> (raw)
In-Reply-To: <20200709074849.GA28968@laureti-dev>

On Thu, Jul 09, 2020 at 09:48:53AM +0200, Helmut Grohne wrote:
> The problems started with the revert (18cc7ac8a28e28). The
> cdns_uart_console.index is statically assigned -1. When the port is
> registered, Linux assigns consecutive numbers to it. It turned out that
> when using ttyPS1 as console, the index is not updated as we are reusing
> the same cdns_uart_console instance for multiple ports. When registering
> ttyPS0, it gets updated from -1 to 0, but when registering ttyPS1, it
> already is 0 and not updated.
> 
> That led to 2ae11c46d5fdc4. It assigns the index prior to registering
> the uart_driver once. Unfortunately, that ended up breaking the
> situation where the probe order does not match the id order. When using
> the same device tree for both uboot and linux, it is important that the
> serial0 alias points to the console. So some boards reverse those
> aliases. This was reported by Jan Kiszka. The proposed fix was reverting
> the index assignment and going back to the previous iteration.
> 
> However such a reversed assignement (serial0 -> uart1, serial1 -> uart0)
> was already partially broken by the revert (18cc7ac8a28e28). While the
> ttyPS device works, the kmsg connection is already broken and kernel
> messages go missing. Reverting the id assignment does not fix this.
> 
> >From the xilinx_uartps driver pov (after reverting the refactoring
> commits), there can be only one console. This manifests in static
> variables console_pprt and cdns_uart_console. These variables are not
> properly linked and can go out of sync. The cdns_uart_console.index is
> important for uart_add_one_port. We call that function for each port -
> one of which hopefully is the console. If it isn't, the CON_ENABLED flag
> is not set and console_port is cleared. The next cdns_uart_probe call
> then tries to register the next port using that same cdns_uart_console.
> 
> It is important that console_port and cdns_uart_console (and its index
> in particular) stay in sync. The index assignment implemented by
> Shubhrajyoti Datta is correct in principle. It just may have to happen a
> second time if the first cdns_uart_probe call didn't encounter the
> console device. And we shouldn't change the index once the console uart
> is registered.
> 
> Reported-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Reported-by: Jan Kiszka <jan.kiszka@web.de>
> Link: https://lore.kernel.org/linux-serial/f4092727-d8f5-5f91-2c9f-76643aace993@siemens.com/
> Fixes: 18cc7ac8a28e28 ("Revert "serial: uartps: Register own uart console and driver structures"")
> Fixes: 2ae11c46d5fdc4 ("tty: xilinx_uartps: Fix missing id assignment to the console")
> Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
> ---
>  drivers/tty/serial/xilinx_uartps.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

What tree/branch is this against?  It doesn't seem to apply to my
tty-linus branch which is where I would think it should go to, right?

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  parent reply	other threads:[~2020-07-10 12:45 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-18  8:11 [PATCH] Revert "tty: xilinx_uartps: Fix missing id assignment to the console" Jan Kiszka
2020-06-18  8:11 ` Jan Kiszka
2020-06-18  8:17 ` Michal Simek
2020-06-18  8:17   ` Michal Simek
2020-07-09  7:48 ` [PATCH] tty: xilinx_uartps: Really fix id assignment Helmut Grohne
2020-07-09  7:48   ` Helmut Grohne
2020-07-10 11:45   ` Michal Simek
2020-07-10 11:45     ` Michal Simek
2020-07-13  7:11     ` Helmut Grohne
2020-07-13  7:11       ` Helmut Grohne
2020-07-13 11:49       ` Michal Simek
2020-07-13 11:49         ` Michal Simek
2020-07-13 12:10         ` Helmut Grohne
2020-07-13 12:10           ` Helmut Grohne
2020-07-13 16:08           ` Maarten Brock
2020-07-13 16:08             ` Maarten Brock
2020-07-22  7:18             ` Michal Simek
2020-07-22  7:18               ` Michal Simek
2020-07-22 16:50               ` Maarten Brock
2020-07-22 16:50                 ` Maarten Brock
2020-07-24  9:19                 ` Michal Simek
2020-07-24  9:19                   ` Michal Simek
2020-07-22  7:14           ` Michal Simek
2020-07-23  9:50             ` Helmut Grohne
2020-07-23  9:50               ` Helmut Grohne
2020-07-24  8:43               ` Michal Simek
2020-07-24  8:43                 ` Michal Simek
2020-07-10 12:45   ` Greg Kroah-Hartman [this message]
2020-07-10 12:45     ` Greg Kroah-Hartman
2020-07-13  7:32     ` [PATCH v2] " Helmut Grohne
2020-07-13  7:32       ` Helmut Grohne

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200710124535.GA1584780@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=helmut.grohne@intenta.de \
    --cc=jan.kiszka@web.de \
    --cc=jslaby@suse.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=michal.simek@xilinx.com \
    --cc=shubhrajyoti.datta@xilinx.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.