* (no subject) @ 2021-10-14 7:10 Francesco Dolcini 2021-10-14 7:10 ` [PATCH 1/2] serial: imx: fix crash when un/re-binding console Francesco Dolcini ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Francesco Dolcini @ 2021-10-14 7:10 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Stefan Agner Cc: Francesco Dolcini, linux-serial, linux-arm-kernel, linux-kernel Date: Thu, 14 Oct 2021 00:12:55 +0200 Subject: [PATCH 0/2] serial: imx: fix unbind/bind console Hi, here a couple of fixes to properly handle imx serial console bind/unbind at runtime. The patch from Stefan was already posted years ago but never merged [1], however is still valid and it's pretty easy to trigger the problem. [1] https://marc.info/?l=linux-serial&m=154221779312036&w=2 Francesco Dolcini (1): serial: imx: disable console clocks on unregister Stefan Agner (1): serial: imx: fix crash when un/re-binding console drivers/tty/serial/imx.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] serial: imx: fix crash when un/re-binding console 2021-10-14 7:10 Francesco Dolcini @ 2021-10-14 7:10 ` Francesco Dolcini 2021-10-14 7:33 ` Greg Kroah-Hartman 2021-10-14 7:10 ` [PATCH 2/2] serial: imx: disable console clocks on unregister Francesco Dolcini 2021-10-14 7:17 ` [PATCH 0/2] serial: imx: fix unbind/bind console Francesco Dolcini 2 siblings, 1 reply; 9+ messages in thread From: Francesco Dolcini @ 2021-10-14 7:10 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Stefan Agner Cc: Francesco Dolcini, linux-serial, linux-arm-kernel, linux-kernel From: Stefan Agner <stefan@agner.ch> 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. Signed-off-by: Stefan Agner <stefan@agner.ch> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> --- drivers/tty/serial/imx.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index 8b121cd869e9..51a9f9423b1a 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c @@ -2017,7 +2017,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) { @@ -2076,7 +2076,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; -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] serial: imx: fix crash when un/re-binding console 2021-10-14 7:10 ` [PATCH 1/2] serial: imx: fix crash when un/re-binding console Francesco Dolcini @ 2021-10-14 7:33 ` Greg Kroah-Hartman 2021-10-14 8:01 ` Francesco Dolcini 0 siblings, 1 reply; 9+ messages in thread From: Greg Kroah-Hartman @ 2021-10-14 7:33 UTC (permalink / raw) To: Francesco Dolcini Cc: Jiri Slaby, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Stefan Agner, linux-serial, linux-arm-kernel, linux-kernel On Thu, Oct 14, 2021 at 09:10:52AM +0200, Francesco Dolcini wrote: > From: Stefan Agner <stefan@agner.ch> > > 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. > > Signed-off-by: Stefan Agner <stefan@agner.ch> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > --- > drivers/tty/serial/imx.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) What commit does this "fix"? Should this go to stable kernels? If so, how far back? > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > index 8b121cd869e9..51a9f9423b1a 100644 > --- a/drivers/tty/serial/imx.c > +++ b/drivers/tty/serial/imx.c > @@ -2017,7 +2017,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) > { > @@ -2076,7 +2076,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) Why didn't we get a build warning about this section being called by code that was not thrown away? That feels odd... thanks, greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] serial: imx: fix crash when un/re-binding console 2021-10-14 7:33 ` Greg Kroah-Hartman @ 2021-10-14 8:01 ` Francesco Dolcini 2021-10-14 8:10 ` Uwe Kleine-König 2021-10-14 10:28 ` Francesco Dolcini 0 siblings, 2 replies; 9+ messages in thread From: Francesco Dolcini @ 2021-10-14 8:01 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Francesco Dolcini, Jiri Slaby, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Stefan Agner, linux-serial, linux-arm-kernel, linux-kernel Hello Greg, On Thu, Oct 14, 2021 at 09:33:55AM +0200, Greg Kroah-Hartman wrote: > On Thu, Oct 14, 2021 at 09:10:52AM +0200, Francesco Dolcini wrote: > > From: Stefan Agner <stefan@agner.ch> > > > > 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. > > > > Signed-off-by: Stefan Agner <stefan@agner.ch> > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > --- > > drivers/tty/serial/imx.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > What commit does this "fix"? root@colibri-imx6ull-06746657:~# cat /sys/devices/virtual/tty/console/active tty1 ttymxc0 root@colibri-imx6ull-06746657:~# echo -n N > /sys/devices/virtual/tty/console/subsystem/ttymxc0/console root@colibri-imx6ull-06746657:~# echo -n Y > /sys/devices/virtual/tty/console/subsystem/ttymxc0/console struct console ->setup is called at that time, but imx_uart_console_setup() is gone ... According to the original report from Stefan the issue is the same with unbind/bind of the console, see https://marc.info/?l=linux-serial&m=154221779312036&w=2. > Should this go to stable kernels? If so, how far back? This is present also in 4.19 kernel, I feel like the issue is there since it is possible to unbind a console driver (since ever considering the current LTS releases?). I could investigate this a little bit more. We have this patch in our kernel branch since 3 years but for some reason we never managed to upstream it. Francesco ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] serial: imx: fix crash when un/re-binding console 2021-10-14 8:01 ` Francesco Dolcini @ 2021-10-14 8:10 ` Uwe Kleine-König 2021-10-14 9:12 ` Francesco Dolcini 2021-10-14 10:28 ` Francesco Dolcini 1 sibling, 1 reply; 9+ messages in thread From: Uwe Kleine-König @ 2021-10-14 8:10 UTC (permalink / raw) To: Francesco Dolcini Cc: Greg Kroah-Hartman, Stefan Agner, linux-serial, Jiri Slaby, Sascha Hauer, linux-kernel, Shawn Guo, NXP Linux Team, Pengutronix Kernel Team, Fabio Estevam, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 2102 bytes --] On Thu, Oct 14, 2021 at 10:01:53AM +0200, Francesco Dolcini wrote: > Hello Greg, > > On Thu, Oct 14, 2021 at 09:33:55AM +0200, Greg Kroah-Hartman wrote: > > On Thu, Oct 14, 2021 at 09:10:52AM +0200, Francesco Dolcini wrote: > > > From: Stefan Agner <stefan@agner.ch> > > > > > > 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. > > > > > > Signed-off-by: Stefan Agner <stefan@agner.ch> > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > > --- > > > drivers/tty/serial/imx.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > What commit does this "fix"? > > root@colibri-imx6ull-06746657:~# cat /sys/devices/virtual/tty/console/active > tty1 ttymxc0 > root@colibri-imx6ull-06746657:~# echo -n N > /sys/devices/virtual/tty/console/subsystem/ttymxc0/console > root@colibri-imx6ull-06746657:~# echo -n Y > /sys/devices/virtual/tty/console/subsystem/ttymxc0/console > > struct console ->setup is called at that time, but imx_uart_console_setup() is gone ... > > According to the original report from Stefan the issue is the same with unbind/bind of the console, > see https://marc.info/?l=linux-serial&m=154221779312036&w=2. > > > > Should this go to stable kernels? If so, how far back? > This is present also in 4.19 kernel, I feel like the issue is there since it is possible to unbind > a console driver (since ever considering the current LTS releases?). I could > investigate this a little bit more. > > We have this patch in our kernel branch since 3 years but for some reason we never managed to upstream it. I think this is a bigger problem not only affecting imx.c. Just look at the output of: git grep -E '__init.*setup' drivers/tty/serial/ Best regards 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] 9+ messages in thread
* Re: [PATCH 1/2] serial: imx: fix crash when un/re-binding console 2021-10-14 8:10 ` Uwe Kleine-König @ 2021-10-14 9:12 ` Francesco Dolcini 0 siblings, 0 replies; 9+ messages in thread From: Francesco Dolcini @ 2021-10-14 9:12 UTC (permalink / raw) To: Uwe Kleine-König Cc: Francesco Dolcini, Greg Kroah-Hartman, Stefan Agner, linux-serial, Jiri Slaby, Sascha Hauer, linux-kernel, Shawn Guo, NXP Linux Team, Pengutronix Kernel Team, Fabio Estevam, linux-arm-kernel Hello Uwe, On Thu, Oct 14, 2021 at 10:10:28AM +0200, Uwe Kleine-König wrote: > On Thu, Oct 14, 2021 at 10:01:53AM +0200, Francesco Dolcini wrote: > > Hello Greg, > > > > On Thu, Oct 14, 2021 at 09:33:55AM +0200, Greg Kroah-Hartman wrote: > > > On Thu, Oct 14, 2021 at 09:10:52AM +0200, Francesco Dolcini wrote: > > > > From: Stefan Agner <stefan@agner.ch> > > > > > > > > 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. > > > > > > > > Signed-off-by: Stefan Agner <stefan@agner.ch> > > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > > > --- > > > > drivers/tty/serial/imx.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > What commit does this "fix"? > > > > root@colibri-imx6ull-06746657:~# cat /sys/devices/virtual/tty/console/active > > tty1 ttymxc0 > > root@colibri-imx6ull-06746657:~# echo -n N > /sys/devices/virtual/tty/console/subsystem/ttymxc0/console > > root@colibri-imx6ull-06746657:~# echo -n Y > /sys/devices/virtual/tty/console/subsystem/ttymxc0/console > > > > struct console ->setup is called at that time, but imx_uart_console_setup() is gone ... > > > > According to the original report from Stefan the issue is the same with unbind/bind of the console, > > see https://marc.info/?l=linux-serial&m=154221779312036&w=2. > > > > > > > Should this go to stable kernels? If so, how far back? > > This is present also in 4.19 kernel, I feel like the issue is there since it is possible to unbind > > a console driver (since ever considering the current LTS releases?). I could > > investigate this a little bit more. > > > > We have this patch in our kernel branch since 3 years but for some reason we never managed to upstream it. > > I think this is a bigger problem not only affecting imx.c. Just look at > the output of: > > git grep -E '__init.*setup' drivers/tty/serial/ I agree, but I think that all the earlyconsole stuff is not affected, so the amount of drivers that should be fixed is less than it looks like. Apart for that once you fix this issue you need to be sure that on the unbind/unregister path you undo all you did in the setup, maybe for most of the drivers is nothing, but for serial/imx I had to disable the clock there (see patch 2/2 in this patchset). Francesco ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] serial: imx: fix crash when un/re-binding console 2021-10-14 8:01 ` Francesco Dolcini 2021-10-14 8:10 ` Uwe Kleine-König @ 2021-10-14 10:28 ` Francesco Dolcini 1 sibling, 0 replies; 9+ messages in thread From: Francesco Dolcini @ 2021-10-14 10:28 UTC (permalink / raw) To: Greg Kroah-Hartman, Andy Shevchenko Cc: Jiri Slaby, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Stefan Agner, linux-serial, linux-arm-kernel, linux-kernel On Thu, Oct 14, 2021 at 10:01:53AM +0200, Francesco Dolcini wrote: > Hello Greg, > > On Thu, Oct 14, 2021 at 09:33:55AM +0200, Greg Kroah-Hartman wrote: > > On Thu, Oct 14, 2021 at 09:10:52AM +0200, Francesco Dolcini wrote: > > > From: Stefan Agner <stefan@agner.ch> > > > > > > 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. > > > > > > Signed-off-by: Stefan Agner <stefan@agner.ch> > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > > --- > > > drivers/tty/serial/imx.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > What commit does this "fix"? > > root@colibri-imx6ull-06746657:~# cat /sys/devices/virtual/tty/console/active > tty1 ttymxc0 > root@colibri-imx6ull-06746657:~# echo -n N > /sys/devices/virtual/tty/console/subsystem/ttymxc0/console > root@colibri-imx6ull-06746657:~# echo -n Y > /sys/devices/virtual/tty/console/subsystem/ttymxc0/console > > struct console ->setup is called at that time, but imx_uart_console_setup() is gone ... This specific way I described here to reproduce the issue was introduced with a3cb39d258ef serial: core: Allow detach and attach serial device for console that is kernel 5.9+ (Andy added to to:) But the original report was about bind/unbind of the uart driver, I guess this is possible since way more time. > > According to the original report from Stefan the issue is the same with unbind/bind of the console, > see https://marc.info/?l=linux-serial&m=154221779312036&w=2. > > > > Should this go to stable kernels? If so, how far back? > This is present also in 4.19 kernel, I feel like the issue is there since it is possible to unbind > a console driver (since ever considering the current LTS releases?). I could > investigate this a little bit more. > > We have this patch in our kernel branch since 3 years but for some reason we never managed to upstream it. > > > Francesco ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] serial: imx: disable console clocks on unregister 2021-10-14 7:10 Francesco Dolcini 2021-10-14 7:10 ` [PATCH 1/2] serial: imx: fix crash when un/re-binding console Francesco Dolcini @ 2021-10-14 7:10 ` Francesco Dolcini 2021-10-14 7:17 ` [PATCH 0/2] serial: imx: fix unbind/bind console Francesco Dolcini 2 siblings, 0 replies; 9+ messages in thread From: Francesco Dolcini @ 2021-10-14 7:10 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Stefan Agner Cc: Francesco Dolcini, linux-serial, linux-arm-kernel, linux-kernel During console setup imx_uart_console_setup() enables clocks, but they are never disabled when the console is unregistered, this leads to clk_prepare_enable() being called multiple times without a matching clk_disable_unprepare() in case of unbind/bind. Ensure that clock enable/disable are balanced adding clk_disable_unprepare() in the console exit callback. Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> --- drivers/tty/serial/imx.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index 51a9f9423b1a..90f82e6c54e4 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c @@ -2124,12 +2124,24 @@ imx_uart_console_setup(struct console *co, char *options) return retval; } +static int +imx_uart_console_exit(struct console *co) +{ + struct imx_port *sport = imx_uart_ports[co->index]; + + clk_disable_unprepare(sport->clk_per); + clk_disable_unprepare(sport->clk_ipg); + + return 0; +} + static struct uart_driver imx_uart_uart_driver; static struct console imx_uart_console = { .name = DEV_NAME, .write = imx_uart_console_write, .device = uart_console_device, .setup = imx_uart_console_setup, + .exit = imx_uart_console_exit, .flags = CON_PRINTBUFFER, .index = -1, .data = &imx_uart_uart_driver, -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 0/2] serial: imx: fix unbind/bind console 2021-10-14 7:10 Francesco Dolcini 2021-10-14 7:10 ` [PATCH 1/2] serial: imx: fix crash when un/re-binding console Francesco Dolcini 2021-10-14 7:10 ` [PATCH 2/2] serial: imx: disable console clocks on unregister Francesco Dolcini @ 2021-10-14 7:17 ` Francesco Dolcini 2 siblings, 0 replies; 9+ messages in thread From: Francesco Dolcini @ 2021-10-14 7:17 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Stefan Agner Cc: Francesco Dolcini, linux-serial, linux-arm-kernel, linux-kernel Hi, here a couple of fixes to properly handle imx serial console bind/unbind at runtime. The patch from Stefan was already posted years ago but never merged [1], however is still valid and it's pretty easy to trigger the problem. [1] https://marc.info/?l=linux-serial&m=154221779312036&w=2 Francesco Dolcini (1): serial: imx: disable console clocks on unregister Stefan Agner (1): serial: imx: fix crash when un/re-binding console drivers/tty/serial/imx.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-10-14 10:28 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-14 7:10 Francesco Dolcini 2021-10-14 7:10 ` [PATCH 1/2] serial: imx: fix crash when un/re-binding console Francesco Dolcini 2021-10-14 7:33 ` Greg Kroah-Hartman 2021-10-14 8:01 ` Francesco Dolcini 2021-10-14 8:10 ` Uwe Kleine-König 2021-10-14 9:12 ` Francesco Dolcini 2021-10-14 10:28 ` Francesco Dolcini 2021-10-14 7:10 ` [PATCH 2/2] serial: imx: disable console clocks on unregister Francesco Dolcini 2021-10-14 7:17 ` [PATCH 0/2] serial: imx: fix unbind/bind console 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).