linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Revert "serial: fsl_lpuart: Reset prior to registration"
@ 2022-09-29  8:53 Sherry Sun
  2022-09-29 10:51 ` Ilpo Järvinen
  2022-12-06 14:51 ` Francesco Dolcini
  0 siblings, 2 replies; 11+ messages in thread
From: Sherry Sun @ 2022-09-29  8:53 UTC (permalink / raw)
  To: gregkh, jirislaby, lukas; +Cc: linux-serial, linux-kernel, linux-imx

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;
-
 	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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] Revert "serial: fsl_lpuart: Reset prior to registration"
  2022-09-29  8:53 [PATCH] Revert "serial: fsl_lpuart: Reset prior to registration" Sherry Sun
@ 2022-09-29 10:51 ` Ilpo Järvinen
  2022-09-29 11:10   ` Sherry Sun
  2022-12-06 14:51 ` Francesco Dolcini
  1 sibling, 1 reply; 11+ messages in thread
From: Ilpo Järvinen @ 2022-09-29 10:51 UTC (permalink / raw)
  To: Sherry Sun
  Cc: Greg Kroah-Hartman, Jiri Slaby, Lukas Wunner, linux-serial, LKML,
	linux-imx

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?

-- 
 i.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] Revert "serial: fsl_lpuart: Reset prior to registration"
  2022-09-29 10:51 ` Ilpo Järvinen
@ 2022-09-29 11:10   ` Sherry Sun
  2022-09-29 11:19     ` Ilpo Järvinen
  0 siblings, 1 reply; 11+ messages in thread
From: Sherry Sun @ 2022-09-29 11:10 UTC (permalink / raw)
  To: Ilpo Järvinen
  Cc: Greg Kroah-Hartman, Jiri Slaby, Lukas Wunner, linux-serial, LKML,
	dl-linux-imx



> 
> > 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] Revert "serial: fsl_lpuart: Reset prior to registration"
  2022-09-29 11:10   ` Sherry Sun
@ 2022-09-29 11:19     ` Ilpo Järvinen
  2022-09-30  8:02       ` Sherry Sun
  0 siblings, 1 reply; 11+ messages in thread
From: Ilpo Järvinen @ 2022-09-29 11:19 UTC (permalink / raw)
  To: Sherry Sun
  Cc: Greg Kroah-Hartman, Jiri Slaby, Lukas Wunner, linux-serial, LKML,
	dl-linux-imx

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?

-- 
 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
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] Revert "serial: fsl_lpuart: Reset prior to registration"
  2022-09-29 11:19     ` Ilpo Järvinen
@ 2022-09-30  8:02       ` Sherry Sun
  2022-09-30 12:59         ` Ilpo Järvinen
  0 siblings, 1 reply; 11+ messages in thread
From: Sherry Sun @ 2022-09-30  8:02 UTC (permalink / raw)
  To: Ilpo Järvinen
  Cc: Greg Kroah-Hartman, Jiri Slaby, Lukas Wunner, linux-serial, LKML,
	dl-linux-imx



> 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
> >


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] Revert "serial: fsl_lpuart: Reset prior to registration"
  2022-09-30  8:02       ` Sherry Sun
@ 2022-09-30 12:59         ` Ilpo Järvinen
  2022-10-09 10:23           ` Sherry Sun
  0 siblings, 1 reply; 11+ messages in thread
From: Ilpo Järvinen @ 2022-09-30 12:59 UTC (permalink / raw)
  To: Sherry Sun
  Cc: Greg Kroah-Hartman, Jiri Slaby, Lukas Wunner, linux-serial, LKML,
	dl-linux-imx

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).

-- 
 i.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] Revert "serial: fsl_lpuart: Reset prior to registration"
  2022-09-30 12:59         ` Ilpo Järvinen
@ 2022-10-09 10:23           ` Sherry Sun
  2022-10-10  9:09             ` Lukas Wunner
  0 siblings, 1 reply; 11+ messages in thread
From: Sherry Sun @ 2022-10-09 10:23 UTC (permalink / raw)
  To: Ilpo Järvinen
  Cc: Greg Kroah-Hartman, Jiri Slaby, Lukas Wunner, linux-serial, LKML,
	dl-linux-imx



> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Sent: 2022年9月30日 20:59
> To: Sherry Sun <sherry.sun@nxp.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby
> <jirislaby@kernel.org>; Lukas Wunner <lukas@wunner.de>; 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 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).

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Revert "serial: fsl_lpuart: Reset prior to registration"
  2022-10-09 10:23           ` Sherry Sun
@ 2022-10-10  9:09             ` Lukas Wunner
  2022-10-11  8:07               ` Sherry Sun
  0 siblings, 1 reply; 11+ messages in thread
From: Lukas Wunner @ 2022-10-10  9:09 UTC (permalink / raw)
  To: Sherry Sun
  Cc: Ilpo Järvinen, Greg Kroah-Hartman, Jiri Slaby, linux-serial,
	LKML, dl-linux-imx

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] Revert "serial: fsl_lpuart: Reset prior to registration"
  2022-10-10  9:09             ` Lukas Wunner
@ 2022-10-11  8:07               ` Sherry Sun
  0 siblings, 0 replies; 11+ messages in thread
From: Sherry Sun @ 2022-10-11  8:07 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: Ilpo Järvinen, Greg Kroah-Hartman, Jiri Slaby, linux-serial,
	LKML, dl-linux-imx



> -----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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Revert "serial: fsl_lpuart: Reset prior to registration"
  2022-09-29  8:53 [PATCH] Revert "serial: fsl_lpuart: Reset prior to registration" Sherry Sun
  2022-09-29 10:51 ` Ilpo Järvinen
@ 2022-12-06 14:51 ` Francesco Dolcini
  2022-12-06 15:08   ` Francesco Dolcini
  1 sibling, 1 reply; 11+ messages in thread
From: Francesco Dolcini @ 2022-12-06 14:51 UTC (permalink / raw)
  To: Sherry Sun, gregkh, jirislaby, ilpo.jarvinen
  Cc: lukas, linux-serial, linux-kernel, linux-imx, Fabio Estevam,
	max.krummenacher

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?

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Revert "serial: fsl_lpuart: Reset prior to registration"
  2022-12-06 14:51 ` Francesco Dolcini
@ 2022-12-06 15:08   ` Francesco Dolcini
  0 siblings, 0 replies; 11+ messages in thread
From: Francesco Dolcini @ 2022-12-06 15:08 UTC (permalink / raw)
  To: Francesco Dolcini
  Cc: Sherry Sun, gregkh, jirislaby, ilpo.jarvinen, lukas,
	linux-serial, linux-kernel, linux-imx, Fabio Estevam,
	max.krummenacher

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")


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-12-06 15:13 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-29  8:53 [PATCH] Revert "serial: fsl_lpuart: Reset prior to registration" Sherry Sun
2022-09-29 10:51 ` Ilpo Järvinen
2022-09-29 11:10   ` Sherry Sun
2022-09-29 11:19     ` Ilpo Järvinen
2022-09-30  8:02       ` Sherry Sun
2022-09-30 12:59         ` Ilpo Järvinen
2022-10-09 10:23           ` Sherry Sun
2022-10-10  9:09             ` Lukas Wunner
2022-10-11  8:07               ` Sherry Sun
2022-12-06 14:51 ` Francesco Dolcini
2022-12-06 15:08   ` Francesco Dolcini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).