Skip to contentSkip to conversation

sc16is7xx: a lot of time is spend in sc16is7xx_port_irq

PATCH In review Sean Nyekjær 7 participants 25 messages Feb 24, 2016 – Mar 14, 2016 1 message not in the archive TTY layer and serial drivers

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Sean Nyekjær

author
#1 lore raw

CC'ing RT

Quoted 4 lines · Sean Nyekjær

Hi

I'm using the sc16is750 on an imx6d platform with RT patches applied.

Quoted 8 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#2 lore raw
Quoted 1 line · Sean Nyekj?r

CC'ing RT

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

Quoted 11 lines · Sean Nyekjær
Quoted 10 lines

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

#3 lore raw
Quoted 17 lines · * Josh Cartwright | 2016-02-24 16:35:47 [-0600]:
Quoted 1 line · Sean Nyekj?r

CC'ing RT

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

Quoted 11 lines · Sean Nyekjær
Quoted 10 lines

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.

Quoted 8 lines

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.

Quoted 1 line

Josh

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

#4 lore raw

PATCH 1/1 tty: serial: sc16is7xx: use threaded interrupts instead of homegrow

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 <[email protected]>

---

 drivers/tty/serial/sc16is7xx.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)
drivers/tty/serial/sc16is7xx.c +4 -13
index edb5305b9d4d..b53a13be5754 100644
@@ -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;
Signature · 1 line
2.7.0
#5 lore raw

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

Quoted 11 lines · 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 <[email protected]>

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.

#6 lore raw

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

Quoted 11 lines · Kuba Kicinski
Quoted 10 lines · 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.

Quoted 4 lines
Quoted 1 line
Signed-off-by Sebastian Andrzej Siewior <[email protected]>

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

#7 lore raw

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

Quoted 18 lines · Josh Cartwright
Quoted 1 line · Kuba Kicinski

On 26 February 2016 06:48:09 GMT-05:00, Sebastian Andrzej Siewior

Quoted 10 lines · <[email protected]> wrote:
Quoted 10 lines

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.

Quoted 4 lines
Quoted 1 line
Signed-off-by Sebastian Andrzej Siewior <[email protected]>

The reason why this driver is not using threaded irqs is that it already has a kthread to handle other things.

Quoted 6 lines

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.

Quoted 5 lines

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?

#8 lore raw

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

Quoted 1 line · Kuba Kicinski

On 26 February 2016 11:52:28 GMT-05:00, Josh Cartwright <[email protected]> wrote:

[..]

Quoted 9 lines
Quoted 5 lines

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

#9 lore raw

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

Quoted 9 lines · Josh Cartwright
Quoted 1 line · Kuba Kicinski

On 26 February 2016 11:52:28 GMT-05:00, Josh Cartwright <[email protected]> wrote:

[..]

Quoted 6 lines
Quoted 5 lines

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.

Quoted 6 lines
Quoted 3 lines

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?

Quoted 3 lines

Josh

Sebastian

#10 lore raw

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

Quoted 10 lines · Sebastian Andrzej Siewior
Quoted 9 lines · Josh Cartwright
Quoted 1 line · Kuba Kicinski

On 26 February 2016 11:52:28 GMT-05:00, Josh Cartwright <[email protected]> wrote:

[..]

Quoted 6 lines
Quoted 5 lines

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.

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

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

author
#11 lore raw

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

Quoted 15 lines · Josh Cartwright
Quoted 14 lines · Sebastian Andrzej Siewior
Quoted 13 lines · Josh Cartwright
Quoted 1 line · Kuba Kicinski

On 26 February 2016 11:52:28 GMT-05:00, Josh Cartwright <[email protected]> wrote:

[..]

Quoted 8 lines
Quoted 5 lines

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.

Quoted 8 lines
Quoted 1 line

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

Quoted 3 lines

Thanks for the ping,
Josh

#12 lore raw

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

In reply to Josh Cartwright
Quoted 7 lines · Josh Cartwright
Quoted 1 line

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

author
#13 lore raw

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

Quoted 8 lines · Jakub Kicinski
Quoted 6 lines · Josh Cartwright
Quoted 1 line

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.

Quoted 4 lines

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

#14 lore raw

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

Quoted 13 lines · Sean Nyekjær
Quoted 8 lines · Jakub Kicinski
Quoted 6 lines · Josh Cartwright
Quoted 1 line

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.

Quoted 3 lines

It should be a pretty simple patch.

Quoted 7 lines
Quoted 1 line

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#15 lore raw

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

Quoted 11 lines · Jakub Kicinski
Quoted 6 lines

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

author
#16 lore raw

PATCH 1/1 tty: serial: sc16is7xx: implemented our own oneshot-like handling

Signed-off-by Sean Nyekjaer <[email protected]>

---
This patch depends on patch "sc16is7xx: drop bogus use of IRQF_ONESHOT"

 drivers/tty/serial/sc16is7xx.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/tty/serial/sc16is7xx.c +8 -1
index 73d46e2..96d1f3e 100644
@@ -706,13 +706,20 @@ static void sc16is7xx_ist(struct kthread_work *ws)
 	struct sc16is7xx_port *s = to_sc16is7xx_port(ws, irq_work);
 	int i;
 
-	for (i = 0; i < s->devtype->nr_uart; ++i)
+	for (i = 0; i < s->devtype->nr_uart; ++i) {
 		sc16is7xx_port_irq(s, i);
+		enable_irq(s->p[i].port.irq);
+	}
+
 }
 
 static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
 {
 	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
+	int i;
+
+	for (i = 0; i < s->devtype->nr_uart; ++i)
+		disable_irq_nosync(s->p[i].port.irq);
 
 	queue_kthread_work(&s->kworker, &s->irq_work);
Signature · 1 line
2.7.2
author
#17 lore raw

Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

Quoted 12 lines · Sean Nyekjaer
Signed-off-by Sean Nyekjaer <[email protected]>

---
This patch depends on patch "sc16is7xx: drop bogus use of IRQF_ONESHOT"

  drivers/tty/serial/sc16is7xx.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)
drivers/tty/serial/sc16is7xx.c +0 -0
index 73d46e2..96d1f3e 100644

drivers/tty/serial/sc16is7xx.c static void sc16is7xx_ist(struct kthread_work *ws) · line 713

Quoted 9 lines
drivers/tty/serial/sc16is7xx.c +4 -1
@@ -706,13 +706,20 @@ static void sc16is7xx_ist(struct kthread_work *ws)
  	struct sc16is7xx_port *s = to_sc16is7xx_port(ws, irq_work);
  	int i;
 
-	for (i = 0; i < s->devtype->nr_uart; ++i)
+	for (i = 0; i < s->devtype->nr_uart; ++i) {
  		sc16is7xx_port_irq(s, i);
+		enable_irq(s->p[i].port.irq);
+	}
+

Ops one-line break to much :-(
Waiting for other comments before fixing :-)

/Sean

Quoted 12 lines
drivers/tty/serial/sc16is7xx.c +4 -0
  }
 
  static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
  {
  	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
+	int i;
+
+	for (i = 0; i < s->devtype->nr_uart; ++i)
+		disable_irq_nosync(s->p[i].port.irq);
 
  	queue_kthread_work(&s->kworker, &s->irq_work);
#18 lore raw

Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

Quoted 1 line · * Sean Nyekjær | 2016-03-11 11:36:42 [+0100]:

Waiting for other comments before fixing :-)

You could start by writting a changelog stating why you think this change is a good idea instead leaving it empty.

Quoted 1 line

/Sean

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

author
#19 lore raw

Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

Quoted 5 lines · Sebastian Andrzej Siewior
Quoted 1 line · * Sean Nyekjær | 2016-03-11 11:36:42 [+0100]:

Waiting for other comments before fixing :-)

You could start by writting a changelog stating why you think this change is a good idea instead leaving it empty.

Cool down a bit ;-)

It's left empty because I have been encouraged by Josh and Jakub to make this PATCH. I have not seen any problems after simply dropping the IRQ_ONESHOT flag, but they proposed this fix.

Quoted 2 lines
Quoted 1 line

/Sean

Sebastian

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

author
#20 lore raw

Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

In reply to Sean Nyekjær
Quoted 3 lines · Sean Nyekjær

On 2016-03-11 11:34, Sean Nyekjaer wrote:

Could this work as a description?

The 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.

This implements 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.

Quoted 24 lines
Quoted 12 lines
Signed-off-by Sean Nyekjaer <[email protected]>

---
This patch depends on patch "sc16is7xx: drop bogus use of IRQF_ONESHOT"

  drivers/tty/serial/sc16is7xx.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)
drivers/tty/serial/sc16is7xx.c +0 -0
index 73d46e2..96d1f3e 100644

drivers/tty/serial/sc16is7xx.c static void sc16is7xx_ist(struct kthread_work *ws) · line 713

Quoted 8 lines
drivers/tty/serial/sc16is7xx.c +4 -0
@@ -706,13 +706,20 @@ static void sc16is7xx_ist(struct kthread_work *ws)
      struct sc16is7xx_port *s = to_sc16is7xx_port(ws, irq_work);
      int i;
  -    for (i = 0; i < s->devtype->nr_uart; ++i)
+    for (i = 0; i < s->devtype->nr_uart; ++i) {
          sc16is7xx_port_irq(s, i);
+        enable_irq(s->p[i].port.irq);
+    }
+

Ops one-line break to much :-(
Waiting for other comments before fixing :-)

/Sean

Quoted 14 lines
Quoted 9 lines
drivers/tty/serial/sc16is7xx.c +4 -0
  }
    static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
  {
      struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
+    int i;
+
+    for (i = 0; i < s->devtype->nr_uart; ++i)
+        disable_irq_nosync(s->p[i].port.irq);
        queue_kthread_work(&s->kworker, &s->irq_work);
Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

#22 lore raw

Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

In reply to Sean Nyekjaer

drivers/tty/serial/sc16is7xx.c static void sc16is7xx_ist(struct kthread_work *ws) · line 722

Quoted 7 lines · Sean Nyekjaer

static irqreturn_t sc16is7xx_irq(int irq, void *dev_id) {

drivers/tty/serial/sc16is7xx.c +4 -0
 	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
+	int i;
+
+	for (i = 0; i < s->devtype->nr_uart; ++i)
+		disable_irq_nosync(s->p[i].port.irq);

Aside of the lack of a changelog. This is completely bogus. You disable the same interrupt a gazillion of times.

Quoted 1 line

queue_kthread_work(&s->kworker, &s->irq_work);

This driver should use a threaded interrupt instead of trying to emulate it via dis/enable_irq and a worker thread.

Then you simply call c16is7xx_port_irq() right from the interrupt routine and the core code deals with the interrupt mask/unmask automatically.

Thanks,

tglx

author
#23 lore raw

Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

Quoted 11 lines · Thomas Gleixner

drivers/tty/serial/sc16is7xx.c static void sc16is7xx_ist(struct kthread_work *ws) · line 722

Quoted 7 lines · Sean Nyekjaer
  static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
  {
  	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
drivers/tty/serial/sc16is7xx.c +4 -0
+	int i;
+
+	for (i = 0; i < s->devtype->nr_uart; ++i)
+		disable_irq_nosync(s->p[i].port.irq);

Aside of the lack of a changelog. This is completely bogus. You disable the same interrupt a gazillion of times.

I can't see why the interrupt is disabled a gazillion times.
When the irq is disabled the function will not be called before it's enabled again... Or have i missed something?

Quoted 3 lines
Quoted 1 line
  	queue_kthread_work(&s->kworker, &s->irq_work);

This driver should use a threaded interrupt instead of trying to emulate it via dis/enable_irq and a worker thread.

I agree, I and "Sebastian Andrzej Siewior" have proposed this fix, but it was turned down. :-)

Quoted 8 lines

Then you simply call c16is7xx_port_irq() right from the interrupt routine and the core code deals with the interrupt mask/unmask automatically.

Thanks,

tglx

/Sean

#24 lore raw

Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

Quoted 12 lines · Sean Nyekjær
Quoted 11 lines · Thomas Gleixner

drivers/tty/serial/sc16is7xx.c static void sc16is7xx_ist(struct kthread_work *ws) · line 722

Quoted 7 lines · Sean Nyekjaer
  static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
  {
  	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
drivers/tty/serial/sc16is7xx.c +4 -0
+	int i;
+
+	for (i = 0; i < s->devtype->nr_uart; ++i)
+		disable_irq_nosync(s->p[i].port.irq);

Aside of the lack of a changelog. This is completely bogus. You disable the same interrupt a gazillion of times.

Quoted 6 lines

I can't see why the interrupt is disabled a gazillion times. When the irq is disabled the function will not be called before it's enabled again... Or have i missed something?

Quoted 2 lines

drivers/tty/serial/sc16is7xx.c static void sc16is7xx_ist(struct kthread_work *ws) · line 722

Quoted 2 lines
drivers/tty/serial/sc16is7xx.c +2 -0
+	for (i = 0; i < s->devtype->nr_uart; ++i)
+		disable_irq_nosync(s->p[i].port.irq);

port.irq is the same for all ports at least according to the init function. It is actually @irq, the handler function argument.

Thanks,

tglx

#25 lore raw

Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

In reply to Sean Nyekjaer
Quoted 12 lines · Sean Nyekjaer
Signed-off-by Sean Nyekjaer <[email protected]>

---
This patch depends on patch "sc16is7xx: drop bogus use of IRQF_ONESHOT"

 drivers/tty/serial/sc16is7xx.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/tty/serial/sc16is7xx.c +0 -0
index 73d46e2..96d1f3e 100644

drivers/tty/serial/sc16is7xx.c static void sc16is7xx_ist(struct kthread_work *ws) · line 714

Quoted 10 lines
drivers/tty/serial/sc16is7xx.c +4 -1
@@ -706,13 +706,20 @@ static void sc16is7xx_ist(struct kthread_work *ws)
 	struct sc16is7xx_port *s = to_sc16is7xx_port(ws, irq_work);
 	int i;
 
-	for (i = 0; i < s->devtype->nr_uart; ++i)
+	for (i = 0; i < s->devtype->nr_uart; ++i) {
 		sc16is7xx_port_irq(s, i);
+		enable_irq(s->p[i].port.irq);
+	}
+
 }

Apart from needless multiple disable/enable (as tglx commented) I think you also have to re-read the IRQ status (i.e. call sc16is7xx_port_irq()) because IRQs coming between status read and enable_irq may get lost for some IRQ controllers.