On 2016-02-18 00:25, Josh Cartwright wrote:
On Wed, Feb 17, 2016 at 09:54:27PM +0000, Jakub Kicinski wrote:
[CC: linux-rt-users, can you guys help?]
On Wed, 17 Feb 2016 08:22:13 +0100, Sean Nyekjær wrote:
On 2016-02-16 22:02, Jakub Kici??ski wrote:
On Tue, 16 Feb 2016 08:37:11 +0100, Sean Nyekjær wrote:
On 2016-02-15 23:08, Jakub Kicinski wrote:
On Mon, 15 Feb 2016 08:58:53 +0100, Sean Nyekjaer wrote:
Working with RT patches reveals this driver have some spin_lock issues.
spin_lock moved from sc16is7xx_reg_proc to protect to call to
uart_handle_cts_change as it's the only call that isn't already
protected in serial-core.
Sorry but this does not look fine. You are removing all async items
from the driver. This works for you because in RT code can sleep with
spin locks taken but for upstream it's not acceptable. Did you test
this patch with upstream kernel and lockdep enabled?
I have a 4.1.y with cherry-pick of the commits from tty-testing
regarding sc16is7xx. (exept the new gpio api)
On our custom board we have 3 sc16is750 on a single SPI channel. I'm
using an FTDI chip(connected to a test PC) TX wired to the 3x RX pins on
the sc16is750.
The problem is easy reproducible just by creating data from the PC at
115200 baud, causes the kernel oops almost immediately.
I have not seen any issues with a vanilla 4.1.y, only when i'm applying
the RT patch and loading the system.
Thanks for providing the details, I think there is a misunderstanding
between us. Allow me to explain again what I meant ;)
I do acknowledge that there may be a problem with the driver on RT
kernels and I do trust you that you tested your patch on RT kernel
with latest version of the driver cherry-picked from mainline and it
works there.
However, IIUC you are proposing your patch to be included in mainline.
What I'm trying to say is that in mainline we need the async works
which you remove in your patch because we cannot perform SPI/I2C
blocking I/O under a spinlock... If you compile mainline with your
patch you will see a slew of "sleeping in atomic context" warnings.
So basically the questions are do you want this patch in mainline and
have you tested it on non-RT kernels? (Please don't be discouraged -
there definitely must be a bug somewhere if you're seeing crashes, but
I'm afraid the proposed solutions is not good enough...)
Thanks
Yeah the first priority must be for the driver to work on the mainline
kernel :-)
I just wanted to flag a problem with the driver with the RT patches
applied. I thought
RT patch (maybe) revaled that the driver have underlying problem.
So maybe this patch should be included in the RT patch?
No i have not tested this with a non-RT kernel, maybe i should try :-)
I've looked at the stack dump you posted in previous email. It looks
like we deadlock on the on spinlock_irqsave() in queue_kthread_work()
because our interrupt is not threaded!? I thought all interrupts are
threaded in RT by default.
I believe the actual problem is that the handler is being registered w/
IRQF_ONESHOT. I'm not sure that even makes sense in this scenario, but
is perhaps an unintended carry over from request_threaded_irq ->
request_irq transition in 9e6f4ca3 ("sc16is7xx: use kthread_worker for
tx_work and irq").
Sean-
Does this solve your problem?
Josh
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index e78fa99..1bb5c0e 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1257,7 +1257,7 @@ static int sc16is7xx_probe(struct device *dev,
/* Setup interrupt */
ret = devm_request_irq(dev, irq, sc16is7xx_irq,
- IRQF_ONESHOT | flags, dev_name(dev), s);
+ flags, dev_name(dev), s);
if (!ret)
return 0;
Wow, nice that you CC'd the RT guys :-)
Josh that was it, it runs without deadlocks now :-D
Thank you.
Jakub, is this fix ok? Who is making this into a PATCH?
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
