can: fix sja1000 pre_irq/post_irq handling

PATCH Superseded Oliver Hartkopp 4 participants 9 messages Nov 17, 2013 – Nov 19, 2013 CAN network drivers

lore

Share & tools

lore

Oliver Hartkopp

author

2013-11-17 14:17 UTC #0 lore raw

PATCH 1/1 can: fix sja1000 pre_irq/post_irq handling

The SJA1000 driver provides optional function pointers to handle specific hardware setups at interrupt time.

This patch fixes the issue that the sja1000_interrupt() function may have returned IRQ_NONE without processing the post_irq function. Additionally an introduced return value from the pre_irq function is checked to skip the entire interrupt handling on pre_irq function errors.

Reported-by Wolfgang Grandegger <[email protected]>

Signed-off-by Oliver Hartkopp <[email protected]>

---

drivers/net/can/sja1000/sja1000.c +7 -7

index 7164a99..bb20470 100644
@@ -494,12 +494,12 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 	uint8_t isrc, status;
 	int n = 0;
 
+	if (priv->pre_irq && priv->pre_irq(priv))
+		goto out;
+
 	/* Shared interrupts and IRQ off? */
 	if (priv->read_reg(priv, SJA1000_IER) == IRQ_OFF)
-		return IRQ_NONE;
-
-	if (priv->pre_irq)
-		priv->pre_irq(priv);
+		goto out;
 
 	while ((isrc = priv->read_reg(priv, SJA1000_IR)) &&
 	       (n < SJA1000_MAX_IRQ)) {

@@ -507,7 +507,7 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 		status = priv->read_reg(priv, SJA1000_SR);
 		/* check for absent controller due to hw unplug */
 		if (status == 0xFF && sja1000_is_absent(priv))
-			return IRQ_NONE;
+			goto out;
 
 		if (isrc & IRQ_WUI)
 			netdev_warn(dev, "wakeup interrupt\n");

@@ -535,7 +535,7 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 				status = priv->read_reg(priv, SJA1000_SR);
 				/* check for absent controller */
 				if (status == 0xFF && sja1000_is_absent(priv))
-					return IRQ_NONE;
+					goto out;
 			}
 		}
 		if (isrc & (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {

@@ -544,7 +544,7 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 				break;
 		}
 	}
-
+out:
 	if (priv->post_irq)
 		priv->post_irq(priv);

drivers/net/can/sja1000/sja1000.h +1 -1

index 9d46398..53c9797 100644
@@ -157,7 +157,7 @@ struct sja1000_priv {
 	/* the lower-layer is responsible for appropriate locking */
 	u8 (*read_reg) (const struct sja1000_priv *priv, int reg);
 	void (*write_reg) (const struct sja1000_priv *priv, int reg, u8 val);
-	void (*pre_irq) (const struct sja1000_priv *priv);
+	int (*pre_irq) (const struct sja1000_priv *priv);
 	void (*post_irq) (const struct sja1000_priv *priv);
 
 	void *priv;		/* for board-specific data */

Wolfgang Grandegger

2013-11-17 20:51 UTC #1 lore raw

Hi Oliver,

Quoted 18 lines · Oliver Hartkopp

The SJA1000 driver provides optional function pointers to handle specific hardware setups at interrupt time.
This patch fixes the issue that the sja1000_interrupt() function may have returned IRQ_NONE without processing the post_irq function. Additionally an introduced return value from the pre_irq function is checked to skip the entire interrupt handling on pre_irq function errors.
Reported-by Wolfgang Grandegger <[email protected]>
Signed-off-by Oliver Hartkopp <[email protected]>
---
drivers/net/can/sja1000/sja1000.c +0 -0
index 7164a99..bb20470 100644

drivers/net/can/sja1000/sja1000.c irqreturn_t sja1000_interrupt(int irq, void *dev_id) · line 499

Quoted 6 lines

drivers/net/can/sja1000/sja1000.c +3 -0

@@ -494,12 +494,12 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 	uint8_t isrc, status;
 	int n = 0;
 
+	if (priv->pre_irq && priv->pre_irq(priv))
+		goto out;
+

Well, not sure if we need that. Looks like a fatal error to me.

Quoted 11 lines

drivers/net/can/sja1000/sja1000.c +1 -4

 	/* Shared interrupts and IRQ off? */
 	if (priv->read_reg(priv, SJA1000_IER) == IRQ_OFF)
-		return IRQ_NONE;
-
-	if (priv->pre_irq)
-		priv->pre_irq(priv);
+		goto out;
 
 	while ((isrc = priv->read_reg(priv, SJA1000_IR)) &&
 	       (n < SJA1000_MAX_IRQ)) {

drivers/net/can/sja1000/sja1000.c irqreturn_t sja1000_interrupt(int irq, void *dev_id) · line 510

Quoted 5 lines

drivers/net/can/sja1000/sja1000.c +1 -1

@@ -507,7 +507,7 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 		status = priv->read_reg(priv, SJA1000_SR);
 		/* check for absent controller due to hw unplug */
 		if (status == 0xFF && sja1000_is_absent(priv))
-			return IRQ_NONE;
+			goto out;

Be aware that the function will return IRQ_HANDLED if n > 0... which may happen. Not exactly the same behaviour.

drivers/net/can/sja1000/sja1000.c irqreturn_t sja1000_interrupt(int irq, void *dev_id) · line 538

Quoted 9 lines

drivers/net/can/sja1000/sja1000.c +1 -1

 		if (isrc & IRQ_WUI)
 			netdev_warn(dev, "wakeup interrupt\n");

@@ -535,7 +535,7 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 				status = priv->read_reg(priv, SJA1000_SR);
 				/* check for absent controller */
 				if (status == 0xFF && sja1000_is_absent(priv))
-					return IRQ_NONE;
+					goto out;

Ditto.

drivers/net/can/sja1000/sja1000.c irqreturn_t sja1000_interrupt(int irq, void *dev_id) · line 549

Quoted 11 lines

drivers/net/can/sja1000/sja1000.c +1 -1

 			}
 		}
 		if (isrc & (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {

@@ -544,7 +544,7 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 				break;
 		}
 	}
-
+out:
 	if (priv->post_irq)
 		priv->post_irq(priv);

Wolfgang.

Oliver Hartkopp

author

2013-11-18 06:17 UTC #2 lore raw

Quoted 24 lines · Wolfgang Grandegger

Hi Oliver,
Quoted 21 lines · Oliver Hartkopp
The SJA1000 driver provides optional function pointers to handle specific hardware setups at interrupt time.
This patch fixes the issue that the sja1000_interrupt() function may have returned IRQ_NONE without processing the post_irq function. Additionally an introduced return value from the pre_irq function is checked to skip the entire interrupt handling on pre_irq function errors.
Reported-by Wolfgang Grandegger <[email protected]>
Signed-off-by Oliver Hartkopp <[email protected]>
---
drivers/net/can/sja1000/sja1000.c +0 -0
index 7164a99..bb20470 100644
@@ -494,12 +494,12 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 	uint8_t isrc, status;
 	int n = 0;
 

Quoted 6 lines

drivers/net/can/sja1000/sja1000.c irqreturn_t sja1000_interrupt(int irq, void *dev_id) · line 499
Quoted 3 lines
drivers/net/can/sja1000/sja1000.c +3 -0
+	if (priv->pre_irq && priv->pre_irq(priv))
+		goto out;
+
Well, not sure if we need that. Looks like a fatal error to me.

Why?
Think about a pre_irq() function that can look into hw specific registers and which can determine, that there was no interrupt for this CAN device. In this case the access to the sja1000 registers might/should be skipped depending on the new return value of pre_irq().

So what's the error here?
Btw. by now there's no user of priv->pre_irq() anyway.

Quoted 8 lines

drivers/net/can/sja1000/sja1000.c +1 -4

 	/* Shared interrupts and IRQ off? */
 	if (priv->read_reg(priv, SJA1000_IER) == IRQ_OFF)
-		return IRQ_NONE;
-
-	if (priv->pre_irq)
-		priv->pre_irq(priv);
+		goto out;

Quoted 12 lines

drivers/net/can/sja1000/sja1000.c irqreturn_t sja1000_interrupt(int irq, void *dev_id) · line 510
Quoted 8 lines
drivers/net/can/sja1000/sja1000.c +1 -1
 	while ((isrc = priv->read_reg(priv, SJA1000_IR)) &&
 	       (n < SJA1000_MAX_IRQ)) {
@@ -507,7 +507,7 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 		status = priv->read_reg(priv, SJA1000_SR);
 		/* check for absent controller due to hw unplug */
 		if (status == 0xFF && sja1000_is_absent(priv))
-			return IRQ_NONE;
+			goto out;
Be aware that the function will return IRQ_HANDLED if n > 0... which may happen. Not exactly the same behaviour.

Right.

We should move the 'n++' from the begin to the end of this while statement.

Setting 'n=0' before 'goto out' won't be correct either, as you might have processed a number of correct CAN frames before.

Regards,
Oliver

Wolfgang Grandegger

2013-11-18 07:49 UTC #3 lore raw

On Mon, 18 Nov 2013 07:17:19 +0100, Oliver Hartkopp

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

Quoted 13 lines · Wolfgang Grandegger
Hi Oliver,
Quoted 7 lines · Oliver Hartkopp
The SJA1000 driver provides optional function pointers to handle
specific
hardware setups at interrupt time.

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
This patch fixes the issue that the sja1000_interrupt() function may
have
returned IRQ_NONE without processing the post_irq function.

Additionally

Quoted 1 line

Quoted 1 line
Quoted 1 line
an introduced return value from the pre_irq function is checked to

skip

Quoted 50 lines

Quoted 38 lines
Quoted 37 lines
the
entire interrupt handling on pre_irq function errors.
Reported-by Wolfgang Grandegger <[email protected]>
Signed-off-by Oliver Hartkopp <[email protected]>
---
(quoted diff) +0 -0
b/drivers/net/can/sja1000/sja1000.c
index 7164a99..bb20470 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
(quoted diff) +1 -0
@@ -494,12 +494,12 @@ irqreturn_t sja1000_interrupt(int irq, void
 
 *dev_id)
 
 	uint8_t isrc, status;
 
 	int n = 0;
 
 
 
+	if (priv->pre_irq && priv->pre_irq(priv))
Quoted 9 lines
drivers/net/can/sja1000/sja1000.c irqreturn_t sja1000_interrupt(int irq, void *dev_id) · line 499
Quoted 3 lines
drivers/net/can/sja1000/sja1000.c +2 -0
+		goto out;
 
+
Well, not sure if we need that. Looks like a fatal error to me.

Quoted 3 lines

Why?
Think about a pre_irq() function that can look into hw specific

registers

Quoted 7 lines

and
which can determine, that there was no interrupt for this CAN device.
In this case the access to the sja1000 registers might/should be skipped
depending on the new return value of pre_irq().

OK, I see. But you should add some comments about the valid return values.

Quoted 26 lines

So what's the error here?
Btw. by now there's no user of priv->pre_irq() anyway.
Quoted 15 lines
Quoted 15 lines
(quoted diff) +1 -4
 	/* Shared interrupts and IRQ off? */
 
 	if (priv->read_reg(priv, SJA1000_IER) == IRQ_OFF)
 
-		return IRQ_NONE;
 
-
 
-	if (priv->pre_irq)
 
-		priv->pre_irq(priv);
 
+		goto out;

Quoted 5 lines

Quoted 5 lines
drivers/net/can/sja1000/sja1000.c irqreturn_t sja1000_interrupt(int irq, void
Quoted 5 lines
drivers/net/can/sja1000/sja1000.c +0 -0
 	while ((isrc = priv->read_reg(priv, SJA1000_IR)) &&
 
 	       (n < SJA1000_MAX_IRQ)) {
@@ -507,7 +507,7 @@ irqreturn_t sja1000_interrupt(int irq, void

*dev_id)

Quoted 6 lines

Quoted 5 lines

drivers/net/can/sja1000/sja1000.c +0 -0

 		status = priv->read_reg(priv, SJA1000_SR);
 
 		/* check for absent controller due to hw unplug */
 
 		if (status == 0xFF && sja1000_is_absent(priv))

Quoted 7 lines

Quoted 7 lines
drivers/net/can/sja1000/sja1000.c irqreturn_t sja1000_interrupt(int irq, void *dev_id)
Quoted 3 lines
drivers/net/can/sja1000/sja1000.c +1 -1
-			return IRQ_NONE;
 
+			goto out;
Be aware that the function will return IRQ_HANDLED if n > 0... which

may

Quoted 11 lines

Quoted 3 lines
happen. Not exactly the same behaviour.
Right.
We should move the 'n++' from the begin to the end of this while

statement.

Quoted 3 lines

Setting 'n=0' before 'goto out' won't be correct either, as you might

have

Quoted 1 line

processed a number of correct CAN frames before.

I think you code is correct. If at least one interrupt has been handled we

should return IRQ_HANDLED. I just realized that this is another mistake,

apart from not calling "post_irq". Sorry for confusion.

Another thing I don't like is that the device present check is done for

all devices including devices which will never hotplug. I think this

code has been introduced for the PCMCIA/PCcard CAN controllers. I would

prefer somethink like:

         if (priv->hotpluggable && status == 0xFF &&

             sja1000_is_absent(priv)) {

                 ...

         }

What do you think?

Wolfgang.

Oliver Hartkopp

author

2013-11-18 13:13 UTC #4 lore raw

Quoted 22 lines · Wolfgang Grandegger

Another thing I don't like is that the device present check is done for
all devices including devices which will never hotplug. I think this
code has been introduced for the PCMCIA/PCcard CAN controllers. I would
prefer somethink like:
         if (priv->hotpluggable && status == 0xFF &&

             sja1000_is_absent(priv)) {

                 ...

         }

Quoted 1 line

What do you think?

The status variable is retrieved from the sja1000 register anyway and sits in the CPU register.

Checking the status against 0xFF already hints to several problems (bus-off together with overrun, etc. ) which are unusual in normal operation. Only in this unusual cased the sja1000_is_absent() is invoked.

This is pretty optimal.

Accessing priv->hotpluggable needs additional pointer dereferencing which is more costly than accessing/checking the already retrieved status variable.

Best regards,
Oliver

Wolfgang Grandegger

2013-11-18 13:33 UTC #5 lore raw

On Mon, 18 Nov 2013 14:13:55 +0100, Oliver Hartkopp

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

Quoted 43 lines · Wolfgang Grandegger
Another thing I don't like is that the device present check is done for
all devices including devices which will never hotplug. I think this
code has been introduced for the PCMCIA/PCcard CAN controllers. I would
prefer somethink like:
         if (priv->hotpluggable && status == 0xFF &&



             sja1000_is_absent(priv)) {



                 ...



         }

Quoted 5 lines

Quoted 1 line
What do you think?
The status variable is retrieved from the sja1000 register anyway and

sits

Quoted 7 lines

in
the CPU register.
Checking the status against 0xFF already hints to several problems

(bus-off

Quoted 11 lines

together with overrun, etc. ) which are unusual in normal operation.
Only in this unusual cased the sja1000_is_absent() is invoked.
This is pretty optimal.
Accessing priv->hotpluggable needs additional pointer dereferencing

which

Quoted 3 lines

is
more costly than accessing/checking the already retrieved status

variable.

You are right, sorry for the noise.

Wolfgang.

Kurt Van Dijck

2013-11-18 19:48 UTC #6 lore raw

In reply to Oliver Hartkopp

Oliver,

just 2 small remarks below.

Quoted 34 lines · Oliver Hartkopp

Quoted 24 lines · Wolfgang Grandegger
Hi Oliver,
Quoted 21 lines · Oliver Hartkopp
The SJA1000 driver provides optional function pointers to handle specific hardware setups at interrupt time.
This patch fixes the issue that the sja1000_interrupt() function may have returned IRQ_NONE without processing the post_irq function. Additionally an introduced return value from the pre_irq function is checked to skip the entire interrupt handling on pre_irq function errors.
Reported-by Wolfgang Grandegger <[email protected]>
Signed-off-by Oliver Hartkopp <[email protected]>
---
drivers/net/can/sja1000/sja1000.c +0 -0
index 7164a99..bb20470 100644
@@ -494,12 +494,12 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 	uint8_t isrc, status;
 	int n = 0;
 
Quoted 6 lines
drivers/net/can/sja1000/sja1000.c irqreturn_t sja1000_interrupt(int irq, void *dev_id) · line 499
Quoted 3 lines
drivers/net/can/sja1000/sja1000.c +3 -0
+	if (priv->pre_irq && priv->pre_irq(priv))
+		goto out;
+
Well, not sure if we need that. Looks like a fatal error to me.

Quoted 7 lines

Why?
Think about a pre_irq() function that can look into hw specific registers and which can determine, that there was no interrupt for this CAN device. In this case the access to the sja1000 registers might/should be skipped depending on the new return value of pre_irq().
So what's the error here?

The accuracy of reading register SJA1000_IR remains unbeaten when deciding if an SJA1000 has interrupts pending.
Any pre_irq() return value is close approximation.

Quoted 1 line

Btw. by now there's no user of priv->pre_irq() anyway.

"No in-kernel users" is a strong argument for deleting the pre_irq entirely.

Kind regards,
Kurt

Wolfgang Grandegger

2013-11-18 20:04 UTC #7 lore raw

Quoted 47 lines · Kurt Van Dijck

Oliver,
just 2 small remarks below.
Quoted 34 lines · Oliver Hartkopp
Quoted 24 lines · Wolfgang Grandegger
Hi Oliver,
Quoted 21 lines · Oliver Hartkopp
The SJA1000 driver provides optional function pointers to handle specific hardware setups at interrupt time.
This patch fixes the issue that the sja1000_interrupt() function may have returned IRQ_NONE without processing the post_irq function. Additionally an introduced return value from the pre_irq function is checked to skip the entire interrupt handling on pre_irq function errors.
Reported-by Wolfgang Grandegger <[email protected]>
Signed-off-by Oliver Hartkopp <[email protected]>
---
drivers/net/can/sja1000/sja1000.c +0 -0
index 7164a99..bb20470 100644
@@ -494,12 +494,12 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 	uint8_t isrc, status;
 	int n = 0;
 
Quoted 6 lines
drivers/net/can/sja1000/sja1000.c irqreturn_t sja1000_interrupt(int irq, void *dev_id) · line 499
Quoted 3 lines
drivers/net/can/sja1000/sja1000.c +3 -0
+	if (priv->pre_irq && priv->pre_irq(priv))
+		goto out;
+
Well, not sure if we need that. Looks like a fatal error to me.
Quoted 7 lines
Why?
Think about a pre_irq() function that can look into hw specific registers and which can determine, that there was no interrupt for this CAN device. In this case the access to the sja1000 registers might/should be skipped depending on the new return value of pre_irq().
So what's the error here?

Quoted 8 lines

The accuracy of reading register SJA1000_IR remains unbeaten when deciding if an SJA1000 has interrupts pending.
Any pre_irq() return value is close approximation.
Quoted 1 line
Btw. by now there's no user of priv->pre_irq() anyway.
"No in-kernel users" is a strong argument for deleting the pre_irq entirely.

In general yes. But we may rename that callback to "irq_is_pending" and use it as fast path to "return IRQ_NONE". I could then be used on the peak_pci to save quite a few instructions.

Wolfgang.

Pavel Pisa

2013-11-19 00:05 UTC #8 lore raw

In reply to Kurt Van Dijck

Hello all,

Quoted 4 lines · On Monday 18 of November 2013 20:48:33 Kurt Van Dijck wrote:

Oliver,
just 2 small remarks below.

Quoted 10 lines

Quoted 2 lines · Oliver Hartkopp
On 17.11.2013 21:51, Wolfgang Grandegger wrote:
So what's the error here?
The accuracy of reading register SJA1000_IR remains unbeaten when deciding if an SJA1000 has interrupts pending.
Any pre_irq() return value is close approximation.
Quoted 1 line
Btw. by now there's no user of priv->pre_irq() anyway.

The use of some PCI to local-bus bridge register to check SJA1000 interrupt signal/pin state can be much much faster then SJA1000 IR register access for some hardware. SJA1000 has multiplexed bus and quite low maximal allowed frequency when compared to today PCI/PCIe connection (even that PCIe has quite some overhead and latencies for single register read too - it is optimized for stream/net/video data, but that is different story).

I have found some while to go through kernel interrupt handling to refresh my memory and state is as I have it in memory from years ago.

There could be present pathological situation of more edge triggered interrupt sources which do not provide way to read actual state of interrupt line behind bridge read through some interrupt controller/bridge register. (if the register can be read after interrupts handling then it can be used to start next loop through handles) But if such (considered broken) local bus is connected through local bus to PCI bridge then there are significant problems. Default PCI interrupts behavior is/expect level triggered logic. It may be possible to reconfigure given PCI interrupt to IRQ_TYPE_EDGE_RISING or FAILING type, there was even some set of kernel parameters options for that. But I expect that it is not possible to request from CAN board driver easily or at all on x86 PC architecture.

Anyway, even if IRQ_TYPE_EDGE_FAILING type of IRQ is selected then there is probably problem in

linux-devel/kernel/irq/chip.c:
handle_edge_irq()

Because it calls handle_irq_event() only once if there is present next sequence of events

- chip B signals IRQ
- handle_irq_event() is called
- chip A resturns IRQ_NONE
- chip A event arrives and A signals IRQ but line is held by B so no edge appears and IRQS_PENDING is not set
- chip B is handled stops signalling, returns IRQ_HANDLED, but IRQ line is still held by chip A

loop condition in handle_edge_irq() is false and returns even that hidden IRQ line is active still. The safe handling for such case is to call handle_irq_event() as long as it does not return IRQ_NONE (or do as many iterations through action next as one cycle with IRQ_NONE is reached).
But that such change in handle_edge_irq() or handle_irq_event_percpu() cannot be done even for testing because last call to handle_irq_event() would finish with IRQ_NONE and that triggers spurious interrupt handling

linux-devel/kernel/irq/spurious.c:
note_interrupt()

after couple of IRQs arrival. So even more changes would be required to support that corner case.

Concussion:

My understanding is that if there is no register which allows to read actual interrupt signal state behind bridge then there is no clean kernel level solution and local loop in the driver over sibling chips behind bridge with spin-locking etc. is required.

If the register to read actual wire-ored or all line states is present then option is to register IRQ chip for given PCI-localbus bridge and connect SJA1000 IRQs handling to this IRQ chip node.

I am sending copy to Thomas Gleixner because he can find most quickly if I am wrong and generic IRQ infrastructure has solution for this problem or my analysis is wrong (which would be win for solution of the initially reported problem).

Best wishes,

Pavel