linux-serial.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 tty/serial 1/1] tty: serial: imx: keep console clocks always on
@ 2020-11-11  2:51 Fugang Duan
  2020-11-11  9:52 ` Uwe Kleine-König
  2020-11-12  8:40 ` Greg KH
  0 siblings, 2 replies; 4+ messages in thread
From: Fugang Duan @ 2020-11-11  2:51 UTC (permalink / raw)
  To: gregkh, u.kleine-koenig; +Cc: linux-serial, lukas, fugang.duan

For below code, there has chance to cause deadlock in SMP system:
Thread 1:
clk_enable_lock();
pr_info("debug message");
clk_enable_unlock();

Thread 2:
imx_uart_console_write()
	clk_enable()
		clk_enable_lock();

Thread 1:
Acuired clk enable_lock -> printk -> console_trylock_spinning
Thread 2:
console_unlock() -> imx_uart_console_write -> clk_disable -> Acquite clk enable_lock

So the patch is to keep console port clocks always on like
other console drivers.

Fixes: 1cf93e0d5488 ("serial: imx: remove the uart_console() check")
Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
---
v2: Add fixes tag in commit message.
---
 drivers/tty/serial/imx.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 1731d9728865..4d6c009ddc31 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2004,15 +2004,6 @@ imx_uart_console_write(struct console *co, const char *s, unsigned int count)
 	int locked = 1;
 	int retval;
 
-	retval = clk_enable(sport->clk_per);
-	if (retval)
-		return;
-	retval = clk_enable(sport->clk_ipg);
-	if (retval) {
-		clk_disable(sport->clk_per);
-		return;
-	}
-
 	if (sport->port.sysrq)
 		locked = 0;
 	else if (oops_in_progress)
@@ -2047,9 +2038,6 @@ imx_uart_console_write(struct console *co, const char *s, unsigned int count)
 
 	if (locked)
 		spin_unlock_irqrestore(&sport->port.lock, flags);
-
-	clk_disable(sport->clk_ipg);
-	clk_disable(sport->clk_per);
 }
 
 /*
@@ -2150,15 +2138,14 @@ imx_uart_console_setup(struct console *co, char *options)
 
 	retval = uart_set_options(&sport->port, co, baud, parity, bits, flow);
 
-	clk_disable(sport->clk_ipg);
 	if (retval) {
-		clk_unprepare(sport->clk_ipg);
+		clk_disable_unprepare(sport->clk_ipg);
 		goto error_console;
 	}
 
-	retval = clk_prepare(sport->clk_per);
+	retval = clk_prepare_enable(sport->clk_per);
 	if (retval)
-		clk_unprepare(sport->clk_ipg);
+		clk_disable_unprepare(sport->clk_ipg);
 
 error_console:
 	return retval;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH v2 tty/serial 1/1] tty: serial: imx: keep console clocks always on
  2020-11-11  2:51 [PATCH v2 tty/serial 1/1] tty: serial: imx: keep console clocks always on Fugang Duan
@ 2020-11-11  9:52 ` Uwe Kleine-König
  2020-11-12  8:40 ` Greg KH
  1 sibling, 0 replies; 4+ messages in thread
From: Uwe Kleine-König @ 2020-11-11  9:52 UTC (permalink / raw)
  To: Fugang Duan; +Cc: gregkh, linux-serial, lukas

[-- Attachment #1: Type: text/plain, Size: 968 bytes --]

On Wed, Nov 11, 2020 at 10:51:36AM +0800, Fugang Duan wrote:
> For below code, there has chance to cause deadlock in SMP system:
> Thread 1:
> clk_enable_lock();
> pr_info("debug message");
> clk_enable_unlock();
> 
> Thread 2:
> imx_uart_console_write()
> 	clk_enable()
> 		clk_enable_lock();
> 
> Thread 1:
> Acuired clk enable_lock -> printk -> console_trylock_spinning
> Thread 2:
> console_unlock() -> imx_uart_console_write -> clk_disable -> Acquite clk enable_lock
> 
> So the patch is to keep console port clocks always on like
> other console drivers.
> 
> Fixes: 1cf93e0d5488 ("serial: imx: remove the uart_console() check")
> Signed-off-by: Fugang Duan <fugang.duan@nxp.com>

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Cc: stable@kernel.org

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH v2 tty/serial 1/1] tty: serial: imx: keep console clocks always on
  2020-11-11  2:51 [PATCH v2 tty/serial 1/1] tty: serial: imx: keep console clocks always on Fugang Duan
  2020-11-11  9:52 ` Uwe Kleine-König
@ 2020-11-12  8:40 ` Greg KH
  2020-11-12  9:10   ` [EXT] " Andy Duan
  1 sibling, 1 reply; 4+ messages in thread
From: Greg KH @ 2020-11-12  8:40 UTC (permalink / raw)
  To: Fugang Duan; +Cc: u.kleine-koenig, linux-serial, lukas

On Wed, Nov 11, 2020 at 10:51:36AM +0800, Fugang Duan wrote:
> For below code, there has chance to cause deadlock in SMP system:
> Thread 1:
> clk_enable_lock();
> pr_info("debug message");
> clk_enable_unlock();
> 
> Thread 2:
> imx_uart_console_write()
> 	clk_enable()
> 		clk_enable_lock();
> 
> Thread 1:
> Acuired clk enable_lock -> printk -> console_trylock_spinning
> Thread 2:
> console_unlock() -> imx_uart_console_write -> clk_disable -> Acquite clk enable_lock
> 
> So the patch is to keep console port clocks always on like
> other console drivers.
> 
> Fixes: 1cf93e0d5488 ("serial: imx: remove the uart_console() check")
> Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> ---
> v2: Add fixes tag in commit message.
> ---
>  drivers/tty/serial/imx.c | 19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 1731d9728865..4d6c009ddc31 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -2004,15 +2004,6 @@ imx_uart_console_write(struct console *co, const char *s, unsigned int count)
>  	int locked = 1;
>  	int retval;
>  
> -	retval = clk_enable(sport->clk_per);
> -	if (retval)
> -		return;
> -	retval = clk_enable(sport->clk_ipg);
> -	if (retval) {
> -		clk_disable(sport->clk_per);
> -		return;
> -	}
> -
>  	if (sport->port.sysrq)
>  		locked = 0;
>  	else if (oops_in_progress)
> @@ -2047,9 +2038,6 @@ imx_uart_console_write(struct console *co, const char *s, unsigned int count)
>  
>  	if (locked)
>  		spin_unlock_irqrestore(&sport->port.lock, flags);
> -
> -	clk_disable(sport->clk_ipg);
> -	clk_disable(sport->clk_per);
>  }
>  
>  /*
> @@ -2150,15 +2138,14 @@ imx_uart_console_setup(struct console *co, char *options)
>  
>  	retval = uart_set_options(&sport->port, co, baud, parity, bits, flow);
>  
> -	clk_disable(sport->clk_ipg);
>  	if (retval) {
> -		clk_unprepare(sport->clk_ipg);
> +		clk_disable_unprepare(sport->clk_ipg);
>  		goto error_console;
>  	}
>  
> -	retval = clk_prepare(sport->clk_per);
> +	retval = clk_prepare_enable(sport->clk_per);
>  	if (retval)
> -		clk_unprepare(sport->clk_ipg);
> +		clk_disable_unprepare(sport->clk_ipg);
>  
>  error_console:
>  	return retval;
> -- 
> 2.17.1
> 

Did you test build this change and totally ignore the build warning you
now get:

drivers/tty/serial/imx.c: In function ‘imx_uart_console_write’:
drivers/tty/serial/imx.c:2011:6: warning: unused variable ‘retval’ [-Wunused-variable]
 2011 |  int retval;
      |      ^~~~~~

Not good...

I'll go fix it.

greg k-h

^ permalink raw reply	[flat|nested] 4+ messages in thread
* RE: [EXT] Re: [PATCH v2 tty/serial 1/1] tty: serial: imx: keep console clocks always on
  2020-11-12  8:40 ` Greg KH
@ 2020-11-12  9:10   ` Andy Duan
  0 siblings, 0 replies; 4+ messages in thread
From: Andy Duan @ 2020-11-12  9:10 UTC (permalink / raw)
  To: Greg KH; +Cc: u.kleine-koenig, linux-serial, lukas

From: Greg KH <gregkh@linuxfoundation.org> Sent: Thursday, November 12, 2020 4:40 PM
> On Wed, Nov 11, 2020 at 10:51:36AM +0800, Fugang Duan wrote:
> > For below code, there has chance to cause deadlock in SMP system:
> > Thread 1:
> > clk_enable_lock();
> > pr_info("debug message");
> > clk_enable_unlock();
> >
> > Thread 2:
> > imx_uart_console_write()
> >       clk_enable()
> >               clk_enable_lock();
> >
> > Thread 1:
> > Acuired clk enable_lock -> printk -> console_trylock_spinning Thread
> > 2:
> > console_unlock() -> imx_uart_console_write -> clk_disable -> Acquite
> > clk enable_lock
> >
> > So the patch is to keep console port clocks always on like other
> > console drivers.
> >
> > Fixes: 1cf93e0d5488 ("serial: imx: remove the uart_console() check")
> > Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> > ---
> > v2: Add fixes tag in commit message.
> > ---
> >  drivers/tty/serial/imx.c | 19 +++----------------
> >  1 file changed, 3 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index
> > 1731d9728865..4d6c009ddc31 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -2004,15 +2004,6 @@ imx_uart_console_write(struct console *co, const
> char *s, unsigned int count)
> >       int locked = 1;
> >       int retval;
> >
> > -     retval = clk_enable(sport->clk_per);
> > -     if (retval)
> > -             return;
> > -     retval = clk_enable(sport->clk_ipg);
> > -     if (retval) {
> > -             clk_disable(sport->clk_per);
> > -             return;
> > -     }
> > -
> >       if (sport->port.sysrq)
> >               locked = 0;
> >       else if (oops_in_progress)
> > @@ -2047,9 +2038,6 @@ imx_uart_console_write(struct console *co, const
> > char *s, unsigned int count)
> >
> >       if (locked)
> >               spin_unlock_irqrestore(&sport->port.lock, flags);
> > -
> > -     clk_disable(sport->clk_ipg);
> > -     clk_disable(sport->clk_per);
> >  }
> >
> >  /*
> > @@ -2150,15 +2138,14 @@ imx_uart_console_setup(struct console *co,
> > char *options)
> >
> >       retval = uart_set_options(&sport->port, co, baud, parity, bits,
> > flow);
> >
> > -     clk_disable(sport->clk_ipg);
> >       if (retval) {
> > -             clk_unprepare(sport->clk_ipg);
> > +             clk_disable_unprepare(sport->clk_ipg);
> >               goto error_console;
> >       }
> >
> > -     retval = clk_prepare(sport->clk_per);
> > +     retval = clk_prepare_enable(sport->clk_per);
> >       if (retval)
> > -             clk_unprepare(sport->clk_ipg);
> > +             clk_disable_unprepare(sport->clk_ipg);
> >
> >  error_console:
> >       return retval;
> > --
> > 2.17.1
> >
> 
> Did you test build this change and totally ignore the build warning you now get:
> 
> drivers/tty/serial/imx.c: In function ‘imx_uart_console_write’:
> drivers/tty/serial/imx.c:2011:6: warning: unused variable ‘retval’
> [-Wunused-variable]
>  2011 |  int retval;
>       |      ^~~~~~
> 
> Not good...
> 
> I'll go fix it.
> 
> greg k-h

Thanks, Greg, I ignore the build warning.

Andy

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-11-12  9:10 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-11  2:51 [PATCH v2 tty/serial 1/1] tty: serial: imx: keep console clocks always on Fugang Duan
2020-11-11  9:52 ` Uwe Kleine-König
2020-11-12  8:40 ` Greg KH
2020-11-12  9:10   ` [EXT] " Andy Duan

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