Re: sc16is7xx: a lot of time is spend in sc16is7xx_port_irq

Re: sc16is7xx: a lot of time is spend in sc16is7xx_port_irq

[Sean Nyekjær]

CC'ing RT

On 2016-02-24 16:31, Sean Nyekjær wrote:
> Hi
>
> I'm using the sc16is750 on an imx6d platform with RT patches applied.
>
> I have observed that sometimes the driver locks the whole system as is 
> loops inside the sc16is7xx_port_irq.
> I have applied some debug and seen we are looping upto 130000 times in 
> the while(1) loop.
> The SC16IS7XX_IIR_REG does signal the SC16IS7XX_IIR_XOFFI_SRC but the 
> SC16IS7XX_IER_XOFFI_BIT is not set.
>
> Please ask for more debug info if needed :-)
>
> Br
> Sean

--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: sc16is7xx: a lot of time is spend in sc16is7xx_port_irq

[Josh Cartwright]

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

On Wed, Feb 24, 2016 at 06:39:46PM +0100, Sean Nyekj?r wrote:
> CC'ing RT

I'm assuming this is with the other patch killing off IRQF_ONESHOT?

> On 2016-02-24 16:31, Sean Nyekjær wrote:
> >Hi
> >
> >I'm using the sc16is750 on an imx6d platform with RT patches applied.
> >
> >I have observed that sometimes the driver locks the whole system as is
> >loops inside the sc16is7xx_port_irq.
> >I have applied some debug and seen we are looping upto 130000 times in the
> >while(1) loop.
> >The SC16IS7XX_IIR_REG does signal the SC16IS7XX_IIR_XOFFI_SRC but the
> >SC16IS7XX_IER_XOFFI_BIT is not set.

Ouch.

I don't quite understand how the interrupt handling in this driver is
supposed to work.  sc16is7xx_irq() is invoked to handle the interrupt,
but it does absolutely nothing to squelch it...  how does this not cause
an interrupt storm?  The irq core is free to unmask the interrupt when
the handler return IRQ_HANDLED, and if you're dealing with
level-sensitive interrupts...

  Josh

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

Re: sc16is7xx: a lot of time is spend in sc16is7xx_port_irq

[Sebastian Andrzej Siewior]

* Josh Cartwright | 2016-02-24 16:35:47 [-0600]:

>On Wed, Feb 24, 2016 at 06:39:46PM +0100, Sean Nyekj?r wrote:
>> CC'ing RT
>
>I'm assuming this is with the other patch killing off IRQF_ONESHOT?
>
>> On 2016-02-24 16:31, Sean Nyekjær wrote:
>> >Hi
>> >
>> >I'm using the sc16is750 on an imx6d platform with RT patches applied.
>> >
>> >I have observed that sometimes the driver locks the whole system as is
>> >loops inside the sc16is7xx_port_irq.
>> >I have applied some debug and seen we are looping upto 130000 times in the
>> >while(1) loop.
>> >The SC16IS7XX_IIR_REG does signal the SC16IS7XX_IIR_XOFFI_SRC but the
>> >SC16IS7XX_IER_XOFFI_BIT is not set.
>
>Ouch.
>
>I don't quite understand how the interrupt handling in this driver is
>supposed to work.  sc16is7xx_irq() is invoked to handle the interrupt,
>but it does absolutely nothing to squelch it...  how does this not cause
>an interrupt storm?  The irq core is free to unmask the interrupt when
>the handler return IRQ_HANDLED, and if you're dealing with
>level-sensitive interrupts...

The irqcore does not shut down the driver because it always returns
IRQ_HANDLED. The reason why don't end up in a storm is the IRQF_ONESHOT
part. However it should use proper irq threads and not this homegrow
stuff. I could be a bug in general and not related to -RT.

>  Josh

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] tty: serial: sc16is7xx: use threaded interrupts instead of homegrow

[Sebastian Andrzej Siewior]

This ONESHOT + workqueue combo is something that is not required because
we have infrastrucure for this kind of things: threaded interrupts.

This is compile tested only due to -ENODEV.
Now that we that sc16is7xx_irq() is an actual interrupt handler
sc16is7xx_port_irq() could be improved so the former can return IRQ_NONE
if nothing has been done.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---

 drivers/tty/serial/sc16is7xx.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index edb5305b9d4d..b53a13be5754 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -331,7 +331,6 @@ struct sc16is7xx_port {
 	unsigned char			buf[SC16IS7XX_FIFO_SIZE];
 	struct kthread_worker		kworker;
 	struct task_struct		*kworker_task;
-	struct kthread_work		irq_work;
 	struct sc16is7xx_one		p[0];
 };
 
@@ -688,20 +687,13 @@ static void sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
 	} while (1);
 }
 
-static void sc16is7xx_ist(struct kthread_work *ws)
+static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
 {
-	struct sc16is7xx_port *s = to_sc16is7xx_port(ws, irq_work);
+	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
 	int i;
 
 	for (i = 0; i < s->devtype->nr_uart; ++i)
 		sc16is7xx_port_irq(s, i);
-}
-
-static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
-{
-	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
-
-	queue_kthread_work(&s->kworker, &s->irq_work);
 
 	return IRQ_HANDLED;
 }
@@ -1167,7 +1159,6 @@ static int sc16is7xx_probe(struct device *dev,
 	dev_set_drvdata(dev, s);
 
 	init_kthread_worker(&s->kworker);
-	init_kthread_work(&s->irq_work, sc16is7xx_ist);
 	s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
 				      "sc16is7xx");
 	if (IS_ERR(s->kworker_task)) {
@@ -1229,8 +1220,8 @@ static int sc16is7xx_probe(struct device *dev,
 	}
 
 	/* Setup interrupt */
-	ret = devm_request_irq(dev, irq, sc16is7xx_irq,
-			       IRQF_ONESHOT | flags, dev_name(dev), s);
+	ret = devm_request_threaded_irq(dev, irq, NULL, sc16is7xx_irq,
+			       flags, dev_name(dev), s);
 	if (!ret)
 		return 0;
 
-- 
2.7.0

Re: [PATCH] tty: serial: sc16is7xx: use threaded interrupts instead of homegrow

[Kuba Kicinski]

On 26 February 2016 06:48:09 GMT-05:00, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
>This ONESHOT + workqueue combo is something that is not required
>because
>we have infrastrucure for this kind of things: threaded interrupts.
>
>This is compile tested only due to -ENODEV.
>Now that we that sc16is7xx_irq() is an actual interrupt handler
>sc16is7xx_port_irq() could be improved so the former can return
>IRQ_NONE
>if nothing has been done.
>
>Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

The reason why this driver is not using threaded irqs is that it already has a kthread to handle other things. Having two threads per port seems like a big waste plus we may need some additional locking between them. Can we just return IRQ_WAKE_THREAD from the handler?

IMHO drivers should be perfectly fine to have their own threaded irq setups and core needs to support it.

Re: [PATCH] tty: serial: sc16is7xx: use threaded interrupts instead of homegrow

[Josh Cartwright]

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

On Fri, Feb 26, 2016 at 09:34:57AM -0500, Kuba Kicinski wrote:
> On 26 February 2016 06:48:09 GMT-05:00, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> >This ONESHOT + workqueue combo is something that is not required
> >because
> >we have infrastrucure for this kind of things: threaded interrupts.
> >
> >This is compile tested only due to -ENODEV.
> >Now that we that sc16is7xx_irq() is an actual interrupt handler
> >sc16is7xx_port_irq() could be improved so the former can return
> >IRQ_NONE
> >if nothing has been done.
> >
> >Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>
> The reason why this driver is not using threaded irqs is that it
> already has a kthread to handle other things.

The core problem here is still the use of IRQF_ONESHOT.  The semantics
of IRQF_ONESHOT are only defined w.r.t. the irq core's threaded
interrupt handling.

If the driver isn't going to make use of the existing irqthread
functionality, then it _also_ should not be making use of IRQF_ONESHOT.

Instead, the driver needs to implement it's own oneshot-like handling at
the device-level: in the registered irq handler, capture triggered
interrupt state, squelch/mask, and enqueue the kthread_work.  In the
tail-end of the kthread_work, re-enable interrupts at the device level.

Now, with forced_irqthreads, this still isn't ideal, as we'd have two
threads, but perhaps the right way forward there is to be more clever at
the kthread_worker() layer to make it's work list management amenable to
being done under a raw spinlock.

  Josh

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

Re: [PATCH] tty: serial: sc16is7xx: use threaded interrupts instead of homegrow

[Kuba Kicinski]

On 26 February 2016 11:52:28 GMT-05:00, Josh Cartwright <joshc@ni.com> wrote:
>On Fri, Feb 26, 2016 at 09:34:57AM -0500, Kuba Kicinski wrote:
>> On 26 February 2016 06:48:09 GMT-05:00, Sebastian Andrzej Siewior
><bigeasy@linutronix.de> wrote:
>> >This ONESHOT + workqueue combo is something that is not required
>> >because
>> >we have infrastrucure for this kind of things: threaded interrupts.
>> >
>> >This is compile tested only due to -ENODEV.
>> >Now that we that sc16is7xx_irq() is an actual interrupt handler
>> >sc16is7xx_port_irq() could be improved so the former can return
>> >IRQ_NONE
>> >if nothing has been done.
>> >
>> >Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>>
>> The reason why this driver is not using threaded irqs is that it
>> already has a kthread to handle other things.
>
>The core problem here is still the use of IRQF_ONESHOT.  The semantics
>of IRQF_ONESHOT are only defined w.r.t. the irq core's threaded
>interrupt handling.
>
>If the driver isn't going to make use of the existing irqthread
>functionality, then it _also_ should not be making use of IRQF_ONESHOT.

Yes, I definitely dropped the ball there.

>Instead, the driver needs to implement it's own oneshot-like handling
>at
>the device-level: in the registered irq handler, capture triggered
>interrupt state, squelch/mask, and enqueue the kthread_work.  In the
>tail-end of the kthread_work, re-enable interrupts at the device level.

The problem there being IIRC that i2c doesn't provide async writes so we can't mask from irq callback. The only option would be disable_irq/enable_irq, right?

Re: [PATCH] tty: serial: sc16is7xx: use threaded interrupts instead of homegrow

[Josh Cartwright]

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

On Fri, Feb 26, 2016 at 01:26:27PM -0500, Kuba Kicinski wrote:
> On 26 February 2016 11:52:28 GMT-05:00, Josh Cartwright <joshc@ni.com> wrote:
[..]
> > Instead, the driver needs to implement it's own oneshot-like
> > handling at the device-level: in the registered irq handler, capture
> > triggered interrupt state, squelch/mask, and enqueue the
> > kthread_work.  In the tail-end of the kthread_work, re-enable
> > interrupts at the device level.
>
> The problem there being IIRC that i2c doesn't provide async writes so
> we can't mask from irq callback. The only option would be
> disable_irq/enable_irq, right?

Ah, yes, that is a problem.  If by disable_irq(), you mean
disable_irq_nosync(), then yes, I think that'd work.

  Josh

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

Re: [PATCH] tty: serial: sc16is7xx: use threaded interrupts instead of homegrow

[Sebastian Andrzej Siewior]

On 02/26/2016 08:00 PM, Josh Cartwright wrote:
> On Fri, Feb 26, 2016 at 01:26:27PM -0500, Kuba Kicinski wrote:
>> On 26 February 2016 11:52:28 GMT-05:00, Josh Cartwright <joshc@ni.com> wrote:
> [..]
>>> Instead, the driver needs to implement it's own oneshot-like
>>> handling at the device-level: in the registered irq handler, capture
>>> triggered interrupt state, squelch/mask, and enqueue the
>>> kthread_work.  In the tail-end of the kthread_work, re-enable
>>> interrupts at the device level.
>>
>> The problem there being IIRC that i2c doesn't provide async writes so
>> we can't mask from irq callback. The only option would be
>> disable_irq/enable_irq, right?
> 
> Ah, yes, that is a problem.  If by disable_irq(), you mean
> disable_irq_nosync(), then yes, I think that'd work.

I got lost here. Where do we stand here now?

> 
>   Josh
> 
Sebastian

Re: [PATCH] tty: serial: sc16is7xx: use threaded interrupts instead of homegrow

[Josh Cartwright]

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

On Mon, Mar 07, 2016 at 05:41:14PM +0100, Sebastian Andrzej Siewior wrote:
> On 02/26/2016 08:00 PM, Josh Cartwright wrote:
> > On Fri, Feb 26, 2016 at 01:26:27PM -0500, Kuba Kicinski wrote:
> >> On 26 February 2016 11:52:28 GMT-05:00, Josh Cartwright <joshc@ni.com> wrote:
> > [..]
> >>> Instead, the driver needs to implement it's own oneshot-like
> >>> handling at the device-level: in the registered irq handler, capture
> >>> triggered interrupt state, squelch/mask, and enqueue the
> >>> kthread_work.  In the tail-end of the kthread_work, re-enable
> >>> interrupts at the device level.
> >>
> >> The problem there being IIRC that i2c doesn't provide async writes so
> >> we can't mask from irq callback. The only option would be
> >> disable_irq/enable_irq, right?
> > 
> > Ah, yes, that is a problem.  If by disable_irq(), you mean
> > disable_irq_nosync(), then yes, I think that'd work.
> 
> I got lost here. Where do we stand here now?

I understood the comment from Kuba to mean that he would be implementing
the disable_irq()/enable_irq() idea above to fix all the problems with
this driver.

Kuba- did I read that right?

Sean- are you still stuck without this?

Thanks for the ping,
  Josh

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

Re: [PATCH] tty: serial: sc16is7xx: use threaded interrupts instead of homegrow

[Sean Nyekjær]

On 2016-03-07 17:58, Josh Cartwright wrote:
> On Mon, Mar 07, 2016 at 05:41:14PM +0100, Sebastian Andrzej Siewior wrote:
>> On 02/26/2016 08:00 PM, Josh Cartwright wrote:
>>> On Fri, Feb 26, 2016 at 01:26:27PM -0500, Kuba Kicinski wrote:
>>>> On 26 February 2016 11:52:28 GMT-05:00, Josh Cartwright <joshc@ni.com> wrote:
>>> [..]
>>>>> Instead, the driver needs to implement it's own oneshot-like
>>>>> handling at the device-level: in the registered irq handler, capture
>>>>> triggered interrupt state, squelch/mask, and enqueue the
>>>>> kthread_work.  In the tail-end of the kthread_work, re-enable
>>>>> interrupts at the device level.
>>>> The problem there being IIRC that i2c doesn't provide async writes so
>>>> we can't mask from irq callback. The only option would be
>>>> disable_irq/enable_irq, right?
>>> Ah, yes, that is a problem.  If by disable_irq(), you mean
>>> disable_irq_nosync(), then yes, I think that'd work.
>> I got lost here. Where do we stand here now?
> I understood the comment from Kuba to mean that he would be implementing
> the disable_irq()/enable_irq() idea above to fix all the problems with
> this driver.
>
> Kuba- did I read that right?
>
> Sean- are you still stuck without this?
The oneshot fix, fixed it for me :-)

We have encountered another problem regarding flow control in this driver.
Flowcontrol simply gets deactivated right after it's activated :-(
My college will submit a patch, hopefully in a couple of days...

And the "sc16is7xx_get_mctrl() is invoked under the uart port spinlock" 
problem is still there but with RT patches it's hidden.

/Sean
>
> Thanks for the ping,
>    Josh

Re: [PATCH] tty: serial: sc16is7xx: use threaded interrupts instead of homegrow

[Jakub Kicinski]

On Mon, 7 Mar 2016 10:58:09 -0600, Josh Cartwright wrote:
> > I got lost here. Where do we stand here now?  
> 
> I understood the comment from Kuba to mean that he would be implementing
> the disable_irq()/enable_irq() idea above to fix all the problems with
> this driver.
> 
> Kuba- did I read that right?

I was hoping Sean or someone else would take up this task ;)

It should be a pretty simple patch.

Kuba

Re: [PATCH] tty: serial: sc16is7xx: use threaded interrupts instead of homegrow

[Sean Nyekjær]

On 2016-03-08 22:18, Jakub Kicinski wrote:
> On Mon, 7 Mar 2016 10:58:09 -0600, Josh Cartwright wrote:
>>> I got lost here. Where do we stand here now?
>> I understood the comment from Kuba to mean that he would be implementing
>> the disable_irq()/enable_irq() idea above to fix all the problems with
>> this driver.
>>
>> Kuba- did I read that right?
> I was hoping Sean or someone else would take up this task ;)
Yes i could try :-) At least i have a working setup with the hardware.
>
> It should be a pretty simple patch.
>
> Kuba
I not entirely sure what i have to do...
- Reenable the ONE_SHOT
- Disable irq when running in the loop, and enable when returning?
- Implement threaded irq?

/Sean

Re: [PATCH] tty: serial: sc16is7xx: use threaded interrupts instead of homegrow

[Jakub Kicinski]

On Wed, 9 Mar 2016 08:03:06 +0100, Sean Nyekjær wrote:
> On 2016-03-08 22:18, Jakub Kicinski wrote:
> > On Mon, 7 Mar 2016 10:58:09 -0600, Josh Cartwright wrote:
> >>> I got lost here. Where do we stand here now?
> >> I understood the comment from Kuba to mean that he would be implementing
> >> the disable_irq()/enable_irq() idea above to fix all the problems with
> >> this driver.
> >>
> >> Kuba- did I read that right?
> > I was hoping Sean or someone else would take up this task ;)
> Yes i could try :-) At least i have a working setup with the hardware.
> >
> > It should be a pretty simple patch.
> >
> > Kuba
> I not entirely sure what i have to do...
> - Reenable the ONE_SHOT
> - Disable irq when running in the loop, and enable when returning?
> - Implement threaded irq?
> 
> /Sean

Do not reenable ONE_SHOT.  Disable interrupt (disable_irq_nosync())
in sc16is7xx_irq() and reenable at the end of sc16is7xx_ist().
I think reenabling at the end of sc16is7xx_ist() is fine since it's a
level triggered IRQ.
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tty: serial: sc16is7xx: use threaded interrupts instead of homegrow

[Sebastian Andrzej Siewior]

On 03/09/2016 12:13 PM, Jakub Kicinski wrote:
>> I not entirely sure what i have to do...
>> - Reenable the ONE_SHOT
>> - Disable irq when running in the loop, and enable when returning?
>> - Implement threaded irq?
>>
>> /Sean
> 
> Do not reenable ONE_SHOT.  Disable interrupt (disable_irq_nosync())
> in sc16is7xx_irq() and reenable at the end of sc16is7xx_ist().
> I think reenabling at the end of sc16is7xx_ist() is fine since it's a
> level triggered IRQ.

Josh's patch [0] which drops the ONE_SHOT flag has not yet been taken
by Greg. It sits only in the -RT tree at the moment. It might have
something to do with Greg not being in TO: or CC:

*I* don't care how you fix this in the end but please do something
before the merge window opens.

An ack by the maintainer of the driver and re-send to Greg (the usual
way) would be one way.

[0] http://lkml.iu.edu/hypermail/linux/kernel/1602.2/02637.html

Sebastian