* [PATCH] tty/serial: do not free trasnmit buffer page under port lock
From: Sergey Senozhatsky @ 2018-12-13 4:58 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: linux-serial, linux-kernel, Sergey Senozhatsky, Andrew Morton,
Peter Zijlstra, Waiman Long, Dmitry Safonov, Steven Rostedt,
Petr Mladek
LKP has hit yet another circular locking dependency between uart
console drivers and debugobjects [1]:
CPU0 CPU1
rhltable_init()
__init_work()
debug_object_init
uart_shutdown() /* db->lock */
/* uart_port->lock */ debug_print_object()
free_page() printk()
call_console_drivers()
debug_check_no_obj_freed() /* uart_port->lock */
/* db->lock */
debug_print_object()
So there are two dependency chains:
uart_port->lock -> db->lock
And
db->lock -> uart_port->lock
This particular circular locking dependency can be addressed in several
ways:
a) One way would be to move debug_print_object() out of db->lock scope
and, thus, break the db->lock -> uart_port->lock chain.
b) Another one would be to free() transmit buffer page out of db->lock
in UART code; which is what this patch does.
It makes sense to apply a) and b) independently: there are too many things
going on behind free(), none of which depend on uart_port->lock.
The patch fixes transmit buffer page free() in uart_shutdown() and,
additionally, in uart_port_startup() (as was suggested by Dmitry Safonov).
[1] https://lore.kernel.org/lkml/20181211091154.GL23332@shao2-debian/T/#u
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Waiman Long <longman@redhat.com>
Cc: Dmitry Safonov <dima@arista.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Petr Mladek <pmladek@suse.com>
---
drivers/tty/serial/serial_core.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index c439a5a1e6c0..d4cca5bdaf1c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -205,10 +205,15 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
if (!state->xmit.buf) {
state->xmit.buf = (unsigned char *) page;
uart_circ_clear(&state->xmit);
+ uart_port_unlock(uport, flags);
} else {
+ uart_port_unlock(uport, flags);
+ /*
+ * Do not free() the page under the port lock, see
+ * uart_shutdown().
+ */
free_page(page);
}
- uart_port_unlock(uport, flags);
retval = uport->ops->startup(uport);
if (retval == 0) {
@@ -268,6 +273,7 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
struct uart_port *uport = uart_port_check(state);
struct tty_port *port = &state->port;
unsigned long flags = 0;
+ char *xmit_buf = NULL;
/*
* Set the TTY IO error marker
@@ -298,14 +304,18 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
tty_port_set_suspended(port, 0);
/*
- * Free the transmit buffer page.
+ * Do not free() the transmit buffer page under the port lock since
+ * this can create various circular locking scenarios. For instance,
+ * console driver may need to allocate/free a debug object, which
+ * can endup in printk() recursion.
*/
uart_port_lock(state, flags);
- if (state->xmit.buf) {
- free_page((unsigned long)state->xmit.buf);
- state->xmit.buf = NULL;
- }
+ xmit_buf = state->xmit.buf;
+ state->xmit.buf = NULL;
uart_port_unlock(uport, flags);
+
+ if (xmit_buf)
+ free_page((unsigned long)xmit_buf);
}
/**
--
2.20.0
^ permalink raw reply related
* [PATCH AUTOSEL 3.18 09/16] drivers/tty: add missing of_node_put()
From: Sasha Levin @ 2018-12-13 4:33 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Yangtao Li, David S . Miller, Sasha Levin, sparclinux,
linux-serial
In-Reply-To: <20181213043343.76896-1-sashal@kernel.org>
From: Yangtao Li <tiny.windzz@gmail.com>
[ Upstream commit dac097c4546e4c5b16dd303a1e97c1d319c8ab3e ]
of_find_node_by_path() acquires a reference to the node
returned by it and that reference needs to be dropped by its caller.
This place is not doing this, so fix it.
Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/tty/serial/suncore.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/tty/serial/suncore.c b/drivers/tty/serial/suncore.c
index 6e4ac8db2d79..2b06c1603f23 100644
--- a/drivers/tty/serial/suncore.c
+++ b/drivers/tty/serial/suncore.c
@@ -112,6 +112,7 @@ void sunserial_console_termios(struct console *con, struct device_node *uart_dp)
mode = of_get_property(dp, mode_prop, NULL);
if (!mode)
mode = "9600,8,n,1,-";
+ of_node_put(dp);
}
cflag = CREAD | HUPCL | CLOCAL;
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.4 12/23] drivers/tty: add missing of_node_put()
From: Sasha Levin @ 2018-12-13 4:32 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Yangtao Li, David S . Miller, Sasha Levin, sparclinux,
linux-serial
In-Reply-To: <20181213043259.76643-1-sashal@kernel.org>
From: Yangtao Li <tiny.windzz@gmail.com>
[ Upstream commit dac097c4546e4c5b16dd303a1e97c1d319c8ab3e ]
of_find_node_by_path() acquires a reference to the node
returned by it and that reference needs to be dropped by its caller.
This place is not doing this, so fix it.
Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/tty/serial/suncore.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/tty/serial/suncore.c b/drivers/tty/serial/suncore.c
index 127472bd6a7c..209f314745ab 100644
--- a/drivers/tty/serial/suncore.c
+++ b/drivers/tty/serial/suncore.c
@@ -111,6 +111,7 @@ void sunserial_console_termios(struct console *con, struct device_node *uart_dp)
mode = of_get_property(dp, mode_prop, NULL);
if (!mode)
mode = "9600,8,n,1,-";
+ of_node_put(dp);
}
cflag = CREAD | HUPCL | CLOCAL;
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.9 13/34] drivers/tty: add missing of_node_put()
From: Sasha Levin @ 2018-12-13 4:31 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Yangtao Li, David S . Miller, Sasha Levin, sparclinux,
linux-serial
In-Reply-To: <20181213043200.76295-1-sashal@kernel.org>
From: Yangtao Li <tiny.windzz@gmail.com>
[ Upstream commit dac097c4546e4c5b16dd303a1e97c1d319c8ab3e ]
of_find_node_by_path() acquires a reference to the node
returned by it and that reference needs to be dropped by its caller.
This place is not doing this, so fix it.
Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/tty/serial/suncore.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/tty/serial/suncore.c b/drivers/tty/serial/suncore.c
index 127472bd6a7c..209f314745ab 100644
--- a/drivers/tty/serial/suncore.c
+++ b/drivers/tty/serial/suncore.c
@@ -111,6 +111,7 @@ void sunserial_console_termios(struct console *con, struct device_node *uart_dp)
mode = of_get_property(dp, mode_prop, NULL);
if (!mode)
mode = "9600,8,n,1,-";
+ of_node_put(dp);
}
cflag = CREAD | HUPCL | CLOCAL;
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.14 15/41] drivers/tty: add missing of_node_put()
From: Sasha Levin @ 2018-12-13 4:30 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Yangtao Li, David S . Miller, Sasha Levin, sparclinux,
linux-serial
In-Reply-To: <20181213043054.75891-1-sashal@kernel.org>
From: Yangtao Li <tiny.windzz@gmail.com>
[ Upstream commit dac097c4546e4c5b16dd303a1e97c1d319c8ab3e ]
of_find_node_by_path() acquires a reference to the node
returned by it and that reference needs to be dropped by its caller.
This place is not doing this, so fix it.
Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/tty/serial/suncore.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/tty/serial/suncore.c b/drivers/tty/serial/suncore.c
index 127472bd6a7c..209f314745ab 100644
--- a/drivers/tty/serial/suncore.c
+++ b/drivers/tty/serial/suncore.c
@@ -111,6 +111,7 @@ void sunserial_console_termios(struct console *con, struct device_node *uart_dp)
mode = of_get_property(dp, mode_prop, NULL);
if (!mode)
mode = "9600,8,n,1,-";
+ of_node_put(dp);
}
cflag = CREAD | HUPCL | CLOCAL;
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 30/73] drivers/tty: add missing of_node_put()
From: Sasha Levin @ 2018-12-13 4:27 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Yangtao Li, David S . Miller, Sasha Levin, sparclinux,
linux-serial
In-Reply-To: <20181213042838.75160-1-sashal@kernel.org>
From: Yangtao Li <tiny.windzz@gmail.com>
[ Upstream commit dac097c4546e4c5b16dd303a1e97c1d319c8ab3e ]
of_find_node_by_path() acquires a reference to the node
returned by it and that reference needs to be dropped by its caller.
This place is not doing this, so fix it.
Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/tty/serial/suncore.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/tty/serial/suncore.c b/drivers/tty/serial/suncore.c
index 70a4ea4eaa6e..990376576970 100644
--- a/drivers/tty/serial/suncore.c
+++ b/drivers/tty/serial/suncore.c
@@ -112,6 +112,7 @@ void sunserial_console_termios(struct console *con, struct device_node *uart_dp)
mode = of_get_property(dp, mode_prop, NULL);
if (!mode)
mode = "9600,8,n,1,-";
+ of_node_put(dp);
}
cflag = CREAD | HUPCL | CLOCAL;
--
2.19.1
^ permalink raw reply related
* Re: [PATCH] serial: 8250: Fix clearing FIFOs in RS485 mode again
From: Marek Vasut @ 2018-12-13 4:18 UTC (permalink / raw)
To: Paul Burton
Cc: Greg Kroah-Hartman, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org
In-Reply-To: <20181213033301.febmn5qt3chn3vqb@pburton-laptop>
On 12/13/2018 04:33 AM, Paul Burton wrote:
> Hi Marek,
Hello Paul,
> On Thu, Dec 13, 2018 at 03:27:51AM +0100, Marek Vasut wrote:
>>> I wonder whether the issue may be the JZ4780 UART not automatically
>>> resetting the FIFOs when FCR[0] changes. This is a guess, but the manual
>>> doesn't explicitly say it'll happen & the programming example it gives
>>> says to explicitly clear the FIFOs using FCR[2:1]. Since this is what
>>> the kernel has been doing for at least the whole git era I wouldn't be
>>> surprised if other devices are bitten by the change as people start
>>> trying 4.20 on them.
>>
>> The patch you're complaining about is doing exactly that -- it sets
>> UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT in FCR , and then clears it.
>> Those two bits are exactly FCR[2:1]. It also explicitly does not touch
>> any other bits in FCR.
>
> No - your patch does that *only* if the FIFO enable bit is set, and
> presumes that clearing FIFOs is a no-op when they're disabled. I don't
> believe that's true on my system. I also believe that not touching the
> FIFO enable bit is problematic, since some callers clearly expect that
> to be cleared.
Why would you disable FIFO to clear it ? This doesn't make sense to me,
the FIFO must be operational for it to be cleared. Moreover, it
contradicts your previous statement, "the programming example it gives
says to explicitly clear the FIFOs using FCR[2:1]" .
What exactly does your programming example for the JZ4780 say about the
FIFO clearing ? And does it work in that example ?
>>> @@ -558,25 +558,26 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
>>> if (p->capabilities & UART_CAP_FIFO) {
>>> /*
>>> * Make sure to avoid changing FCR[7:3] and ENABLE_FIFO bits.
>>> - * In case ENABLE_FIFO is not set, there is nothing to flush
>>> - * so just return. Furthermore, on certain implementations of
>>> - * the 8250 core, the FCR[7:3] bits may only be changed under
>>> - * specific conditions and changing them if those conditions
>>> - * are not met can have nasty side effects. One such core is
>>> - * the 8250-omap present in TI AM335x.
>>> + * On certain implementations of the 8250 core, the FCR[7:3]
>>> + * bits may only be changed under specific conditions and
>>> + * changing them if those conditions are not met can have nasty
>>> + * side effects. One such core is the 8250-omap present in TI
>>> + * AM335x.
>>> */
>>> fcr = serial_in(p, UART_FCR);
>>> + serial_out(p, UART_FCR, fcr | clr_mask);
>>> + serial_out(p, UART_FCR, fcr & ~clr);
>>
>> Note that, if I understand the patch correctly, this will _not_ restore
>> the original (broken) behavior. The original behavior cleared the entire
>> FCR at the end, which cleared even bits that were not supposed to be
>> cleared .
>
> It will restore the original behaviour with regards to disabling the
> FIFOs, so long as callers that expect that use
> serial8250_clear_and_disable_fifos().
Does your platform need the FIFO to be explicitly disabled for it to be
cleared, can you confirm that ? And if so, does this apply to other
platforms with 8250 UART or is this specific to JZ4780 implementation of
the UART block ?
I just remembered U-Boot has this patch for JZ4780 UART [1], which
touches the FCR UME bit, so the FCR behavior on JZ4780 does differ from
the generic 8250 core. Could it be that this is what you're hitting ?
[1]
http://git.denx.de/?p=u-boot.git;a=commitdiff;h=0b060eefd951fc111ecb77c7b1932b158e6a4f2c;hp=79fd9281880974f076c5b4b354b57faa6e0cc146
>> This patch will make things worse by retaining the clr_mask set in the
>> FCR, thus the FCR[2:1] will be set when you return from this function.
>> That cannot be right ?
>
> You're mistaken - clr_mask (ie. the FIFO reset bits) get cleared again
> by the second call to serial_out(), I just did it without modifying the
> fcr variable - ie. we write the original fcr value again at the end, but
> with UART_FCR_ENABLE_FIFO cleared in the
> serial8250_clear_and_disable_fifos() case. It would probably be clearer
> with the clr argument renamed disable or something like that.
Uhh, OK, thanks for clarifying.
--
Best regards,
Marek Vasut
^ permalink raw reply
* Re: [PATCH] serial: 8250: Fix clearing FIFOs in RS485 mode again
From: Paul Burton @ 2018-12-13 3:33 UTC (permalink / raw)
To: Marek Vasut
Cc: Greg Kroah-Hartman, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org
In-Reply-To: <117fda17-40e6-664b-2b8a-f1032610bf0b@denx.de>
Hi Marek,
On Thu, Dec 13, 2018 at 03:27:51AM +0100, Marek Vasut wrote:
> > I wonder whether the issue may be the JZ4780 UART not automatically
> > resetting the FIFOs when FCR[0] changes. This is a guess, but the manual
> > doesn't explicitly say it'll happen & the programming example it gives
> > says to explicitly clear the FIFOs using FCR[2:1]. Since this is what
> > the kernel has been doing for at least the whole git era I wouldn't be
> > surprised if other devices are bitten by the change as people start
> > trying 4.20 on them.
>
> The patch you're complaining about is doing exactly that -- it sets
> UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT in FCR , and then clears it.
> Those two bits are exactly FCR[2:1]. It also explicitly does not touch
> any other bits in FCR.
No - your patch does that *only* if the FIFO enable bit is set, and
presumes that clearing FIFOs is a no-op when they're disabled. I don't
believe that's true on my system. I also believe that not touching the
FIFO enable bit is problematic, since some callers clearly expect that
to be cleared.
> > @@ -558,25 +558,26 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
> > if (p->capabilities & UART_CAP_FIFO) {
> > /*
> > * Make sure to avoid changing FCR[7:3] and ENABLE_FIFO bits.
> > - * In case ENABLE_FIFO is not set, there is nothing to flush
> > - * so just return. Furthermore, on certain implementations of
> > - * the 8250 core, the FCR[7:3] bits may only be changed under
> > - * specific conditions and changing them if those conditions
> > - * are not met can have nasty side effects. One such core is
> > - * the 8250-omap present in TI AM335x.
> > + * On certain implementations of the 8250 core, the FCR[7:3]
> > + * bits may only be changed under specific conditions and
> > + * changing them if those conditions are not met can have nasty
> > + * side effects. One such core is the 8250-omap present in TI
> > + * AM335x.
> > */
> > fcr = serial_in(p, UART_FCR);
> > + serial_out(p, UART_FCR, fcr | clr_mask);
> > + serial_out(p, UART_FCR, fcr & ~clr);
>
> Note that, if I understand the patch correctly, this will _not_ restore
> the original (broken) behavior. The original behavior cleared the entire
> FCR at the end, which cleared even bits that were not supposed to be
> cleared .
It will restore the original behaviour with regards to disabling the
FIFOs, so long as callers that expect that use
serial8250_clear_and_disable_fifos().
> This patch will make things worse by retaining the clr_mask set in the
> FCR, thus the FCR[2:1] will be set when you return from this function.
> That cannot be right ?
You're mistaken - clr_mask (ie. the FIFO reset bits) get cleared again
by the second call to serial_out(), I just did it without modifying the
fcr variable - ie. we write the original fcr value again at the end, but
with UART_FCR_ENABLE_FIFO cleared in the
serial8250_clear_and_disable_fifos() case. It would probably be clearer
with the clr argument renamed disable or something like that.
Thanks,
Paul
^ permalink raw reply
* Re: [PATCH] serial: 8250: Fix clearing FIFOs in RS485 mode again
From: Marek Vasut @ 2018-12-13 2:27 UTC (permalink / raw)
To: Paul Burton
Cc: Greg Kroah-Hartman, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org
In-Reply-To: <20181213014805.77u5dzydo23cm6fq@pburton-laptop>
On 12/13/2018 02:48 AM, Paul Burton wrote:
> Hi Marek,
Hi,
> On Thu, Dec 13, 2018 at 01:55:29AM +0100, Marek Vasut wrote:
>> On 12/13/2018 01:41 AM, Paul Burton wrote:
>>> This patch has broken the UART on my Ingenic JZ4780 based MIPS Creator
>>> Ci20 board. After this patch the system seems to detect garbage input
>>> that is recognized as SysRq triggers which spam the console & eventually
>>> reset the system.
>>>
>>> One thing of note is that both serial8250_do_startup() &
>>> serial8250_do_shutdown() contain comments that explicitly state their
>>> expectation that the FIFOs will be disabled by serial8250_clear_fifos(),
>>> which is no longer true after this patch.
>>>
>>> I found that restoring the old behaviour for serial8250_do_startup() is
>>> enough to make my system work again, but this is obviously a hack:
>> %
>>> Any ideas? Given the mismatch between this patch & comments that clearly
>>> expect the old behaviour I think the __do_stop_tx_rs485() case probably
>>> needs something different to other callers.
>>
>> The problem the original patch fixed was that too many bits were cleared
>> in the FCR on OMAP3/AM335x . If you have such a system (a beaglebone or
>> similar), you can come up with a solution which can accommodate both the
>> JZ4780 and AM335x. Can you take a look ? The datasheet for both should
>> be public too.
>
> I don't have such a system, but hey - the Ingenic JZ4780 manual is
> public too [1] so you have just as much opportunity to fix it :)
>
> I wonder whether the issue may be the JZ4780 UART not automatically
> resetting the FIFOs when FCR[0] changes. This is a guess, but the manual
> doesn't explicitly say it'll happen & the programming example it gives
> says to explicitly clear the FIFOs using FCR[2:1]. Since this is what
> the kernel has been doing for at least the whole git era I wouldn't be
> surprised if other devices are bitten by the change as people start
> trying 4.20 on them.
The patch you're complaining about is doing exactly that -- it sets
UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT in FCR , and then clears it.
Those two bits are exactly FCR[2:1]. It also explicitly does not touch
any other bits in FCR.
> So I think the safest option might be to restore the original behaviour
> & just keep your change for the __do_stop_tx_rs485() path specifically,
> something like the below. Could you test it on your system?
>
> Assuming it works I'll clean it up & submit. Otherwise your patch is a
> regression so I think a revert would make sense until the problem is
> found.
>
> Thanks,
> Paul
>
> [1] ftp://ftp.ingenic.cn/SOC/JZ4780/JZ4780_pm.pdf
>
> ---
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index f776b3eafb96..0df0ed437a87 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -550,7 +550,7 @@ static unsigned int serial_icr_read(struct uart_8250_port *up, int offset)
> /*
> * FIFO support.
> */
> -static void serial8250_clear_fifos(struct uart_8250_port *p)
> +static void __serial8250_clear_fifos(struct uart_8250_port *p, int clr)
> {
> unsigned char fcr;
> unsigned char clr_mask = UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT;
> @@ -558,25 +558,26 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
> if (p->capabilities & UART_CAP_FIFO) {
> /*
> * Make sure to avoid changing FCR[7:3] and ENABLE_FIFO bits.
> - * In case ENABLE_FIFO is not set, there is nothing to flush
> - * so just return. Furthermore, on certain implementations of
> - * the 8250 core, the FCR[7:3] bits may only be changed under
> - * specific conditions and changing them if those conditions
> - * are not met can have nasty side effects. One such core is
> - * the 8250-omap present in TI AM335x.
> + * On certain implementations of the 8250 core, the FCR[7:3]
> + * bits may only be changed under specific conditions and
> + * changing them if those conditions are not met can have nasty
> + * side effects. One such core is the 8250-omap present in TI
> + * AM335x.
> */
> fcr = serial_in(p, UART_FCR);
> + serial_out(p, UART_FCR, fcr | clr_mask);
> + serial_out(p, UART_FCR, fcr & ~clr);
Note that, if I understand the patch correctly, this will _not_ restore
the original (broken) behavior. The original behavior cleared the entire
FCR at the end, which cleared even bits that were not supposed to be
cleared .
This patch will make things worse by retaining the clr_mask set in the
FCR, thus the FCR[2:1] will be set when you return from this function.
That cannot be right ?
> + }
> +}
>
> - /* FIFO is not enabled, there's nothing to clear. */
> - if (!(fcr & UART_FCR_ENABLE_FIFO))
> - return;
> -
> - fcr |= clr_mask;
> - serial_out(p, UART_FCR, fcr);
> +static void serial8250_clear_fifos(struct uart_8250_port *p)
> +{
> + __serial8250_clear_fifos(p, 0);
> +}
>
> - fcr &= ~clr_mask;
> - serial_out(p, UART_FCR, fcr);
> - }
> +static void serial8250_clear_and_disable_fifos(struct uart_8250_port *p)
> +{
> + __serial8250_clear_fifos(p, UART_FCR_ENABLE_FIFO);
> }
>
> static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
> @@ -595,7 +596,7 @@ static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t);
>
> void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p)
> {
> - serial8250_clear_fifos(p);
> + serial8250_clear_and_disable_fifos(p);
> serial_out(p, UART_FCR, p->fcr);
> }
> EXPORT_SYMBOL_GPL(serial8250_clear_and_reinit_fifos);
> @@ -1364,7 +1365,7 @@ static void autoconfig(struct uart_8250_port *up)
> serial_out(up, UART_RSA_FRR, 0);
> #endif
> serial8250_out_MCR(up, save_mcr);
> - serial8250_clear_fifos(up);
> + serial8250_clear_and_disable_fifos(up);
> serial_in(up, UART_RX);
> if (up->capabilities & UART_CAP_UUE)
> serial_out(up, UART_IER, UART_IER_UUE);
> @@ -2210,7 +2211,7 @@ int serial8250_do_startup(struct uart_port *port)
> * Clear the FIFO buffers and disable them.
> * (they will be reenabled in set_termios())
> */
> - serial8250_clear_fifos(up);
> + serial8250_clear_and_disable_fifos(up);
>
> /*
> * Clear the interrupt registers.
> @@ -2460,7 +2461,7 @@ void serial8250_do_shutdown(struct uart_port *port)
> */
> serial_port_out(port, UART_LCR,
> serial_port_in(port, UART_LCR) & ~UART_LCR_SBC);
> - serial8250_clear_fifos(up);
> + serial8250_clear_and_disable_fifos(up);
>
> #ifdef CONFIG_SERIAL_8250_RSA
> /*
>
--
Best regards,
Marek Vasut
^ permalink raw reply
* Re: [LKP] [tty] c96cf923a9: WARNING:possible_circular_locking_dependency_detected
From: Sergey Senozhatsky @ 2018-12-13 2:19 UTC (permalink / raw)
To: Dmitry Safonov
Cc: Sergey Senozhatsky, Jiri Slaby, kernel test robot, lkp,
Greg Kroah-Hartman, Mikulas Patocka, LKML, linux-serial,
Sergey Senozhatsky, Petr Mladek, Steven Rostedt, Peter Zijlstra,
Waiman Long
In-Reply-To: <137479ad-d38e-7d26-dae4-a4e721d69928@arista.com>
On (12/12/18 14:54), Dmitry Safonov wrote:
> > In this particular case we probably can just move free_page()
> > out of uart_port lock scope. Note that free_page()->MM can printk()
> > on its own.
> >
> >
> > Something like this (not tested):
>
> Looks good to me.
> Probably, it's worth to update comment about freeing just to make sure
> no one will "refactor"/"simplify" it some day.
>
> Does it make sense to add this to your patch?
Makes perfect sense, thanks! I'll send a patch a bit later.
-ss
^ permalink raw reply
* Re: [PATCH] serial: 8250: Fix clearing FIFOs in RS485 mode again
From: Paul Burton @ 2018-12-13 1:48 UTC (permalink / raw)
To: Marek Vasut
Cc: Greg Kroah-Hartman, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org
In-Reply-To: <cd3e2787-48e1-ce70-0fa7-94df6dc81794@denx.de>
Hi Marek,
On Thu, Dec 13, 2018 at 01:55:29AM +0100, Marek Vasut wrote:
> On 12/13/2018 01:41 AM, Paul Burton wrote:
> > This patch has broken the UART on my Ingenic JZ4780 based MIPS Creator
> > Ci20 board. After this patch the system seems to detect garbage input
> > that is recognized as SysRq triggers which spam the console & eventually
> > reset the system.
> >
> > One thing of note is that both serial8250_do_startup() &
> > serial8250_do_shutdown() contain comments that explicitly state their
> > expectation that the FIFOs will be disabled by serial8250_clear_fifos(),
> > which is no longer true after this patch.
> >
> > I found that restoring the old behaviour for serial8250_do_startup() is
> > enough to make my system work again, but this is obviously a hack:
>%
> > Any ideas? Given the mismatch between this patch & comments that clearly
> > expect the old behaviour I think the __do_stop_tx_rs485() case probably
> > needs something different to other callers.
>
> The problem the original patch fixed was that too many bits were cleared
> in the FCR on OMAP3/AM335x . If you have such a system (a beaglebone or
> similar), you can come up with a solution which can accommodate both the
> JZ4780 and AM335x. Can you take a look ? The datasheet for both should
> be public too.
I don't have such a system, but hey - the Ingenic JZ4780 manual is
public too [1] so you have just as much opportunity to fix it :)
I wonder whether the issue may be the JZ4780 UART not automatically
resetting the FIFOs when FCR[0] changes. This is a guess, but the manual
doesn't explicitly say it'll happen & the programming example it gives
says to explicitly clear the FIFOs using FCR[2:1]. Since this is what
the kernel has been doing for at least the whole git era I wouldn't be
surprised if other devices are bitten by the change as people start
trying 4.20 on them.
So I think the safest option might be to restore the original behaviour
& just keep your change for the __do_stop_tx_rs485() path specifically,
something like the below. Could you test it on your system?
Assuming it works I'll clean it up & submit. Otherwise your patch is a
regression so I think a revert would make sense until the problem is
found.
Thanks,
Paul
[1] ftp://ftp.ingenic.cn/SOC/JZ4780/JZ4780_pm.pdf
---
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index f776b3eafb96..0df0ed437a87 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -550,7 +550,7 @@ static unsigned int serial_icr_read(struct uart_8250_port *up, int offset)
/*
* FIFO support.
*/
-static void serial8250_clear_fifos(struct uart_8250_port *p)
+static void __serial8250_clear_fifos(struct uart_8250_port *p, int clr)
{
unsigned char fcr;
unsigned char clr_mask = UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT;
@@ -558,25 +558,26 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
if (p->capabilities & UART_CAP_FIFO) {
/*
* Make sure to avoid changing FCR[7:3] and ENABLE_FIFO bits.
- * In case ENABLE_FIFO is not set, there is nothing to flush
- * so just return. Furthermore, on certain implementations of
- * the 8250 core, the FCR[7:3] bits may only be changed under
- * specific conditions and changing them if those conditions
- * are not met can have nasty side effects. One such core is
- * the 8250-omap present in TI AM335x.
+ * On certain implementations of the 8250 core, the FCR[7:3]
+ * bits may only be changed under specific conditions and
+ * changing them if those conditions are not met can have nasty
+ * side effects. One such core is the 8250-omap present in TI
+ * AM335x.
*/
fcr = serial_in(p, UART_FCR);
+ serial_out(p, UART_FCR, fcr | clr_mask);
+ serial_out(p, UART_FCR, fcr & ~clr);
+ }
+}
- /* FIFO is not enabled, there's nothing to clear. */
- if (!(fcr & UART_FCR_ENABLE_FIFO))
- return;
-
- fcr |= clr_mask;
- serial_out(p, UART_FCR, fcr);
+static void serial8250_clear_fifos(struct uart_8250_port *p)
+{
+ __serial8250_clear_fifos(p, 0);
+}
- fcr &= ~clr_mask;
- serial_out(p, UART_FCR, fcr);
- }
+static void serial8250_clear_and_disable_fifos(struct uart_8250_port *p)
+{
+ __serial8250_clear_fifos(p, UART_FCR_ENABLE_FIFO);
}
static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
@@ -595,7 +596,7 @@ static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t);
void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p)
{
- serial8250_clear_fifos(p);
+ serial8250_clear_and_disable_fifos(p);
serial_out(p, UART_FCR, p->fcr);
}
EXPORT_SYMBOL_GPL(serial8250_clear_and_reinit_fifos);
@@ -1364,7 +1365,7 @@ static void autoconfig(struct uart_8250_port *up)
serial_out(up, UART_RSA_FRR, 0);
#endif
serial8250_out_MCR(up, save_mcr);
- serial8250_clear_fifos(up);
+ serial8250_clear_and_disable_fifos(up);
serial_in(up, UART_RX);
if (up->capabilities & UART_CAP_UUE)
serial_out(up, UART_IER, UART_IER_UUE);
@@ -2210,7 +2211,7 @@ int serial8250_do_startup(struct uart_port *port)
* Clear the FIFO buffers and disable them.
* (they will be reenabled in set_termios())
*/
- serial8250_clear_fifos(up);
+ serial8250_clear_and_disable_fifos(up);
/*
* Clear the interrupt registers.
@@ -2460,7 +2461,7 @@ void serial8250_do_shutdown(struct uart_port *port)
*/
serial_port_out(port, UART_LCR,
serial_port_in(port, UART_LCR) & ~UART_LCR_SBC);
- serial8250_clear_fifos(up);
+ serial8250_clear_and_disable_fifos(up);
#ifdef CONFIG_SERIAL_8250_RSA
/*
^ permalink raw reply related
* Re: [PATCH] serial: 8250: Fix clearing FIFOs in RS485 mode again
From: Marek Vasut @ 2018-12-13 0:55 UTC (permalink / raw)
To: Paul Burton, Greg Kroah-Hartman
Cc: linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-mips@vger.kernel.org
In-Reply-To: <20181213004120.yn35mzfo4skffabv@pburton-laptop>
On 12/13/2018 01:41 AM, Paul Burton wrote:
> Hi Marek / Greg / all,
>
> On Mon, Sep 03, 2018 at 12:44:52AM +0000, Marek Vasut wrote:
>> The 8250 FIFOs indeed need to be cleared after stopping transmission in
>> RS485 mode without SER_RS485_RX_DURING_TX flag set. But there are two
>> problems with the approach taken by the previous patch from Fixes tag.
>>
>> First, serial8250_clear_fifos() should clear fifos, but what it really
>> does is it enables the FIFOs unconditionally if present, clears them
>> and then sets the FCR register to zero, which effectively disables the
>> FIFOs. In case the FIFO is disabled, enabling it and clearing it makes
>> no sense and in fact can trigger misbehavior of the 8250 core. Moreover,
>> the FCR register may contain other FIFO configuration bits which may not
>> be writable unconditionally and writing them incorrectly can trigger
>> misbehavior of the 8250 core too. (ie. AM335x UART swallows the first
>> byte and retransmits the last byte twice because of this FCR write).
>>
>> Second, serial8250_clear_and_reinit_fifos() completely reloads the FCR,
>> but what really has to happen at the end of the RS485 transmission is
>> clearing of the FIFOs and nothing else.
>>
>> This patch repairs serial8250_clear_fifos() so that it really only
>> clears the FIFOs by operating on FCR[2:1] bits and leaves all the
>> other bits alone. It also undoes serial8250_clear_and_reinit_fifos()
>> from __do_stop_tx_rs485() as serial8250_clear_fifos() is sufficient.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Fixes: 2bed8a8e7072 ("Clearing FIFOs in RS485 emulation mode causes subsequent transmits to break")
>> Cc: Daniel Jedrychowski <avistel@gmail.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> ---
>> NOTE: I am not entirely certain this won't break some other hardware,
>> esp. since there might be dependencies on the weird previous
>> behavior of the serial8250_clear_fifos() somewhere.
>
> This patch has broken the UART on my Ingenic JZ4780 based MIPS Creator
> Ci20 board. After this patch the system seems to detect garbage input
> that is recognized as SysRq triggers which spam the console & eventually
> reset the system.
>
> One thing of note is that both serial8250_do_startup() &
> serial8250_do_shutdown() contain comments that explicitly state their
> expectation that the FIFOs will be disabled by serial8250_clear_fifos(),
> which is no longer true after this patch.
>
> I found that restoring the old behaviour for serial8250_do_startup() is
> enough to make my system work again, but this is obviously a hack:
>
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index f776b3eafb96..8def02933d19 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2210,7 +2210,11 @@ int serial8250_do_startup(struct uart_port *port)
> * Clear the FIFO buffers and disable them.
> * (they will be reenabled in set_termios())
> */
> - serial8250_clear_fifos(up);
> + if (up->capabilities & UART_CAP_FIFO) {
> + serial_port_out(port, UART_FCR, UART_FCR_ENABLE_FIFO);
> + serial8250_clear_fifos(up);
> + serial_port_out(port, UART_FCR, 0);
> + }
>
> /*
> * Clear the interrupt registers.
>
> Any ideas? Given the mismatch between this patch & comments that clearly
> expect the old behaviour I think the __do_stop_tx_rs485() case probably
> needs something different to other callers.
The problem the original patch fixed was that too many bits were cleared
in the FCR on OMAP3/AM335x . If you have such a system (a beaglebone or
similar), you can come up with a solution which can accommodate both the
JZ4780 and AM335x. Can you take a look ? The datasheet for both should
be public too.
--
Best regards,
Marek Vasut
^ permalink raw reply
* Re: [PATCH] serial: 8250: Fix clearing FIFOs in RS485 mode again
From: Paul Burton @ 2018-12-13 0:41 UTC (permalink / raw)
To: Marek Vasut, Greg Kroah-Hartman
Cc: linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-mips@vger.kernel.org
Hi Marek / Greg / all,
On Mon, Sep 03, 2018 at 12:44:52AM +0000, Marek Vasut wrote:
> The 8250 FIFOs indeed need to be cleared after stopping transmission in
> RS485 mode without SER_RS485_RX_DURING_TX flag set. But there are two
> problems with the approach taken by the previous patch from Fixes tag.
>
> First, serial8250_clear_fifos() should clear fifos, but what it really
> does is it enables the FIFOs unconditionally if present, clears them
> and then sets the FCR register to zero, which effectively disables the
> FIFOs. In case the FIFO is disabled, enabling it and clearing it makes
> no sense and in fact can trigger misbehavior of the 8250 core. Moreover,
> the FCR register may contain other FIFO configuration bits which may not
> be writable unconditionally and writing them incorrectly can trigger
> misbehavior of the 8250 core too. (ie. AM335x UART swallows the first
> byte and retransmits the last byte twice because of this FCR write).
>
> Second, serial8250_clear_and_reinit_fifos() completely reloads the FCR,
> but what really has to happen at the end of the RS485 transmission is
> clearing of the FIFOs and nothing else.
>
> This patch repairs serial8250_clear_fifos() so that it really only
> clears the FIFOs by operating on FCR[2:1] bits and leaves all the
> other bits alone. It also undoes serial8250_clear_and_reinit_fifos()
> from __do_stop_tx_rs485() as serial8250_clear_fifos() is sufficient.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Fixes: 2bed8a8e7072 ("Clearing FIFOs in RS485 emulation mode causes subsequent transmits to break")
> Cc: Daniel Jedrychowski <avistel@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> NOTE: I am not entirely certain this won't break some other hardware,
> esp. since there might be dependencies on the weird previous
> behavior of the serial8250_clear_fifos() somewhere.
This patch has broken the UART on my Ingenic JZ4780 based MIPS Creator
Ci20 board. After this patch the system seems to detect garbage input
that is recognized as SysRq triggers which spam the console & eventually
reset the system.
One thing of note is that both serial8250_do_startup() &
serial8250_do_shutdown() contain comments that explicitly state their
expectation that the FIFOs will be disabled by serial8250_clear_fifos(),
which is no longer true after this patch.
I found that restoring the old behaviour for serial8250_do_startup() is
enough to make my system work again, but this is obviously a hack:
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index f776b3eafb96..8def02933d19 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2210,7 +2210,11 @@ int serial8250_do_startup(struct uart_port *port)
* Clear the FIFO buffers and disable them.
* (they will be reenabled in set_termios())
*/
- serial8250_clear_fifos(up);
+ if (up->capabilities & UART_CAP_FIFO) {
+ serial_port_out(port, UART_FCR, UART_FCR_ENABLE_FIFO);
+ serial8250_clear_fifos(up);
+ serial_port_out(port, UART_FCR, 0);
+ }
/*
* Clear the interrupt registers.
Any ideas? Given the mismatch between this patch & comments that clearly
expect the old behaviour I think the __do_stop_tx_rs485() case probably
needs something different to other callers.
Thanks,
Paul
^ permalink raw reply related
* Re: [PATCH] serial/sunsu: fix refcount leak
From: Frank Lee @ 2018-12-12 16:05 UTC (permalink / raw)
To: davem, gregkh, jslaby, sparclinux
Cc: Daniel Lezcano, linux-serial, linux-kernel
In-Reply-To: <20181212160145.23117-1-tiny.windzz@gmail.com>
Sorry for got to add a prefix(PATCH v2).
I hope this has no effect.
MBR,
Yangtao
^ permalink raw reply
* [PATCH] serial/sunsu: fix refcount leak
From: Yangtao Li @ 2018-12-12 16:01 UTC (permalink / raw)
To: davem, gregkh, jslaby, sparclinux
Cc: daniel.lezcano, linux-serial, linux-kernel, Yangtao Li
The function of_find_node_by_path() acquires a reference to the node
returned by it and that reference needs to be dropped by its caller.
su_get_type() doesn't do that. The match node are used as an identifier
to compare against the current node, so we can directly drop the refcount
after getting the node from the path as it is not used as pointer.
Fix this by use a single variable and drop the refcount right after
of_find_node_by_path().
Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
---
drivers/tty/serial/sunsu.c | 31 ++++++++++++++++++++++++++-----
1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/serial/sunsu.c b/drivers/tty/serial/sunsu.c
index 6cf3e9b0728f..c5428b0426a9 100644
--- a/drivers/tty/serial/sunsu.c
+++ b/drivers/tty/serial/sunsu.c
@@ -1394,22 +1394,43 @@ static inline struct console *SUNSU_CONSOLE(void)
static enum su_type su_get_type(struct device_node *dp)
{
struct device_node *ap = of_find_node_by_path("/aliases");
+ enum su_type rc = SU_PORT_PORT;
if (ap) {
const char *keyb = of_get_property(ap, "keyboard", NULL);
const char *ms = of_get_property(ap, "mouse", NULL);
+ struct device_node *match;
if (keyb) {
- if (dp == of_find_node_by_path(keyb))
- return SU_PORT_KBD;
+ match = of_find_node_by_path(keyb);
+
+ /*
+ * The pointer is used as an identifier not
+ * as a pointer, we can drop the refcount on
+ * the of__node immediately after getting it.
+ */
+ of_node_put(match);
+
+ if (dp == match) {
+ rc = SU_PORT_KBD;
+ goto out;
+ }
}
if (ms) {
- if (dp == of_find_node_by_path(ms))
- return SU_PORT_MS;
+ match = of_find_node_by_path(ms);
+
+ of_node_put(match);
+
+ if (dp == match) {
+ rc = SU_PORT_MS;
+ goto out;
+ }
}
}
- return SU_PORT_PORT;
+out:
+ of_node_put(ap);
+ return rc;
}
static int su_probe(struct platform_device *op)
--
2.17.0
^ permalink raw reply related
* Re: [PATCH v4 11/15] clocksource: Add clock driver for RDA8810PL SoC
From: Daniel Lezcano @ 2018-12-12 15:52 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: olof, arnd, robh+dt, tglx, jason, marc.zyngier, gregkh, jslaby,
linux-unisoc, afaerber, linux-arm-kernel, linux-kernel,
devicetree, linux-serial, amit.kucheria, linus.walleij,
zhao_steven
In-Reply-To: <20181212154748.GA28130@Mani-XPS-13-9360>
On 12/12/2018 16:47, Manivannan Sadhasivam wrote:
> Hi Daniel,
>
> On Wed, Dec 12, 2018 at 04:07:53PM +0100, Daniel Lezcano wrote:
>> On 10/12/2018 18:35, Manivannan Sadhasivam wrote:
>>> Add clock driver for RDA Micro RDA8810PL SoC supporting OSTIMER
>>> and HWTIMER.
>>>
>>> RDA8810PL has two independent timers: OSTIMER (56 bit) and HWTIMER
>>> (64 bit). Each timer provides optional interrupt support. In this
>>> driver, OSTIMER is used for clockevents and HWTIMER is used for
>>> clocksource.
>>>
>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>
>> The driver looks good to me. Do you want me to pick it up via my tree?
>>
>
> Yes, please do. Marc is going to pick up the irqchip driver but I'm not
> sure about the serial driver. The rest of the patches can be picked up
> by the ARM maintainers (I need to send another version for dropping
> Andreas from MAINTAINERS).
Ok, applied.
Thanks
-- Daniel
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply
* Re: [PATCH v4 11/15] clocksource: Add clock driver for RDA8810PL SoC
From: Manivannan Sadhasivam @ 2018-12-12 15:47 UTC (permalink / raw)
To: Daniel Lezcano
Cc: olof, arnd, robh+dt, tglx, jason, marc.zyngier, gregkh, jslaby,
linux-unisoc, afaerber, linux-arm-kernel, linux-kernel,
devicetree, linux-serial, amit.kucheria, linus.walleij,
zhao_steven
In-Reply-To: <0b90a179-aa4b-57f7-0874-996d5c535ea9@linaro.org>
Hi Daniel,
On Wed, Dec 12, 2018 at 04:07:53PM +0100, Daniel Lezcano wrote:
> On 10/12/2018 18:35, Manivannan Sadhasivam wrote:
> > Add clock driver for RDA Micro RDA8810PL SoC supporting OSTIMER
> > and HWTIMER.
> >
> > RDA8810PL has two independent timers: OSTIMER (56 bit) and HWTIMER
> > (64 bit). Each timer provides optional interrupt support. In this
> > driver, OSTIMER is used for clockevents and HWTIMER is used for
> > clocksource.
> >
> > Signed-off-by: Andreas Färber <afaerber@suse.de>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>
> The driver looks good to me. Do you want me to pick it up via my tree?
>
Yes, please do. Marc is going to pick up the irqchip driver but I'm not
sure about the serial driver. The rest of the patches can be picked up
by the ARM maintainers (I need to send another version for dropping
Andreas from MAINTAINERS).
Thanks,
Mani
> > ---
> > drivers/clocksource/Kconfig | 8 ++
> > drivers/clocksource/Makefile | 1 +
> > drivers/clocksource/timer-rda.c | 195 ++++++++++++++++++++++++++++++++
> > 3 files changed, 204 insertions(+)
> > create mode 100644 drivers/clocksource/timer-rda.c
> >
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index 55c77e44bb2d..598b592e03d7 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -105,6 +105,14 @@ config OWL_TIMER
> > help
> > Enables the support for the Actions Semi Owl timer driver.
> >
> > +config RDA_TIMER
> > + bool "RDA timer driver" if COMPILE_TEST
> > + depends on GENERIC_CLOCKEVENTS
> > + select CLKSRC_MMIO
> > + select TIMER_OF
> > + help
> > + Enables the support for the RDA Micro timer driver.
> > +
> > config SUN4I_TIMER
> > bool "Sun4i timer driver" if COMPILE_TEST
> > depends on HAS_IOMEM
> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> > index dd9138104568..150020a90707 100644
> > --- a/drivers/clocksource/Makefile
> > +++ b/drivers/clocksource/Makefile
> > @@ -57,6 +57,7 @@ obj-$(CONFIG_OXNAS_RPS_TIMER) += timer-oxnas-rps.o
> > obj-$(CONFIG_OWL_TIMER) += timer-owl.o
> > obj-$(CONFIG_SPRD_TIMER) += timer-sprd.o
> > obj-$(CONFIG_NPCM7XX_TIMER) += timer-npcm7xx.o
> > +obj-$(CONFIG_RDA_TIMER) += timer-rda.o
> >
> > obj-$(CONFIG_ARC_TIMERS) += arc_timer.o
> > obj-$(CONFIG_ARM_ARCH_TIMER) += arm_arch_timer.o
> > diff --git a/drivers/clocksource/timer-rda.c b/drivers/clocksource/timer-rda.c
> > new file mode 100644
> > index 000000000000..fd1199c189bf
> > --- /dev/null
> > +++ b/drivers/clocksource/timer-rda.c
> > @@ -0,0 +1,195 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * RDA8810PL SoC timer driver
> > + *
> > + * Copyright RDA Microelectronics Company Limited
> > + * Copyright (c) 2017 Andreas Färber
> > + * Copyright (c) 2018 Manivannan Sadhasivam
> > + *
> > + * RDA8810PL has two independent timers: OSTIMER (56 bit) and HWTIMER (64 bit).
> > + * Each timer provides optional interrupt support. In this driver, OSTIMER is
> > + * used for clockevents and HWTIMER is used for clocksource.
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +
> > +#include "timer-of.h"
> > +
> > +#define RDA_OSTIMER_LOADVAL_L 0x000
> > +#define RDA_OSTIMER_CTRL 0x004
> > +#define RDA_HWTIMER_LOCKVAL_L 0x024
> > +#define RDA_HWTIMER_LOCKVAL_H 0x028
> > +#define RDA_TIMER_IRQ_MASK_SET 0x02c
> > +#define RDA_TIMER_IRQ_MASK_CLR 0x030
> > +#define RDA_TIMER_IRQ_CLR 0x034
> > +
> > +#define RDA_OSTIMER_CTRL_ENABLE BIT(24)
> > +#define RDA_OSTIMER_CTRL_REPEAT BIT(28)
> > +#define RDA_OSTIMER_CTRL_LOAD BIT(30)
> > +
> > +#define RDA_TIMER_IRQ_MASK_OSTIMER BIT(0)
> > +
> > +#define RDA_TIMER_IRQ_CLR_OSTIMER BIT(0)
> > +
> > +static int rda_ostimer_start(void __iomem *base, bool periodic, u64 cycles)
> > +{
> > + u32 ctrl, load_l;
> > +
> > + load_l = (u32)cycles;
> > + ctrl = ((cycles >> 32) & 0xffffff);
> > + ctrl |= RDA_OSTIMER_CTRL_LOAD | RDA_OSTIMER_CTRL_ENABLE;
> > + if (periodic)
> > + ctrl |= RDA_OSTIMER_CTRL_REPEAT;
> > + /* Enable ostimer interrupt first */
> > + writel_relaxed(RDA_TIMER_IRQ_MASK_OSTIMER,
> > + base + RDA_TIMER_IRQ_MASK_SET);
> > +
> > + /* Write low 32 bits first, high 24 bits are with ctrl */
> > + writel_relaxed(load_l, base + RDA_OSTIMER_LOADVAL_L);
> > + writel_relaxed(ctrl, base + RDA_OSTIMER_CTRL);
> > +
> > + return 0;
> > +}
> > +
> > +static int rda_ostimer_stop(void __iomem *base)
> > +{
> > + /* Disable ostimer interrupt first */
> > + writel_relaxed(RDA_TIMER_IRQ_MASK_OSTIMER,
> > + base + RDA_TIMER_IRQ_MASK_CLR);
> > +
> > + writel_relaxed(0, base + RDA_OSTIMER_CTRL);
> > +
> > + return 0;
> > +}
> > +
> > +static int rda_ostimer_set_state_shutdown(struct clock_event_device *evt)
> > +{
> > + struct timer_of *to = to_timer_of(evt);
> > +
> > + rda_ostimer_stop(timer_of_base(to));
> > +
> > + return 0;
> > +}
> > +
> > +static int rda_ostimer_set_state_oneshot(struct clock_event_device *evt)
> > +{
> > + struct timer_of *to = to_timer_of(evt);
> > +
> > + rda_ostimer_stop(timer_of_base(to));
> > +
> > + return 0;
> > +}
> > +
> > +static int rda_ostimer_set_state_periodic(struct clock_event_device *evt)
> > +{
> > + struct timer_of *to = to_timer_of(evt);
> > + unsigned long cycles_per_jiffy;
> > +
> > + rda_ostimer_stop(timer_of_base(to));
> > +
> > + cycles_per_jiffy = ((unsigned long long)NSEC_PER_SEC / HZ *
> > + evt->mult) >> evt->shift;
> > + rda_ostimer_start(timer_of_base(to), true, cycles_per_jiffy);
> > +
> > + return 0;
> > +}
> > +
> > +static int rda_ostimer_tick_resume(struct clock_event_device *evt)
> > +{
> > + return 0;
> > +}
> > +
> > +static int rda_ostimer_set_next_event(unsigned long evt,
> > + struct clock_event_device *ev)
> > +{
> > + struct timer_of *to = to_timer_of(ev);
> > +
> > + rda_ostimer_start(timer_of_base(to), false, evt);
> > +
> > + return 0;
> > +}
> > +
> > +static irqreturn_t rda_ostimer_interrupt(int irq, void *dev_id)
> > +{
> > + struct clock_event_device *evt = dev_id;
> > + struct timer_of *to = to_timer_of(evt);
> > +
> > + /* clear timer int */
> > + writel_relaxed(RDA_TIMER_IRQ_CLR_OSTIMER,
> > + timer_of_base(to) + RDA_TIMER_IRQ_CLR);
> > +
> > + if (evt->event_handler)
> > + evt->event_handler(evt);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static struct timer_of rda_ostimer_of = {
> > + .flags = TIMER_OF_IRQ | TIMER_OF_BASE,
> > +
> > + .clkevt = {
> > + .name = "rda-ostimer",
> > + .rating = 250,
> > + .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
> > + CLOCK_EVT_FEAT_DYNIRQ,
> > + .set_state_shutdown = rda_ostimer_set_state_shutdown,
> > + .set_state_oneshot = rda_ostimer_set_state_oneshot,
> > + .set_state_periodic = rda_ostimer_set_state_periodic,
> > + .tick_resume = rda_ostimer_tick_resume,
> > + .set_next_event = rda_ostimer_set_next_event,
> > + },
> > +
> > + .of_base = {
> > + .name = "rda-timer",
> > + .index = 0,
> > + },
> > +
> > + .of_irq = {
> > + .name = "ostimer",
> > + .handler = rda_ostimer_interrupt,
> > + .flags = IRQF_TIMER,
> > + },
> > +};
> > +
> > +static u64 rda_hwtimer_read(struct clocksource *cs)
> > +{
> > + void __iomem *base = timer_of_base(&rda_ostimer_of);
> > + u32 lo, hi;
> > +
> > + /* Always read low 32 bits first */
> > + do {
> > + lo = readl_relaxed(base + RDA_HWTIMER_LOCKVAL_L);
> > + hi = readl_relaxed(base + RDA_HWTIMER_LOCKVAL_H);
> > + } while (hi != readl_relaxed(base + RDA_HWTIMER_LOCKVAL_H));
> > +
> > + return ((u64)hi << 32) | lo;
> > +}
> > +
> > +static struct clocksource rda_hwtimer_clocksource = {
> > + .name = "rda-timer",
> > + .rating = 400,
> > + .read = rda_hwtimer_read,
> > + .mask = CLOCKSOURCE_MASK(64),
> > + .flags = CLOCK_SOURCE_IS_CONTINUOUS,
> > +};
> > +
> > +static int __init rda_timer_init(struct device_node *np)
> > +{
> > + unsigned long rate = 2000000;
> > + int ret;
> > +
> > + ret = timer_of_init(np, &rda_ostimer_of);
> > + if (ret)
> > + return ret;
> > +
> > + clocksource_register_hz(&rda_hwtimer_clocksource, rate);
> > +
> > + clockevents_config_and_register(&rda_ostimer_of.clkevt, rate,
> > + 0x2, UINT_MAX);
> > +
> > + return 0;
> > +}
> > +
> > +TIMER_OF_DECLARE(rda8810pl, "rda,8810pl-timer", rda_timer_init);
> >
>
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>
^ permalink raw reply
* Re: [LKP] [tty] c96cf923a9: WARNING:possible_circular_locking_dependency_detected
From: Waiman Long @ 2018-12-12 15:12 UTC (permalink / raw)
To: Sergey Senozhatsky, Dmitry Safonov
Cc: Jiri Slaby, kernel test robot, lkp, Greg Kroah-Hartman,
Mikulas Patocka, LKML, linux-serial, Sergey Senozhatsky,
Petr Mladek, Steven Rostedt, Peter Zijlstra
In-Reply-To: <20181212050432.GE431@jagdpanzerIV>
On 12/12/2018 12:04 AM, Sergey Senozhatsky wrote:
> On (12/12/18 12:42), Sergey Senozhatsky wrote:
> [..]
>>>>> [ 87.255156] CPU0 CPU1
>>>>> [ 87.255813] ---- ----
>>>>> [ 87.256460] lock(&port_lock_key);
>>>>> [ 87.256973] lock(console_owner);
>>>>> [ 87.257829] lock(&port_lock_key);
>>>>> [ 87.258680] lock(&obj_hash[i].lock);
>> So it's like
>>
>> CPU0 CPU1
>>
>> uart_shutdown() db->lock
>> uart_port->lock debug_print_object()
>> free_page() printk
>> debug_check_no_obj_freed uart_port->lock
>> db->lock
>>
>>
>> In this particular case we probably can just move free_page()
>> out of uart_port lock scope. Note that free_page()->MM can printk()
>> on its own.
>>
> [..]
>> [1] https://lore.kernel.org/lkml/1542653726-5655-8-git-send-email-longman@redhat.com/T/#u
> That said, I'd first try Waiman's patch. The one I suggested is
> more of a defense move - there are too many things happening under
> uart_port->lock. This is not the first time we see lockdep complaining
> about the way uart and the rest of the kernel interact.
>
> -ss
Thanks for the information. I will extract my debugobjects patch out of
my lockdep patchset and send it out as standalone patch.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH v4 11/15] clocksource: Add clock driver for RDA8810PL SoC
From: Daniel Lezcano @ 2018-12-12 15:07 UTC (permalink / raw)
To: Manivannan Sadhasivam, olof, arnd, robh+dt, tglx, jason,
marc.zyngier, gregkh, jslaby
Cc: linux-unisoc, afaerber, linux-arm-kernel, linux-kernel,
devicetree, linux-serial, amit.kucheria, linus.walleij,
zhao_steven
In-Reply-To: <20181210173550.29643-12-manivannan.sadhasivam@linaro.org>
On 10/12/2018 18:35, Manivannan Sadhasivam wrote:
> Add clock driver for RDA Micro RDA8810PL SoC supporting OSTIMER
> and HWTIMER.
>
> RDA8810PL has two independent timers: OSTIMER (56 bit) and HWTIMER
> (64 bit). Each timer provides optional interrupt support. In this
> driver, OSTIMER is used for clockevents and HWTIMER is used for
> clocksource.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
The driver looks good to me. Do you want me to pick it up via my tree?
> ---
> drivers/clocksource/Kconfig | 8 ++
> drivers/clocksource/Makefile | 1 +
> drivers/clocksource/timer-rda.c | 195 ++++++++++++++++++++++++++++++++
> 3 files changed, 204 insertions(+)
> create mode 100644 drivers/clocksource/timer-rda.c
>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 55c77e44bb2d..598b592e03d7 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -105,6 +105,14 @@ config OWL_TIMER
> help
> Enables the support for the Actions Semi Owl timer driver.
>
> +config RDA_TIMER
> + bool "RDA timer driver" if COMPILE_TEST
> + depends on GENERIC_CLOCKEVENTS
> + select CLKSRC_MMIO
> + select TIMER_OF
> + help
> + Enables the support for the RDA Micro timer driver.
> +
> config SUN4I_TIMER
> bool "Sun4i timer driver" if COMPILE_TEST
> depends on HAS_IOMEM
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index dd9138104568..150020a90707 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -57,6 +57,7 @@ obj-$(CONFIG_OXNAS_RPS_TIMER) += timer-oxnas-rps.o
> obj-$(CONFIG_OWL_TIMER) += timer-owl.o
> obj-$(CONFIG_SPRD_TIMER) += timer-sprd.o
> obj-$(CONFIG_NPCM7XX_TIMER) += timer-npcm7xx.o
> +obj-$(CONFIG_RDA_TIMER) += timer-rda.o
>
> obj-$(CONFIG_ARC_TIMERS) += arc_timer.o
> obj-$(CONFIG_ARM_ARCH_TIMER) += arm_arch_timer.o
> diff --git a/drivers/clocksource/timer-rda.c b/drivers/clocksource/timer-rda.c
> new file mode 100644
> index 000000000000..fd1199c189bf
> --- /dev/null
> +++ b/drivers/clocksource/timer-rda.c
> @@ -0,0 +1,195 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * RDA8810PL SoC timer driver
> + *
> + * Copyright RDA Microelectronics Company Limited
> + * Copyright (c) 2017 Andreas Färber
> + * Copyright (c) 2018 Manivannan Sadhasivam
> + *
> + * RDA8810PL has two independent timers: OSTIMER (56 bit) and HWTIMER (64 bit).
> + * Each timer provides optional interrupt support. In this driver, OSTIMER is
> + * used for clockevents and HWTIMER is used for clocksource.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +
> +#include "timer-of.h"
> +
> +#define RDA_OSTIMER_LOADVAL_L 0x000
> +#define RDA_OSTIMER_CTRL 0x004
> +#define RDA_HWTIMER_LOCKVAL_L 0x024
> +#define RDA_HWTIMER_LOCKVAL_H 0x028
> +#define RDA_TIMER_IRQ_MASK_SET 0x02c
> +#define RDA_TIMER_IRQ_MASK_CLR 0x030
> +#define RDA_TIMER_IRQ_CLR 0x034
> +
> +#define RDA_OSTIMER_CTRL_ENABLE BIT(24)
> +#define RDA_OSTIMER_CTRL_REPEAT BIT(28)
> +#define RDA_OSTIMER_CTRL_LOAD BIT(30)
> +
> +#define RDA_TIMER_IRQ_MASK_OSTIMER BIT(0)
> +
> +#define RDA_TIMER_IRQ_CLR_OSTIMER BIT(0)
> +
> +static int rda_ostimer_start(void __iomem *base, bool periodic, u64 cycles)
> +{
> + u32 ctrl, load_l;
> +
> + load_l = (u32)cycles;
> + ctrl = ((cycles >> 32) & 0xffffff);
> + ctrl |= RDA_OSTIMER_CTRL_LOAD | RDA_OSTIMER_CTRL_ENABLE;
> + if (periodic)
> + ctrl |= RDA_OSTIMER_CTRL_REPEAT;
> + /* Enable ostimer interrupt first */
> + writel_relaxed(RDA_TIMER_IRQ_MASK_OSTIMER,
> + base + RDA_TIMER_IRQ_MASK_SET);
> +
> + /* Write low 32 bits first, high 24 bits are with ctrl */
> + writel_relaxed(load_l, base + RDA_OSTIMER_LOADVAL_L);
> + writel_relaxed(ctrl, base + RDA_OSTIMER_CTRL);
> +
> + return 0;
> +}
> +
> +static int rda_ostimer_stop(void __iomem *base)
> +{
> + /* Disable ostimer interrupt first */
> + writel_relaxed(RDA_TIMER_IRQ_MASK_OSTIMER,
> + base + RDA_TIMER_IRQ_MASK_CLR);
> +
> + writel_relaxed(0, base + RDA_OSTIMER_CTRL);
> +
> + return 0;
> +}
> +
> +static int rda_ostimer_set_state_shutdown(struct clock_event_device *evt)
> +{
> + struct timer_of *to = to_timer_of(evt);
> +
> + rda_ostimer_stop(timer_of_base(to));
> +
> + return 0;
> +}
> +
> +static int rda_ostimer_set_state_oneshot(struct clock_event_device *evt)
> +{
> + struct timer_of *to = to_timer_of(evt);
> +
> + rda_ostimer_stop(timer_of_base(to));
> +
> + return 0;
> +}
> +
> +static int rda_ostimer_set_state_periodic(struct clock_event_device *evt)
> +{
> + struct timer_of *to = to_timer_of(evt);
> + unsigned long cycles_per_jiffy;
> +
> + rda_ostimer_stop(timer_of_base(to));
> +
> + cycles_per_jiffy = ((unsigned long long)NSEC_PER_SEC / HZ *
> + evt->mult) >> evt->shift;
> + rda_ostimer_start(timer_of_base(to), true, cycles_per_jiffy);
> +
> + return 0;
> +}
> +
> +static int rda_ostimer_tick_resume(struct clock_event_device *evt)
> +{
> + return 0;
> +}
> +
> +static int rda_ostimer_set_next_event(unsigned long evt,
> + struct clock_event_device *ev)
> +{
> + struct timer_of *to = to_timer_of(ev);
> +
> + rda_ostimer_start(timer_of_base(to), false, evt);
> +
> + return 0;
> +}
> +
> +static irqreturn_t rda_ostimer_interrupt(int irq, void *dev_id)
> +{
> + struct clock_event_device *evt = dev_id;
> + struct timer_of *to = to_timer_of(evt);
> +
> + /* clear timer int */
> + writel_relaxed(RDA_TIMER_IRQ_CLR_OSTIMER,
> + timer_of_base(to) + RDA_TIMER_IRQ_CLR);
> +
> + if (evt->event_handler)
> + evt->event_handler(evt);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static struct timer_of rda_ostimer_of = {
> + .flags = TIMER_OF_IRQ | TIMER_OF_BASE,
> +
> + .clkevt = {
> + .name = "rda-ostimer",
> + .rating = 250,
> + .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
> + CLOCK_EVT_FEAT_DYNIRQ,
> + .set_state_shutdown = rda_ostimer_set_state_shutdown,
> + .set_state_oneshot = rda_ostimer_set_state_oneshot,
> + .set_state_periodic = rda_ostimer_set_state_periodic,
> + .tick_resume = rda_ostimer_tick_resume,
> + .set_next_event = rda_ostimer_set_next_event,
> + },
> +
> + .of_base = {
> + .name = "rda-timer",
> + .index = 0,
> + },
> +
> + .of_irq = {
> + .name = "ostimer",
> + .handler = rda_ostimer_interrupt,
> + .flags = IRQF_TIMER,
> + },
> +};
> +
> +static u64 rda_hwtimer_read(struct clocksource *cs)
> +{
> + void __iomem *base = timer_of_base(&rda_ostimer_of);
> + u32 lo, hi;
> +
> + /* Always read low 32 bits first */
> + do {
> + lo = readl_relaxed(base + RDA_HWTIMER_LOCKVAL_L);
> + hi = readl_relaxed(base + RDA_HWTIMER_LOCKVAL_H);
> + } while (hi != readl_relaxed(base + RDA_HWTIMER_LOCKVAL_H));
> +
> + return ((u64)hi << 32) | lo;
> +}
> +
> +static struct clocksource rda_hwtimer_clocksource = {
> + .name = "rda-timer",
> + .rating = 400,
> + .read = rda_hwtimer_read,
> + .mask = CLOCKSOURCE_MASK(64),
> + .flags = CLOCK_SOURCE_IS_CONTINUOUS,
> +};
> +
> +static int __init rda_timer_init(struct device_node *np)
> +{
> + unsigned long rate = 2000000;
> + int ret;
> +
> + ret = timer_of_init(np, &rda_ostimer_of);
> + if (ret)
> + return ret;
> +
> + clocksource_register_hz(&rda_hwtimer_clocksource, rate);
> +
> + clockevents_config_and_register(&rda_ostimer_of.clkevt, rate,
> + 0x2, UINT_MAX);
> +
> + return 0;
> +}
> +
> +TIMER_OF_DECLARE(rda8810pl, "rda,8810pl-timer", rda_timer_init);
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply
* Re: [LKP] [tty] c96cf923a9: WARNING:possible_circular_locking_dependency_detected
From: Dmitry Safonov @ 2018-12-12 14:54 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Jiri Slaby, kernel test robot, lkp, Greg Kroah-Hartman,
Mikulas Patocka, LKML, linux-serial, Sergey Senozhatsky,
Petr Mladek, Steven Rostedt, Peter Zijlstra, Waiman Long
In-Reply-To: <20181212034252.GD431@jagdpanzerIV>
Hi Sergey,
On 12/12/18 3:42 AM, Sergey Senozhatsky wrote:
[..]
>
> db->lock -> console_sem -> uart_port->lock
>
> obj_hash[i].lock
> /* db->lock */
> __debug_object_init()
> debug_print_object()
> printk()
> spin_lock_irqsave(uart->port_lock)
>
> BTW, there is a patch from Waiman which moves debug_print_object()
> out of db->lock scope [1].
Thanks much for pointing this, didn't know about that and started to
write something like that yesterday :)
>>>> [ 87.239071] -> #0 (&obj_hash[i].lock){-.-.}:
>>>> [ 87.239904] __lock_acquire+0x1f78/0x22d1
>>>> [ 87.240556] lock_acquire+0x28c/0x2e7
>>>> [ 87.241173] _raw_spin_lock_irqsave+0x35/0x49
>>>> [ 87.241882] debug_check_no_obj_freed+0xb4/0x302
>>>> [ 87.242620] free_unref_page_prepare+0x33a/0x483
>>>> [ 87.243368] free_unref_page+0x48/0x80
>>>> [ 87.243991] __free_pages+0x2e/0x40
>>>> [ 87.244611] free_pages+0x54/0x5a
>>>> [ 87.245188] uart_shutdown+0x3df/0x4e2
>>>> [ 87.245817] uart_hangup+0x123/0x280
>>>> [ 87.246406] __tty_hangup+0x4da/0x50f
>>>> [ 87.247025] tty_vhangup_session+0xe/0x10
>>>> [ 87.247680] disassociate_ctty+0xeb/0x5c5
>>>> [ 87.248349] do_exit+0xc97/0x1daf
>>>> [ 87.248920] __x64_sys_exit_group+0x0/0x3e
>>>> [ 87.249587] __wake_up_parent+0x0/0x52
>>>> [ 87.250211] do_syscall_64+0x5e8/0x881
>>>> [ 87.250839] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> But I think what really makes lockdep nervous is this thing:
>
> uart_shutdown()
> uart_port_lock() // spin_lock_irqsave(uart_port->lock)
> free_page()
> debug_check_no_obj_freed()
> db->lock
> debug_print_object()
> printk()
> spin_lock_irqsave(uart_port->lock)
>
>
> Lockdep complains about: uart_port->lock -> db->lock
>
> It knows that we already have the reverse chain: db->lock -> uart_port->lock
> From
> db->lock -> debug_print_object() -> printk() -> console_sem -> uart_port->lock
>
>
>>>> [ 87.255156] CPU0 CPU1
>>>> [ 87.255813] ---- ----
>>>> [ 87.256460] lock(&port_lock_key);
>>>> [ 87.256973] lock(console_owner);
>>>> [ 87.257829] lock(&port_lock_key);
>>>> [ 87.258680] lock(&obj_hash[i].lock);
>
>
> So it's like
>
> CPU0 CPU1
>
> uart_shutdown() db->lock
> uart_port->lock debug_print_object()
> free_page() printk
> debug_check_no_obj_freed uart_port->lock
> db->lock
>
>
> In this particular case we probably can just move free_page()
> out of uart_port lock scope. Note that free_page()->MM can printk()
> on its own.
>
>
> Something like this (not tested):
Looks good to me.
Probably, it's worth to update comment about freeing just to make sure
no one will "refactor"/"simplify" it some day.
Does it make sense to add this to your patch?
--->8---
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -205,10 +205,11 @@ static int uart_port_startup(struct tty_struct
*tty, struct uar>
if (!state->xmit.buf) {
state->xmit.buf = (unsigned char *) page;
uart_circ_clear(&state->xmit);
+ uart_port_unlock(uport, flags);
} else {
+ uart_port_unlock(uport, flags);
free_page(page);
}
- uart_port_unlock(uport, flags);
retval = uport->ops->startup(uport);
if (retval == 0) {
--
Thanks,
Dima
^ permalink raw reply
* Re: [PATCH v4 15/15] MAINTAINERS: Add entry for RDA Micro SoC architecture
From: Manivannan Sadhasivam @ 2018-12-12 14:46 UTC (permalink / raw)
To: Andreas Färber
Cc: olof, arnd, robh+dt, tglx, jason, marc.zyngier, daniel.lezcano,
gregkh, jslaby, devicetree, linus.walleij, linux-kernel,
amit.kucheria, linux-unisoc, linux-serial, linux-arm-kernel
In-Reply-To: <3e4b7143-6ed8-6006-cb07-d72bca40ce5f@suse.de>
Hi Andreas,
On Wed, Dec 12, 2018 at 02:53:42PM +0100, Andreas Färber wrote:
> Hi Mani,
>
> Am 10.12.18 um 18:35 schrieb Manivannan Sadhasivam:
> > Add MAINTAINERS entry for RDA Micro SoC architecture with myself
> > and Andreas Färber as the maintainers.
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> > MAINTAINERS | 15 +++++++++++++++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 6c3fbbb361f8..6411fb222590 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1945,6 +1945,21 @@ M: Lennert Buytenhek <kernel@wantstofly.org>
> > L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
> > S: Maintained
> >
> > +ARM/RDA MICRO ARCHITECTURE
> > +M: Andreas Färber <afaerber@suse.de>
> > +M: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>
> I'd suggest you swap the two of us here, making you the primary
> maintainer. Or drop me, as I should be copied via linux-unisoc anyway.
>
> I won't realistically have the bandwidth to drive review and more pull
> requests at the moment, with Realtek already stuck in irqchip regression
> for too long and LoRa making only slow progress.
>
Okay, no issues. Will drop you from the MAINTAINERS entry so that you
won't get bugged.
Thanks,
Mani
> Regards,
> Andreas
>
> > +L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
> > +L: linux-unisoc@lists.infradead.org (moderated for non-subscribers)
> > +S: Maintained
> > +F: arch/arm/boot/dts/rda8810pl-*
> > +F: drivers/clocksource/timer-rda.c
> > +F: drivers/irqchip/irq-rda-intc.c
> > +F: drivers/tty/serial/rda-uart.c
> > +F: Documentation/devicetree/bindings/arm/rda.txt
> > +F: Documentation/devicetree/bindings/interrupt-controller/rda,8810pl-intc.txt
> > +F: Documentation/devicetree/bindings/serial/rda,8810pl-uart.txt
> > +F: Documentation/devicetree/bindings/timer/rda,8810pl-timer.txt
> > +
> > ARM/REALTEK ARCHITECTURE
> > M: Andreas Färber <afaerber@suse.de>
> > L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
> >
>
>
> --
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
^ permalink raw reply
* Re: [PATCH v4 15/15] MAINTAINERS: Add entry for RDA Micro SoC architecture
From: Andreas Färber @ 2018-12-12 13:53 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: olof, arnd, robh+dt, tglx, jason, marc.zyngier, daniel.lezcano,
gregkh, jslaby, devicetree, linus.walleij, linux-kernel,
amit.kucheria, linux-unisoc, linux-serial, linux-arm-kernel
In-Reply-To: <20181210173550.29643-16-manivannan.sadhasivam@linaro.org>
Hi Mani,
Am 10.12.18 um 18:35 schrieb Manivannan Sadhasivam:
> Add MAINTAINERS entry for RDA Micro SoC architecture with myself
> and Andreas Färber as the maintainers.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
> MAINTAINERS | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6c3fbbb361f8..6411fb222590 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1945,6 +1945,21 @@ M: Lennert Buytenhek <kernel@wantstofly.org>
> L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
> S: Maintained
>
> +ARM/RDA MICRO ARCHITECTURE
> +M: Andreas Färber <afaerber@suse.de>
> +M: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
I'd suggest you swap the two of us here, making you the primary
maintainer. Or drop me, as I should be copied via linux-unisoc anyway.
I won't realistically have the bandwidth to drive review and more pull
requests at the moment, with Realtek already stuck in irqchip regression
for too long and LoRa making only slow progress.
Regards,
Andreas
> +L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
> +L: linux-unisoc@lists.infradead.org (moderated for non-subscribers)
> +S: Maintained
> +F: arch/arm/boot/dts/rda8810pl-*
> +F: drivers/clocksource/timer-rda.c
> +F: drivers/irqchip/irq-rda-intc.c
> +F: drivers/tty/serial/rda-uart.c
> +F: Documentation/devicetree/bindings/arm/rda.txt
> +F: Documentation/devicetree/bindings/interrupt-controller/rda,8810pl-intc.txt
> +F: Documentation/devicetree/bindings/serial/rda,8810pl-uart.txt
> +F: Documentation/devicetree/bindings/timer/rda,8810pl-timer.txt
> +
> ARM/REALTEK ARCHITECTURE
> M: Andreas Färber <afaerber@suse.de>
> L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
>
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply
* Re: [RFC][PATCHv2 3/4] serial: introduce uart_port locking helpers
From: Greg Kroah-Hartman @ 2018-12-12 11:08 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Petr Mladek, Steven Rostedt, Jiri Slaby, linux-kernel,
Daniel Wang, Peter Zijlstra, Andrew Morton, Linus Torvalds,
Alan Cox, Peter Feiner, linux-serial, Sergey Senozhatsky
In-Reply-To: <20181208031249.GA443@jagdpanzerIV>
On Sat, Dec 08, 2018 at 12:12:49PM +0900, Sergey Senozhatsky wrote:
> On (10/16/18 14:04), Sergey Senozhatsky wrote:
> [..]
> > - The first entry point is console ->write() callback, which we call
> > from printk(). A possible deadlock scenario there is:
> >
> > CPU0
> > <NMI>
> > spin_lock_irqsave(&port->lock, flags) << deadlock
> > serial_foo_write()
> > call_console_drivers()
> > console_unlock()
> > console_flush_on_panic()
> > panic()
> > <NMI/>
> > spin_lock_irqsave(&port->lock, flags)
> > serial_foo_write()
> > call_console_drivers()
> > console_unlock()
> > printk()
> > ...
>
> [..]
> > - The rest (of entry points) requires a bit different handling.
> > Let's take a look at the following backtrace:
> >
> > CPU0
> > <IRQ>
> > spin_lock_irqsave(&port->lock, flags) << deadlock
> > serial_foo_write()
> > call_console_drivers()
> > console_unlock()
> > printk()
> > __queue_work()
> > tty_flip_buffer_push()
> > spin_lock_irqsave(&port->lock, flags)
> > serial_foo_handle_IRQ()
> > <IRQ/>
> >
> > Serial drivers invoke tons of core kernel functions - WQ, MM, etc. All
> > of which may printk() in various cases. So we can't really just
> > "remove those printk-s". The simples way to address this seems to be
> > PRINTK_SAFE_CONTEXT_MASK.
>
> serial/UART and printk guys, sorry to bother you, do you hate this
> idea of removing console_driver->CORE KERNEL->printk->console_driver
> deadlock path? Or is there any chance we can move forward?
If done in a sane manner, no objection from me.
^ permalink raw reply
* Re: [LKP] [tty] c96cf923a9: WARNING:possible_circular_locking_dependency_detected
From: Peter Zijlstra @ 2018-12-12 9:07 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Dmitry Safonov, Jiri Slaby, kernel test robot, lkp,
Greg Kroah-Hartman, Mikulas Patocka, LKML, linux-serial,
Sergey Senozhatsky, Petr Mladek, Steven Rostedt, Waiman Long
In-Reply-To: <20181212034252.GD431@jagdpanzerIV>
On Wed, Dec 12, 2018 at 12:42:52PM +0900, Sergey Senozhatsky wrote:
> Something like this (not tested):
>
> ---
>
> drivers/tty/serial/serial_core.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index c439a5a1e6c0..64050f506348 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -268,6 +268,7 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
> struct uart_port *uport = uart_port_check(state);
> struct tty_port *port = &state->port;
> unsigned long flags = 0;
> + char *xmit_buf = NULL;
>
> /*
> * Set the TTY IO error marker
> @@ -297,15 +298,16 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
> */
> tty_port_set_suspended(port, 0);
>
> + uart_port_lock(state, flags);
> + xmit_buf = state->xmit.buf;
> + state->xmit.buf = NULL;
> + uart_port_unlock(uport, flags);
> +
> /*
> * Free the transmit buffer page.
> */
> - uart_port_lock(state, flags);
> - if (state->xmit.buf) {
> - free_page((unsigned long)state->xmit.buf);
> - state->xmit.buf = NULL;
> - }
> - uart_port_unlock(uport, flags);
> + if (xmit_buf)
> + free_page((unsigned long)xmit_buf);
> }
That looks sensible anyhow. One should strive to not do alloc or free
under locks as much as possible anyway.
^ permalink raw reply
* Re: [PATCHv3] panic: avoid deadlocks in re-entrant console drivers
From: Daniel Wang @ 2018-12-12 6:09 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Petr Mladek, linux-kernel, Steven Rostedt, Peter Zijlstra,
Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, Alan Cox,
Jiri Slaby, Peter Feiner, linux-serial, Sergey Senozhatsky
In-Reply-To: <20181212060616.GH431@jagdpanzerIV>
[-- Attachment #1: Type: text/plain, Size: 506 bytes --]
Got it. Thank you.
On Tue, Dec 11, 2018 at 10:06 PM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> On (12/11/18 21:59), Daniel Wang wrote:
> > No worries. I will follow up. You would recommend that all four
> > patches in this set to be backported though, right?
>
> Just the last one, which makes consoles re-entrant
>
> panic: avoid deadlocks in re-entrant console drivers
>
> Because only this one was applied so far. The reset got stuck.
>
> -ss
--
Best,
Daniel
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4849 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox