Skip to contentSkip to conversation

serial: imx: fix error handling in console_setup

PATCH In review Stefan Agner 2 participants 5 messages Nov 14, 2018 – Nov 23, 2018

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/3 serial: imx: fix error handling in console_setup

The ipg clock only needs to be unprepared in case preparing per clock fails. The ipg clock has already disabled at the point.

Fixes 1cf93e0d5488 ("serial: imx: remove the uart_console() check")
---
 drivers/tty/serial/imx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/tty/serial/imx.c +1 -1
index 0f67197a3783..313c3b1900a8 100644
@@ -2068,7 +2068,7 @@ imx_uart_console_setup(struct console *co, char *options)
 
 	retval = clk_prepare(sport->clk_per);
 	if (retval)
-		clk_disable_unprepare(sport->clk_ipg);
+		clk_unprepare(sport->clk_ipg);
 
 error_console:
 	return retval;
Signature · 1 line
2.19.1
#1 lore raw

PATCH 2/3 serial: imx: unprepare console clocks on remove

Currently imx_uart_console_setup() prepares clocks which do not get unprepared anywhere. Check whether the console has been used by testing if index is set and unprepare clocks in this case.

This makes sure that clocks are properly unprepared after the console device has been unbound.

---
 drivers/tty/serial/imx.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/tty/serial/imx.c +11 -2
index 313c3b1900a8..757c91e5105a 100644
@@ -2085,7 +2085,7 @@ static struct console imx_uart_console = {
 	.data		= &imx_uart_uart_driver,
 };
 
-#define IMX_CONSOLE	&imx_uart_console
+#define IMX_CONSOLE	(&imx_uart_console)
 
 #ifdef CONFIG_OF
 static void imx_uart_console_early_putchar(struct uart_port *port, int ch)
@@ -2378,8 +2378,17 @@ static int imx_uart_probe(struct platform_device *pdev)
 static int imx_uart_remove(struct platform_device *pdev)
 {
 	struct imx_port *sport = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = uart_remove_one_port(&imx_uart_uart_driver, &sport->port);
+
+	if (IS_ENABLED(CONFIG_SERIAL_IMX_CONSOLE) && IMX_CONSOLE->index >= 0) {
+		clk_unprepare(sport->clk_ipg);
+		clk_unprepare(sport->clk_per);
+		IMX_CONSOLE->index = -1;
+	}
 
-	return uart_remove_one_port(&imx_uart_uart_driver, &sport->port);
+	return ret;
 }
 
 static void imx_uart_restore_context(struct imx_port *sport)
Signature · 1 line
2.19.1
#2 lore raw

Re: [PATCH 2/3] serial: imx: unprepare console clocks on remove

Quoted 31 lines · Stefan Agner

Currently imx_uart_console_setup() prepares clocks which do not get unprepared anywhere. Check whether the console has been used by testing if index is set and unprepare clocks in this case.

This makes sure that clocks are properly unprepared after the console device has been unbound.

---
 drivers/tty/serial/imx.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/tty/serial/imx.c +3 -1
index 313c3b1900a8..757c91e5105a 100644
@@ -2085,7 +2085,7 @@ static struct console imx_uart_console = {
 	.data		= &imx_uart_uart_driver,
 };
 
-#define IMX_CONSOLE	&imx_uart_console
+#define IMX_CONSOLE	(&imx_uart_console)
 
 #ifdef CONFIG_OF
 static void imx_uart_console_early_putchar(struct uart_port *port, int ch)
@@ -2378,8 +2378,17 @@ static int imx_uart_probe(struct platform_device *pdev)
 static int imx_uart_remove(struct platform_device *pdev)
 {
 	struct imx_port *sport = platform_get_drvdata(pdev);
+	int ret;
+

drivers/tty/serial/imx.c static int imx_uart_probe(struct platform_device *pdev) · line 2391

Quoted 10 lines
drivers/tty/serial/imx.c +8 -1
+	ret = uart_remove_one_port(&imx_uart_uart_driver, &sport->port);
+
+	if (IS_ENABLED(CONFIG_SERIAL_IMX_CONSOLE) && IMX_CONSOLE->index >= 0) {
+		clk_unprepare(sport->clk_ipg);
+		clk_unprepare(sport->clk_per);
+		IMX_CONSOLE->index = -1;
+	}
 
-	return uart_remove_one_port(&imx_uart_uart_driver, &sport->port);
+	return ret;

I doubt this is right. imx_uart_console_setup is called once, and if the console is on (say) ttymxc0 you don't want to unprepare the clocks if ttymxc3 gets unbound.

So I think this cleanup must go into imx_uart_exit().

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#3 lore raw

PATCH 3/3 serial: imx: avoid crash when un/re-binding serial console device

In reply to Stefan Agner

If the device used as a serial console gets un/re-binded, then register_console() will call imx_uart_setup_console() again. Drop __init so that imx_uart_setup_console() can be safely called at runtime.

---
This addresses a kernel panic seen when unbinding/rebinding the i.MX UART which is serial console on i.MX 6/7 via SSH:

# cd /sys/bus/platform/drivers/imx-uart/
# echo 30860000.serial > unbind && echo 30860000.serial > bind

console [ttymxc0] disabled
30860000.serial: ttymxc0 at MMIO 0x30860000 (irq = 52, base_baud = 1500000) is a IMX

Unable to handle kernel paging request at virtual address c0c4b830
pgd = 5e12e3d4
[c0c4b830] *pgd=80c1141e(bad)
Internal error: Oops: 8000000d [#1] SMP ARM
Modules linked in:
CPU: 0 PID: 866 Comm: sh Not tainted 4.18.15-00048-gb3b505988801-dirty #403
Hardware name: Freescale i.MX7 Dual (Device Tree)
PC is at imx_uart_console_setup+0x0/0x274
LR is at register_console+0x184/0x3c4
pc : [<c0c4b830>]    lr : [<c0171314>]    psr: a0070013

sp : e8015db8 ip : c0d06548 fp : c0b4a158
r10: ec1d9380 r9 : 00000001 r8 : 00000000
r7 : 00000000 r6 : c0d819e0 r5 : c0d81e48 r4 : c0d47d68 r3 : c0c4b830 r2 : 00000000 r1 : efffca03 r0 : c0d47d68
Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
Control: 10c5387d Table: a803806a DAC: 00000051 Process sh (pid: 866, stack limit = 0x9c2f1d49)

It seems that also other drivers are affected. An alternative might be to disallow unbinding/rebinding instead.

 drivers/tty/serial/imx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/serial/imx.c +2 -2
index 757c91e5105a..674bd0ea2491 100644
@@ -1966,7 +1966,7 @@ imx_uart_console_write(struct console *co, const char *s, unsigned int count)
  * If the port was already initialised (eg, by a boot loader),
  * try to determine the current setup.
  */
-static void __init
+static void
 imx_uart_console_get_options(struct imx_port *sport, int *baud,
 			     int *parity, int *bits)
 {
@@ -2025,7 +2025,7 @@ imx_uart_console_get_options(struct imx_port *sport, int *baud,
 	}
 }
 
-static int __init
+static int
 imx_uart_console_setup(struct console *co, char *options)
 {
 	struct imx_port *sport;
Signature · 1 line
2.19.1
Reviewed-by on v1 1/3
#4 lore raw
In reply to Stefan Agner
Quoted 14 lines · Stefan Agner

The ipg clock only needs to be unprepared in case preparing per clock fails. The ipg clock has already disabled at the point.

Fixes 1cf93e0d5488 ("serial: imx: remove the uart_console() check")
---
 drivers/tty/serial/imx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/tty/serial/imx.c +0 -0
index 0f67197a3783..313c3b1900a8 100644

drivers/tty/serial/imx.c imx_uart_console_setup(struct console *co, char *options) · line 2071

Quoted 5 lines
drivers/tty/serial/imx.c +1 -1
@@ -2068,7 +2068,7 @@ imx_uart_console_setup(struct console *co, char *options)
 
 	retval = clk_prepare(sport->clk_per);
 	if (retval)
-		clk_disable_unprepare(sport->clk_ipg);
+		clk_unprepare(sport->clk_ipg);

good catch,

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |