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 <[email protected]>
---
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;
-
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;
}
--
2.17.1
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 <[email protected]>
> ---
> 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?
--
i.
>
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > > ---
> > > 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?
--
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 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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]>
> > > > > ---
> > > > > 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).
--
i.
> -----Original Message-----
> From: Ilpo Järvinen <[email protected]>
> Sent: 2022年9月30日 20:59
> To: Sherry Sun <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>; Jiri Slaby
> <[email protected]>; Lukas Wunner <[email protected]>; linux-serial
> <[email protected]>; LKML <[email protected]>; dl-
> linux-imx <[email protected]>
> Subject: RE: [PATCH] Revert "serial: fsl_lpuart: Reset prior to registration"
>
> 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 <[email protected]>
> > > > > > ---
> > > > > > 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).
Hi Ilpo,
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.
I also tried the same way of 8250, it doesn't work, lpuart console still cannot work, as the lpuart_console_setup() is only called through uart_add_one_port() after the console index is initialized, to setup the "real" consoles.
> /* link port to console */
> port->cons = co;
Best Regards
Sherry
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.
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.
Thanks,
Lukas
> -----Original Message-----
> From: Lukas Wunner <[email protected]>
> Sent: 2022年10月10日 17:10
> To: Sherry Sun <[email protected]>
> Cc: Ilpo Järvinen <[email protected]>; Greg Kroah-Hartman
> <[email protected]>; Jiri Slaby <[email protected]>; linux-serial
> <[email protected]>; LKML <[email protected]>; dl-
> linux-imx <[email protected]>
> 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 <[email protected]>
>
> 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")
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 <[email protected]>
What's the status/plan on this?
From what I can tell the commit this patch is reverting introduced a
regression (that was backported to stable kernel), it should have a
Fixes tag and probably a cc:stable.
See also https://github.com/Freescale/linux-fslc/commit/a682d463667a418d3f7277dbbe06cd6347e76761
Francesco