| Message ID | 20220929085318.5268-1-sherry.sun@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | Revert "serial: fsl_lpuart: Reset prior to registration" | expand |
> > > This reverts commit 60f361722ad2ae5ee667d0b0545d40c42f754daf. > > > > commit 60f361722ad2 ("serial: fsl_lpuart: Reset prior to > > registration") causes the lpuart console cannot work any more. Since > > the console is registered in the uart_add_one_port(), the driver > > cannot identify the console port before call uart_add_one_port(), > > which causes all the uart ports including the console port will be global > reset. > > So need to revert this patch to avoid breaking the lpuart console. > > > > Signed-off-by: Sherry Sun <sherry.sun@nxp.com> > > --- > > drivers/tty/serial/fsl_lpuart.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/tty/serial/fsl_lpuart.c > > b/drivers/tty/serial/fsl_lpuart.c index 67fa113f77d4..7da46557fcb3 > > 100644 > > --- a/drivers/tty/serial/fsl_lpuart.c > > +++ b/drivers/tty/serial/fsl_lpuart.c > > @@ -2722,10 +2722,6 @@ static int lpuart_probe(struct platform_device > *pdev) > > handler = lpuart_int; > > } > > > > - ret = lpuart_global_reset(sport); > > - if (ret) > > - goto failed_reset; > > - > > So the problem with this being so early is uart_console() in > lpuart_global_reset() that doesn't detect a console because sport->cons is > not yet assigned? Couldn't that be worked around differently? > > Or is there something else in addition to that I'm missing? > Hi Ilpo, Yes, the root cause of the console cannot work after apply the commit 60f361722ad2 ("serial: fsl_lpuart: Reset prior to registration") is lpuart_global_reset() cannot identify the console port, so reset all uart ports. Actually I've been thinking about any other workaround all afternoon, seems no other good options to me till now. And after a further check, I think the original patch is not needed, as uart_add_one_port() won't open the tty device, it should be safe to global reset the non-console ports after uart_add_one_port(). Best Regards Sherry
> On Thu, 29 Sep 2022, Sherry Sun wrote: > > > > > This reverts commit 60f361722ad2ae5ee667d0b0545d40c42f754daf. > > > > > > > > commit 60f361722ad2 ("serial: fsl_lpuart: Reset prior to > > > > registration") causes the lpuart console cannot work any more. > > > > Since the console is registered in the uart_add_one_port(), the > > > > driver cannot identify the console port before call > > > > uart_add_one_port(), which causes all the uart ports including the > > > > console port will be global > > > reset. > > > > So need to revert this patch to avoid breaking the lpuart console. > > > > > > > > Signed-off-by: Sherry Sun <sherry.sun@nxp.com> > > > > --- > > > > drivers/tty/serial/fsl_lpuart.c | 10 +++++----- > > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/tty/serial/fsl_lpuart.c > > > > b/drivers/tty/serial/fsl_lpuart.c index 67fa113f77d4..7da46557fcb3 > > > > 100644 > > > > --- a/drivers/tty/serial/fsl_lpuart.c > > > > +++ b/drivers/tty/serial/fsl_lpuart.c > > > > @@ -2722,10 +2722,6 @@ static int lpuart_probe(struct > > > > platform_device > > > *pdev) > > > > handler = lpuart_int; > > > > } > > > > > > > > - ret = lpuart_global_reset(sport); > > > > - if (ret) > > > > - goto failed_reset; > > > > - > > > > > > So the problem with this being so early is uart_console() in > > > lpuart_global_reset() that doesn't detect a console because > > > sport->cons is not yet assigned? Couldn't that be worked around > differently? > > > > > > Or is there something else in addition to that I'm missing? > > > > > Hi Ilpo, > > > > Yes, the root cause of the console cannot work after apply the commit > > 60f361722ad2 ("serial: fsl_lpuart: Reset prior to registration") is > > lpuart_global_reset() cannot identify the console port, so reset all > > uart ports. > > This didn't answer my question. Is the main cause just lacking the ->cons > from sport at this point which, I guess, could be just assigned from lpuart_reg > similar to what uart_add_one_port() does before calling to reset? > Hi Ilpo, Actually not only the (port)->cons need to be assigned, but also to get the right (port)->cons->index. 23 #define uart_console(port) \ 24 ((port)->cons && (port)->cons->index == (port)->line) The (port)->cons is assigned in uart_add_one_port(), quite simple. 3076 uport->cons = drv->cons; But the (port)->cons->index is not that easy to get in lpuart driver, now the value is assigned by calling register_console() from uart_configure_port(). Best Regards Sherry > -- > i. > > > Actually I've been thinking about any other workaround all afternoon, > seems no other good options to me till now. And after a further check, I > think the original patch is not needed, as uart_add_one_port() won't open > the tty device, it should be safe to global reset the non-console ports after > uart_add_one_port(). > > > > Best Regards > > Sherry > >
On Fri, 30 Sep 2022, Sherry Sun wrote: > > On Thu, 29 Sep 2022, Sherry Sun wrote: > > > > > > > This reverts commit 60f361722ad2ae5ee667d0b0545d40c42f754daf. > > > > > > > > > > commit 60f361722ad2 ("serial: fsl_lpuart: Reset prior to > > > > > registration") causes the lpuart console cannot work any more. > > > > > Since the console is registered in the uart_add_one_port(), the > > > > > driver cannot identify the console port before call > > > > > uart_add_one_port(), which causes all the uart ports including the > > > > > console port will be global > > > > reset. > > > > > So need to revert this patch to avoid breaking the lpuart console. > > > > > > > > > > Signed-off-by: Sherry Sun <sherry.sun@nxp.com> > > > > > --- > > > > > drivers/tty/serial/fsl_lpuart.c | 10 +++++----- > > > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/drivers/tty/serial/fsl_lpuart.c > > > > > b/drivers/tty/serial/fsl_lpuart.c index 67fa113f77d4..7da46557fcb3 > > > > > 100644 > > > > > --- a/drivers/tty/serial/fsl_lpuart.c > > > > > +++ b/drivers/tty/serial/fsl_lpuart.c > > > > > @@ -2722,10 +2722,6 @@ static int lpuart_probe(struct > > > > > platform_device > > > > *pdev) > > > > > handler = lpuart_int; > > > > > } > > > > > > > > > > - ret = lpuart_global_reset(sport); > > > > > - if (ret) > > > > > - goto failed_reset; > > > > > - > > > > > > > > So the problem with this being so early is uart_console() in > > > > lpuart_global_reset() that doesn't detect a console because > > > > sport->cons is not yet assigned? Couldn't that be worked around > > differently? > > > > > > > > Or is there something else in addition to that I'm missing? > > > > > > > Hi Ilpo, > > > > > > Yes, the root cause of the console cannot work after apply the commit > > > 60f361722ad2 ("serial: fsl_lpuart: Reset prior to registration") is > > > lpuart_global_reset() cannot identify the console port, so reset all > > > uart ports. > > > > This didn't answer my question. Is the main cause just lacking the ->cons > > from sport at this point which, I guess, could be just assigned from lpuart_reg > > similar to what uart_add_one_port() does before calling to reset? > > > > Hi Ilpo, > > Actually not only the (port)->cons need to be assigned, but also to get the right (port)->cons->index. > 23 #define uart_console(port) \ > 24 ((port)->cons && (port)->cons->index == (port)->line) > > The (port)->cons is assigned in uart_add_one_port(), quite simple. > 3076 uport->cons = drv->cons; > > But the (port)->cons->index is not that easy to get in lpuart driver, > now the value is assigned by calling register_console() from > uart_configure_port(). I've some skepticism to this claim. I now played with 8250 myself and confirmed it does have non-NULL ->cons for the console ports before to calls to uart_add_one_port() and index is setup up correctly for cons too! The reason for the cons being setup is this being done in univ8250_console_setup(): /* link port to console */ port->cons = co; (which I think could be easily be done in lpuart_console_setup() too).
> -----Original Message----- > From: Lukas Wunner <lukas@wunner.de> > Sent: 2022年10月10日 17:10 > To: Sherry Sun <sherry.sun@nxp.com> > Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>; Greg Kroah-Hartman > <gregkh@linuxfoundation.org>; Jiri Slaby <jirislaby@kernel.org>; linux-serial > <linux-serial@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>; dl- > linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH] Revert "serial: fsl_lpuart: Reset prior to registration" > > On Sun, Oct 09, 2022 at 10:23:13AM +0000, Sherry Sun wrote: > > I am not familiar with 8250 serial, but at least for imx uart driver > > and lpuart driver, the following behavior is same. > > For the "real" consoles (everything which is not a bootconsole), the > > (port)->cons and (port)->cons->index are initialized through > > uart_add_one_port()->uart_configure_port()->register_console()-> > > try_enable_new_console(), here the console index is assigned by the > > console cmdline parameters. > > Hm, uart_add_one_port() does the following *before* calling > uart_configure_port(): > > /* > * If this port is in use as a console then the spinlock is already > * initialised. > */ > if (!uart_console_enabled(uport)) > uart_port_spin_lock_init(uport); > > It sounds like in the case of fsl_lpuart.c, the spin lock is *always* initialized, > even though a concurrent lpuart_console_write() may be holding it. That's > not solved by moving lpuart_global_reset() around. Hi Lukas, For the "real" consoles that registered through uart_add_one_port() during uart driver probe, yes, the spin lock is *always* initialized. And don't worry about the lpuart_console_write() holding the spin lock, as the lpuart_console_write() will only be used after the lpuart_console_setup(), the spin lock has been initialized. [ 1.824177] Call trace: [ 1.826627] uart_port_spin_lock_init+0x20/0x38 [ 1.831177] uart_add_one_port+0x150/0x5d0 [ 1.835290] lpuart_probe+0x26c/0x484 [ 1.838966] platform_probe+0x68/0xe0 Now the issue is, for the lpuart console port, the uart_console() will only be set correctly in uart_add_one_port(), so if move lpuart_global_reset() before uart_add_one_port(), we cannot recognize the console port, which will cause the console port also been global reset to break the console print. > > The problem with performing lpuart_global_reset() after UART registration is > that as soon as uart_add_one_port() returns, the port is available for user > space to use. So resetting it is a no-go. > Per my understanding, even we don’t do the global reset for the non-console ports after uart_add_one_port(), we still need to initialize the tty ports in lpuart32_startup() when open tty device, it should be safe to add a software reset before initializing the tty ports. Best Regards Sherry
On Tue, Dec 06, 2022 at 03:52:07PM +0100, Francesco Dolcini wrote: > Hello all, > > On Thu, Sep 29, 2022 at 04:53:18PM +0800, Sherry Sun wrote: > > This reverts commit 60f361722ad2ae5ee667d0b0545d40c42f754daf. > > > > commit 60f361722ad2 ("serial: fsl_lpuart: Reset prior to registration") > > causes the lpuart console cannot work any more. Since the console is > > registered in the uart_add_one_port(), the driver cannot identify the > > console port before call uart_add_one_port(), which causes all the uart > > ports including the console port will be global reset. > > So need to revert this patch to avoid breaking the lpuart console. > > > > Signed-off-by: Sherry Sun <sherry.sun@nxp.com> > > What's the status/plan on this? whoops, already solved in 76bad3f88750 ("tty: serial: fsl_lpuart: don't break the on-going transfer when global reset")
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c index 67fa113f77d4..7da46557fcb3 100644 --- a/drivers/tty/serial/fsl_lpuart.c +++ b/drivers/tty/serial/fsl_lpuart.c @@ -2722,10 +2722,6 @@ static int lpuart_probe(struct platform_device *pdev) handler = lpuart_int; } - ret = lpuart_global_reset(sport); - if (ret) - goto failed_reset; - ret = uart_get_rs485_mode(&sport->port); if (ret) goto failed_get_rs485; @@ -2734,6 +2730,10 @@ static int lpuart_probe(struct platform_device *pdev) if (ret) goto failed_attach_port; + ret = lpuart_global_reset(sport); + if (ret) + goto failed_reset; + ret = devm_request_irq(&pdev->dev, sport->port.irq, handler, 0, DRIVER_NAME, sport); if (ret) @@ -2742,10 +2742,10 @@ static int lpuart_probe(struct platform_device *pdev) return 0; failed_irq_request: +failed_reset: uart_remove_one_port(&lpuart_reg, &sport->port); failed_attach_port: failed_get_rs485: -failed_reset: lpuart_disable_clks(sport); return ret; }
This reverts commit 60f361722ad2ae5ee667d0b0545d40c42f754daf. commit 60f361722ad2 ("serial: fsl_lpuart: Reset prior to registration") causes the lpuart console cannot work any more. Since the console is registered in the uart_add_one_port(), the driver cannot identify the console port before call uart_add_one_port(), which causes all the uart ports including the console port will be global reset. So need to revert this patch to avoid breaking the lpuart console. Signed-off-by: Sherry Sun <sherry.sun@nxp.com> --- drivers/tty/serial/fsl_lpuart.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)